feat: review in app server (#6613)
This commit is contained in:
parent
29ca89c414
commit
8ddae8cde3
15 changed files with 667 additions and 31 deletions
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -562,6 +562,45 @@ pub struct TurnStartParams {
|
|||
pub summary: Option<ReasoningSummary>,
|
||||
}
|
||||
|
||||
#[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<String>,
|
||||
},
|
||||
|
||||
/// 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/")]
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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<JsonValue>,
|
||||
|
|
|
|||
|
|
@ -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::<String>();
|
||||
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;
|
||||
|
||||
|
|
|
|||
|
|
@ -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<i64> {
|
||||
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,
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
mod account;
|
||||
mod model_list;
|
||||
mod rate_limits;
|
||||
mod review;
|
||||
mod thread_archive;
|
||||
mod thread_list;
|
||||
mod thread_resume;
|
||||
|
|
|
|||
279
codex-rs/app-server/tests/suite/v2/review.rs
Normal file
279
codex-rs/app-server/tests/suite/v2/review.rs
Normal file
|
|
@ -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::<TurnStartResponse>(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<String> = 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<String> {
|
||||
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::<ThreadStartResponse>(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
|
||||
"#
|
||||
),
|
||||
)
|
||||
}
|
||||
|
|
@ -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;
|
||||
|
||||
|
|
|
|||
|
|
@ -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<SessionTaskContext>, ctx: Arc<TurnContext>) {
|
||||
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<Session>,
|
||||
review_output: Option<ReviewOutputEvent>,
|
||||
ctx: Arc<TurnContext>,
|
||||
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(),
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -1173,11 +1173,13 @@ pub struct GitInfo {
|
|||
pub repository_url: Option<String>,
|
||||
}
|
||||
|
||||
/// 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.
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
},
|
||||
}));
|
||||
})],
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
}),
|
||||
});
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue