mirror of
https://github.com/openai/codex.git
synced 2026-10-29 19:08:43 +00:00
Simple /review
This commit is contained in:
@@ -3293,7 +3293,7 @@ async fn exit_review_mode(
|
||||
<results>
|
||||
{findings_str}
|
||||
</results>
|
||||
</user_tool>
|
||||
</user_action>
|
||||
"#));
|
||||
} else {
|
||||
user_message.push_str(r#"<user_action>
|
||||
@@ -3302,7 +3302,7 @@ async fn exit_review_mode(
|
||||
<results>
|
||||
None.
|
||||
</results>
|
||||
</user_tool>
|
||||
</user_action>
|
||||
"#);
|
||||
}
|
||||
|
||||
|
||||
@@ -22,14 +22,14 @@ pub fn format_review_findings_block(
|
||||
selection: Option<&[bool]>,
|
||||
) -> String {
|
||||
let mut lines: Vec<String> = 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());
|
||||
|
||||
@@ -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(_) => {}
|
||||
|
||||
@@ -1240,6 +1240,7 @@ pub struct TurnAbortedEvent {
|
||||
pub enum TurnAbortReason {
|
||||
Interrupted,
|
||||
Replaced,
|
||||
ReviewEnded,
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
||||
+112
-22
@@ -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<UserMessage>,
|
||||
// Pending notification to show when unfocused on next Draw
|
||||
pending_notification: Option<Notification>,
|
||||
// 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<String>) {
|
||||
// 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<dyn history_cell::HistoryCell> = 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<ratatui::text::Line<'static>> =
|
||||
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<ratatui::text::Line<'static>> = 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);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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<Line<'static>> {
|
||||
// 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 {
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user