agent/codex
2
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

chore: drop review prompt from TUI UX (#18659)

Browse Source 
Due to the app-server rebase of the TUI, the review prompt was leaked
into the transcript on the TUI
This is not a security issue but it was bad UX. This PR fixes this
This commit is contained in:
jif-oai
2026-04-20 14:31:37 +01:00
committed by GitHub
Unverified
parent 2c59806fe0
commit 7e5588699d
3 changed files with 123 additions and 78 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/tui/src/chatwidget.rs
+49 -77
View File
@@ -134,6 +134,7 @@ use codex_protocol::config_types::Settings;
use codex_protocol::config_types::WindowsSandboxLevel;
use codex_protocol::items::AgentMessageContent;
use codex_protocol::items::AgentMessageItem;
use codex_protocol::items::UserMessageItem;
use codex_protocol::models::MessagePhase;
use codex_protocol::models::local_image_label_text;
use codex_protocol::parse_command::ParsedCommand;
@@ -167,7 +168,6 @@ use codex_protocol::protocol::DeprecationNoticeEvent;
use codex_protocol::protocol::ErrorEvent;
#[cfg(test)]
use codex_protocol::protocol::Event;
#[cfg(test)]
use codex_protocol::protocol::EventMsg;
use codex_protocol::protocol::ExecApprovalRequestEvent;
use codex_protocol::protocol::ExecCommandBeginEvent;
@@ -6005,54 +6005,14 @@ impl ChatWidget {
let replay_kind = render_source.replay_kind();
match item {
ThreadItem::UserMessage { id, content } => {
let user_message = codex_protocol::items::UserMessageItem {
let user_message = UserMessageItem {
id,
content: content
.into_iter()
.map(codex_app_server_protocol::UserInput::into_core)
.collect(),
};
let codex_protocol::protocol::EventMsg::UserMessage(event) =
user_message.as_legacy_event()
else {
unreachable!("user message item should convert to a user message event");
};
if from_replay {
self.on_user_message_event(event);
} else {
let rendered = Self::rendered_user_message_event_from_event(&event);
let compare_key =
Self::pending_steer_compare_key_from_items(&user_message.content);
if self
.pending_steers
.front()
.is_some_and(|pending| pending.compare_key == compare_key)
{
if let Some(pending) = self.pending_steers.pop_front() {
self.refresh_pending_input_preview();
let pending_event = UserMessageEvent {
message: pending.user_message.text,
images: Some(pending.user_message.remote_image_urls),
local_images: pending
.user_message
.local_images
.into_iter()
.map(|image| image.path)
.collect(),
text_elements: pending.user_message.text_elements,
};
self.on_user_message_event(pending_event);
} else if self.last_rendered_user_message_event.as_ref() != Some(&rendered)
{
tracing::warn!(
"pending steer matched compare key but queue was empty when rendering committed user message"
);
self.on_user_message_event(event);
}
} else if self.last_rendered_user_message_event.as_ref() != Some(&rendered) {
self.on_user_message_event(event);
}
}
self.on_committed_user_message(&user_message, from_replay);
}
ThreadItem::AgentMessage {
id,
@@ -7168,40 +7128,7 @@ impl ChatWidget {
EventMsg::ItemCompleted(event) => {
let item = event.item;
if !from_replay && let codex_protocol::items::TurnItem::UserMessage(item) = &item {
let EventMsg::UserMessage(event) = item.as_legacy_event() else {
unreachable!("user message item should convert to a legacy user message");
};
let rendered = Self::rendered_user_message_event_from_event(&event);
let compare_key = Self::pending_steer_compare_key_from_item(item);
if self
.pending_steers
.front()
.is_some_and(|pending| pending.compare_key == compare_key)
{
if let Some(pending) = self.pending_steers.pop_front() {
self.refresh_pending_input_preview();
let pending_event = UserMessageEvent {
message: pending.user_message.text,
images: Some(pending.user_message.remote_image_urls),
local_images: pending
.user_message
.local_images
.into_iter()
.map(|image| image.path)
.collect(),
text_elements: pending.user_message.text_elements,
};
self.on_user_message_event(pending_event);
} else if self.last_rendered_user_message_event.as_ref() != Some(&rendered)
{
tracing::warn!(
"pending steer matched compare key but queue was empty when rendering committed user message"
);
self.on_user_message_event(event);
}
} else if self.last_rendered_user_message_event.as_ref() != Some(&rendered) {
self.on_user_message_event(event);
}
self.on_committed_user_message(item, from_replay);
}
if let codex_protocol::items::TurnItem::Plan(plan_item) = &item {
self.on_plan_item_completed(plan_item.text.clone());
@@ -7286,6 +7213,51 @@ impl ChatWidget {
self.exit_review_mode_after_item();
}
fn on_committed_user_message(&mut self, item: &UserMessageItem, from_replay: bool) {
let EventMsg::UserMessage(event) = item.as_legacy_event() else {
unreachable!("user message item should convert to a legacy user message");
};
if from_replay {
if !self.is_review_mode {
self.on_user_message_event(event);
}
return;
}
let rendered = Self::rendered_user_message_event_from_event(&event);
let compare_key = Self::pending_steer_compare_key_from_item(item);
if self
.pending_steers
.front()
.is_some_and(|pending| pending.compare_key == compare_key)
{
if let Some(pending) = self.pending_steers.pop_front() {
self.refresh_pending_input_preview();
let pending_event = UserMessageEvent {
message: pending.user_message.text,
images: Some(pending.user_message.remote_image_urls),
local_images: pending
.user_message
.local_images
.into_iter()
.map(|image| image.path)
.collect(),
text_elements: pending.user_message.text_elements,
};
self.on_user_message_event(pending_event);
} else if self.last_rendered_user_message_event.as_ref() != Some(&rendered) {
tracing::warn!(
"pending steer matched compare key but queue was empty when rendering committed user message"
);
self.on_user_message_event(event);
}
} else if !self.is_review_mode
&& self.last_rendered_user_message_event.as_ref() != Some(&rendered)
{
self.on_user_message_event(event);
}
}
fn on_user_message_event(&mut self, event: UserMessageEvent) {
self.last_rendered_user_message_event =
Some(Self::rendered_user_message_event_from_event(&event));
codex-rs/tui/src/chatwidget/realtime.rs
-1
View File
@@ -132,7 +132,6 @@ impl ChatWidget {
}
}
#[cfg(test)]
pub(super) fn pending_steer_compare_key_from_item(
item: &codex_protocol::items::UserMessageItem,
) -> PendingSteerCompareKey {
codex-rs/tui/src/chatwidget/tests/review_mode.rs
+74
View File
@@ -146,6 +146,80 @@ async fn entered_review_mode_defaults_to_current_changes_banner() {
assert!(chat.is_review_mode);
}
#[tokio::test]
async fn live_core_review_prompt_item_is_not_rendered() {
let (mut chat, mut rx, _ops) = make_chatwidget_manual(/*model_override*/ None).await;
chat.handle_codex_event(Event {
id: "review-start".into(),
msg: EventMsg::EnteredReviewMode(ReviewRequest {
target: ReviewTarget::BaseBranch {
branch: "main".to_string(),
},
user_facing_hint: Some("changes against 'main'".to_string()),
}),
});
let cells = drain_insert_history(&mut rx);
assert_eq!(cells.len(), 1);
assert!(lines_to_single_string(&cells[0]).contains("Code review started"));
complete_user_message(
&mut chat,
"review-prompt",
"Review the code changes against the base branch 'main'.",
);
assert!(drain_insert_history(&mut rx).is_empty());
}
#[tokio::test]
async fn live_app_server_review_prompt_item_is_not_rendered() {
let (mut chat, mut rx, _ops) = make_chatwidget_manual(/*model_override*/ None).await;
let review_mode_item = AppServerThreadItem::EnteredReviewMode {
id: "review-start".to_string(),
review: "changes against 'main'".to_string(),
};
chat.handle_server_notification(
ServerNotification::ItemStarted(ItemStartedNotification {
thread_id: "thread-1".to_string(),
turn_id: "turn-1".to_string(),
item: review_mode_item.clone(),
}),
/*replay_kind*/ None,
);
let cells = drain_insert_history(&mut rx);
assert_eq!(cells.len(), 1);
assert!(lines_to_single_string(&cells[0]).contains("Code review started"));
chat.handle_server_notification(
ServerNotification::ItemCompleted(ItemCompletedNotification {
thread_id: "thread-1".to_string(),
turn_id: "turn-1".to_string(),
item: review_mode_item,
}),
/*replay_kind*/ None,
);
assert!(drain_insert_history(&mut rx).is_empty());
chat.handle_server_notification(
ServerNotification::ItemCompleted(ItemCompletedNotification {
thread_id: "thread-1".to_string(),
turn_id: "turn-1".to_string(),
item: AppServerThreadItem::UserMessage {
id: "review-prompt".to_string(),
content: vec![AppServerUserInput::Text {
text: "Review the code changes against the base branch 'main'.".to_string(),
text_elements: Vec::new(),
}],
},
}),
/*replay_kind*/ None,
);
assert!(drain_insert_history(&mut rx).is_empty());
}
#[tokio::test]
async fn steer_rejection_queues_review_follow_up_before_existing_queued_messages() {
let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await;