openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-09-05 15:18:41 +00:00
Code Issues Packages Projects Releases Wiki Activity

codex: address PR review feedback (#26754)

Browse Source
This commit is contained in:
Eric Traut
2026-06-06 17:16:58 -07:00
parent e5b9dc45d0
commit 3e1be002ca
6 changed files with 113 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files

61
codex-rs/tui/src/app/side.rs
View File

@@ -203,6 +203,7 @@ pub(super) struct PendingSideStart {
}
impl SideThreadState {
#[cfg(test)]
pub(super) fn new(parent_thread_id: ThreadId) -> Self {
Self {
parent_thread_id,
@@ -571,6 +572,8 @@ impl App {
parent_thread_id,
user_message,
});
self.refresh_in_memory_config_from_disk_best_effort("starting a side conversation")
.await;
let request_handle = app_server.request_handle();
let thread_params_mode = app_server.thread_params_mode();
let remote_cwd_override = app_server.remote_cwd_override().map(Path::to_path_buf);
@@ -587,8 +590,7 @@ impl App {
thread_params_mode,
remote_cwd_override,
)
.await
.map_err(|err| format!("{err:#}"));
.await;
app_event_tx.send(AppEvent::SideThreadPrepared { request_id, result });
});
}
@@ -598,7 +600,10 @@ impl App {
tui: &mut tui::Tui,
app_server: &mut AppServerSession,
request_id: Uuid,
result: std::result::Result<AppServerStartedThread, String>,
result: std::result::Result<
AppServerStartedThread,
crate::app_event::SideThreadPrepareError,
>,
) -> Result<()> {
let Some(PendingSideStart {
parent_thread_id,
@@ -606,21 +611,25 @@ impl App {
..
}) = self.take_pending_side_start(request_id)
else {
if let Ok(started) = result {
let thread_id = started.session.thread_id;
if let Some(thread_id) = match &result {
Ok(started) => Some(started.session.thread_id),
Err(err) => err.thread_id,
} {
// ThreadStarted may have created local state before this preparation was canceled.
self.discard_thread_local_state(thread_id).await;
let request_handle = app_server.request_handle();
tokio::spawn(async move {
if let Err(err) =
super::side_server::cleanup_side_thread(request_handle, thread_id).await
{
tracing::warn!(
thread_id = %thread_id,
"failed to clean up abandoned side thread: {err}"
);
}
});
if result.is_ok() {
let request_handle = app_server.request_handle();
tokio::spawn(async move {
if let Err(err) =
super::side_server::cleanup_side_thread(request_handle, thread_id).await
{
tracing::warn!(
thread_id = %thread_id,
"failed to clean up abandoned side thread: {err}"
);
}
});
}
}
return Ok(());
};
@@ -633,8 +642,19 @@ impl App {
let mut store = channel.store.lock().await;
Self::install_side_thread_snapshot(&mut store, started.session, started.turns);
}
self.side_threads
.insert(child_thread_id, SideThreadState::new(parent_thread_id));
let parent_status =
if let Some(channel) = self.thread_event_channels.get(&parent_thread_id) {
channel.store.lock().await.side_parent_status()
} else {
None
};
self.side_threads.insert(
child_thread_id,
SideThreadState {
parent_thread_id,
parent_status,
},
);
self.upsert_agent_picker_thread(
child_thread_id,
/*agent_nickname*/ None,
@@ -683,11 +703,14 @@ impl App {
}
}
Err(err) => {
if let Some(thread_id) = err.thread_id {
self.discard_thread_local_state(thread_id).await;
}
self.restore_side_user_message(user_message.take());
self.chat_widget
.set_side_conversation_context_label(/*label*/ None);
self.chat_widget
.add_error_message(Self::side_start_error_message(&err));
.add_error_message(Self::side_start_error_message(&err.message));
}
}
Ok(())

24
codex-rs/tui/src/app/side_server.rs
View File

@@ -1,4 +1,5 @@
use super::*;
use crate::app_event::SideThreadPrepareError;
use crate::app_server_session::ThreadParamsMode;
use codex_app_server_protocol::RequestId;
use codex_app_server_protocol::ThreadInjectItemsParams;
@@ -14,7 +15,13 @@ pub(super) async fn prepare_side_thread(
parent_thread_id: ThreadId,
thread_params_mode: ThreadParamsMode,
remote_cwd_override: Option<PathBuf>,
) -> Result<AppServerStartedThread> {
) -> std::result::Result<AppServerStartedThread, SideThreadPrepareError> {
let boundary_item = serde_json::to_value(App::side_boundary_prompt_item()).map_err(|err| {
SideThreadPrepareError {
thread_id: None,
message: format!("failed to encode thread/inject_items payload: {err}"),
}
})?;
let started = crate::app_server_session::fork_thread_with_request_handle(
request_handle.clone(),
config,
@@ -22,10 +29,12 @@ pub(super) async fn prepare_side_thread(
thread_params_mode,
remote_cwd_override,
)
.await?;
.await
.map_err(|err| SideThreadPrepareError {
thread_id: None,
message: format!("{err:#}"),
})?;
let child_thread_id = started.session.thread_id;
let boundary_item = serde_json::to_value(App::side_boundary_prompt_item())
.wrap_err("failed to encode thread/inject_items payload")?;
// Keep fork and boundary injection in one background operation so the App never observes a
// side thread that can run before its inherited history is marked reference-only.
@@ -46,7 +55,12 @@ pub(super) async fn prepare_side_thread(
"failed to clean up side thread after inject failure: {cleanup_err}"
);
}
return Err(err).wrap_err("thread/inject_items failed during TUI side conversation setup");
return Err(SideThreadPrepareError {
thread_id: Some(child_thread_id),
message: format!(
"thread/inject_items failed during TUI side conversation setup: {err}"
),
});
}
Ok(started)
}

33
codex-rs/tui/src/app/thread_events.rs
View File

@@ -240,7 +240,7 @@ impl ThreadEventStore {
.has_pending_thread_approvals()
}
pub(super) fn side_parent_pending_status(&self) -> Option<SideParentStatus> {
pub(super) fn side_parent_status(&self) -> Option<SideParentStatus> {
if self
.pending_interactive_replay
.has_pending_thread_user_input()
@@ -252,7 +252,20 @@ impl ThreadEventStore {
{
Some(SideParentStatus::NeedsApproval)
} else {
None
self.buffer
.iter()
.rev()
.find_map(|event| {
let ThreadBufferedEvent::Notification(notification) = event else {
return None;
};
match SideParentStatusChange::for_notification(notification) {
Some(SideParentStatusChange::Set(status)) => Some(Some(status)),
Some(SideParentStatusChange::Clear) => Some(None),
Some(SideParentStatusChange::ClearActionable) | None => None,
}
})
.flatten()
}
}
@@ -507,6 +520,22 @@ mod tests {
assert_eq!(store.active_turn_id(), None);
}
#[test]
fn thread_event_store_reports_latest_side_parent_status() {
let mut store = ThreadEventStore::new(/*capacity*/ 8);
let thread_id = ThreadId::new();
store.push_notification(turn_completed_notification(
thread_id,
"turn-1",
TurnStatus::Failed,
));
assert_eq!(store.side_parent_status(), Some(SideParentStatus::Failed));
store.push_notification(turn_started_notification(thread_id, "turn-2"));
assert_eq!(store.side_parent_status(), None);
}
#[test]
fn thread_event_store_restores_active_turn_from_snapshot_turns() {
let thread_id = ThreadId::new();

6
codex-rs/tui/src/app/thread_routing.rs
View File

@@ -817,7 +817,7 @@ impl App {
};
let status = {
let store = channel.store.lock().await;
store.side_parent_pending_status()
store.side_parent_status()
};
if let Some(status) = status {
self.set_side_parent_status(thread_id, Some(status));
@@ -858,7 +858,7 @@ impl App {
guard.session = Some(session);
}
guard.push_notification(notification.clone());
(guard.active, guard.side_parent_pending_status())
(guard.active, guard.side_parent_status())
};
let notification_status_change = SideParentStatusChange::for_notification(&notification);
@@ -976,7 +976,7 @@ impl App {
let (should_send, pending_status) = {
let mut guard = store.lock().await;
guard.push_request(request.clone());
(guard.active, guard.side_parent_pending_status())
(guard.active, guard.side_parent_status())
};
let request_status = SideParentStatus::for_request(&request);

8
codex-rs/tui/src/app_event.rs
View File

@@ -52,6 +52,12 @@ use uuid::Uuid;
use crate::history_cell::HistoryCell;
#[derive(Debug)]
pub(crate) struct SideThreadPrepareError {
pub(crate) thread_id: Option<ThreadId>,
pub(crate) message: String,
}
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub(crate) enum RealtimeAudioDeviceKind {
Microphone,
@@ -150,7 +156,7 @@ pub(crate) enum AppEvent {
/// Finish preparing a transient side conversation off the TUI event loop.
SideThreadPrepared {
request_id: Uuid,
result: Result<AppServerStartedThread, String>,
result: Result<AppServerStartedThread, SideThreadPrepareError>,
},
/// Submit an op to the specified thread, regardless of current focus.

11
codex-rs/tui/src/session_log.rs
View File

@@ -197,6 +197,17 @@ pub(crate) fn log_inbound_app_event(event: &AppEvent) {
});
LOGGER.write_json_line(value);
}
AppEvent::SideThreadPrepared { request_id, result } => {
let value = json!({
"ts": now_ts(),
"dir": "to_tui",
"kind": "app_event",
"variant": "SideThreadPrepared",
"request_id": request_id,
"ok": result.is_ok(),
});
LOGGER.write_json_line(value);
}
// Noise or control flow – record variant only
other => {
let value = json!({