mirror of
https://github.com/openai/codex.git
synced 2026-11-13 21:30:47 +00:00
Title saved side chats from their first direct input (#61343)
## Why Saved side chats can inherit their parent's name, leaving them labeled for the parent conversation rather than their own question. ## What changed Generate an automatic title from the first successfully submitted direct text input in a newly saved side chat. Allow the generated title to replace the inherited name while preserving manual renames and leaving the parent title unchanged. Image-only first inputs retain the `[Image]` label without generating a title from inherited conversation text. ## Testing Add regression coverage for saved side chat labels from text and image-only inputs, inherited-name replacement, manual rename preservation after switching threads, and retaining title eligibility when turn submission fails. GitOrigin-RevId: 97f08dee8cc3280f1128aca23f27960235e00e86
This commit is contained in:
@@ -3392,6 +3392,7 @@ impl App {
|
||||
thread_id,
|
||||
destination,
|
||||
prompt,
|
||||
expected_name,
|
||||
fallback_title,
|
||||
effort,
|
||||
result,
|
||||
@@ -3401,6 +3402,7 @@ impl App {
|
||||
thread_id,
|
||||
destination,
|
||||
prompt,
|
||||
expected_name,
|
||||
fallback_title,
|
||||
effort,
|
||||
result,
|
||||
|
||||
@@ -4,11 +4,13 @@
|
||||
|
||||
use super::*;
|
||||
use crate::app::session_lifecycle::ThreadAttachPresentation;
|
||||
use crate::sidebar_chooser::side_chat_label;
|
||||
use codex_app_server_client::AppServerEvent;
|
||||
use codex_app_server_protocol::ClientRequest;
|
||||
use codex_app_server_protocol::TurnStartParams;
|
||||
use codex_app_server_protocol::TurnStartResponse;
|
||||
use core_test_support::responses;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
/// Captures one real model request and waits until its turn can be inherited by a fork.
|
||||
pub(super) async fn ask(
|
||||
@@ -104,3 +106,96 @@ async fn temporary_side_chat_sends_policy_on_its_first_request() -> Result<()> {
|
||||
server.shutdown().await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn saved_side_chat_title_uses_only_its_first_direct_input() -> Result<()> {
|
||||
let question = "Why is CI failing on Windows?";
|
||||
let pixel = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR4nGP4z8DwHwAFAAH/iZk9HQAAAABJRU5ErkJggg==";
|
||||
let image = crate::chatwidget::UserMessage {
|
||||
remote_image_urls: vec![pixel.into()],
|
||||
.."".into()
|
||||
};
|
||||
let mut labels = Vec::new();
|
||||
for message in [question.into(), image] {
|
||||
let (mut app, mut events, _ops) = make_test_app_with_channels().await;
|
||||
let model = responses::start_mock_server().await;
|
||||
app_test_support::MockResponsesConfig::new(&model.uri())
|
||||
.enable_feature(Feature::TuiWorkspace)
|
||||
.write(&app.config.codex_home)?;
|
||||
app.config = app
|
||||
.rebuild_config_for_cwd(app.config.cwd.to_path_buf())
|
||||
.await?;
|
||||
let mut server = crate::start_embedded_app_server_for_picker(&app.config).await?;
|
||||
let started = server.start_thread(&app.config).await?;
|
||||
let parent = started.session.thread_id;
|
||||
ask(&mut server, &model, parent, "Prepare a release").await?;
|
||||
server
|
||||
.thread_set_name(parent, "Release plan".into())
|
||||
.await?;
|
||||
app.enqueue_primary_thread_session(started.session, started.turns)
|
||||
.await?;
|
||||
enable_side_retention(&mut app)?;
|
||||
let mut tui = crate::tui::test_support::make_test_tui()?;
|
||||
tui.set_owned_screen(/*owned*/ true)?;
|
||||
let title_prompt = crate::app::thread_title::thread_title_prompt(question);
|
||||
responses::mount_response_handler(&model, move |request| {
|
||||
let answer = if request.message_input_texts("user").contains(&title_prompt) {
|
||||
r#"{"title":"Windows CI failure"}"#
|
||||
} else {
|
||||
"Answer"
|
||||
};
|
||||
responses::sse_response(responses::sse(vec![
|
||||
responses::ev_assistant_message("reply", answer),
|
||||
responses::ev_completed("done"),
|
||||
]))
|
||||
})
|
||||
.await;
|
||||
Box::pin(app.handle_start_side(
|
||||
&mut tui,
|
||||
&mut server,
|
||||
parent,
|
||||
Some(message),
|
||||
crate::app_event::SideStartMode::New,
|
||||
))
|
||||
.await?;
|
||||
|
||||
time::timeout(Duration::from_secs(15), async {
|
||||
let mut child_finished = false;
|
||||
while !child_finished || !app.pending_thread_titles.is_empty() {
|
||||
tokio::select! {
|
||||
Some(event) = events.recv() => {
|
||||
boxed_future!(app.handle_event(&mut tui, &mut server, event)).await?;
|
||||
}
|
||||
Some(event) = server.next_event() => {
|
||||
if let AppServerEvent::ServerNotification(n) = &event.event
|
||||
&& let ServerNotification::TurnCompleted(turn) = n.as_ref()
|
||||
&& app.side_thread_for_parent(parent)
|
||||
.is_some_and(|id| id.to_string() == turn.thread_id)
|
||||
{
|
||||
child_finished = true;
|
||||
}
|
||||
app.handle_backend_event(&server, event).await;
|
||||
}
|
||||
}
|
||||
}
|
||||
Ok::<_, color_eyre::Report>(())
|
||||
})
|
||||
.await??;
|
||||
let saved = server
|
||||
.list_saved_side_chats(parent, app.config.codex_home.as_path())
|
||||
.await?;
|
||||
let [child] = saved.as_slice() else {
|
||||
panic!("expected one saved side chat")
|
||||
};
|
||||
assert!(!server.side_chat_fork_names.contains_key(&child.id));
|
||||
labels.push(side_chat_label(child).unwrap().to_owned());
|
||||
let parent = server.thread_read(parent, /*include_turns*/ false).await?;
|
||||
assert_eq!(parent.name.as_deref(), Some("Release plan"));
|
||||
server.shutdown().await?;
|
||||
}
|
||||
insta::assert_snapshot!(labels.join("\n"), @r"
|
||||
Windows CI failure
|
||||
[Image]
|
||||
");
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -175,6 +175,7 @@ async fn turn_start_failure_is_shown_without_exiting() -> Result<()> {
|
||||
let mut tui = crate::tui::test_support::make_test_tui()?;
|
||||
let mut app_server = Box::pin(crate::start_embedded_app_server_for_picker(&app.config)).await?;
|
||||
let thread_id = ThreadId::from_string("123e4567-e89b-12d3-a456-426614174000")?;
|
||||
app_server.side_chat_fork_names.insert(thread_id, None);
|
||||
app.active_thread_id = Some(thread_id);
|
||||
app.chat_widget
|
||||
.handle_thread_session(test_thread_session(thread_id, app.config.cwd.to_path_buf()));
|
||||
@@ -194,6 +195,7 @@ async fn turn_start_failure_is_shown_without_exiting() -> Result<()> {
|
||||
.handle_event(&mut tui, &mut app_server, AppEvent::CodexOp(op))
|
||||
.await?;
|
||||
|
||||
assert!(app_server.side_chat_fork_names.contains_key(&thread_id));
|
||||
assert!(matches!(control, AppRunControl::Continue));
|
||||
let error_cell = std::iter::from_fn(|| app_event_rx.try_recv().ok())
|
||||
.find_map(|event| match event {
|
||||
|
||||
@@ -2229,18 +2229,7 @@ impl App {
|
||||
&& let ServerNotification::ItemCompleted(notification) = notification.as_ref()
|
||||
&& let ThreadItemPayload::UserMessage { content, .. } = ¬ification.item.payload
|
||||
{
|
||||
Some(
|
||||
content
|
||||
.iter()
|
||||
.filter_map(|item| match item {
|
||||
codex_app_server_protocol::UserInput::Text { text, .. } => {
|
||||
Some(crate::user_prompt::extract_prompt_request_with_offset(text).0)
|
||||
}
|
||||
_ => None,
|
||||
})
|
||||
.collect::<Vec<_>>()
|
||||
.join(" "),
|
||||
)
|
||||
super::thread_title::user_message_text(content)
|
||||
} else {
|
||||
None
|
||||
};
|
||||
@@ -2248,7 +2237,6 @@ impl App {
|
||||
self.handle_thread_event_now_recovering_file_changes(event)
|
||||
.await;
|
||||
if let Some(user_message) = automatic_title_user_message
|
||||
&& !user_message.trim().is_empty()
|
||||
&& let Some(thread_id) = self.active_thread_id
|
||||
{
|
||||
self.generate_thread_title(
|
||||
@@ -2256,6 +2244,7 @@ impl App {
|
||||
thread_id,
|
||||
ThreadTitleDestination::Automatic,
|
||||
&user_message,
|
||||
/*expected_name*/ None,
|
||||
);
|
||||
}
|
||||
if !had_active_modal
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
//! Structured-output schema and normalization for generated TUI thread titles.
|
||||
//!
|
||||
//! Automatic titles use the first message until generation completes and as a fallback;
|
||||
//! the originating thread's saved name takes precedence over an automatic result. Manual
|
||||
//! a saved side chat may also replace the name it inherited at fork. Manual
|
||||
//! renames cancel pending requests; canceled events cannot affect newer requests.
|
||||
|
||||
use super::App;
|
||||
@@ -70,6 +70,7 @@ impl App {
|
||||
thread_id: ThreadId,
|
||||
destination: ThreadTitleDestination,
|
||||
source_text: &str,
|
||||
expected_name: Option<String>,
|
||||
) {
|
||||
if app_server.thread_params_mode().settings_owner() == ThreadSettingsOwner::Server {
|
||||
if let ThreadTitleDestination::RenameSuggestion { request_id } = destination {
|
||||
@@ -87,7 +88,9 @@ impl App {
|
||||
let (prompt, fallback_title) = match destination {
|
||||
ThreadTitleDestination::Automatic => {
|
||||
let fallback = fallback_thread_title(source_text);
|
||||
if let Some(title) = &fallback {
|
||||
if !self.side_threads.contains_key(&thread_id)
|
||||
&& let Some(title) = &fallback
|
||||
{
|
||||
self.chat_widget
|
||||
.display_thread_name(thread_id, title.clone());
|
||||
}
|
||||
@@ -135,6 +138,7 @@ impl App {
|
||||
thread_id,
|
||||
destination,
|
||||
prompt,
|
||||
expected_name,
|
||||
fallback_title,
|
||||
effort,
|
||||
result,
|
||||
@@ -150,6 +154,7 @@ impl App {
|
||||
thread_id: ThreadId,
|
||||
destination: ThreadTitleDestination,
|
||||
prompt: String,
|
||||
expected_name: Option<String>,
|
||||
fallback_title: Option<String>,
|
||||
effort: Option<ReasoningEffort>,
|
||||
result: Result<String, String>,
|
||||
@@ -165,7 +170,7 @@ impl App {
|
||||
return;
|
||||
}
|
||||
let save_name = (destination == ThreadTitleDestination::Automatic)
|
||||
.then(|| app_server.automatic_thread_name_writer(thread_id));
|
||||
.then(|| app_server.automatic_thread_name_writer(thread_id, expected_name));
|
||||
let temporary_thread_id = match result
|
||||
.and_then(|id| ThreadId::from_string(&id).map_err(|error| error.to_string()))
|
||||
{
|
||||
@@ -278,6 +283,7 @@ impl App {
|
||||
thread_id,
|
||||
ThreadTitleDestination::RenameSuggestion { request_id },
|
||||
&conversation,
|
||||
/*expected_name*/ None,
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -354,6 +360,20 @@ pub(super) fn thread_title_prompt(user_message: &str) -> String {
|
||||
format!("{prefix}{user_message}")
|
||||
}
|
||||
|
||||
pub(super) fn user_message_text(content: &[UserInput]) -> Option<String> {
|
||||
let text = content
|
||||
.iter()
|
||||
.filter_map(|item| match item {
|
||||
UserInput::Text { text, .. } => {
|
||||
Some(crate::user_prompt::extract_prompt_request_with_offset(text).0)
|
||||
}
|
||||
_ => None,
|
||||
})
|
||||
.collect::<Vec<_>>()
|
||||
.join(" ");
|
||||
(!text.trim().is_empty()).then_some(text)
|
||||
}
|
||||
|
||||
/// Format recent substantive messages chronologically without trusting their markup.
|
||||
pub(super) fn recent_conversation_messages<'a, I>(items: I) -> Option<String>
|
||||
where
|
||||
|
||||
@@ -109,6 +109,7 @@ async fn cloud_conversations_skip_tui_generated_titles() {
|
||||
thread_id,
|
||||
ThreadTitleDestination::Automatic,
|
||||
"Fix the login timeout",
|
||||
/*expected_name*/ None,
|
||||
);
|
||||
|
||||
assert!(app.pending_thread_titles.is_empty());
|
||||
@@ -118,10 +119,12 @@ async fn cloud_conversations_skip_tui_generated_titles() {
|
||||
#[tokio::test]
|
||||
async fn automatic_thread_title_respects_origin_metadata_after_switching() -> color_eyre::Result<()>
|
||||
{
|
||||
for (switch_threads, manual_name) in [
|
||||
(true, None),
|
||||
(true, Some("Manual title")),
|
||||
(false, Some("Manual title")),
|
||||
for (switch_threads, fork_name, manual_name) in [
|
||||
(true, None, None),
|
||||
(true, Some("Release plan"), None),
|
||||
(true, Some("Release plan"), Some("Manual title")),
|
||||
(true, None, Some("Manual title")),
|
||||
(false, None, Some("Manual title")),
|
||||
] {
|
||||
let mut app = make_test_app().await;
|
||||
let mut app_server = crate::start_embedded_app_server_for_picker(&app.config).await?;
|
||||
@@ -141,7 +144,13 @@ async fn automatic_thread_title_respects_origin_metadata_after_switching() -> co
|
||||
);
|
||||
|
||||
let mut tui = crate::tui::test_support::make_test_tui()?;
|
||||
let automatic = app_server.automatic_thread_name_writer(thread_id);
|
||||
if let Some(name) = fork_name {
|
||||
app_server
|
||||
.thread_set_name(thread_id, name.to_owned())
|
||||
.await?;
|
||||
}
|
||||
let automatic =
|
||||
app_server.automatic_thread_name_writer(thread_id, fork_name.map(str::to_owned));
|
||||
if switch_threads {
|
||||
let second = app_server.start_thread(&app.config).await?;
|
||||
let second_id = second.session.thread_id;
|
||||
@@ -532,7 +541,7 @@ async fn canceled_thread_title_ignores_late_start_and_completion() -> color_eyre
|
||||
app.pending_thread_titles
|
||||
.insert((thread_id, destination), cancellation.clone());
|
||||
|
||||
let writer = app_server.automatic_thread_name_writer(thread_id);
|
||||
let writer = app_server.automatic_thread_name_writer(thread_id, /*expected_name*/ None);
|
||||
let token = cancellation.clone();
|
||||
let mut automatic = std::pin::pin!(writer.save("Stale title".into(), &token));
|
||||
assert!(automatic.as_mut().now_or_never().is_none());
|
||||
@@ -558,6 +567,7 @@ async fn canceled_thread_title_ignores_late_start_and_completion() -> color_eyre
|
||||
thread_id,
|
||||
destination,
|
||||
"prompt".to_string(),
|
||||
/*expected_name*/ None,
|
||||
/*fallback_title*/ None,
|
||||
/*effort*/ None,
|
||||
Ok(temporary_thread_id.to_string()),
|
||||
@@ -603,7 +613,7 @@ async fn manual_rename_waits_for_an_in_flight_automatic_write() -> color_eyre::R
|
||||
codex_app_server_client::AppServerClient::Cloud(client),
|
||||
crate::app_server_session::ThreadParamsMode::Remote,
|
||||
);
|
||||
let writer = session.automatic_thread_name_writer(thread_id);
|
||||
let writer = session.automatic_thread_name_writer(thread_id, /*expected_name*/ None);
|
||||
let token = CancellationToken::new();
|
||||
let mut automatic = std::pin::pin!(writer.save("Generated".into(), &token));
|
||||
assert!(automatic.as_mut().now_or_never().is_none());
|
||||
@@ -915,6 +925,7 @@ async fn failed_thread_title_requests_are_removed() -> color_eyre::Result<()> {
|
||||
thread_id,
|
||||
ThreadTitleDestination::Automatic,
|
||||
"prompt".to_string(),
|
||||
/*expected_name*/ None,
|
||||
/*fallback_title*/ None,
|
||||
/*effort*/ None,
|
||||
Err("startup failed".to_string()),
|
||||
@@ -925,6 +936,7 @@ async fn failed_thread_title_requests_are_removed() -> color_eyre::Result<()> {
|
||||
thread_id,
|
||||
suggestion,
|
||||
"prompt".to_string(),
|
||||
/*expected_name*/ None,
|
||||
/*fallback_title*/ None,
|
||||
/*effort*/ None,
|
||||
Ok("invalid-thread-id".to_string()),
|
||||
|
||||
@@ -17,6 +17,7 @@ pub(super) async fn try_submit_user_turn(
|
||||
) -> Result<bool> {
|
||||
let AppCommand::UserTurn {
|
||||
turn_trigger,
|
||||
items,
|
||||
cloud_submission,
|
||||
client_user_message_id,
|
||||
..
|
||||
@@ -94,6 +95,20 @@ pub(super) async fn try_submit_user_turn(
|
||||
|
||||
match result {
|
||||
Ok((admission, selection)) => {
|
||||
if *turn_trigger == "user"
|
||||
&& !items.is_empty()
|
||||
&& matches!(admission, UserMessageAdmission::StartResponse(_))
|
||||
&& let Some(fork_name) = app_server.side_chat_fork_names.remove(&thread_id)
|
||||
&& let Some(text) = super::thread_title::user_message_text(items)
|
||||
{
|
||||
app.generate_thread_title(
|
||||
app_server,
|
||||
thread_id,
|
||||
ThreadTitleDestination::Automatic,
|
||||
&text,
|
||||
fork_name,
|
||||
);
|
||||
}
|
||||
if let Some(selection) = selection {
|
||||
if selection.model.is_some() {
|
||||
app.chat_widget.cloud_picker_selection = selection.model;
|
||||
|
||||
@@ -441,6 +441,7 @@ pub(crate) enum AppEvent {
|
||||
thread_id: ThreadId,
|
||||
destination: ThreadTitleDestination,
|
||||
prompt: String,
|
||||
expected_name: Option<String>,
|
||||
fallback_title: Option<String>,
|
||||
effort: Option<ReasoningEffort>,
|
||||
result: Result<String, String>,
|
||||
|
||||
@@ -396,6 +396,8 @@ pub(crate) struct AppServerSession {
|
||||
request_id_namespace: Option<Uuid>,
|
||||
next_request_id: i64,
|
||||
automatic_name_writes: HashMap<ThreadId, Arc<Semaphore>>,
|
||||
/// Newly forked saved side chats and the names their first direct turn may replace.
|
||||
pub(crate) side_chat_fork_names: HashMap<ThreadId, Option<String>>,
|
||||
/// The newest docked replay refresh on this connection.
|
||||
pub(crate) pending_side_skills_refresh: Option<Uuid>,
|
||||
history_pagination: HashMap<ThreadId, history::ThreadHistoryPagination>,
|
||||
@@ -430,6 +432,7 @@ pub(crate) struct AutomaticThreadName {
|
||||
session: AppServerSession,
|
||||
thread_id: ThreadId,
|
||||
gate: Arc<Semaphore>,
|
||||
expected_name: Option<String>,
|
||||
}
|
||||
|
||||
impl AutomaticThreadName {
|
||||
@@ -440,7 +443,7 @@ impl AutomaticThreadName {
|
||||
let Some(Ok(thread)) = cancellation.run_until_cancelled(read).await else {
|
||||
return;
|
||||
};
|
||||
if thread.name.is_some() {
|
||||
if thread.name != self.expected_name {
|
||||
return;
|
||||
}
|
||||
let Ok(_permit) = self.gate.acquire().await else {
|
||||
@@ -618,6 +621,7 @@ impl AppServerSession {
|
||||
request_id_namespace: None,
|
||||
next_request_id: 1,
|
||||
automatic_name_writes: HashMap::new(),
|
||||
side_chat_fork_names: HashMap::new(),
|
||||
pending_side_skills_refresh: None,
|
||||
attachment_subscribed: None,
|
||||
history_pagination: HashMap::new(),
|
||||
@@ -1373,6 +1377,11 @@ impl AppServerSession {
|
||||
}
|
||||
|
||||
pub(crate) fn observe_sidebar_fork(&mut self, fork: &SidebarFork) {
|
||||
if fork.storage == SideChatStorage::Attached && fork.boundary.is_ok() {
|
||||
let child = &fork.thread.session;
|
||||
self.side_chat_fork_names
|
||||
.insert(child.thread_id, child.thread_name.clone());
|
||||
}
|
||||
if fork.legacy {
|
||||
self.history_support = ThreadHistorySupport::LegacyOnly;
|
||||
}
|
||||
@@ -1736,6 +1745,7 @@ impl AppServerSession {
|
||||
})
|
||||
.await
|
||||
.wrap_err("failed to delete session")?;
|
||||
self.side_chat_fork_names.remove(&thread_id);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -1953,6 +1963,7 @@ impl AppServerSession {
|
||||
pub(crate) fn automatic_thread_name_writer(
|
||||
&mut self,
|
||||
thread_id: ThreadId,
|
||||
expected_name: Option<String>,
|
||||
) -> AutomaticThreadName {
|
||||
self.automatic_name_writes
|
||||
.retain(|_, gate| Arc::strong_count(gate) > 1);
|
||||
@@ -1965,6 +1976,7 @@ impl AppServerSession {
|
||||
session,
|
||||
thread_id,
|
||||
gate: Arc::clone(gate),
|
||||
expected_name,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1990,6 +2002,7 @@ impl AppServerSession {
|
||||
})
|
||||
.await
|
||||
.wrap_err("thread/name/set failed in TUI")?;
|
||||
self.side_chat_fork_names.remove(&thread_id);
|
||||
if let Some(gate) = gate.as_ref() {
|
||||
gate.close();
|
||||
self.automatic_name_writes.remove(&thread_id);
|
||||
|
||||
Reference in New Issue
Block a user