From 49af46f7d9ef4da06db5574c0e5be440290277cb Mon Sep 17 00:00:00 2001 From: Cooper Gamble Date: Mon, 2 Mar 2026 14:14:29 -0800 Subject: [PATCH] Gate review-loop command behind feature flag --- codex-rs/core/config.schema.json | 6 + codex-rs/core/src/features.rs | 8 + codex-rs/core/src/review_prompts.rs | 82 ++ codex-rs/tui/src/app.rs | 39 + codex-rs/tui/src/app_event.rs | 36 + codex-rs/tui/src/bottom_pane/chat_composer.rs | 101 +-- codex-rs/tui/src/bottom_pane/command_popup.rs | 84 +- .../tui/src/bottom_pane/custom_prompt_view.rs | 111 ++- codex-rs/tui/src/bottom_pane/mod.rs | 5 + .../tui/src/bottom_pane/slash_commands.rs | 167 ++-- codex-rs/tui/src/chatwidget.rs | 771 ++++++++++++++++-- ...tests__review_loop_cap_prompt_invalid.snap | 10 + ...__tests__review_loop_cap_reached_info.snap | 6 + ...review_loop_commit_validation_failure.snap | 6 + ...et__tests__review_loop_settings_popup.snap | 12 + codex-rs/tui/src/chatwidget/tests.rs | 626 ++++++++++++++ codex-rs/tui/src/history_cell.rs | 5 + codex-rs/tui/src/slash_command.rs | 4 + 18 files changed, 1892 insertions(+), 187 deletions(-) create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_loop_cap_prompt_invalid.snap create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_loop_cap_reached_info.snap create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_loop_commit_validation_failure.snap create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_loop_settings_popup.snap diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 689538102c..09a5a3a67e 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -397,6 +397,9 @@ "responses_websockets_v2": { "type": "boolean" }, + "review_loop": { + "type": "boolean" + }, "runtime_metrics": { "type": "boolean" }, @@ -1768,6 +1771,9 @@ "responses_websockets_v2": { "type": "boolean" }, + "review_loop": { + "type": "boolean" + }, "runtime_metrics": { "type": "boolean" }, diff --git a/codex-rs/core/src/features.rs b/codex-rs/core/src/features.rs index cd8af6aa18..1fd4e31c1f 100644 --- a/codex-rs/core/src/features.rs +++ b/codex-rs/core/src/features.rs @@ -143,6 +143,8 @@ pub enum Feature { /// Enable collaboration modes (Plan, Default). /// Kept for config backward compatibility; behavior is always collaboration-modes-enabled. CollaborationModes, + /// Enable `/review-loop` command. + ReviewLoop, /// Enable personality selection in the TUI. Personality, /// Enable voice transcription in the TUI composer. @@ -660,6 +662,12 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::Stable, default_enabled: true, }, + FeatureSpec { + id: Feature::ReviewLoop, + key: "review_loop", + stage: Stage::UnderDevelopment, + default_enabled: false, + }, FeatureSpec { id: Feature::VoiceTranscription, key: "voice_transcription", diff --git a/codex-rs/core/src/review_prompts.rs b/codex-rs/core/src/review_prompts.rs index cfcb7e0b13..3ef8009632 100644 --- a/codex-rs/core/src/review_prompts.rs +++ b/codex-rs/core/src/review_prompts.rs @@ -18,6 +18,8 @@ const BASE_BRANCH_PROMPT: &str = "Review the code changes against the base branc const COMMIT_PROMPT_WITH_TITLE: &str = "Review the code changes introduced by commit {sha} (\"{title}\"). Provide prioritized, actionable findings."; const COMMIT_PROMPT: &str = "Review the code changes introduced by commit {sha}. Provide prioritized, actionable findings."; +const COMMIT_FOLLOW_UP_PROMPT_WITH_TITLE: &str = "Review the cumulative code changes from {sha}^ through HEAD, rooted at commit {sha} (\"{title}\"). Run `git diff {sha}^ HEAD` to inspect the updated stack. Provide prioritized, actionable findings."; +const COMMIT_FOLLOW_UP_PROMPT: &str = "Review the cumulative code changes from {sha}^ through HEAD, rooted at commit {sha}. Run `git diff {sha}^ HEAD` to inspect the updated stack. Provide prioritized, actionable findings."; pub fn resolve_review_request( request: ReviewRequest, @@ -67,6 +69,46 @@ pub fn review_prompt(target: &ReviewTarget, cwd: &Path) -> anyhow::Result, +) -> anyhow::Result { + let prompt = review_prompt(target, cwd)?; + Ok(append_additional_review_instructions( + prompt, + additional_instructions, + )) +} + +pub fn commit_follow_up_review_prompt( + sha: &str, + title: Option<&str>, + additional_instructions: Option<&str>, +) -> String { + let prompt = if let Some(title) = title { + COMMIT_FOLLOW_UP_PROMPT_WITH_TITLE + .replace("{sha}", sha) + .replace("{title}", title) + } else { + COMMIT_FOLLOW_UP_PROMPT.replace("{sha}", sha) + }; + append_additional_review_instructions(prompt, additional_instructions) +} + +fn append_additional_review_instructions( + prompt: String, + additional_instructions: Option<&str>, +) -> String { + let Some(additional_instructions) = additional_instructions.map(str::trim) else { + return prompt; + }; + if additional_instructions.is_empty() { + return prompt; + } + format!("{prompt}\n\nAdditional review instructions:\n{additional_instructions}") +} + pub fn user_facing_hint(target: &ReviewTarget) -> String { match target { ReviewTarget::UncommittedChanges => "current changes".to_string(), @@ -91,3 +133,43 @@ impl From for ReviewRequest { } } } + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn review_prompt_with_additional_instructions_appends_header() { + let cwd = Path::new("/tmp"); + let prompt = review_prompt_with_additional_instructions( + &ReviewTarget::UncommittedChanges, + cwd, + Some("Focus on migration safety."), + ) + .expect("prompt"); + assert_eq!( + prompt, + "Review the current code changes (staged, unstaged, and untracked files) and provide prioritized findings.\n\nAdditional review instructions:\nFocus on migration safety." + ); + } + + #[test] + fn commit_follow_up_review_prompt_references_cumulative_stack() { + let prompt = + commit_follow_up_review_prompt("abc1234", Some("Add review loop"), Some("Find bugs.")); + assert_eq!( + prompt, + "Review the cumulative code changes from abc1234^ through HEAD, rooted at commit abc1234 (\"Add review loop\"). Run `git diff abc1234^ HEAD` to inspect the updated stack. Provide prioritized, actionable findings.\n\nAdditional review instructions:\nFind bugs." + ); + } + + #[test] + fn commit_follow_up_review_prompt_omits_empty_additional_instructions() { + let prompt = commit_follow_up_review_prompt("abc1234", None, Some(" ")); + assert_eq!( + prompt, + "Review the cumulative code changes from abc1234^ through HEAD, rooted at commit abc1234. Run `git diff abc1234^ HEAD` to inspect the updated stack. Provide prioritized, actionable findings." + ); + } +} diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 742a1c9778..3c4ce3b0fc 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -3053,6 +3053,45 @@ impl App { AppEvent::OpenReviewCustomPrompt => { self.chat_widget.show_review_custom_prompt(); } + AppEvent::OpenReviewLoopSelector { + inline_instructions, + } => { + self.chat_widget.open_review_loop_popup(inline_instructions); + } + AppEvent::ReviewLoopTargetSelected { + target, + inline_instructions, + } => { + self.chat_widget + .on_review_loop_target_selected(target, inline_instructions); + } + AppEvent::OpenReviewLoopCapChooser => { + self.chat_widget.open_review_loop_cap_chooser(); + } + AppEvent::OpenReviewLoopCapPrompt => { + self.chat_widget.open_review_loop_cap_prompt(); + } + AppEvent::StartReviewLoop { max_fix_attempts } => { + self.chat_widget.start_review_loop(max_fix_attempts); + } + AppEvent::ReviewLoopInitialHeadResolved { + generation, + head_sha, + } => { + self.chat_widget + .on_review_loop_initial_head_resolved(generation, head_sha); + } + AppEvent::ReviewLoopCommitValidationResolved { + generation, + head_sha, + has_changes, + } => { + self.chat_widget.on_review_loop_commit_validation_resolved( + generation, + head_sha, + has_changes, + ); + } AppEvent::SubmitUserMessageWithMode { text, collaboration_mode, diff --git a/codex-rs/tui/src/app_event.rs b/codex-rs/tui/src/app_event.rs index e769764c91..a91b8c70b6 100644 --- a/codex-rs/tui/src/app_event.rs +++ b/codex-rs/tui/src/app_event.rs @@ -16,6 +16,7 @@ use codex_protocol::ThreadId; use codex_protocol::openai_models::ModelPreset; use codex_protocol::protocol::Event; use codex_protocol::protocol::RateLimitSnapshot; +use codex_protocol::protocol::ReviewTarget; use codex_utils_approval_presets::ApprovalPreset; use crate::bottom_pane::ApprovalRequest; @@ -407,6 +408,41 @@ pub(crate) enum AppEvent { /// Open the custom prompt option from the review popup. OpenReviewCustomPrompt, + /// Re-open the review-loop selector, preserving any inline instructions. + OpenReviewLoopSelector { + inline_instructions: Option, + }, + + /// Continue the review-loop flow after a target is selected. + ReviewLoopTargetSelected { + target: ReviewTarget, + inline_instructions: Option, + }, + + /// Open the review-loop cap chooser. + OpenReviewLoopCapChooser, + + /// Open the review-loop numeric cap prompt. + OpenReviewLoopCapPrompt, + + /// Start the configured review loop with the chosen max fix attempts. + StartReviewLoop { + max_fix_attempts: Option, + }, + + /// Async result for the initial HEAD lookup needed by commit-aware review loops. + ReviewLoopInitialHeadResolved { + generation: u64, + head_sha: Option, + }, + + /// Async result for post-fix commit validation in commit-aware review loops. + ReviewLoopCommitValidationResolved { + generation: u64, + head_sha: Option, + has_changes: Option, + }, + /// Submit a user message with an explicit collaboration mask. SubmitUserMessageWithMode { text: String, diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 5ea67d8c32..0df8ce2986 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -396,6 +396,7 @@ pub(crate) struct ChatComposer { personality_command_enabled: bool, realtime_conversation_enabled: bool, audio_device_selection_enabled: bool, + review_loop_command_enabled: bool, windows_degraded_sandbox_active: bool, status_line_value: Option>, status_line_enabled: bool, @@ -502,6 +503,7 @@ impl ChatComposer { personality_command_enabled: false, realtime_conversation_enabled: false, audio_device_selection_enabled: false, + review_loop_command_enabled: false, windows_degraded_sandbox_active: false, status_line_value: None, status_line_enabled: false, @@ -575,6 +577,10 @@ impl ChatComposer { self.personality_command_enabled = enabled; } + pub fn set_review_loop_command_enabled(&mut self, enabled: bool) { + self.review_loop_command_enabled = enabled; + } + pub fn set_realtime_conversation_enabled(&mut self, enabled: bool) { self.realtime_conversation_enabled = enabled; } @@ -618,6 +624,31 @@ impl ChatComposer { pub fn set_windows_degraded_sandbox_active(&mut self, enabled: bool) { self.windows_degraded_sandbox_active = enabled; } + + fn slash_command_filters(&self) -> slash_commands::SlashCommandFilters { + slash_commands::SlashCommandFilters { + collaboration_modes_enabled: self.collaboration_modes_enabled, + connectors_enabled: self.connectors_enabled, + personality_command_enabled: self.personality_command_enabled, + realtime_conversation_enabled: self.realtime_conversation_enabled, + audio_device_selection_enabled: self.audio_device_selection_enabled, + review_loop_command_enabled: self.review_loop_command_enabled, + allow_elevate_sandbox: !self.windows_degraded_sandbox_active, + } + } + + fn slash_command_popup_flags(&self) -> CommandPopupFlags { + CommandPopupFlags { + collaboration_modes_enabled: self.collaboration_modes_enabled, + connectors_enabled: self.connectors_enabled, + personality_command_enabled: self.personality_command_enabled, + realtime_conversation_enabled: self.realtime_conversation_enabled, + audio_device_selection_enabled: self.audio_device_selection_enabled, + review_loop_command_enabled: self.review_loop_command_enabled, + windows_degraded_sandbox_active: self.windows_degraded_sandbox_active, + } + } + fn layout_areas(&self, area: Rect) -> [Rect; 4] { let footer_props = self.footer_props(); let footer_hint_height = self @@ -2257,16 +2288,9 @@ impl ChatComposer { { let treat_as_plain_text = input_starts_with_space || name.contains('/'); if !treat_as_plain_text { - let is_builtin = slash_commands::find_builtin_command( - name, - self.collaboration_modes_enabled, - self.connectors_enabled, - self.personality_command_enabled, - self.realtime_conversation_enabled, - self.audio_device_selection_enabled, - self.windows_degraded_sandbox_active, - ) - .is_some(); + let is_builtin = + slash_commands::find_builtin_command(name, self.slash_command_filters()) + .is_some(); let prompt_prefix = format!("{PROMPTS_CMD_PREFIX}:"); let is_known_prompt = name .strip_prefix(&prompt_prefix) @@ -2474,15 +2498,8 @@ impl ChatComposer { let first_line = self.textarea.text().lines().next().unwrap_or(""); if let Some((name, rest, _rest_offset)) = parse_slash_name(first_line) && rest.is_empty() - && let Some(cmd) = slash_commands::find_builtin_command( - name, - self.collaboration_modes_enabled, - self.connectors_enabled, - self.personality_command_enabled, - self.realtime_conversation_enabled, - self.audio_device_selection_enabled, - self.windows_degraded_sandbox_active, - ) + && let Some(cmd) = + slash_commands::find_builtin_command(name, self.slash_command_filters()) { if self.reject_slash_command_if_unavailable(cmd) { return Some(InputResult::None); @@ -2510,15 +2527,7 @@ impl ChatComposer { return None; } - let cmd = slash_commands::find_builtin_command( - name, - self.collaboration_modes_enabled, - self.connectors_enabled, - self.personality_command_enabled, - self.realtime_conversation_enabled, - self.audio_device_selection_enabled, - self.windows_degraded_sandbox_active, - )?; + let cmd = slash_commands::find_builtin_command(name, self.slash_command_filters())?; if !cmd.supports_inline_args() { return None; @@ -3330,16 +3339,8 @@ impl ChatComposer { } fn is_known_slash_name(&self, name: &str) -> bool { - let is_builtin = slash_commands::find_builtin_command( - name, - self.collaboration_modes_enabled, - self.connectors_enabled, - self.personality_command_enabled, - self.realtime_conversation_enabled, - self.audio_device_selection_enabled, - self.windows_degraded_sandbox_active, - ) - .is_some(); + let is_builtin = + slash_commands::find_builtin_command(name, self.slash_command_filters()).is_some(); if is_builtin { return true; } @@ -3393,15 +3394,7 @@ impl ChatComposer { return rest_after_name.is_empty(); } - if slash_commands::has_builtin_prefix( - name, - self.collaboration_modes_enabled, - self.connectors_enabled, - self.personality_command_enabled, - self.realtime_conversation_enabled, - self.audio_device_selection_enabled, - self.windows_degraded_sandbox_active, - ) { + if slash_commands::has_builtin_prefix(name, self.slash_command_filters()) { return true; } @@ -3450,21 +3443,9 @@ impl ChatComposer { } _ => { if is_editing_slash_command_name { - let collaboration_modes_enabled = self.collaboration_modes_enabled; - let connectors_enabled = self.connectors_enabled; - let personality_command_enabled = self.personality_command_enabled; - let realtime_conversation_enabled = self.realtime_conversation_enabled; - let audio_device_selection_enabled = self.audio_device_selection_enabled; let mut command_popup = CommandPopup::new( self.custom_prompts.clone(), - CommandPopupFlags { - collaboration_modes_enabled, - connectors_enabled, - personality_command_enabled, - realtime_conversation_enabled, - audio_device_selection_enabled, - windows_degraded_sandbox_active: self.windows_degraded_sandbox_active, - }, + self.slash_command_popup_flags(), ); command_popup.on_composer_text_change(first_line.to_string()); self.active_popup = ActivePopup::Command(command_popup); diff --git a/codex-rs/tui/src/bottom_pane/command_popup.rs b/codex-rs/tui/src/bottom_pane/command_popup.rs index cf43f65686..f9c6e4ffdf 100644 --- a/codex-rs/tui/src/bottom_pane/command_popup.rs +++ b/codex-rs/tui/src/bottom_pane/command_popup.rs @@ -41,23 +41,26 @@ pub(crate) struct CommandPopupFlags { pub(crate) personality_command_enabled: bool, pub(crate) realtime_conversation_enabled: bool, pub(crate) audio_device_selection_enabled: bool, + pub(crate) review_loop_command_enabled: bool, pub(crate) windows_degraded_sandbox_active: bool, } impl CommandPopup { pub(crate) fn new(mut prompts: Vec, flags: CommandPopupFlags) -> Self { // Keep built-in availability in sync with the composer. - let builtins: Vec<(&'static str, SlashCommand)> = slash_commands::builtins_for_input( - flags.collaboration_modes_enabled, - flags.connectors_enabled, - flags.personality_command_enabled, - flags.realtime_conversation_enabled, - flags.audio_device_selection_enabled, - flags.windows_degraded_sandbox_active, - ) - .into_iter() - .filter(|(name, _)| !name.starts_with("debug")) - .collect(); + let builtins: Vec<(&'static str, SlashCommand)> = + slash_commands::builtins_for_input(slash_commands::SlashCommandFilters { + collaboration_modes_enabled: flags.collaboration_modes_enabled, + connectors_enabled: flags.connectors_enabled, + personality_command_enabled: flags.personality_command_enabled, + realtime_conversation_enabled: flags.realtime_conversation_enabled, + audio_device_selection_enabled: flags.audio_device_selection_enabled, + review_loop_command_enabled: flags.review_loop_command_enabled, + allow_elevate_sandbox: !flags.windows_degraded_sandbox_active, + }) + .into_iter() + .filter(|(name, _)| !name.starts_with("debug")) + .collect(); // Exclude prompts that collide with builtin command names and sort by name. let exclude: HashSet = builtins.iter().map(|(n, _)| (*n).to_string()).collect(); prompts.retain(|p| !exclude.contains(&p.name)); @@ -501,6 +504,7 @@ mod tests { personality_command_enabled: true, realtime_conversation_enabled: false, audio_device_selection_enabled: false, + review_loop_command_enabled: false, windows_degraded_sandbox_active: false, }, ); @@ -522,6 +526,7 @@ mod tests { personality_command_enabled: true, realtime_conversation_enabled: false, audio_device_selection_enabled: false, + review_loop_command_enabled: false, windows_degraded_sandbox_active: false, }, ); @@ -543,6 +548,7 @@ mod tests { personality_command_enabled: false, realtime_conversation_enabled: false, audio_device_selection_enabled: false, + review_loop_command_enabled: false, windows_degraded_sandbox_active: false, }, ); @@ -572,6 +578,7 @@ mod tests { personality_command_enabled: true, realtime_conversation_enabled: false, audio_device_selection_enabled: false, + review_loop_command_enabled: false, windows_degraded_sandbox_active: false, }, ); @@ -593,6 +600,7 @@ mod tests { personality_command_enabled: true, realtime_conversation_enabled: true, audio_device_selection_enabled: false, + review_loop_command_enabled: false, windows_degraded_sandbox_active: false, }, ); @@ -630,4 +638,58 @@ mod tests { "expected no /debug* command in popup menu, got {cmds:?}" ); } + + #[test] + fn review_loop_command_hidden_when_disabled() { + let mut popup = CommandPopup::new( + Vec::new(), + CommandPopupFlags { + collaboration_modes_enabled: true, + connectors_enabled: false, + personality_command_enabled: true, + realtime_conversation_enabled: false, + audio_device_selection_enabled: false, + review_loop_command_enabled: false, + windows_degraded_sandbox_active: false, + }, + ); + popup.on_composer_text_change("/review".to_string()); + + let cmds: Vec<&str> = popup + .filtered_items() + .into_iter() + .filter_map(|item| match item { + CommandItem::Builtin(cmd) => Some(cmd.command()), + CommandItem::UserPrompt(_) => None, + }) + .collect(); + assert!( + !cmds.contains(&"review-loop"), + "expected '/review-loop' to be hidden when feature is disabled, got {cmds:?}" + ); + } + + #[test] + fn review_loop_command_visible_when_enabled() { + let mut popup = CommandPopup::new( + Vec::new(), + CommandPopupFlags { + collaboration_modes_enabled: true, + connectors_enabled: false, + personality_command_enabled: true, + realtime_conversation_enabled: false, + audio_device_selection_enabled: false, + review_loop_command_enabled: true, + windows_degraded_sandbox_active: false, + }, + ); + popup.on_composer_text_change("/review-loop".to_string()); + + match popup.selected_item() { + Some(CommandItem::Builtin(cmd)) => assert_eq!(cmd.command(), "review-loop"), + other => { + panic!("expected review-loop to be selected for exact match, got {other:?}") + } + } + } } diff --git a/codex-rs/tui/src/bottom_pane/custom_prompt_view.rs b/codex-rs/tui/src/bottom_pane/custom_prompt_view.rs index e9f0ee697f..4e1b826560 100644 --- a/codex-rs/tui/src/bottom_pane/custom_prompt_view.rs +++ b/codex-rs/tui/src/bottom_pane/custom_prompt_view.rs @@ -22,7 +22,8 @@ use super::textarea::TextArea; use super::textarea::TextAreaState; /// Callback invoked when the user submits a custom prompt. -pub(crate) type PromptSubmitted = Box; +pub(crate) type PromptSubmitted = Box Result<(), String> + Send + Sync>; +pub(crate) type PromptCancelled = Option>; /// Minimal multi-line text input view to collect custom review instructions. pub(crate) struct CustomPromptView { @@ -30,10 +31,12 @@ pub(crate) struct CustomPromptView { placeholder: String, context_label: Option, on_submit: PromptSubmitted, + on_cancel: PromptCancelled, // UI state textarea: TextArea, textarea_state: RefCell, + error_message: Option, complete: bool, } @@ -43,14 +46,17 @@ impl CustomPromptView { placeholder: String, context_label: Option, on_submit: PromptSubmitted, + on_cancel: PromptCancelled, ) -> Self { Self { title, placeholder, context_label, on_submit, + on_cancel, textarea: TextArea::new(), textarea_state: RefCell::new(TextAreaState::default()), + error_message: None, complete: false, } } @@ -71,23 +77,30 @@ impl BottomPaneView for CustomPromptView { } => { let text = self.textarea.text().trim().to_string(); if !text.is_empty() { - (self.on_submit)(text); - self.complete = true; + match (self.on_submit)(text) { + Ok(()) => self.complete = true, + Err(err) => self.error_message = Some(err), + } } } KeyEvent { code: KeyCode::Enter, .. } => { + self.error_message = None; self.textarea.input(key_event); } other => { + self.error_message = None; self.textarea.input(other); } } } fn on_ctrl_c(&mut self) -> CancellationEvent { + if let Some(on_cancel) = &self.on_cancel { + on_cancel(); + } self.complete = true; CancellationEvent::Handled } @@ -100,6 +113,7 @@ impl BottomPaneView for CustomPromptView { if pasted.is_empty() { return false; } + self.error_message = None; self.textarea.insert_str(&pasted); true } @@ -107,7 +121,11 @@ impl BottomPaneView for CustomPromptView { impl Renderable for CustomPromptView { fn desired_height(&self, width: u16) -> u16 { - let extra_top: u16 = if self.context_label.is_some() { 1 } else { 0 }; + let extra_top: u16 = if self.context_label.is_some() || self.error_message.is_some() { + 1 + } else { + 0 + }; 1u16 + extra_top + self.input_height(width) + 3u16 } @@ -130,7 +148,17 @@ impl Renderable for CustomPromptView { // Optional context line let mut input_y = area.y.saturating_add(1); - if let Some(context_label) = &self.context_label { + if let Some(error_message) = &self.error_message { + let context_area = Rect { + x: area.x, + y: input_y, + width: area.width, + height: 1, + }; + let spans: Vec> = vec![gutter(), error_message.clone().red()]; + Paragraph::new(Line::from(spans)).render(context_area, buf); + input_y = input_y.saturating_add(1); + } else if let Some(context_label) = &self.context_label { let context_area = Rect { x: area.x, y: input_y, @@ -221,7 +249,11 @@ impl Renderable for CustomPromptView { if text_area_height == 0 { return None; } - let extra_offset: u16 = if self.context_label.is_some() { 1 } else { 0 }; + let extra_offset: u16 = if self.error_message.is_some() || self.context_label.is_some() { + 1 + } else { + 0 + }; let top_line_count = 1u16 + extra_offset; let textarea_rect = Rect { x: area.x.saturating_add(2), @@ -245,3 +277,70 @@ impl CustomPromptView { fn gutter() -> Span<'static> { "▌ ".cyan() } + +#[cfg(test)] +mod tests { + use super::*; + use ratatui::layout::Rect; + + #[test] + fn custom_prompt_cursor_uses_error_row_offset() { + let mut view = CustomPromptView::new( + "Set max fix attempts".to_string(), + "Type a positive integer and press Enter".to_string(), + None, + Box::new(|value: String| match value.parse::() { + Ok(value) if value > 0 => Ok(()), + Ok(_) | Err(_) => Err("Enter a positive integer.".to_string()), + }), + None, + ); + + view.handle_key_event(KeyEvent::new(KeyCode::Char('0'), KeyModifiers::NONE)); + view.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + + let prompt_area = Rect::new(0, 0, 80, view.desired_height(80)); + let (_x, y) = view + .cursor_pos(prompt_area) + .expect("cursor should be positioned when popup is active"); + assert_eq!(y, 3); + } + + #[test] + fn custom_prompt_cursor_uses_context_row_offset() { + let mut view = CustomPromptView::new( + "Custom prompt".to_string(), + "Type your input".to_string(), + Some("Optional context".to_string()), + Box::new(|_| Ok(())), + None, + ); + + view.handle_key_event(KeyEvent::new(KeyCode::Char('x'), KeyModifiers::NONE)); + + let prompt_area = Rect::new(0, 0, 80, view.desired_height(80)); + let (_x, y) = view + .cursor_pos(prompt_area) + .expect("cursor should be positioned when popup is active"); + assert_eq!(y, 3); + } + + #[test] + fn custom_prompt_cursor_uses_basic_row_offset() { + let mut view = CustomPromptView::new( + "Custom prompt".to_string(), + "Type your input".to_string(), + None, + Box::new(|_| Ok(())), + None, + ); + + view.handle_key_event(KeyEvent::new(KeyCode::Char('x'), KeyModifiers::NONE)); + + let prompt_area = Rect::new(0, 0, 80, view.desired_height(80)); + let (_x, y) = view + .cursor_pos(prompt_area) + .expect("cursor should be positioned when popup is active"); + assert_eq!(y, 2); + } +} diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index ac903d996b..d7174ae4e3 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -294,6 +294,11 @@ impl BottomPane { self.request_redraw(); } + pub fn set_review_loop_command_enabled(&mut self, enabled: bool) { + self.composer.set_review_loop_command_enabled(enabled); + self.request_redraw(); + } + pub fn set_realtime_conversation_enabled(&mut self, enabled: bool) { self.composer.set_realtime_conversation_enabled(enabled); self.request_redraw(); diff --git a/codex-rs/tui/src/bottom_pane/slash_commands.rs b/codex-rs/tui/src/bottom_pane/slash_commands.rs index 981c61c451..3cf17666d2 100644 --- a/codex-rs/tui/src/bottom_pane/slash_commands.rs +++ b/codex-rs/tui/src/bottom_pane/slash_commands.rs @@ -8,72 +8,52 @@ use codex_utils_fuzzy_match::fuzzy_match; use crate::slash_command::SlashCommand; use crate::slash_command::built_in_slash_commands; +#[derive(Clone, Copy, Debug)] +pub(crate) struct SlashCommandFilters { + pub(crate) collaboration_modes_enabled: bool, + pub(crate) connectors_enabled: bool, + pub(crate) personality_command_enabled: bool, + pub(crate) realtime_conversation_enabled: bool, + pub(crate) audio_device_selection_enabled: bool, + pub(crate) review_loop_command_enabled: bool, + pub(crate) allow_elevate_sandbox: bool, +} + /// Return the built-ins that should be visible/usable for the current input. pub(crate) fn builtins_for_input( - collaboration_modes_enabled: bool, - connectors_enabled: bool, - personality_command_enabled: bool, - realtime_conversation_enabled: bool, - audio_device_selection_enabled: bool, - allow_elevate_sandbox: bool, + filters: SlashCommandFilters, ) -> Vec<(&'static str, SlashCommand)> { built_in_slash_commands() .into_iter() - .filter(|(_, cmd)| allow_elevate_sandbox || *cmd != SlashCommand::ElevateSandbox) + .filter(|(_, cmd)| filters.allow_elevate_sandbox || *cmd != SlashCommand::ElevateSandbox) .filter(|(_, cmd)| { - collaboration_modes_enabled + filters.collaboration_modes_enabled || !matches!(*cmd, SlashCommand::Collab | SlashCommand::Plan) }) - .filter(|(_, cmd)| connectors_enabled || *cmd != SlashCommand::Apps) - .filter(|(_, cmd)| personality_command_enabled || *cmd != SlashCommand::Personality) - .filter(|(_, cmd)| realtime_conversation_enabled || *cmd != SlashCommand::Realtime) - .filter(|(_, cmd)| audio_device_selection_enabled || *cmd != SlashCommand::Settings) + .filter(|(_, cmd)| filters.connectors_enabled || *cmd != SlashCommand::Apps) + .filter(|(_, cmd)| filters.personality_command_enabled || *cmd != SlashCommand::Personality) + .filter(|(_, cmd)| filters.realtime_conversation_enabled || *cmd != SlashCommand::Realtime) + .filter(|(_, cmd)| filters.audio_device_selection_enabled || *cmd != SlashCommand::Settings) + .filter(|(_, cmd)| filters.review_loop_command_enabled || *cmd != SlashCommand::ReviewLoop) .collect() } /// Find a single built-in command by exact name, after applying the gating rules. pub(crate) fn find_builtin_command( name: &str, - collaboration_modes_enabled: bool, - connectors_enabled: bool, - personality_command_enabled: bool, - realtime_conversation_enabled: bool, - audio_device_selection_enabled: bool, - allow_elevate_sandbox: bool, + filters: SlashCommandFilters, ) -> Option { - builtins_for_input( - collaboration_modes_enabled, - connectors_enabled, - personality_command_enabled, - realtime_conversation_enabled, - audio_device_selection_enabled, - allow_elevate_sandbox, - ) - .into_iter() - .find(|(command_name, _)| *command_name == name) - .map(|(_, cmd)| cmd) + builtins_for_input(filters) + .into_iter() + .find(|(command_name, _)| *command_name == name) + .map(|(_, cmd)| cmd) } /// Whether any visible built-in fuzzily matches the provided prefix. -pub(crate) fn has_builtin_prefix( - name: &str, - collaboration_modes_enabled: bool, - connectors_enabled: bool, - personality_command_enabled: bool, - realtime_conversation_enabled: bool, - audio_device_selection_enabled: bool, - allow_elevate_sandbox: bool, -) -> bool { - builtins_for_input( - collaboration_modes_enabled, - connectors_enabled, - personality_command_enabled, - realtime_conversation_enabled, - audio_device_selection_enabled, - allow_elevate_sandbox, - ) - .into_iter() - .any(|(command_name, _)| fuzzy_match(command_name, name).is_some()) +pub(crate) fn has_builtin_prefix(name: &str, filters: SlashCommandFilters) -> bool { + builtins_for_input(filters) + .into_iter() + .any(|(command_name, _)| fuzzy_match(command_name, name).is_some()) } #[cfg(test)] @@ -83,39 +63,106 @@ mod tests { #[test] fn debug_command_still_resolves_for_dispatch() { - let cmd = find_builtin_command("debug-config", true, true, true, false, false, false); + let flags = SlashCommandFilters { + collaboration_modes_enabled: true, + connectors_enabled: true, + personality_command_enabled: true, + realtime_conversation_enabled: false, + audio_device_selection_enabled: false, + review_loop_command_enabled: false, + allow_elevate_sandbox: false, + }; + let cmd = find_builtin_command("debug-config", flags); assert_eq!(cmd, Some(SlashCommand::DebugConfig)); } #[test] fn clear_command_resolves_for_dispatch() { + let flags = SlashCommandFilters { + collaboration_modes_enabled: true, + connectors_enabled: true, + personality_command_enabled: true, + realtime_conversation_enabled: false, + audio_device_selection_enabled: false, + review_loop_command_enabled: false, + allow_elevate_sandbox: false, + }; assert_eq!( - find_builtin_command("clear", true, true, true, false, false, false), + find_builtin_command("clear", flags), Some(SlashCommand::Clear) ); } #[test] fn realtime_command_is_hidden_when_realtime_is_disabled() { - assert_eq!( - find_builtin_command("realtime", true, true, true, false, true, false), - None - ); + let flags = SlashCommandFilters { + collaboration_modes_enabled: true, + connectors_enabled: true, + personality_command_enabled: true, + realtime_conversation_enabled: false, + audio_device_selection_enabled: true, + review_loop_command_enabled: false, + allow_elevate_sandbox: false, + }; + assert_eq!(find_builtin_command("realtime", flags), None); } #[test] fn settings_command_is_hidden_when_realtime_is_disabled() { - assert_eq!( - find_builtin_command("settings", true, true, true, false, false, false), - None - ); + let flags = SlashCommandFilters { + collaboration_modes_enabled: true, + connectors_enabled: true, + personality_command_enabled: true, + realtime_conversation_enabled: false, + audio_device_selection_enabled: false, + review_loop_command_enabled: false, + allow_elevate_sandbox: false, + }; + assert_eq!(find_builtin_command("settings", flags), None); } #[test] fn settings_command_is_hidden_when_audio_device_selection_is_disabled() { + let flags = SlashCommandFilters { + collaboration_modes_enabled: true, + connectors_enabled: true, + personality_command_enabled: true, + realtime_conversation_enabled: true, + audio_device_selection_enabled: false, + review_loop_command_enabled: false, + allow_elevate_sandbox: false, + }; + assert_eq!(find_builtin_command("settings", flags), None); + } + + #[test] + fn review_loop_command_is_hidden_when_disabled() { + let flags = SlashCommandFilters { + collaboration_modes_enabled: true, + connectors_enabled: true, + personality_command_enabled: true, + realtime_conversation_enabled: false, + audio_device_selection_enabled: false, + review_loop_command_enabled: false, + allow_elevate_sandbox: false, + }; + assert_eq!(find_builtin_command("review-loop", flags), None); + } + + #[test] + fn review_loop_command_is_visible_when_enabled() { + let flags = SlashCommandFilters { + collaboration_modes_enabled: true, + connectors_enabled: true, + personality_command_enabled: true, + realtime_conversation_enabled: false, + audio_device_selection_enabled: false, + review_loop_command_enabled: true, + allow_elevate_sandbox: false, + }; assert_eq!( - find_builtin_command("settings", true, true, true, true, false, false), - None + find_builtin_command("review-loop", flags), + Some(SlashCommand::ReviewLoop) ); } } diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 78e8fcc669..22a4021234 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -62,11 +62,14 @@ use codex_core::features::Feature; use codex_core::find_thread_name_by_id; use codex_core::git_info::current_branch_name; use codex_core::git_info::get_git_repo_root; +use codex_core::git_info::get_has_changes; +use codex_core::git_info::get_head_commit_hash; use codex_core::git_info::local_git_branches; use codex_core::mcp::McpManager; use codex_core::models_manager::manager::ModelsManager; use codex_core::plugins::PluginsManager; use codex_core::project_doc::DEFAULT_PROJECT_DOC_FILENAME; +use codex_core::review_format::render_review_output_text; use codex_core::skills::model::SkillMetadata; use codex_core::terminal::TerminalName; use codex_core::terminal::terminal_info; @@ -119,6 +122,7 @@ use codex_protocol::protocol::McpToolCallEndEvent; use codex_protocol::protocol::Op; use codex_protocol::protocol::PatchApplyBeginEvent; use codex_protocol::protocol::RateLimitSnapshot; +use codex_protocol::protocol::ReviewOutputEvent; use codex_protocol::protocol::ReviewRequest; use codex_protocol::protocol::ReviewTarget; use codex_protocol::protocol::SkillMetadata as ProtocolSkillMetadata; @@ -513,6 +517,49 @@ pub(crate) enum ExternalEditorState { Active, } +#[derive(Clone, Debug, Default, PartialEq, Eq)] +enum ReviewSelectorMode { + #[default] + SingleReview, + ReviewLoop { + inline_instructions: Option, + }, +} + +#[derive(Clone, Debug, PartialEq)] +struct ReviewLoopDraft { + target: ReviewTarget, + selector_inline_instructions: Option, + extra_reviewer_instructions: Option, +} + +#[derive(Clone, Debug, PartialEq)] +struct ReviewLoopSpec { + target: ReviewTarget, + extra_reviewer_instructions: Option, + max_fix_attempts: Option, + requires_commit: bool, +} + +#[derive(Clone, Debug, PartialEq, Eq)] +enum ReviewLoopStage { + Preparing, + Reviewing, + AwaitingFixTurn, + Fixing, + AwaitingCommitValidation, +} + +#[derive(Clone, Debug, PartialEq)] +struct ReviewLoopState { + generation: u64, + spec: ReviewLoopSpec, + stage: ReviewLoopStage, + fix_attempts_started: u32, + baseline_head_sha: Option, + pending_review_output: Option, +} + /// Maintains the per-session UI state and interaction state machines for the chat screen. /// /// `ChatWidget` owns the state derived from the protocol event stream (history cells, streaming @@ -630,6 +677,9 @@ pub(crate) struct ChatWidget { quit_shortcut_key: Option, // Simple review mode flag; used to adjust layout and banners. is_review_mode: bool, + review_selector_mode: ReviewSelectorMode, + pending_review_loop_draft: Option, + review_loop_state: Option, // Snapshot of token usage to restore after review mode exits. pre_review_token_info: Option>, // Whether the next streamed assistant content should be preceded by a final message separator. @@ -1477,7 +1527,12 @@ impl ChatWidget { self.unified_exec_wait_streak = None; self.request_redraw(); - if !from_replay && self.queued_user_messages.is_empty() { + let review_loop_consumed_turn = if from_replay { + false + } else { + self.handle_review_loop_turn_complete() + }; + if !from_replay && !review_loop_consumed_turn && self.queued_user_messages.is_empty() { self.maybe_prompt_plan_implementation(); } // Keep this flag for replayed completion events so a subsequent live TurnComplete can @@ -1486,7 +1541,9 @@ impl ChatWidget { self.saw_plan_item_this_turn = false; } // If there is a queued user message, send exactly one now to begin the next turn. - self.maybe_send_next_queued_input(); + if !review_loop_consumed_turn { + self.maybe_send_next_queued_input(); + } // Emit a notification when the turn completes (suppressed if focused). self.notify(Notification::AgentTurnComplete { response: last_agent_message.unwrap_or_default(), @@ -1720,6 +1777,7 @@ impl ChatWidget { } fn on_server_overloaded_error(&mut self, message: String) { + self.clear_review_loop_state(); self.finalize_turn(); let message = if message.trim().is_empty() { @@ -1734,6 +1792,7 @@ impl ChatWidget { } fn on_error(&mut self, message: String) { + self.clear_review_loop_state(); self.finalize_turn(); self.add_to_history(history_cell::new_error_event(message)); self.request_redraw(); @@ -1819,6 +1878,7 @@ impl ChatWidget { /// When there are queued user messages, restore them into the composer /// separated by newlines rather than auto‑submitting the next one. fn on_interrupted_turn(&mut self, reason: TurnAbortReason) { + self.clear_review_loop_state(); // Finalize, log a gentle prompt, and clear running state. self.finalize_turn(); if reason == TurnAbortReason::Interrupted { @@ -2882,6 +2942,9 @@ impl ChatWidget { quit_shortcut_expires_at: None, quit_shortcut_key: None, is_review_mode: false, + review_selector_mode: ReviewSelectorMode::SingleReview, + pending_review_loop_draft: None, + review_loop_state: None, pre_review_token_info: None, needs_final_message_separator: false, had_work_activity: false, @@ -2922,6 +2985,7 @@ impl ChatWidget { .set_status_line_enabled(!widget.configured_status_line_items().is_empty()); widget.bottom_pane.set_collaboration_modes_enabled(true); widget.sync_personality_command_enabled(); + widget.sync_review_loop_command_enabled(); widget .bottom_pane .set_queued_message_edit_binding(widget.queued_message_edit_binding); @@ -3065,6 +3129,9 @@ impl ChatWidget { quit_shortcut_expires_at: None, quit_shortcut_key: None, is_review_mode: false, + review_selector_mode: ReviewSelectorMode::SingleReview, + pending_review_loop_draft: None, + review_loop_state: None, pre_review_token_info: None, needs_final_message_separator: false, had_work_activity: false, @@ -3101,6 +3168,7 @@ impl ChatWidget { .set_status_line_enabled(!widget.configured_status_line_items().is_empty()); widget.bottom_pane.set_collaboration_modes_enabled(true); widget.sync_personality_command_enabled(); + widget.sync_review_loop_command_enabled(); widget .bottom_pane .set_queued_message_edit_binding(widget.queued_message_edit_binding); @@ -3229,6 +3297,9 @@ impl ChatWidget { quit_shortcut_expires_at: None, quit_shortcut_key: None, is_review_mode: false, + review_selector_mode: ReviewSelectorMode::SingleReview, + pending_review_loop_draft: None, + review_loop_state: None, pre_review_token_info: None, needs_final_message_separator: false, had_work_activity: false, @@ -3269,6 +3340,7 @@ impl ChatWidget { .set_status_line_enabled(!widget.configured_status_line_items().is_empty()); widget.bottom_pane.set_collaboration_modes_enabled(true); widget.sync_personality_command_enabled(); + widget.sync_review_loop_command_enabled(); widget .bottom_pane .set_queued_message_edit_binding(widget.queued_message_edit_binding); @@ -3396,7 +3468,8 @@ impl ChatWidget { // `maybe_send_next_queued_input()`, so no typed prompt is dropped. let should_submit_now = self.is_session_configured() && !self.is_plan_streaming_in_tui() - && self.stream_controller.is_none(); + && self.stream_controller.is_none() + && self.review_loop_state.is_none(); if should_submit_now { // Submitted is emitted when user submits. // Reset any reasoning header only when we are actually submitting a turn. @@ -3504,7 +3577,12 @@ impl ChatWidget { } fn dispatch_command(&mut self, cmd: SlashCommand) { - if !cmd.available_during_task() && self.bottom_pane.is_task_running() { + if cmd == SlashCommand::ReviewLoop && self.review_loop_command_not_enabled() { + return; + } + if !cmd.available_during_task() + && (self.bottom_pane.is_task_running() || self.review_loop_state.is_some()) + { let message = format!( "'/{}' is disabled while a task is in progress.", cmd.command() @@ -3559,6 +3637,9 @@ impl ChatWidget { SlashCommand::Review => { self.open_review_popup(); } + SlashCommand::ReviewLoop => { + self.open_review_loop_popup(None); + } SlashCommand::Rename => { self.otel_manager.counter("codex.thread.rename", 1, &[]); self.show_rename_prompt(); @@ -3829,11 +3910,16 @@ impl ChatWidget { args: String, _text_elements: Vec, ) { + if cmd == SlashCommand::ReviewLoop && self.review_loop_command_not_enabled() { + return; + } if !cmd.supports_inline_args() { self.dispatch_command(cmd); return; } - if !cmd.available_during_task() && self.bottom_pane.is_task_running() { + if !cmd.available_during_task() + && (self.bottom_pane.is_task_running() || self.review_loop_state.is_some()) + { let message = format!( "'/{}' is disabled while a task is in progress.", cmd.command() @@ -3909,6 +3995,15 @@ impl ChatWidget { }); self.bottom_pane.drain_pending_submission_state(); } + SlashCommand::ReviewLoop if !trimmed.is_empty() => { + let Some((prepared_args, _prepared_elements)) = + self.bottom_pane.prepare_inline_args_submission(false) + else { + return; + }; + self.open_review_loop_popup(Some(prepared_args)); + self.bottom_pane.drain_pending_submission_state(); + } SlashCommand::SandboxReadRoot if !trimmed.is_empty() => { let Some((prepared_args, _prepared_elements)) = self.bottom_pane.prepare_inline_args_submission(false) @@ -3925,6 +4020,20 @@ impl ChatWidget { } } + fn review_loop_command_not_enabled(&mut self) -> bool { + if self.config.features.enabled(Feature::ReviewLoop) { + return false; + } + + self.add_info_message( + "Review loop is disabled.".to_string(), + Some("Enable the `review_loop` feature to use /review-loop.".to_string()), + ); + self.bottom_pane.drain_pending_submission_state(); + self.request_redraw(); + true + } + fn show_rename_prompt(&mut self) { let tx = self.app_event_tx.clone(); let has_name = self @@ -3943,15 +4052,14 @@ impl ChatWidget { None, Box::new(move |name: String| { let Some(name) = codex_core::util::normalize_thread_name(&name) else { - tx.send(AppEvent::InsertHistoryCell(Box::new( - history_cell::new_error_event("Thread name cannot be empty.".to_string()), - ))); - return; + return Err("Thread name cannot be empty.".to_string()); }; let cell = Self::rename_confirmation_cell(&name, thread_id); tx.send(AppEvent::InsertHistoryCell(Box::new(cell))); tx.send(AppEvent::CodexOp(Op::SetThreadName { name })); + Ok(()) }), + None, ); self.bottom_pane.show_view(Box::new(view)); @@ -4011,6 +4119,7 @@ impl ChatWidget { if !self.is_session_configured() || self.bottom_pane.is_task_running() || self.is_review_mode + || self.review_loop_state.is_some() { self.queued_user_messages.push_back(user_message); self.refresh_queued_user_messages(); @@ -4020,6 +4129,14 @@ impl ChatWidget { } fn submit_user_message(&mut self, user_message: UserMessage) { + self.submit_user_message_with_collaboration_mode(user_message, None) + } + + fn submit_user_message_with_collaboration_mode( + &mut self, + user_message: UserMessage, + collaboration_mode_override: Option, + ) { if !self.is_session_configured() { tracing::warn!("cannot submit user message before session is configured; queueing"); self.queued_user_messages.push_front(user_message); @@ -4171,13 +4288,14 @@ impl ChatWidget { } let effective_mode = self.effective_collaboration_mode(); - let collaboration_mode = if self.collaboration_modes_enabled() { - self.active_collaboration_mask - .as_ref() - .map(|_| effective_mode.clone()) - } else { - None - }; + let collaboration_mode = collaboration_mode_override.or_else(|| { + self.collaboration_modes_enabled().then(|| { + self.active_collaboration_mask.as_ref().map_or_else( + || effective_mode.clone(), + |mask| effective_mode.apply_mask(mask), + ) + }) + }); let personality = self .config .personality @@ -4579,7 +4697,47 @@ impl ChatWidget { .send(AppEvent::InsertHistoryCell(Box::new(body_cell))); } } + if self.review_loop_state.is_some() { + if output.findings.is_empty() + && output.overall_correctness.trim().is_empty() + && !output.overall_explanation.trim().is_empty() + { + self.clear_review_loop_state(); + self.add_error_message( + "Review loop stopped because the review turn did not produce structured output." + .to_string(), + ); + } else if output.findings.is_empty() { + self.clear_review_loop_state(); + } else { + let stop_message = if let Some(state) = self.review_loop_state.as_mut() { + if let Some(max_fix_attempts) = state.spec.max_fix_attempts + && state.fix_attempts_started >= max_fix_attempts + { + Some(format!( + "Review loop stopped after reaching the limit of {max_fix_attempts} fix attempt{}.", + if max_fix_attempts == 1 { "" } else { "s" } + )) + } else { + state.pending_review_output = Some(output); + state.stage = ReviewLoopStage::AwaitingFixTurn; + None + } + } else { + None + }; + if let Some(message) = stop_message { + self.clear_review_loop_state(); + self.add_info_message(message, None); + } + } + } // Final message is rendered as part of the AgentMessage. + } else if self.review_loop_state.is_some() { + self.clear_review_loop_state(); + self.add_error_message( + "Review loop stopped because the review turn did not finish cleanly.".to_string(), + ); } self.is_review_mode = false; @@ -4687,6 +4845,106 @@ impl ChatWidget { self.bottom_pane.set_queued_user_messages(messages); } + fn handle_review_loop_turn_complete(&mut self) -> bool { + enum Action { + Stop(String), + SubmitFix(String), + ValidateCommit { generation: u64, cwd: PathBuf }, + ReviewAgain, + Hold, + None, + } + + let cwd = self.config.cwd.clone(); + let action = { + let Some(state) = self.review_loop_state.as_mut() else { + return false; + }; + match state.stage { + ReviewLoopStage::AwaitingFixTurn => match state.pending_review_output.take() { + Some(review_output) => { + state.fix_attempts_started += 1; + state.stage = ReviewLoopStage::Fixing; + let requires_commit = state.spec.requires_commit; + let mut prompt = "Please implement fixes for these findings and rerun the relevant test suites to ensure they are passing.".to_string(); + let findings_text = render_review_output_text(&review_output); + if !findings_text.trim().is_empty() { + prompt.push_str("\n\n"); + prompt.push_str(&findings_text); + } + if requires_commit { + prompt.push_str( + "\n\nBefore finishing, commit the fixes in a new git commit so the next review pass includes them.", + ); + } + Action::SubmitFix(prompt) + } + None => Action::Stop( + "Review loop stopped because the review findings were not available." + .to_string(), + ), + }, + ReviewLoopStage::Fixing if state.spec.requires_commit => { + state.stage = ReviewLoopStage::AwaitingCommitValidation; + Action::ValidateCommit { + generation: state.generation, + cwd, + } + } + ReviewLoopStage::Fixing => { + state.stage = ReviewLoopStage::Reviewing; + Action::ReviewAgain + } + ReviewLoopStage::AwaitingCommitValidation => Action::Hold, + ReviewLoopStage::Preparing | ReviewLoopStage::Reviewing => Action::None, + } + }; + + match action { + Action::Stop(message) => { + self.clear_review_loop_state(); + self.add_error_message(message); + false + } + Action::SubmitFix(prompt) => { + self.reasoning_buffer.clear(); + self.full_reasoning_buffer.clear(); + self.set_status_header(String::from("Working")); + let default_collaboration_mode = + collaboration_modes::default_mask(self.models_manager.as_ref()) + .map(|mask| self.current_collaboration_mode.apply_mask(&mask)); + self.submit_user_message_with_collaboration_mode( + prompt.into(), + default_collaboration_mode, + ); + true + } + Action::ValidateCommit { generation, cwd } => { + let tx = self.app_event_tx.clone(); + tokio::spawn(async move { + let head_sha = get_head_commit_hash(&cwd).await; + let has_changes = get_has_changes(&cwd).await; + tx.send(AppEvent::ReviewLoopCommitValidationResolved { + generation, + head_sha, + has_changes, + }); + }); + true + } + Action::ReviewAgain => { + if let Err(err) = self.submit_review_loop_review() { + self.clear_review_loop_state(); + self.add_error_message(err); + return false; + } + true + } + Action::Hold => true, + Action::None => false, + } + } + pub(crate) fn set_pending_thread_approvals(&mut self, threads: Vec) { self.bottom_pane.set_pending_thread_approvals(threads); } @@ -6741,6 +6999,9 @@ impl ChatWidget { if feature == Feature::Personality { self.sync_personality_command_enabled(); } + if feature == Feature::ReviewLoop { + self.sync_review_loop_command_enabled(); + } if feature == Feature::PreventIdleSleep { self.turn_sleep_inhibitor = SleepInhibitor::new(enabled); self.turn_sleep_inhibitor @@ -6880,6 +7141,11 @@ impl ChatWidget { .set_personality_command_enabled(self.config.features.enabled(Feature::Personality)); } + fn sync_review_loop_command_enabled(&mut self) { + self.bottom_pane + .set_review_loop_command_enabled(self.config.features.enabled(Feature::ReviewLoop)); + } + fn current_model_supports_personality(&self) -> bool { let model = self.current_model(); self.models_manager @@ -7509,7 +7775,7 @@ impl ChatWidget { return; } self.set_collaboration_mask(collaboration_mode); - let should_queue = self.is_plan_streaming_in_tui(); + let should_queue = self.is_plan_streaming_in_tui() || self.review_loop_state.is_some(); let user_message = UserMessage { text, local_images: Vec::new(), @@ -7693,6 +7959,21 @@ impl ChatWidget { } pub(crate) fn open_review_popup(&mut self) { + self.review_selector_mode = ReviewSelectorMode::SingleReview; + self.pending_review_loop_draft = None; + self.show_review_selector(); + } + + pub(crate) fn open_review_loop_popup(&mut self, inline_instructions: Option) { + self.review_selector_mode = ReviewSelectorMode::ReviewLoop { + inline_instructions, + }; + self.pending_review_loop_draft = None; + self.show_review_selector(); + } + + fn show_review_selector(&mut self) { + let selector_mode = self.review_selector_mode.clone(); let mut items: Vec = Vec::new(); items.push(SelectionItem { @@ -7708,21 +7989,33 @@ impl ChatWidget { ..Default::default() }); + let selector_mode_for_uncommitted = selector_mode.clone(); items.push(SelectionItem { name: "Review uncommitted changes".to_string(), - actions: vec![Box::new(move |tx: &AppEventSender| { - tx.send(AppEvent::CodexOp(Op::Review { - review_request: ReviewRequest { - target: ReviewTarget::UncommittedChanges, - user_facing_hint: None, - }, - })); - })], + actions: vec![Box::new( + move |tx: &AppEventSender| match &selector_mode_for_uncommitted { + ReviewSelectorMode::SingleReview => { + tx.send(AppEvent::CodexOp(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::UncommittedChanges, + user_facing_hint: None, + }, + })); + } + ReviewSelectorMode::ReviewLoop { + inline_instructions, + } => { + tx.send(AppEvent::ReviewLoopTargetSelected { + target: ReviewTarget::UncommittedChanges, + inline_instructions: inline_instructions.clone(), + }); + } + }, + )], dismiss_on_select: true, ..Default::default() }); - // New: Review a specific commit (opens commit picker) items.push(SelectionItem { name: "Review a commit".to_string(), actions: vec![Box::new({ @@ -7735,12 +8028,36 @@ impl ChatWidget { ..Default::default() }); + let selector_mode_for_custom = selector_mode; + let custom_dismiss_on_select = matches!( + selector_mode_for_custom, + ReviewSelectorMode::ReviewLoop { + inline_instructions: Some(_) + } + ); items.push(SelectionItem { name: "Custom review instructions".to_string(), - actions: vec![Box::new(move |tx| { - tx.send(AppEvent::OpenReviewCustomPrompt); + actions: vec![Box::new(move |tx| match &selector_mode_for_custom { + ReviewSelectorMode::SingleReview => { + tx.send(AppEvent::OpenReviewCustomPrompt); + } + ReviewSelectorMode::ReviewLoop { + inline_instructions: Some(inline_instructions), + } => { + tx.send(AppEvent::ReviewLoopTargetSelected { + target: ReviewTarget::Custom { + instructions: inline_instructions.clone(), + }, + inline_instructions: Some(inline_instructions.clone()), + }); + } + ReviewSelectorMode::ReviewLoop { + inline_instructions: None, + } => { + tx.send(AppEvent::OpenReviewCustomPrompt); + } })], - dismiss_on_select: false, + dismiss_on_select: custom_dismiss_on_select, ..Default::default() }); @@ -7753,6 +8070,7 @@ impl ChatWidget { } pub(crate) async fn show_review_branch_picker(&mut self, cwd: &Path) { + let selector_mode = self.review_selector_mode.clone(); let branches = local_git_branches(cwd).await; let current_branch = current_branch_name(cwd) .await @@ -7761,17 +8079,30 @@ impl ChatWidget { for option in branches { let branch = option.clone(); + let selector_mode = selector_mode.clone(); items.push(SelectionItem { name: format!("{current_branch} -> {branch}"), - actions: vec![Box::new(move |tx3: &AppEventSender| { - tx3.send(AppEvent::CodexOp(Op::Review { - review_request: ReviewRequest { + actions: vec![Box::new(move |tx3: &AppEventSender| match &selector_mode { + ReviewSelectorMode::SingleReview => { + tx3.send(AppEvent::CodexOp(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::BaseBranch { + branch: branch.clone(), + }, + user_facing_hint: None, + }, + })); + } + ReviewSelectorMode::ReviewLoop { + inline_instructions, + } => { + tx3.send(AppEvent::ReviewLoopTargetSelected { target: ReviewTarget::BaseBranch { branch: branch.clone(), }, - user_facing_hint: None, - }, - })); + inline_instructions: inline_instructions.clone(), + }); + } })], dismiss_on_select: true, search_value: Some(option), @@ -7790,6 +8121,7 @@ impl ChatWidget { } pub(crate) async fn show_review_commit_picker(&mut self, cwd: &Path) { + let selector_mode = self.review_selector_mode.clone(); let commits = codex_core::git_info::recent_commits(cwd, 100).await; let mut items: Vec = Vec::with_capacity(commits.len()); @@ -7797,19 +8129,33 @@ impl ChatWidget { let subject = entry.subject.clone(); let sha = entry.sha.clone(); let search_val = format!("{subject} {sha}"); + let selector_mode = selector_mode.clone(); items.push(SelectionItem { name: subject.clone(), - actions: vec![Box::new(move |tx3: &AppEventSender| { - tx3.send(AppEvent::CodexOp(Op::Review { - review_request: ReviewRequest { + actions: vec![Box::new(move |tx3: &AppEventSender| match &selector_mode { + ReviewSelectorMode::SingleReview => { + tx3.send(AppEvent::CodexOp(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::Commit { + sha: sha.clone(), + title: Some(subject.clone()), + }, + user_facing_hint: None, + }, + })); + } + ReviewSelectorMode::ReviewLoop { + inline_instructions, + } => { + tx3.send(AppEvent::ReviewLoopTargetSelected { target: ReviewTarget::Commit { sha: sha.clone(), title: Some(subject.clone()), }, - user_facing_hint: None, - }, - })); + inline_instructions: inline_instructions.clone(), + }); + } })], dismiss_on_select: true, search_value: Some(search_val), @@ -7828,6 +8174,7 @@ impl ChatWidget { } pub(crate) fn show_review_custom_prompt(&mut self) { + let selector_mode = self.review_selector_mode.clone(); let tx = self.app_event_tx.clone(); let view = CustomPromptView::new( "Custom review instructions".to_string(), @@ -7836,21 +8183,345 @@ impl ChatWidget { Box::new(move |prompt: String| { let trimmed = prompt.trim().to_string(); if trimmed.is_empty() { - return; + return Err("Review prompt cannot be empty.".to_string()); } - tx.send(AppEvent::CodexOp(Op::Review { - review_request: ReviewRequest { - target: ReviewTarget::Custom { - instructions: trimmed, - }, - user_facing_hint: None, - }, - })); + match &selector_mode { + ReviewSelectorMode::SingleReview => { + tx.send(AppEvent::CodexOp(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::Custom { + instructions: trimmed, + }, + user_facing_hint: None, + }, + })); + } + ReviewSelectorMode::ReviewLoop { + inline_instructions, + } => { + tx.send(AppEvent::ReviewLoopTargetSelected { + target: ReviewTarget::Custom { + instructions: trimmed, + }, + inline_instructions: inline_instructions.clone(), + }); + } + } + Ok(()) }), + None, ); self.bottom_pane.show_view(Box::new(view)); } + pub(crate) fn on_review_loop_target_selected( + &mut self, + target: ReviewTarget, + inline_instructions: Option, + ) { + let extra_reviewer_instructions = if matches!(target, ReviewTarget::Custom { .. }) { + None + } else { + inline_instructions + .as_deref() + .map(str::trim) + .filter(|instructions| !instructions.is_empty()) + .map(str::to_string) + }; + self.review_selector_mode = ReviewSelectorMode::SingleReview; + self.pending_review_loop_draft = Some(ReviewLoopDraft { + target, + selector_inline_instructions: inline_instructions, + extra_reviewer_instructions, + }); + self.open_review_loop_cap_chooser(); + } + + pub(crate) fn open_review_loop_cap_chooser(&mut self) { + let Some(draft) = self.pending_review_loop_draft.clone() else { + self.add_error_message("Review loop settings are no longer available.".to_string()); + return; + }; + let mut items = Vec::new(); + items.push(SelectionItem { + name: "Run until clean".to_string(), + description: Some("Repeat until the reviewer returns no findings.".to_string()), + actions: vec![Box::new(move |tx| { + tx.send(AppEvent::StartReviewLoop { + max_fix_attempts: None, + }); + })], + dismiss_on_select: true, + ..Default::default() + }); + items.push(SelectionItem { + name: "Set max fix attempts".to_string(), + description: Some("Stop after a fixed number of fix/review iterations.".to_string()), + actions: vec![Box::new(move |tx| { + tx.send(AppEvent::OpenReviewLoopCapPrompt); + })], + dismiss_on_select: false, + ..Default::default() + }); + let inline_instructions = draft.selector_inline_instructions; + self.bottom_pane.show_selection_view(SelectionViewParams { + title: Some("Review loop settings".to_string()), + footer_hint: Some(standard_popup_hint_line()), + items, + on_cancel: Some(Box::new(move |tx| { + tx.send(AppEvent::OpenReviewLoopSelector { + inline_instructions: inline_instructions.clone(), + }); + })), + ..Default::default() + }); + } + + pub(crate) fn open_review_loop_cap_prompt(&mut self) { + let tx = self.app_event_tx.clone(); + let cancel_tx = self.app_event_tx.clone(); + let view = CustomPromptView::new( + "Set max fix attempts".to_string(), + "Type a positive integer and press Enter".to_string(), + Some("Review loop will stop after this many fix attempts.".to_string()), + Box::new(move |value: String| { + let Ok(max_fix_attempts) = value.parse::() else { + return Err("Enter a positive integer.".to_string()); + }; + if max_fix_attempts == 0 { + return Err("Enter a positive integer.".to_string()); + } + tx.send(AppEvent::StartReviewLoop { + max_fix_attempts: Some(max_fix_attempts), + }); + Ok(()) + }), + Some(Box::new(move || { + cancel_tx.send(AppEvent::OpenReviewLoopCapChooser); + })), + ); + self.bottom_pane.show_view(Box::new(view)); + } + + pub(crate) fn start_review_loop(&mut self, max_fix_attempts: Option) { + let Some(draft) = self.pending_review_loop_draft.take() else { + self.add_error_message("Review loop target is no longer available.".to_string()); + return; + }; + self.review_selector_mode = ReviewSelectorMode::SingleReview; + let requires_commit = matches!( + draft.target, + ReviewTarget::BaseBranch { .. } | ReviewTarget::Commit { .. } + ); + let mut rng = rand::rng(); + let generation = rng.random(); + self.review_loop_state = Some(ReviewLoopState { + generation, + spec: ReviewLoopSpec { + target: draft.target, + extra_reviewer_instructions: draft.extra_reviewer_instructions, + max_fix_attempts, + requires_commit, + }, + stage: if requires_commit { + ReviewLoopStage::Preparing + } else { + ReviewLoopStage::Reviewing + }, + fix_attempts_started: 0, + baseline_head_sha: None, + pending_review_output: None, + }); + if requires_commit { + let tx = self.app_event_tx.clone(); + let cwd = self.config.cwd.clone(); + tokio::spawn(async move { + let head_sha = get_head_commit_hash(&cwd).await; + tx.send(AppEvent::ReviewLoopInitialHeadResolved { + generation, + head_sha, + }); + }); + } else if let Err(err) = self.submit_review_loop_review() { + self.clear_review_loop_state(); + self.add_error_message(err); + } + } + + pub(crate) fn on_review_loop_initial_head_resolved( + &mut self, + generation: u64, + head_sha: Option, + ) { + let matches_generation = matches!( + self.review_loop_state.as_ref(), + Some(state) + if state.generation == generation && state.stage == ReviewLoopStage::Preparing + ); + if !matches_generation { + return; + } + let Some(head_sha) = head_sha else { + self.clear_review_loop_state(); + self.add_error_message( + "Review loop could not resolve HEAD for this repository.".to_string(), + ); + self.maybe_send_next_queued_input(); + return; + }; + let Some(state) = self.review_loop_state.as_mut() else { + return; + }; + state.baseline_head_sha = Some(head_sha); + state.stage = ReviewLoopStage::Reviewing; + if let Err(err) = self.submit_review_loop_review() { + self.clear_review_loop_state(); + self.add_error_message(err); + self.maybe_send_next_queued_input(); + } + } + + pub(crate) fn on_review_loop_commit_validation_resolved( + &mut self, + generation: u64, + head_sha: Option, + has_changes: Option, + ) { + let action = { + let Some(state) = self.review_loop_state.as_mut() else { + return; + }; + if state.generation != generation + || state.stage != ReviewLoopStage::AwaitingCommitValidation + { + return; + } + if matches!( + (&state.baseline_head_sha, &head_sha), + (Some(previous_head), Some(current_head)) if previous_head != current_head + ) { + match has_changes { + Some(false) => { + state.baseline_head_sha = head_sha; + state.stage = ReviewLoopStage::Reviewing; + None + } + Some(true) => Some( + "Review loop stopped because the fix turn created a new commit but left uncommitted changes." + .to_string(), + ), + None => Some( + "Review loop stopped because commit validation could not confirm a new commit." + .to_string(), + ), + } + } else { + Some(match has_changes { + Some(true) => { + "Review loop stopped because the fix turn ended with uncommitted changes and no new commit." + .to_string() + } + Some(false) => { + "Review loop stopped because the fix turn did not create a new commit." + .to_string() + } + None => { + "Review loop stopped because commit validation could not confirm a new commit." + .to_string() + } + }) + } + }; + if let Some(message) = action { + self.clear_review_loop_state(); + self.add_error_message(message); + self.maybe_send_next_queued_input(); + return; + } + if let Err(err) = self.submit_review_loop_review() { + self.clear_review_loop_state(); + self.add_error_message(err); + self.maybe_send_next_queued_input(); + } + } + + fn clear_review_loop_state(&mut self) { + self.review_selector_mode = ReviewSelectorMode::SingleReview; + self.pending_review_loop_draft = None; + self.review_loop_state = None; + } + + fn submit_review_loop_review(&mut self) -> Result<(), String> { + let Some(state) = self.review_loop_state.as_ref() else { + return Err("Review loop is no longer active.".to_string()); + }; + let extra_instructions = state.spec.extra_reviewer_instructions.as_deref(); + let is_commit_follow_up = state.fix_attempts_started > 0 + && matches!(state.spec.target, ReviewTarget::Commit { .. }); + let request = match &state.spec.target { + ReviewTarget::UncommittedChanges | ReviewTarget::BaseBranch { .. } + if extra_instructions.is_none() => + { + ReviewRequest { + target: state.spec.target.clone(), + user_facing_hint: None, + } + } + ReviewTarget::Commit { .. } + if state.fix_attempts_started == 0 && extra_instructions.is_none() => + { + ReviewRequest { + target: state.spec.target.clone(), + user_facing_hint: None, + } + } + ReviewTarget::Custom { .. } => ReviewRequest { + target: state.spec.target.clone(), + user_facing_hint: None, + }, + ReviewTarget::Commit { sha, title } if is_commit_follow_up => { + let prompt = codex_core::review_prompts::commit_follow_up_review_prompt( + sha, + title.as_deref(), + extra_instructions, + ); + ReviewRequest { + target: ReviewTarget::Custom { + instructions: prompt, + }, + user_facing_hint: Some(format!( + "stack since {}", + codex_core::review_prompts::user_facing_hint(&state.spec.target) + )), + } + } + _ => { + let prompt = + codex_core::review_prompts::review_prompt_with_additional_instructions( + &state.spec.target, + self.config.cwd.as_path(), + extra_instructions, + ) + .map_err(|err| format!("Failed to build review loop prompt: {err}"))?; + ReviewRequest { + target: ReviewTarget::Custom { + instructions: prompt, + }, + user_facing_hint: Some(codex_core::review_prompts::user_facing_hint( + &state.spec.target, + )), + } + } + }; + if self.submit_op(Op::Review { + review_request: request, + }) { + Ok(()) + } else { + Err("Failed to start the next review-loop review.".to_string()) + } + } + pub(crate) fn token_usage(&self) -> TokenUsage { self.token_info .as_ref() diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_loop_cap_prompt_invalid.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_loop_cap_prompt_invalid.snap new file mode 100644 index 0000000000..2ef91e1122 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_loop_cap_prompt_invalid.snap @@ -0,0 +1,10 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: "render_bottom_popup(&chat, 70)" +--- +▌ Set max fix attempts +▌ Enter a positive integer. +▌ +▌ 0 + +Press enter to confirm or esc to go back diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_loop_cap_reached_info.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_loop_cap_reached_info.snap new file mode 100644 index 0000000000..c24790b6c3 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_loop_cap_reached_info.snap @@ -0,0 +1,6 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: rendered +--- + +• Review loop stopped after reaching the limit of 1 fix attempt. diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_loop_commit_validation_failure.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_loop_commit_validation_failure.snap new file mode 100644 index 0000000000..754939bd11 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_loop_commit_validation_failure.snap @@ -0,0 +1,6 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: rendered +--- + +■ Review loop stopped because the fix turn ended with uncommitted changes and no new commit. diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_loop_settings_popup.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_loop_settings_popup.snap new file mode 100644 index 0000000000..e07c7293a6 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_loop_settings_popup.snap @@ -0,0 +1,12 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: "render_bottom_popup(&chat, 70)" +--- + Review loop settings + +› 1. Run until clean Repeat until the reviewer returns no + findings. + 2. Set max fix attempts Stop after a fixed number of fix/review + iterations. + + Press enter to confirm or esc to go back diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index d89ed4bf55..478a2d5fbf 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -29,6 +29,7 @@ use codex_core::features::FEATURES; use codex_core::features::Feature; use codex_core::models_manager::collaboration_mode_presets::CollaborationModesConfig; use codex_core::models_manager::manager::ModelsManager; +use codex_core::review_format::render_review_output_text; use codex_core::skills::model::SkillMetadata; use codex_core::terminal::TerminalName; use codex_otel::OtelManager; @@ -78,6 +79,10 @@ use codex_protocol::protocol::PatchApplyBeginEvent; use codex_protocol::protocol::PatchApplyEndEvent; use codex_protocol::protocol::PatchApplyStatus as CorePatchApplyStatus; use codex_protocol::protocol::RateLimitWindow; +use codex_protocol::protocol::ReviewCodeLocation; +use codex_protocol::protocol::ReviewFinding; +use codex_protocol::protocol::ReviewLineRange; +use codex_protocol::protocol::ReviewOutputEvent; use codex_protocol::protocol::ReviewRequest; use codex_protocol::protocol::ReviewTarget; use codex_protocol::protocol::SessionSource; @@ -1714,6 +1719,9 @@ async fn make_chatwidget_manual( quit_shortcut_expires_at: None, quit_shortcut_key: None, is_review_mode: false, + review_selector_mode: ReviewSelectorMode::SingleReview, + pending_review_loop_draft: None, + review_loop_state: None, pre_review_token_info: None, needs_final_message_separator: false, had_work_activity: false, @@ -1764,6 +1772,53 @@ fn assert_no_submit_op(op_rx: &mut tokio::sync::mpsc::UnboundedReceiver) { } } +fn next_review_request(op_rx: &mut tokio::sync::mpsc::UnboundedReceiver) -> ReviewRequest { + loop { + match op_rx.try_recv() { + Ok(Op::Review { review_request }) => return review_request, + Ok(_) => continue, + Err(TryRecvError::Empty) => panic!("expected a review op but queue was empty"), + Err(TryRecvError::Disconnected) => panic!("expected review op but channel closed"), + } + } +} + +fn assert_no_review_or_submit_op(op_rx: &mut tokio::sync::mpsc::UnboundedReceiver) { + while let Ok(op) = op_rx.try_recv() { + assert!( + !matches!(op, Op::Review { .. } | Op::UserTurn { .. }), + "unexpected follow-up op: {op:?}" + ); + } +} + +fn sample_review_output() -> ReviewOutputEvent { + ReviewOutputEvent { + findings: vec![ReviewFinding { + title: "Missing regression coverage".to_string(), + body: "The loop retries review passes but never verifies that the related tests still pass after applying fixes.".to_string(), + confidence_score: 0.93, + priority: 1, + code_location: ReviewCodeLocation { + absolute_file_path: PathBuf::from("/tmp/review-loop.rs"), + line_range: ReviewLineRange { start: 42, end: 42 }, + }, + }], + overall_correctness: "incorrect".to_string(), + overall_explanation: "The change needs follow-up before it is ready.".to_string(), + overall_confidence_score: 0.91, + } +} + +fn no_findings_review_output() -> ReviewOutputEvent { + ReviewOutputEvent { + findings: Vec::new(), + overall_correctness: "correct".to_string(), + overall_explanation: "No additional issues found.".to_string(), + overall_confidence_score: 0.97, + } +} + fn set_chatgpt_auth(chat: &mut ChatWidget) { chat.auth_manager = codex_core::test_support::auth_manager_from_auth( CodexAuth::create_dummy_chatgpt_auth_for_testing(), @@ -5358,6 +5413,577 @@ async fn review_branch_picker_escape_navigates_back_then_dismisses() { ); } +#[tokio::test] +async fn slash_review_loop_is_blocked_when_feature_disabled() { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; + chat.dispatch_command(SlashCommand::ReviewLoop); + + let header = render_bottom_first_row(&chat, 60); + assert!( + header.contains("Ask Codex to do anything"), + "expected normal composer prompt after blocked review-loop command, got {header:?}" + ); + assert!(chat.review_loop_state.is_none()); +} + +#[tokio::test] +async fn slash_review_loop_opens_review_selector() { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; + chat.set_feature_enabled(Feature::ReviewLoop, true); + + chat.dispatch_command(SlashCommand::ReviewLoop); + + let header = render_bottom_first_row(&chat, 60); + assert!( + header.contains("Select a review preset"), + "expected review selector header: {header:?}" + ); +} + +#[tokio::test] +async fn slash_review_loop_inline_args_are_preserved_for_selected_target() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; + chat.set_feature_enabled(Feature::ReviewLoop, true); + + chat.bottom_pane.set_composer_text( + "/review-loop focus on migration safety".to_string(), + Vec::new(), + Vec::new(), + ); + chat.handle_key_event(KeyEvent::from(KeyCode::Enter)); + chat.handle_key_event(KeyEvent::new(KeyCode::Down, KeyModifiers::NONE)); + chat.handle_key_event(KeyEvent::from(KeyCode::Enter)); + + match rx.try_recv() { + Ok(AppEvent::ReviewLoopTargetSelected { + target: ReviewTarget::UncommittedChanges, + inline_instructions, + }) => { + assert_eq!( + inline_instructions, + Some("focus on migration safety".to_string()) + ); + } + other => panic!("expected ReviewLoopTargetSelected, got {other:?}"), + } +} + +#[tokio::test] +async fn review_loop_custom_selection_uses_inline_prompt_without_extra_prompt() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; + chat.set_feature_enabled(Feature::ReviewLoop, true); + + chat.bottom_pane.set_composer_text( + "/review-loop inspect transaction boundaries".to_string(), + Vec::new(), + Vec::new(), + ); + chat.handle_key_event(KeyEvent::from(KeyCode::Enter)); + chat.handle_key_event(KeyEvent::new(KeyCode::Down, KeyModifiers::NONE)); + chat.handle_key_event(KeyEvent::new(KeyCode::Down, KeyModifiers::NONE)); + chat.handle_key_event(KeyEvent::new(KeyCode::Down, KeyModifiers::NONE)); + chat.handle_key_event(KeyEvent::from(KeyCode::Enter)); + + match rx.try_recv() { + Ok(AppEvent::ReviewLoopTargetSelected { + target: ReviewTarget::Custom { instructions }, + inline_instructions, + }) => { + assert_eq!(instructions, "inspect transaction boundaries"); + assert_eq!( + inline_instructions, + Some("inspect transaction boundaries".to_string()) + ); + } + other => panic!("expected ReviewLoopTargetSelected, got {other:?}"), + } +} + +#[tokio::test] +async fn review_loop_cap_chooser_unlimited_emits_start_event() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; + + chat.on_review_loop_target_selected(ReviewTarget::UncommittedChanges, None); + assert_snapshot!("review_loop_settings_popup", render_bottom_popup(&chat, 70)); + + chat.handle_key_event(KeyEvent::from(KeyCode::Enter)); + + match rx.try_recv() { + Ok(AppEvent::StartReviewLoop { max_fix_attempts }) => { + assert_eq!(max_fix_attempts, None); + } + other => panic!("expected StartReviewLoop, got {other:?}"), + } +} + +#[tokio::test] +async fn review_loop_cap_prompt_accepts_positive_integer() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; + + chat.on_review_loop_target_selected(ReviewTarget::UncommittedChanges, None); + chat.open_review_loop_cap_prompt(); + chat.handle_paste("3".to_string()); + chat.handle_key_event(KeyEvent::from(KeyCode::Enter)); + + match rx.try_recv() { + Ok(AppEvent::StartReviewLoop { max_fix_attempts }) => { + assert_eq!(max_fix_attempts, Some(3)); + } + other => panic!("expected StartReviewLoop, got {other:?}"), + } +} + +#[tokio::test] +async fn review_loop_cap_prompt_rejects_invalid_value() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; + + chat.on_review_loop_target_selected(ReviewTarget::UncommittedChanges, None); + chat.open_review_loop_cap_prompt(); + chat.handle_paste("0".to_string()); + chat.handle_key_event(KeyEvent::from(KeyCode::Enter)); + + assert!( + rx.try_recv().is_err(), + "expected no app event for invalid cap" + ); + assert_snapshot!( + "review_loop_cap_prompt_invalid", + render_bottom_popup(&chat, 70) + ); +} + +#[tokio::test] +async fn review_loop_stops_immediately_when_first_review_has_no_findings() { + let (mut chat, _rx, mut op_rx) = make_chatwidget_manual(None).await; + chat.thread_id = Some(ThreadId::new()); + + chat.on_review_loop_target_selected(ReviewTarget::UncommittedChanges, None); + chat.start_review_loop(None); + + assert_eq!( + next_review_request(&mut op_rx), + ReviewRequest { + target: ReviewTarget::UncommittedChanges, + user_facing_hint: None, + } + ); + + chat.on_exited_review_mode(ExitedReviewModeEvent { + review_output: Some(no_findings_review_output()), + }); + chat.on_task_complete(None, false); + + assert!(chat.review_loop_state.is_none()); + assert_no_review_or_submit_op(&mut op_rx); +} + +#[tokio::test] +async fn review_loop_findings_trigger_fix_turn_with_required_prompt() { + let (mut chat, _rx, mut op_rx) = make_chatwidget_manual(None).await; + chat.thread_id = Some(ThreadId::new()); + let review_output = sample_review_output(); + + chat.on_review_loop_target_selected(ReviewTarget::UncommittedChanges, None); + chat.start_review_loop(None); + let _ = next_review_request(&mut op_rx); + + chat.on_exited_review_mode(ExitedReviewModeEvent { + review_output: Some(review_output.clone()), + }); + chat.on_task_complete(None, false); + + let items = match next_submit_op(&mut op_rx) { + Op::UserTurn { items, .. } => items, + other => panic!("expected Op::UserTurn, got {other:?}"), + }; + assert_eq!( + items, + vec![UserInput::Text { + text: format!( + "Please implement fixes for these findings and rerun the relevant test suites to ensure they are passing.\n\n{}", + render_review_output_text(&review_output) + ), + text_elements: Vec::new(), + }] + ); +} + +#[tokio::test] +async fn review_loop_findings_trigger_fix_turn_with_default_mode_in_plan() { + let (mut chat, _rx, mut op_rx) = make_chatwidget_manual(None).await; + chat.thread_id = Some(ThreadId::new()); + chat.set_feature_enabled(Feature::CollaborationModes, true); + let plan_mask = collaboration_modes::plan_mask(chat.models_manager.as_ref()) + .expect("plan collaboration mode should be available"); + chat.set_collaboration_mask(plan_mask); + let review_output = sample_review_output(); + + chat.on_review_loop_target_selected(ReviewTarget::UncommittedChanges, None); + chat.start_review_loop(None); + let _ = next_review_request(&mut op_rx); + + chat.on_exited_review_mode(ExitedReviewModeEvent { + review_output: Some(review_output), + }); + chat.on_task_complete(None, false); + + match next_submit_op(&mut op_rx) { + Op::UserTurn { + collaboration_mode: + Some(CollaborationMode { + mode: ModeKind::Default, + .. + }), + .. + } => {} + other => panic!("expected fix turn in default mode, got {other:?}"), + } + + assert_eq!(chat.active_mode_kind(), ModeKind::Plan); +} + +#[tokio::test] +async fn review_loop_commit_targets_append_commit_requirement_to_fix_prompt() { + let (mut chat, _rx, mut op_rx) = make_chatwidget_manual(None).await; + chat.thread_id = Some(ThreadId::new()); + let review_output = sample_review_output(); + + chat.on_review_loop_target_selected( + ReviewTarget::BaseBranch { + branch: "main".to_string(), + }, + None, + ); + chat.start_review_loop(None); + let generation = chat + .review_loop_state + .as_ref() + .expect("loop state") + .generation; + chat.on_review_loop_initial_head_resolved(generation, Some("head-1".to_string())); + let _ = next_review_request(&mut op_rx); + + chat.on_exited_review_mode(ExitedReviewModeEvent { + review_output: Some(review_output), + }); + chat.on_task_complete(None, false); + + let items = match next_submit_op(&mut op_rx) { + Op::UserTurn { items, .. } => items, + other => panic!("expected Op::UserTurn, got {other:?}"), + }; + let UserInput::Text { + text, + text_elements, + } = &items[0] + else { + panic!("expected text item in fix prompt"); + }; + assert_eq!(text_elements, &Vec::new()); + assert!( + text.contains("Before finishing, commit the fixes in a new git commit so the next review pass includes them."), + "expected commit reminder in fix prompt: {text:?}" + ); +} + +#[tokio::test] +async fn review_loop_commit_follow_up_review_uses_cumulative_stack_prompt() { + let (mut chat, _rx, mut op_rx) = make_chatwidget_manual(None).await; + chat.thread_id = Some(ThreadId::new()); + let review_output = sample_review_output(); + + chat.on_review_loop_target_selected( + ReviewTarget::Commit { + sha: "abc1234".to_string(), + title: Some("Add review loop".to_string()), + }, + Some("Focus on migration safety.".to_string()), + ); + chat.start_review_loop(None); + let generation = chat + .review_loop_state + .as_ref() + .expect("loop state") + .generation; + chat.on_review_loop_initial_head_resolved(generation, Some("head-1".to_string())); + let _ = next_review_request(&mut op_rx); + + chat.on_exited_review_mode(ExitedReviewModeEvent { + review_output: Some(review_output), + }); + chat.on_task_complete(None, false); + let _ = next_submit_op(&mut op_rx); + + chat.on_task_complete(None, false); + chat.on_review_loop_commit_validation_resolved( + generation, + Some("head-2".to_string()), + Some(false), + ); + + let expected_prompt = codex_core::review_prompts::commit_follow_up_review_prompt( + "abc1234", + Some("Add review loop"), + Some("Focus on migration safety."), + ); + assert_eq!( + next_review_request(&mut op_rx), + ReviewRequest { + target: ReviewTarget::Custom { + instructions: expected_prompt, + }, + user_facing_hint: Some("stack since commit abc1234: Add review loop".to_string()), + } + ); +} + +#[tokio::test] +async fn review_loop_stops_when_fix_attempt_cap_is_reached() { + let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(None).await; + chat.thread_id = Some(ThreadId::new()); + let review_output = sample_review_output(); + + chat.on_review_loop_target_selected(ReviewTarget::UncommittedChanges, None); + chat.start_review_loop(Some(1)); + let _ = next_review_request(&mut op_rx); + + chat.on_exited_review_mode(ExitedReviewModeEvent { + review_output: Some(review_output.clone()), + }); + chat.on_task_complete(None, false); + let _ = next_submit_op(&mut op_rx); + + chat.on_task_complete(None, false); + let _ = next_review_request(&mut op_rx); + + chat.on_exited_review_mode(ExitedReviewModeEvent { + review_output: Some(review_output), + }); + chat.on_task_complete(None, false); + + assert!(chat.review_loop_state.is_none()); + assert_no_review_or_submit_op(&mut op_rx); + let rendered = drain_insert_history(&mut rx) + .into_iter() + .map(|lines| lines_to_single_string(&lines)) + .find(|rendered| rendered.contains("Review loop stopped after reaching the limit")) + .expect("expected cap reached message"); + assert_snapshot!("review_loop_cap_reached_info", rendered); +} + +#[tokio::test] +async fn review_loop_queued_messages_wait_until_loop_completes() { + let (mut chat, _rx, mut op_rx) = make_chatwidget_manual(None).await; + chat.thread_id = Some(ThreadId::new()); + let review_output = sample_review_output(); + + chat.on_review_loop_target_selected(ReviewTarget::UncommittedChanges, None); + chat.start_review_loop(None); + let _ = next_review_request(&mut op_rx); + + chat.queue_user_message("after the loop".to_string().into()); + assert_eq!(chat.queued_user_messages.len(), 1); + + chat.on_exited_review_mode(ExitedReviewModeEvent { + review_output: Some(review_output), + }); + chat.on_task_complete(None, false); + let fix_items = match next_submit_op(&mut op_rx) { + Op::UserTurn { items, .. } => items, + other => panic!("expected fix Op::UserTurn, got {other:?}"), + }; + assert_ne!( + fix_items, + vec![UserInput::Text { + text: "after the loop".to_string(), + text_elements: Vec::new(), + }] + ); + assert_eq!(chat.queued_user_messages.len(), 1); + + chat.on_task_complete(None, false); + let _ = next_review_request(&mut op_rx); + assert_eq!(chat.queued_user_messages.len(), 1); + + chat.on_exited_review_mode(ExitedReviewModeEvent { + review_output: Some(no_findings_review_output()), + }); + chat.on_task_complete(None, false); + + let queued_items = match next_submit_op(&mut op_rx) { + Op::UserTurn { items, .. } => items, + other => panic!("expected queued Op::UserTurn, got {other:?}"), + }; + assert_eq!( + queued_items, + vec![UserInput::Text { + text: "after the loop".to_string(), + text_elements: Vec::new(), + }] + ); + assert!(chat.queued_user_messages.is_empty()); +} + +#[tokio::test] +async fn review_loop_commit_validation_failure_stops_with_error() { + let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(None).await; + chat.thread_id = Some(ThreadId::new()); + let review_output = sample_review_output(); + + chat.on_review_loop_target_selected( + ReviewTarget::BaseBranch { + branch: "main".to_string(), + }, + None, + ); + chat.start_review_loop(None); + let generation = chat + .review_loop_state + .as_ref() + .expect("loop state") + .generation; + chat.on_review_loop_initial_head_resolved(generation, Some("head-1".to_string())); + let _ = next_review_request(&mut op_rx); + + chat.on_exited_review_mode(ExitedReviewModeEvent { + review_output: Some(review_output), + }); + chat.on_task_complete(None, false); + let _ = next_submit_op(&mut op_rx); + + chat.on_task_complete(None, false); + chat.on_review_loop_commit_validation_resolved( + generation, + Some("head-1".to_string()), + Some(true), + ); + + assert!(chat.review_loop_state.is_none()); + assert_no_review_or_submit_op(&mut op_rx); + let rendered = drain_insert_history(&mut rx) + .last() + .map(|lines| lines_to_single_string(lines)) + .expect("expected commit validation error"); + assert_snapshot!("review_loop_commit_validation_failure", rendered); +} + +#[tokio::test] +async fn review_loop_commit_validation_stops_when_fix_is_dirty_with_new_commit() { + let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(None).await; + chat.thread_id = Some(ThreadId::new()); + let review_output = sample_review_output(); + + chat.on_review_loop_target_selected( + ReviewTarget::BaseBranch { + branch: "main".to_string(), + }, + None, + ); + chat.start_review_loop(None); + let generation = chat + .review_loop_state + .as_ref() + .expect("loop state") + .generation; + chat.on_review_loop_initial_head_resolved(generation, Some("head-1".to_string())); + let _ = next_review_request(&mut op_rx); + + chat.on_exited_review_mode(ExitedReviewModeEvent { + review_output: Some(review_output), + }); + chat.on_task_complete(None, false); + let _ = next_submit_op(&mut op_rx); + + chat.on_task_complete(None, false); + chat.on_review_loop_commit_validation_resolved( + generation, + Some("head-2".to_string()), + Some(true), + ); + + assert!(chat.review_loop_state.is_none()); + assert_no_review_or_submit_op(&mut op_rx); + let rendered = drain_insert_history(&mut rx) + .last() + .map(|lines| lines_to_single_string(lines)) + .expect("expected commit validation failure"); + assert!(rendered.contains("created a new commit but left uncommitted changes.")); +} + +#[tokio::test] +async fn review_loop_state_survives_stream_error_during_fix_turn() { + let (mut chat, _rx, mut op_rx) = make_chatwidget_manual(None).await; + chat.thread_id = Some(ThreadId::new()); + let review_output = sample_review_output(); + + chat.on_review_loop_target_selected(ReviewTarget::UncommittedChanges, None); + chat.start_review_loop(None); + let _ = next_review_request(&mut op_rx); + + chat.on_exited_review_mode(ExitedReviewModeEvent { + review_output: Some(review_output), + }); + chat.on_task_complete(None, false); + let _ = next_submit_op(&mut op_rx); + + let state = chat.review_loop_state.as_ref().expect("loop state"); + assert_eq!(state.stage, ReviewLoopStage::Fixing); + + chat.handle_codex_event(Event { + id: "sub-1".into(), + msg: EventMsg::StreamError(StreamErrorEvent { + message: "Reconnecting... 1/5".to_string(), + codex_error_info: Some(CodexErrorInfo::Other), + additional_details: Some("Idle timeout waiting for SSE".to_string()), + }), + }); + + let state = chat.review_loop_state.as_ref().expect("loop state"); + assert_eq!(state.stage, ReviewLoopStage::Fixing); + + chat.on_task_complete(None, false); + let _ = next_review_request(&mut op_rx); +} + +#[tokio::test] +async fn review_loop_ignores_stale_commit_validation_callbacks() { + let (mut chat, _rx, mut op_rx) = make_chatwidget_manual(None).await; + chat.thread_id = Some(ThreadId::new()); + let review_output = sample_review_output(); + + chat.on_review_loop_target_selected( + ReviewTarget::BaseBranch { + branch: "main".to_string(), + }, + None, + ); + chat.start_review_loop(None); + let generation = chat + .review_loop_state + .as_ref() + .expect("loop state") + .generation; + chat.on_review_loop_initial_head_resolved(generation, Some("head-1".to_string())); + let _ = next_review_request(&mut op_rx); + + chat.on_exited_review_mode(ExitedReviewModeEvent { + review_output: Some(review_output), + }); + chat.on_task_complete(None, false); + let _ = next_submit_op(&mut op_rx); + + chat.on_task_complete(None, false); + chat.on_review_loop_commit_validation_resolved( + generation.wrapping_add(1), + Some("head-2".to_string()), + Some(false), + ); + + let state = chat.review_loop_state.as_ref().expect("loop state"); + assert_eq!(state.stage, ReviewLoopStage::AwaitingCommitValidation); + assert_eq!(state.baseline_head_sha.as_deref(), Some("head-1")); +} + fn render_bottom_first_row(chat: &ChatWidget, width: u16) -> String { let height = chat.desired_height(width); let area = Rect::new(0, 0, width, height); diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 14510c10ae..936d0b8224 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -1093,6 +1093,11 @@ pub(crate) fn new_session_info( "/review".into(), " - review any changes and find issues".dim(), ]), + Line::from(vec![ + " ".into(), + "/review-loop".into(), + " - run review/fix/review until clean".dim(), + ]), ]; parts.push(Box::new(PlainHistoryCell { lines: help_lines })); diff --git a/codex-rs/tui/src/slash_command.rs b/codex-rs/tui/src/slash_command.rs index 361c91a4a6..d8f258a841 100644 --- a/codex-rs/tui/src/slash_command.rs +++ b/codex-rs/tui/src/slash_command.rs @@ -22,6 +22,7 @@ pub enum SlashCommand { Experimental, Skills, Review, + ReviewLoop, Rename, New, Resume, @@ -70,6 +71,7 @@ impl SlashCommand { SlashCommand::Init => "create an AGENTS.md file with instructions for Codex", SlashCommand::Compact => "summarize conversation to prevent hitting the context limit", SlashCommand::Review => "review my current changes and find issues", + SlashCommand::ReviewLoop => "run review/fix/review until clean", SlashCommand::Rename => "rename the current thread", SlashCommand::Resume => "resume a saved chat", SlashCommand::Clear => "clear the terminal and start a new chat", @@ -121,6 +123,7 @@ impl SlashCommand { matches!( self, SlashCommand::Review + | SlashCommand::ReviewLoop | SlashCommand::Rename | SlashCommand::Plan | SlashCommand::SandboxReadRoot @@ -144,6 +147,7 @@ impl SlashCommand { | SlashCommand::SandboxReadRoot | SlashCommand::Experimental | SlashCommand::Review + | SlashCommand::ReviewLoop | SlashCommand::Plan | SlashCommand::Clear | SlashCommand::Logout