From 2405772af3ec2e4b45441f9850e2089e3e0ba852 Mon Sep 17 00:00:00 2001 From: Daniel Edrisian Date: Tue, 16 Sep 2025 23:47:49 -0700 Subject: [PATCH] Simple /review --- codex-rs/core/src/codex.rs | 4 +- codex-rs/core/src/review_format.rs | 10 +- .../src/event_processor_with_human_output.rs | 3 + codex-rs/protocol/src/protocol.rs | 1 + codex-rs/tui/src/chatwidget.rs | 134 +++++++++++++++--- codex-rs/tui/src/chatwidget/tests.rs | 81 +++++++++++ codex-rs/tui/src/history_cell.rs | 28 ++++ codex-rs/tui/src/slash_command.rs | 3 + 8 files changed, 235 insertions(+), 29 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index b3b75ec76a..09f7b74485 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -3293,7 +3293,7 @@ async fn exit_review_mode( {findings_str} - + "#)); } else { user_message.push_str(r#" @@ -3302,7 +3302,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 272010d564..6a64d06fa1 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 c126208fad..5ef7d51a26 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 c3aebcdd42..c467cba346 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 fb91d75456..0c7f186fdd 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; @@ -91,7 +94,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 +143,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,11 +283,13 @@ impl ChatWidget { } /// 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) { + fn finalize_turn_with_error_message(&mut self, message: Option) { // 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)); + if let Some(message) = message { + 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(); @@ -291,7 +297,7 @@ impl ChatWidget { } fn on_error(&mut self, message: String) { - self.finalize_turn_with_error_message(message); + self.finalize_turn_with_error_message(Some(message)); self.request_redraw(); // After an error ends the turn, try sending the next queued input. @@ -301,11 +307,13 @@ 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_with_error_message(if reason == TurnAbortReason::ReviewEnded { + None + } else { + Some("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() { @@ -398,9 +406,9 @@ impl ChatWidget { fn on_web_search_end(&mut self, ev: WebSearchEndEvent) { self.flush_answer_stream_with_separator(); + let query = ev.query; self.add_to_history(history_cell::new_web_search_call(format!( - "Searched: {}", - ev.query + "Searched: {query}" ))); } @@ -631,7 +639,11 @@ impl ChatWidget { } fn layout_areas(&self, area: Rect) -> [Rect; 3] { - let bottom_min = self.bottom_pane.desired_height(area.width).min(area.height); + let bottom_min = if self.is_review_mode { + 3.min(area.height) + } else { + self.bottom_pane.desired_height(area.width).min(area.height) + }; let remaining = area.height.saturating_sub(bottom_min); let active_desired = self @@ -700,6 +712,7 @@ impl ChatWidget { show_welcome_banner: true, suppress_session_configured_redraw: false, pending_notification: None, + is_review_mode: false, } } @@ -756,6 +769,7 @@ impl ChatWidget { show_welcome_banner: true, suppress_session_configured_redraw: true, pending_notification: None, + is_review_mode: false, } } @@ -870,6 +884,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(); } @@ -976,8 +999,8 @@ impl ChatWidget { fn flush_active_exec_cell(&mut self) { if let Some(active) = self.active_exec_cell.take() { - self.app_event_tx - .send(AppEvent::InsertHistoryCell(Box::new(active))); + let cell: Box = Box::new(active); + self.app_event_tx.send(AppEvent::InsertHistoryCell(cell)); } } @@ -1087,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) => { @@ -1128,11 +1154,68 @@ 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() { + // Show explanation or a fallback when there are no structured findings. + let mut lines: Vec> = + codex_core::review_format::format_review_findings_block(&[], None) + .lines() + .map(|s| ratatui::text::Line::from(s.to_string())) + .collect(); + lines.push("".into()); + let explanation = output.overall_explanation.trim().to_string(); + if explanation.is_empty() { + lines.push("Review failed -- no response found".into()); + } else { + for l in explanation.lines() { + lines.push(ratatui::text::Line::from(l.to_string())); + } + } + let body_cell = crate::history_cell::AgentMessageCell::new(lines, 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 message_lines: Vec> = message_text + .lines() + .map(|s| ratatui::text::Line::from(s.to_string())) + .collect(); + let body_cell = crate::history_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) @@ -1406,7 +1489,7 @@ impl ChatWidget { self.bottom_pane.clear_esc_backtrack_hint(); } /// Forward an `Op` directly to codex. - pub(crate) fn submit_op(&self, op: Op) { + pub(crate) fn submit_op(&mut self, op: Op) { // Record outbound operation for session replay fidelity. crate::session_log::log_outbound_op(&op); if let Err(e) = self.codex_op_tx.send(op) { @@ -1432,7 +1515,14 @@ impl ChatWidget { if text.is_empty() { return; } - self.submit_user_message(text.into()); + + let user_message: UserMessage = text.into(); + if self.bottom_pane.is_task_running() { + self.queued_user_messages.push_back(user_message); + self.refresh_queued_user_messages(); + } else { + self.submit_user_message(user_message); + } } pub(crate) fn token_usage(&self) -> TokenUsage { @@ -1470,10 +1560,10 @@ impl WidgetRef for &ChatWidget { if !active_cell_area.is_empty() && let Some(cell) = &self.active_exec_cell { - let mut active_cell_area = active_cell_area; - active_cell_area.y = active_cell_area.y.saturating_add(1); - active_cell_area.height -= 1; - cell.render_ref(active_cell_area, buf); + let mut area_to_render = active_cell_area; + area_to_render.y = area_to_render.y.saturating_add(1); + area_to_render.height = area_to_render.height.saturating_sub(1); + cell.render_ref(area_to_render, buf); } } } diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index fb4ea5e888..96dc14202a 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, "\n>> 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, "\n>> 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 9cdd29dd02..d5b6cd6090 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -229,6 +229,29 @@ impl HistoryCell for TranscriptOnlyHistoryCell { } } +/// Insert a transcript line describing the current review status. +#[derive(Debug)] +pub(crate) struct ReviewStatusHistoryCell { + message: String, +} + +impl HistoryCell for ReviewStatusHistoryCell { + fn display_lines(&self, _width: u16) -> Vec> { + // Add one empty line above the review status banner for visual padding + vec!["".into(), Line::from(self.message.clone().cyan())] + } + + fn is_stream_continuation(&self) -> bool { + // Treat status lines as part of the same history block to avoid an extra + // blank separator when following other content in the same turn. + true + } +} + +pub(crate) fn new_review_status_line(message: String) -> ReviewStatusHistoryCell { + ReviewStatusHistoryCell { message } +} + #[derive(Debug)] pub(crate) struct PatchHistoryCell { event_type: PatchEventType, @@ -691,6 +714,11 @@ pub(crate) fn new_session_info( "/model".into(), " - choose what model and reasoning effort to use".dim(), ]), + Line::from(vec![ + " ".into(), + "/review".into(), + " - review current changes and find issues".dim(), + ]), ]; CompositeHistoryCell { diff --git a/codex-rs/tui/src/slash_command.rs b/codex-rs/tui/src/slash_command.rs index 3268a92a2d..433c0a6d7f 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