openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-18 17:18:52 +00:00
Code Issues Packages Projects Releases Wiki Activity

feat(core): emit canonical review mode items (#31473)

Browse Source 
## Description

This PR moves review-mode markers onto canonical `TurnItem` lifecycle:

- `TurnItem::EnteredReviewMode`
- `TurnItem::ExitedReviewMode`

Core now emits `ItemStarted` / `ItemCompleted` for both. The completed
items map back into the existing `EnteredReviewMode` /
`ExitedReviewMode` events, so raw core event consumers and legacy
rollout persistence keep seeing the old events.

This is the compatibility layer needed before paginated rollouts persist
review markers as `ItemCompleted(TurnItem)`.

## Why

Review markers were one of the remaining app-server thread items created
directly from legacy events. Giving them canonical items lets paginated
history persist stable turn/item IDs without changing legacy rollouts.

## What changed

- Added canonical review-mode `TurnItem`s and switched review flow to
emit their lifecycle.
- Added completed-item → legacy review event mappings with stable
turn/item IDs.
- Switched app-server live notifications to the generic canonical item
path and kept legacy replay compatible with old payloads.
- Updated `ThreadHistoryBuilder` to replay canonical review items even
though review turns still do not emit `TurnStarted`.
This commit is contained in:
Owen Lin
2026-07-08 09:59:50 -07:00
committed by GitHub
parent a219b6fdb4
commit 23aac925e7
17 changed files with 458 additions and 153 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/app-server-protocol/src/protocol/item_builders.rs
+9
View File
@@ -33,6 +33,9 @@ use codex_protocol::protocol::GuardianAssessmentAction;
use codex_protocol::protocol::GuardianAssessmentEvent;
use codex_protocol::protocol::PatchApplyBeginEvent;
use codex_protocol::protocol::PatchApplyEndEvent;
use codex_protocol::protocol::ReviewOutputEvent;
use codex_protocol::review_format::REVIEW_FALLBACK_MESSAGE;
use codex_protocol::review_format::render_review_output_text;
use codex_shell_command::parse_command::parse_command;
use codex_shell_command::parse_command::shlex_join;
use codex_utils_path_uri::PathConvention;
@@ -41,6 +44,12 @@ use std::collections::HashMap;
use std::path::PathBuf;
use tracing::warn;
pub(crate) fn review_output_text(output: Option<&ReviewOutputEvent>) -> String {
output
.map(render_review_output_text)
.unwrap_or_else(|| REVIEW_FALLBACK_MESSAGE.to_string())
}
pub fn build_file_change_approval_request_item(
payload: &ApplyPatchApprovalRequestEvent,
) -> ThreadItem {
codex-rs/app-server-protocol/src/protocol/thread_history.rs
+165 -27
View File
@@ -4,6 +4,7 @@ use crate::protocol::item_builders::build_file_change_approval_request_item;
use crate::protocol::item_builders::build_file_change_begin_item;
use crate::protocol::item_builders::build_file_change_end_item;
use crate::protocol::item_builders::build_item_from_guardian_event;
use crate::protocol::item_builders::review_output_text;
use crate::protocol::v2::CollabAgentState;
use crate::protocol::v2::CollabAgentTool;
use crate::protocol::v2::CollabAgentToolCallStatus;
@@ -49,7 +50,6 @@ use codex_protocol::protocol::McpToolCallBeginEvent;
use codex_protocol::protocol::McpToolCallEndEvent;
use codex_protocol::protocol::PatchApplyBeginEvent;
use codex_protocol::protocol::PatchApplyEndEvent;
use codex_protocol::protocol::ReviewOutputEvent;
use codex_protocol::protocol::RolloutItem;
use codex_protocol::protocol::ThreadRolledBackEvent;
use codex_protocol::protocol::TurnAbortedEvent;
@@ -59,6 +59,8 @@ use codex_protocol::protocol::UserMessageEvent;
use codex_protocol::protocol::ViewImageToolCallEvent;
use codex_protocol::protocol::WebSearchBeginEvent;
use codex_protocol::protocol::WebSearchEndEvent;
#[cfg(test)]
use codex_protocol::review_format::REVIEW_FALLBACK_MESSAGE;
use std::collections::HashMap;
use tracing::warn;
use uuid::Uuid;
@@ -590,6 +592,11 @@ impl ThreadHistoryBuilder {
turn_id: &str,
item: &codex_protocol::items::TurnItem,
) {
let is_review_mode_item = matches!(
item,
codex_protocol::items::TurnItem::EnteredReviewMode(_)
| codex_protocol::items::TurnItem::ExitedReviewMode(_)
);
let should_upsert = match item {
codex_protocol::items::TurnItem::Plan(plan) => !plan.text.is_empty(),
codex_protocol::items::TurnItem::Sleep(_)
@@ -597,7 +604,9 @@ impl ThreadHistoryBuilder {
| codex_protocol::items::TurnItem::DynamicToolCall(_)
| codex_protocol::items::TurnItem::CollabAgentToolCall(_)
| codex_protocol::items::TurnItem::SubAgentActivity(_)
| codex_protocol::items::TurnItem::Extension(_) => true,
| codex_protocol::items::TurnItem::Extension(_)
| codex_protocol::items::TurnItem::EnteredReviewMode(_)
| codex_protocol::items::TurnItem::ExitedReviewMode(_) => true,
codex_protocol::items::TurnItem::UserMessage(_)
| codex_protocol::items::TurnItem::HookPrompt(_)
| codex_protocol::items::TurnItem::AgentMessage(_)
@@ -611,7 +620,12 @@ impl ThreadHistoryBuilder {
};
if should_upsert {
self.upsert_item_in_turn_id(turn_id, ThreadItem::from(item.clone()));
let item = ThreadItem::from(item.clone());
if is_review_mode_item {
self.upsert_review_mode_item(Some(turn_id), item);
} else {
self.upsert_item_in_turn_id(turn_id, item);
}
}
}
@@ -1108,26 +1122,55 @@ impl ThreadHistoryBuilder {
self.push_item_in_current_turn(ThreadItem::ContextCompaction { id });
}
fn handle_entered_review_mode(&mut self, payload: &codex_protocol::protocol::ReviewRequest) {
fn handle_entered_review_mode(
&mut self,
payload: &codex_protocol::protocol::EnteredReviewModeEvent,
) {
let review = payload
.user_facing_hint
.clone()
.unwrap_or_else(|| "Review requested.".to_string());
let id = self.next_item_id();
self.push_item_in_current_turn(ThreadItem::EnteredReviewMode { id, review });
let id = payload
.item_id
.clone()
.unwrap_or_else(|| self.next_item_id());
self.upsert_review_mode_item(
payload.turn_id.as_deref(),
ThreadItem::EnteredReviewMode { id, review },
);
}
fn handle_exited_review_mode(
&mut self,
payload: &codex_protocol::protocol::ExitedReviewModeEvent,
) {
let review = payload
.review_output
let review = review_output_text(payload.review_output.as_ref());
let id = payload
.item_id
.clone()
.unwrap_or_else(|| self.next_item_id());
self.upsert_review_mode_item(
payload.turn_id.as_deref(),
ThreadItem::ExitedReviewMode { id, review },
);
}
fn upsert_review_mode_item(&mut self, turn_id: Option<&str>, item: ThreadItem) {
let Some(turn_id) = turn_id else {
self.upsert_item_in_current_turn(item);
return;
};
let current_turn_matches = self
.current_turn
.as_ref()
.map(render_review_output_text)
.unwrap_or_else(|| REVIEW_FALLBACK_MESSAGE.to_string());
let id = self.next_item_id();
self.push_item_in_current_turn(ThreadItem::ExitedReviewMode { id, review });
.is_some_and(|turn| turn.id == turn_id);
if !current_turn_matches && !self.turns.iter().any(|turn| turn.id == turn_id) {
self.finish_current_turn();
let turn = self.new_turn(Some(turn_id.to_string()));
self.record_changed_pending_turn(&turn);
self.current_turn = Some(turn);
}
self.upsert_item_in_turn_id(turn_id, item);
}
fn handle_error(&mut self, payload: &ErrorEvent) {
@@ -1443,17 +1486,6 @@ impl ThreadHistoryBuilder {
}
}
const REVIEW_FALLBACK_MESSAGE: &str = "Reviewer failed to output a response.";
fn render_review_output_text(output: &ReviewOutputEvent) -> String {
let explanation = output.overall_explanation.trim();
if explanation.is_empty() {
REVIEW_FALLBACK_MESSAGE.to_string()
} else {
explanation.to_string()
}
}
fn convert_dynamic_tool_content_items(
items: &[codex_protocol::dynamic_tools::DynamicToolCallOutputContentItem],
) -> Vec<DynamicToolCallOutputContentItem> {
@@ -1558,6 +1590,8 @@ mod tests {
use codex_protocol::dynamic_tools::DynamicToolCallOutputContentItem as CoreDynamicToolCallOutputContentItem;
use codex_protocol::items::CommandExecutionItem as CoreCommandExecutionItem;
use codex_protocol::items::CommandExecutionStatus as CoreCommandExecutionStatus;
use codex_protocol::items::EnteredReviewModeItem as CoreEnteredReviewModeItem;
use codex_protocol::items::ExitedReviewModeItem as CoreExitedReviewModeItem;
use codex_protocol::items::HookPromptFragment as CoreHookPromptFragment;
use codex_protocol::items::SleepItem as CoreSleepItem;
use codex_protocol::items::TurnItem as CoreTurnItem;
@@ -1575,12 +1609,15 @@ mod tests {
use codex_protocol::protocol::CodexErrorInfo;
use codex_protocol::protocol::CompactedItem;
use codex_protocol::protocol::DynamicToolCallResponseEvent;
use codex_protocol::protocol::EnteredReviewModeEvent;
use codex_protocol::protocol::ExecCommandEndEvent;
use codex_protocol::protocol::ExecCommandSource;
use codex_protocol::protocol::ExitedReviewModeEvent;
use codex_protocol::protocol::ItemStartedEvent;
use codex_protocol::protocol::McpInvocation;
use codex_protocol::protocol::McpToolCallEndEvent;
use codex_protocol::protocol::PatchApplyBeginEvent;
use codex_protocol::protocol::ReviewTarget;
use codex_protocol::protocol::ThreadRolledBackEvent;
use codex_protocol::protocol::TurnAbortReason;
use codex_protocol::protocol::TurnAbortedEvent;
@@ -1705,6 +1742,111 @@ mod tests {
);
}
#[test]
fn review_mode_events_replay_persisted_ids() {
let events = vec![
EventMsg::EnteredReviewMode(EnteredReviewModeEvent {
target: ReviewTarget::Custom {
instructions: "review this".into(),
},
user_facing_hint: Some("Review requested.".into()),
turn_id: Some("turn-1".into()),
item_id: Some("entered-review".into()),
}),
EventMsg::ExitedReviewMode(ExitedReviewModeEvent {
turn_id: Some("turn-1".into()),
item_id: Some("exited-review".into()),
review_output: None,
}),
EventMsg::TurnComplete(TurnCompleteEvent {
turn_id: "turn-1".into(),
last_agent_message: None,
completed_at: None,
duration_ms: None,
time_to_first_token_ms: None,
}),
];
let mut builder = ThreadHistoryBuilder::new();
for event in &events {
builder.handle_event(event);
}
let turns = builder.finish();
assert_eq!(turns[0].id, "turn-1");
assert_eq!(
turns[0].items,
vec![
ThreadItem::EnteredReviewMode {
id: "entered-review".into(),
review: "Review requested.".into(),
},
ThreadItem::ExitedReviewMode {
id: "exited-review".into(),
review: REVIEW_FALLBACK_MESSAGE.into(),
},
]
);
}
#[test]
fn review_mode_items_replay_without_turn_started() {
let thread_id = ThreadId::new();
let entered = CoreTurnItem::EnteredReviewMode(CoreEnteredReviewModeItem {
id: "entered-review".into(),
target: ReviewTarget::Custom {
instructions: "review this".into(),
},
user_facing_hint: "Review requested.".into(),
});
let exited = CoreTurnItem::ExitedReviewMode(CoreExitedReviewModeItem {
id: "exited-review".into(),
review_output: None,
});
let events = vec![
EventMsg::ItemCompleted(ItemCompletedEvent {
thread_id,
turn_id: "turn-1".into(),
item: entered,
completed_at_ms: 0,
}),
EventMsg::ItemCompleted(ItemCompletedEvent {
thread_id,
turn_id: "turn-1".into(),
item: exited,
completed_at_ms: 0,
}),
EventMsg::TurnComplete(TurnCompleteEvent {
turn_id: "turn-1".into(),
last_agent_message: None,
completed_at: None,
duration_ms: None,
time_to_first_token_ms: None,
}),
];
let mut builder = ThreadHistoryBuilder::new();
for event in &events {
builder.handle_event(event);
}
let turns = builder.finish();
assert_eq!(turns[0].id, "turn-1");
assert_eq!(
turns[0].items,
vec![
ThreadItem::EnteredReviewMode {
id: "entered-review".into(),
review: "Review requested.".into(),
},
ThreadItem::ExitedReviewMode {
id: "exited-review".into(),
review: REVIEW_FALLBACK_MESSAGE.into(),
},
]
);
}
#[test]
fn rebuilds_user_message_image_details_from_legacy_events() {
let local_path = PathBuf::from("/tmp/local.png");
@@ -3932,7 +4074,6 @@ mod tests {
..Default::default()
}),
));
assert_eq!(
changes,
ThreadHistoryChangeSet {
@@ -3979,7 +4120,6 @@ mod tests {
},
}),
));
assert_eq!(
changes,
ThreadHistoryChangeSet {
@@ -4014,7 +4154,6 @@ mod tests {
text: "raw content".into(),
}),
));
assert_eq!(
changes,
ThreadHistoryChangeSet {
@@ -4114,7 +4253,6 @@ mod tests {
},
})),
]);
assert_eq!(
changes,
ThreadHistoryChangeSet {
codex-rs/app-server-protocol/src/protocol/v2/item.rs
+9
View File
@@ -10,6 +10,7 @@ use super::UserInput;
use super::shared::v2_enum_from_core;
use crate::protocol::item_builders::command_actions_for_path_uri;
use crate::protocol::item_builders::convert_patch_changes;
use crate::protocol::item_builders::review_output_text;
use codex_experimental_api_macros::ExperimentalApi;
use codex_extension_items::ExtensionItem;
pub use codex_extension_items::image_generation::ImageGenerationItem;
@@ -923,6 +924,14 @@ impl From<CoreTurnItem> for ThreadItem {
saved_path: image.saved_path,
})
}
CoreTurnItem::EnteredReviewMode(review) => ThreadItem::EnteredReviewMode {
id: review.id,
review: review.user_facing_hint,
},
CoreTurnItem::ExitedReviewMode(review) => ThreadItem::ExitedReviewMode {
id: review.id,
review: review_output_text(review.review_output.as_ref()),
},
CoreTurnItem::FileChange(file_change) => ThreadItem::FileChange {
id: file_change.id,
changes: convert_patch_changes(&file_change.changes),
codex-rs/app-server/src/bespoke_event_handling.rs
+5 -83
View File
@@ -86,8 +86,6 @@ use codex_app_server_protocol::guardian_auto_approval_review_notification;
use codex_app_server_protocol::item_event_to_server_notification;
use codex_core::CodexThread;
use codex_core::ThreadManager;
use codex_core::review_format::format_review_findings_block;
use codex_core::review_prompts;
use codex_protocol::ThreadId;
use codex_protocol::items::CollabAgentTool as CoreCollabAgentTool;
use codex_protocol::items::TurnItem as CoreTurnItem;
@@ -101,7 +99,6 @@ use codex_protocol::protocol::ExecApprovalRequestEvent;
use codex_protocol::protocol::Op;
use codex_protocol::protocol::RealtimeEvent;
use codex_protocol::protocol::ReviewDecision;
use codex_protocol::protocol::ReviewOutputEvent;
use codex_protocol::protocol::SubAgentActivityKind;
use codex_protocol::protocol::TokenCountEvent;
use codex_protocol::protocol::TurnAbortedEvent;
@@ -837,11 +834,13 @@ pub(crate) async fn apply_bespoke_event_handling(
| EventMsg::CollabResumeEnd(_)
| EventMsg::SubAgentActivity(_)
| EventMsg::ExecCommandBegin(_)
| EventMsg::ExecCommandEnd(_) => {
| EventMsg::ExecCommandEnd(_)
| EventMsg::EnteredReviewMode(_)
| EventMsg::ExitedReviewMode(_) => {
// Deprecated item lifecycle events are still fanned out for raw-event and rollout
// compatibility consumers.
// App-server v2 receives canonical TurnItem lifecycle instead, and dispatches
// dynamic tool requests from canonical DynamicToolCall starts.
// App-server v2 receives TurnItem lifecycle instead, and dispatches dynamic tool
// requests from DynamicToolCall starts.
}
EventMsg::McpToolCallBegin(_) | EventMsg::McpToolCallEnd(_) => {
// Deprecated MCP tool-call events are still fanned out for raw-event and rollout
@@ -938,33 +937,6 @@ pub(crate) async fn apply_bespoke_event_handling(
.await;
}
EventMsg::ViewImageToolCall(_) => {}
EventMsg::EnteredReviewMode(review_request) => {
let review = review_request
.user_facing_hint
.unwrap_or_else(|| review_prompts::user_facing_hint(&review_request.target));
let item = ThreadItem::EnteredReviewMode {
id: event_turn_id.clone(),
review,
};
let started = ItemStartedNotification {
thread_id: conversation_id.to_string(),
turn_id: event_turn_id.clone(),
started_at_ms: now_unix_timestamp_ms(),
item: item.clone(),
};
outgoing
.send_server_notification(ServerNotification::ItemStarted(started))
.await;
let completed = ItemCompletedNotification {
thread_id: conversation_id.to_string(),
turn_id: event_turn_id.clone(),
completed_at_ms: now_unix_timestamp_ms(),
item,
};
outgoing
.send_server_notification(ServerNotification::ItemCompleted(completed))
.await;
}
EventMsg::ItemStarted(event) => {
let should_emit = match &event.item {
// Approval and guardian flows can emit the command start notification before core
@@ -1049,34 +1021,6 @@ pub(crate) async fn apply_bespoke_event_handling(
.send_server_notification(ServerNotification::HookCompleted(notification))
.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 item = ThreadItem::ExitedReviewMode {
id: event_turn_id.clone(),
review,
};
let started = ItemStartedNotification {
thread_id: conversation_id.to_string(),
turn_id: event_turn_id.clone(),
started_at_ms: now_unix_timestamp_ms(),
item: item.clone(),
};
outgoing
.send_server_notification(ServerNotification::ItemStarted(started))
.await;
let completed = ItemCompletedNotification {
thread_id: conversation_id.to_string(),
turn_id: event_turn_id.clone(),
completed_at_ms: now_unix_timestamp_ms(),
item,
};
outgoing
.send_server_notification(ServerNotification::ItemCompleted(completed))
.await;
}
EventMsg::RawResponseItem(raw_response_item_event) => {
maybe_emit_hook_prompt_item_completed(
conversation_id,
@@ -1952,28 +1896,6 @@ fn request_permissions_response_from_client_result(
}))
}
const REVIEW_FALLBACK_MESSAGE: &str = "Reviewer failed to output a response.";
fn render_review_output_text(output: &ReviewOutputEvent) -> String {
let mut sections = Vec::new();
let explanation = output.overall_explanation.trim();
if !explanation.is_empty() {
sections.push(explanation.to_string());
}
if !output.findings.is_empty() {
let findings = format_review_findings_block(&output.findings, /*selection*/ None);
let trimmed = findings.trim();
if !trimmed.is_empty() {
sections.push(trimmed.to_string());
}
}
if sections.is_empty() {
REVIEW_FALLBACK_MESSAGE.to_string()
} else {
sections.join("\n\n")
}
}
fn map_file_change_approval_decision(decision: FileChangeApprovalDecision) -> ReviewDecision {
match decision {
FileChangeApprovalDecision::Accept => ReviewDecision::Approved,
codex-rs/app-server/tests/suite/v2/review.rs
+4 -4
View File
@@ -113,8 +113,8 @@ async fn review_start_runs_review_turn_and_emits_code_review_item() -> Result<()
let started: ItemStartedNotification =
serde_json::from_value(item_started.params.expect("params must be present"))?;
match started.item {
ThreadItem::EnteredReviewMode { id, review } => {
assert_eq!(id, turn_id);
ThreadItem::EnteredReviewMode { review, .. } => {
assert_eq!(started.turn_id, turn_id);
assert_eq!(review, "commit 1234567: Tidy UI colors");
saw_entered_review_mode = true;
break;
@@ -139,8 +139,8 @@ async fn review_start_runs_review_turn_and_emits_code_review_item() -> Result<()
let completed: ItemCompletedNotification =
serde_json::from_value(review_notif.params.expect("params must be present"))?;
match completed.item {
ThreadItem::ExitedReviewMode { id, review } => {
assert_eq!(id, turn_id);
ThreadItem::ExitedReviewMode { review, .. } => {
assert_eq!(completed.turn_id, turn_id);
review_body = Some(review);
break;
}
codex-rs/core/Cargo.toml
+1 -1
View File
@@ -118,7 +118,7 @@ toml = { workspace = true }
toml_edit = { workspace = true }
tracing = { workspace = true, features = ["log"] }
url = { workspace = true }
uuid = { workspace = true, features = ["serde", "v4", "v5"] }
uuid = { workspace = true, features = ["serde", "v4", "v5", "v7"] }
which = { workspace = true }
whoami = { workspace = true }
codex-rs/core/src/lib.rs
-1
View File
@@ -110,7 +110,6 @@ pub mod windows_sandbox;
pub use client::X_RESPONSESAPI_INCLUDE_TIMING_METRICS_HEADER;
pub use codex_protocol::config_types::ModelProviderAuthInfo;
mod event_mapping;
pub mod review_format;
pub use codex_prompts as review_prompts;
mod thread_manager;
pub(crate) mod web_search;
codex-rs/core/src/session/mod.rs
+1 -1
View File
@@ -97,6 +97,7 @@ use codex_protocol::config_types::Settings;
use codex_protocol::config_types::WebSearchMode;
use codex_protocol::dynamic_tools::DynamicToolResponse;
use codex_protocol::dynamic_tools::DynamicToolSpec;
use codex_protocol::items::EnteredReviewModeItem;
use codex_protocol::items::TurnItem;
use codex_protocol::items::UserMessageItem;
use codex_protocol::models::ActivePermissionProfile;
@@ -117,7 +118,6 @@ use codex_protocol::protocol::ItemCompletedEvent;
use codex_protocol::protocol::ItemStartedEvent;
use codex_protocol::protocol::MultiAgentVersion;
use codex_protocol::protocol::RawResponseItemEvent;
use codex_protocol::protocol::ReviewRequest;
use codex_protocol::protocol::RolloutItem;
use codex_protocol::protocol::SessionSource;
use codex_protocol::protocol::SubAgentSource;
codex-rs/core/src/session/review.rs
+6 -5
View File
@@ -165,10 +165,11 @@ pub(super) async fn spawn_review_thread(
sess.spawn_task(tc.clone(), input, ReviewTask::new()).await;
// Announce entering review mode so UIs can switch modes.
let review_request = ReviewRequest {
let item = TurnItem::EnteredReviewMode(EnteredReviewModeItem {
id: uuid::Uuid::now_v7().to_string(),
target: resolved.target,
user_facing_hint: Some(resolved.user_facing_hint),
};
sess.send_event(&tc, EventMsg::EnteredReviewMode(review_request))
.await;
user_facing_hint: resolved.user_facing_hint,
});
sess.emit_turn_item_started(&tc, &item).await;
sess.emit_turn_item_completed(&tc, item).await;
}
codex-rs/core/src/tasks/review.rs
+11 -11
View File
@@ -3,6 +3,7 @@ use std::sync::Arc;
use codex_prompts::render_review_exit_interrupted;
use codex_prompts::render_review_exit_success;
use codex_protocol::config_types::WebSearchMode;
use codex_protocol::items::ExitedReviewModeItem;
use codex_protocol::items::TurnItem;
use codex_protocol::models::ContentItem;
use codex_protocol::models::ResponseItem;
@@ -10,16 +11,15 @@ use codex_protocol::protocol::AgentMessageContentDeltaEvent;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::Event;
use codex_protocol::protocol::EventMsg;
use codex_protocol::protocol::ExitedReviewModeEvent;
use codex_protocol::protocol::ItemCompletedEvent;
use codex_protocol::protocol::ReviewOutputEvent;
use codex_protocol::protocol::SubAgentSource;
use codex_protocol::review_format::format_review_findings_block;
use codex_protocol::review_format::render_review_output_text;
use tokio_util::sync::CancellationToken;
use crate::codex_delegate::run_codex_thread_one_shot;
use crate::config::Constrained;
use crate::review_format::format_review_findings_block;
use crate::review_format::render_review_output_text;
use crate::session::TurnInput;
use crate::session::session::Session;
use crate::session::turn_context::TurnContext;
@@ -210,8 +210,8 @@ fn parse_review_output_event(text: &str) -> ReviewOutputEvent {
}
}
/// Emits an ExitedReviewMode Event with optional ReviewOutput,
/// and records a developer message with the review output.
/// Emits ExitedReviewMode item lifecycle with optional ReviewOutput,
/// and records the review output back into conversation history.
pub(crate) async fn exit_review_mode(
session: Arc<Session>,
review_output: Option<ReviewOutputEvent>,
@@ -253,12 +253,12 @@ pub(crate) async fn exit_review_mode(
)
.await;
session
.send_event(
ctx.as_ref(),
EventMsg::ExitedReviewMode(ExitedReviewModeEvent { review_output }),
)
.await;
let item = TurnItem::ExitedReviewMode(ExitedReviewModeItem {
id: uuid::Uuid::now_v7().to_string(),
review_output,
});
session.emit_turn_item_started(ctx.as_ref(), &item).await;
session.emit_turn_item_completed(ctx.as_ref(), item).await;
session
.record_response_item_and_emit_turn_item(
ctx.as_ref(),
codex-rs/core/tests/suite/review.rs
+87 -14
View File
@@ -1,7 +1,7 @@
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::items::TurnItem;
use codex_protocol::models::ContentItem;
use codex_protocol::models::ResponseItem;
use codex_protocol::protocol::ENVIRONMENT_CONTEXT_OPEN_TAG;
@@ -16,6 +16,7 @@ use codex_protocol::protocol::ReviewRequest;
use codex_protocol::protocol::ReviewTarget;
use codex_protocol::protocol::RolloutItem;
use codex_protocol::protocol::RolloutLine;
use codex_protocol::review_format::render_review_output_text;
use codex_protocol::user_input::UserInput;
use core_test_support::PathBufExt;
use core_test_support::responses;
@@ -35,9 +36,8 @@ use tokio::io::AsyncWriteExt as _;
use uuid::Uuid;
use wiremock::MockServer;
/// Verify that submitting `Op::Review` spawns a child task and emits
/// EnteredReviewMode -> ExitedReviewMode(None) -> TurnComplete
/// in that order when the model returns a structured review JSON payload.
/// Verify that submitting `Op::Review` emits review item lifecycle,
/// legacy review events, and TurnComplete when the model returns a structured review payload.
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn review_op_emits_lifecycle_and_review_output() {
// Skip under Codex sandbox network restrictions.
@@ -84,13 +84,82 @@ async fn review_op_emits_lifecycle_and_review_output() {
.await
.unwrap();
// Verify lifecycle: Entered -> Exited(Some(review)) -> TurnComplete.
let _entered = wait_for_event(&codex, |ev| matches!(ev, EventMsg::EnteredReviewMode(_))).await;
// Item lifecycle events are emitted first, then the legacy review event is fanned out
// with the same stable IDs for compatibility consumers.
let entered_started = wait_for_event(&codex, |ev| {
matches!(
ev,
EventMsg::ItemStarted(event)
if matches!(event.item, TurnItem::EnteredReviewMode(_))
)
})
.await;
let (review_turn_id, entered_item_id) = match entered_started {
EventMsg::ItemStarted(event) => (event.turn_id, event.item.id()),
other => panic!("expected entered review item start, got {other:?}"),
};
let entered_completed = wait_for_event(&codex, |ev| {
matches!(
ev,
EventMsg::ItemCompleted(event)
if matches!(event.item, TurnItem::EnteredReviewMode(_))
)
})
.await;
match entered_completed {
EventMsg::ItemCompleted(event) => {
assert_eq!(event.turn_id, review_turn_id);
assert_eq!(event.item.id(), entered_item_id);
}
other => panic!("expected entered review item completion, got {other:?}"),
}
let entered = wait_for_event(&codex, |ev| matches!(ev, EventMsg::EnteredReviewMode(_))).await;
match entered {
EventMsg::EnteredReviewMode(event) => {
assert_eq!(event.turn_id.as_deref(), Some(review_turn_id.as_str()));
assert_eq!(event.item_id.as_deref(), Some(entered_item_id.as_str()));
}
other => panic!("expected EnteredReviewMode(..), got {other:?}"),
}
let exited_started = wait_for_event(&codex, |ev| {
matches!(
ev,
EventMsg::ItemStarted(event)
if matches!(event.item, TurnItem::ExitedReviewMode(_))
)
})
.await;
let exited_item_id = match exited_started {
EventMsg::ItemStarted(event) => {
assert_eq!(event.turn_id, review_turn_id);
event.item.id()
}
other => panic!("expected exited review item start, got {other:?}"),
};
let exited_completed = wait_for_event(&codex, |ev| {
matches!(
ev,
EventMsg::ItemCompleted(event)
if matches!(event.item, TurnItem::ExitedReviewMode(_))
)
})
.await;
match exited_completed {
EventMsg::ItemCompleted(event) => {
assert_eq!(event.turn_id, review_turn_id);
assert_eq!(event.item.id(), exited_item_id);
}
other => panic!("expected exited review item completion, got {other:?}"),
}
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)"),
EventMsg::ExitedReviewMode(ev) => {
assert_eq!(ev.turn_id.as_deref(), Some(review_turn_id.as_str()));
assert_eq!(ev.item_id.as_deref(), Some(exited_item_id.as_str()));
ev.review_output
.expect("expected ExitedReviewMode with Some(review_output)")
}
other => panic!("expected ExitedReviewMode(..), got {other:?}"),
};
@@ -271,7 +340,7 @@ async fn cancelled_review_does_not_forward_delegate_mcp_startup() {
event @ (EventMsg::McpStartupUpdate(_) | EventMsg::McpStartupComplete(_)) => {
panic!("cancelled review forwarded delegate MCP startup: {event:?}")
}
EventMsg::ExitedReviewMode(ExitedReviewModeEvent { review_output }) => {
EventMsg::ExitedReviewMode(ExitedReviewModeEvent { review_output, .. }) => {
assert_eq!(review_output, None);
exited_review = true;
}
@@ -512,7 +581,8 @@ async fn review_uses_custom_review_model_from_config() {
matches!(
ev,
EventMsg::ExitedReviewMode(ExitedReviewModeEvent {
review_output: None
review_output: None,
..
})
)
})
@@ -561,7 +631,8 @@ async fn review_uses_session_model_when_review_model_unset() {
matches!(
ev,
EventMsg::ExitedReviewMode(ExitedReviewModeEvent {
review_output: None
review_output: None,
..
})
)
})
@@ -677,7 +748,8 @@ async fn review_input_isolated_from_parent_history() {
matches!(
ev,
EventMsg::ExitedReviewMode(ExitedReviewModeEvent {
review_output: None
review_output: None,
..
})
)
})
@@ -788,7 +860,8 @@ async fn review_history_surfaces_in_parent_session() {
matches!(
ev,
EventMsg::ExitedReviewMode(ExitedReviewModeEvent {
review_output: Some(_)
review_output: Some(_),
..
})
)
})
codex-rs/protocol/src/items.rs
+19
View File
@@ -16,6 +16,8 @@ use crate::protocol::ExecCommandSource;
use crate::protocol::ExecCommandStatus;
use crate::protocol::FileChange;
use crate::protocol::PatchApplyStatus;
use crate::protocol::ReviewOutputEvent;
use crate::protocol::ReviewTarget;
use crate::protocol::SubAgentActivityKind;
use crate::user_input::ByteRange;
use crate::user_input::TextElement;
@@ -64,6 +66,8 @@ pub enum TurnItem {
/// This remains separate from [`Self::Extension`] because core still owns
/// hosted image persistence and legacy-event fanout.
ImageGeneration(ImageGenerationItem),
EnteredReviewMode(EnteredReviewModeItem),
ExitedReviewMode(ExitedReviewModeItem),
FileChange(FileChangeItem),
McpToolCall(McpToolCallItem),
ContextCompaction(ContextCompactionItem),
@@ -129,6 +133,19 @@ pub struct AgentMessageItem {
pub memory_citation: Option<MemoryCitation>,
}
#[derive(Debug, Clone, Deserialize, Serialize, TS, JsonSchema)]
pub struct EnteredReviewModeItem {
pub id: String,
pub target: ReviewTarget,
pub user_facing_hint: String,
}
#[derive(Debug, Clone, Deserialize, Serialize, TS, JsonSchema)]
pub struct ExitedReviewModeItem {
pub id: String,
pub review_output: Option<ReviewOutputEvent>,
}
#[derive(Debug, Clone, Deserialize, Serialize, TS, JsonSchema)]
pub struct PlanItem {
pub id: String,
@@ -621,6 +638,8 @@ impl TurnItem {
TurnItem::Sleep(item) => item.id.clone(),
TurnItem::Extension(item) => item.id().to_string(),
TurnItem::ImageGeneration(item) => item.id.clone(),
TurnItem::EnteredReviewMode(item) => item.id.clone(),
TurnItem::ExitedReviewMode(item) => item.id.clone(),
TurnItem::FileChange(item) => item.id.clone(),
TurnItem::McpToolCall(item) => item.id.clone(),
TurnItem::ContextCompaction(item) => item.id.clone(),
codex-rs/protocol/src/legacy_events.rs
+32
View File
@@ -10,6 +10,8 @@ use crate::items::CommandExecutionStatus;
use crate::items::ContextCompactionItem;
use crate::items::DynamicToolCallItem;
use crate::items::DynamicToolCallStatus;
use crate::items::EnteredReviewModeItem;
use crate::items::ExitedReviewModeItem;
use crate::items::FileChangeItem;
use crate::items::ImageGenerationItem;
use crate::items::McpToolCallItem;
@@ -36,10 +38,12 @@ use crate::protocol::CollabWaitingBeginEvent;
use crate::protocol::CollabWaitingEndEvent;
use crate::protocol::ContextCompactedEvent;
use crate::protocol::DynamicToolCallResponseEvent;
use crate::protocol::EnteredReviewModeEvent;
use crate::protocol::EventMsg;
use crate::protocol::ExecCommandBeginEvent;
use crate::protocol::ExecCommandEndEvent;
use crate::protocol::ExecCommandStatus;
use crate::protocol::ExitedReviewModeEvent;
use crate::protocol::ImageGenerationBeginEvent;
use crate::protocol::ImageGenerationEndEvent;
use crate::protocol::ItemCompletedEvent;
@@ -101,6 +105,27 @@ impl AgentMessageItem {
}
}
impl EnteredReviewModeItem {
pub fn as_legacy_event(&self, turn_id: String) -> EventMsg {
EventMsg::EnteredReviewMode(EnteredReviewModeEvent {
target: self.target.clone(),
user_facing_hint: Some(self.user_facing_hint.clone()),
turn_id: Some(turn_id),
item_id: Some(self.id.clone()),
})
}
}
impl ExitedReviewModeItem {
pub fn as_legacy_event(&self, turn_id: String) -> EventMsg {
EventMsg::ExitedReviewMode(ExitedReviewModeEvent {
turn_id: Some(turn_id),
item_id: Some(self.id.clone()),
review_output: self.review_output.clone(),
})
}
}
impl ReasoningItem {
pub fn as_legacy_events(&self, show_raw_agent_reasoning: bool) -> Vec<EventMsg> {
let mut events = Vec::new();
@@ -489,6 +514,7 @@ impl TurnItem {
TurnItem::Sleep(_) => Vec::new(),
TurnItem::Extension(_) => Vec::new(),
TurnItem::ImageGeneration(item) => vec![item.as_legacy_event()],
TurnItem::EnteredReviewMode(_) | TurnItem::ExitedReviewMode(_) => Vec::new(),
TurnItem::FileChange(item) => item
.as_legacy_end_event(String::new())
.into_iter()
@@ -551,6 +577,12 @@ impl HasLegacyEvent for ItemCompletedEvent {
TurnItem::SubAgentActivity(item) => {
vec![item.as_legacy_event(self.completed_at_ms)]
}
TurnItem::EnteredReviewMode(item) => {
vec![item.as_legacy_event(self.turn_id.clone())]
}
TurnItem::ExitedReviewMode(item) => {
vec![item.as_legacy_event(self.turn_id.clone())]
}
_ => self.item.as_legacy_events(show_raw_agent_reasoning),
}
}
codex-rs/protocol/src/lib.rs
+1
View File
@@ -30,5 +30,6 @@ pub mod plan_tool;
pub mod protocol;
pub mod request_permissions;
pub mod request_user_input;
pub mod review_format;
pub mod shell_environment;
pub mod user_input;
codex-rs/protocol/src/protocol.rs
+101 -1
View File
@@ -1428,7 +1428,7 @@ pub enum EventMsg {
ShutdownComplete,
/// Entered review mode.
EnteredReviewMode(ReviewRequest),
EnteredReviewMode(EnteredReviewModeEvent),
/// Exited review mode with an optional final result to apply.
ExitedReviewMode(ExitedReviewModeEvent),
@@ -1852,8 +1852,28 @@ pub struct ReasoningRawContentDeltaEvent {
pub content_index: i64,
}
#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)]
pub struct EnteredReviewModeEvent {
pub target: ReviewTarget,
#[serde(skip_serializing_if = "Option::is_none")]
#[ts(optional)]
pub user_facing_hint: Option<String>,
#[serde(default, skip_serializing_if = "Option::is_none")]
#[ts(optional)]
pub turn_id: Option<String>,
#[serde(default, skip_serializing_if = "Option::is_none")]
#[ts(optional)]
pub item_id: Option<String>,
}
#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)]
pub struct ExitedReviewModeEvent {
#[serde(default, skip_serializing_if = "Option::is_none")]
#[ts(optional)]
pub turn_id: Option<String>,
#[serde(default, skip_serializing_if = "Option::is_none")]
#[ts(optional)]
pub item_id: Option<String>,
pub review_output: Option<ReviewOutputEvent>,
}
@@ -4313,6 +4333,8 @@ mod tests {
use crate::items::CommandExecutionStatus;
use crate::items::DynamicToolCallItem;
use crate::items::DynamicToolCallStatus;
use crate::items::EnteredReviewModeItem;
use crate::items::ExitedReviewModeItem;
use crate::items::FileChangeItem;
use crate::items::ImageGenerationItem;
use crate::items::McpToolCallItem;
@@ -5427,6 +5449,63 @@ mod tests {
));
}
#[test]
fn review_mode_item_completion_emits_legacy_events_with_ids() {
let entered = ItemCompletedEvent {
thread_id: ThreadId::new(),
turn_id: "turn-1".into(),
completed_at_ms: 0,
item: TurnItem::EnteredReviewMode(EnteredReviewModeItem {
id: "entered-review".into(),
target: ReviewTarget::Custom {
instructions: "review this".into(),
},
user_facing_hint: "Review requested.".into(),
}),
};
let exited = ItemCompletedEvent {
thread_id: ThreadId::new(),
turn_id: "turn-1".into(),
completed_at_ms: 0,
item: TurnItem::ExitedReviewMode(ExitedReviewModeItem {
id: "exited-review".into(),
review_output: Some(ReviewOutputEvent {
overall_explanation: "Looks good.".into(),
..Default::default()
}),
}),
};
assert!(matches!(
entered
.as_legacy_events(/*show_raw_agent_reasoning*/ false)
.as_slice(),
[EventMsg::EnteredReviewMode(EnteredReviewModeEvent {
target: ReviewTarget::Custom { instructions },
user_facing_hint: Some(user_facing_hint),
turn_id: Some(turn_id),
item_id: Some(item_id),
})]
if instructions == "review this"
&& user_facing_hint == "Review requested."
&& turn_id == "turn-1"
&& item_id == "entered-review"
));
assert!(matches!(
exited
.as_legacy_events(/*show_raw_agent_reasoning*/ false)
.as_slice(),
[EventMsg::ExitedReviewMode(ExitedReviewModeEvent {
turn_id: Some(turn_id),
item_id: Some(item_id),
review_output: Some(review_output),
})]
if turn_id == "turn-1"
&& item_id == "exited-review"
&& review_output.overall_explanation == "Looks good."
));
}
#[test]
fn item_started_event_requires_started_at_ms() {
let mut value = serde_json::to_value(ItemStartedEvent {
@@ -5456,6 +5535,27 @@ mod tests {
assert_eq!(event.completed_at_ms, 0);
}
#[test]
fn review_mode_events_deserialize_legacy_payloads() {
let entered = serde_json::from_value::<EnteredReviewModeEvent>(json!({
"target": {
"type": "custom",
"instructions": "review this"
},
"user_facing_hint": "hint"
}))
.unwrap();
assert_eq!(entered.turn_id, None);
assert_eq!(entered.item_id, None);
let exited = serde_json::from_value::<ExitedReviewModeEvent>(json!({
"review_output": null
}))
.unwrap();
assert_eq!(exited.turn_id, None);
assert_eq!(exited.item_id, None);
}
#[test]
fn rollback_failed_error_does_not_affect_turn_status() {
let event = ErrorEvent {
codex-rs/core/src/review_format.rs → codex-rs/protocol/src/review_format.rs
+5 -5
View File
@@ -1,8 +1,7 @@
use codex_protocol::protocol::ReviewFinding;
use codex_protocol::protocol::ReviewOutputEvent;
use crate::protocol::ReviewFinding;
use crate::protocol::ReviewOutputEvent;
// Note: We keep this module UI-agnostic. It returns plain strings that
// higher layers (e.g., TUI) may style as needed.
// These helpers return plain strings that higher layers may style as needed.
fn format_location(item: &ReviewFinding) -> String {
let path = item.code_location.absolute_file_path.display();
@@ -11,7 +10,8 @@ fn format_location(item: &ReviewFinding) -> String {
format!("{path}:{start}-{end}")
}
const REVIEW_FALLBACK_MESSAGE: &str = "Reviewer failed to output a response.";
/// Fallback text used when a review contains no displayable output.
pub const REVIEW_FALLBACK_MESSAGE: &str = "Reviewer failed to output a response.";
/// Format a full review findings block as plain text lines.
///
codex-rs/rollout/src/persistence_metrics.rs
+2
View File
@@ -256,6 +256,8 @@ fn turn_item_type(item: &TurnItem) -> &'static str {
TurnItem::Sleep(_) => "sleep",
TurnItem::Extension(_) => "extension",
TurnItem::ImageGeneration(_) => "image_generation",
TurnItem::EnteredReviewMode(_) => "entered_review_mode",
TurnItem::ExitedReviewMode(_) => "exited_review_mode",
TurnItem::FileChange(_) => "file_change",
TurnItem::McpToolCall(_) => "mcp_tool_call",
TurnItem::ContextCompaction(_) => "context_compaction",