openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-13 21:30:47 +00:00
Code Issues Packages Projects Releases Wiki Activity

[codex-rs] harden review-loop validation flow [ci changed_files]

Browse Source 
Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
Cooper Gamble
2026-03-09 17:27:13 -07:00
co-authored by Codex
parent 27003226dd
commit d4eedd7662
24 changed files with 3049 additions and 120 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/app-server-protocol/schema/json/EventMsg.json
+18
View File
@@ -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": [
{
codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json
+9
View File
@@ -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": [
{
codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json
+9
View File
@@ -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": [
{
codex-rs/app-server-protocol/schema/typescript/ExitedReviewModeEvent.ts
+1 -1
View File
@@ -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, };
codex-rs/app-server-protocol/schema/typescript/ReviewRequest.ts
+1 -1
View File
@@ -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, };
codex-rs/app-server-protocol/src/protocol/thread_history.rs
+43 -5
View File
@@ -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<std::path::PathBuf, codex_protocol::protocol::FileChange>,
) -> Vec<FileUpdateChange> {
@@ -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::<Vec<_>>();
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![
codex-rs/app-server/src/bespoke_event_handling.rs
+17 -4
View File
@@ -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<PatchApplyStatus>) {
codex-rs/app-server/src/codex_message_processor.rs
+1
View File
@@ -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))
codex-rs/core/src/codex.rs
+10 -1
View File
@@ -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<TurnContext>,
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;
codex-rs/core/src/codex_delegate.rs
+2 -2
View File
@@ -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,
codex-rs/core/src/codex_tests.rs
+22 -2
View File
@@ -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) => {
codex-rs/core/src/review_prompts.rs
+73 -1
View File
@@ -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<String
match target {
ReviewTarget::UncommittedChanges => 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<ResolvedReviewRequest> for ReviewRequest {
ReviewRequest {
target: resolved.target,
user_facing_hint: Some(resolved.user_facing_hint),
validate_findings: false,
}
}
}
@@ -138,6 +145,32 @@ impl From<ResolvedReviewRequest> 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."
);
}
}
codex-rs/core/src/tasks/mod.rs
+1
View File
@@ -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;
codex-rs/core/src/tasks/review.rs
+1039 -62
View File
File diff suppressed because it is too large Load Diff
codex-rs/core/src/tools/spec.rs
+54 -1
View File
@@ -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();
codex-rs/core/tests/suite/codex_delegate.rs
+3
View File
@@ -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
codex-rs/core/tests/suite/review.rs
+1510 -5
View File
File diff suppressed because it is too large Load Diff
codex-rs/exec/src/lib.rs
+4
View File
@@ -1579,6 +1579,7 @@ fn build_review_request(args: &ReviewArgs) -> anyhow::Result<ReviewRequest> {
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);
codex-rs/protocol/src/protocol.rs
+5
View File
@@ -1535,6 +1535,9 @@ impl HasLegacyEvent for EventMsg {
#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)]
pub struct ExitedReviewModeEvent {
pub review_output: Option<ReviewOutputEvent>,
#[serde(default, skip_serializing_if = "Option::is_none")]
#[ts(optional)]
pub failure_message: Option<String>,
}
// 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<String>,
#[serde(default, skip_serializing_if = "std::ops::Not::not")]
pub validate_findings: bool,
}
/// Structured review result produced by a child review session.
codex-rs/tui/src/chatwidget.rs
+49 -14
View File
@@ -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<CollaborationMode>,
final_output_json_schema: Option<Value>,
) {
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,
},
}));
})],
codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__review_loop_validation_failure.snap
+9
View File
@@ -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.
codex-rs/tui/src/chatwidget/tests.rs
+131
View File
@@ -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::<Vec<_>>()
.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::<Vec<_>>()
.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);
codex-rs/utils/git/src/branch.rs
+37 -21
View File
@@ -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 <branch>` 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<Option<String>, 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<Option<String>, 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<Option<String>, 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;
codex-rs/utils/git/src/lib.rs
+1
View File
@@ -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;