From a40a28bc734f93b41258dad1f0653d73ea2a88e1 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Thu, 13 Nov 2025 19:10:56 +0100 Subject: [PATCH 01/10] feat: review in app server --- .../src/protocol/common.rs | 4 + .../app-server-protocol/src/protocol/v2.rs | 29 ++ codex-rs/app-server/README.md | 39 +++ .../app-server/src/codex_message_processor.rs | 172 ++++++++++++ .../app-server/tests/common/mcp_process.rs | 10 + codex-rs/app-server/tests/suite/v2/mod.rs | 1 + codex-rs/app-server/tests/suite/v2/review.rs | 258 ++++++++++++++++++ 7 files changed, 513 insertions(+) create mode 100644 codex-rs/app-server/tests/suite/v2/review.rs diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index f754ece5620..8fb7345fb9b 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -137,6 +137,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 10196884aa8..07ddd373747 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -449,6 +449,35 @@ 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, +} + +#[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. + BaseBranch { branch: String }, + + /// Review the changes introduced by a specific commit. + Commit { + sha: String, + /// Optional human-readable label (e.g., commit subject) for UIs. + title: Option, + }, + + /// Arbitrary instructions, equivalent to the old free-form prompt. + 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 2efd52a0fb8..4405e26e9af 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -47,6 +47,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 @@ -163,6 +164,44 @@ 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. +### 5) 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. + +Example request/response: + +```json +{ "method": "review/start", "id": 40, "params": { + "threadId": "thr_123", + "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. When the reviewer finishes, the server emits `item/completed` containing a `codeReview` item: + +```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/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 23fbe59a90c..8781158a125 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -68,6 +68,8 @@ use codex_app_server_protocol::RequestId; use codex_app_server_protocol::Result as JsonRpcResult; 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; @@ -129,7 +131,10 @@ use codex_core::protocol::EventMsg; use codex_core::protocol::ExecApprovalRequestEvent; use codex_core::protocol::Op; use codex_core::protocol::ReviewDecision; +use codex_core::protocol::ReviewOutputEvent; +use codex_core::protocol::ReviewRequest; use codex_core::read_head_for_summary; +use codex_core::review_format::format_review_findings_block; use codex_feedback::CodexFeedback; use codex_login::ServerOptions as LoginServerOptions; use codex_login::ShutdownHandle; @@ -246,6 +251,79 @@ impl CodexMessageProcessor { } } + fn review_request_from_target( + target: ReviewTarget, + ) -> Result<(ReviewRequest, String), JSONRPCErrorError> { + fn invalid_request(message: String) -> JSONRPCErrorError { + JSONRPCErrorError { + code: INVALID_REQUEST_ERROR_CODE, + message, + data: None, + } + } + + match target { + 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(), + }, + "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 }, 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 }, 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(), + }, + trimmed, + )) + } + } + } + pub async fn process_request(&mut self, request: ClientRequest) { match request { ClientRequest::Initialize { .. } => { @@ -277,6 +355,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 @@ -2356,6 +2437,60 @@ impl CodexMessageProcessor { } } + async fn review_start(&self, request_id: RequestId, params: ReviewStartParams) { + let ReviewStartParams { thread_id, target } = 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) { + 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; @@ -2732,6 +2867,21 @@ 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; + } // If this is a TurnAborted, reply to any pending interrupt requests. EventMsg::TurnAborted(turn_aborted_event) => { let pending = { @@ -2760,6 +2910,28 @@ async fn apply_bespoke_event_handling( } } +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 derive_config_from_params( overrides: ConfigOverrides, cli_overrides: Option>, diff --git a/codex-rs/app-server/tests/common/mcp_process.rs b/codex-rs/app-server/tests/common/mcp_process.rs index 75851eda2cc..920a6fa01d7 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 587afef1089..a8594e7ca27 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 00000000000..311a2f320c9 --- /dev/null +++ b/codex-rs/app-server/tests/suite/v2/review.rs @@ -0,0 +1,258 @@ +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::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(), + 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)?; + 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 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 { review, .. } => { + 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, + 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, + 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, + 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 +"# + ), + ) +} From 70fdf54122634a08973853d6c2a97ed75d211983 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Thu, 13 Nov 2025 19:16:53 +0100 Subject: [PATCH 02/10] Clippy --- codex-rs/app-server/tests/suite/v2/review.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/codex-rs/app-server/tests/suite/v2/review.rs b/codex-rs/app-server/tests/suite/v2/review.rs index 311a2f320c9..f8733099865 100644 --- a/codex-rs/app-server/tests/suite/v2/review.rs +++ b/codex-rs/app-server/tests/suite/v2/review.rs @@ -80,7 +80,7 @@ async fn review_start_runs_review_turn_and_emits_code_review_item() -> Result<() codex_app_server_protocol::UserInput::Text { .. } )); } - other => panic!("expected user message, got {:?}", other), + other => panic!("expected user message, got {other:?}"), } let _started: JSONRPCNotification = timeout( @@ -104,7 +104,7 @@ async fn review_start_runs_review_turn_and_emits_code_review_item() -> Result<() break; } ThreadItem::UserMessage { .. } => continue, - other => panic!("unexpected item/completed payload: {:?}", other), + other => panic!("unexpected item/completed payload: {other:?}"), } } From 2547b376295c52fd3ae2e67fc15283e071128a49 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Tue, 18 Nov 2025 14:28:01 +0000 Subject: [PATCH 03/10] Fix merge --- .../app-server/src/bespoke_event_handling.rs | 39 ++++ .../app-server/src/codex_message_processor.rs | 188 ------------------ 2 files changed, 39 insertions(+), 188 deletions(-) diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 8ed343f03a9..8f67488ab30 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -43,6 +43,8 @@ use std::convert::TryFrom; use std::sync::Arc; use tokio::sync::oneshot; use tracing::error; +use codex_core::review_format::format_review_findings_block; +use codex_protocol::protocol::ReviewOutputEvent; type JsonValue = serde_json::Value; @@ -203,6 +205,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 +399,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 755c159eeca..0756b14494c 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -2776,194 +2776,6 @@ impl CodexMessageProcessor { } } -async fn apply_bespoke_event_handling( - event: Event, - conversation_id: ConversationId, - conversation: Arc, - outgoing: Arc, - pending_interrupts: PendingInterrupts, -) { - let Event { id: event_id, msg } = event; - match msg { - EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { - call_id, - changes, - reason, - grant_root, - }) => { - let params = ApplyPatchApprovalParams { - conversation_id, - call_id, - file_changes: changes, - reason, - grant_root, - }; - let rx = outgoing - .send_request(ServerRequestPayload::ApplyPatchApproval(params)) - .await; - // TODO(mbolin): Enforce a timeout so this task does not live indefinitely? - tokio::spawn(async move { - on_patch_approval_response(event_id, rx, conversation).await; - }); - } - EventMsg::AgentMessageContentDelta(event) => { - let notification = AgentMessageDeltaNotification { - item_id: event.item_id, - delta: event.delta, - }; - outgoing - .send_server_notification(ServerNotification::AgentMessageDelta(notification)) - .await; - } - EventMsg::ReasoningContentDelta(event) => { - let notification = ReasoningSummaryTextDeltaNotification { - item_id: event.item_id, - delta: event.delta, - summary_index: event.summary_index, - }; - outgoing - .send_server_notification(ServerNotification::ReasoningSummaryTextDelta( - notification, - )) - .await; - } - EventMsg::ReasoningRawContentDelta(event) => { - let notification = ReasoningTextDeltaNotification { - item_id: event.item_id, - delta: event.delta, - content_index: event.content_index, - }; - outgoing - .send_server_notification(ServerNotification::ReasoningTextDelta(notification)) - .await; - } - EventMsg::AgentReasoningSectionBreak(event) => { - let notification = ReasoningSummaryPartAddedNotification { - item_id: event.item_id, - summary_index: event.summary_index, - }; - outgoing - .send_server_notification(ServerNotification::ReasoningSummaryPartAdded( - notification, - )) - .await; - } - EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent { - call_id, - command, - cwd, - reason, - risk, - parsed_cmd, - }) => { - let params = ExecCommandApprovalParams { - conversation_id, - call_id, - command, - cwd, - reason, - risk, - parsed_cmd, - }; - let rx = outgoing - .send_request(ServerRequestPayload::ExecCommandApproval(params)) - .await; - - // TODO(mbolin): Enforce a timeout so this task does not live indefinitely? - tokio::spawn(async move { - on_exec_approval_response(event_id, rx, conversation).await; - }); - } - EventMsg::TokenCount(token_count_event) => { - if let Some(rate_limits) = token_count_event.rate_limits { - outgoing - .send_server_notification(ServerNotification::AccountRateLimitsUpdated( - AccountRateLimitsUpdatedNotification { - rate_limits: rate_limits.into(), - }, - )) - .await; - } - } - EventMsg::ItemStarted(item_started_event) => { - let item: ThreadItem = item_started_event.item.clone().into(); - let notification = ItemStartedNotification { item }; - outgoing - .send_server_notification(ServerNotification::ItemStarted(notification)) - .await; - } - EventMsg::ItemCompleted(item_completed_event) => { - let item: ThreadItem = item_completed_event.item.clone().into(); - let notification = ItemCompletedNotification { item }; - outgoing - .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; - } - // If this is a TurnAborted, reply to any pending interrupt requests. - EventMsg::TurnAborted(turn_aborted_event) => { - let pending = { - let mut map = pending_interrupts.lock().await; - map.remove(&conversation_id).unwrap_or_default() - }; - if !pending.is_empty() { - for (rid, ver) in pending { - match ver { - ApiVersion::V1 => { - let response = InterruptConversationResponse { - abort_reason: turn_aborted_event.reason.clone(), - }; - outgoing.send_response(rid, response).await; - } - ApiVersion::V2 => { - let response = TurnInterruptResponse {}; - outgoing.send_response(rid, response).await; - } - } - } - } - } - - _ => {} - } -} - -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 derive_config_from_params( overrides: ConfigOverrides, cli_overrides: Option>, From c1a0404d31e9b2a5f2b48d1b62f2b134205883b7 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Tue, 18 Nov 2025 14:36:03 +0000 Subject: [PATCH 04/10] Clippy --- codex-rs/app-server/src/codex_message_processor.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 0756b14494c..c3c4fdcb952 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -117,11 +117,8 @@ 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::ReviewDecision; -use codex_core::protocol::ReviewOutputEvent; use codex_core::protocol::ReviewRequest; use codex_core::read_head_for_summary; -use codex_core::review_format::format_review_findings_block; use codex_feedback::CodexFeedback; use codex_login::ServerOptions as LoginServerOptions; use codex_login::ShutdownHandle; From 1b54babdf2801e117137cbfcef6730b87fdef908 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Tue, 18 Nov 2025 14:58:11 +0000 Subject: [PATCH 05/10] Re-work threads --- .../app-server-protocol/src/protocol/v2.rs | 8 ++ codex-rs/app-server/README.md | 2 + .../app-server/src/bespoke_event_handling.rs | 4 +- .../app-server/src/codex_message_processor.rs | 55 ++++++++----- codex-rs/app-server/tests/suite/v2/review.rs | 4 + codex-rs/core/src/codex.rs | 10 ++- codex-rs/core/src/tasks/review.rs | 81 ++++++++++++------- codex-rs/core/tests/suite/codex_delegate.rs | 3 + codex-rs/core/tests/suite/review.rs | 7 ++ codex-rs/protocol/src/protocol.rs | 4 +- codex-rs/tui/src/chatwidget.rs | 5 ++ codex-rs/tui/src/chatwidget/tests.rs | 2 + 12 files changed, 133 insertions(+), 52 deletions(-) diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index f656f7e72f9..867d3cef6f7 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -66,6 +66,10 @@ v2_enum_from_core!( } ); +fn default_true() -> bool { + true +} + #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] @@ -568,6 +572,10 @@ pub struct TurnStartParams { pub struct ReviewStartParams { pub thread_id: String, pub target: ReviewTarget, + + /// When true (default), also append the final review message to the original thread. + #[serde(default = "default_true")] + pub append_to_original_thread: bool, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index ac150cbfd5f..c48cdc098f5 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -190,12 +190,14 @@ Use `review/start` to run Codex’s reviewer on the currently checked-out projec - `{"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 `true`) — 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": { diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 8f67488ab30..595de9be2c5 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -38,13 +38,13 @@ 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; use tracing::error; -use codex_core::review_format::format_review_findings_block; -use codex_protocol::protocol::ReviewOutputEvent; type JsonValue = serde_json::Value; diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index c3c4fdcb952..9f1a995050c 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -237,6 +237,7 @@ 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 { @@ -247,10 +248,12 @@ impl CodexMessageProcessor { } 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(), )), @@ -259,12 +262,17 @@ impl CodexMessageProcessor { 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 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 }, display)) + Ok(( + ReviewRequest { + prompt, + user_facing_hint: hint, + append_to_original_thread, + }, + display, + )) } ReviewTarget::Commit { sha, title } => { let sha = sha.trim().to_string(); @@ -275,13 +283,9 @@ impl CodexMessageProcessor { .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." - ) + 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." - ) + 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}"); @@ -290,7 +294,14 @@ impl CodexMessageProcessor { } else { format!("Review commit {short_sha}") }; - Ok((ReviewRequest { prompt, user_facing_hint: hint }, display)) + Ok(( + ReviewRequest { + prompt, + user_facing_hint: hint, + append_to_original_thread, + }, + display, + )) } ReviewTarget::Custom { instructions } => { let trimmed = instructions.trim().to_string(); @@ -301,6 +312,7 @@ impl CodexMessageProcessor { ReviewRequest { prompt: trimmed.clone(), user_facing_hint: trimmed.clone(), + append_to_original_thread, }, trimmed, )) @@ -2422,7 +2434,11 @@ impl CodexMessageProcessor { } async fn review_start(&self, request_id: RequestId, params: ReviewStartParams) { - let ReviewStartParams { thread_id, target } = params; + 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) => { @@ -2431,13 +2447,14 @@ impl CodexMessageProcessor { } }; - let (review_request, display_text) = match Self::review_request_from_target(target) { - Ok(value) => value, - Err(err) => { - self.outgoing.send_error(request_id, err).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; diff --git a/codex-rs/app-server/tests/suite/v2/review.rs b/codex-rs/app-server/tests/suite/v2/review.rs index f8733099865..6c7a77e52ca 100644 --- a/codex-rs/app-server/tests/suite/v2/review.rs +++ b/codex-rs/app-server/tests/suite/v2/review.rs @@ -58,6 +58,7 @@ async fn review_start_runs_review_turn_and_emits_code_review_item() -> Result<() 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()), @@ -128,6 +129,7 @@ async fn review_start_rejects_empty_base_branch() -> Result<()> { let request_id = mcp .send_review_start_request(ReviewStartParams { thread_id, + append_to_original_thread: true, target: ReviewTarget::BaseBranch { branch: " ".to_string(), }, @@ -161,6 +163,7 @@ async fn review_start_rejects_empty_commit_sha() -> Result<()> { 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, @@ -195,6 +198,7 @@ async fn review_start_rejects_empty_custom_instructions() -> Result<()> { let request_id = mcp .send_review_start_request(ReviewStartParams { thread_id, + append_to_original_thread: true, target: ReviewTarget::Custom { instructions: "\n\n".to_string(), }, diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 31961768237..2a04d431643 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1765,7 +1765,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)) @@ -2753,7 +2758,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 e0bb7d4e934..14a95dba5c3 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 c6ece7fe564..6339bfa71ac 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 9c3f812c297..3904f18f8ee 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 553f6437a65..87d95351086 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -1171,11 +1171,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 cdcc78abad4..45d862b00e7 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -2721,6 +2721,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, }, })); }, @@ -2777,6 +2778,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, }, })); })], @@ -2817,6 +2819,7 @@ impl ChatWidget { review_request: ReviewRequest { prompt, user_facing_hint: hint, + append_to_original_thread: true, }, })); })], @@ -2851,6 +2854,7 @@ impl ChatWidget { review_request: ReviewRequest { prompt: trimmed.clone(), user_facing_hint: trimmed, + append_to_original_thread: true, }, })); }), @@ -3061,6 +3065,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 abd9a612388..8f2a7925ade 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -140,6 +140,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, }), }); @@ -159,6 +160,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, }), }); From e881a142e0b634df3e32c6c39fb0170559aa9fdf Mon Sep 17 00:00:00 2001 From: jif-oai Date: Tue, 18 Nov 2025 15:17:08 +0000 Subject: [PATCH 06/10] Update v2.rs --- codex-rs/app-server-protocol/src/protocol/v2.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 867d3cef6f7..d0898de65cd 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -66,10 +66,6 @@ v2_enum_from_core!( } ); -fn default_true() -> bool { - true -} - #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] @@ -574,7 +570,7 @@ pub struct ReviewStartParams { pub target: ReviewTarget, /// When true (default), also append the final review message to the original thread. - #[serde(default = "default_true")] + #[serde(default)] pub append_to_original_thread: bool, } From e3799413a1ce46060e19a6e85014989e0289a8e9 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Tue, 18 Nov 2025 15:18:24 +0000 Subject: [PATCH 07/10] Update README.md --- codex-rs/app-server/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index c48cdc098f5..d56a12f6f33 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -190,7 +190,7 @@ Use `review/start` to run Codex’s reviewer on the currently checked-out projec - `{"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 `true`) — 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. +- `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: From 1fb1c46df2974b2dd4cc109c88328fab2f429a4d Mon Sep 17 00:00:00 2001 From: jif-oai Date: Tue, 18 Nov 2025 17:07:51 +0000 Subject: [PATCH 08/10] NITs --- codex-rs/app-server-protocol/src/protocol/v2.rs | 2 +- codex-rs/app-server/README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index d0898de65cd..b9961882676 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -569,7 +569,7 @@ pub struct ReviewStartParams { pub thread_id: String, pub target: ReviewTarget, - /// When true (default), also append the final review message to the original thread. + /// When true, also append the final review message to the original thread. #[serde(default)] pub append_to_original_thread: bool, } diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index d56a12f6f33..db4820e041d 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -182,7 +182,7 @@ 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. -### 5) Request a code review +### 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: From afd7d97a46c3ad30df0bd38a84aa296a6ba59b3b Mon Sep 17 00:00:00 2001 From: jif-oai Date: Tue, 18 Nov 2025 17:18:54 +0000 Subject: [PATCH 09/10] Process comments --- codex-rs/app-server-protocol/src/protocol/v2.rs | 6 ++++++ codex-rs/app-server/README.md | 14 +++++++++++++- .../app-server/src/bespoke_event_handling.rs | 11 +++++++++++ codex-rs/app-server/tests/suite/v2/review.rs | 16 ++++++++++++++++ 4 files changed, 46 insertions(+), 1 deletion(-) diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index b9961882676..f4a6d636aa5 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -582,9 +582,13 @@ pub enum ReviewTarget { 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. @@ -592,6 +596,8 @@ pub enum ReviewTarget { }, /// Arbitrary instructions, equivalent to the old free-form prompt. + #[serde(rename_all = "camelCase")] + #[ts(rename_all = "camelCase")] Custom { instructions: String }, } diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index db4820e041d..35653099aad 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -210,7 +210,19 @@ Example request/response: } } } ``` -Codex streams the usual `turn/started` notification. When the reviewer finishes, the server emits `item/completed` containing a `codeReview` item: +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": { diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 595de9be2c5..ce4f7590e22 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -191,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 }; diff --git a/codex-rs/app-server/tests/suite/v2/review.rs b/codex-rs/app-server/tests/suite/v2/review.rs index 6c7a77e52ca..61c9146bf65 100644 --- a/codex-rs/app-server/tests/suite/v2/review.rs +++ b/codex-rs/app-server/tests/suite/v2/review.rs @@ -4,6 +4,7 @@ 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; @@ -71,6 +72,7 @@ async fn review_start_runs_review_turn_and_emits_code_review_item() -> Result<() ) .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] { @@ -89,6 +91,20 @@ async fn review_start_runs_review_turn_and_emits_code_review_item() -> Result<() 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 { From 57287e02724e8823fd8faecf1597ce49a163e1fa Mon Sep 17 00:00:00 2001 From: jif-oai Date: Tue, 18 Nov 2025 20:20:41 +0000 Subject: [PATCH 10/10] Assert --- codex-rs/app-server/tests/suite/v2/review.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/codex-rs/app-server/tests/suite/v2/review.rs b/codex-rs/app-server/tests/suite/v2/review.rs index 61c9146bf65..194ed6ae96c 100644 --- a/codex-rs/app-server/tests/suite/v2/review.rs +++ b/codex-rs/app-server/tests/suite/v2/review.rs @@ -116,7 +116,8 @@ async fn review_start_runs_review_turn_and_emits_code_review_item() -> Result<() let completed: ItemCompletedNotification = serde_json::from_value(review_notif.params.expect("params must be present"))?; match completed.item { - ThreadItem::CodeReview { review, .. } => { + ThreadItem::CodeReview { id, review } => { + assert_eq!(id, turn_id); review_body = Some(review); break; }