diff --git a/codex-rs/app-server-protocol/schema/json/EventMsg.json b/codex-rs/app-server-protocol/schema/json/EventMsg.json index 1a4bac2b02..0b58cef444 100644 --- a/codex-rs/app-server-protocol/schema/json/EventMsg.json +++ b/codex-rs/app-server-protocol/schema/json/EventMsg.json @@ -2706,6 +2706,9 @@ "string", "null" ] + }, + "validate_findings": { + "type": "boolean" } }, "required": [ @@ -2718,6 +2721,12 @@ { "description": "Exited review mode with an optional final result to apply.", "properties": { + "failure_message": { + "type": [ + "string", + "null" + ] + }, "review_output": { "anyOf": [ { @@ -8576,6 +8585,9 @@ "string", "null" ] + }, + "validate_findings": { + "type": "boolean" } }, "required": [ @@ -8588,6 +8600,12 @@ { "description": "Exited review mode with an optional final result to apply.", "properties": { + "failure_message": { + "type": [ + "string", + "null" + ] + }, "review_output": { "anyOf": [ { diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index 7aec380766..e1da4fc50e 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -4033,6 +4033,9 @@ "string", "null" ] + }, + "validate_findings": { + "type": "boolean" } }, "required": [ @@ -4045,6 +4048,12 @@ { "description": "Exited review mode with an optional final result to apply.", "properties": { + "failure_message": { + "type": [ + "string", + "null" + ] + }, "review_output": { "anyOf": [ { diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index 39408567d7..f43bf067b1 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -5839,6 +5839,9 @@ "string", "null" ] + }, + "validate_findings": { + "type": "boolean" } }, "required": [ @@ -5851,6 +5854,12 @@ { "description": "Exited review mode with an optional final result to apply.", "properties": { + "failure_message": { + "type": [ + "string", + "null" + ] + }, "review_output": { "anyOf": [ { diff --git a/codex-rs/app-server-protocol/schema/typescript/ExitedReviewModeEvent.ts b/codex-rs/app-server-protocol/schema/typescript/ExitedReviewModeEvent.ts index 7271f07a3f..6d32ccd47a 100644 --- a/codex-rs/app-server-protocol/schema/typescript/ExitedReviewModeEvent.ts +++ b/codex-rs/app-server-protocol/schema/typescript/ExitedReviewModeEvent.ts @@ -3,4 +3,4 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. import type { ReviewOutputEvent } from "./ReviewOutputEvent"; -export type ExitedReviewModeEvent = { review_output: ReviewOutputEvent | null, }; +export type ExitedReviewModeEvent = { review_output: ReviewOutputEvent | null, failure_message?: string, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/ReviewRequest.ts b/codex-rs/app-server-protocol/schema/typescript/ReviewRequest.ts index 1e9b8ad2ee..9eee63f8cb 100644 --- a/codex-rs/app-server-protocol/schema/typescript/ReviewRequest.ts +++ b/codex-rs/app-server-protocol/schema/typescript/ReviewRequest.ts @@ -6,4 +6,4 @@ import type { ReviewTarget } from "./ReviewTarget"; /** * Review request sent to the review session. */ -export type ReviewRequest = { target: ReviewTarget, user_facing_hint?: string, }; +export type ReviewRequest = { target: ReviewTarget, user_facing_hint?: string, validate_findings?: boolean, }; diff --git a/codex-rs/app-server-protocol/src/protocol/thread_history.rs b/codex-rs/app-server-protocol/src/protocol/thread_history.rs index 1ad1591292..57dc4bd14b 100644 --- a/codex-rs/app-server-protocol/src/protocol/thread_history.rs +++ b/codex-rs/app-server-protocol/src/protocol/thread_history.rs @@ -782,11 +782,7 @@ impl ThreadHistoryBuilder { &mut self, payload: &codex_protocol::protocol::ExitedReviewModeEvent, ) { - let review = payload - .review_output - .as_ref() - .map(render_review_output_text) - .unwrap_or_else(|| REVIEW_FALLBACK_MESSAGE.to_string()); + let review = render_exited_review_mode_text(payload); let id = self.next_item_id(); self.ensure_turn() .items @@ -993,6 +989,22 @@ fn render_review_output_text(output: &ReviewOutputEvent) -> String { } } +fn render_exited_review_mode_text( + event: &codex_protocol::protocol::ExitedReviewModeEvent, +) -> String { + if let Some(message) = event.failure_message.as_deref() { + let message = message.trim(); + if !message.is_empty() { + return message.to_string(); + } + } + event + .review_output + .as_ref() + .map(render_review_output_text) + .unwrap_or_else(|| REVIEW_FALLBACK_MESSAGE.to_string()) +} + pub fn convert_patch_changes( changes: &HashMap, ) -> Vec { @@ -1315,6 +1327,32 @@ mod tests { ); } + #[test] + fn exited_review_mode_uses_explicit_failure_message() { + let events = vec![EventMsg::ExitedReviewMode( + codex_protocol::protocol::ExitedReviewModeEvent { + review_output: None, + failure_message: Some( + "Review findings validation did not complete cleanly.".into(), + ), + }, + )]; + + let items = events + .into_iter() + .map(RolloutItem::EventMsg) + .collect::>(); + let turns = build_turns_from_rollout_items(&items); + assert_eq!(turns.len(), 1); + assert_eq!( + turns[0].items[0], + ThreadItem::ExitedReviewMode { + id: "item-1".into(), + review: "Review findings validation did not complete cleanly.".into(), + } + ); + } + #[test] fn splits_reasoning_when_interleaved() { let events = vec![ diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 38b03006bd..59065ade4b 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -1307,10 +1307,7 @@ pub(crate) async fn apply_bespoke_event_handling( .await; } EventMsg::ExitedReviewMode(review_event) => { - let review = match review_event.review_output { - Some(output) => render_review_output_text(&output), - None => REVIEW_FALLBACK_MESSAGE.to_string(), - }; + let review = render_exited_review_mode_text(&review_event); let item = ThreadItem::ExitedReviewMode { id: event_turn_id.clone(), review, @@ -2266,6 +2263,22 @@ fn render_review_output_text(output: &ReviewOutputEvent) -> String { } } +fn render_exited_review_mode_text( + event: &codex_protocol::protocol::ExitedReviewModeEvent, +) -> String { + if let Some(message) = event.failure_message.as_deref() { + let message = message.trim(); + if !message.is_empty() { + return message.to_string(); + } + } + event + .review_output + .as_ref() + .map(render_review_output_text) + .unwrap_or_else(|| REVIEW_FALLBACK_MESSAGE.to_string()) +} + fn map_file_change_approval_decision( decision: FileChangeApprovalDecision, ) -> (ReviewDecision, Option) { diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index b2d5c0a75b..a177958520 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -603,6 +603,7 @@ impl CodexMessageProcessor { let review_request = ReviewRequest { target: core_target, user_facing_hint: Some(hint.clone()), + validate_findings: false, }; Ok((review_request, hint)) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index ef8e7b21bf..735101d770 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -4968,6 +4968,7 @@ mod handlers { sub_id: String, review_request: ReviewRequest, ) { + let validate_findings = review_request.validate_findings; let turn_context = sess.new_default_turn_with_sub_id(sub_id.clone()).await; sess.maybe_emit_unknown_model_warning_for_turn(turn_context.as_ref()) .await; @@ -4980,6 +4981,7 @@ mod handlers { turn_context.clone(), sub_id, resolved, + validate_findings, ) .await; } @@ -5004,6 +5006,7 @@ async fn spawn_review_thread( parent_turn_context: Arc, sub_id: String, resolved: crate::review_prompts::ResolvedReviewRequest, + validate_findings: bool, ) { let model = config .review_model @@ -5126,12 +5129,18 @@ async fn spawn_review_thread( // TODO(ccunningham): Review turns currently rely on `spawn_task` for TurnComplete but do not // emit a parent TurnStarted. Consider giving review a full parent turn lifecycle // (TurnStarted + TurnComplete) for consistency with other standalone tasks. - sess.spawn_task(tc.clone(), input, ReviewTask::new()).await; + sess.spawn_task( + tc.clone(), + input, + ReviewTask::new(validate_findings, resolved.clone()), + ) + .await; // Announce entering review mode so UIs can switch modes. let review_request = ReviewRequest { target: resolved.target, user_facing_hint: Some(resolved.user_facing_hint), + validate_findings, }; sess.send_event(&tc, EventMsg::EnteredReviewMode(review_request)) .await; diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 1595830a58..7765cb3323 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -426,11 +426,11 @@ async fn handle_request_user_input( event: RequestUserInputEvent, cancel_token: &CancellationToken, ) { + let call_id = event.call_id.clone(); let args = RequestUserInputArgs { questions: event.questions, }; - let response_fut = - parent_session.request_user_input(parent_ctx, parent_ctx.sub_id.clone(), args); + let response_fut = parent_session.request_user_input(parent_ctx, call_id, args); let response = await_user_input_with_cancel( response_fut, parent_session, diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index e311767338..76a35351bb 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -31,6 +31,7 @@ use crate::protocol::TokenUsage; use crate::protocol::TokenUsageInfo; use crate::protocol::TurnCompleteEvent; use crate::protocol::UserMessageEvent; +use crate::review_prompts::ResolvedReviewRequest; use crate::rollout::policy::EventPersistenceMode; use crate::rollout::recorder::RolloutRecorder; use crate::rollout::recorder::RolloutRecorderParams; @@ -3494,8 +3495,21 @@ async fn abort_review_task_emits_exited_then_aborted_and_records_history() { text: "start review".to_string(), text_elements: Vec::new(), }]; - sess.spawn_task(Arc::clone(&tc), input, ReviewTask::new()) - .await; + sess.spawn_task( + Arc::clone(&tc), + input, + ReviewTask::new( + false, + ResolvedReviewRequest { + target: codex_protocol::protocol::ReviewTarget::Custom { + instructions: "start review".to_string(), + }, + prompt: "start review".to_string(), + user_facing_hint: "start review".to_string(), + }, + ), + ) + .await; sess.abort_all_tasks(TurnAbortReason::Interrupted).await; @@ -3517,6 +3531,12 @@ async fn abort_review_task_emits_exited_then_aborted_and_records_history() { match evt.msg { EventMsg::ExitedReviewMode(ev) => { assert!(ev.review_output.is_none()); + assert_eq!( + ev.failure_message.as_deref(), + Some( + "Review was interrupted. Please re-run /review and wait for it to complete." + ) + ); exited_review_mode_idx = Some(event_idx); } EventMsg::TurnAborted(ev) => { diff --git a/codex-rs/core/src/review_prompts.rs b/codex-rs/core/src/review_prompts.rs index 3ef8009632..29bb7d503b 100644 --- a/codex-rs/core/src/review_prompts.rs +++ b/codex-rs/core/src/review_prompts.rs @@ -1,3 +1,4 @@ +use codex_git::branch_upstream; use codex_git::merge_base_with_head; use codex_protocol::protocol::ReviewRequest; use codex_protocol::protocol::ReviewTarget; @@ -13,6 +14,7 @@ pub struct ResolvedReviewRequest { const UNCOMMITTED_PROMPT: &str = "Review the current code changes (staged, unstaged, and untracked files) and provide prioritized findings."; const BASE_BRANCH_PROMPT_BACKUP: &str = "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."; +const BASE_BRANCH_PROMPT_WITH_UPSTREAM: &str = "Review the code changes against the base branch '{branch}', whose configured upstream is '{upstream}'. If tool permissions allow it, refresh that upstream ref first (for example with `git fetch`) so the comparison uses the latest remote state. Then compare '{branch}' and '{upstream}'. If '{upstream}' is ahead of '{branch}' at all, including diverged histories where both refs have unique commits, use '{upstream}' as the base ref for this review; otherwise use '{branch}'. Find the merge diff between the current branch and that chosen base ref, and run `git diff` against the merge-base SHA to inspect what would merge into {branch}. Provide prioritized, actionable findings."; const BASE_BRANCH_PROMPT: &str = "Review the code changes against the base branch '{baseBranch}'. The merge base commit for this comparison is {mergeBaseSha}. Run `git diff {mergeBaseSha}` to inspect the changes relative to {baseBranch}. Provide prioritized, actionable findings."; const COMMIT_PROMPT_WITH_TITLE: &str = "Review the code changes introduced by commit {sha} (\"{title}\"). Provide prioritized, actionable findings."; @@ -42,7 +44,11 @@ pub fn review_prompt(target: &ReviewTarget, cwd: &Path) -> anyhow::Result Ok(UNCOMMITTED_PROMPT.to_string()), ReviewTarget::BaseBranch { branch } => { - if let Some(commit) = merge_base_with_head(cwd, branch)? { + if let Some(upstream) = branch_upstream(cwd, branch)? { + Ok(BASE_BRANCH_PROMPT_WITH_UPSTREAM + .replace("{branch}", branch) + .replace("{upstream}", &upstream)) + } else if let Some(commit) = merge_base_with_head(cwd, branch)? { Ok(BASE_BRANCH_PROMPT .replace("{baseBranch}", branch) .replace("{mergeBaseSha}", &commit)) @@ -130,6 +136,7 @@ impl From for ReviewRequest { ReviewRequest { target: resolved.target, user_facing_hint: Some(resolved.user_facing_hint), + validate_findings: false, } } } @@ -138,6 +145,32 @@ impl From for ReviewRequest { mod tests { use super::*; use pretty_assertions::assert_eq; + use std::process::Command; + use tempfile::tempdir; + + fn run_git_in(repo_path: &Path, args: &[&str]) { + let status = Command::new("git") + .current_dir(repo_path) + .args(args) + .status() + .expect("git command"); + assert!(status.success(), "git command failed: {args:?}"); + } + + fn commit(repo_path: &Path, message: &str) { + run_git_in( + repo_path, + &[ + "-c", + "user.name=Tester", + "-c", + "user.email=test@example.com", + "commit", + "-m", + message, + ], + ); + } #[test] fn review_prompt_with_additional_instructions_appends_header() { @@ -172,4 +205,43 @@ mod tests { "Review the cumulative code changes from abc1234^ through HEAD, rooted at commit abc1234. Run `git diff abc1234^ HEAD` to inspect the updated stack. Provide prioritized, actionable findings." ); } + + #[test] + fn review_prompt_uses_newer_of_local_or_upstream_when_branch_tracks_remote() { + let temp = tempdir().expect("temp dir"); + let repo = temp.path().join("repo"); + let remote = temp.path().join("remote.git"); + std::fs::create_dir_all(&repo).expect("repo dir"); + std::fs::create_dir_all(&remote).expect("remote dir"); + + run_git_in(&remote, &["init", "--bare"]); + run_git_in(&repo, &["init", "--initial-branch=main"]); + run_git_in(&repo, &["config", "core.autocrlf", "false"]); + std::fs::write(repo.join("base.txt"), "base\n").expect("write base"); + run_git_in(&repo, &["add", "base.txt"]); + commit(&repo, "base commit"); + run_git_in( + &repo, + &[ + "remote", + "add", + "origin", + remote.to_str().expect("remote path"), + ], + ); + run_git_in(&repo, &["push", "-u", "origin", "main"]); + + let prompt = review_prompt( + &ReviewTarget::BaseBranch { + branch: "main".to_string(), + }, + &repo, + ) + .expect("prompt"); + + assert_eq!( + prompt, + "Review the code changes against the base branch 'main', whose configured upstream is 'origin/main'. If tool permissions allow it, refresh that upstream ref first (for example with `git fetch`) so the comparison uses the latest remote state. Then compare 'main' and 'origin/main'. If 'origin/main' is ahead of 'main' at all, including diverged histories where both refs have unique commits, use 'origin/main' as the base ref for this review; otherwise use 'main'. Find the merge diff between the current branch and that chosen base ref, and run `git diff` against the merge-base SHA to inspect what would merge into main. Provide prioritized, actionable findings." + ); + } } diff --git a/codex-rs/core/src/tasks/mod.rs b/codex-rs/core/src/tasks/mod.rs index a07e52f730..275fb8cbd7 100644 --- a/codex-rs/core/src/tasks/mod.rs +++ b/codex-rs/core/src/tasks/mod.rs @@ -44,6 +44,7 @@ use crate::features::Feature; pub(crate) use compact::CompactTask; pub(crate) use ghost_snapshot::GhostSnapshotTask; pub(crate) use regular::RegularTask; +pub(crate) use review::REVIEW_FINDING_VALIDATOR_SUBAGENT; pub(crate) use review::ReviewTask; pub(crate) use undo::UndoTask; pub(crate) use user_shell::UserShellCommandMode; diff --git a/codex-rs/core/src/tasks/review.rs b/codex-rs/core/src/tasks/review.rs index 1146be615d..61f2966d19 100644 --- a/codex-rs/core/src/tasks/review.rs +++ b/codex-rs/core/src/tasks/review.rs @@ -1,40 +1,93 @@ use std::sync::Arc; use async_trait::async_trait; +use codex_protocol::config_types::ModeKind; use codex_protocol::config_types::WebSearchMode; use codex_protocol::items::TurnItem; use codex_protocol::models::ContentItem; use codex_protocol::models::ResponseItem; +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemSandboxKind; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AgentMessageContentDeltaEvent; use codex_protocol::protocol::AgentMessageDeltaEvent; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::ExecCommandStatus; use codex_protocol::protocol::ExitedReviewModeEvent; use codex_protocol::protocol::ItemCompletedEvent; +use codex_protocol::protocol::ReadOnlyAccess; +use codex_protocol::protocol::ReviewFinding; use codex_protocol::protocol::ReviewOutputEvent; +use codex_protocol::protocol::SandboxPolicy; use codex_protocol::protocol::SubAgentSource; +use codex_protocol::user_input::UserInput; +use serde_json::Value; use tokio_util::sync::CancellationToken; use crate::codex::Session; use crate::codex::TurnContext; use crate::codex_delegate::run_codex_thread_one_shot; +use crate::config::Config; use crate::config::Constrained; +use crate::config::types::WindowsSandboxModeToml; use crate::features::Feature; use crate::review_format::format_review_findings_block; use crate::review_format::render_review_output_text; +use crate::review_prompts::ResolvedReviewRequest; use crate::state::TaskKind; -use codex_protocol::user_input::UserInput; use super::SessionTask; use super::SessionTaskContext; -#[derive(Clone, Copy)] -pub(crate) struct ReviewTask; +pub(crate) const REVIEW_FINDING_VALIDATOR_SUBAGENT: &str = "review_finding_validator"; +const REVIEW_FINDING_VALIDATION_FAILURE_EXPLANATION: &str = + "Review findings validation did not complete cleanly, so no findings were surfaced."; +const REVIEW_MALFORMED_OUTPUT_FAILURE_EXPLANATION: &str = + "Reviewer did not return valid structured output, so the review did not complete cleanly."; +const REVIEW_UNEXPECTED_VALIDATED_FINDINGS_FAILURE_EXPLANATION: &str = "Review findings validation returned findings that were not present in the original review, so no findings were surfaced."; +const REVIEW_CORRECT_VERDICT: &str = "patch is correct"; +const REVIEW_INCORRECT_VERDICT: &str = "patch is incorrect"; +const REVIEW_FINDING_VALIDATION_PROMPT: &str = "You are validating an existing code review against the current codebase.\n\ +Do not perform a fresh review.\n\ +Do not introduce new findings.\n\ +Only keep, discard, or clarify findings that already exist in the provided review output.\n\ +Use repository inspection as needed to verify the existing findings.\n\ +When you finish, return only strict JSON matching the provided schema."; +const REVIEW_SUBAGENT_REPO_GUIDANCE_PROMPT: &str = "Follow repository-specific guidance from inherited developer/user instructions when it affects review scope, diff selection, generated-file handling, or validation criteria.\n\ +Ignore workflow instructions unrelated to producing the review result, including notifications, commits, PR creation, or file edits."; +const REVIEW_INTERRUPTED_MESSAGE: &str = + "Review was interrupted. Please re-run /review and wait for it to complete."; + +enum ReviewExit { + Completed(ReviewOutputEvent), + Failed(String), + Interrupted, +} + +#[derive(Debug, PartialEq)] +enum ParsedReviewOutput { + Structured(ReviewOutputEvent), + Fallback(ReviewOutputEvent), +} + +#[derive(Clone)] +pub(crate) struct ReviewTask { + validate_findings: bool, + resolved_review_request: ResolvedReviewRequest, +} impl ReviewTask { - pub(crate) fn new() -> Self { - Self + pub(crate) fn new( + validate_findings: bool, + resolved_review_request: ResolvedReviewRequest, + ) -> Self { + Self { + validate_findings, + resolved_review_request, + } } } @@ -62,25 +115,43 @@ impl SessionTask for ReviewTask { .counter("codex.task.review", 1, &[]); // Start sub-codex conversation and get the receiver for events. - let output = match start_review_conversation( + let review_exit = match start_review_conversation( session.clone(), ctx.clone(), input, + self.validate_findings, cancellation_token.clone(), ) .await { - Some(receiver) => process_review_events(session.clone(), ctx.clone(), receiver).await, - None => None, + Some(receiver) => { + maybe_validate_review_output( + session.clone(), + ctx.clone(), + process_review_events( + session.clone(), + ctx.clone(), + receiver, + !self.validate_findings, + !self.validate_findings, + ) + .await, + self.validate_findings, + &self.resolved_review_request, + cancellation_token.clone(), + ) + .await + } + None => ReviewExit::Interrupted, }; if !cancellation_token.is_cancelled() { - exit_review_mode(session.clone_session(), output.clone(), ctx.clone()).await; + exit_review_mode(session.clone_session(), review_exit, ctx.clone()).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(), ReviewExit::Interrupted, ctx).await; } } @@ -88,29 +159,26 @@ async fn start_review_conversation( session: Arc, ctx: Arc, input: Vec, + validate_findings: bool, cancellation_token: CancellationToken, ) -> Option> { let config = ctx.config.clone(); - let mut sub_agent_config = config.as_ref().clone(); - // Carry over review-only feature restrictions so the delegate cannot - // re-enable blocked tools (web search, collab tools, view image). - if let Err(err) = sub_agent_config - .web_search_mode - .set(WebSearchMode::Disabled) - { - panic!("by construction Constrained must always support Disabled: {err}"); - } - let _ = sub_agent_config.features.disable(Feature::Collab); - - // Set explicit review rubric for the sub-agent - sub_agent_config.base_instructions = Some(crate::REVIEW_PROMPT.to_string()); - sub_agent_config.permissions.approval_policy = Constrained::allow_only(AskForApproval::Never); - let model = config .review_model .clone() .unwrap_or_else(|| ctx.model_info.slug.clone()); - sub_agent_config.model = Some(model); + let (sub_agent_config, _) = build_review_sub_agent_config( + config.as_ref(), + &model, + crate::REVIEW_PROMPT, + false, + validate_findings, + ); + let final_output_json_schema = if validate_findings { + Some(review_output_schema()) + } else { + None + }; (run_codex_thread_one_shot( sub_agent_config, session.auth_manager(), @@ -120,7 +188,7 @@ async fn start_review_conversation( ctx.clone(), cancellation_token, SubAgentSource::Review, - None, + final_output_json_schema, None, ) .await) @@ -128,16 +196,130 @@ async fn start_review_conversation( .map(|io| io.rx_event) } +fn build_review_sub_agent_config( + parent_config: &Config, + model: &str, + base_instructions: &str, + default_mode_request_user_input: bool, + force_read_only_sandbox: bool, +) -> (Config, bool) { + let mut sub_agent_config = parent_config.clone(); + // Carry over review-only feature restrictions so the delegate cannot + // re-enable blocked tools (web search, collab tools, view image). + if let Err(err) = sub_agent_config + .web_search_mode + .set(WebSearchMode::Disabled) + { + panic!("by construction Constrained must always support Disabled: {err}"); + } + let _ = sub_agent_config.features.disable(Feature::Collab); + let request_user_input_available = if default_mode_request_user_input { + let _ = sub_agent_config + .features + .enable(Feature::DefaultModeRequestUserInput); + ModeKind::Default.allows_request_user_input() + || sub_agent_config + .features + .enabled(Feature::DefaultModeRequestUserInput) + } else { + false + }; + if force_read_only_sandbox { + let parent_sandbox_policy = parent_config.permissions.sandbox_policy.get(); + let read_only_access = if parent_sandbox_policy.has_full_disk_read_access() { + ReadOnlyAccess::FullAccess + } else { + ReadOnlyAccess::Restricted { + include_platform_defaults: parent_sandbox_policy.include_platform_defaults(), + readable_roots: parent_sandbox_policy + .get_readable_roots_with_cwd(parent_config.cwd.as_path()), + } + }; + let read_only_policy = SandboxPolicy::ReadOnly { + access: read_only_access, + network_access: false, + }; + sub_agent_config.permissions.sandbox_policy = + Constrained::allow_only(read_only_policy.clone()); + sub_agent_config.permissions.file_system_sandbox_policy = + match parent_config.permissions.file_system_sandbox_policy.kind { + FileSystemSandboxKind::Restricted => FileSystemSandboxPolicy::restricted( + parent_config + .permissions + .file_system_sandbox_policy + .entries + .iter() + .cloned() + .map(|mut entry| { + entry.access = if entry.access.can_read() { + FileSystemAccessMode::Read + } else { + FileSystemAccessMode::None + }; + entry + }) + .collect(), + ), + FileSystemSandboxKind::Unrestricted | FileSystemSandboxKind::ExternalSandbox => { + FileSystemSandboxPolicy::from(&read_only_policy) + } + }; + sub_agent_config.permissions.network_sandbox_policy = + NetworkSandboxPolicy::from(&read_only_policy); + sub_agent_config.permissions.network = None; + sub_agent_config.permissions.windows_sandbox_mode = + Some(WindowsSandboxModeToml::Unelevated); + } + sub_agent_config.base_instructions = Some(format!( + "{base_instructions}\n\n{REVIEW_SUBAGENT_REPO_GUIDANCE_PROMPT}" + )); + sub_agent_config.permissions.approval_policy = Constrained::allow_only(AskForApproval::Never); + sub_agent_config.model = Some(model.to_string()); + (sub_agent_config, request_user_input_available) +} + +fn should_forward_hidden_review_diagnostic(msg: &EventMsg) -> bool { + match msg { + EventMsg::Error(_) | EventMsg::Warning(_) | EventMsg::StreamError(_) => true, + EventMsg::ExecCommandEnd(event) => !matches!(event.status, ExecCommandStatus::Completed), + EventMsg::PatchApplyEnd(event) => !event.success, + EventMsg::McpToolCallEnd(event) => !event.is_success(), + EventMsg::DynamicToolCallResponse(event) => !event.success, + _ => false, + } +} + +fn remap_hidden_review_diagnostic_to_parent_turn(msg: EventMsg, parent_turn_id: &str) -> EventMsg { + match msg { + EventMsg::ExecCommandEnd(mut event) => { + event.turn_id = parent_turn_id.to_string(); + EventMsg::ExecCommandEnd(event) + } + EventMsg::PatchApplyEnd(mut event) => { + event.turn_id = parent_turn_id.to_string(); + EventMsg::PatchApplyEnd(event) + } + EventMsg::DynamicToolCallResponse(mut event) => { + event.turn_id = parent_turn_id.to_string(); + EventMsg::DynamicToolCallResponse(event) + } + other => other, + } +} + async fn process_review_events( session: Arc, ctx: Arc, receiver: async_channel::Receiver, -) -> Option { + forward_intermediate_agent_messages: bool, + allow_wrapped_json: bool, +) -> Option { let mut prev_agent_message: Option = None; while let Ok(event) = receiver.recv().await { match event.clone().msg { EventMsg::AgentMessage(_) => { - if let Some(prev) = prev_agent_message.take() { + if forward_intermediate_agent_messages && let Some(prev) = prev_agent_message.take() + { session .clone_session() .send_event(ctx.as_ref(), prev.msg) @@ -154,18 +336,43 @@ async fn process_review_events( }) | EventMsg::AgentMessageDelta(AgentMessageDeltaEvent { .. }) | EventMsg::AgentMessageContentDelta(AgentMessageContentDeltaEvent { .. }) => {} + EventMsg::ItemCompleted(ItemCompletedEvent { + item: TurnItem::Reasoning(_), + .. + }) + | EventMsg::AgentReasoning(_) + | EventMsg::AgentReasoningDelta(_) + | EventMsg::AgentReasoningRawContent(_) + | EventMsg::AgentReasoningRawContentDelta(_) + | EventMsg::AgentReasoningSectionBreak(_) + | EventMsg::RawResponseItem(_) + | EventMsg::ReasoningContentDelta(_) + | EventMsg::ReasoningRawContentDelta(_) + if !forward_intermediate_agent_messages => {} EventMsg::TurnComplete(task_complete) => { // Parse review output from the last agent message (if present). let out = task_complete .last_agent_message .as_deref() - .map(parse_review_output_event); + .map(|text| parse_review_output_event(text, allow_wrapped_json)); return out; } EventMsg::TurnAborted(_) => { // Cancellation or abort: consumer will finalize with None. return None; } + _ if !forward_intermediate_agent_messages + && should_forward_hidden_review_diagnostic(&event.msg) => + { + session + .clone_session() + .send_event( + ctx.as_ref(), + remap_hidden_review_diagnostic_to_parent_turn(event.msg, &ctx.sub_id), + ) + .await; + } + _ if !forward_intermediate_agent_messages => {} other => { session .clone_session() @@ -178,57 +385,332 @@ async fn process_review_events( None } +async fn maybe_validate_review_output( + session: Arc, + ctx: Arc, + review_output: Option, + validate_findings: bool, + resolved_review_request: &ResolvedReviewRequest, + cancellation_token: CancellationToken, +) -> ReviewExit { + let Some(review_output) = review_output else { + return ReviewExit::Interrupted; + }; + let review_output = match review_output { + ParsedReviewOutput::Structured(review_output) => review_output, + ParsedReviewOutput::Fallback(review_output) => { + if validate_findings { + return ReviewExit::Failed(REVIEW_MALFORMED_OUTPUT_FAILURE_EXPLANATION.to_string()); + } + review_output + } + }; + if !validate_findings { + return ReviewExit::Completed(review_output); + } + if review_output.findings.is_empty() { + return ReviewExit::Completed(normalize_validated_review_output(review_output)); + } + + match run_review_validation_subagent( + session, + ctx, + review_output.clone(), + resolved_review_request, + cancellation_token.clone(), + ) + .await + { + Ok(Some(validated_output)) => match ensure_validated_findings_match_original( + &review_output.findings, + validated_output, + ) { + Ok(validated_output) => { + ReviewExit::Completed(normalize_validated_review_output(validated_output)) + } + Err(message) => ReviewExit::Failed(message), + }, + Ok(None) if cancellation_token.is_cancelled() => ReviewExit::Interrupted, + Ok(None) => ReviewExit::Failed(REVIEW_FINDING_VALIDATION_FAILURE_EXPLANATION.to_string()), + Err(message) => ReviewExit::Failed(message), + } +} + +async fn run_review_validation_subagent( + session: Arc, + ctx: Arc, + review_output: ReviewOutputEvent, + resolved_review_request: &ResolvedReviewRequest, + cancellation_token: CancellationToken, +) -> Result, String> { + let config = ctx.config.clone(); + let model = config + .review_model + .clone() + .unwrap_or_else(|| ctx.model_info.slug.clone()); + let (sub_agent_config, request_user_input_available) = build_review_sub_agent_config( + config.as_ref(), + &model, + REVIEW_FINDING_VALIDATION_PROMPT, + true, + true, + ); + let prompt = review_validation_prompt( + resolved_review_request, + &review_output, + request_user_input_available, + ); + let receiver = (run_codex_thread_one_shot( + sub_agent_config, + session.auth_manager(), + session.models_manager(), + vec![UserInput::Text { + text: prompt, + text_elements: Vec::new(), + }], + session.clone_session(), + ctx.clone(), + cancellation_token, + SubAgentSource::Other(REVIEW_FINDING_VALIDATOR_SUBAGENT.to_string()), + Some(review_output_schema()), + None, + ) + .await) + .ok() + .map(|io| io.rx_event) + .ok_or_else(|| REVIEW_FINDING_VALIDATION_FAILURE_EXPLANATION.to_string())?; + Ok(process_review_validation_events(session, ctx, receiver).await) +} + +async fn process_review_validation_events( + session: Arc, + ctx: Arc, + receiver: async_channel::Receiver, +) -> Option { + while let Ok(event) = receiver.recv().await { + match event.clone().msg { + EventMsg::TurnComplete(task_complete) => { + return task_complete + .last_agent_message + .as_deref() + .and_then(|text| serde_json::from_str::(text).ok()); + } + EventMsg::TurnAborted(_) => return None, + EventMsg::ItemCompleted(ItemCompletedEvent { + item: TurnItem::AgentMessage(_), + .. + }) + | EventMsg::AgentMessage(_) + | EventMsg::AgentMessageDelta(AgentMessageDeltaEvent { .. }) + | EventMsg::AgentMessageContentDelta(AgentMessageContentDeltaEvent { .. }) => {} + _ if should_forward_hidden_review_diagnostic(&event.msg) => { + session + .clone_session() + .send_event( + ctx.as_ref(), + remap_hidden_review_diagnostic_to_parent_turn(event.msg, &ctx.sub_id), + ) + .await; + } + _ => {} + } + } + None +} + /// Parse a ReviewOutputEvent from a text blob returned by the reviewer model. /// If the text is valid JSON matching ReviewOutputEvent, deserialize it. -/// Otherwise, attempt to extract the first JSON object substring and parse it. +/// If allowed, attempt to extract the first JSON object substring and parse it. /// If parsing still fails, return a structured fallback carrying the plain text /// in `overall_explanation`. -fn parse_review_output_event(text: &str) -> ReviewOutputEvent { +fn parse_review_output_event(text: &str, allow_wrapped_json: bool) -> ParsedReviewOutput { if let Ok(ev) = serde_json::from_str::(text) { - return ev; + return ParsedReviewOutput::Structured(ev); } - if let (Some(start), Some(end)) = (text.find('{'), text.rfind('}')) + if allow_wrapped_json + && let (Some(start), Some(end)) = (text.find('{'), text.rfind('}')) && start < end && let Some(slice) = text.get(start..=end) && let Ok(ev) = serde_json::from_str::(slice) { - return ev; + return ParsedReviewOutput::Structured(ev); } - ReviewOutputEvent { + ParsedReviewOutput::Fallback(ReviewOutputEvent { overall_explanation: text.to_string(), ..Default::default() - } + }) } -/// Emits an ExitedReviewMode Event with optional ReviewOutput, +fn review_validation_prompt( + resolved_review_request: &ResolvedReviewRequest, + review_output: &ReviewOutputEvent, + request_user_input_available: bool, +) -> String { + let review_context_json = serde_json::json!({ + "original_review_target_summary": resolved_review_request.user_facing_hint.trim(), + "original_review_scope": resolved_review_request.prompt.trim(), + "original_review_output": review_output, + }); + let review_context_json = serde_json::to_string_pretty(&review_context_json) + .or_else(|_| serde_json::to_string(&review_context_json)) + .unwrap_or_else(|_| render_review_output_text(review_output)); + let clarification_instruction = if request_user_input_available { + "If you are unsure whether a finding is valid or need clarification from the user before deciding, call `request_user_input`, wait for the answer, and then continue.\n\ +Do not keep uncertain findings unless you validated them or resolved the uncertainty with the user.\n\n" + } else { + "If you are unsure whether a finding is valid and user clarification would be required, do not guess.\n\ +Discard the finding and explain briefly in `overall_explanation` that it could not be confirmed from repository inspection alone.\n\n" + }; + format!( + "Validate the review findings below against the current codebase before they are surfaced to the user.\n\n\ +Use repository inspection to verify each finding.\n\ +Discard any finding that is incorrect, unsupported by the code, already fixed, or otherwise not actionable.\n\ +{clarification_instruction}\ +When you finish, return strict JSON matching the review output schema:\n\ +- `findings` must contain only the findings that remain valid after verification.\n\ +- Do not introduce any new finding that was not already present in the original review output.\n\ +- Each surviving finding must match one original finding exactly by `title`, `priority`, and `code_location`.\n\ +- You may adjust `body`, `overall_explanation`, and confidence scores when needed for accuracy.\n\ +- If no findings remain, return `findings: []`, set `overall_correctness` to `{REVIEW_CORRECT_VERDICT}`, and explain briefly what was discarded.\n\ +- If any findings remain, set `overall_correctness` to `{REVIEW_INCORRECT_VERDICT}`.\n\ +- `overall_explanation` should summarize what you kept and what you discarded.\n\ +- Do not include markdown fences or any text outside the JSON object.\n\n\ +Original review context JSON:\n\ +{review_context_json}" + ) +} + +fn review_output_schema() -> Value { + serde_json::json!({ + "type": "object", + "additionalProperties": false, + "properties": { + "findings": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "properties": { + "title": { "type": "string" }, + "body": { "type": "string" }, + "confidence_score": { "type": "number" }, + "priority": { "type": "integer" }, + "code_location": { + "type": "object", + "additionalProperties": false, + "properties": { + "absolute_file_path": { "type": "string" }, + "line_range": { + "type": "object", + "additionalProperties": false, + "properties": { + "start": { "type": "integer", "minimum": 0 }, + "end": { "type": "integer", "minimum": 0 } + }, + "required": ["start", "end"] + } + }, + "required": ["absolute_file_path", "line_range"] + } + }, + "required": [ + "title", + "body", + "confidence_score", + "priority", + "code_location" + ] + } + }, + "overall_correctness": { "type": "string" }, + "overall_explanation": { "type": "string" }, + "overall_confidence_score": { "type": "number" } + }, + "required": [ + "findings", + "overall_correctness", + "overall_explanation", + "overall_confidence_score" + ] + }) +} + +fn normalize_validated_review_output(mut review_output: ReviewOutputEvent) -> ReviewOutputEvent { + review_output.overall_correctness = if review_output.findings.is_empty() { + REVIEW_CORRECT_VERDICT.to_string() + } else { + REVIEW_INCORRECT_VERDICT.to_string() + }; + if review_output.findings.is_empty() && review_output.overall_explanation.trim().is_empty() { + review_output.overall_explanation = "No findings.".to_string(); + } + review_output +} + +fn ensure_validated_findings_match_original( + original_findings: &[ReviewFinding], + validated_output: ReviewOutputEvent, +) -> Result { + let mut matched_original_findings = vec![false; original_findings.len()]; + for validated_finding in &validated_output.findings { + let Some((matched_idx, _)) = + original_findings + .iter() + .enumerate() + .find(|(idx, original_finding)| { + !matched_original_findings[*idx] + && validated_finding.title == original_finding.title + && validated_finding.priority == original_finding.priority + && validated_finding.code_location == original_finding.code_location + }) + else { + return Err(REVIEW_UNEXPECTED_VALIDATED_FINDINGS_FAILURE_EXPLANATION.to_string()); + }; + matched_original_findings[matched_idx] = true; + } + Ok(validated_output) +} + +/// Emits an ExitedReviewMode Event with either review output or an explicit failure message, /// and records a developer message with the review output. -pub(crate) async fn exit_review_mode( - session: Arc, - review_output: Option, - ctx: Arc, -) { +async fn exit_review_mode(session: Arc, review_exit: ReviewExit, ctx: Arc) { const REVIEW_USER_MESSAGE_ID: &str = "review_rollout_user"; const REVIEW_ASSISTANT_MESSAGE_ID: &str = "review_rollout_assistant"; - let (user_message, assistant_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); + let (review_output, failure_message, user_message, assistant_message) = match review_exit { + ReviewExit::Completed(out) => { + 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}")); + } + let rendered = + crate::client_common::REVIEW_EXIT_SUCCESS_TMPL.replace("{results}", &findings_str); + let assistant_message = render_review_output_text(&out); + (Some(out), None, rendered, assistant_message) } - if !out.findings.is_empty() { - let block = format_review_findings_block(&out.findings, None); - findings_str.push_str(&format!("\n{block}")); + ReviewExit::Failed(message) => { + let assistant_message = if message.trim().is_empty() { + REVIEW_INTERRUPTED_MESSAGE.to_string() + } else { + message + }; + ( + None, + Some(assistant_message.clone()), + crate::client_common::REVIEW_EXIT_INTERRUPTED_TMPL.to_string(), + assistant_message, + ) } - let rendered = - crate::client_common::REVIEW_EXIT_SUCCESS_TMPL.replace("{results}", &findings_str); - let assistant_message = render_review_output_text(&out); - (rendered, assistant_message) - } else { - let rendered = crate::client_common::REVIEW_EXIT_INTERRUPTED_TMPL.to_string(); - let assistant_message = - "Review was interrupted. Please re-run /review and wait for it to complete." - .to_string(); - (rendered, assistant_message) + ReviewExit::Interrupted => ( + None, + Some(REVIEW_INTERRUPTED_MESSAGE.to_string()), + crate::client_common::REVIEW_EXIT_INTERRUPTED_TMPL.to_string(), + REVIEW_INTERRUPTED_MESSAGE.to_string(), + ), }; session @@ -247,7 +729,10 @@ pub(crate) async fn exit_review_mode( session .send_event( ctx.as_ref(), - EventMsg::ExitedReviewMode(ExitedReviewModeEvent { review_output }), + EventMsg::ExitedReviewMode(ExitedReviewModeEvent { + review_output, + failure_message, + }), ) .await; session @@ -270,3 +755,495 @@ pub(crate) async fn exit_review_mode( // file creation + git metadata collection cannot delay client-facing items. session.ensure_rollout_materialized().await; } + +#[cfg(test)] +mod tests { + use super::*; + use crate::codex::make_session_and_context_with_dynamic_tools_and_rx; + use crate::config::ManagedFeatures; + use crate::config::test_config; + use crate::config_loader::FeatureRequirementsToml; + use crate::config_loader::RequirementSource; + use crate::config_loader::Sourced; + use codex_protocol::permissions::FileSystemPath; + use codex_protocol::permissions::FileSystemSandboxEntry; + use codex_protocol::permissions::FileSystemSpecialPath; + use codex_protocol::protocol::Event; + use codex_protocol::protocol::ExecCommandEndEvent; + use codex_protocol::protocol::ExecCommandSource; + use codex_protocol::protocol::ReviewTarget; + use codex_protocol::protocol::TurnCompleteEvent; + use codex_utils_absolute_path::AbsolutePathBuf; + use pretty_assertions::assert_eq; + use std::collections::BTreeMap; + use std::path::PathBuf; + use std::time::Duration; + use tempfile::tempdir; + use tokio::time::timeout; + + fn failed_exec_end_event(call_id: &str) -> EventMsg { + EventMsg::ExecCommandEnd(ExecCommandEndEvent { + call_id: call_id.to_string(), + process_id: None, + turn_id: "hidden-review-turn".to_string(), + command: vec!["git".to_string(), "diff".to_string()], + cwd: PathBuf::from("/tmp"), + parsed_cmd: Vec::new(), + source: ExecCommandSource::Agent, + interaction_input: None, + stdout: String::new(), + stderr: "fatal: bad revision".to_string(), + aggregated_output: "fatal: bad revision".to_string(), + exit_code: 128, + duration: Duration::ZERO, + formatted_output: "fatal: bad revision".to_string(), + status: ExecCommandStatus::Failed, + }) + } + + async fn recv_forwarded_exec_failure( + rx_event: &async_channel::Receiver, + call_id: &str, + ) -> ExecCommandEndEvent { + timeout(Duration::from_secs(2), async { + loop { + let event = rx_event.recv().await.expect("forwarded event"); + if let EventMsg::ExecCommandEnd(end) = event.msg + && end.call_id == call_id + { + return end; + } + } + }) + .await + .expect("timed out waiting for forwarded exec failure") + } + + #[test] + fn validation_subagent_config_forces_read_only_sandbox() { + let mut parent_config = test_config(); + parent_config.user_instructions = Some("user".to_string()); + parent_config.developer_instructions = Some("developer".to_string()); + parent_config.permissions.sandbox_policy = + Constrained::allow_only(SandboxPolicy::DangerFullAccess); + parent_config.permissions.file_system_sandbox_policy = + FileSystemSandboxPolicy::from(&SandboxPolicy::DangerFullAccess); + parent_config.permissions.network_sandbox_policy = NetworkSandboxPolicy::Enabled; + parent_config.permissions.windows_sandbox_mode = None; + + let (validator_config, request_user_input_available) = build_review_sub_agent_config( + &parent_config, + "gpt-5", + REVIEW_FINDING_VALIDATION_PROMPT, + true, + true, + ); + + assert_eq!( + validator_config.permissions.sandbox_policy.get(), + &SandboxPolicy::new_read_only_policy() + ); + assert_eq!( + validator_config.permissions.file_system_sandbox_policy, + FileSystemSandboxPolicy::from(&SandboxPolicy::new_read_only_policy()) + ); + assert_eq!( + validator_config.permissions.network_sandbox_policy, + NetworkSandboxPolicy::Restricted + ); + assert_eq!(validator_config.permissions.network, None); + assert_eq!( + validator_config.permissions.windows_sandbox_mode, + Some(WindowsSandboxModeToml::Unelevated) + ); + assert_eq!(validator_config.user_instructions, Some("user".to_string())); + assert_eq!( + validator_config.developer_instructions, + Some("developer".to_string()) + ); + assert!( + validator_config + .base_instructions + .as_deref() + .expect("validator base instructions") + .contains(REVIEW_SUBAGENT_REPO_GUIDANCE_PROMPT) + ); + assert!(request_user_input_available); + } + + #[test] + fn validation_subagent_config_preserves_parent_read_restrictions() { + let temp = tempdir().expect("temp dir"); + let cwd = temp.path().join("repo"); + let readable_root = temp.path().join("readable"); + let writable_root = temp.path().join("scratch"); + let denied_root = writable_root.join("secret"); + std::fs::create_dir_all(&cwd).expect("cwd dir"); + std::fs::create_dir_all(&readable_root).expect("readable dir"); + std::fs::create_dir_all(&writable_root).expect("writable dir"); + std::fs::create_dir_all(&denied_root).expect("denied dir"); + + let cwd = AbsolutePathBuf::from_absolute_path(cwd).expect("absolute cwd"); + let readable_root = + AbsolutePathBuf::from_absolute_path(readable_root).expect("absolute readable root"); + let writable_root = + AbsolutePathBuf::from_absolute_path(writable_root).expect("absolute writable root"); + let denied_root = + AbsolutePathBuf::from_absolute_path(denied_root).expect("absolute denied root"); + + let mut parent_config = test_config(); + parent_config.cwd = cwd.to_path_buf(); + parent_config.permissions.sandbox_policy = + Constrained::allow_only(SandboxPolicy::WorkspaceWrite { + writable_roots: vec![writable_root.clone()], + read_only_access: ReadOnlyAccess::Restricted { + include_platform_defaults: false, + readable_roots: vec![readable_root.clone()], + }, + network_access: true, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }); + parent_config.permissions.file_system_sandbox_policy = + FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::CurrentWorkingDirectory, + }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: readable_root.clone(), + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: writable_root.clone(), + }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: denied_root.clone(), + }, + access: FileSystemAccessMode::None, + }, + ]); + parent_config.permissions.network_sandbox_policy = NetworkSandboxPolicy::Enabled; + + let (validator_config, _) = build_review_sub_agent_config( + &parent_config, + "gpt-5", + REVIEW_FINDING_VALIDATION_PROMPT, + true, + true, + ); + + assert_eq!( + validator_config.permissions.sandbox_policy.get(), + &SandboxPolicy::ReadOnly { + access: ReadOnlyAccess::Restricted { + include_platform_defaults: false, + readable_roots: vec![readable_root.clone(), cwd.clone(), writable_root.clone(),], + }, + network_access: false, + } + ); + assert_eq!( + validator_config.permissions.file_system_sandbox_policy, + FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::CurrentWorkingDirectory, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: readable_root.clone(), + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: writable_root.clone(), + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: denied_root }, + access: FileSystemAccessMode::None, + }, + ]) + ); + assert_eq!( + validator_config.permissions.network_sandbox_policy, + NetworkSandboxPolicy::Restricted + ); + } + + #[test] + fn review_subagent_config_preserves_parent_sandbox() { + let mut parent_config = test_config(); + parent_config.user_instructions = Some("user".to_string()); + parent_config.developer_instructions = Some("developer".to_string()); + parent_config.permissions.sandbox_policy = + Constrained::allow_only(SandboxPolicy::DangerFullAccess); + parent_config.permissions.file_system_sandbox_policy = + FileSystemSandboxPolicy::from(&SandboxPolicy::DangerFullAccess); + parent_config.permissions.network_sandbox_policy = NetworkSandboxPolicy::Enabled; + parent_config.permissions.windows_sandbox_mode = None; + + let (reviewer_config, request_user_input_available) = build_review_sub_agent_config( + &parent_config, + "gpt-5", + crate::REVIEW_PROMPT, + false, + false, + ); + + assert_eq!( + reviewer_config.permissions.sandbox_policy.get(), + &SandboxPolicy::DangerFullAccess + ); + assert_eq!( + reviewer_config.permissions.file_system_sandbox_policy, + FileSystemSandboxPolicy::from(&SandboxPolicy::DangerFullAccess) + ); + assert_eq!( + reviewer_config.permissions.network_sandbox_policy, + NetworkSandboxPolicy::Enabled + ); + assert_eq!(reviewer_config.permissions.windows_sandbox_mode, None); + assert_eq!(reviewer_config.user_instructions, Some("user".to_string())); + assert_eq!( + reviewer_config.developer_instructions, + Some("developer".to_string()) + ); + assert!( + reviewer_config + .base_instructions + .as_deref() + .expect("reviewer base instructions") + .contains(REVIEW_SUBAGENT_REPO_GUIDANCE_PROMPT) + ); + assert!(!request_user_input_available); + } + + #[test] + fn review_subagent_config_forces_read_only_sandbox_when_requested() { + let mut parent_config = test_config(); + parent_config.permissions.sandbox_policy = + Constrained::allow_only(SandboxPolicy::DangerFullAccess); + parent_config.permissions.file_system_sandbox_policy = + FileSystemSandboxPolicy::from(&SandboxPolicy::DangerFullAccess); + parent_config.permissions.network_sandbox_policy = NetworkSandboxPolicy::Enabled; + parent_config.permissions.windows_sandbox_mode = None; + + let (reviewer_config, request_user_input_available) = build_review_sub_agent_config( + &parent_config, + "gpt-5", + crate::REVIEW_PROMPT, + false, + true, + ); + + assert_eq!( + reviewer_config.permissions.sandbox_policy.get(), + &SandboxPolicy::new_read_only_policy() + ); + assert_eq!( + reviewer_config.permissions.file_system_sandbox_policy, + FileSystemSandboxPolicy::from(&SandboxPolicy::new_read_only_policy()) + ); + assert_eq!( + reviewer_config.permissions.network_sandbox_policy, + NetworkSandboxPolicy::Restricted + ); + assert_eq!(reviewer_config.permissions.network, None); + assert_eq!( + reviewer_config.permissions.windows_sandbox_mode, + Some(WindowsSandboxModeToml::Unelevated) + ); + assert!(!request_user_input_available); + } + + #[test] + fn validation_subagent_config_allows_pinned_off_request_user_input_feature() { + let mut parent_config = test_config(); + parent_config.features = ManagedFeatures::from_configured( + parent_config.features.get().clone(), + Some(Sourced { + value: FeatureRequirementsToml { + entries: BTreeMap::from([( + "default_mode_request_user_input".to_string(), + false, + )]), + }, + source: RequirementSource::Unknown, + }), + ) + .expect("managed features"); + + let (validator_config, request_user_input_available) = build_review_sub_agent_config( + &parent_config, + "gpt-5", + REVIEW_FINDING_VALIDATION_PROMPT, + true, + true, + ); + + assert!( + !request_user_input_available, + "request_user_input should remain unavailable when the feature is pinned off" + ); + assert!( + !validator_config + .features + .enabled(Feature::DefaultModeRequestUserInput) + ); + } + + #[test] + fn review_validation_prompt_omits_request_user_input_when_unavailable() { + let prompt = review_validation_prompt( + &ResolvedReviewRequest { + target: ReviewTarget::UncommittedChanges, + prompt: "Review the current changes".to_string(), + user_facing_hint: "Uncommitted changes".to_string(), + }, + &ReviewOutputEvent { + overall_explanation: "Initial review output".to_string(), + ..Default::default() + }, + false, + ); + + assert!(!prompt.contains("request_user_input")); + assert!(prompt.contains("Discard the finding")); + } + + #[tokio::test] + async fn review_event_pump_forwards_hidden_failed_exec_diagnostics() { + let (session, turn_context, rx_event) = + make_session_and_context_with_dynamic_tools_and_rx(Vec::new()).await; + let expected_turn_id = turn_context.sub_id.clone(); + let session = Arc::new(SessionTaskContext::new(session)); + let (tx_child, rx_child) = async_channel::unbounded(); + let call_id = "first-pass-exec-failure"; + let review_output = serde_json::json!({ + "findings": [], + "overall_correctness": "patch is correct", + "overall_explanation": "No findings.", + "overall_confidence_score": 0.8 + }) + .to_string(); + + let review_events = tokio::spawn(process_review_events( + session, + turn_context, + rx_child, + false, + false, + )); + + tx_child + .send(Event { + id: "child-1".to_string(), + msg: failed_exec_end_event(call_id), + }) + .await + .expect("send exec failure"); + tx_child + .send(Event { + id: "child-2".to_string(), + msg: EventMsg::TurnComplete(TurnCompleteEvent { + turn_id: "child-turn".to_string(), + last_agent_message: Some(review_output), + }), + }) + .await + .expect("send completion"); + drop(tx_child); + + let exec_failure = recv_forwarded_exec_failure(&rx_event, call_id).await; + assert_eq!(exec_failure.call_id, call_id); + assert_eq!(exec_failure.turn_id, expected_turn_id); + assert_eq!(exec_failure.status, ExecCommandStatus::Failed); + assert_eq!(exec_failure.exit_code, 128); + assert_eq!(exec_failure.formatted_output, "fatal: bad revision"); + + let parsed = review_events.await.expect("review event pump"); + assert_eq!( + parsed, + Some(ParsedReviewOutput::Structured(ReviewOutputEvent { + findings: Vec::new(), + overall_correctness: "patch is correct".to_string(), + overall_explanation: "No findings.".to_string(), + overall_confidence_score: 0.8, + })) + ); + } + + #[tokio::test] + async fn review_validation_event_pump_forwards_hidden_failed_exec_diagnostics() { + let (session, turn_context, rx_event) = + make_session_and_context_with_dynamic_tools_and_rx(Vec::new()).await; + let expected_turn_id = turn_context.sub_id.clone(); + let session = Arc::new(SessionTaskContext::new(session)); + let (tx_child, rx_child) = async_channel::unbounded(); + let call_id = "validator-exec-failure"; + let validated_output = serde_json::json!({ + "findings": [], + "overall_correctness": "patch is correct", + "overall_explanation": "Validated output.", + "overall_confidence_score": 0.9 + }) + .to_string(); + + let validation_events = tokio::spawn(process_review_validation_events( + session, + turn_context, + rx_child, + )); + + tx_child + .send(Event { + id: "child-1".to_string(), + msg: failed_exec_end_event(call_id), + }) + .await + .expect("send exec failure"); + tx_child + .send(Event { + id: "child-2".to_string(), + msg: EventMsg::TurnComplete(TurnCompleteEvent { + turn_id: "child-turn".to_string(), + last_agent_message: Some(validated_output), + }), + }) + .await + .expect("send completion"); + drop(tx_child); + + let exec_failure = recv_forwarded_exec_failure(&rx_event, call_id).await; + assert_eq!(exec_failure.call_id, call_id); + assert_eq!(exec_failure.turn_id, expected_turn_id); + assert_eq!(exec_failure.status, ExecCommandStatus::Failed); + assert_eq!(exec_failure.exit_code, 128); + assert_eq!(exec_failure.formatted_output, "fatal: bad revision"); + + let parsed = validation_events.await.expect("validation event pump"); + assert_eq!( + parsed, + Some(ReviewOutputEvent { + findings: Vec::new(), + overall_correctness: "patch is correct".to_string(), + overall_explanation: "Validated output.".to_string(), + overall_confidence_score: 0.9, + }) + ); + } +} diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index f3cec19a15..cb634f8f4d 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -7,6 +7,7 @@ use crate::features::Feature; use crate::features::Features; use crate::mcp_connection_manager::ToolInfo; use crate::models_manager::collaboration_mode_presets::CollaborationModesConfig; +use crate::tasks::REVIEW_FINDING_VALIDATOR_SUBAGENT; use crate::tools::handlers::PLAN_TOOL; use crate::tools::handlers::SEARCH_TOOL_BM25_DEFAULT_LIMIT; use crate::tools::handlers::SEARCH_TOOL_BM25_TOOL_NAME; @@ -85,6 +86,20 @@ pub(crate) struct ToolsConfigParams<'a> { pub(crate) session_source: SessionSource, } +fn session_source_allows_request_user_input( + session_source: &SessionSource, + features: &Features, +) -> bool { + match session_source { + SessionSource::SubAgent(SubAgentSource::Other(label)) => { + label == REVIEW_FINDING_VALIDATOR_SUBAGENT + && features.enabled(Feature::DefaultModeRequestUserInput) + } + SessionSource::SubAgent(_) => false, + _ => true, + } +} + impl ToolsConfig { pub fn new(params: &ToolsConfigParams) -> Self { let ToolsConfigParams { @@ -98,7 +113,8 @@ impl ToolsConfig { let include_js_repl_tools_only = include_js_repl && features.enabled(Feature::JsReplToolsOnly); let include_collab_tools = features.enabled(Feature::Collab); - let include_request_user_input = !matches!(session_source, SessionSource::SubAgent(_)); + let include_request_user_input = + session_source_allows_request_user_input(session_source, features); let include_default_mode_request_user_input = include_request_user_input && features.enabled(Feature::DefaultModeRequestUserInput); let include_search_tool = features.enabled(Feature::Apps); @@ -2420,6 +2436,43 @@ mod tests { assert_lacks_tool_name(&tools, "request_user_input"); } + #[test] + fn test_build_specs_review_finding_validator_includes_request_user_input() { + let config = test_config(); + let model_info = + ModelsManager::construct_model_info_offline_for_tests("gpt-5-codex", &config); + let mut features = Features::with_defaults(); + features.enable(Feature::DefaultModeRequestUserInput); + let tools_config = ToolsConfig::new(&ToolsConfigParams { + model_info: &model_info, + features: &features, + web_search_mode: Some(WebSearchMode::Cached), + session_source: SessionSource::SubAgent(SubAgentSource::Other( + REVIEW_FINDING_VALIDATOR_SUBAGENT.to_string(), + )), + }); + let (tools, _) = build_specs(&tools_config, None, None, &[]).build(); + assert_contains_tool_names(&tools, &["request_user_input"]); + } + + #[test] + fn test_build_specs_review_finding_validator_omits_request_user_input_when_disabled() { + let config = test_config(); + let model_info = + ModelsManager::construct_model_info_offline_for_tests("gpt-5-codex", &config); + let features = Features::with_defaults(); + let tools_config = ToolsConfig::new(&ToolsConfigParams { + model_info: &model_info, + features: &features, + web_search_mode: Some(WebSearchMode::Cached), + session_source: SessionSource::SubAgent(SubAgentSource::Other( + REVIEW_FINDING_VALIDATOR_SUBAGENT.to_string(), + )), + }); + let (tools, _) = build_specs(&tools_config, None, None, &[]).build(); + assert_lacks_tool_name(&tools, "request_user_input"); + } + #[test] fn request_user_input_description_reflects_default_mode_feature_flag() { let config = test_config(); diff --git a/codex-rs/core/tests/suite/codex_delegate.rs b/codex-rs/core/tests/suite/codex_delegate.rs index e71d99b620..21e9ac5d93 100644 --- a/codex-rs/core/tests/suite/codex_delegate.rs +++ b/codex-rs/core/tests/suite/codex_delegate.rs @@ -77,6 +77,7 @@ async fn codex_delegate_forwards_exec_approval_and_proceeds_on_approval() { instructions: "Please review".to_string(), }, user_facing_hint: None, + validate_findings: false, }, }) .await @@ -160,6 +161,7 @@ async fn codex_delegate_forwards_patch_approval_and_proceeds_on_decision() { instructions: "Please review".to_string(), }, user_facing_hint: None, + validate_findings: false, }, }) .await @@ -219,6 +221,7 @@ async fn codex_delegate_ignores_legacy_deltas() { instructions: "Please review".to_string(), }, user_facing_hint: None, + validate_findings: false, }, }) .await diff --git a/codex-rs/core/tests/suite/review.rs b/codex-rs/core/tests/suite/review.rs index 3a18cf157b..d2bfefc27b 100644 --- a/codex-rs/core/tests/suite/review.rs +++ b/codex-rs/core/tests/suite/review.rs @@ -2,6 +2,9 @@ use codex_core::CodexThread; use codex_core::REVIEW_PROMPT; use codex_core::config::Config; use codex_core::review_format::render_review_output_text; +use codex_protocol::config_types::CollaborationMode; +use codex_protocol::config_types::ModeKind; +use codex_protocol::config_types::Settings; use codex_protocol::models::ContentItem; use codex_protocol::models::ResponseItem; use codex_protocol::protocol::ENVIRONMENT_CONTEXT_OPEN_TAG; @@ -16,15 +19,29 @@ use codex_protocol::protocol::ReviewRequest; use codex_protocol::protocol::ReviewTarget; use codex_protocol::protocol::RolloutItem; use codex_protocol::protocol::RolloutLine; +use codex_protocol::request_user_input::RequestUserInputAnswer; +use codex_protocol::request_user_input::RequestUserInputResponse; use codex_protocol::user_input::UserInput; use core_test_support::load_sse_fixture_with_id_from_str; use core_test_support::responses::ResponseMock; +use core_test_support::responses::ev_assistant_message; +use core_test_support::responses::ev_completed; +use core_test_support::responses::ev_function_call; +use core_test_support::responses::ev_reasoning_item; +use core_test_support::responses::ev_reasoning_item_added; +use core_test_support::responses::ev_reasoning_summary_text_delta; +use core_test_support::responses::ev_reasoning_text_delta; +use core_test_support::responses::ev_response_created; use core_test_support::responses::mount_sse_sequence; +use core_test_support::responses::sse; +use core_test_support::responses::sse_failed; use core_test_support::responses::start_mock_server; use core_test_support::skip_if_no_network; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_event; +use core_test_support::wait_for_event_match; use pretty_assertions::assert_eq; +use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; use tempfile::TempDir; @@ -81,6 +98,7 @@ async fn review_op_emits_lifecycle_and_review_output() { instructions: "Please review my changes".to_string(), }, user_facing_hint: None, + validate_findings: false, }, }) .await @@ -202,6 +220,7 @@ async fn review_op_with_plain_text_emits_review_fallback() { instructions: "Plain text review".to_string(), }, user_facing_hint: None, + validate_findings: false, }, }) .await @@ -228,6 +247,158 @@ async fn review_op_with_plain_text_emits_review_fallback() { server.verify().await; } +#[cfg_attr(windows, tokio::test(flavor = "multi_thread", worker_threads = 4))] +#[cfg_attr(not(windows), tokio::test(flavor = "multi_thread", worker_threads = 2))] +async fn review_op_with_plain_text_and_findings_validation_fails_closed() { + skip_if_no_network!(); + + let sse_raw = r#"[ + {"type":"response.output_item.done", "item":{ + "type":"message", "role":"assistant", + "content":[{"type":"output_text","text":"just plain text"}] + }}, + {"type":"response.completed", "response": {"id": "__ID__"}} + ]"#; + let (server, request_log) = start_responses_server_with_sse(sse_raw, 1).await; + let codex_home = Arc::new(TempDir::new().unwrap()); + let codex = new_conversation_for_server(&server, codex_home.clone(), |_| {}).await; + + codex + .submit(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::Custom { + instructions: "Plain text review with validation".to_string(), + }, + user_facing_hint: None, + validate_findings: true, + }, + }) + .await + .unwrap(); + + let _entered = wait_for_event(&codex, |ev| matches!(ev, EventMsg::EnteredReviewMode(_))).await; + let closed = wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExitedReviewMode(_))).await; + let failure_message = match closed { + EventMsg::ExitedReviewMode(ev) => { + assert_eq!(ev.review_output, None); + ev.failure_message + .expect("expected explicit failure message") + } + other => panic!("expected ExitedReviewMode(..), got {other:?}"), + }; + assert_eq!( + failure_message, + "Reviewer did not return valid structured output, so the review did not complete cleanly." + ); + let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + let rollout_path = codex.rollout_path().expect("rollout path"); + let rollout = std::fs::read_to_string(&rollout_path).expect("read rollout file"); + assert!( + rollout.contains("but was interrupted"), + "malformed reviewer output should persist as an interrupted review" + ); + assert!( + !rollout.contains("just plain text"), + "malformed reviewer output should not be persisted as a successful review result" + ); + + let request_body = request_log.single_request().body_json(); + assert_eq!( + request_body["text"]["format"]["type"].as_str(), + Some("json_schema") + ); + assert_eq!( + request_body["text"]["format"]["strict"].as_bool(), + Some(true) + ); + + let _codex_home_guard = codex_home; + server.verify().await; +} + +#[cfg_attr(windows, tokio::test(flavor = "multi_thread", worker_threads = 4))] +#[cfg_attr(not(windows), tokio::test(flavor = "multi_thread", worker_threads = 2))] +async fn review_op_with_wrapped_json_and_findings_validation_fails_closed() { + skip_if_no_network!(); + + let wrapped_review = format!( + "Here is the review:\n{}", + serde_json::json!({ + "findings": [ + { + "title": "Wrapped finding", + "body": "This wrapped JSON should not be accepted during validated review.", + "confidence_score": 0.8, + "priority": 1, + "code_location": { + "absolute_file_path": "/tmp/wrapped.rs", + "line_range": {"start": 10, "end": 11} + } + } + ], + "overall_correctness": "patch is incorrect", + "overall_explanation": "Wrapped JSON should fail closed.", + "overall_confidence_score": 0.8 + }) + ); + let sse_raw = format!( + r#"[ + {{"type":"response.output_item.done", "item":{{ + "type":"message", "role":"assistant", + "content":[{{"type":"output_text","text":{wrapped_review:?}}}] + }}}}, + {{"type":"response.completed", "response": {{"id": "__ID__"}}}} + ]"# + ); + let (server, _request_log) = start_responses_server_with_sse(&sse_raw, 1).await; + let codex_home = Arc::new(TempDir::new().unwrap()); + let codex = new_conversation_for_server(&server, codex_home.clone(), |_| {}).await; + + codex + .submit(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::Custom { + instructions: "Wrapped JSON review with validation".to_string(), + }, + user_facing_hint: None, + validate_findings: true, + }, + }) + .await + .unwrap(); + + let _entered = wait_for_event(&codex, |ev| matches!(ev, EventMsg::EnteredReviewMode(_))).await; + let closed = wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExitedReviewMode(_))).await; + let failure_message = match closed { + EventMsg::ExitedReviewMode(ev) => { + assert_eq!(ev.review_output, None); + ev.failure_message + .expect("expected explicit failure message") + } + other => panic!("expected ExitedReviewMode(..), got {other:?}"), + }; + assert_eq!( + failure_message, + "Reviewer did not return valid structured output, so the review did not complete cleanly." + ); + let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + let rollout_path = codex.rollout_path().expect("rollout path"); + let rollout = std::fs::read_to_string(&rollout_path).expect("read rollout file"); + assert!( + rollout.contains("but was interrupted"), + "wrapped reviewer output should persist as an interrupted review" + ); + assert!( + !rollout.contains("Wrapped finding"), + "wrapped reviewer output should not be persisted as a successful review result" + ); + + let _codex_home_guard = codex_home; + server.verify().await; +} + /// Ensure review flow suppresses assistant-specific streaming/completion events: /// - AgentMessageContentDelta /// - AgentMessageDelta (legacy) @@ -263,6 +434,7 @@ async fn review_filters_agent_message_related_events() { instructions: "Filter streaming events".to_string(), }, user_facing_hint: None, + validate_findings: false, }, }) .await @@ -345,6 +517,7 @@ async fn review_does_not_emit_agent_message_on_structured_output() { instructions: "check structured".to_string(), }, user_facing_hint: None, + validate_findings: false, }, }) .await @@ -405,6 +578,7 @@ async fn review_uses_custom_review_model_from_config() { instructions: "use custom model".to_string(), }, user_facing_hint: None, + validate_findings: false, }, }) .await @@ -416,7 +590,8 @@ async fn review_uses_custom_review_model_from_config() { matches!( ev, EventMsg::ExitedReviewMode(ExitedReviewModeEvent { - review_output: None + review_output: None, + .. }) ) }) @@ -458,6 +633,7 @@ async fn review_uses_session_model_when_review_model_unset() { instructions: "use session model".to_string(), }, user_facing_hint: None, + validate_findings: false, }, }) .await @@ -468,7 +644,8 @@ async fn review_uses_session_model_when_review_model_unset() { matches!( ev, EventMsg::ExitedReviewMode(ExitedReviewModeEvent { - review_output: None + review_output: None, + .. }) ) }) @@ -576,6 +753,7 @@ async fn review_input_isolated_from_parent_history() { instructions: review_prompt.clone(), }, user_facing_hint: None, + validate_findings: false, }, }) .await @@ -586,7 +764,8 @@ async fn review_input_isolated_from_parent_history() { matches!( ev, EventMsg::ExitedReviewMode(ExitedReviewModeEvent { - review_output: None + review_output: None, + .. }) ) }) @@ -629,7 +808,16 @@ async fn review_input_isolated_from_parent_history() { // Ensure the REVIEW_PROMPT rubric is sent via instructions. let instructions = body["instructions"].as_str().expect("instructions string"); - assert_eq!(instructions, REVIEW_PROMPT); + assert!( + instructions.starts_with(REVIEW_PROMPT), + "review instructions should start with the review rubric" + ); + assert!( + instructions.contains( + "Follow repository-specific guidance from inherited developer/user instructions" + ), + "review instructions should preserve repo guidance for review subagents" + ); // Also verify that a user interruption note was recorded in the rollout. let path = codex.rollout_path().expect("rollout path"); @@ -692,6 +880,7 @@ async fn review_history_surfaces_in_parent_session() { instructions: "Start a review".to_string(), }, user_facing_hint: None, + validate_findings: false, }, }) .await @@ -701,7 +890,8 @@ async fn review_history_surfaces_in_parent_session() { matches!( ev, EventMsg::ExitedReviewMode(ExitedReviewModeEvent { - review_output: Some(_) + review_output: Some(_), + .. }) ) }) @@ -765,6 +955,1320 @@ async fn review_history_surfaces_in_parent_session() { server.verify().await; } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn review_validation_filters_transcript_and_parent_context() { + skip_if_no_network!(); + let draft_review_message = + "Draft review note that should stay hidden until validation completes."; + + let initial_review_json = serde_json::json!({ + "findings": [ + { + "title": "Stale finding", + "body": "This report should be discarded by validation.", + "confidence_score": 0.9, + "priority": 1, + "code_location": { + "absolute_file_path": "/tmp/stale.rs", + "line_range": {"start": 1, "end": 2} + } + }, + { + "title": "Validated finding", + "body": "This report should survive validation with a clearer explanation.", + "confidence_score": 0.7, + "priority": 1, + "code_location": { + "absolute_file_path": "/tmp/validated.rs", + "line_range": {"start": 5, "end": 6} + } + } + ], + "overall_correctness": "incorrect", + "overall_explanation": "Initial review output before validation.", + "overall_confidence_score": 0.8 + }) + .to_string(); + let validated_output = ReviewOutputEvent { + findings: vec![ReviewFinding { + title: "Validated finding".to_string(), + body: "This finding survived validation.".to_string(), + confidence_score: 0.95, + priority: 1, + code_location: ReviewCodeLocation { + absolute_file_path: PathBuf::from("/tmp/validated.rs"), + line_range: ReviewLineRange { start: 5, end: 6 }, + }, + }], + overall_correctness: "patch is incorrect".to_string(), + overall_explanation: "Kept the validated finding after discarding the stale report." + .to_string(), + overall_confidence_score: 0.91, + }; + let validated_json = serde_json::to_string(&validated_output).expect("validated json"); + + let server = start_mock_server().await; + let request_log = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_assistant_message("msg-draft", draft_review_message), + ev_assistant_message("msg-1", &initial_review_json), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-2", &validated_json), + ev_completed("resp-2"), + ]), + sse(vec![ + ev_response_created("resp-3"), + ev_assistant_message("msg-3", "parent ack"), + ev_completed("resp-3"), + ]), + ], + ) + .await; + + let codex_home = Arc::new(TempDir::new().unwrap()); + let codex = new_conversation_for_server(&server, codex_home.clone(), |_| {}).await; + + codex + .submit(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::Custom { + instructions: "Validate review findings".to_string(), + }, + user_facing_hint: None, + validate_findings: true, + }, + }) + .await + .unwrap(); + + let _entered = wait_for_event(&codex, |ev| matches!(ev, EventMsg::EnteredReviewMode(_))).await; + let closed = wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExitedReviewMode(_))).await; + let review = match closed { + EventMsg::ExitedReviewMode(ev) => ev + .review_output + .expect("expected ExitedReviewMode with Some(review_output)"), + other => panic!("expected ExitedReviewMode(..), got {other:?}"), + }; + assert_eq!(validated_output, review); + let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + let rollout_path = codex.rollout_path().expect("rollout path"); + let rollout = std::fs::read_to_string(&rollout_path).expect("read rollout file"); + assert!( + rollout.contains("Validated finding"), + "validated finding missing from rollout" + ); + assert!( + !rollout.contains("Stale finding"), + "stale finding leaked into rollout" + ); + assert!( + !rollout.contains(draft_review_message), + "draft reviewer message leaked into rollout before validation" + ); + assert!( + !rollout.contains( + "Validate the review findings below against the current codebase before they are surfaced to the user." + ), + "validator prompt leaked into rollout" + ); + + codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: "back to parent".to_string(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + }) + .await + .unwrap(); + let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + let requests = request_log.requests(); + assert_eq!(requests.len(), 3); + let follow_up_input = requests[2].input(); + let follow_up_text = follow_up_input + .iter() + .filter_map(|msg| msg.get("content").and_then(|content| content.as_array())) + .flat_map(|content| content.iter()) + .filter_map(|entry| entry.get("text").and_then(|text| text.as_str())) + .collect::>() + .join("\n"); + assert!( + follow_up_text.contains("Validated finding"), + "validated finding missing from downstream parent context" + ); + assert!( + !follow_up_text.contains("Stale finding"), + "stale finding leaked into downstream parent context" + ); + assert!( + !follow_up_text.contains(draft_review_message), + "draft reviewer message leaked into downstream parent context" + ); + assert!( + !follow_up_text.contains( + "Validate the review findings below against the current codebase before they are surfaced to the user." + ), + "validator prompt leaked into downstream parent context" + ); + + let _codex_home_guard = codex_home; + server.verify().await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn review_validation_suppresses_reasoning_and_raw_events_before_exit() { + skip_if_no_network!(); + + let reasoning_summary = "Draft reasoning finding that must stay hidden."; + let reasoning_raw = "Draft raw reasoning detail that must stay hidden."; + let initial_review_json = serde_json::json!({ + "findings": [ + { + "title": "Validated finding", + "body": "This finding should survive validation.", + "confidence_score": 0.8, + "priority": 1, + "code_location": { + "absolute_file_path": "/tmp/validated.rs", + "line_range": {"start": 5, "end": 6} + } + } + ], + "overall_correctness": "incorrect", + "overall_explanation": "Initial review output before validation.", + "overall_confidence_score": 0.8 + }) + .to_string(); + let validated_output = ReviewOutputEvent { + findings: vec![ReviewFinding { + title: "Validated finding".to_string(), + body: "This finding survived validation.".to_string(), + confidence_score: 0.95, + priority: 1, + code_location: ReviewCodeLocation { + absolute_file_path: PathBuf::from("/tmp/validated.rs"), + line_range: ReviewLineRange { start: 5, end: 6 }, + }, + }], + overall_correctness: "patch is incorrect".to_string(), + overall_explanation: "Kept the validated finding.".to_string(), + overall_confidence_score: 0.91, + }; + let validated_json = serde_json::to_string(&validated_output).expect("validated json"); + + let server = start_mock_server().await; + let _request_log = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_reasoning_item_added("reasoning-1", &[""]), + ev_reasoning_summary_text_delta(reasoning_summary), + ev_reasoning_text_delta(reasoning_raw), + ev_reasoning_item("reasoning-1", &[reasoning_summary], &[reasoning_raw]), + ev_assistant_message("msg-1", &initial_review_json), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-2", &validated_json), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let codex_home = Arc::new(TempDir::new().unwrap()); + let codex = new_conversation_for_server(&server, codex_home.clone(), |_| {}).await; + + codex + .submit(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::Custom { + instructions: "Validate review findings".to_string(), + }, + user_facing_hint: None, + validate_findings: true, + }, + }) + .await + .unwrap(); + + let mut leaked_event_kinds = Vec::new(); + loop { + match wait_for_event(&codex, |_| true).await { + EventMsg::EnteredReviewMode(_) => {} + EventMsg::AgentReasoning(_) => leaked_event_kinds.push("AgentReasoning"), + EventMsg::AgentReasoningDelta(_) => leaked_event_kinds.push("AgentReasoningDelta"), + EventMsg::AgentReasoningRawContent(_) => { + leaked_event_kinds.push("AgentReasoningRawContent"); + } + EventMsg::AgentReasoningRawContentDelta(_) => { + leaked_event_kinds.push("AgentReasoningRawContentDelta"); + } + EventMsg::AgentReasoningSectionBreak(_) => { + leaked_event_kinds.push("AgentReasoningSectionBreak"); + } + EventMsg::RawResponseItem(raw) + if matches!(raw.item, ResponseItem::Reasoning { .. }) => + { + leaked_event_kinds.push("RawResponseItem::Reasoning"); + } + EventMsg::ReasoningContentDelta(_) => leaked_event_kinds.push("ReasoningContentDelta"), + EventMsg::ReasoningRawContentDelta(_) => { + leaked_event_kinds.push("ReasoningRawContentDelta"); + } + EventMsg::ExitedReviewMode(ev) => { + assert_eq!(ev.review_output, Some(validated_output.clone())); + break; + } + _ => {} + } + } + + assert!( + leaked_event_kinds.is_empty(), + "reasoning/raw events leaked before validation finished: {leaked_event_kinds:?}" + ); + let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + let _codex_home_guard = codex_home; + server.verify().await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn review_validation_suppresses_first_pass_tool_events_before_exit() { + skip_if_no_network!(); + + let call_id = "review-validation-exec"; + let exec_args = serde_json::json!({ + "cmd": "", + "yield_time_ms": 1_000, + }) + .to_string(); + let initial_review_json = serde_json::json!({ + "findings": [ + { + "title": "Validated finding", + "body": "This finding should survive validation.", + "confidence_score": 0.8, + "priority": 1, + "code_location": { + "absolute_file_path": "/tmp/validated.rs", + "line_range": {"start": 5, "end": 6} + } + } + ], + "overall_correctness": "incorrect", + "overall_explanation": "Initial review output before validation.", + "overall_confidence_score": 0.8 + }) + .to_string(); + let validated_output = ReviewOutputEvent { + findings: vec![ReviewFinding { + title: "Validated finding".to_string(), + body: "This finding survived validation.".to_string(), + confidence_score: 0.95, + priority: 1, + code_location: ReviewCodeLocation { + absolute_file_path: PathBuf::from("/tmp/validated.rs"), + line_range: ReviewLineRange { start: 5, end: 6 }, + }, + }], + overall_correctness: "patch is incorrect".to_string(), + overall_explanation: "Kept the validated finding.".to_string(), + overall_confidence_score: 0.91, + }; + let validated_json = serde_json::to_string(&validated_output).expect("validated json"); + + let server = start_mock_server().await; + let _request_log = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call(call_id, "exec_command", &exec_args), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", &initial_review_json), + ev_completed("resp-2"), + ]), + sse(vec![ + ev_response_created("resp-3"), + ev_assistant_message("msg-2", &validated_json), + ev_completed("resp-3"), + ]), + ], + ) + .await; + + let codex_home = Arc::new(TempDir::new().unwrap()); + let codex = new_conversation_for_server(&server, codex_home.clone(), |_| {}).await; + + codex + .submit(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::Custom { + instructions: "Validate review findings".to_string(), + }, + user_facing_hint: None, + validate_findings: true, + }, + }) + .await + .unwrap(); + + let mut leaked_event_kinds = Vec::new(); + loop { + match wait_for_event(&codex, |_| true).await { + EventMsg::EnteredReviewMode(_) => {} + EventMsg::ExecCommandBegin(_) => leaked_event_kinds.push("ExecCommandBegin"), + EventMsg::ExecCommandOutputDelta(_) => { + leaked_event_kinds.push("ExecCommandOutputDelta"); + } + EventMsg::ExecCommandEnd(_) => leaked_event_kinds.push("ExecCommandEnd"), + EventMsg::ExitedReviewMode(ev) => { + assert_eq!(ev.review_output, Some(validated_output.clone())); + break; + } + _ => {} + } + } + + assert!( + leaked_event_kinds.is_empty(), + "tool events leaked before validation finished: {leaked_event_kinds:?}" + ); + let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + let _codex_home_guard = codex_home; + server.verify().await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn review_validation_preserves_first_pass_error_diagnostics_before_exit() { + skip_if_no_network!(); + + let server = start_mock_server().await; + let _request_log = mount_sse_sequence( + &server, + vec![sse_failed( + "resp-1", + "server_error", + "simulated first-pass review failure", + )], + ) + .await; + + let codex_home = Arc::new(TempDir::new().unwrap()); + let codex = new_conversation_for_server(&server, codex_home.clone(), |config| { + config.model_provider.stream_max_retries = Some(0); + }) + .await; + + codex + .submit(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::Custom { + instructions: "Validate review findings".to_string(), + }, + user_facing_hint: None, + validate_findings: true, + }, + }) + .await + .unwrap(); + + let _entered = wait_for_event(&codex, |ev| matches!(ev, EventMsg::EnteredReviewMode(_))).await; + let error_message = wait_for_event_match(&codex, |event| match event { + EventMsg::Error(err) => Some(err.message.clone()), + _ => None, + }) + .await; + assert!( + error_message.contains("simulated first-pass review failure"), + "expected first-pass error to surface before exit, got {error_message}" + ); + + let exited = wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExitedReviewMode(_))).await; + let EventMsg::ExitedReviewMode(exited) = exited else { + unreachable!("wait_for_event returned non-review-exit event"); + }; + assert_eq!(exited.review_output, None); + assert_eq!( + exited.failure_message, + Some( + "Review was interrupted. Please re-run /review and wait for it to complete." + .to_string(), + ) + ); + + let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + let _codex_home_guard = codex_home; + server.verify().await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn review_validation_failure_exits_with_unstructured_error_output() { + skip_if_no_network!(); + + let initial_review_json = serde_json::json!({ + "findings": [ + { + "title": "Needs validation", + "body": "This finding should not be surfaced if validation fails.", + "confidence_score": 0.8, + "priority": 1, + "code_location": { + "absolute_file_path": "/tmp/needs-validation.rs", + "line_range": {"start": 11, "end": 12} + } + } + ], + "overall_correctness": "incorrect", + "overall_explanation": "Initial review output before validation failure.", + "overall_confidence_score": 0.8 + }) + .to_string(); + + let server = start_mock_server().await; + let request_log = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_assistant_message("msg-1", &initial_review_json), + ev_completed("resp-1"), + ]), + sse(vec![ev_response_created("resp-2"), ev_completed("resp-2")]), + sse(vec![ + ev_response_created("resp-3"), + ev_assistant_message("msg-2", "parent ack"), + ev_completed("resp-3"), + ]), + ], + ) + .await; + + let codex_home = Arc::new(TempDir::new().unwrap()); + let codex = new_conversation_for_server(&server, codex_home.clone(), |_| {}).await; + + codex + .submit(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::Custom { + instructions: "Surface validation failure".to_string(), + }, + user_facing_hint: None, + validate_findings: true, + }, + }) + .await + .unwrap(); + + let _entered = wait_for_event(&codex, |ev| matches!(ev, EventMsg::EnteredReviewMode(_))).await; + let closed = wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExitedReviewMode(_))).await; + let failure_message = match closed { + EventMsg::ExitedReviewMode(ev) => { + assert_eq!(ev.review_output, None); + ev.failure_message + .expect("expected explicit failure message") + } + other => panic!("expected ExitedReviewMode(..), got {other:?}"), + }; + assert_eq!( + failure_message, + "Review findings validation did not complete cleanly, so no findings were surfaced." + ); + let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + let rollout_path = codex.rollout_path().expect("rollout path"); + let rollout = std::fs::read_to_string(&rollout_path).expect("read rollout file"); + assert!( + rollout.contains("but was interrupted"), + "validation failure should persist as an interrupted review" + ); + assert!( + !rollout.contains("Initial review output before validation failure."), + "initial findings should not be persisted as a successful validated review" + ); + + codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: "back to parent".to_string(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + }) + .await + .unwrap(); + let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + let requests = request_log.requests(); + assert_eq!(requests.len(), 3); + let follow_up_input = requests[2].input(); + let follow_up_text = follow_up_input + .iter() + .filter_map(|msg| msg.get("content").and_then(|content| content.as_array())) + .flat_map(|content| content.iter()) + .filter_map(|entry| entry.get("text").and_then(|text| text.as_str())) + .collect::>() + .join("\n"); + assert!( + follow_up_text.contains("Review findings validation did not complete cleanly"), + "validation failure explanation missing from downstream parent context" + ); + assert!( + follow_up_text.contains("but was interrupted"), + "validation failure should be preserved as an interrupted review in downstream context" + ); + + let _codex_home_guard = codex_home; + server.verify().await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn review_validation_forwards_validator_error_diagnostics_before_exit() { + skip_if_no_network!(); + + let initial_review_json = serde_json::json!({ + "findings": [ + { + "title": "Needs validator diagnostics", + "body": "Validator-side errors should be surfaced before the generic failure.", + "confidence_score": 0.8, + "priority": 1, + "code_location": { + "absolute_file_path": "/tmp/validator-error.rs", + "line_range": {"start": 14, "end": 15} + } + } + ], + "overall_correctness": "incorrect", + "overall_explanation": "Initial review output before validator failure.", + "overall_confidence_score": 0.8 + }) + .to_string(); + + let server = start_mock_server().await; + let _request_log = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_assistant_message("msg-1", &initial_review_json), + ev_completed("resp-1"), + ]), + sse_failed("resp-2", "server_error", "simulated validator failure"), + ], + ) + .await; + + let codex_home = Arc::new(TempDir::new().unwrap()); + let codex = new_conversation_for_server(&server, codex_home.clone(), |config| { + config.model_provider.stream_max_retries = Some(0); + }) + .await; + + codex + .submit(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::Custom { + instructions: "Surface validator-side errors".to_string(), + }, + user_facing_hint: None, + validate_findings: true, + }, + }) + .await + .unwrap(); + + let _entered = wait_for_event(&codex, |ev| matches!(ev, EventMsg::EnteredReviewMode(_))).await; + let error_message = wait_for_event_match(&codex, |event| match event { + EventMsg::Error(err) => Some(err.message.clone()), + _ => None, + }) + .await; + assert!( + error_message.contains("simulated validator failure"), + "expected validator error to surface before exit, got {error_message}" + ); + + let closed = wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExitedReviewMode(_))).await; + let failure_message = match closed { + EventMsg::ExitedReviewMode(ev) => { + assert_eq!(ev.review_output, None); + ev.failure_message + .expect("expected explicit failure message") + } + other => panic!("expected ExitedReviewMode(..), got {other:?}"), + }; + assert_eq!( + failure_message, + "Review findings validation did not complete cleanly, so no findings were surfaced." + ); + let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + let _codex_home_guard = codex_home; + server.verify().await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn review_validation_rejects_findings_not_in_original_review() { + skip_if_no_network!(); + + let initial_review_json = serde_json::json!({ + "findings": [ + { + "title": "Original finding", + "body": "This finding is the only one the validator may keep.", + "confidence_score": 0.8, + "priority": 1, + "code_location": { + "absolute_file_path": "/tmp/original.rs", + "line_range": {"start": 3, "end": 4} + } + } + ], + "overall_correctness": "incorrect", + "overall_explanation": "Initial review output before subset enforcement.", + "overall_confidence_score": 0.8 + }) + .to_string(); + let validator_json = serde_json::json!({ + "findings": [ + { + "title": "Invented finding", + "body": "This finding was not in the original review.", + "confidence_score": 0.92, + "priority": 1, + "code_location": { + "absolute_file_path": "/tmp/invented.rs", + "line_range": {"start": 10, "end": 11} + } + } + ], + "overall_correctness": "patch is incorrect", + "overall_explanation": "The validator tried to add a new finding.", + "overall_confidence_score": 0.92 + }) + .to_string(); + + let server = start_mock_server().await; + let _request_log = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_assistant_message("msg-1", &initial_review_json), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-2", &validator_json), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let codex_home = Arc::new(TempDir::new().unwrap()); + let codex = new_conversation_for_server(&server, codex_home.clone(), |_| {}).await; + + codex + .submit(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::Custom { + instructions: "Reject invented validator findings".to_string(), + }, + user_facing_hint: None, + validate_findings: true, + }, + }) + .await + .unwrap(); + + let _entered = wait_for_event(&codex, |ev| matches!(ev, EventMsg::EnteredReviewMode(_))).await; + let closed = wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExitedReviewMode(_))).await; + let failure_message = match closed { + EventMsg::ExitedReviewMode(ev) => { + assert_eq!(ev.review_output, None); + ev.failure_message + .expect("expected explicit failure message") + } + other => panic!("expected ExitedReviewMode(..), got {other:?}"), + }; + assert_eq!( + failure_message, + "Review findings validation returned findings that were not present in the original review, so no findings were surfaced." + ); + let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + let rollout_path = codex.rollout_path().expect("rollout path"); + let rollout = std::fs::read_to_string(&rollout_path).expect("read rollout file"); + assert!( + rollout.contains("but was interrupted"), + "invented validator findings should persist as an interrupted review" + ); + assert!( + !rollout.contains("Invented finding"), + "invented validator findings should not be persisted as validated review output" + ); + + let _codex_home_guard = codex_home; + server.verify().await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn review_validation_treats_wrapped_json_output_as_failure() { + skip_if_no_network!(); + + let initial_review_json = serde_json::json!({ + "findings": [ + { + "title": "Needs strict validation", + "body": "Wrapped validator JSON should fail closed.", + "confidence_score": 0.8, + "priority": 1, + "code_location": { + "absolute_file_path": "/tmp/strict-validation.rs", + "line_range": {"start": 21, "end": 22} + } + } + ], + "overall_correctness": "incorrect", + "overall_explanation": "Initial review output before wrapped validator reply.", + "overall_confidence_score": 0.8 + }) + .to_string(); + let validator_reply = format!( + "Here is the validated output:\n{}", + serde_json::json!({ + "findings": [], + "overall_correctness": "patch is correct", + "overall_explanation": "This should be rejected because it is wrapped in prose.", + "overall_confidence_score": 0.95 + }) + ); + + let server = start_mock_server().await; + let _request_log = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_assistant_message("msg-1", &initial_review_json), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-2", &validator_reply), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let codex_home = Arc::new(TempDir::new().unwrap()); + let codex = new_conversation_for_server(&server, codex_home.clone(), |_| {}).await; + + codex + .submit(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::Custom { + instructions: "Reject wrapped validator output".to_string(), + }, + user_facing_hint: None, + validate_findings: true, + }, + }) + .await + .unwrap(); + + let _entered = wait_for_event(&codex, |ev| matches!(ev, EventMsg::EnteredReviewMode(_))).await; + let closed = wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExitedReviewMode(_))).await; + let failure_message = match closed { + EventMsg::ExitedReviewMode(ev) => { + assert_eq!(ev.review_output, None); + ev.failure_message + .expect("expected explicit failure message") + } + other => panic!("expected ExitedReviewMode(..), got {other:?}"), + }; + assert_eq!( + failure_message, + "Review findings validation did not complete cleanly, so no findings were surfaced." + ); + let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + let rollout_path = codex.rollout_path().expect("rollout path"); + let rollout = std::fs::read_to_string(&rollout_path).expect("read rollout file"); + assert!( + rollout.contains("but was interrupted"), + "wrapped validator output should persist as an interrupted review" + ); + assert!( + !rollout.contains("Here is the validated output:"), + "wrapped validator reply should not be persisted as validated review output" + ); + + let _codex_home_guard = codex_home; + server.verify().await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn review_validation_realigns_verdict_when_no_findings_remain() { + skip_if_no_network!(); + + let initial_review_json = serde_json::json!({ + "findings": [ + { + "title": "Discard me", + "body": "This finding should be dropped by validation.", + "confidence_score": 0.8, + "priority": 1, + "code_location": { + "absolute_file_path": "/tmp/discard-me.rs", + "line_range": {"start": 3, "end": 4} + } + } + ], + "overall_correctness": "incorrect", + "overall_explanation": "Initial review output before verdict realignment.", + "overall_confidence_score": 0.8 + }) + .to_string(); + let validator_json = serde_json::json!({ + "findings": [], + "overall_correctness": "patch is incorrect", + "overall_explanation": "Everything was discarded during validation.", + "overall_confidence_score": 0.84 + }) + .to_string(); + + let server = start_mock_server().await; + let _request_log = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_assistant_message("msg-1", &initial_review_json), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-2", &validator_json), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let codex_home = Arc::new(TempDir::new().unwrap()); + let codex = new_conversation_for_server(&server, codex_home.clone(), |_| {}).await; + + codex + .submit(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::Custom { + instructions: "Realign review verdict".to_string(), + }, + user_facing_hint: None, + validate_findings: true, + }, + }) + .await + .unwrap(); + + let _entered = wait_for_event(&codex, |ev| matches!(ev, EventMsg::EnteredReviewMode(_))).await; + let closed = wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExitedReviewMode(_))).await; + let review = match closed { + EventMsg::ExitedReviewMode(ev) => ev + .review_output + .expect("expected ExitedReviewMode with Some(review_output)"), + other => panic!("expected ExitedReviewMode(..), got {other:?}"), + }; + assert!(review.findings.is_empty()); + assert_eq!(review.overall_correctness, "patch is correct"); + assert_eq!( + review.overall_explanation, + "Everything was discarded during validation." + ); + let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + let _codex_home_guard = codex_home; + server.verify().await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn review_validation_normalizes_zero_finding_first_pass_output() { + skip_if_no_network!(); + + let review_json = serde_json::json!({ + "findings": [], + "overall_correctness": "patch is incorrect", + "overall_explanation": "", + "overall_confidence_score": 0.2 + }) + .to_string(); + + let sse_raw = r#"[ + {"type":"response.output_item.done", "item":{ + "type":"message", "role":"assistant", + "content":[{"type":"output_text","text":__REVIEW__}] + }}, + {"type":"response.completed", "response": {"id": "__ID__"}} + ]"# + .replace("__REVIEW__", &serde_json::to_string(&review_json).unwrap()); + let (server, _request_log) = start_responses_server_with_sse(&sse_raw, 1).await; + let codex_home = Arc::new(TempDir::new().unwrap()); + let codex = new_conversation_for_server(&server, codex_home.clone(), |_| {}).await; + + codex + .submit(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::Custom { + instructions: "Normalize zero-finding review output".to_string(), + }, + user_facing_hint: None, + validate_findings: true, + }, + }) + .await + .unwrap(); + + let _entered = wait_for_event(&codex, |ev| matches!(ev, EventMsg::EnteredReviewMode(_))).await; + let closed = wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExitedReviewMode(_))).await; + let review = match closed { + EventMsg::ExitedReviewMode(ev) => ev + .review_output + .expect("expected ExitedReviewMode with Some(review_output)"), + other => panic!("expected ExitedReviewMode(..), got {other:?}"), + }; + + assert_eq!( + review, + ReviewOutputEvent { + findings: Vec::new(), + overall_correctness: "patch is correct".to_string(), + overall_explanation: "No findings.".to_string(), + overall_confidence_score: 0.2, + } + ); + let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + let _codex_home_guard = codex_home; + server.verify().await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn review_validation_prompt_includes_original_review_scope() { + skip_if_no_network!(); + + let initial_review_json = serde_json::json!({ + "findings": [ + { + "title": "Needs scope", + "body": "The validator should receive the original review scope.\n```rust\nlet value = 1;\n```", + "confidence_score": 0.85, + "priority": 1, + "code_location": { + "absolute_file_path": "/tmp/original-scope.rs", + "line_range": {"start": 30, "end": 31} + } + } + ], + "overall_correctness": "incorrect", + "overall_explanation": "Initial review output before scope-aware validation.", + "overall_confidence_score": 0.85 + }) + .to_string(); + let validated_json = serde_json::json!({ + "findings": [], + "overall_correctness": "patch is correct", + "overall_explanation": "No findings remain after validating against the commit scope.", + "overall_confidence_score": 0.9 + }) + .to_string(); + + let server = start_mock_server().await; + let request_log = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_assistant_message("msg-1", &initial_review_json), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-2", &validated_json), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let codex_home = Arc::new(TempDir::new().unwrap()); + let codex = new_conversation_for_server(&server, codex_home.clone(), |_| {}).await; + + codex + .submit(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::Commit { + sha: "abc1234".to_string(), + title: Some("Add review loop".to_string()), + }, + user_facing_hint: None, + validate_findings: true, + }, + }) + .await + .unwrap(); + + let _entered = wait_for_event(&codex, |ev| matches!(ev, EventMsg::EnteredReviewMode(_))).await; + let _closed = wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExitedReviewMode(_))).await; + let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + let requests = request_log.requests(); + assert_eq!(requests.len(), 2); + let validator_request = requests[1].body_json(); + let validator_instructions = validator_request["instructions"] + .as_str() + .expect("validator instructions string"); + assert_ne!( + validator_instructions, REVIEW_PROMPT, + "validator should not run under the full review system prompt" + ); + assert!( + validator_instructions.contains("Do not perform a fresh review."), + "validator instructions should use the dedicated validation prompt" + ); + let validator_prompt = requests[1] + .input() + .iter() + .filter_map(|msg| msg.get("content").and_then(|content| content.as_array())) + .flat_map(|content| content.iter()) + .filter_map(|entry| entry.get("text").and_then(|text| text.as_str())) + .collect::>() + .join("\n"); + assert!( + validator_prompt + .contains("\"original_review_target_summary\": \"commit abc1234: Add review loop\""), + "validator prompt missing original review target summary" + ); + assert!( + validator_prompt.contains("\"original_review_scope\":"), + "validator prompt missing original review scope instructions" + ); + assert!( + validator_prompt + .contains("Review the code changes introduced by commit abc1234 (\\\"Add review loop\\\"). Provide prioritized, actionable findings."), + "validator prompt missing escaped original review scope text" + ); + assert!( + validator_prompt + .contains("Do not introduce any new finding that was not already present in the original review output."), + "validator prompt should forbid introducing brand-new findings" + ); + assert!( + validator_prompt + .contains("Each surviving finding must match one original finding exactly by `title`, `priority`, and `code_location`."), + "validator prompt should require stable finding identity" + ); + assert!( + validator_prompt.contains("```rust\\nlet value = 1;\\n```"), + "validator prompt should preserve review content containing code fences" + ); + assert!( + !validator_prompt.contains("```text"), + "validator prompt should not wrap review scope in hard-coded text fences" + ); + assert!( + !validator_prompt.contains("```json"), + "validator prompt should not wrap review output in hard-coded json fences" + ); + + let _codex_home_guard = codex_home; + server.verify().await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn review_validation_can_request_user_input_before_exiting_review_mode() { + skip_if_no_network!(); + + let call_id = "review-validation-question"; + let initial_review_json = serde_json::json!({ + "findings": [ + { + "title": "Needs clarification", + "body": "The validator should ask whether this finding is still relevant.", + "confidence_score": 0.7, + "priority": 1, + "code_location": { + "absolute_file_path": "/tmp/needs-clarification.rs", + "line_range": {"start": 7, "end": 8} + } + } + ], + "overall_correctness": "incorrect", + "overall_explanation": "Initial review output before user clarification.", + "overall_confidence_score": 0.7 + }) + .to_string(); + let request_args = serde_json::json!({ + "questions": [{ + "id": "finding_status", + "header": "Finding", + "question": "Does this finding still apply after the refactor?", + "options": [{ + "label": "No (Recommended)", + "description": "The finding is outdated and should be discarded." + }, { + "label": "Yes", + "description": "The finding still applies." + }] + }] + }) + .to_string(); + let validated_output = ReviewOutputEvent { + findings: Vec::new(), + overall_correctness: "patch is correct".to_string(), + overall_explanation: "The user confirmed the finding is outdated, so it was discarded." + .to_string(), + overall_confidence_score: 0.88, + }; + let validated_json = serde_json::to_string(&validated_output).expect("validated json"); + + let server = start_mock_server().await; + let request_log = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_assistant_message("msg-1", &initial_review_json), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_function_call(call_id, "request_user_input", &request_args), + ev_completed("resp-2"), + ]), + sse(vec![ + ev_response_created("resp-3"), + ev_assistant_message("msg-2", &validated_json), + ev_completed("resp-3"), + ]), + ], + ) + .await; + + let codex_home = Arc::new(TempDir::new().unwrap()); + let codex = new_conversation_for_server(&server, codex_home.clone(), |_| {}).await; + + codex + .submit(Op::OverrideTurnContext { + cwd: None, + approval_policy: None, + sandbox_policy: None, + windows_sandbox_level: None, + model: None, + effort: None, + summary: None, + service_tier: None, + collaboration_mode: Some(CollaborationMode { + mode: ModeKind::Execute, + settings: Settings { + model: "gpt-5.1".to_string(), + reasoning_effort: None, + developer_instructions: None, + }, + }), + personality: None, + }) + .await + .unwrap(); + + codex + .submit(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::Custom { + instructions: "Validate review findings with clarification".to_string(), + }, + user_facing_hint: None, + validate_findings: true, + }, + }) + .await + .unwrap(); + + let _entered = wait_for_event(&codex, |ev| matches!(ev, EventMsg::EnteredReviewMode(_))).await; + let request = wait_for_event(&codex, |ev| matches!(ev, EventMsg::RequestUserInput(_))).await; + let request = match request { + EventMsg::RequestUserInput(request) => request, + other => panic!("expected RequestUserInput(..), got {other:?}"), + }; + assert_eq!(request.call_id, call_id); + assert_eq!(request.questions.len(), 1); + + let mut answers = HashMap::new(); + answers.insert( + "finding_status".to_string(), + RequestUserInputAnswer { + answers: vec!["outdated".to_string()], + }, + ); + codex + .submit(Op::UserInputAnswer { + id: request.turn_id.clone(), + response: RequestUserInputResponse { answers }, + }) + .await + .unwrap(); + + let closed = wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExitedReviewMode(_))).await; + let review = match closed { + EventMsg::ExitedReviewMode(ev) => ev + .review_output + .expect("expected ExitedReviewMode with Some(review_output)"), + other => panic!("expected ExitedReviewMode(..), got {other:?}"), + }; + assert_eq!(validated_output, review); + let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + let requests = request_log.requests(); + assert_eq!(requests.len(), 3); + let output_text = requests[2] + .function_call_output_text(call_id) + .expect("request_user_input output should be included"); + let output_json: serde_json::Value = + serde_json::from_str(&output_text).expect("valid request_user_input output json"); + assert_eq!( + output_json, + serde_json::json!({ + "answers": { + "finding_status": { "answers": ["outdated"] } + } + }) + ); + + let _codex_home_guard = codex_home; + server.verify().await; +} + /// `/review` should use the session's current cwd (including runtime overrides) /// when resolving base-branch review prompts (merge-base computation). #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -844,6 +2348,7 @@ async fn review_uses_overridden_cwd_for_base_branch_merge_base() { branch: "main".to_string(), }, user_facing_hint: None, + validate_findings: false, }, }) .await diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index d0a6ac2c3e..03933e4337 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -1579,6 +1579,7 @@ fn build_review_request(args: &ReviewArgs) -> anyhow::Result { Ok(ReviewRequest { target, user_facing_hint: None, + validate_findings: false, }) } @@ -1637,6 +1638,7 @@ mod tests { let expected = ReviewRequest { target: ReviewTarget::UncommittedChanges, user_facing_hint: None, + validate_findings: false, }; assert_eq!(request, expected); @@ -1659,6 +1661,7 @@ mod tests { title: Some("Add review command".to_string()), }, user_facing_hint: None, + validate_findings: false, }; assert_eq!(request, expected); @@ -1680,6 +1683,7 @@ mod tests { instructions: "custom review instructions".to_string(), }, user_facing_hint: None, + validate_findings: false, }; assert_eq!(request, expected); diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 82e5084dba..7963668c64 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -1535,6 +1535,9 @@ impl HasLegacyEvent for EventMsg { #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] pub struct ExitedReviewModeEvent { pub review_output: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + #[ts(optional)] + pub failure_message: Option, } // Individual event payload types matching each `EventMsg` variant. @@ -2370,6 +2373,8 @@ pub struct ReviewRequest { #[serde(skip_serializing_if = "Option::is_none")] #[ts(optional)] pub user_facing_hint: Option, + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + pub validate_findings: 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 9b04f952d1..d0ddbacb47 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -165,6 +165,7 @@ use ratatui::style::Stylize; use ratatui::text::Line; use ratatui::widgets::Paragraph; use ratatui::widgets::Wrap; +use serde_json::Value; use tokio::sync::mpsc::UnboundedSender; use tokio::task::JoinHandle; use tracing::debug; @@ -183,7 +184,6 @@ const PLAN_MODE_REASONING_SCOPE_TITLE: &str = "Apply reasoning change"; const PLAN_MODE_REASONING_SCOPE_PLAN_ONLY: &str = "Apply to Plan mode override"; const PLAN_MODE_REASONING_SCOPE_ALL_MODES: &str = "Apply to global default and Plan mode override"; const CONNECTORS_SELECTION_VIEW_ID: &str = "connectors-selection"; - /// Choose the keybinding used to edit the most-recently queued message. /// /// Apple Terminal, Warp, and VSCode integrated terminals intercept or silently @@ -4400,6 +4400,7 @@ impl ChatWidget { instructions: prepared_args, }, user_facing_hint: None, + validate_findings: false, }, }); self.bottom_pane.drain_pending_submission_state(); @@ -4538,13 +4539,14 @@ impl ChatWidget { } fn submit_user_message(&mut self, user_message: UserMessage) { - self.submit_user_message_with_collaboration_mode(user_message, None) + self.submit_user_message_with_collaboration_mode(user_message, None, None) } fn submit_user_message_with_collaboration_mode( &mut self, user_message: UserMessage, collaboration_mode_override: Option, + final_output_json_schema: Option, ) { if !self.is_session_configured() { tracing::warn!("cannot submit user message before session is configured; queueing"); @@ -4762,7 +4764,7 @@ impl ChatWidget { effort: effective_mode.reasoning_effort(), summary: None, service_tier, - final_output_json_schema: None, + final_output_json_schema, collaboration_mode, personality, }; @@ -5211,17 +5213,21 @@ impl ChatWidget { } } if self.review_loop_state.is_some() { - if output.findings.is_empty() - && output.overall_correctness.trim().is_empty() - && !output.overall_explanation.trim().is_empty() - { - self.clear_review_loop_state(); - self.add_error_message( - "Review loop stopped because the review turn did not produce structured output." - .to_string(), - ); - } else if output.findings.is_empty() { - self.clear_review_loop_state(); + if output.findings.is_empty() { + if self.review_loop_state.as_ref().is_some_and(|state| { + state.fix_attempts_started == 0 + && matches!(state.spec.target, ReviewTarget::Commit { .. }) + && output.overall_correctness.trim().is_empty() + && output.overall_confidence_score == 0.0 + }) { + self.clear_review_loop_state(); + self.add_error_message( + "Review loop stopped because reviewer did not return valid structured output." + .to_string(), + ); + } else { + self.clear_review_loop_state(); + } } else { let stop_message = if let Some(state) = self.review_loop_state.as_mut() { if let Some(max_fix_attempts) = state.spec.max_fix_attempts @@ -5246,6 +5252,24 @@ impl ChatWidget { } } // Final message is rendered as part of the AgentMessage. + } else if let Some(failure_message) = review.failure_message { + self.flush_answer_stream_with_separator(); + self.flush_interrupt_queue(); + self.flush_active_cell(); + + let failure_message = failure_message.trim().to_string(); + let failure_message = if failure_message.is_empty() { + "Review turn did not finish cleanly.".to_string() + } else { + failure_message + }; + if self.review_loop_state.is_some() { + self.clear_review_loop_state(); + } else { + tracing::debug!( + "review failed cleanly; relying on final assistant message: {failure_message}" + ); + } } else if self.review_loop_state.is_some() { self.clear_review_loop_state(); self.add_error_message( @@ -5443,6 +5467,7 @@ impl ChatWidget { self.submit_user_message_with_collaboration_mode( prompt.into(), default_collaboration_mode, + None, ); true } @@ -8630,6 +8655,7 @@ impl ChatWidget { review_request: ReviewRequest { target: ReviewTarget::UncommittedChanges, user_facing_hint: None, + validate_findings: false, }, })); } @@ -8721,6 +8747,7 @@ impl ChatWidget { branch: branch.clone(), }, user_facing_hint: None, + validate_findings: false, }, })); } @@ -8773,6 +8800,7 @@ impl ChatWidget { title: Some(subject.clone()), }, user_facing_hint: None, + validate_findings: false, }, })); } @@ -8824,6 +8852,7 @@ impl ChatWidget { instructions: trimmed, }, user_facing_hint: None, + validate_findings: false, }, })); } @@ -9096,6 +9125,7 @@ impl ChatWidget { ReviewRequest { target: state.spec.target.clone(), user_facing_hint: None, + validate_findings: true, } } ReviewTarget::Commit { .. } @@ -9104,11 +9134,13 @@ impl ChatWidget { ReviewRequest { target: state.spec.target.clone(), user_facing_hint: None, + validate_findings: false, } } ReviewTarget::Custom { .. } => ReviewRequest { target: state.spec.target.clone(), user_facing_hint: None, + validate_findings: true, }, ReviewTarget::Commit { sha, title } if is_commit_follow_up => { let prompt = codex_core::review_prompts::commit_follow_up_review_prompt( @@ -9124,6 +9156,7 @@ impl ChatWidget { "stack since {}", codex_core::review_prompts::user_facing_hint(&state.spec.target) )), + validate_findings: true, } } _ => { @@ -9141,6 +9174,7 @@ impl ChatWidget { user_facing_hint: Some(codex_core::review_prompts::user_facing_hint( &state.spec.target, )), + validate_findings: true, } } }; @@ -9496,6 +9530,7 @@ pub(crate) fn show_review_commit_picker_with_entries( title: Some(subject.clone()), }, user_facing_hint: None, + validate_findings: false, }, })); })], diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_loop_validation_failure.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_loop_validation_failure.snap new file mode 100644 index 0000000000..0bef16c68a --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_loop_validation_failure.snap @@ -0,0 +1,9 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: rendered +--- +<< Code review finished >> + + +• Review findings validation did not complete cleanly, so no findings were + surfaced. diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index cdb96fe1e1..5bc9c4e429 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -1475,6 +1475,7 @@ async fn entered_review_mode_uses_request_hint() { branch: "feature".to_string(), }, user_facing_hint: Some("feature branch".to_string()), + validate_findings: false, }), }); @@ -1494,6 +1495,7 @@ async fn entered_review_mode_defaults_to_current_changes_banner() { msg: EventMsg::EnteredReviewMode(ReviewRequest { target: ReviewTarget::UncommittedChanges, user_facing_hint: None, + validate_findings: false, }), }); @@ -1512,6 +1514,7 @@ async fn live_agent_message_renders_during_review_mode() { msg: EventMsg::EnteredReviewMode(ReviewRequest { target: ReviewTarget::UncommittedChanges, user_facing_hint: None, + validate_findings: false, }), }); let _ = drain_insert_history(&mut rx); @@ -1538,6 +1541,7 @@ async fn thread_snapshot_replay_preserves_agent_message_during_review_mode() { msg: EventMsg::EnteredReviewMode(ReviewRequest { target: ReviewTarget::UncommittedChanges, user_facing_hint: None, + validate_findings: false, }), }); let _ = drain_insert_history(&mut rx); @@ -1580,6 +1584,7 @@ async fn review_restores_context_window_indicator() { branch: "feature".to_string(), }, user_facing_hint: Some("feature branch".to_string()), + validate_findings: false, }), }); @@ -1596,6 +1601,7 @@ async fn review_restores_context_window_indicator() { id: "review-end".into(), msg: EventMsg::ExitedReviewMode(ExitedReviewModeEvent { review_output: None, + failure_message: None, }), }); let _ = drain_insert_history(&mut rx); @@ -6309,6 +6315,7 @@ async fn custom_prompt_submit_sends_review_op() { instructions: "please audit dependencies".to_string(), }, user_facing_hint: None, + validate_findings: false, } ); } @@ -6692,11 +6699,13 @@ async fn review_loop_stops_immediately_when_first_review_has_no_findings() { ReviewRequest { target: ReviewTarget::UncommittedChanges, user_facing_hint: None, + validate_findings: true, } ); chat.on_exited_review_mode(ExitedReviewModeEvent { review_output: Some(no_findings_review_output()), + failure_message: None, }); chat.on_task_complete(None, false); @@ -6704,6 +6713,34 @@ async fn review_loop_stops_immediately_when_first_review_has_no_findings() { assert_no_review_or_submit_op(&mut op_rx); } +#[tokio::test] +async fn review_loop_validation_failure_stops_with_error() { + let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(None).await; + chat.thread_id = Some(ThreadId::new()); + let failure_message = + "Review findings validation did not complete cleanly, so no findings were surfaced."; + + chat.on_review_loop_target_selected(ReviewTarget::UncommittedChanges, None); + chat.start_review_loop(None); + let _ = next_review_request(&mut op_rx); + + chat.on_exited_review_mode(ExitedReviewModeEvent { + review_output: None, + failure_message: Some(failure_message.to_string()), + }); + complete_assistant_message(&mut chat, "review-failure", failure_message, None); + chat.on_task_complete(None, false); + + assert!(chat.review_loop_state.is_none()); + assert_no_review_or_submit_op(&mut op_rx); + let rendered = drain_insert_history(&mut rx) + .iter() + .map(|lines| lines_to_single_string(lines)) + .collect::>() + .join("\n"); + assert_snapshot!("review_loop_validation_failure", rendered); +} + #[tokio::test] async fn review_loop_findings_trigger_fix_turn_with_required_prompt() { let (mut chat, _rx, mut op_rx) = make_chatwidget_manual(None).await; @@ -6716,6 +6753,7 @@ async fn review_loop_findings_trigger_fix_turn_with_required_prompt() { chat.on_exited_review_mode(ExitedReviewModeEvent { review_output: Some(review_output.clone()), + failure_message: None, }); chat.on_task_complete(None, false); @@ -6751,6 +6789,7 @@ async fn review_loop_findings_trigger_fix_turn_with_default_mode_in_plan() { chat.on_exited_review_mode(ExitedReviewModeEvent { review_output: Some(review_output), + failure_message: None, }); chat.on_task_complete(None, false); @@ -6792,6 +6831,7 @@ async fn review_loop_commit_targets_append_commit_requirement_to_fix_prompt() { chat.on_exited_review_mode(ExitedReviewModeEvent { review_output: Some(review_output), + failure_message: None, }); chat.on_task_complete(None, false); @@ -6813,6 +6853,85 @@ async fn review_loop_commit_targets_append_commit_requirement_to_fix_prompt() { ); } +#[tokio::test] +async fn review_loop_initial_commit_review_skips_findings_validation() { + let (mut chat, _rx, mut op_rx) = make_chatwidget_manual(None).await; + chat.thread_id = Some(ThreadId::new()); + + chat.on_review_loop_target_selected( + ReviewTarget::Commit { + sha: "abc1234".to_string(), + title: Some("Add review loop".to_string()), + }, + None, + ); + chat.start_review_loop(None); + let generation = chat + .review_loop_state + .as_ref() + .expect("loop state") + .generation; + chat.on_review_loop_initial_head_resolved(generation, Some("head-1".to_string())); + + assert_eq!( + next_review_request(&mut op_rx), + ReviewRequest { + target: ReviewTarget::Commit { + sha: "abc1234".to_string(), + title: Some("Add review loop".to_string()), + }, + user_facing_hint: None, + validate_findings: false, + } + ); +} + +#[tokio::test] +async fn review_loop_initial_commit_review_fallback_stops_with_error() { + let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(None).await; + chat.thread_id = Some(ThreadId::new()); + + chat.on_review_loop_target_selected( + ReviewTarget::Commit { + sha: "abc1234".to_string(), + title: Some("Add review loop".to_string()), + }, + None, + ); + chat.start_review_loop(None); + let generation = chat + .review_loop_state + .as_ref() + .expect("loop state") + .generation; + chat.on_review_loop_initial_head_resolved(generation, Some("head-1".to_string())); + let _ = next_review_request(&mut op_rx); + + chat.on_exited_review_mode(ExitedReviewModeEvent { + review_output: Some(ReviewOutputEvent { + overall_explanation: "plain text review output".to_string(), + ..Default::default() + }), + failure_message: None, + }); + chat.on_task_complete(None, false); + + assert!(chat.review_loop_state.is_none()); + assert_no_review_or_submit_op(&mut op_rx); + + let rendered = drain_insert_history(&mut rx) + .iter() + .map(|lines| lines_to_single_string(lines)) + .collect::>() + .join("\n"); + assert!( + rendered.contains( + "Review loop stopped because reviewer did not return valid structured output." + ), + "expected malformed commit review to stop the loop with an error: {rendered}" + ); +} + #[tokio::test] async fn review_loop_commit_follow_up_review_uses_cumulative_stack_prompt() { let (mut chat, _rx, mut op_rx) = make_chatwidget_manual(None).await; @@ -6837,6 +6956,7 @@ async fn review_loop_commit_follow_up_review_uses_cumulative_stack_prompt() { chat.on_exited_review_mode(ExitedReviewModeEvent { review_output: Some(review_output), + failure_message: None, }); chat.on_task_complete(None, false); let _ = next_submit_op(&mut op_rx); @@ -6860,6 +6980,7 @@ async fn review_loop_commit_follow_up_review_uses_cumulative_stack_prompt() { instructions: expected_prompt, }, user_facing_hint: Some("stack since commit abc1234: Add review loop".to_string()), + validate_findings: true, } ); } @@ -6876,6 +6997,7 @@ async fn review_loop_stops_when_fix_attempt_cap_is_reached() { chat.on_exited_review_mode(ExitedReviewModeEvent { review_output: Some(review_output.clone()), + failure_message: None, }); chat.on_task_complete(None, false); let _ = next_submit_op(&mut op_rx); @@ -6885,6 +7007,7 @@ async fn review_loop_stops_when_fix_attempt_cap_is_reached() { chat.on_exited_review_mode(ExitedReviewModeEvent { review_output: Some(review_output), + failure_message: None, }); chat.on_task_complete(None, false); @@ -6913,6 +7036,7 @@ async fn review_loop_queued_messages_wait_until_loop_completes() { chat.on_exited_review_mode(ExitedReviewModeEvent { review_output: Some(review_output), + failure_message: None, }); chat.on_task_complete(None, false); let fix_items = match next_submit_op(&mut op_rx) { @@ -6934,6 +7058,7 @@ async fn review_loop_queued_messages_wait_until_loop_completes() { chat.on_exited_review_mode(ExitedReviewModeEvent { review_output: Some(no_findings_review_output()), + failure_message: None, }); chat.on_task_complete(None, false); @@ -6974,6 +7099,7 @@ async fn review_loop_commit_validation_failure_stops_with_error() { chat.on_exited_review_mode(ExitedReviewModeEvent { review_output: Some(review_output), + failure_message: None, }); chat.on_task_complete(None, false); let _ = next_submit_op(&mut op_rx); @@ -7017,6 +7143,7 @@ async fn review_loop_commit_validation_stops_when_fix_is_dirty_with_new_commit() chat.on_exited_review_mode(ExitedReviewModeEvent { review_output: Some(review_output), + failure_message: None, }); chat.on_task_complete(None, false); let _ = next_submit_op(&mut op_rx); @@ -7049,6 +7176,7 @@ async fn review_loop_state_survives_stream_error_during_fix_turn() { chat.on_exited_review_mode(ExitedReviewModeEvent { review_output: Some(review_output), + failure_message: None, }); chat.on_task_complete(None, false); let _ = next_submit_op(&mut op_rx); @@ -7095,6 +7223,7 @@ async fn review_loop_ignores_stale_commit_validation_callbacks() { chat.on_exited_review_mode(ExitedReviewModeEvent { review_output: Some(review_output), + failure_message: None, }); chat.on_task_complete(None, false); let _ = next_submit_op(&mut op_rx); @@ -10563,6 +10692,7 @@ async fn enter_queues_user_messages_while_review_is_running() { msg: EventMsg::EnteredReviewMode(ReviewRequest { target: ReviewTarget::UncommittedChanges, user_facing_hint: Some("current changes".to_string()), + validate_findings: false, }), }); let _ = drain_insert_history(&mut rx); @@ -10594,6 +10724,7 @@ async fn review_queues_user_messages_snapshot() { msg: EventMsg::EnteredReviewMode(ReviewRequest { target: ReviewTarget::UncommittedChanges, user_facing_hint: Some("current changes".to_string()), + validate_findings: false, }), }); let _ = drain_insert_history(&mut rx); diff --git a/codex-rs/utils/git/src/branch.rs b/codex-rs/utils/git/src/branch.rs index f65de0c6bf..eeda39dd53 100644 --- a/codex-rs/utils/git/src/branch.rs +++ b/codex-rs/utils/git/src/branch.rs @@ -7,8 +7,8 @@ use crate::operations::resolve_head; use crate::operations::resolve_repository_root; use crate::operations::run_git_for_stdout; -/// Returns the merge-base commit between `HEAD` and the latest version between local -/// and remote of the provided branch, if both exist. +/// Returns the merge-base commit between `HEAD` and the provided branch using +/// only refs already available in the local repository. /// /// The function mirrors `git merge-base HEAD ` but returns `Ok(None)` when /// the repository has no `HEAD` yet or when the branch cannot be resolved. @@ -69,25 +69,8 @@ fn resolve_upstream_if_remote_ahead( repo_root: &Path, branch: &str, ) -> Result, GitToolingError> { - let upstream = match run_git_for_stdout( - repo_root, - vec![ - OsString::from("rev-parse"), - OsString::from("--abbrev-ref"), - OsString::from("--symbolic-full-name"), - OsString::from(format!("{branch}@{{upstream}}")), - ], - None, - ) { - Ok(name) => { - let trimmed = name.trim(); - if trimmed.is_empty() { - return Ok(None); - } - trimmed.to_string() - } - Err(GitToolingError::GitCommand { .. }) => return Ok(None), - Err(other) => return Err(other), + let Some(upstream) = resolve_upstream_branch(repo_root, branch)? else { + return Ok(None); }; let counts = match run_git_for_stdout( @@ -116,6 +99,39 @@ fn resolve_upstream_if_remote_ahead( } } +pub fn branch_upstream(repo_path: &Path, branch: &str) -> Result, GitToolingError> { + ensure_git_repository(repo_path)?; + let repo_root = resolve_repository_root(repo_path)?; + resolve_upstream_branch(repo_root.as_path(), branch) +} + +fn resolve_upstream_branch( + repo_root: &Path, + branch: &str, +) -> Result, GitToolingError> { + match run_git_for_stdout( + repo_root, + vec![ + OsString::from("rev-parse"), + OsString::from("--abbrev-ref"), + OsString::from("--symbolic-full-name"), + OsString::from(format!("{branch}@{{upstream}}")), + ], + None, + ) { + Ok(name) => { + let trimmed = name.trim(); + if trimmed.is_empty() { + Ok(None) + } else { + Ok(Some(trimmed.to_string())) + } + } + Err(GitToolingError::GitCommand { .. }) => Ok(None), + Err(other) => Err(other), + } +} + #[cfg(test)] mod tests { use super::merge_base_with_head; diff --git a/codex-rs/utils/git/src/lib.rs b/codex-rs/utils/git/src/lib.rs index c4db10fc69..743b8f742b 100644 --- a/codex-rs/utils/git/src/lib.rs +++ b/codex-rs/utils/git/src/lib.rs @@ -14,6 +14,7 @@ pub use apply::apply_git_patch; pub use apply::extract_paths_from_patch; pub use apply::parse_git_apply_output; pub use apply::stage_paths; +pub use branch::branch_upstream; pub use branch::merge_base_with_head; pub use errors::GitToolingError; pub use ghost_commits::CreateGhostCommitOptions;