diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index db9bed111..f5408a622 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -129,6 +129,10 @@ client_request_definitions! { params: v2::TurnInterruptParams, response: v2::TurnInterruptResponse, }, + ReviewStart => "review/start" { + params: v2::ReviewStartParams, + response: v2::TurnStartResponse, + }, ModelList => "model/list" { params: v2::ModelListParams, diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index a2b9cee3f..f4a6d636a 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -562,6 +562,45 @@ pub struct TurnStartParams { pub summary: Option, } +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct ReviewStartParams { + pub thread_id: String, + pub target: ReviewTarget, + + /// When true, also append the final review message to the original thread. + #[serde(default)] + pub append_to_original_thread: bool, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] +#[serde(tag = "type", rename_all = "camelCase")] +#[ts(tag = "type", export_to = "v2/")] +pub enum ReviewTarget { + /// Review the working tree: staged, unstaged, and untracked files. + UncommittedChanges, + + /// Review changes between the current branch and the given base branch. + #[serde(rename_all = "camelCase")] + #[ts(rename_all = "camelCase")] + BaseBranch { branch: String }, + + /// Review the changes introduced by a specific commit. + #[serde(rename_all = "camelCase")] + #[ts(rename_all = "camelCase")] + Commit { + sha: String, + /// Optional human-readable label (e.g., commit subject) for UIs. + title: Option, + }, + + /// Arbitrary instructions, equivalent to the old free-form prompt. + #[serde(rename_all = "camelCase")] + #[ts(rename_all = "camelCase")] + Custom { instructions: String }, +} + #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 5f9b87458..35653099a 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -65,6 +65,7 @@ The JSON-RPC API exposes dedicated methods for managing Codex conversations. Thr - `thread/archive` — move a thread’s rollout file into the archived directory; returns `{}` on success. - `turn/start` — add user input to a thread and begin Codex generation; responds with the initial `turn` object and streams `turn/started`, `item/*`, and `turn/completed` notifications. - `turn/interrupt` — request cancellation of an in-flight turn by `(thread_id, turn_id)`; success is an empty `{}` response and the turn finishes with `status: "interrupted"`. +- `review/start` — kick off Codex’s automated reviewer for a thread; responds like `turn/start` and emits a `item/completed` notification with a `codeReview` item when results are ready. ### 1) Start or resume a thread @@ -181,6 +182,58 @@ You can cancel a running Turn with `turn/interrupt`. The server requests cancellations for running subprocesses, then emits a `turn/completed` event with `status: "interrupted"`. Rely on the `turn/completed` to know when Codex-side cleanup is done. +### 6) Request a code review + +Use `review/start` to run Codex’s reviewer on the currently checked-out project. The request takes the thread id plus a `target` describing what should be reviewed: + +- `{"type":"uncommittedChanges"}` — staged, unstaged, and untracked files. +- `{"type":"baseBranch","branch":"main"}` — diff against the provided branch’s upstream (see prompt for the exact `git merge-base`/`git diff` instructions Codex will run). +- `{"type":"commit","sha":"abc1234","title":"Optional subject"}` — review a specific commit. +- `{"type":"custom","instructions":"Free-form reviewer instructions"}` — fallback prompt equivalent to the legacy manual review request. +- `appendToOriginalThread` (bool, default `false`) — when `true`, Codex also records a final assistant-style message with the review summary in the original thread. When `false`, only the `codeReview` item is emitted for the review run and no extra message is added to the original thread. + +Example request/response: + +```json +{ "method": "review/start", "id": 40, "params": { + "threadId": "thr_123", + "appendToOriginalThread": true, + "target": { "type": "commit", "sha": "1234567deadbeef", "title": "Polish tui colors" } +} } +{ "id": 40, "result": { "turn": { + "id": "turn_900", + "status": "inProgress", + "items": [ + { "type": "userMessage", "id": "turn_900", "content": [ { "type": "text", "text": "Review commit 1234567: Polish tui colors" } ] } + ], + "error": null +} } } +``` + +Codex streams the usual `turn/started` notification followed by an `item/started` +with the same `codeReview` item id so clients can show progress: + +```json +{ "method": "item/started", "params": { "item": { + "type": "codeReview", + "id": "turn_900", + "review": "current changes" +} } } +``` + +When the reviewer finishes, the server emits `item/completed` containing the same +`codeReview` item with the final review text: + +```json +{ "method": "item/completed", "params": { "item": { + "type": "codeReview", + "id": "turn_900", + "review": "Looks solid overall...\n\n- Prefer Stylize helpers — app.rs:10-20\n ..." +} } } +``` + +The `review` string is plain text that already bundles the overall explanation plus a bullet list for each structured finding (matching `ThreadItem::CodeReview` in the generated schema). Use this notification to render the reviewer output in your client. + ## Auth endpoints The JSON-RPC auth/account surface exposes request/response methods plus server-initiated notifications (no `id`). Use these to determine auth state, start or cancel logins, logout, and inspect ChatGPT rate limits. diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 8ed343f03..ce4f7590e 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -38,7 +38,9 @@ use codex_core::protocol::McpToolCallBeginEvent; use codex_core::protocol::McpToolCallEndEvent; use codex_core::protocol::Op; use codex_core::protocol::ReviewDecision; +use codex_core::review_format::format_review_findings_block; use codex_protocol::ConversationId; +use codex_protocol::protocol::ReviewOutputEvent; use std::convert::TryFrom; use std::sync::Arc; use tokio::sync::oneshot; @@ -189,6 +191,17 @@ pub(crate) async fn apply_bespoke_event_handling( .await; } } + EventMsg::EnteredReviewMode(review_request) => { + let notification = ItemStartedNotification { + item: ThreadItem::CodeReview { + id: event_id.clone(), + review: review_request.user_facing_hint, + }, + }; + outgoing + .send_server_notification(ServerNotification::ItemStarted(notification)) + .await; + } EventMsg::ItemStarted(item_started_event) => { let item: ThreadItem = item_started_event.item.clone().into(); let notification = ItemStartedNotification { item }; @@ -203,6 +216,21 @@ pub(crate) async fn apply_bespoke_event_handling( .send_server_notification(ServerNotification::ItemCompleted(notification)) .await; } + EventMsg::ExitedReviewMode(review_event) => { + let review_text = match review_event.review_output { + Some(output) => render_review_output_text(&output), + None => REVIEW_FALLBACK_MESSAGE.to_string(), + }; + let notification = ItemCompletedNotification { + item: ThreadItem::CodeReview { + id: event_id, + review: review_text, + }, + }; + outgoing + .send_server_notification(ServerNotification::ItemCompleted(notification)) + .await; + } EventMsg::ExecCommandBegin(exec_command_begin_event) => { let item = ThreadItem::CommandExecution { id: exec_command_begin_event.call_id.clone(), @@ -382,6 +410,28 @@ async fn on_exec_approval_response( } } +const REVIEW_FALLBACK_MESSAGE: &str = "Reviewer failed to output a response."; + +fn render_review_output_text(output: &ReviewOutputEvent) -> String { + let mut sections = Vec::new(); + let explanation = output.overall_explanation.trim(); + if !explanation.is_empty() { + sections.push(explanation.to_string()); + } + if !output.findings.is_empty() { + let findings = format_review_findings_block(&output.findings, None); + let trimmed = findings.trim(); + if !trimmed.is_empty() { + sections.push(trimmed.to_string()); + } + } + if sections.is_empty() { + REVIEW_FALLBACK_MESSAGE.to_string() + } else { + sections.join("\n\n") + } +} + async fn on_command_execution_request_approval_response( event_id: String, receiver: oneshot::Receiver, diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index c5fa2a7fa..017245807 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -60,6 +60,8 @@ use codex_app_server_protocol::RemoveConversationSubscriptionResponse; use codex_app_server_protocol::RequestId; use codex_app_server_protocol::ResumeConversationParams; use codex_app_server_protocol::ResumeConversationResponse; +use codex_app_server_protocol::ReviewStartParams; +use codex_app_server_protocol::ReviewTarget; use codex_app_server_protocol::SandboxMode; use codex_app_server_protocol::SendUserMessageParams; use codex_app_server_protocol::SendUserMessageResponse; @@ -115,6 +117,7 @@ use codex_core::git_info::git_diff_to_remote; use codex_core::parse_cursor; use codex_core::protocol::EventMsg; use codex_core::protocol::Op; +use codex_core::protocol::ReviewRequest; use codex_core::read_head_for_summary; use codex_feedback::CodexFeedback; use codex_login::ServerOptions as LoginServerOptions; @@ -232,6 +235,91 @@ impl CodexMessageProcessor { } } + fn review_request_from_target( + target: ReviewTarget, + append_to_original_thread: bool, + ) -> Result<(ReviewRequest, String), JSONRPCErrorError> { + fn invalid_request(message: String) -> JSONRPCErrorError { + JSONRPCErrorError { + code: INVALID_REQUEST_ERROR_CODE, + message, + data: None, + } + } + + match target { + // TODO(jif) those messages will be extracted in a follow-up PR. + ReviewTarget::UncommittedChanges => Ok(( + ReviewRequest { + prompt: "Review the current code changes (staged, unstaged, and untracked files) and provide prioritized findings.".to_string(), + user_facing_hint: "current changes".to_string(), + append_to_original_thread, + }, + "Review uncommitted changes".to_string(), + )), + ReviewTarget::BaseBranch { branch } => { + let branch = branch.trim().to_string(); + if branch.is_empty() { + return Err(invalid_request("branch must not be empty".to_string())); + } + let prompt = format!("Review the code changes against the base branch '{branch}'. Start by finding the merge diff between the current branch and {branch}'s upstream e.g. (`git merge-base HEAD \"$(git rev-parse --abbrev-ref \"{branch}@{{upstream}}\")\"`), then run `git diff` against that SHA to see what changes we would merge into the {branch} branch. Provide prioritized, actionable findings."); + let hint = format!("changes against '{branch}'"); + let display = format!("Review changes against base branch '{branch}'"); + Ok(( + ReviewRequest { + prompt, + user_facing_hint: hint, + append_to_original_thread, + }, + display, + )) + } + ReviewTarget::Commit { sha, title } => { + let sha = sha.trim().to_string(); + if sha.is_empty() { + return Err(invalid_request("sha must not be empty".to_string())); + } + let brief_title = title + .map(|t| t.trim().to_string()) + .filter(|t| !t.is_empty()); + let prompt = if let Some(title) = brief_title.clone() { + format!("Review the code changes introduced by commit {sha} (\"{title}\"). Provide prioritized, actionable findings.") + } else { + format!("Review the code changes introduced by commit {sha}. Provide prioritized, actionable findings.") + }; + let short_sha = sha.chars().take(7).collect::(); + let hint = format!("commit {short_sha}"); + let display = if let Some(title) = brief_title { + format!("Review commit {short_sha}: {title}") + } else { + format!("Review commit {short_sha}") + }; + Ok(( + ReviewRequest { + prompt, + user_facing_hint: hint, + append_to_original_thread, + }, + display, + )) + } + ReviewTarget::Custom { instructions } => { + let trimmed = instructions.trim().to_string(); + if trimmed.is_empty() { + return Err(invalid_request("instructions must not be empty".to_string())); + } + Ok(( + ReviewRequest { + prompt: trimmed.clone(), + user_facing_hint: trimmed.clone(), + append_to_original_thread, + }, + trimmed, + )) + } + } + } + pub async fn process_request(&mut self, request: ClientRequest) { match request { ClientRequest::Initialize { .. } => { @@ -263,6 +351,9 @@ impl CodexMessageProcessor { ClientRequest::TurnInterrupt { request_id, params } => { self.turn_interrupt(request_id, params).await; } + ClientRequest::ReviewStart { request_id, params } => { + self.review_start(request_id, params).await; + } ClientRequest::NewConversation { request_id, params } => { // Do not tokio::spawn() to process new_conversation() // asynchronously because we need to ensure the conversation is @@ -2342,6 +2433,65 @@ impl CodexMessageProcessor { } } + async fn review_start(&self, request_id: RequestId, params: ReviewStartParams) { + let ReviewStartParams { + thread_id, + target, + append_to_original_thread, + } = params; + let (_, conversation) = match self.conversation_from_thread_id(&thread_id).await { + Ok(v) => v, + Err(error) => { + self.outgoing.send_error(request_id, error).await; + return; + } + }; + + let (review_request, display_text) = + match Self::review_request_from_target(target, append_to_original_thread) { + Ok(value) => value, + Err(err) => { + self.outgoing.send_error(request_id, err).await; + return; + } + }; + + let turn_id = conversation.submit(Op::Review { review_request }).await; + + match turn_id { + Ok(turn_id) => { + let mut items = Vec::new(); + if !display_text.is_empty() { + items.push(ThreadItem::UserMessage { + id: turn_id.clone(), + content: vec![V2UserInput::Text { text: display_text }], + }); + } + let turn = Turn { + id: turn_id.clone(), + items, + status: TurnStatus::InProgress, + error: None, + }; + let response = TurnStartResponse { turn: turn.clone() }; + self.outgoing.send_response(request_id, response).await; + + let notif = TurnStartedNotification { turn }; + self.outgoing + .send_server_notification(ServerNotification::TurnStarted(notif)) + .await; + } + Err(err) => { + let error = JSONRPCErrorError { + code: INTERNAL_ERROR_CODE, + message: format!("failed to start review: {err}"), + data: None, + }; + self.outgoing.send_error(request_id, error).await; + } + } + } + async fn turn_interrupt(&mut self, request_id: RequestId, params: TurnInterruptParams) { let TurnInterruptParams { thread_id, .. } = params; diff --git a/codex-rs/app-server/tests/common/mcp_process.rs b/codex-rs/app-server/tests/common/mcp_process.rs index 75851eda2..920a6fa01 100644 --- a/codex-rs/app-server/tests/common/mcp_process.rs +++ b/codex-rs/app-server/tests/common/mcp_process.rs @@ -35,6 +35,7 @@ use codex_app_server_protocol::NewConversationParams; use codex_app_server_protocol::RemoveConversationListenerParams; use codex_app_server_protocol::RequestId; use codex_app_server_protocol::ResumeConversationParams; +use codex_app_server_protocol::ReviewStartParams; use codex_app_server_protocol::SendUserMessageParams; use codex_app_server_protocol::SendUserTurnParams; use codex_app_server_protocol::ServerRequest; @@ -377,6 +378,15 @@ impl McpProcess { self.send_request("turn/interrupt", params).await } + /// Send a `review/start` JSON-RPC request (v2). + pub async fn send_review_start_request( + &mut self, + params: ReviewStartParams, + ) -> anyhow::Result { + let params = Some(serde_json::to_value(params)?); + self.send_request("review/start", params).await + } + /// Send a `cancelLoginChatGpt` JSON-RPC request. pub async fn send_cancel_login_chat_gpt_request( &mut self, diff --git a/codex-rs/app-server/tests/suite/v2/mod.rs b/codex-rs/app-server/tests/suite/v2/mod.rs index 587afef10..a8594e7ca 100644 --- a/codex-rs/app-server/tests/suite/v2/mod.rs +++ b/codex-rs/app-server/tests/suite/v2/mod.rs @@ -1,6 +1,7 @@ mod account; mod model_list; mod rate_limits; +mod review; mod thread_archive; mod thread_list; mod thread_resume; diff --git a/codex-rs/app-server/tests/suite/v2/review.rs b/codex-rs/app-server/tests/suite/v2/review.rs new file mode 100644 index 000000000..194ed6ae9 --- /dev/null +++ b/codex-rs/app-server/tests/suite/v2/review.rs @@ -0,0 +1,279 @@ +use anyhow::Result; +use app_test_support::McpProcess; +use app_test_support::create_final_assistant_message_sse_response; +use app_test_support::create_mock_chat_completions_server_unchecked; +use app_test_support::to_response; +use codex_app_server_protocol::ItemCompletedNotification; +use codex_app_server_protocol::ItemStartedNotification; +use codex_app_server_protocol::JSONRPCError; +use codex_app_server_protocol::JSONRPCNotification; +use codex_app_server_protocol::JSONRPCResponse; +use codex_app_server_protocol::RequestId; +use codex_app_server_protocol::ReviewStartParams; +use codex_app_server_protocol::ReviewTarget; +use codex_app_server_protocol::ThreadItem; +use codex_app_server_protocol::ThreadStartParams; +use codex_app_server_protocol::ThreadStartResponse; +use codex_app_server_protocol::TurnStartResponse; +use codex_app_server_protocol::TurnStatus; +use serde_json::json; +use tempfile::TempDir; +use tokio::time::timeout; + +const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); +const INVALID_REQUEST_ERROR_CODE: i64 = -32600; + +#[tokio::test] +async fn review_start_runs_review_turn_and_emits_code_review_item() -> Result<()> { + let review_payload = json!({ + "findings": [ + { + "title": "Prefer Stylize helpers", + "body": "Use .dim()/.bold() chaining instead of manual Style.", + "confidence_score": 0.9, + "priority": 1, + "code_location": { + "absolute_file_path": "/tmp/file.rs", + "line_range": {"start": 10, "end": 20} + } + } + ], + "overall_correctness": "good", + "overall_explanation": "Looks solid overall with minor polish suggested.", + "overall_confidence_score": 0.75 + }) + .to_string(); + let responses = vec![create_final_assistant_message_sse_response( + &review_payload, + )?]; + let server = create_mock_chat_completions_server_unchecked(responses).await; + + let codex_home = TempDir::new()?; + create_config_toml(codex_home.path(), &server.uri())?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let thread_id = start_default_thread(&mut mcp).await?; + + let review_req = mcp + .send_review_start_request(ReviewStartParams { + thread_id: thread_id.clone(), + append_to_original_thread: true, + target: ReviewTarget::Commit { + sha: "1234567deadbeef".to_string(), + title: Some("Tidy UI colors".to_string()), + }, + }) + .await?; + let review_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(review_req)), + ) + .await??; + let TurnStartResponse { turn } = to_response::(review_resp)?; + let turn_id = turn.id.clone(); + assert_eq!(turn.status, TurnStatus::InProgress); + assert_eq!(turn.items.len(), 1); + match &turn.items[0] { + ThreadItem::UserMessage { content, .. } => { + assert_eq!(content.len(), 1); + assert!(matches!( + &content[0], + codex_app_server_protocol::UserInput::Text { .. } + )); + } + other => panic!("expected user message, got {other:?}"), + } + + let _started: JSONRPCNotification = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("turn/started"), + ) + .await??; + let item_started: JSONRPCNotification = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("item/started"), + ) + .await??; + let started: ItemStartedNotification = + serde_json::from_value(item_started.params.expect("params must be present"))?; + match started.item { + ThreadItem::CodeReview { id, review } => { + assert_eq!(id, turn_id); + assert_eq!(review, "commit 1234567"); + } + other => panic!("expected code review item, got {other:?}"), + } + + let mut review_body: Option = None; + for _ in 0..5 { + let review_notif: JSONRPCNotification = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("item/completed"), + ) + .await??; + let completed: ItemCompletedNotification = + serde_json::from_value(review_notif.params.expect("params must be present"))?; + match completed.item { + ThreadItem::CodeReview { id, review } => { + assert_eq!(id, turn_id); + review_body = Some(review); + break; + } + ThreadItem::UserMessage { .. } => continue, + other => panic!("unexpected item/completed payload: {other:?}"), + } + } + + let review = review_body.expect("did not observe a code review item"); + assert!(review.contains("Prefer Stylize helpers")); + assert!(review.contains("/tmp/file.rs:10-20")); + + Ok(()) +} + +#[tokio::test] +async fn review_start_rejects_empty_base_branch() -> Result<()> { + let server = create_mock_chat_completions_server_unchecked(vec![]).await; + let codex_home = TempDir::new()?; + create_config_toml(codex_home.path(), &server.uri())?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + let thread_id = start_default_thread(&mut mcp).await?; + + let request_id = mcp + .send_review_start_request(ReviewStartParams { + thread_id, + append_to_original_thread: true, + target: ReviewTarget::BaseBranch { + branch: " ".to_string(), + }, + }) + .await?; + let error: JSONRPCError = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_error_message(RequestId::Integer(request_id)), + ) + .await??; + assert_eq!(error.error.code, INVALID_REQUEST_ERROR_CODE); + assert!( + error.error.message.contains("branch must not be empty"), + "unexpected message: {}", + error.error.message + ); + + Ok(()) +} + +#[tokio::test] +async fn review_start_rejects_empty_commit_sha() -> Result<()> { + let server = create_mock_chat_completions_server_unchecked(vec![]).await; + let codex_home = TempDir::new()?; + create_config_toml(codex_home.path(), &server.uri())?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + let thread_id = start_default_thread(&mut mcp).await?; + + let request_id = mcp + .send_review_start_request(ReviewStartParams { + thread_id, + append_to_original_thread: true, + target: ReviewTarget::Commit { + sha: "\t".to_string(), + title: None, + }, + }) + .await?; + let error: JSONRPCError = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_error_message(RequestId::Integer(request_id)), + ) + .await??; + assert_eq!(error.error.code, INVALID_REQUEST_ERROR_CODE); + assert!( + error.error.message.contains("sha must not be empty"), + "unexpected message: {}", + error.error.message + ); + + Ok(()) +} + +#[tokio::test] +async fn review_start_rejects_empty_custom_instructions() -> Result<()> { + let server = create_mock_chat_completions_server_unchecked(vec![]).await; + let codex_home = TempDir::new()?; + create_config_toml(codex_home.path(), &server.uri())?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + let thread_id = start_default_thread(&mut mcp).await?; + + let request_id = mcp + .send_review_start_request(ReviewStartParams { + thread_id, + append_to_original_thread: true, + target: ReviewTarget::Custom { + instructions: "\n\n".to_string(), + }, + }) + .await?; + let error: JSONRPCError = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_error_message(RequestId::Integer(request_id)), + ) + .await??; + assert_eq!(error.error.code, INVALID_REQUEST_ERROR_CODE); + assert!( + error + .error + .message + .contains("instructions must not be empty"), + "unexpected message: {}", + error.error.message + ); + + Ok(()) +} + +async fn start_default_thread(mcp: &mut McpProcess) -> Result { + let thread_req = mcp + .send_thread_start_request(ThreadStartParams { + model: Some("mock-model".to_string()), + ..Default::default() + }) + .await?; + let thread_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(thread_req)), + ) + .await??; + let ThreadStartResponse { thread } = to_response::(thread_resp)?; + Ok(thread.id) +} + +fn create_config_toml(codex_home: &std::path::Path, server_uri: &str) -> std::io::Result<()> { + let config_toml = codex_home.join("config.toml"); + std::fs::write( + config_toml, + format!( + r#" +model = "mock-model" +approval_policy = "never" +sandbox_mode = "read-only" + +model_provider = "mock_provider" + +[model_providers.mock_provider] +name = "Mock provider" +base_url = "{server_uri}/v1" +wire_api = "chat" +request_max_retries = 0 +stream_max_retries = 0 +"# + ), + ) +} diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index e308601c5..19ef0a8c2 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1774,7 +1774,12 @@ async fn spawn_review_thread( text: review_prompt, }]; let tc = Arc::new(review_turn_context); - sess.spawn_task(tc.clone(), input, ReviewTask).await; + sess.spawn_task( + tc.clone(), + input, + ReviewTask::new(review_request.append_to_original_thread), + ) + .await; // Announce entering review mode so UIs can switch modes. sess.send_event(&tc, EventMsg::EnteredReviewMode(review_request)) @@ -2780,7 +2785,8 @@ mod tests { let input = vec![UserInput::Text { text: "start review".to_string(), }]; - sess.spawn_task(Arc::clone(&tc), input, ReviewTask).await; + sess.spawn_task(Arc::clone(&tc), input, ReviewTask::new(true)) + .await; sess.abort_all_tasks(TurnAbortReason::Interrupted).await; diff --git a/codex-rs/core/src/tasks/review.rs b/codex-rs/core/src/tasks/review.rs index e0bb7d4e9..14a95dba5 100644 --- a/codex-rs/core/src/tasks/review.rs +++ b/codex-rs/core/src/tasks/review.rs @@ -23,8 +23,18 @@ use codex_protocol::user_input::UserInput; use super::SessionTask; use super::SessionTaskContext; -#[derive(Clone, Copy, Default)] -pub(crate) struct ReviewTask; +#[derive(Clone, Copy)] +pub(crate) struct ReviewTask { + append_to_original_thread: bool, +} + +impl ReviewTask { + pub(crate) fn new(append_to_original_thread: bool) -> Self { + Self { + append_to_original_thread, + } + } +} #[async_trait] impl SessionTask for ReviewTask { @@ -52,13 +62,25 @@ impl SessionTask for ReviewTask { None => None, }; if !cancellation_token.is_cancelled() { - exit_review_mode(session.clone_session(), output.clone(), ctx.clone()).await; + exit_review_mode( + session.clone_session(), + output.clone(), + ctx.clone(), + self.append_to_original_thread, + ) + .await; } None } async fn abort(&self, session: Arc, ctx: Arc) { - exit_review_mode(session.clone_session(), None, ctx).await; + exit_review_mode( + session.clone_session(), + None, + ctx, + self.append_to_original_thread, + ) + .await; } } @@ -175,32 +197,35 @@ pub(crate) async fn exit_review_mode( session: Arc, review_output: Option, ctx: Arc, + append_to_original_thread: bool, ) { - let user_message = if let Some(out) = review_output.clone() { - 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}")); - } - crate::client_common::REVIEW_EXIT_SUCCESS_TMPL.replace("{results}", &findings_str) - } else { - crate::client_common::REVIEW_EXIT_INTERRUPTED_TMPL.to_string() - }; + if append_to_original_thread { + let user_message = if let Some(out) = review_output.clone() { + 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}")); + } + crate::client_common::REVIEW_EXIT_SUCCESS_TMPL.replace("{results}", &findings_str) + } else { + crate::client_common::REVIEW_EXIT_INTERRUPTED_TMPL.to_string() + }; - session - .record_conversation_items( - &ctx, - &[ResponseItem::Message { - id: None, - role: "user".to_string(), - content: vec![ContentItem::InputText { text: user_message }], - }], - ) - .await; + session + .record_conversation_items( + &ctx, + &[ResponseItem::Message { + id: None, + role: "user".to_string(), + content: vec![ContentItem::InputText { text: user_message }], + }], + ) + .await; + } session .send_event( ctx.as_ref(), diff --git a/codex-rs/core/tests/suite/codex_delegate.rs b/codex-rs/core/tests/suite/codex_delegate.rs index c6ece7fe5..6339bfa71 100644 --- a/codex-rs/core/tests/suite/codex_delegate.rs +++ b/codex-rs/core/tests/suite/codex_delegate.rs @@ -70,6 +70,7 @@ async fn codex_delegate_forwards_exec_approval_and_proceeds_on_approval() { review_request: ReviewRequest { prompt: "Please review".to_string(), user_facing_hint: "review".to_string(), + append_to_original_thread: true, }, }) .await @@ -145,6 +146,7 @@ async fn codex_delegate_forwards_patch_approval_and_proceeds_on_decision() { review_request: ReviewRequest { prompt: "Please review".to_string(), user_facing_hint: "review".to_string(), + append_to_original_thread: true, }, }) .await @@ -199,6 +201,7 @@ async fn codex_delegate_ignores_legacy_deltas() { review_request: ReviewRequest { prompt: "Please review".to_string(), user_facing_hint: "review".to_string(), + append_to_original_thread: true, }, }) .await diff --git a/codex-rs/core/tests/suite/review.rs b/codex-rs/core/tests/suite/review.rs index 9c3f812c2..3904f18f8 100644 --- a/codex-rs/core/tests/suite/review.rs +++ b/codex-rs/core/tests/suite/review.rs @@ -82,6 +82,7 @@ async fn review_op_emits_lifecycle_and_review_output() { review_request: ReviewRequest { prompt: "Please review my changes".to_string(), user_facing_hint: "my changes".to_string(), + append_to_original_thread: true, }, }) .await @@ -178,6 +179,7 @@ async fn review_op_with_plain_text_emits_review_fallback() { review_request: ReviewRequest { prompt: "Plain text review".to_string(), user_facing_hint: "plain text review".to_string(), + append_to_original_thread: true, }, }) .await @@ -236,6 +238,7 @@ async fn review_filters_agent_message_related_events() { review_request: ReviewRequest { prompt: "Filter streaming events".to_string(), user_facing_hint: "Filter streaming events".to_string(), + append_to_original_thread: true, }, }) .await @@ -320,6 +323,7 @@ async fn review_does_not_emit_agent_message_on_structured_output() { review_request: ReviewRequest { prompt: "check structured".to_string(), user_facing_hint: "check structured".to_string(), + append_to_original_thread: true, }, }) .await @@ -373,6 +377,7 @@ async fn review_uses_custom_review_model_from_config() { review_request: ReviewRequest { prompt: "use custom model".to_string(), user_facing_hint: "use custom model".to_string(), + append_to_original_thread: true, }, }) .await @@ -490,6 +495,7 @@ async fn review_input_isolated_from_parent_history() { review_request: ReviewRequest { prompt: review_prompt.clone(), user_facing_hint: review_prompt.clone(), + append_to_original_thread: true, }, }) .await @@ -602,6 +608,7 @@ async fn review_history_does_not_leak_into_parent_session() { review_request: ReviewRequest { prompt: "Start a review".to_string(), user_facing_hint: "Start a review".to_string(), + append_to_original_thread: true, }, }) .await diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 7f5e5228d..934b6ad28 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -1173,11 +1173,13 @@ pub struct GitInfo { pub repository_url: Option, } -/// Review request sent to the review session. #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, JsonSchema, TS)] +/// Review request sent to the review session. pub struct ReviewRequest { pub prompt: String, pub user_facing_hint: String, + #[serde(default)] + pub append_to_original_thread: bool, } /// Structured review result produced by a child review session. diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 4e9f496ba..c17eea92d 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -2711,6 +2711,7 @@ impl ChatWidget { review_request: ReviewRequest { prompt: "Review the current code changes (staged, unstaged, and untracked files) and provide prioritized findings.".to_string(), user_facing_hint: "current changes".to_string(), + append_to_original_thread: true, }, })); }, @@ -2767,6 +2768,7 @@ impl ChatWidget { "Review the code changes against the base branch '{branch}'. Start by finding the merge diff between the current branch and {branch}'s upstream e.g. (`git merge-base HEAD \"$(git rev-parse --abbrev-ref \"{branch}@{{upstream}}\")\"`), then run `git diff` against that SHA to see what changes we would merge into the {branch} branch. Provide prioritized, actionable findings." ), user_facing_hint: format!("changes against '{branch}'"), + append_to_original_thread: true, }, })); })], @@ -2807,6 +2809,7 @@ impl ChatWidget { review_request: ReviewRequest { prompt, user_facing_hint: hint, + append_to_original_thread: true, }, })); })], @@ -2841,6 +2844,7 @@ impl ChatWidget { review_request: ReviewRequest { prompt: trimmed.clone(), user_facing_hint: trimmed, + append_to_original_thread: true, }, })); }), @@ -3051,6 +3055,7 @@ pub(crate) fn show_review_commit_picker_with_entries( review_request: ReviewRequest { prompt, user_facing_hint: hint, + append_to_original_thread: true, }, })); })], diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 013b5c231..d6d8f0248 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -145,6 +145,7 @@ fn entered_review_mode_uses_request_hint() { msg: EventMsg::EnteredReviewMode(ReviewRequest { prompt: "Review the latest changes".to_string(), user_facing_hint: "feature branch".to_string(), + append_to_original_thread: true, }), }); @@ -164,6 +165,7 @@ fn entered_review_mode_defaults_to_current_changes_banner() { msg: EventMsg::EnteredReviewMode(ReviewRequest { prompt: "Review the current changes".to_string(), user_facing_hint: "current changes".to_string(), + append_to_original_thread: true, }), });