diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 59b3b6ae..b3b75ec7 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -11,6 +11,7 @@ use std::time::Duration; use crate::AuthManager; use crate::client_common::REVIEW_PROMPT; use crate::event_mapping::map_response_item_to_event_messages; +use crate::review_format::format_review_findings_block; use async_channel::Receiver; use async_channel::Sender; use codex_apply_patch::ApplyPatchAction; @@ -3259,7 +3260,8 @@ fn convert_call_tool_result_to_function_call_output_payload( } } -/// Emits an ExitedReviewMode Event with optional ReviewOutput. +/// Emits an ExitedReviewMode Event with optional ReviewOutput, +/// and records a developer message with the review output. async fn exit_review_mode( session: Arc, task_sub_id: String, @@ -3267,9 +3269,50 @@ async fn exit_review_mode( ) { let event = Event { id: task_sub_id, - msg: EventMsg::ExitedReviewMode(ExitedReviewModeEvent { review_output }), + msg: EventMsg::ExitedReviewMode(ExitedReviewModeEvent { + review_output: review_output.clone(), + }), }; session.send_event(event).await; + + let mut user_message = String::new(); + if let Some(out) = review_output { + let mut findings_str = String::new(); + let text = out.overall_explanation.trim(); + if !text.is_empty() { + findings_str.push_str(text); + } + if !out.findings.is_empty() { + let block = format_review_findings_block(&out.findings, None); + findings_str.push_str(&format!("\n{block}")); + } + user_message.push_str(&format!( + r#" + User initiated a review task. Here's the full review output from reviewer model. User may select one or more comments to resolve. + review + + {findings_str} + + +"#)); + } else { + user_message.push_str(r#" + User initiated a review task, but was interrupted. If user asks about this, tell them to re-initiate a review with `/review` and wait for it to complete. + review + + None. + + +"#); + } + + session + .record_conversation_items(&[ResponseItem::Message { + id: None, + role: "user".to_string(), + content: vec![ContentItem::InputText { text: user_message }], + }]) + .await; } #[cfg(test)] diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index aa2e176b..e024effb 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -46,6 +46,7 @@ pub use model_provider_info::built_in_model_providers; pub use model_provider_info::create_oss_provider_with_base_url; mod conversation_manager; mod event_mapping; +pub mod review_format; pub use codex_protocol::protocol::InitialHistory; pub use conversation_manager::ConversationManager; pub use conversation_manager::NewConversation; diff --git a/codex-rs/core/src/review_format.rs b/codex-rs/core/src/review_format.rs new file mode 100644 index 00000000..272010d5 --- /dev/null +++ b/codex-rs/core/src/review_format.rs @@ -0,0 +1,55 @@ +use crate::protocol::ReviewFinding; + +// Note: We keep this module UI-agnostic. It returns plain strings that +// higher layers (e.g., TUI) may style as needed. + +fn format_location(item: &ReviewFinding) -> String { + let path = item.code_location.absolute_file_path.display(); + let start = item.code_location.line_range.start; + let end = item.code_location.line_range.end; + format!("{path}:{start}-{end}") +} + +/// Format a full review findings block as plain text lines. +/// +/// - When `selection` is `Some`, each item line includes a checkbox marker: +/// "[x]" for selected items and "[ ]" for unselected. Missing indices +/// default to selected. +/// - When `selection` is `None`, the marker is omitted and a simple bullet is +/// rendered ("- Title — path:start-end"). +pub fn format_review_findings_block( + findings: &[ReviewFinding], + selection: Option<&[bool]>, +) -> String { + let mut lines: Vec = Vec::new(); + + // Header + let header = if findings.len() > 1 { + "Full review comments:" + } else { + "Review comment:" + }; + lines.push(header.to_string()); + + for (idx, item) in findings.iter().enumerate() { + lines.push(String::new()); + + let title = &item.title; + let location = format_location(item); + + if let Some(flags) = selection { + // Default to selected if index is out of bounds. + let checked = flags.get(idx).copied().unwrap_or(true); + let marker = if checked { "[x]" } else { "[ ]" }; + lines.push(format!("- {marker} {title} — {location}")); + } else { + lines.push(format!("- {title} — {location}")); + } + + for body_line in item.body.lines() { + lines.push(format!(" {body_line}")); + } + } + + lines.join("\n") +} diff --git a/codex-rs/core/tests/suite/review.rs b/codex-rs/core/tests/suite/review.rs index 1ea56d5a..a20807e4 100644 --- a/codex-rs/core/tests/suite/review.rs +++ b/codex-rs/core/tests/suite/review.rs @@ -1,10 +1,13 @@ use codex_core::CodexAuth; use codex_core::CodexConversation; +use codex_core::ContentItem; use codex_core::ConversationManager; use codex_core::ModelProviderInfo; use codex_core::REVIEW_PROMPT; +use codex_core::ResponseItem; use codex_core::built_in_model_providers; use codex_core::config::Config; +use codex_core::protocol::ConversationPathResponseEvent; use codex_core::protocol::ENVIRONMENT_CONTEXT_OPEN_TAG; use codex_core::protocol::EventMsg; use codex_core::protocol::ExitedReviewModeEvent; @@ -15,6 +18,8 @@ use codex_core::protocol::ReviewFinding; use codex_core::protocol::ReviewLineRange; use codex_core::protocol::ReviewOutputEvent; use codex_core::protocol::ReviewRequest; +use codex_core::protocol::RolloutItem; +use codex_core::protocol::RolloutLine; use codex_core::spawn::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; use core_test_support::load_default_config_for_test; use core_test_support::load_sse_fixture_with_id_from_str; @@ -117,6 +122,46 @@ async fn review_op_emits_lifecycle_and_review_output() { assert_eq!(expected, review); let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + // Also verify that a user message with the header and a formatted finding + // was recorded back in the parent session's rollout. + codex.submit(Op::GetPath).await.unwrap(); + let history_event = + wait_for_event(&codex, |ev| matches!(ev, EventMsg::ConversationPath(_))).await; + let path = match history_event { + EventMsg::ConversationPath(ConversationPathResponseEvent { path, .. }) => path, + other => panic!("expected ConversationPath event, got {other:?}"), + }; + let text = std::fs::read_to_string(&path).expect("read rollout file"); + + let mut saw_header = false; + let mut saw_finding_line = false; + for line in text.lines() { + if line.trim().is_empty() { + continue; + } + let v: serde_json::Value = serde_json::from_str(line).expect("jsonl line"); + let rl: RolloutLine = serde_json::from_value(v).expect("rollout line"); + if let RolloutItem::ResponseItem(ResponseItem::Message { role, content, .. }) = rl.item + && role == "user" + { + for c in content { + if let ContentItem::InputText { text } = c { + if text.contains("full review output from reviewer model") { + saw_header = true; + } + if text.contains("- Prefer Stylize helpers — /tmp/file.rs:10-20") { + saw_finding_line = true; + } + } + } + } + } + assert!(saw_header, "user header missing from rollout"); + assert!( + saw_finding_line, + "formatted finding line missing from rollout" + ); + server.verify().await; } @@ -452,6 +497,43 @@ async fn review_input_isolated_from_parent_history() { format!("{REVIEW_PROMPT}\n\n---\n\nNow, here's your task: Please review only this",) ); + // Also verify that a user interruption note was recorded in the rollout. + codex.submit(Op::GetPath).await.unwrap(); + let history_event = + wait_for_event(&codex, |ev| matches!(ev, EventMsg::ConversationPath(_))).await; + let path = match history_event { + EventMsg::ConversationPath(ConversationPathResponseEvent { path, .. }) => path, + other => panic!("expected ConversationPath event, got {other:?}"), + }; + let text = std::fs::read_to_string(&path).expect("read rollout file"); + let mut saw_interruption_message = false; + for line in text.lines() { + if line.trim().is_empty() { + continue; + } + let v: serde_json::Value = serde_json::from_str(line).expect("jsonl line"); + let rl: RolloutLine = serde_json::from_value(v).expect("rollout line"); + if let RolloutItem::ResponseItem(ResponseItem::Message { role, content, .. }) = rl.item + && role == "user" + { + for c in content { + if let ContentItem::InputText { text } = c + && text.contains("User initiated a review task, but was interrupted.") + { + saw_interruption_message = true; + break; + } + } + } + if saw_interruption_message { + break; + } + } + assert!( + saw_interruption_message, + "expected user interruption message in rollout" + ); + server.verify().await; }