mirror of
https://github.com/openai/codex.git
synced 2026-10-20 00:18:49 +00:00
Populate the agents overview from background tool responses (#46946)
## Why Tool responses can arrive without a `thread/started` notification, leaving background threads missing from the agents overview. ## What changed Register metadata returned by background thread creation, forking, and resumption as a fallback, then refresh and repaint the overview. Clear turn history before registration and exclude ephemeral threads. Preserve existing metadata and track archived or deleted threads so delayed tool responses cannot restore removed rows. Clear that removal tracking when a thread starts or is unarchived. ## Testing Extend tests to cover tool-created and forked thread registration, missing start notifications, and delayed responses alongside metadata refreshes and lifecycle removals. GitOrigin-RevId: a6f892f1c839dc69fac24b00c34927a7c5f8ce84
This commit is contained in:
@@ -34,6 +34,8 @@ pub(crate) const AGENTS_OVERVIEW_VIEW_ID: &str = "agents-overview";
|
||||
pub(super) struct AgentsOverviewState {
|
||||
/// Missing metadata records a local resume until the next metadata refresh.
|
||||
pub(super) threads: HashMap<ThreadId, Option<Thread>>,
|
||||
/// Lifecycle removals take precedence over delayed tool registration responses.
|
||||
pub(super) removed_threads: HashSet<ThreadId>,
|
||||
/// Local visibility only; activity and metadata refreshes never reveal hidden roots.
|
||||
pub(super) hidden_threads: HashSet<ThreadId>,
|
||||
pub(super) last_messages: HashMap<ThreadId, String>,
|
||||
|
||||
@@ -285,7 +285,8 @@ fn overview_thread(
|
||||
#[tokio::test]
|
||||
async fn shared_overview_keeps_rows_and_replays_changes_over_stale_reads() -> Result<()> {
|
||||
let mut app = make_test_app().await;
|
||||
let app_server = crate::start_embedded_app_server_for_picker(&app.config).await?;
|
||||
let mut app_server = crate::start_embedded_app_server_for_picker(&app.config).await?;
|
||||
let mut tui = crate::tui::test_support::make_test_tui()?;
|
||||
let [retained, archived, deleted, created] = std::array::from_fn(|_| ThreadId::new());
|
||||
let [
|
||||
mut retained_thread,
|
||||
@@ -305,9 +306,6 @@ async fn shared_overview_keeps_rows_and_replays_changes_over_stale_reads() -> Re
|
||||
app.agents_overview.request_id = Some(request_id);
|
||||
// The seed is still in flight and the command center has never been opened.
|
||||
for notification in [
|
||||
ServerNotification::ThreadStarted(ThreadStartedNotification {
|
||||
thread: created_thread.clone(),
|
||||
}),
|
||||
ServerNotification::ThreadNameUpdated(ThreadNameUpdatedNotification {
|
||||
thread_id: retained.to_string(),
|
||||
thread_name: Some("New name".to_string()),
|
||||
@@ -328,6 +326,35 @@ async fn shared_overview_keeps_rows_and_replays_changes_over_stale_reads() -> Re
|
||||
)
|
||||
.await;
|
||||
}
|
||||
// Tool responses can arrive without thread/started or after newer updates/removals.
|
||||
for thread in [
|
||||
retained_thread.clone(),
|
||||
created_thread.clone(),
|
||||
archived_thread.clone(),
|
||||
deleted_thread.clone(),
|
||||
] {
|
||||
let (registered, registration) = tokio::sync::oneshot::channel();
|
||||
Box::pin(app.handle_event(
|
||||
&mut tui,
|
||||
&mut app_server,
|
||||
AppEvent::DynamicToolThreadStarted {
|
||||
thread,
|
||||
task_tools_available: false,
|
||||
registered,
|
||||
},
|
||||
))
|
||||
.await?;
|
||||
registration.await?;
|
||||
}
|
||||
assert_eq!(
|
||||
app.agents_overview.threads,
|
||||
HashMap::from([
|
||||
(retained, Some(retained_thread.clone())),
|
||||
(created, Some(created_thread.clone())),
|
||||
])
|
||||
);
|
||||
// A successful read supersedes registration; queued notifications supersede the read.
|
||||
retained_thread.preview = "Fresh preview from the read".to_string();
|
||||
app.apply_agents_overview_thread_refresh(
|
||||
&app_server,
|
||||
request_id,
|
||||
|
||||
@@ -74,11 +74,13 @@ impl App {
|
||||
if started.thread.ephemeral {
|
||||
return;
|
||||
}
|
||||
self.agents_overview.removed_threads.remove(&thread_id);
|
||||
let mut thread = started.thread.clone();
|
||||
thread.turns.clear();
|
||||
self.agents_overview.threads.insert(thread_id, Some(thread));
|
||||
}
|
||||
ServerNotification::ThreadArchived(_) | ServerNotification::ThreadDeleted(_) => {
|
||||
self.agents_overview.removed_threads.insert(thread_id);
|
||||
self.agents_overview
|
||||
.selected_permission_profiles
|
||||
.remove(&thread_id);
|
||||
@@ -88,6 +90,9 @@ impl App {
|
||||
self.agents_overview.threads.remove(&thread_id);
|
||||
self.agents_overview.refresh_thread_ids.remove(&thread_id);
|
||||
}
|
||||
ServerNotification::ThreadUnarchived(_) => {
|
||||
self.agents_overview.removed_threads.remove(&thread_id);
|
||||
}
|
||||
ServerNotification::ThreadClosed(_) => {
|
||||
self.agents_overview.activity.remove(&thread_id);
|
||||
if let Some(usage) = self.agents_overview.usage.get_mut(&thread_id) {
|
||||
|
||||
@@ -248,10 +248,13 @@ impl App {
|
||||
.await?;
|
||||
}
|
||||
AppEvent::DynamicToolThreadStarted {
|
||||
thread_id,
|
||||
thread,
|
||||
task_tools_available,
|
||||
registered,
|
||||
} => {
|
||||
let Ok(thread_id) = ThreadId::from_string(&thread.id) else {
|
||||
return Ok(AppRunControl::Continue);
|
||||
};
|
||||
self.agents_overview
|
||||
.dispatched_requests
|
||||
.entry(thread_id)
|
||||
@@ -259,6 +262,15 @@ impl App {
|
||||
if task_tools_available {
|
||||
app_server.remember_task_tool_thread(thread_id);
|
||||
}
|
||||
// Fallback metadata must not replay after fresh reads or newer notifications.
|
||||
if !thread.ephemeral
|
||||
&& !self.agents_overview.removed_threads.contains(&thread_id)
|
||||
&& !self.agents_overview.threads.get(&thread_id).is_some_and(Option::is_some) {
|
||||
self.agents_overview.threads.insert(thread_id, Some(thread));
|
||||
self.agents_overview.refresh_thread_ids.insert(thread_id);
|
||||
}
|
||||
self.refresh_changed_agents_overview_threads(app_server);
|
||||
self.repaint_agents_overview();
|
||||
let _ = registered.send(());
|
||||
}
|
||||
AppEvent::DynamicToolCallCompleted {
|
||||
|
||||
@@ -1314,13 +1314,14 @@ async fn local_daemon_registers_approval_gated_mcp_tools_for_both_start_paths()
|
||||
}
|
||||
};
|
||||
let AppEvent::DynamicToolThreadStarted {
|
||||
thread_id: child_thread_id,
|
||||
thread,
|
||||
task_tools_available,
|
||||
registered,
|
||||
} = registration
|
||||
else {
|
||||
panic!("expected the MCP-created task to register")
|
||||
};
|
||||
let child_thread_id = ThreadId::from_string(&thread.id)?;
|
||||
assert!(task_tools_available);
|
||||
assert!(registered.send(()).is_ok());
|
||||
let created = creation.await??;
|
||||
@@ -1335,14 +1336,34 @@ async fn local_daemon_registers_approval_gated_mcp_tools_for_both_start_paths()
|
||||
child["config"]["mcp_servers.codex_tui"],
|
||||
starts[0]["config"]["mcp_servers.codex_tui"]
|
||||
);
|
||||
let forked = call_tool(
|
||||
3,
|
||||
"fork_thread",
|
||||
serde_json::json!({"threadId": delegation_source}),
|
||||
)
|
||||
.send()
|
||||
.await?;
|
||||
// Fork another task: this synthetic MCP call has no active turn to cut before.
|
||||
let fork_source =
|
||||
create_history_rollout(&app.config, ThreadHistoryMode::Legacy, "Task to fork")?;
|
||||
let forked = tokio::spawn(
|
||||
call_tool(
|
||||
3,
|
||||
"fork_thread",
|
||||
serde_json::json!({"threadId": fork_source}),
|
||||
)
|
||||
.send(),
|
||||
);
|
||||
let registration = tokio::time::timeout(Duration::from_secs(/*secs*/ 5), events.recv())
|
||||
.await?
|
||||
.expect("fork registration event");
|
||||
let AppEvent::DynamicToolThreadStarted { thread, .. } = ®istration else {
|
||||
panic!("expected the MCP fork to register")
|
||||
};
|
||||
let forked_thread_id = ThreadId::from_string(&thread.id)?;
|
||||
let expected_thread = thread.clone();
|
||||
let mut tui = crate::tui::test_support::make_test_tui()?;
|
||||
Box::pin(app.handle_event(&mut tui, &mut app_server, registration)).await?;
|
||||
assert_eq!(
|
||||
app.agents_overview.threads[&forked_thread_id],
|
||||
Some(expected_thread)
|
||||
);
|
||||
let forked = forked.await??;
|
||||
assert!(forked.status().is_success());
|
||||
assert!(forked.text().await?.contains(&forked_thread_id.to_string()));
|
||||
let forked = recorded_params(&requests, "thread/fork")
|
||||
.pop()
|
||||
.expect("MCP-created fork request");
|
||||
@@ -1763,7 +1784,7 @@ async fn dynamic_tool_requests_ignore_other_namespaces_and_dispatch_tui_namespac
|
||||
.await?
|
||||
.expect("background task registration event");
|
||||
let AppEvent::DynamicToolThreadStarted {
|
||||
thread_id: created_thread_id,
|
||||
thread,
|
||||
task_tools_available,
|
||||
registered,
|
||||
} = registration
|
||||
@@ -1771,16 +1792,22 @@ async fn dynamic_tool_requests_ignore_other_namespaces_and_dispatch_tui_namespac
|
||||
panic!("expected background task registration before its first turn: {registration:?}")
|
||||
};
|
||||
assert!(recorded_params(&requests, "turn/start").is_empty());
|
||||
let created_thread_id = ThreadId::from_string(&thread.id)?;
|
||||
let expected_thread = thread.clone();
|
||||
Box::pin(app.handle_event(
|
||||
&mut tui,
|
||||
&mut app_server,
|
||||
AppEvent::DynamicToolThreadStarted {
|
||||
thread_id: created_thread_id,
|
||||
thread,
|
||||
task_tools_available,
|
||||
registered,
|
||||
},
|
||||
))
|
||||
.await?;
|
||||
assert_eq!(
|
||||
app.agents_overview.threads[&created_thread_id],
|
||||
Some(expected_thread)
|
||||
);
|
||||
assert!(
|
||||
app.agents_overview
|
||||
.dispatched_requests
|
||||
@@ -1848,7 +1875,7 @@ async fn dynamic_tool_requests_ignore_other_namespaces_and_dispatch_tui_namespac
|
||||
},
|
||||
);
|
||||
let AppEvent::DynamicToolThreadStarted {
|
||||
thread_id: continued_thread_id,
|
||||
thread,
|
||||
task_tools_available,
|
||||
registered,
|
||||
} = tokio::time::timeout(std::time::Duration::from_secs(/*secs*/ 5), events.recv())
|
||||
@@ -1857,13 +1884,13 @@ async fn dynamic_tool_requests_ignore_other_namespaces_and_dispatch_tui_namespac
|
||||
else {
|
||||
panic!("expected follow-up task registration before its next turn")
|
||||
};
|
||||
assert_eq!(continued_thread_id, creation_source);
|
||||
assert_eq!(ThreadId::from_string(&thread.id)?, creation_source);
|
||||
assert_eq!(recorded_params(&requests, "turn/start").len(), 1);
|
||||
Box::pin(app.handle_event(
|
||||
&mut tui,
|
||||
&mut app_server,
|
||||
AppEvent::DynamicToolThreadStarted {
|
||||
thread_id: continued_thread_id,
|
||||
thread,
|
||||
task_tools_available,
|
||||
registered,
|
||||
},
|
||||
|
||||
@@ -518,9 +518,9 @@ pub(crate) enum AppEvent {
|
||||
result: color_eyre::Result<AppServerStartedThread>,
|
||||
},
|
||||
|
||||
/// Register a dynamically created background thread before its first turn starts.
|
||||
/// Register a tool-created or resumed background thread and its overview metadata.
|
||||
DynamicToolThreadStarted {
|
||||
thread_id: ThreadId,
|
||||
thread: Thread,
|
||||
task_tools_available: bool,
|
||||
registered: tokio::sync::oneshot::Sender<()>,
|
||||
},
|
||||
|
||||
@@ -643,8 +643,8 @@ async fn execute_inner(
|
||||
)
|
||||
.await
|
||||
.map_err(|error| error.to_string())?;
|
||||
let thread_id = started.thread.id;
|
||||
register_background_thread(app_event_tx, &thread_id, task_tools_available).await?;
|
||||
let thread_id = started.thread.id.clone();
|
||||
register_background_thread(app_event_tx, started.thread, task_tools_available).await?;
|
||||
if let Some(title) = arguments.title
|
||||
&& let Err(error) = request::<ThreadSetNameResponse>(&handle, |request_id| {
|
||||
ClientRequest::ThreadSetName {
|
||||
@@ -735,10 +735,17 @@ async fn execute_inner(
|
||||
},
|
||||
)
|
||||
.await?;
|
||||
let forked_thread_id = response.thread.id.clone();
|
||||
register_background_thread(
|
||||
app_event_tx,
|
||||
response.thread,
|
||||
/*task_tools_available*/ false,
|
||||
)
|
||||
.await?;
|
||||
Ok(json!({
|
||||
"environment": {"type": "same-directory"},
|
||||
"sourceThreadId": thread_id,
|
||||
"threadId": response.thread.id,
|
||||
"threadId": forked_thread_id,
|
||||
"continuation": "The fork contains completed history only. If the source thread was running, the active turn and unfinished response are not in the child. Send a follow-up message to threadId only if the task requires work to continue there."
|
||||
}))
|
||||
}
|
||||
@@ -752,7 +759,7 @@ async fn execute_inner(
|
||||
validate_prompt(&prompt, MAX_DELEGATED_INPUT_BYTES)?;
|
||||
let thread = read_thread(&handle, &arguments.thread_id).await?;
|
||||
let exclude_turns = thread.history_mode == ThreadHistoryMode::Paginated;
|
||||
let _: ThreadResumeResponse = request_with_history_fallback(
|
||||
let resumed: ThreadResumeResponse = request_with_history_fallback(
|
||||
&handle,
|
||||
exclude_turns,
|
||||
|request_id, exclude_turns| ClientRequest::ThreadResume {
|
||||
@@ -768,7 +775,7 @@ async fn execute_inner(
|
||||
.await?;
|
||||
register_background_thread(
|
||||
app_event_tx,
|
||||
&arguments.thread_id,
|
||||
resumed.thread,
|
||||
/*task_tools_available*/ false,
|
||||
)
|
||||
.await?;
|
||||
@@ -1209,13 +1216,14 @@ fn same_thread_id(first: &str, second: &str) -> bool {
|
||||
|
||||
async fn register_background_thread(
|
||||
app_event_tx: Option<&AppEventSender>,
|
||||
thread_id: &str,
|
||||
mut thread: Thread,
|
||||
task_tools_available: bool,
|
||||
) -> Result<(), String> {
|
||||
if let Some(app_event_tx) = app_event_tx {
|
||||
thread.turns.clear();
|
||||
let (registered, registration) = tokio::sync::oneshot::channel();
|
||||
app_event_tx.send(AppEvent::DynamicToolThreadStarted {
|
||||
thread_id: ThreadId::from_string(thread_id).map_err(|error| error.to_string())?,
|
||||
thread,
|
||||
task_tools_available,
|
||||
registered,
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user