mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[codex] Fix pathless thread summaries (#21266)
## Summary Fix `getConversationSummary` so thread-id summaries work for stored threads that do not have a local rollout path, such as remote thread stores. The root cause was that `summary_from_stored_thread` returned `None` when `StoredThread.rollout_path` was absent, and `get_thread_summary_response_inner` treated that as an internal error. This made conversation-id lookups depend on a local-only field even though the thread store can address the thread by id.
This commit is contained in:
@@ -13,10 +13,8 @@ use crate::outgoing_message::RequestContext;
|
||||
use crate::outgoing_message::ThreadScopedOutgoingMessageSender;
|
||||
use crate::thread_status::ThreadWatchManager;
|
||||
use crate::thread_status::resolve_thread_status;
|
||||
use chrono::DateTime;
|
||||
use chrono::Duration as ChronoDuration;
|
||||
use chrono::SecondsFormat;
|
||||
use chrono::Utc;
|
||||
use codex_analytics::AnalyticsEventsClient;
|
||||
use codex_analytics::AnalyticsJsonRpcError;
|
||||
use codex_analytics::InputError;
|
||||
@@ -495,6 +493,7 @@ pub(crate) use self::thread_lifecycle::populate_thread_turns_from_history;
|
||||
pub(crate) use self::thread_processor::thread_from_stored_thread;
|
||||
#[cfg(test)]
|
||||
pub(crate) use self::thread_summary::read_summary_from_rollout;
|
||||
#[cfg(test)]
|
||||
pub(crate) use self::thread_summary::summary_to_thread;
|
||||
|
||||
pub(crate) fn build_api_turns_from_rollout_items(items: &[RolloutItem]) -> Vec<Turn> {
|
||||
|
||||
@@ -1605,11 +1605,8 @@ impl ThreadRequestProcessor {
|
||||
.unarchive_thread(StoreArchiveThreadParams { thread_id })
|
||||
.await
|
||||
.map_err(|err| thread_store_archive_error("unarchive", err))?;
|
||||
let summary = summary_from_stored_thread(stored_thread, fallback_provider.as_str())
|
||||
.ok_or_else(|| {
|
||||
internal_error(format!("failed to read unarchived thread {thread_id}"))
|
||||
})?;
|
||||
let mut thread = summary_to_thread(summary, &self.config.cwd);
|
||||
let (mut thread, _) =
|
||||
thread_from_stored_thread(stored_thread, fallback_provider.as_str(), &self.config.cwd);
|
||||
|
||||
thread.status = resolve_thread_status(
|
||||
self.thread_watch_manager
|
||||
@@ -3215,12 +3212,7 @@ impl ThreadRequestProcessor {
|
||||
};
|
||||
|
||||
let stored_thread = read_result?;
|
||||
let summary =
|
||||
summary_from_stored_thread(stored_thread, fallback_provider).ok_or_else(|| {
|
||||
internal_error(
|
||||
"failed to load conversation summary: thread is missing rollout path",
|
||||
)
|
||||
})?;
|
||||
let summary = summary_from_stored_thread(stored_thread, fallback_provider);
|
||||
Ok(GetConversationSummaryResponse { summary })
|
||||
}
|
||||
|
||||
@@ -3697,8 +3689,8 @@ pub(crate) fn thread_from_stored_thread(
|
||||
fn summary_from_stored_thread(
|
||||
thread: StoredThread,
|
||||
fallback_provider: &str,
|
||||
) -> Option<ConversationSummary> {
|
||||
let path = thread.rollout_path?;
|
||||
) -> ConversationSummary {
|
||||
let path = thread.rollout_path.unwrap_or_default();
|
||||
let source = with_thread_spawn_agent_metadata(
|
||||
thread.source,
|
||||
thread.agent_nickname.clone(),
|
||||
@@ -3709,7 +3701,7 @@ fn summary_from_stored_thread(
|
||||
branch: git.branch,
|
||||
origin_url: git.repository_url,
|
||||
});
|
||||
Some(ConversationSummary {
|
||||
ConversationSummary {
|
||||
conversation_id: thread.thread_id,
|
||||
path,
|
||||
preview: thread.first_user_message.unwrap_or(thread.preview),
|
||||
@@ -3734,7 +3726,7 @@ fn summary_from_stored_thread(
|
||||
cli_version: thread.cli_version,
|
||||
source,
|
||||
git_info,
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
|
||||
@@ -409,8 +409,7 @@ mod thread_processor_behavior_tests {
|
||||
history: None,
|
||||
};
|
||||
|
||||
let summary =
|
||||
summary_from_stored_thread(stored_thread, "fallback").expect("summary should exist");
|
||||
let summary = summary_from_stored_thread(stored_thread, "fallback");
|
||||
|
||||
assert_eq!(
|
||||
summary.timestamp.as_deref(),
|
||||
|
||||
@@ -1,5 +1,10 @@
|
||||
use super::*;
|
||||
|
||||
#[cfg(test)]
|
||||
use chrono::DateTime;
|
||||
#[cfg(test)]
|
||||
use chrono::Utc;
|
||||
|
||||
#[cfg(test)]
|
||||
pub(crate) async fn read_summary_from_rollout(
|
||||
path: &Path,
|
||||
@@ -203,6 +208,7 @@ pub(super) fn thread_response_sandbox_policy(
|
||||
sandbox_policy.into()
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
fn parse_datetime(timestamp: Option<&str>) -> Option<DateTime<Utc>> {
|
||||
timestamp.and_then(|ts| {
|
||||
chrono::DateTime::parse_from_rfc3339(ts)
|
||||
@@ -229,6 +235,7 @@ pub(super) fn thread_started_notification(mut thread: Thread) -> ThreadStartedNo
|
||||
ThreadStartedNotification { thread }
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
pub(crate) fn summary_to_thread(
|
||||
summary: ConversationSummary,
|
||||
fallback_cwd: &AbsolutePathBuf,
|
||||
@@ -257,6 +264,7 @@ pub(crate) fn summary_to_thread(
|
||||
AbsolutePathBuf::relative_to_current_dir(path_utils::normalize_for_native_workdir(cwd))
|
||||
.unwrap_or_else(|err| {
|
||||
warn!(
|
||||
conversation_id = %conversation_id,
|
||||
path = %path.display(),
|
||||
"failed to normalize thread cwd while summarizing thread: {err}"
|
||||
);
|
||||
@@ -274,7 +282,7 @@ pub(crate) fn summary_to_thread(
|
||||
created_at: created_at.map(|dt| dt.timestamp()).unwrap_or(0),
|
||||
updated_at: updated_at.map(|dt| dt.timestamp()).unwrap_or(0),
|
||||
status: ThreadStatus::NotLoaded,
|
||||
path: Some(path),
|
||||
path: (!path.as_os_str().is_empty()).then_some(path),
|
||||
cwd,
|
||||
cli_version,
|
||||
agent_nickname: source.get_nickname(),
|
||||
|
||||
Reference in New Issue
Block a user