mirror of
https://github.com/openai/codex.git
synced 2026-10-20 17:38:45 +00:00
Allow naming forked chats from the TUI (#36036)
## What changed - Accept an optional thread name with `/fork`, trimming surrounding whitespace. - Apply the name to the newly forked thread and update the active session metadata. - Keep the successful fork active and show an error when naming it fails. ## Testing - Cover unnamed and named `/fork` dispatch, persisted names, and naming failures. GitOrigin-RevId: 1799576f82ef0856f992a0378f041e4bab89621b
This commit is contained in:
@@ -162,7 +162,7 @@ impl App {
|
||||
AppEvent::DeleteCurrentThread => {
|
||||
return Ok(self.delete_current_thread(app_server).await);
|
||||
}
|
||||
AppEvent::ForkCurrentSession => {
|
||||
AppEvent::ForkCurrentSession { name } => {
|
||||
self.session_telemetry.counter(
|
||||
"codex.thread.fork",
|
||||
/*inc*/ 1,
|
||||
@@ -184,7 +184,23 @@ impl App {
|
||||
fork_config.model_reasoning_effort =
|
||||
self.chat_widget.current_reasoning_effort();
|
||||
match app_server.fork_thread(fork_config, thread_id).await {
|
||||
Ok(forked) => {
|
||||
Ok(mut forked) => {
|
||||
let name_error = if let Some(name) = name {
|
||||
match app_server
|
||||
.thread_set_name(forked.session.thread_id, name.clone())
|
||||
.await
|
||||
{
|
||||
Ok(()) => {
|
||||
forked.session.thread_name = Some(name);
|
||||
None
|
||||
}
|
||||
Err(err) => {
|
||||
Some(format!("Failed to name the forked session: {err}"))
|
||||
}
|
||||
}
|
||||
} else {
|
||||
None
|
||||
};
|
||||
self.shutdown_current_thread(app_server).await;
|
||||
match self
|
||||
.replace_chat_widget_with_app_server_thread(
|
||||
@@ -196,6 +212,9 @@ impl App {
|
||||
.await
|
||||
{
|
||||
Ok(()) => {
|
||||
if let Some(err) = name_error {
|
||||
self.chat_widget.add_error_message(err);
|
||||
}
|
||||
if let Some(summary) = summary {
|
||||
let mut lines: Vec<Line<'static>> = Vec::new();
|
||||
if let Some(usage_line) = summary.usage_line {
|
||||
|
||||
@@ -6292,9 +6292,12 @@ async fn in_app_resume_uses_configured_or_explicit_cwd() -> Result<()> {
|
||||
));
|
||||
assert_eq!(app.chat_widget.thread_id(), Some(thread_id));
|
||||
|
||||
let control =
|
||||
Box::pin(app.handle_event(&mut tui, &mut app_server, AppEvent::ForkCurrentSession))
|
||||
.await?;
|
||||
let control = Box::pin(app.handle_event(
|
||||
&mut tui,
|
||||
&mut app_server,
|
||||
AppEvent::ForkCurrentSession { name: None },
|
||||
))
|
||||
.await?;
|
||||
|
||||
assert!(matches!(control, AppRunControl::Continue));
|
||||
assert!(!crate::session_resume::cwds_differ(
|
||||
|
||||
@@ -28,8 +28,12 @@ async fn fork_current_session_preserves_conversation_ultra() -> Result<()> {
|
||||
let mut tui = crate::tui::test_support::make_test_tui()?;
|
||||
let mut app_server = crate::start_embedded_app_server_for_picker(&app.config).await?;
|
||||
|
||||
let control =
|
||||
Box::pin(app.handle_event(&mut tui, &mut app_server, AppEvent::ForkCurrentSession)).await?;
|
||||
let control = Box::pin(app.handle_event(
|
||||
&mut tui,
|
||||
&mut app_server,
|
||||
AppEvent::ForkCurrentSession { name: None },
|
||||
))
|
||||
.await?;
|
||||
|
||||
assert!(matches!(control, AppRunControl::Continue));
|
||||
assert_ne!(app.chat_widget.thread_id(), Some(source_thread_id));
|
||||
|
||||
@@ -5,6 +5,7 @@ use app_test_support::rollout_path;
|
||||
use codex_app_server_protocol::ClientNotification;
|
||||
use codex_app_server_protocol::ClientRequest;
|
||||
use codex_app_server_protocol::JSONRPCError;
|
||||
use codex_app_server_protocol::JSONRPCErrorError;
|
||||
use codex_app_server_protocol::JSONRPCMessage;
|
||||
use codex_app_server_protocol::JSONRPCResponse;
|
||||
use codex_app_server_protocol::SortDirection;
|
||||
@@ -40,6 +41,7 @@ fn take_backfill_counts(requests: &Arc<Mutex<Vec<String>>>) -> (usize, usize) {
|
||||
async fn start_recording_app_server(
|
||||
config: &Config,
|
||||
mut blocked_thread_list: Option<(ThreadId, oneshot::Sender<()>, oneshot::Receiver<()>)>,
|
||||
failed_thread_name: Option<&'static str>,
|
||||
) -> Result<(
|
||||
AppServerSession,
|
||||
Arc<Mutex<Vec<String>>>,
|
||||
@@ -105,15 +107,31 @@ async fn start_recording_app_server(
|
||||
let _ = started.send(());
|
||||
let _ = release.await;
|
||||
}
|
||||
let response = match embedded.request(request).await? {
|
||||
Ok(result) => JSONRPCMessage::Response(JSONRPCResponse {
|
||||
let force_failure = matches!(
|
||||
&request,
|
||||
ClientRequest::ThreadSetName { params, .. }
|
||||
if failed_thread_name == Some(params.name.as_str())
|
||||
);
|
||||
let response = if force_failure {
|
||||
JSONRPCMessage::Error(JSONRPCError {
|
||||
id: request_id,
|
||||
result,
|
||||
}),
|
||||
Err(error) => JSONRPCMessage::Error(JSONRPCError {
|
||||
id: request_id,
|
||||
error,
|
||||
}),
|
||||
error: JSONRPCErrorError {
|
||||
code: -32603,
|
||||
message: "forced thread/name/set failure".to_string(),
|
||||
data: None,
|
||||
},
|
||||
})
|
||||
} else {
|
||||
match embedded.request(request).await? {
|
||||
Ok(result) => JSONRPCMessage::Response(JSONRPCResponse {
|
||||
id: request_id,
|
||||
result,
|
||||
}),
|
||||
Err(error) => JSONRPCMessage::Error(JSONRPCError {
|
||||
id: request_id,
|
||||
error,
|
||||
}),
|
||||
}
|
||||
};
|
||||
websocket
|
||||
.send(Message::Text(serde_json::to_string(&response)?.into()))
|
||||
@@ -166,8 +184,12 @@ fn fresh_session_applies_requested_name() -> Result<()> {
|
||||
let codex_home = tempdir()?;
|
||||
app.config.codex_home = codex_home.path().to_path_buf().abs();
|
||||
app.config.sqlite = SqliteConfig::new_for_testing(codex_home.path().abs());
|
||||
let (mut app_server, requests, proxy) =
|
||||
start_recording_app_server(&app.config, /*blocked_thread_list*/ None).await?;
|
||||
let (mut app_server, requests, proxy) = start_recording_app_server(
|
||||
&app.config,
|
||||
/*blocked_thread_list*/ None,
|
||||
/*failed_thread_name*/ None,
|
||||
)
|
||||
.await?;
|
||||
let mut tui = crate::tui::test_support::make_test_tui()?;
|
||||
|
||||
app.start_fresh_session_with_summary_hint(
|
||||
@@ -267,6 +289,7 @@ fn session_lifecycle_avoids_redundant_subagent_metadata_reads() -> Result<()> {
|
||||
let (mut app_server, requests, proxy) = start_recording_app_server(
|
||||
&app.config,
|
||||
Some((root_thread_id, started_tx, release_rx)),
|
||||
Some("Failed Fork"),
|
||||
)
|
||||
.await?;
|
||||
let root = app_server
|
||||
@@ -291,15 +314,59 @@ fn session_lifecycle_avoids_redundant_subagent_metadata_reads() -> Result<()> {
|
||||
let control = Box::pin(app.handle_event(
|
||||
&mut tui,
|
||||
&mut app_server,
|
||||
AppEvent::ForkCurrentSession,
|
||||
AppEvent::ForkCurrentSession {
|
||||
name: Some("Add User Fork".to_string()),
|
||||
},
|
||||
))
|
||||
.await?;
|
||||
|
||||
assert!(matches!(control, AppRunControl::Continue));
|
||||
assert_ne!(app.chat_widget.thread_id(), Some(root_thread_id));
|
||||
let named_fork_id = app
|
||||
.chat_widget
|
||||
.thread_id()
|
||||
.expect("named fork should have a thread id");
|
||||
assert_eq!(
|
||||
app.chat_widget.thread_name(),
|
||||
Some("Add User Fork".to_string())
|
||||
);
|
||||
// Forking may read the source metadata once when the response includes its parent
|
||||
// id. It must not scan or backfill loaded threads for the newly created fork.
|
||||
assert!(matches!(take_backfill_counts(&requests), (0, 0) | (0, 1)));
|
||||
let named_fork = app_server
|
||||
.thread_read(named_fork_id, /*include_turns*/ false)
|
||||
.await?;
|
||||
assert_eq!(named_fork.name.as_deref(), Some("Add User Fork"));
|
||||
take_backfill_counts(&requests);
|
||||
|
||||
let control = Box::pin(app.handle_event(
|
||||
&mut tui,
|
||||
&mut app_server,
|
||||
AppEvent::ForkCurrentSession {
|
||||
name: Some("Failed Fork".to_string()),
|
||||
},
|
||||
))
|
||||
.await?;
|
||||
|
||||
assert!(matches!(control, AppRunControl::Continue));
|
||||
assert_ne!(app.chat_widget.thread_id(), Some(named_fork_id));
|
||||
let name_error = std::iter::from_fn(|| app_event_rx.try_recv().ok())
|
||||
.find_map(|event| match event {
|
||||
AppEvent::InsertHistoryCell(cell) => {
|
||||
let rendered =
|
||||
lines_to_single_string(&cell.display_lines(/*width*/ 80));
|
||||
rendered
|
||||
.contains("Failed to name the forked session")
|
||||
.then_some(rendered)
|
||||
}
|
||||
_ => None,
|
||||
})
|
||||
.expect("fork naming error history cell");
|
||||
insta::assert_snapshot!(
|
||||
name_error,
|
||||
@"■ Failed to name the forked session: thread/name/set failed in TUI"
|
||||
);
|
||||
assert!(matches!(take_backfill_counts(&requests), (0, 0) | (0, 1)));
|
||||
|
||||
app.start_fresh_session_with_summary_hint(
|
||||
&mut tui,
|
||||
|
||||
@@ -287,8 +287,10 @@ pub(crate) enum AppEvent {
|
||||
/// Permanently delete the current active main thread and exit after it succeeds.
|
||||
DeleteCurrentThread,
|
||||
|
||||
/// Fork the current session into a new thread.
|
||||
ForkCurrentSession,
|
||||
/// Fork the current session into a new thread, optionally assigning it a name.
|
||||
ForkCurrentSession {
|
||||
name: Option<String>,
|
||||
},
|
||||
|
||||
/// Branch before a selected prompt and reopen it in the new thread's composer.
|
||||
ForkSessionForPromptEdit {
|
||||
|
||||
@@ -242,7 +242,8 @@ impl ChatWidget {
|
||||
self.app_event_tx.send(AppEvent::OpenResumePicker);
|
||||
}
|
||||
SlashCommand::Fork => {
|
||||
self.app_event_tx.send(AppEvent::ForkCurrentSession);
|
||||
self.app_event_tx
|
||||
.send(AppEvent::ForkCurrentSession { name: None });
|
||||
}
|
||||
SlashCommand::App => {
|
||||
let Some(thread_id) = self.thread_id else {
|
||||
@@ -744,6 +745,11 @@ impl ChatWidget {
|
||||
name: Some(trimmed.to_string()),
|
||||
});
|
||||
}
|
||||
SlashCommand::Fork if !trimmed.is_empty() => {
|
||||
self.app_event_tx.send(AppEvent::ForkCurrentSession {
|
||||
name: Some(trimmed.to_string()),
|
||||
});
|
||||
}
|
||||
SlashCommand::Plan if !trimmed.is_empty() => {
|
||||
if !self.apply_plan_slash_command() {
|
||||
return;
|
||||
|
||||
@@ -2547,7 +2547,26 @@ async fn slash_fork_requests_current_fork() {
|
||||
|
||||
chat.dispatch_command(SlashCommand::Fork);
|
||||
|
||||
assert_matches!(rx.try_recv(), Ok(AppEvent::ForkCurrentSession));
|
||||
assert_matches!(
|
||||
rx.try_recv(),
|
||||
Ok(AppEvent::ForkCurrentSession { name: None })
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn slash_fork_with_name_requests_named_fork() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
chat.bottom_pane
|
||||
.set_composer_text("/fork Add User ".to_string(), Vec::new(), Vec::new());
|
||||
|
||||
chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
|
||||
|
||||
assert_matches!(
|
||||
rx.try_recv(),
|
||||
Ok(AppEvent::ForkCurrentSession {
|
||||
name: Some(name)
|
||||
}) if name == "Add User"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -157,6 +157,7 @@ impl SlashCommand {
|
||||
| SlashCommand::Rename
|
||||
| SlashCommand::New
|
||||
| SlashCommand::Clear
|
||||
| SlashCommand::Fork
|
||||
| SlashCommand::Plan
|
||||
| SlashCommand::Goal
|
||||
| SlashCommand::Ide
|
||||
|
||||
Reference in New Issue
Block a user