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

Render delegated patch approval details (#19709)

Browse Source 
## Why

Fixes #19632.

When a delegated agent requests approval for an in-progress file change,
the parent TUI handles that request from an inactive thread. The app
server already sent the `FileChange` item with the proposed diff, but
the inactive-thread approval path was not recovering and rendering it
the same way as the active-thread path.

The result was an inconsistent approval prompt: main-thread edits show a
normal patch preview history item before the approval modal, while
delegated edits did not show that preview in the transcript flow.

## What Changed

- Recover buffered or historical `FileChange` item changes when building
inactive-thread file-change approval requests.
- Reuse the app-server file-change conversion helper for both live
transcript rendering and inactive-thread approvals.
- Render recovered delegated patches as a normal patch preview history
cell before the approval modal.
- Keep apply-patch approval modals focused on the decision prompt and
optional metadata; they do not render a synthetic command line or embed
the diff body.

## Manual Repro And Verification

I manually reproduced the issue using a file under `~/Desktop` so the
write would require approval.

Before the fix:

1. Ask the main thread: `Use apply_patch, not shell redirection or
Python, to create ~/Desktop/bug1.txt with three short lines.`
2. Observe the expected TUI shape: the transcript shows a normal patch
preview such as `• Added ~/Desktop/bug1.txt (+N -0)` above the approval
modal, and the modal contains only the approval prompt/options without a
synthetic command line.
3. Ask for the delegated path: `Spawn a worker. Have it use apply_patch,
not shell redirection or Python, to create ~/Desktop/bug1.txt with four
short lines.`
4. Observe the delegated approval is inconsistent: the parent view does
not render the proposed patch as the normal transcript preview before
the modal, so the diff context is missing from the stream or appears
inside the modal instead of in the history flow.

After the fix:

1. Repeat the delegated worker prompt with `apply_patch`.
2. Confirm the parent view renders the same normal patch preview history
cell (`• Added ~/Desktop/bug1.txt (+N -0)` plus the diff) immediately
before the approval modal.
3. Confirm the approval modal remains focused on the decision prompt.
For delegated approvals it may show the worker thread label, but it
should not show a `$ apply_patch` command line or embed the diff body in
the modal.
This commit is contained in:
Eric Traut
2026-04-27 10:07:15 -07:00
committed by GitHub
Unverified
parent 0e2300c02c
commit 48dd7b58f0
9 changed files with 253 additions and 116 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/tui/src/app/tests.rs
+74
View File
@@ -30,6 +30,9 @@ use codex_app_server_protocol::AdditionalPermissionProfile;
use codex_app_server_protocol::AgentMessageDeltaNotification;
use codex_app_server_protocol::CommandExecutionRequestApprovalParams;
use codex_app_server_protocol::ConfigWarningNotification;
use codex_app_server_protocol::FileChangeRequestApprovalParams;
use codex_app_server_protocol::FileUpdateChange;
use codex_app_server_protocol::ItemStartedNotification;
use codex_app_server_protocol::JSONRPCErrorError;
use codex_app_server_protocol::McpServerStartupState;
use codex_app_server_protocol::McpServerStatusUpdatedNotification;
@@ -38,6 +41,7 @@ use codex_app_server_protocol::NetworkApprovalProtocol as AppServerNetworkApprov
use codex_app_server_protocol::NetworkPolicyAmendment as AppServerNetworkPolicyAmendment;
use codex_app_server_protocol::NetworkPolicyRuleAction as AppServerNetworkPolicyRuleAction;
use codex_app_server_protocol::NonSteerableTurnKind as AppServerNonSteerableTurnKind;
use codex_app_server_protocol::PatchChangeKind;
use codex_app_server_protocol::PermissionsRequestApprovalParams;
use codex_app_server_protocol::RequestId as AppServerRequestId;
use codex_app_server_protocol::ServerNotification;
@@ -70,6 +74,7 @@ use codex_protocol::models::PermissionProfile;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::Event;
use codex_protocol::protocol::EventMsg;
use codex_protocol::protocol::FileChange;
use codex_protocol::protocol::NetworkApprovalContext;
use codex_protocol::protocol::NetworkApprovalProtocol;
use codex_protocol::protocol::RolloutItem;
@@ -2522,6 +2527,75 @@ async fn inactive_thread_exec_approval_splits_shell_wrapped_command() {
);
}
#[tokio::test]
async fn inactive_thread_file_change_approval_recovers_buffered_changes() {
let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await;
let thread_id = ThreadId::new();
app.enqueue_thread_notification(
thread_id,
ServerNotification::ItemStarted(ItemStartedNotification {
thread_id: thread_id.to_string(),
turn_id: "turn-approval".to_string(),
item: ThreadItem::FileChange {
id: "patch-approval".to_string(),
changes: vec![FileUpdateChange {
path: "README.md".to_string(),
kind: PatchChangeKind::Add,
diff: "hello\n".to_string(),
}],
status: codex_app_server_protocol::PatchApplyStatus::InProgress,
},
}),
)
.await
.expect("enqueue file change item");
let request = ServerRequest::FileChangeRequestApproval {
request_id: AppServerRequestId::Integer(9),
params: FileChangeRequestApprovalParams {
thread_id: thread_id.to_string(),
turn_id: "turn-approval".to_string(),
item_id: "patch-approval".to_string(),
reason: Some("command failed; retry without sandbox?".to_string()),
grant_root: None,
},
};
let request = app
.interactive_request_for_thread_request(thread_id, &request)
.await
.expect("expected file change approval request");
let ThreadInteractiveRequest::Approval(ApprovalRequest::ApplyPatch {
changes, reason, ..
}) = &request
else {
panic!("expected apply-patch approval request");
};
assert_eq!(
changes,
&HashMap::from([(
PathBuf::from("README.md"),
FileChange::Add {
content: "hello\n".to_string(),
},
)])
);
assert_eq!(
reason,
&Some("command failed; retry without sandbox?".to_string())
);
app.push_thread_interactive_request(request);
let cell = match app_event_rx.try_recv() {
Ok(AppEvent::InsertHistoryCell(cell)) => cell,
other => panic!("expected patch preview history cell, saw {other:?}"),
};
let rendered = lines_to_single_string(&cell.display_lines(/*width*/ 80));
assert!(rendered.contains("• Added README.md (+1 -0)"));
assert!(rendered.contains("1 +hello"));
}
#[tokio::test]
async fn inactive_thread_permissions_approval_preserves_file_system_permissions() {
let app = make_test_app().await;
codex-rs/tui/src/app/thread_events.rs
+48
View File
@@ -157,6 +157,40 @@ impl ThreadEventStore {
.collect()
}
pub(super) fn file_change_changes(
&self,
turn_id: &str,
item_id: &str,
) -> Option<Vec<codex_app_server_protocol::FileUpdateChange>> {
self.buffer
.iter()
.rev()
.find_map(|event| match event {
ThreadBufferedEvent::Notification(ServerNotification::ItemStarted(
notification,
)) if turn_id_matches(turn_id, &notification.turn_id) => {
file_change_item_changes(&notification.item, item_id)
}
ThreadBufferedEvent::Notification(ServerNotification::ItemCompleted(
notification,
)) if turn_id_matches(turn_id, &notification.turn_id) => {
file_change_item_changes(&notification.item, item_id)
}
ThreadBufferedEvent::Request(_)
| ThreadBufferedEvent::Notification(_)
| ThreadBufferedEvent::HistoryEntryResponse(_)
| ThreadBufferedEvent::FeedbackSubmission(_) => None,
})
.or_else(|| {
self.turns
.iter()
.rev()
.filter(|turn| turn_id_matches(turn_id, &turn.id))
.flat_map(|turn| turn.items.iter().rev())
.find_map(|item| file_change_item_changes(item, item_id))
})
}
pub(super) fn apply_thread_rollback(&mut self, response: &ThreadRollbackResponse) {
self.turns = response.thread.turns.clone();
self.buffer.clear();
@@ -231,6 +265,20 @@ impl ThreadEventStore {
}
}
fn turn_id_matches(request_turn_id: &str, candidate_turn_id: &str) -> bool {
request_turn_id.is_empty() || request_turn_id == candidate_turn_id
}
fn file_change_item_changes(
item: &ThreadItem,
item_id: &str,
) -> Option<Vec<codex_app_server_protocol::FileUpdateChange>> {
match item {
ThreadItem::FileChange { id, changes, .. } if id == item_id => Some(changes.clone()),
_ => None,
}
}
#[derive(Debug)]
pub(super) struct ThreadEventChannel {
pub(super) sender: mpsc::Sender<ThreadBufferedEvent>,
codex-rs/tui/src/app/thread_routing.rs
+34 -1
View File
@@ -194,6 +194,17 @@ impl App {
store.session.as_ref().map(|session| session.cwd.clone())
}
async fn thread_file_change_changes(
&self,
thread_id: ThreadId,
turn_id: &str,
item_id: &str,
) -> Option<Vec<codex_app_server_protocol::FileUpdateChange>> {
let channel = self.thread_event_channels.get(&thread_id)?;
let store = channel.store.lock().await;
store.file_change_changes(turn_id, item_id)
}
pub(super) async fn interactive_request_for_thread_request(
&self,
thread_id: ThreadId,
@@ -264,7 +275,11 @@ impl App {
.thread_cwd(thread_id)
.await
.unwrap_or_else(|| self.config.cwd.clone()),
changes: HashMap::new(),
changes: self
.thread_file_change_changes(thread_id, &params.turn_id, &params.item_id)
.await
.map(crate::app_server_approval_conversions::file_update_changes_to_core)
.unwrap_or_default(),
}),
),
ServerRequest::McpServerElicitationRequest { request_id, params } => {
@@ -311,6 +326,7 @@ impl App {
pub(super) fn push_thread_interactive_request(&mut self, request: ThreadInteractiveRequest) {
match request {
ThreadInteractiveRequest::Approval(request) => {
self.render_inactive_patch_preview(&request);
self.chat_widget.push_approval_request(request);
}
ThreadInteractiveRequest::McpServerElicitation(request) => {
@@ -320,6 +336,23 @@ impl App {
}
}
fn render_inactive_patch_preview(&mut self, request: &ApprovalRequest) {
let ApprovalRequest::ApplyPatch {
thread_label,
cwd,
changes,
..
} = request
else {
return;
};
if thread_label.is_none() || changes.is_empty() {
return;
}
self.chat_widget
.add_to_history(history_cell::new_patch_event(changes.clone(), cwd));
}
pub(super) async fn pending_inactive_thread_requests(&self) -> Vec<(ThreadId, ServerRequest)> {
let channels: Vec<(ThreadId, Arc<Mutex<ThreadEventStore>>)> = self
.thread_event_channels
codex-rs/tui/src/app_server_approval_conversions.rs
+51
View File
@@ -1,9 +1,14 @@
use codex_app_server_protocol::AdditionalNetworkPermissions;
use codex_app_server_protocol::FileUpdateChange;
use codex_app_server_protocol::GrantedPermissionProfile;
use codex_app_server_protocol::NetworkApprovalContext as AppServerNetworkApprovalContext;
use codex_app_server_protocol::PatchChangeKind;
use codex_protocol::protocol::FileChange;
use codex_protocol::protocol::NetworkApprovalContext;
use codex_protocol::protocol::NetworkApprovalProtocol;
use codex_protocol::request_permissions::RequestPermissionProfile as CoreRequestPermissionProfile;
use std::collections::HashMap;
use std::path::PathBuf;
pub(crate) fn network_approval_context_to_core(
value: AppServerNetworkApprovalContext,
@@ -38,21 +43,50 @@ pub(crate) fn granted_permission_profile_from_request(
}
}
pub(crate) fn file_update_changes_to_core(
changes: Vec<FileUpdateChange>,
) -> HashMap<PathBuf, FileChange> {
changes
.into_iter()
.map(|change| {
let path = PathBuf::from(change.path);
let file_change = match change.kind {
PatchChangeKind::Add => FileChange::Add {
content: change.diff,
},
PatchChangeKind::Delete => FileChange::Delete {
content: change.diff,
},
PatchChangeKind::Update { move_path } => FileChange::Update {
unified_diff: change.diff,
move_path,
},
};
(path, file_change)
})
.collect()
}
#[cfg(test)]
mod tests {
use super::file_update_changes_to_core;
use super::granted_permission_profile_from_request;
use super::network_approval_context_to_core;
use codex_app_server_protocol::FileUpdateChange;
use codex_app_server_protocol::PatchChangeKind;
use codex_protocol::models::FileSystemPermissions;
use codex_protocol::models::NetworkPermissions;
use codex_protocol::permissions::FileSystemAccessMode;
use codex_protocol::permissions::FileSystemPath;
use codex_protocol::permissions::FileSystemSandboxEntry;
use codex_protocol::permissions::FileSystemSpecialPath;
use codex_protocol::protocol::FileChange;
use codex_protocol::protocol::NetworkApprovalContext;
use codex_protocol::protocol::NetworkApprovalProtocol;
use codex_protocol::request_permissions::RequestPermissionProfile as CoreRequestPermissionProfile;
use codex_utils_absolute_path::AbsolutePathBuf;
use pretty_assertions::assert_eq;
use std::collections::HashMap;
use std::path::PathBuf;
fn absolute_path(path: &str) -> AbsolutePathBuf {
@@ -73,6 +107,23 @@ mod tests {
);
}
#[test]
fn converts_file_update_changes_to_core() {
assert_eq!(
file_update_changes_to_core(vec![FileUpdateChange {
path: "foo.txt".to_string(),
kind: PatchChangeKind::Add,
diff: "hello\n".to_string(),
}]),
HashMap::from([(
PathBuf::from("foo.txt"),
FileChange::Add {
content: "hello\n".to_string(),
},
)])
);
}
#[test]
fn converts_request_permissions_into_granted_permissions() {
assert_eq!(
codex-rs/tui/src/bottom_pane/approval_overlay.rs
+29 -6
View File
@@ -9,7 +9,6 @@ use crate::bottom_pane::CancellationEvent;
use crate::bottom_pane::list_selection_view::ListSelectionView;
use crate::bottom_pane::list_selection_view::SelectionItem;
use crate::bottom_pane::list_selection_view::SelectionViewParams;
use crate::diff_render::DiffSummary;
use crate::exec_command::strip_bash_lc_and_escape;
use crate::history_cell;
use crate::key_hint;
@@ -633,8 +632,6 @@ fn build_header(request: &ApprovalRequest) -> Box<dyn Renderable> {
ApprovalRequest::ApplyPatch {
thread_label,
reason,
cwd,
changes,
..
} => {
let mut header: Vec<Box<dyn Renderable>> = Vec::new();
@@ -643,11 +640,13 @@ fn build_header(request: &ApprovalRequest) -> Box<dyn Renderable> {
"Thread: ".into(),
thread_label.clone().bold(),
])));
header.push(Box::new(Line::from("")));
}
if let Some(reason) = reason
&& !reason.is_empty()
{
if !header.is_empty() {
header.push(Box::new(Line::from("")));
}
header.push(Box::new(
Paragraph::new(Line::from_iter([
"Reason: ".into(),
@@ -655,9 +654,7 @@ fn build_header(request: &ApprovalRequest) -> Box<dyn Renderable> {
]))
.wrap(Wrap { trim: false }),
));
header.push(Box::new(Line::from("")));
}
header.push(DiffSummary::new(changes.clone(), cwd.clone()).into());
Box::new(ColumnRenderable::with(header))
}
ApprovalRequest::McpElicitation {
@@ -1556,6 +1553,32 @@ mod tests {
);
}
#[test]
fn apply_patch_prompt_with_thread_label_omits_command_line() {
let (tx, _rx) = unbounded_channel::<AppEvent>();
let tx = AppEventSender::new(tx);
let mut changes = HashMap::new();
changes.insert(
PathBuf::from("bug1.txt"),
FileChange::Add {
content: "one\ntwo\nthree\n".to_string(),
},
);
let request = ApprovalRequest::ApplyPatch {
thread_id: ThreadId::new(),
thread_label: Some("Banach [worker]".to_string()),
id: "test".to_string(),
reason: None,
cwd: absolute_path("/tmp"),
changes,
};
let view = ApprovalOverlay::new(request, tx, Features::with_defaults());
let rendered = render_overlay_lines(&view, /*width*/ 120);
assert!(rendered.contains("Thread: Banach [worker]"));
assert!(rendered.contains("o to open thread"));
assert!(!rendered.contains("$ apply_patch"));
}
#[test]
fn network_exec_prompt_title_includes_host() {
let (tx, _rx) = unbounded_channel::<AppEvent>();
codex-rs/tui/src/chatwidget.rs
+3 -32
View File
@@ -47,6 +47,7 @@ use self::realtime::PendingSteerCompareKey;
use crate::app::app_server_requests::ResolvedAppServerRequest;
use crate::app_command::AppCommand;
use crate::app_event::RealtimeAudioDeviceKind;
use crate::app_server_approval_conversions::file_update_changes_to_core;
use crate::app_server_approval_conversions::network_approval_context_to_core;
use crate::app_server_session::ThreadSessionState;
#[cfg(not(target_os = "linux"))]
@@ -1774,36 +1775,6 @@ fn patch_approval_request_from_params(
}
}
fn app_server_patch_changes_to_core(
changes: Vec<codex_app_server_protocol::FileUpdateChange>,
) -> HashMap<PathBuf, codex_protocol::protocol::FileChange> {
changes
.into_iter()
.map(|change| {
let path = PathBuf::from(change.path);
let file_change = match change.kind {
codex_app_server_protocol::PatchChangeKind::Add => {
codex_protocol::protocol::FileChange::Add {
content: change.diff,
}
}
codex_app_server_protocol::PatchChangeKind::Delete => {
codex_protocol::protocol::FileChange::Delete {
content: change.diff,
}
}
codex_app_server_protocol::PatchChangeKind::Update { move_path } => {
codex_protocol::protocol::FileChange::Update {
unified_diff: change.diff,
move_path,
}
}
};
(path, file_change)
})
.collect()
}
fn app_server_collab_thread_id_to_core(thread_id: &str) -> Option<ThreadId> {
match ThreadId::from_string(thread_id) {
Ok(thread_id) => Some(thread_id),
@@ -6797,7 +6768,7 @@ impl ChatWidget {
status,
codex_app_server_protocol::PatchApplyStatus::Failed
),
changes: app_server_patch_changes_to_core(changes),
changes: file_update_changes_to_core(changes),
status: match status {
codex_app_server_protocol::PatchApplyStatus::Completed => {
codex_protocol::protocol::PatchApplyStatus::Completed
@@ -7356,7 +7327,7 @@ impl ChatWidget {
call_id: id,
turn_id: notification.turn_id,
auto_approved: false,
changes: app_server_patch_changes_to_core(changes),
changes: file_update_changes_to_core(changes),
});
}
ThreadItem::McpToolCall {
codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap
+2 -9
View File
@@ -1,18 +1,11 @@
---
source: tui/src/chatwidget/tests.rs
expression: terminal.backend().vt100().screen().contents()
source: tui/src/chatwidget/tests/exec_flow.rs
expression: contents
---
Would you like to make the following edits?
Reason: The model wants to apply changes
README.md (+2 -0)
1 +hello
2 +world
› 1. Yes, proceed (y)
2. Yes, and don't ask again for these files (a)
3. No, and tell Codex what to do differently (esc)
codex-rs/tui/src/chatwidget/tests/app_server.rs
-19
View File
@@ -349,25 +349,6 @@ async fn live_app_server_command_execution_strips_shell_wrapper() {
);
}
#[test]
fn app_server_patch_changes_to_core_preserves_diffs() {
let changes = app_server_patch_changes_to_core(vec![FileUpdateChange {
path: "foo.txt".to_string(),
kind: PatchChangeKind::Add,
diff: "hello\n".to_string(),
}]);
assert_eq!(
changes,
HashMap::from([(
PathBuf::from("foo.txt"),
FileChange::Add {
content: "hello\n".to_string(),
},
)])
);
}
#[tokio::test]
async fn live_app_server_collab_wait_items_render_history() {
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
codex-rs/tui/src/chatwidget/tests/exec_flow.rs
+12 -49
View File
@@ -1321,10 +1321,9 @@ async fn approval_modal_patch_snapshot() -> anyhow::Result<()> {
terminal
.draw(|f| chat.render(f.area(), f.buffer_mut()))
.expect("draw patch approval modal");
assert_chatwidget_snapshot!(
"approval_modal_patch",
terminal.backend().vt100().screen().contents()
);
let contents = terminal.backend().vt100().screen().contents();
assert!(!contents.contains("$ apply_patch"));
assert_chatwidget_snapshot!("approval_modal_patch", contents);
Ok(())
}
@@ -1469,28 +1468,11 @@ async fn apply_patch_events_emit_history_cells() {
id: "s1".into(),
msg: EventMsg::ApplyPatchApprovalRequest(ev),
});
let cells = drain_insert_history(&mut rx);
assert!(
cells.is_empty(),
drain_insert_history(&mut rx).is_empty(),
"expected approval request to surface via modal without emitting history cells"
);
let area = Rect::new(0, 0, 80, chat.desired_height(/*width*/ 80));
let mut buf = ratatui::buffer::Buffer::empty(area);
chat.render(area, &mut buf);
let mut saw_summary = false;
for y in 0..area.height {
let mut row = String::new();
for x in 0..area.width {
row.push(buf[(x, y)].symbol().chars().next().unwrap_or(' '));
}
if row.contains("foo.txt (+1 -0)") {
saw_summary = true;
break;
}
}
assert!(saw_summary, "expected approval modal to show diff summary");
// 2) Begin apply -> per-file apply block cell (no global header)
let mut changes2 = HashMap::new();
changes2.insert(
@@ -1820,7 +1802,7 @@ async fn apply_patch_untrusted_shows_approval_modal() -> anyhow::Result<()> {
}
#[tokio::test]
async fn apply_patch_request_shows_diff_summary() -> anyhow::Result<()> {
async fn apply_patch_request_omits_diff_summary_from_modal() -> anyhow::Result<()> {
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
// Ensure we are in OnRequest so an approval is surfaced
@@ -1849,43 +1831,24 @@ async fn apply_patch_request_shows_diff_summary() -> anyhow::Result<()> {
}),
});
// No history entries yet; the modal should contain the diff summary
let cells = drain_insert_history(&mut rx);
assert!(
cells.is_empty(),
drain_insert_history(&mut rx).is_empty(),
"expected approval request to render via modal instead of history"
);
let area = Rect::new(0, 0, 80, chat.desired_height(/*width*/ 80));
let mut buf = ratatui::buffer::Buffer::empty(area);
chat.render(area, &mut buf);
let mut saw_header = false;
let mut saw_line1 = false;
let mut saw_line2 = false;
let mut contents = String::new();
for y in 0..area.height {
let mut row = String::new();
for x in 0..area.width {
row.push(buf[(x, y)].symbol().chars().next().unwrap_or(' '));
}
if row.contains("README.md (+2 -0)") {
saw_header = true;
}
if row.contains("+line one") {
saw_line1 = true;
}
if row.contains("+line two") {
saw_line2 = true;
}
if saw_header && saw_line1 && saw_line2 {
break;
contents.push(buf[(x, y)].symbol().chars().next().unwrap_or(' '));
}
contents.push('\n');
}
assert!(saw_header, "expected modal to show diff header with totals");
assert!(
saw_line1 && saw_line2,
"expected modal to show per-line diff summary"
);
assert!(!contents.contains("README.md (+2 -0)"));
assert!(!contents.contains("+line one"));
assert!(!contents.contains("+line two"));
Ok(())
}