diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 1de37f6f..8caa2e8e 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -3303,7 +3303,7 @@ async fn exit_review_mode( {findings_str} - + "#)); } else { user_message.push_str(r#" @@ -3312,7 +3312,7 @@ async fn exit_review_mode( None. - + "#); } diff --git a/codex-rs/core/src/review_format.rs b/codex-rs/core/src/review_format.rs index 272010d5..6a64d06f 100644 --- a/codex-rs/core/src/review_format.rs +++ b/codex-rs/core/src/review_format.rs @@ -22,14 +22,14 @@ pub fn format_review_findings_block( selection: Option<&[bool]>, ) -> String { let mut lines: Vec = Vec::new(); + lines.push(String::new()); // Header - let header = if findings.len() > 1 { - "Full review comments:" + if findings.len() > 1 { + lines.push("Full review comments:".to_string()); } else { - "Review comment:" - }; - lines.push(header.to_string()); + lines.push("Review comment:".to_string()); + } for (idx, item) in findings.iter().enumerate() { lines.push(String::new()); diff --git a/codex-rs/exec/src/event_processor_with_human_output.rs b/codex-rs/exec/src/event_processor_with_human_output.rs index c126208f..5ef7d51a 100644 --- a/codex-rs/exec/src/event_processor_with_human_output.rs +++ b/codex-rs/exec/src/event_processor_with_human_output.rs @@ -558,6 +558,9 @@ impl EventProcessor for EventProcessorWithHumanOutput { TurnAbortReason::Replaced => { ts_println!(self, "task aborted: replaced by a new task"); } + TurnAbortReason::ReviewEnded => { + ts_println!(self, "task aborted: review ended"); + } }, EventMsg::ShutdownComplete => return CodexStatus::Shutdown, EventMsg::ConversationPath(_) => {} diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index dddaeee2..8009ac9b 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -1240,6 +1240,7 @@ pub struct TurnAbortedEvent { pub enum TurnAbortReason { Interrupted, Replaced, + ReviewEnded, } #[cfg(test)] diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 7ab954f2..e024fd0f 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -19,6 +19,7 @@ use codex_core::protocol::EventMsg; use codex_core::protocol::ExecApprovalRequestEvent; use codex_core::protocol::ExecCommandBeginEvent; use codex_core::protocol::ExecCommandEndEvent; +use codex_core::protocol::ExitedReviewModeEvent; use codex_core::protocol::InputItem; use codex_core::protocol::InputMessageKind; use codex_core::protocol::ListCustomPromptsResponseEvent; @@ -27,6 +28,7 @@ use codex_core::protocol::McpToolCallBeginEvent; use codex_core::protocol::McpToolCallEndEvent; use codex_core::protocol::Op; use codex_core::protocol::PatchApplyBeginEvent; +use codex_core::protocol::ReviewRequest; use codex_core::protocol::StreamErrorEvent; use codex_core::protocol::TaskCompleteEvent; use codex_core::protocol::TokenUsage; @@ -36,6 +38,7 @@ use codex_core::protocol::TurnDiffEvent; use codex_core::protocol::UserMessageEvent; use codex_core::protocol::WebSearchBeginEvent; use codex_core::protocol::WebSearchEndEvent; +use codex_protocol::mcp_protocol::ConversationId; use codex_protocol::parse_command::ParsedCommand; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; @@ -63,10 +66,12 @@ use crate::clipboard_paste::paste_image_to_temp_png; use crate::diff_render::display_path_for; use crate::get_git_diff::get_git_diff; use crate::history_cell; +use crate::history_cell::AgentMessageCell; use crate::history_cell::CommandOutput; use crate::history_cell::ExecCell; use crate::history_cell::HistoryCell; use crate::history_cell::PatchEventType; +use crate::markdown::append_markdown; use crate::slash_command::SlashCommand; use crate::text_formatting::truncate_text; use crate::tui::FrameRequester; @@ -91,7 +96,6 @@ use codex_core::protocol::AskForApproval; use codex_core::protocol::SandboxPolicy; use codex_core::protocol_config_types::ReasoningEffort as ReasoningEffortConfig; use codex_file_search::FileMatch; -use codex_protocol::mcp_protocol::ConversationId; // Track information about an in-flight exec command. struct RunningCommand { @@ -141,6 +145,8 @@ pub(crate) struct ChatWidget { queued_user_messages: VecDeque, // Pending notification to show when unfocused on next Draw pending_notification: Option, + // Simple review mode flag; used to adjust layout and banners. + is_review_mode: bool, } struct UserMessage { @@ -279,13 +285,10 @@ impl ChatWidget { self.bottom_pane.set_token_usage(info.clone()); self.token_info = info; } - /// Finalize any active exec as failed, push an error message into history, - /// and stop/clear running UI state. - fn finalize_turn_with_error_message(&mut self, message: String) { + /// Finalize any active exec as failed and stop/clear running UI state. + fn finalize_turn(&mut self) { // Ensure any spinner is replaced by a red ✗ and flushed into history. self.finalize_active_exec_cell_as_failed(); - // Emit the provided error message/history cell. - self.add_to_history(history_cell::new_error_event(message)); // Reset running state and clear streaming buffers. self.bottom_pane.set_task_running(false); self.running_commands.clear(); @@ -293,7 +296,8 @@ impl ChatWidget { } fn on_error(&mut self, message: String) { - self.finalize_turn_with_error_message(message); + self.finalize_turn(); + self.add_to_history(history_cell::new_error_event(message)); self.request_redraw(); // After an error ends the turn, try sending the next queued input. @@ -303,11 +307,15 @@ impl ChatWidget { /// Handle a turn aborted due to user interrupt (Esc). /// 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) { + fn on_interrupted_turn(&mut self, reason: TurnAbortReason) { // Finalize, log a gentle prompt, and clear running state. - self.finalize_turn_with_error_message( - "Conversation interrupted - tell the model what to do differently".to_owned(), - ); + self.finalize_turn(); + + if reason != TurnAbortReason::ReviewEnded { + self.add_to_history(history_cell::new_error_event( + "Conversation interrupted - tell the model what to do differently".to_owned(), + )); + } // If any messages were queued during the task, restore them into the composer. if !self.queued_user_messages.is_empty() { @@ -702,6 +710,7 @@ impl ChatWidget { show_welcome_banner: true, suppress_session_configured_redraw: false, pending_notification: None, + is_review_mode: false, } } @@ -758,6 +767,7 @@ impl ChatWidget { show_welcome_banner: true, suppress_session_configured_redraw: true, pending_notification: None, + is_review_mode: false, } } @@ -872,6 +882,15 @@ impl ChatWidget { self.clear_token_usage(); self.app_event_tx.send(AppEvent::CodexOp(Op::Compact)); } + SlashCommand::Review => { + // Simplified flow: directly send a review op for current changes. + self.submit_op(Op::Review { + review_request: ReviewRequest { + prompt: "review current changes".to_string(), + user_facing_hint: "current changes".to_string(), + }, + }); + } SlashCommand::Model => { self.open_model_popup(); } @@ -1091,11 +1110,14 @@ impl ChatWidget { EventMsg::Error(ErrorEvent { message }) => self.on_error(message), EventMsg::TurnAborted(ev) => match ev.reason { TurnAbortReason::Interrupted => { - self.on_interrupted_turn(); + self.on_interrupted_turn(ev.reason); } TurnAbortReason::Replaced => { self.on_error("Turn aborted: replaced by a new task".to_owned()) } + TurnAbortReason::ReviewEnded => { + self.on_interrupted_turn(ev.reason); + } }, EventMsg::PlanUpdate(update) => self.on_plan_update(update), EventMsg::ExecApprovalRequest(ev) => { @@ -1132,11 +1154,62 @@ impl ChatWidget { self.app_event_tx .send(crate::app_event::AppEvent::ConversationHistory(ev)); } - EventMsg::EnteredReviewMode(_) => {} - EventMsg::ExitedReviewMode(_) => {} + EventMsg::EnteredReviewMode(review_request) => { + self.on_entered_review_mode(review_request) + } + EventMsg::ExitedReviewMode(review) => self.on_exited_review_mode(review), } } + fn on_entered_review_mode(&mut self, review: ReviewRequest) { + // Enter review mode and emit a concise banner + self.is_review_mode = true; + let banner = format!(">> Code review started: {} <<", review.user_facing_hint); + self.add_to_history(history_cell::new_review_status_line(banner)); + self.request_redraw(); + } + + fn on_exited_review_mode(&mut self, review: ExitedReviewModeEvent) { + // Leave review mode; if output is present, flush pending stream + show results. + if let Some(output) = review.review_output { + self.flush_answer_stream_with_separator(); + self.flush_interrupt_queue(); + self.flush_active_exec_cell(); + + if output.findings.is_empty() { + let explanation = output.overall_explanation.trim().to_string(); + if explanation.is_empty() { + tracing::error!("Reviewer failed to output a response."); + self.add_to_history(history_cell::new_error_event( + "Reviewer failed to output a response.".to_owned(), + )); + } else { + // Show explanation when there are no structured findings. + let mut rendered: Vec> = vec!["".into()]; + append_markdown(&explanation, &mut rendered, &self.config); + let body_cell = AgentMessageCell::new(rendered, false); + self.app_event_tx + .send(AppEvent::InsertHistoryCell(Box::new(body_cell))); + } + } else { + let message_text = + codex_core::review_format::format_review_findings_block(&output.findings, None); + let mut message_lines: Vec> = Vec::new(); + append_markdown(&message_text, &mut message_lines, &self.config); + let body_cell = AgentMessageCell::new(message_lines, true); + self.app_event_tx + .send(AppEvent::InsertHistoryCell(Box::new(body_cell))); + } + } + + self.is_review_mode = false; + // Append a finishing banner at the end of this turn. + self.add_to_history(history_cell::new_review_status_line( + "<< Code review finished >>".to_string(), + )); + self.request_redraw(); + } + fn on_user_message_event(&mut self, event: UserMessageEvent) { match event.kind { Some(InputMessageKind::EnvironmentContext) diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index fb4ea5e8..ffe3f3f7 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -19,10 +19,17 @@ use codex_core::protocol::EventMsg; use codex_core::protocol::ExecApprovalRequestEvent; use codex_core::protocol::ExecCommandBeginEvent; use codex_core::protocol::ExecCommandEndEvent; +use codex_core::protocol::ExitedReviewModeEvent; use codex_core::protocol::FileChange; use codex_core::protocol::InputMessageKind; +use codex_core::protocol::Op; use codex_core::protocol::PatchApplyBeginEvent; use codex_core::protocol::PatchApplyEndEvent; +use codex_core::protocol::ReviewCodeLocation; +use codex_core::protocol::ReviewFinding; +use codex_core::protocol::ReviewLineRange; +use codex_core::protocol::ReviewOutputEvent; +use codex_core::protocol::ReviewRequest; use codex_core::protocol::StreamErrorEvent; use codex_core::protocol::TaskCompleteEvent; use codex_core::protocol::TaskStartedEvent; @@ -184,6 +191,79 @@ fn resumed_initial_messages_render_history() { ); } +/// Entering review mode uses the hint provided by the review request. +#[test] +fn entered_review_mode_uses_request_hint() { + let (mut chat, mut rx, _ops) = make_chatwidget_manual(); + + chat.handle_codex_event(Event { + id: "review-start".into(), + msg: EventMsg::EnteredReviewMode(ReviewRequest { + prompt: "Review the latest changes".to_string(), + user_facing_hint: "feature branch".to_string(), + }), + }); + + let cells = drain_insert_history(&mut rx); + let banner = lines_to_single_string(cells.last().expect("review banner")); + assert_eq!(banner, ">> Code review started: feature branch <<\n"); + assert!(chat.is_review_mode); +} + +/// Entering review mode renders the current changes banner when requested. +#[test] +fn entered_review_mode_defaults_to_current_changes_banner() { + let (mut chat, mut rx, _ops) = make_chatwidget_manual(); + + chat.handle_codex_event(Event { + id: "review-start".into(), + msg: EventMsg::EnteredReviewMode(ReviewRequest { + prompt: "Review the current changes".to_string(), + user_facing_hint: "current changes".to_string(), + }), + }); + + let cells = drain_insert_history(&mut rx); + let banner = lines_to_single_string(cells.last().expect("review banner")); + assert_eq!(banner, ">> Code review started: current changes <<\n"); + assert!(chat.is_review_mode); +} + +/// Completing review with findings shows the selection popup and finishes with +/// the closing banner while clearing review mode state. +#[test] +fn exited_review_mode_emits_results_and_finishes() { + let (mut chat, mut rx, _ops) = make_chatwidget_manual(); + + let review = ReviewOutputEvent { + findings: vec![ReviewFinding { + title: "[P1] Fix bug".to_string(), + body: "Something went wrong".to_string(), + confidence_score: 0.9, + priority: 1, + code_location: ReviewCodeLocation { + absolute_file_path: PathBuf::from("src/lib.rs"), + line_range: ReviewLineRange { start: 10, end: 12 }, + }, + }], + overall_correctness: "needs work".to_string(), + overall_explanation: "Investigate the failure".to_string(), + overall_confidence_score: 0.5, + }; + + chat.handle_codex_event(Event { + id: "review-end".into(), + msg: EventMsg::ExitedReviewMode(ExitedReviewModeEvent { + review_output: Some(review), + }), + }); + + let cells = drain_insert_history(&mut rx); + let banner = lines_to_single_string(cells.last().expect("finished banner")); + assert_eq!(banner, "\n<< Code review finished >>\n"); + assert!(!chat.is_review_mode); +} + #[cfg_attr( target_os = "macos", ignore = "system configuration APIs are blocked under macOS seatbelt" @@ -252,6 +332,7 @@ fn make_chatwidget_manual() -> ( queued_user_messages: VecDeque::new(), suppress_session_configured_redraw: false, pending_notification: None, + is_review_mode: false, }; (widget, rx, op_rx) } diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index ad3d5a90..a99a4dd8 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -236,6 +236,13 @@ impl HistoryCell for TranscriptOnlyHistoryCell { } } +/// Cyan history cell line showing the current review status. +pub(crate) fn new_review_status_line(message: String) -> PlainHistoryCell { + PlainHistoryCell { + lines: vec![Line::from(message.cyan())], + } +} + #[derive(Debug)] pub(crate) struct PatchHistoryCell { event_type: PatchEventType, diff --git a/codex-rs/tui/src/slash_command.rs b/codex-rs/tui/src/slash_command.rs index 3268a92a..433c0a6d 100644 --- a/codex-rs/tui/src/slash_command.rs +++ b/codex-rs/tui/src/slash_command.rs @@ -14,6 +14,7 @@ pub enum SlashCommand { // more frequently used commands should be listed first. Model, Approvals, + Review, New, Init, Compact, @@ -34,6 +35,7 @@ impl SlashCommand { SlashCommand::New => "start a new chat during a conversation", 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::Quit => "exit Codex", SlashCommand::Diff => "show git diff (including untracked files)", SlashCommand::Mention => "mention a file", @@ -61,6 +63,7 @@ impl SlashCommand { | SlashCommand::Compact | SlashCommand::Model | SlashCommand::Approvals + | SlashCommand::Review | SlashCommand::Logout => false, SlashCommand::Diff | SlashCommand::Mention