diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 6ee6baf2dc..ff9e8a747e 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -170,14 +170,14 @@ Example with notification opt-out: - `thread/rollback` — deprecated and will be removed soon. Drop the last N turns from the agent’s in-memory context and persist a rollback marker in the rollout so future resumes see the pruned history; returns the updated `thread` (with `turns` populated) 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. `clientUserMessageId` is optional; when supplied, the corresponding `userMessage` item echoes it as `clientId`. Experimental `runtimeWorkspaceRoots` replaces the thread-scoped runtime workspace roots used to materialize `:workspace_roots`; paths must be absolute. Prefer experimental `permissions` profile selection by id for permission overrides; the legacy `sandboxPolicy` field is still accepted but cannot be combined with `permissions`. For `collaborationMode`, `settings.developer_instructions: null` means "use built-in instructions for the selected mode". Deprecated experimental `multiAgentMode` is ignored; Ultra reasoning effort selects proactive behavior. - `thread/inject_items` — append raw Responses API items to a loaded thread’s model-visible history without starting a user turn; returns `{}` on success. -- `turn/steer` — add user input to an already in-flight regular turn without starting a new turn; returns the active `turnId` that accepted the input. `clientUserMessageId` is optional; when supplied, the corresponding `userMessage` item echoes it as `clientId`. Review and manual compaction turns reject `turn/steer`. +- `turn/steer` — add user input to an already in-flight regular turn without starting a new turn; returns the active `turnId` that accepted the input. `clientUserMessageId` is optional; when supplied, the corresponding `userMessage` item echoes it as `clientId`. Manual compaction turns reject `turn/steer`. - `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"`. - `thread/realtime/start` — start a thread-scoped realtime session (experimental); pass `outputModality: "text"` or `outputModality: "audio"` to choose model output, optionally pass `model` and, for websocket transport only, `version` to override configured realtime selection for this session only, and pass `includeStartupContext: false` to omit Codex's generated startup context. By default, automatic Codex text follows the protocol's speakable output path. Pass `clientManagedHandoffs: true` to disable automatic Codex response delivery so only the client's explicit append calls produce handoffs. Pass `codexResponsesAsItems: true` to send automatic Codex responses as realtime conversation items instead, and optionally pass `codexResponseItemPrefix` to prepend experiment instructions to those items. For V1 sessions, pass `codexResponseHandoffPrefix` while item mode is disabled to route automatic Codex commentary through `conversation.handoff.append` with that prefix; final answers remain unprefixed. Returns `{}` and streams `thread/realtime/*` notifications. Omit `transport` for the websocket transport, or pass `{ "type": "webrtc", "sdp": "..." }` to create an AVAS/v1 WebRTC session from a browser-generated SDP offer; the remote answer SDP is emitted as `thread/realtime/sdp`. Explicit `version: "v2"` requests are rejected for WebRTC. - `thread/realtime/appendAudio` — append an input audio chunk to the active realtime session (experimental); returns `{}`. - `thread/realtime/appendText` — append text input to the active realtime session with a required `role` of `user`, `developer`, or `assistant` (experimental); returns `{}`. Older clients that omit `role` default to `user`. - `thread/realtime/appendSpeech` — append text that the realtime model should speak to the user (experimental); returns `{}`. - `thread/realtime/stop` — stop the active realtime session for the thread (experimental); returns `{}`. -- `review/start` — kick off Codex’s automated reviewer for a thread; responds like `turn/start` and emits `item/started`/`item/completed` notifications with `enteredReviewMode` and `exitedReviewMode` items, plus a final assistant `agentMessage` containing the review. +- `review/start` — start a regular turn that asks Codex to delegate the requested review to a sub-agent using the bundled `$review-agent` skill. The response shape is unchanged, while progress and the final review use the normal turn, collaboration, and `agentMessage` events. - `command/exec` — run a single command under the server sandbox without starting a thread/turn (handy for utilities and validation). - `command/exec/write` — write base64-decoded stdin bytes to a running `command/exec` session or close stdin; returns `{}`. - `command/exec/resize` — resize a running PTY-backed `command/exec` session by `processId`; returns `{}`. @@ -987,20 +987,23 @@ not emit `turn/started` and does not accept thread settings overrides. ``` `expectedTurnId` is required. If there is no active turn, `expectedTurnId` does not match the -active turn, or the active turn kind does not accept same-turn steering (for example review or -manual compaction), the request fails with an `invalid request` error. +active turn, or the active turn kind does not accept same-turn steering (for example manual +compaction), the request fails with an `invalid request` error. ### Example: 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: +Use `review/start` to ask Codex to delegate a review of the currently checked-out project to a +sub-agent. App-server translates the target into an ordinary user turn that explicitly invokes the +bundled `$review-agent` skill. 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":"baseBranch","branch":"main"}` — diff against the provided base branch. - `{"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. -- `delivery` (`"inline"` or `"detached"`, default `"inline"`) — where the review runs: - - `"inline"`: run the review as a new turn on the existing thread. The response’s `reviewThreadId` equals the original `threadId`, and no new `thread/started` notification is emitted. - - `"detached"`: fork a new review thread from the parent conversation and run the review there. The response’s `reviewThreadId` is the id of this new review thread, and the server emits a `thread/started` notification for it before streaming review items. +- `delivery` (`"inline"` or `"detached"`, default `"inline"`) — where the delegation turn runs: + - `"inline"`: run the delegation as a new turn on the existing thread. The response’s `reviewThreadId` equals the original `threadId`, and no new `thread/started` notification is emitted. + - `"detached"`: fork a new thread from the parent conversation and run the delegation there. The response’s `reviewThreadId` is the id of this new thread, and the server emits a `thread/started` notification for it before streaming turn items. Example request/response: @@ -1015,7 +1018,7 @@ Example request/response: "id": "turn_900", "status": "inProgress", "items": [ - { "type": "userMessage", "id": "turn_900", "content": [ { "type": "text", "text": "Review commit 1234567: Polish tui colors" } ] } + { "type": "userMessage", "id": "turn_900", "content": [ { "type": "text", "text": "Spawn one sub-agent with the current conversation context. Tell it to use the $review-agent skill to review the target below. The sub-agent must perform the review itself and must not delegate to other agents. Wait for it to finish, then return its complete response as your final answer. Do not perform the review yourself.\n\nReview target:\nReview the changes introduced by commit 1234567deadbeef (\"Polish tui colors\")." } ] } ], "error": null }, @@ -1023,41 +1026,15 @@ Example request/response: } } ``` -For a detached review, use `"delivery": "detached"`. The response is the same shape, but `reviewThreadId` will be the id of the new review thread (different from the original `threadId`). The server also emits a `thread/started` notification for that new thread before streaming the review turn. +For a detached review, use `"delivery": "detached"`. The response is the same shape, but +`reviewThreadId` will be the id of the new thread (different from the original `threadId`). The +server also emits a `thread/started` notification for that thread before streaming the delegation +turn. -Codex streams the usual `turn/started` notification followed by an `item/started` -with an `enteredReviewMode` item so clients can show progress: - -```json -{ - "method": "item/started", - "params": { - "item": { - "type": "enteredReviewMode", - "id": "turn_900", - "review": "current changes" - } - } -} -``` - -When the reviewer finishes, the server emits `item/started` and `item/completed` -containing an `exitedReviewMode` item with the final review text: - -```json -{ - "method": "item/completed", - "params": { - "item": { - "type": "exitedReviewMode", - "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::ExitedReviewMode` in the generated schema). Use this notification to render the reviewer output in your client. +The delegation is an ordinary user turn. Clients receive the usual `turn/started`, `item/*`, +collaboration or sub-agent activity, final `agentMessage`, and `turn/completed` events. New reviews +do not emit `enteredReviewMode` or `exitedReviewMode`; those item types remain in the schema so +clients can replay older rollouts. Render the final `agentMessage` as the review result. ### Example: One-off command execution @@ -1378,8 +1355,8 @@ Today both notifications carry an empty `items` array even when item events were - `webSearch` — `{id, query, action?}` for a web search request issued by the agent; `action` mirrors the Responses API web_search action payload (`search`, `open_page`, `find_in_page`) and may be omitted until completion. - `imageView` — `{id, path}` emitted when the agent invokes the image viewer tool. - `sleep` — `{id, durationMs}` emitted while the agent waits for a duration or new input. -- `enteredReviewMode` — `{id, review}` sent when the reviewer starts; `review` is a short user-facing label such as `"current changes"` or the requested target description. -- `exitedReviewMode` — `{id, review}` emitted when the reviewer finishes; `review` is the full plain-text review (usually, overall notes plus bullet point findings). +- `enteredReviewMode` — legacy `{id, review}` item retained so clients can replay older review rollouts. +- `exitedReviewMode` — legacy `{id, review}` item retained so clients can replay older review rollouts. - `contextCompaction` — `{id}` emitted when codex compacts the conversation history. This can happen automatically. - `compacted` - `{threadId, turnId}` when codex compacts the conversation history. This can happen automatically. **Deprecated:** Use `contextCompaction` instead. diff --git a/codex-rs/app-server/src/request_processors.rs b/codex-rs/app-server/src/request_processors.rs index da88d0e067..2d98f5b187 100644 --- a/codex-rs/app-server/src/request_processors.rs +++ b/codex-rs/app-server/src/request_processors.rs @@ -430,8 +430,6 @@ use codex_protocol::protocol::Op; use codex_protocol::protocol::RealtimeVoicesList; use codex_protocol::protocol::ResumedHistory; use codex_protocol::protocol::ReviewDelivery as CoreReviewDelivery; -use codex_protocol::protocol::ReviewRequest; -use codex_protocol::protocol::ReviewTarget as CoreReviewTarget; use codex_protocol::protocol::RolloutItem; use codex_protocol::protocol::SessionConfiguredEvent; #[cfg(test)] diff --git a/codex-rs/app-server/src/request_processors/turn_processor.rs b/codex-rs/app-server/src/request_processors/turn_processor.rs index 3ff64cd34e..4ac8833ef0 100644 --- a/codex-rs/app-server/src/request_processors/turn_processor.rs +++ b/codex-rs/app-server/src/request_processors/turn_processor.rs @@ -12,6 +12,7 @@ use crate::image_url::is_remote_image_url; const DIRECT_INPUT_TO_MULTI_AGENT_V2_SUBAGENT_ERROR: &str = "direct app-server input is not allowed for multi-agent v2 sub-agents"; +const REVIEW_AGENT_SKILL_NAME: &str = "review-agent"; fn validate_user_input_image_urls(input: &[V2UserInput]) -> Result<(), JSONRPCErrorError> { if input.iter().any(|item| { @@ -350,55 +351,47 @@ impl TurnRequestProcessor { collaboration_mode } - fn review_request_from_target( - target: ApiReviewTarget, - ) -> Result<(ReviewRequest, String), JSONRPCErrorError> { - let cleaned_target = match target { - ApiReviewTarget::UncommittedChanges => ApiReviewTarget::UncommittedChanges, + fn review_prompt_from_target(target: ApiReviewTarget) -> Result { + let target = match target { + ApiReviewTarget::UncommittedChanges => { + "Review the current code changes (staged, unstaged, and untracked files)." + .to_string() + } ApiReviewTarget::BaseBranch { branch } => { let branch = branch.trim().to_string(); if branch.is_empty() { return Err(invalid_request("branch must not be empty".to_string())); } - ApiReviewTarget::BaseBranch { branch } + format!("Review the code changes against the base branch '{branch}'.") } ApiReviewTarget::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 title = title + if let Some(title) = title .map(|t| t.trim().to_string()) - .filter(|t| !t.is_empty()); - ApiReviewTarget::Commit { sha, title } + .filter(|t| !t.is_empty()) + { + format!("Review the changes introduced by commit {sha} (\"{title}\").") + } else { + format!("Review the changes introduced by commit {sha}.") + } } ApiReviewTarget::Custom { instructions } => { - let trimmed = instructions.trim().to_string(); - if trimmed.is_empty() { + let instructions = instructions.trim().to_string(); + if instructions.is_empty() { return Err(invalid_request( "instructions must not be empty".to_string(), )); } - ApiReviewTarget::Custom { - instructions: trimmed, - } + instructions } }; - let core_target = match cleaned_target { - ApiReviewTarget::UncommittedChanges => CoreReviewTarget::UncommittedChanges, - ApiReviewTarget::BaseBranch { branch } => CoreReviewTarget::BaseBranch { branch }, - ApiReviewTarget::Commit { sha, title } => CoreReviewTarget::Commit { sha, title }, - ApiReviewTarget::Custom { instructions } => CoreReviewTarget::Custom { instructions }, - }; - - let hint = codex_core::review_prompts::user_facing_hint(&core_target); - let review_request = ReviewRequest { - target: core_target, - user_facing_hint: Some(hint.clone()), - }; - - Ok((review_request, hint)) + Ok(format!( + "Spawn one sub-agent with the current conversation context. Tell it to use the ${REVIEW_AGENT_SKILL_NAME} skill to review the target below. The sub-agent must perform the review itself and must not delegate to other agents. Wait for it to finish, then return its complete response as your final answer. Do not perform the review yourself.\n\nReview target:\n{target}" + )) } async fn request_trace_context( @@ -1129,16 +1122,16 @@ impl TurnRequestProcessor { Ok(Some(ThreadRealtimeStopResponse::default())) } - fn build_review_turn(turn_id: String, display_text: &str) -> Turn { - let items = if display_text.is_empty() { + fn build_review_turn(turn_id: String, prompt: &str) -> Turn { + let items = if prompt.is_empty() { Vec::new() } else { vec![ThreadItem::UserMessage { id: turn_id.clone(), client_id: None, content: vec![V2UserInput::Text { - text: display_text.to_string(), - // Review prompt display text is synthesized; no UI element ranges to preserve. + text: prompt.to_string(), + // The review prompt is synthesized; no UI element ranges need preservation. text_elements: Vec::new(), }], }] @@ -1156,6 +1149,19 @@ impl TurnRequestProcessor { } } + fn review_turn_op(prompt: &str) -> Op { + Op::UserInput { + items: vec![CoreInputItem::Text { + text: prompt.to_string(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: Default::default(), + } + } + async fn emit_review_started( &self, request_id: &ConnectionRequestId, @@ -1175,19 +1181,18 @@ impl TurnRequestProcessor { &self, request_id: &ConnectionRequestId, parent_thread: Arc, - review_request: ReviewRequest, - display_text: &str, + prompt: &str, parent_thread_id: String, ) -> std::result::Result<(), JSONRPCErrorError> { let turn_id = self .submit_core_op( request_id, parent_thread.as_ref(), - Op::Review { review_request }, + Self::review_turn_op(prompt), ) .await .map_err(|err| internal_error(format!("failed to start review: {err}")))?; - let turn = Self::build_review_turn(turn_id, display_text); + let turn = Self::build_review_turn(turn_id, prompt); self.emit_review_started(request_id, turn, parent_thread_id) .await; Ok(()) @@ -1198,8 +1203,7 @@ impl TurnRequestProcessor { request_id: &ConnectionRequestId, parent_thread_id: ThreadId, parent_thread: Arc, - review_request: ReviewRequest, - display_text: &str, + prompt: &str, ) -> std::result::Result<(), JSONRPCErrorError> { parent_thread.ensure_rollout_materialized().await; parent_thread.flush_rollout().await.map_err(|err| { @@ -1216,10 +1220,7 @@ impl TurnRequestProcessor { )) })?; - let mut config = self.config.as_ref().clone(); - if let Some(review_model) = &config.review_model { - config.model = Some(review_model.clone()); - } + let config = self.config.as_ref().clone(); let NewThread { thread_id, @@ -1229,7 +1230,7 @@ impl TurnRequestProcessor { .thread_manager .fork_thread_from_history( ForkSnapshot::Interrupted, - config.clone(), + config, InitialHistory::Resumed(ResumedHistory { conversation_id: parent_thread_id, history: Arc::new(parent_history.items), @@ -1290,14 +1291,14 @@ impl TurnRequestProcessor { .submit_core_op( request_id, review_thread.as_ref(), - Op::Review { review_request }, + Self::review_turn_op(prompt), ) .await .map_err(|err| { internal_error(format!("failed to start detached review turn: {err}")) })?; - let turn = Self::build_review_turn(turn_id, display_text); + let turn = Self::build_review_turn(turn_id, prompt); let review_thread_id = thread_id.to_string(); self.emit_review_started(request_id, turn, review_thread_id) .await; @@ -1317,27 +1318,17 @@ impl TurnRequestProcessor { } = params; let (parent_thread_id, parent_thread) = self.load_thread(&thread_id).await?; - let (review_request, display_text) = Self::review_request_from_target(target)?; + self.ensure_direct_input_allowed(request_id, parent_thread.as_ref()) + .await?; + let prompt = Self::review_prompt_from_target(target)?; match delivery.unwrap_or(ApiReviewDelivery::Inline).to_core() { CoreReviewDelivery::Inline => { - self.start_inline_review( - request_id, - parent_thread, - review_request, - &display_text, - thread_id, - ) - .await?; + self.start_inline_review(request_id, parent_thread, &prompt, thread_id) + .await?; } CoreReviewDelivery::Detached => { - self.start_detached_review( - request_id, - parent_thread_id, - parent_thread, - review_request, - &display_text, - ) - .await?; + self.start_detached_review(request_id, parent_thread_id, parent_thread, &prompt) + .await?; } } Ok(()) diff --git a/codex-rs/app-server/tests/suite/v2/review.rs b/codex-rs/app-server/tests/suite/v2/review.rs index 786a3eab5b..b54a39cbae 100644 --- a/codex-rs/app-server/tests/suite/v2/review.rs +++ b/codex-rs/app-server/tests/suite/v2/review.rs @@ -1,22 +1,15 @@ use anyhow::Result; use app_test_support::TestAppServer; -use app_test_support::create_final_assistant_message_sse_response; use app_test_support::create_mock_responses_server_repeating_assistant; -use app_test_support::create_mock_responses_server_sequence; -use app_test_support::create_shell_command_sse_response; 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::JSONRPCMessage; -use codex_app_server_protocol::JSONRPCNotification; use codex_app_server_protocol::JSONRPCResponse; use codex_app_server_protocol::RequestId; use codex_app_server_protocol::ReviewDelivery; use codex_app_server_protocol::ReviewStartParams; use codex_app_server_protocol::ReviewStartResponse; use codex_app_server_protocol::ReviewTarget; -use codex_app_server_protocol::ServerRequest; use codex_app_server_protocol::ThreadItem; use codex_app_server_protocol::ThreadStartParams; use codex_app_server_protocol::ThreadStartResponse; @@ -26,8 +19,8 @@ use codex_app_server_protocol::TurnItemsView; use codex_app_server_protocol::TurnStartParams; use codex_app_server_protocol::TurnStatus; use codex_app_server_protocol::UserInput as V2UserInput; +use core_test_support::responses; use pretty_assertions::assert_eq; -use serde_json::json; use tempfile::TempDir; use tokio::time::timeout; @@ -35,26 +28,17 @@ const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs 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 server = create_mock_responses_server_repeating_assistant(&review_payload).await; +async fn review_start_runs_regular_turn_with_review_agent_skill() -> Result<()> { + let server = responses::start_mock_server().await; + let response_mock = responses::mount_sse_once( + &server, + responses::sse(vec![ + responses::ev_response_created("resp-1"), + responses::ev_assistant_message("msg-1", "Done"), + responses::ev_completed("resp-1"), + ]), + ) + .await; let codex_home = TempDir::new()?; create_config_toml(codex_home.path(), &server.uri())?; @@ -90,169 +74,42 @@ async fn review_start_runs_review_turn_and_emits_code_review_item() -> Result<() let turn_id = turn.id.clone(); assert_eq!(turn.status, TurnStatus::InProgress); assert_eq!(turn.items_view, TurnItemsView::NotLoaded); - assert_eq!( - turn.items, - vec![ThreadItem::UserMessage { - id: turn_id.clone(), - client_id: None, - content: vec![V2UserInput::Text { - text: "commit 1234567: Tidy UI colors".to_string(), - text_elements: Vec::new(), - }], - }] - ); - - // Confirm we see the EnteredReviewMode marker on the main thread. - let mut saw_entered_review_mode = false; - for _ in 0..10 { - 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::EnteredReviewMode { review, .. } => { - assert_eq!(started.turn_id, turn_id); - assert_eq!(review, "commit 1234567: Tidy UI colors"); - saw_entered_review_mode = true; - break; - } - _ => continue, - } - } - assert!( - saw_entered_review_mode, - "did not observe enteredReviewMode item" - ); - - // Confirm we see the ExitedReviewMode marker (with review text) - // on the same turn. Ignore any other items the stream surfaces. - let mut review_body: Option = None; - for _ in 0..10 { - 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::ExitedReviewMode { review, .. } => { - assert_eq!(completed.turn_id, turn_id); - review_body = Some(review); - break; - } - _ => continue, - } - } - - 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] -#[ignore = "TODO(owenlin0): flaky"] -async fn review_start_exec_approval_item_id_matches_command_execution_item() -> Result<()> { - let responses = vec![ - create_shell_command_sse_response( - vec![ - "git".to_string(), - "rev-parse".to_string(), - "HEAD".to_string(), - ], - /*workdir*/ None, - Some(5000), - "review-call-1", - )?, - create_final_assistant_message_sse_response("done")?, - ]; - let server = create_mock_responses_server_sequence(responses).await; - - let codex_home = TempDir::new()?; - create_config_toml_with_approval_policy(codex_home.path(), &server.uri(), "untrusted")?; - - let mut mcp = TestAppServer::builder() - .with_codex_home(codex_home.path()) - .build() - .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, - delivery: Some(ReviewDelivery::Inline), - target: ReviewTarget::Commit { - sha: "1234567deadbeef".to_string(), - title: Some("Check review approvals".to_string()), - }, - }) - .await?; - let review_resp: JSONRPCResponse = timeout( - DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_response_message(RequestId::Integer(review_req)), - ) - .await??; - let ReviewStartResponse { turn, .. } = to_response::(review_resp)?; - let turn_id = turn.id.clone(); - assert_eq!(turn.items_view, TurnItemsView::NotLoaded); - assert_eq!( - turn.items, - vec![ThreadItem::UserMessage { - id: turn_id.clone(), - client_id: None, - content: vec![V2UserInput::Text { - text: "commit 1234567: Check review approvals".to_string(), - text_elements: Vec::new(), - }], - }] - ); - - let server_req = timeout( - DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_request_message(), - ) - .await??; - let ServerRequest::CommandExecutionRequestApproval { request_id, params } = server_req else { - panic!("expected CommandExecutionRequestApproval request"); - }; - assert_eq!(params.item_id, "review-call-1"); - assert_eq!(params.turn_id, turn_id); - - let mut command_item_id = None; - for _ in 0..10 { - 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"))?; - if let ThreadItem::CommandExecution { id, .. } = started.item { - command_item_id = Some(id); - break; - } - } - let command_item_id = command_item_id.expect("did not observe command execution item"); - assert_eq!(command_item_id, params.item_id); - - mcp.send_response( - request_id, - serde_json::json!({ "decision": codex_protocol::protocol::ReviewDecision::Approved }), - ) - .await?; timeout( DEFAULT_READ_TIMEOUT, mcp.read_stream_until_notification_message("turn/completed"), ) .await??; + let request = response_mock.single_request(); + let user_messages = request.message_input_texts("user"); + let prompt = user_messages + .iter() + .find(|text| text.contains("Spawn one sub-agent")) + .expect("review delegation prompt should be model-visible"); + assert!(prompt.contains("$review-agent")); + assert!(prompt.contains("1234567deadbeef")); + assert!(prompt.contains("Tidy UI colors")); + + let skill_fragments = user_messages + .iter() + .filter(|text| text.starts_with("")) + .collect::>(); + assert_eq!(skill_fragments.len(), 1); + assert!(skill_fragments[0].contains("review-agent")); + assert!(skill_fragments[0].contains("Do not modify files")); + + assert_eq!( + turn.items, + vec![ThreadItem::UserMessage { + id: turn_id, + client_id: None, + content: vec![V2UserInput::Text { + text: prompt.to_string(), + text_elements: Vec::new(), + }], + }] + ); + Ok(()) } @@ -296,14 +153,7 @@ async fn review_start_rejects_empty_base_branch() -> Result<()> { #[cfg_attr(target_os = "windows", ignore = "flaky on windows CI")] #[tokio::test] async fn review_start_with_detached_delivery_returns_new_thread_id() -> Result<()> { - let review_payload = json!({ - "findings": [], - "overall_correctness": "ok", - "overall_explanation": "detached review", - "overall_confidence_score": 0.5 - }) - .to_string(); - let server = create_mock_responses_server_repeating_assistant(&review_payload).await; + let server = create_mock_responses_server_repeating_assistant("Done").await; let codex_home = TempDir::new()?; create_config_toml(codex_home.path(), &server.uri())?; @@ -338,17 +188,22 @@ async fn review_start_with_detached_delivery_returns_new_thread_id() -> Result<( assert_eq!(turn.status, TurnStatus::InProgress); assert_eq!(turn.items_view, TurnItemsView::NotLoaded); - assert_eq!( - turn.items, - vec![ThreadItem::UserMessage { - id: turn.id.clone(), - client_id: None, - content: vec![V2UserInput::Text { - text: "detached review".to_string(), - text_elements: Vec::new(), - }], - }] - ); + let [ThreadItem::UserMessage { content, .. }] = turn.items.as_slice() else { + panic!("expected synthesized review user message"); + }; + let [ + V2UserInput::Text { + text, + text_elements, + }, + ] = content.as_slice() + else { + panic!("expected synthesized review text"); + }; + assert!(text.contains("Spawn one sub-agent")); + assert!(text.contains("$review-agent")); + assert!(text.ends_with("Review target:\ndetached review")); + assert!(text_elements.is_empty()); assert_ne!( review_thread_id, thread_id, "detached review should run on a different thread" @@ -508,21 +363,13 @@ async fn materialize_thread_rollout(mcp: &mut TestAppServer, thread_id: &str) -> } fn create_config_toml(codex_home: &std::path::Path, server_uri: &str) -> std::io::Result<()> { - create_config_toml_with_approval_policy(codex_home, server_uri, "never") -} - -fn create_config_toml_with_approval_policy( - codex_home: &std::path::Path, - server_uri: &str, - approval_policy: &str, -) -> std::io::Result<()> { let config_toml = codex_home.join("config.toml"); std::fs::write( config_toml, format!( r#" model = "mock-model" -approval_policy = "{approval_policy}" +approval_policy = "never" sandbox_mode = "read-only" model_provider = "mock_provider" diff --git a/codex-rs/skills/src/assets/samples/review-agent/SKILL.md b/codex-rs/skills/src/assets/samples/review-agent/SKILL.md new file mode 100644 index 0000000000..f0fa2d660d --- /dev/null +++ b/codex-rs/skills/src/assets/samples/review-agent/SKILL.md @@ -0,0 +1,50 @@ +--- +name: review-agent +description: Perform a read-only, defect-first review of a specified code change and return every actionable finding. Use when another agent delegates review of uncommitted changes, a base-branch diff, a commit, or custom review instructions. +--- + +# Review Agent + +When assigned as the reviewer, inspect the requested target directly and return every finding that +the author would likely fix. Do not modify files, create commits, push branches, or post review +comments. + +## Review the change + +1. Read the applicable `AGENTS.md` instructions. +2. Inspect the complete diff for the requested target and enough surrounding code to understand + each changed path. +3. Identify concrete regressions introduced by the change. Continue through the whole diff after + finding the first issue. +4. Check the relevant tests and call sites to confirm that each finding is real and actionable. + +Flag an issue only when all of these are true: + +- It affects correctness, security, performance, or maintainability in a meaningful way. +- It is discrete and actionable. +- It was introduced by the reviewed change. +- The affected scenario or call path can be demonstrated from the code. +- The author would probably fix it if they knew about it. + +Do not flag speculative concerns, pre-existing problems, intentional behavior changes, or style +nits that do not obscure the code. + +## Write the result + +Present findings first, ordered by severity. Use one entry per issue in this form: + +`[P1] Imperative finding title — path/to/file.rs:line` + +Follow the title with one short paragraph explaining the affected scenario and why the behavior is +wrong. Keep the cited range as small as possible and make sure it overlaps the reviewed diff. + +Use these priorities: + +- `P0`: universal release blocker or critical failure. +- `P1`: urgent defect that should be fixed next. +- `P2`: ordinary defect that should be fixed. +- `P3`: low-impact issue that is still worth fixing. + +If there are no qualifying findings, say `No findings.` Do not invent a finding to fill the result. +After the findings, add a brief overall assessment and mention any material test gaps or residual +risks. diff --git a/codex-rs/skills/src/assets/samples/review-agent/agents/openai.yaml b/codex-rs/skills/src/assets/samples/review-agent/agents/openai.yaml new file mode 100644 index 0000000000..ebbb0b041f --- /dev/null +++ b/codex-rs/skills/src/assets/samples/review-agent/agents/openai.yaml @@ -0,0 +1,6 @@ +interface: + display_name: "Review Agent" + short_description: "Find actionable bugs in code changes" + default_prompt: "Use $review-agent to review the requested code changes and return actionable findings." +policy: + allow_implicit_invocation: false