mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Use app server metadata for fork parent titles (#18632)
## Problem The TUI resolved fork parent titles from local CODEX_HOME metadata, which could show missing or stale titles when app-server metadata is authoritative. This is a lingering bug left over from the migration of the TUI to the app-server interface. I found it when I asked Codex to review all places where the TUI code was still directly accessing the local CODEX_HOME. ## Solution Route fork parent title metadata through the app-server session state and render only that supplied title, with focused snapshot coverage for stale local metadata. ## Testing I manually tested by renaming a thread then forking it and confirming that the "forked from" message indicated the parent thread's name.
This commit is contained in:
committed by
GitHub
Unverified
parent
54bd07d28c
commit
b8e78e8869
+2
-44
@@ -180,6 +180,7 @@ mod loaded_threads;
|
||||
mod pending_interactive_replay;
|
||||
mod replay_filter;
|
||||
mod side;
|
||||
mod thread_session_state;
|
||||
|
||||
use self::agent_navigation::AgentNavigationDirection;
|
||||
use self::agent_navigation::AgentNavigationState;
|
||||
@@ -3439,50 +3440,6 @@ impl App {
|
||||
}
|
||||
}
|
||||
|
||||
async fn session_state_for_thread_read(
|
||||
&self,
|
||||
thread_id: ThreadId,
|
||||
thread: &codex_app_server_protocol::Thread,
|
||||
) -> ThreadSessionState {
|
||||
let mut session = self
|
||||
.primary_session_configured
|
||||
.clone()
|
||||
.unwrap_or(ThreadSessionState {
|
||||
thread_id,
|
||||
forked_from_id: None,
|
||||
thread_name: None,
|
||||
model: self.chat_widget.current_model().to_string(),
|
||||
model_provider_id: self.config.model_provider_id.clone(),
|
||||
service_tier: self.chat_widget.current_service_tier(),
|
||||
approval_policy: self.config.permissions.approval_policy.value(),
|
||||
approvals_reviewer: self.config.approvals_reviewer,
|
||||
sandbox_policy: self.config.permissions.sandbox_policy.get().clone(),
|
||||
cwd: thread.cwd.clone(),
|
||||
instruction_source_paths: Vec::new(),
|
||||
reasoning_effort: self.chat_widget.current_reasoning_effort(),
|
||||
history_log_id: 0,
|
||||
history_entry_count: 0,
|
||||
network_proxy: None,
|
||||
rollout_path: thread.path.clone(),
|
||||
});
|
||||
session.thread_id = thread_id;
|
||||
session.thread_name = thread.name.clone();
|
||||
session.model_provider_id = thread.model_provider.clone();
|
||||
session.cwd = thread.cwd.clone();
|
||||
session.instruction_source_paths = Vec::new();
|
||||
session.rollout_path = thread.path.clone();
|
||||
if let Some(model) =
|
||||
read_session_model(&self.config, thread_id, thread.path.as_deref()).await
|
||||
{
|
||||
session.model = model;
|
||||
} else if thread.path.is_some() {
|
||||
session.model.clear();
|
||||
}
|
||||
session.history_log_id = 0;
|
||||
session.history_entry_count = 0;
|
||||
session
|
||||
}
|
||||
|
||||
/// Materializes a live thread into local replay state when the picker knows about it but the
|
||||
/// TUI has not cached a local event channel yet.
|
||||
///
|
||||
@@ -10976,6 +10933,7 @@ guardian_approval = true
|
||||
ThreadSessionState {
|
||||
thread_id,
|
||||
forked_from_id: None,
|
||||
fork_parent_title: None,
|
||||
thread_name: None,
|
||||
model: "gpt-test".to_string(),
|
||||
model_provider_id: "test-provider".to_string(),
|
||||
|
||||
@@ -0,0 +1,52 @@
|
||||
use super::App;
|
||||
use crate::app_server_session::ThreadSessionState;
|
||||
use crate::read_session_model;
|
||||
use codex_app_server_protocol::Thread;
|
||||
use codex_protocol::ThreadId;
|
||||
|
||||
impl App {
|
||||
pub(super) async fn session_state_for_thread_read(
|
||||
&self,
|
||||
thread_id: ThreadId,
|
||||
thread: &Thread,
|
||||
) -> ThreadSessionState {
|
||||
let mut session = self
|
||||
.primary_session_configured
|
||||
.clone()
|
||||
.unwrap_or(ThreadSessionState {
|
||||
thread_id,
|
||||
forked_from_id: None,
|
||||
fork_parent_title: None,
|
||||
thread_name: None,
|
||||
model: self.chat_widget.current_model().to_string(),
|
||||
model_provider_id: self.config.model_provider_id.clone(),
|
||||
service_tier: self.chat_widget.current_service_tier(),
|
||||
approval_policy: self.config.permissions.approval_policy.value(),
|
||||
approvals_reviewer: self.config.approvals_reviewer,
|
||||
sandbox_policy: self.config.permissions.sandbox_policy.get().clone(),
|
||||
cwd: thread.cwd.clone(),
|
||||
instruction_source_paths: Vec::new(),
|
||||
reasoning_effort: self.chat_widget.current_reasoning_effort(),
|
||||
history_log_id: 0,
|
||||
history_entry_count: 0,
|
||||
network_proxy: None,
|
||||
rollout_path: thread.path.clone(),
|
||||
});
|
||||
session.thread_id = thread_id;
|
||||
session.thread_name = thread.name.clone();
|
||||
session.model_provider_id = thread.model_provider.clone();
|
||||
session.cwd = thread.cwd.clone();
|
||||
session.instruction_source_paths = Vec::new();
|
||||
session.rollout_path = thread.path.clone();
|
||||
if let Some(model) =
|
||||
read_session_model(&self.config, thread_id, thread.path.as_deref()).await
|
||||
{
|
||||
session.model = model;
|
||||
} else if thread.path.is_some() {
|
||||
session.model.clear();
|
||||
}
|
||||
session.history_log_id = 0;
|
||||
session.history_entry_count = 0;
|
||||
session
|
||||
}
|
||||
}
|
||||
@@ -138,6 +138,7 @@ pub(crate) struct AppServerSession {
|
||||
pub(crate) struct ThreadSessionState {
|
||||
pub(crate) thread_id: ThreadId,
|
||||
pub(crate) forked_from_id: Option<ThreadId>,
|
||||
pub(crate) fork_parent_title: Option<String>,
|
||||
pub(crate) thread_name: Option<String>,
|
||||
pub(crate) model: String,
|
||||
pub(crate) model_provider_id: String,
|
||||
@@ -369,7 +370,12 @@ impl AppServerSession {
|
||||
})
|
||||
.await
|
||||
.wrap_err("thread/resume failed during TUI bootstrap")?;
|
||||
started_thread_from_resume_response(response, &config).await
|
||||
let fork_parent_title = self
|
||||
.fork_parent_title_from_app_server(response.thread.forked_from_id.as_deref())
|
||||
.await;
|
||||
let mut started = started_thread_from_resume_response(response, &config).await?;
|
||||
started.session.fork_parent_title = fork_parent_title;
|
||||
Ok(started)
|
||||
}
|
||||
|
||||
pub(crate) async fn fork_thread(
|
||||
@@ -391,7 +397,12 @@ impl AppServerSession {
|
||||
})
|
||||
.await
|
||||
.wrap_err("thread/fork failed during TUI bootstrap")?;
|
||||
started_thread_from_fork_response(response, &config).await
|
||||
let fork_parent_title = self
|
||||
.fork_parent_title_from_app_server(response.thread.forked_from_id.as_deref())
|
||||
.await;
|
||||
let mut started = started_thread_from_fork_response(response, &config).await?;
|
||||
started.session.fork_parent_title = fork_parent_title;
|
||||
Ok(started)
|
||||
}
|
||||
|
||||
fn thread_params_mode(&self) -> ThreadParamsMode {
|
||||
@@ -401,6 +412,31 @@ impl AppServerSession {
|
||||
}
|
||||
}
|
||||
|
||||
async fn fork_parent_title_from_app_server(
|
||||
&mut self,
|
||||
forked_from_id: Option<&str>,
|
||||
) -> Option<String> {
|
||||
let forked_from_id = forked_from_id?;
|
||||
let forked_from_id = match ThreadId::from_string(forked_from_id) {
|
||||
Ok(thread_id) => thread_id,
|
||||
Err(err) => {
|
||||
tracing::warn!("Failed to parse fork parent thread id from app server: {err}");
|
||||
return None;
|
||||
}
|
||||
};
|
||||
|
||||
match self
|
||||
.thread_read(forked_from_id, /*include_turns*/ false)
|
||||
.await
|
||||
{
|
||||
Ok(thread) => thread.name,
|
||||
Err(err) => {
|
||||
tracing::warn!("Failed to read fork parent metadata from app server: {err}");
|
||||
None
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) async fn thread_list(
|
||||
&mut self,
|
||||
params: ThreadListParams,
|
||||
@@ -1204,6 +1240,7 @@ async fn thread_session_state_from_thread_response(
|
||||
Ok(ThreadSessionState {
|
||||
thread_id,
|
||||
forked_from_id,
|
||||
fork_parent_title: None,
|
||||
thread_name,
|
||||
model,
|
||||
model_provider_id,
|
||||
|
||||
@@ -2081,14 +2081,20 @@ impl ChatWidget {
|
||||
}
|
||||
|
||||
// --- Small event handlers ---
|
||||
#[cfg(test)]
|
||||
fn on_session_configured(&mut self, event: codex_protocol::protocol::SessionConfiguredEvent) {
|
||||
self.on_session_configured_with_display(event, SessionConfiguredDisplay::Normal);
|
||||
self.on_session_configured_with_display_and_fork_parent_title(
|
||||
event,
|
||||
SessionConfiguredDisplay::Normal,
|
||||
/*fork_parent_title*/ None,
|
||||
);
|
||||
}
|
||||
|
||||
fn on_session_configured_with_display(
|
||||
fn on_session_configured_with_display_and_fork_parent_title(
|
||||
&mut self,
|
||||
event: codex_protocol::protocol::SessionConfiguredEvent,
|
||||
display: SessionConfiguredDisplay,
|
||||
fork_parent_title: Option<String>,
|
||||
) {
|
||||
self.last_agent_markdown = None;
|
||||
self.agent_turn_markdowns.clear();
|
||||
@@ -2190,7 +2196,7 @@ impl ChatWidget {
|
||||
if display == SessionConfiguredDisplay::Normal
|
||||
&& let Some(forked_from_id) = forked_from_id
|
||||
{
|
||||
self.emit_forked_thread_event(forked_from_id);
|
||||
self.emit_forked_thread_event(forked_from_id, fork_parent_title);
|
||||
}
|
||||
if !self.suppress_session_configured_redraw {
|
||||
self.request_redraw();
|
||||
@@ -2209,33 +2215,59 @@ impl ChatWidget {
|
||||
|
||||
pub(crate) fn handle_thread_session(&mut self, session: ThreadSessionState) {
|
||||
self.instruction_source_paths = session.instruction_source_paths.clone();
|
||||
self.on_session_configured(thread_session_state_to_legacy_event(session));
|
||||
let fork_parent_title = session.fork_parent_title.clone();
|
||||
self.on_session_configured_with_display_and_fork_parent_title(
|
||||
thread_session_state_to_legacy_event(session),
|
||||
SessionConfiguredDisplay::Normal,
|
||||
fork_parent_title,
|
||||
);
|
||||
}
|
||||
|
||||
pub(crate) fn handle_thread_session_quiet(&mut self, session: ThreadSessionState) {
|
||||
self.instruction_source_paths = session.instruction_source_paths.clone();
|
||||
self.on_session_configured_with_display(
|
||||
self.on_session_configured_with_display_and_fork_parent_title(
|
||||
thread_session_state_to_legacy_event(session),
|
||||
SessionConfiguredDisplay::Quiet,
|
||||
/*fork_parent_title*/ None,
|
||||
);
|
||||
}
|
||||
|
||||
pub(crate) fn handle_side_thread_session(&mut self, session: ThreadSessionState) {
|
||||
self.instruction_source_paths = session.instruction_source_paths.clone();
|
||||
self.on_session_configured_with_display(
|
||||
let fork_parent_title = session.fork_parent_title.clone();
|
||||
self.on_session_configured_with_display_and_fork_parent_title(
|
||||
thread_session_state_to_legacy_event(session),
|
||||
SessionConfiguredDisplay::SideConversation,
|
||||
fork_parent_title,
|
||||
);
|
||||
}
|
||||
|
||||
fn emit_forked_thread_event(&mut self, forked_from_id: ThreadId) {
|
||||
fn emit_forked_thread_event(
|
||||
&mut self,
|
||||
forked_from_id: ThreadId,
|
||||
fork_parent_title: Option<String>,
|
||||
) {
|
||||
let forked_from_id_text = forked_from_id.to_string();
|
||||
let line: Line<'static> = vec![
|
||||
"• ".dim(),
|
||||
"Thread forked from ".into(),
|
||||
forked_from_id_text.cyan(),
|
||||
]
|
||||
.into();
|
||||
let line: Line<'static> = if let Some(name) = fork_parent_title
|
||||
&& !name.trim().is_empty()
|
||||
{
|
||||
vec![
|
||||
"• ".dim(),
|
||||
"Thread forked from ".into(),
|
||||
name.cyan(),
|
||||
" (".into(),
|
||||
forked_from_id_text.cyan(),
|
||||
")".into(),
|
||||
]
|
||||
.into()
|
||||
} else {
|
||||
vec![
|
||||
"• ".dim(),
|
||||
"Thread forked from ".into(),
|
||||
forked_from_id_text.cyan(),
|
||||
]
|
||||
.into()
|
||||
};
|
||||
self.app_event_tx.send(AppEvent::InsertHistoryCell(Box::new(
|
||||
PlainHistoryCell::new(vec![line]),
|
||||
)));
|
||||
|
||||
+5
@@ -0,0 +1,5 @@
|
||||
---
|
||||
source: tui/src/chatwidget/tests/history_replay.rs
|
||||
expression: combined
|
||||
---
|
||||
• Thread forked from app-server-parent-thread (e9f18a88-8081-4e51-9d4e-8af5cde2d8dd)
|
||||
+5
@@ -0,0 +1,5 @@
|
||||
---
|
||||
source: tui/src/chatwidget/tests/history_replay.rs
|
||||
expression: combined
|
||||
---
|
||||
• Thread forked from 019c2d47-4935-7423-a190-05691f566092
|
||||
+5
@@ -0,0 +1,5 @@
|
||||
---
|
||||
source: tui/src/chatwidget/tests/history_replay.rs
|
||||
expression: combined
|
||||
---
|
||||
• Thread forked from named-thread (e9f18a88-8081-4e51-9d4e-8af5cde2d8dd)
|
||||
@@ -387,6 +387,36 @@ async fn replayed_user_message_with_only_local_images_does_not_render_history_ce
|
||||
assert!(!found_user_history_cell);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn forked_thread_history_line_includes_name_and_id_snapshot() {
|
||||
let (chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
let mut chat = chat;
|
||||
|
||||
let forked_from_id =
|
||||
ThreadId::from_string("e9f18a88-8081-4e51-9d4e-8af5cde2d8dd").expect("forked id");
|
||||
|
||||
chat.emit_forked_thread_event(forked_from_id, Some("named-thread".to_string()));
|
||||
|
||||
let history_cell = tokio::time::timeout(std::time::Duration::from_secs(2), async {
|
||||
loop {
|
||||
match rx.recv().await {
|
||||
Some(AppEvent::InsertHistoryCell(cell)) => break cell,
|
||||
Some(_) => continue,
|
||||
None => panic!("app event channel closed before forked thread history was emitted"),
|
||||
}
|
||||
}
|
||||
})
|
||||
.await
|
||||
.expect("timed out waiting for forked thread history");
|
||||
let combined = lines_to_single_string(&history_cell.display_lines(/*width*/ 80));
|
||||
|
||||
assert!(
|
||||
combined.contains("Thread forked from"),
|
||||
"expected forked thread message in history"
|
||||
);
|
||||
assert_chatwidget_snapshot!("forked_thread_history_line", combined);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn forked_thread_history_line_without_name_shows_id_once_snapshot() {
|
||||
let (chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
@@ -398,7 +428,7 @@ async fn forked_thread_history_line_without_name_shows_id_once_snapshot() {
|
||||
|
||||
let forked_from_id =
|
||||
ThreadId::from_string("019c2d47-4935-7423-a190-05691f566092").expect("forked id");
|
||||
chat.emit_forked_thread_event(forked_from_id);
|
||||
chat.emit_forked_thread_event(forked_from_id, /*fork_parent_title*/ None);
|
||||
|
||||
let history_cell = tokio::time::timeout(std::time::Duration::from_secs(2), async {
|
||||
loop {
|
||||
@@ -416,6 +446,88 @@ async fn forked_thread_history_line_without_name_shows_id_once_snapshot() {
|
||||
assert_chatwidget_snapshot!("forked_thread_history_line_without_name", combined);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn app_server_forked_thread_history_line_uses_app_server_title_snapshot() {
|
||||
let (chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
let mut chat = chat;
|
||||
let temp = tempdir().expect("tempdir");
|
||||
chat.config.codex_home =
|
||||
codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path(temp.path())
|
||||
.expect("temp dir is absolute");
|
||||
|
||||
let forked_from_id =
|
||||
ThreadId::from_string("e9f18a88-8081-4e51-9d4e-8af5cde2d8dd").expect("forked id");
|
||||
let session_index_entry = format!(
|
||||
"{{\"id\":\"{forked_from_id}\",\"thread_name\":\"stale-local-thread\",\"updated_at\":\"2024-01-02T00:00:00Z\"}}\n"
|
||||
);
|
||||
std::fs::write(temp.path().join("session_index.jsonl"), session_index_entry)
|
||||
.expect("write session index");
|
||||
|
||||
chat.emit_forked_thread_event(forked_from_id, Some("app-server-parent-thread".to_string()));
|
||||
|
||||
let history_cell = tokio::time::timeout(std::time::Duration::from_secs(2), async {
|
||||
loop {
|
||||
match rx.recv().await {
|
||||
Some(AppEvent::InsertHistoryCell(cell)) => break cell,
|
||||
Some(_) => continue,
|
||||
None => panic!("app event channel closed before forked thread history was emitted"),
|
||||
}
|
||||
}
|
||||
})
|
||||
.await
|
||||
.expect("timed out waiting for forked thread history");
|
||||
let combined = lines_to_single_string(&history_cell.display_lines(/*width*/ 80));
|
||||
|
||||
assert!(combined.contains("app-server-parent-thread"));
|
||||
assert!(
|
||||
!combined.contains("stale-local-thread"),
|
||||
"app-server fork title lookup should not read local CODEX_HOME"
|
||||
);
|
||||
assert_chatwidget_snapshot!("app_server_forked_thread_history_line", combined);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn app_server_forked_thread_history_line_without_app_server_name_ignores_local_snapshot() {
|
||||
let (chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
let mut chat = chat;
|
||||
let temp = tempdir().expect("tempdir");
|
||||
chat.config.codex_home =
|
||||
codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path(temp.path())
|
||||
.expect("temp dir is absolute");
|
||||
|
||||
let forked_from_id =
|
||||
ThreadId::from_string("019c2d47-4935-7423-a190-05691f566092").expect("forked id");
|
||||
let session_index_entry = format!(
|
||||
"{{\"id\":\"{forked_from_id}\",\"thread_name\":\"stale-local-thread\",\"updated_at\":\"2024-01-02T00:00:00Z\"}}\n"
|
||||
);
|
||||
std::fs::write(temp.path().join("session_index.jsonl"), session_index_entry)
|
||||
.expect("write session index");
|
||||
|
||||
chat.emit_forked_thread_event(forked_from_id, /*fork_parent_title*/ None);
|
||||
|
||||
let history_cell = tokio::time::timeout(std::time::Duration::from_secs(2), async {
|
||||
loop {
|
||||
match rx.recv().await {
|
||||
Some(AppEvent::InsertHistoryCell(cell)) => break cell,
|
||||
Some(_) => continue,
|
||||
None => panic!("app event channel closed before forked thread history was emitted"),
|
||||
}
|
||||
}
|
||||
})
|
||||
.await
|
||||
.expect("timed out waiting for forked thread history");
|
||||
let combined = lines_to_single_string(&history_cell.display_lines(/*width*/ 80));
|
||||
|
||||
assert!(
|
||||
!combined.contains("stale-local-thread"),
|
||||
"app-server fork title lookup should not read local CODEX_HOME"
|
||||
);
|
||||
assert_chatwidget_snapshot!(
|
||||
"app_server_forked_thread_history_line_without_app_server_name",
|
||||
combined
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_snapshot_replay_preserves_agent_message_during_review_mode() {
|
||||
let (mut chat, mut rx, _ops) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
|
||||
@@ -9,7 +9,7 @@ async fn forked_thread_history_line_without_name_shows_id_once_snapshot() {
|
||||
|
||||
let forked_from_id =
|
||||
ThreadId::from_string("019c2d47-4935-7423-a190-05691f566092").expect("forked id");
|
||||
chat.emit_forked_thread_event(forked_from_id);
|
||||
chat.emit_forked_thread_event(forked_from_id, /*fork_parent_title*/ None);
|
||||
|
||||
let history_cell = tokio::time::timeout(std::time::Duration::from_secs(2), async {
|
||||
loop {
|
||||
|
||||
Reference in New Issue
Block a user