mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[codex] Omit fork turns from thread started notifications (#19093)
## Why `thread/fork` responses intentionally include copied history so the caller can render the fork immediately, but `thread/started` is a lifecycle notification. The v2 `Thread` contract says notifications should return `turns: []`, and the fork path was reusing the response thread directly, causing copied turns to be emitted through `thread/started` as well. ## What Changed - Route app-server `thread/started` notification construction through a helper that clears `thread.turns` before sending. - Keep `thread/fork` responses unchanged so callers still receive copied history. - Add persistent and ephemeral fork coverage that asserts `thread/started` emits an empty `turns` array while the response retains fork history. ## Testing - `just fmt` - `cargo test -p codex-app-server`
This commit is contained in:
committed by
GitHub
Unverified
parent
0db6811b7c
commit
a3cccbd8ed
@@ -2859,7 +2859,7 @@ impl CodexMessageProcessor {
|
||||
))
|
||||
.await;
|
||||
|
||||
let notif = ThreadStartedNotification { thread };
|
||||
let notif = thread_started_notification(thread);
|
||||
listener_task_context
|
||||
.outgoing
|
||||
.send_server_notification(ServerNotification::ThreadStarted(notif))
|
||||
@@ -5365,7 +5365,7 @@ impl CodexMessageProcessor {
|
||||
.await;
|
||||
}
|
||||
|
||||
let notif = ThreadStartedNotification { thread };
|
||||
let notif = thread_started_notification(thread);
|
||||
self.outgoing
|
||||
.send_server_notification(ServerNotification::ThreadStarted(notif))
|
||||
.await;
|
||||
@@ -7744,7 +7744,7 @@ impl CodexMessageProcessor {
|
||||
.await,
|
||||
/*has_in_progress_turn*/ false,
|
||||
);
|
||||
let notif = ThreadStartedNotification { thread };
|
||||
let notif = thread_started_notification(thread);
|
||||
self.outgoing
|
||||
.send_server_notification(ServerNotification::ThreadStarted(notif))
|
||||
.await;
|
||||
@@ -10094,6 +10094,11 @@ fn build_thread_from_snapshot(
|
||||
}
|
||||
}
|
||||
|
||||
fn thread_started_notification(mut thread: Thread) -> ThreadStartedNotification {
|
||||
thread.turns.clear();
|
||||
ThreadStartedNotification { thread }
|
||||
}
|
||||
|
||||
pub(crate) fn summary_to_thread(
|
||||
summary: ConversationSummary,
|
||||
fallback_cwd: &AbsolutePathBuf,
|
||||
|
||||
@@ -181,9 +181,16 @@ async fn thread_fork_creates_new_thread_and_emits_started() -> Result<()> {
|
||||
Some(&Value::Null),
|
||||
"thread/started must serialize `name: null` when unset"
|
||||
);
|
||||
assert_eq!(
|
||||
started_thread_json.get("turns"),
|
||||
Some(&json!([])),
|
||||
"thread/started must not emit copied fork turns"
|
||||
);
|
||||
let started: ThreadStartedNotification =
|
||||
serde_json::from_value(notif.params.expect("params must be present"))?;
|
||||
assert_eq!(started.thread, thread);
|
||||
let mut expected_started_thread = thread;
|
||||
expected_started_thread.turns.clear();
|
||||
assert_eq!(started.thread, expected_started_thread);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
@@ -582,9 +589,16 @@ async fn thread_fork_ephemeral_remains_pathless_and_omits_listing() -> Result<()
|
||||
Some(true),
|
||||
"thread/started should serialize `ephemeral: true` for ephemeral forks"
|
||||
);
|
||||
assert_eq!(
|
||||
started_thread_json.get("turns"),
|
||||
Some(&json!([])),
|
||||
"thread/started must not emit copied ephemeral fork turns"
|
||||
);
|
||||
let started: ThreadStartedNotification =
|
||||
serde_json::from_value(notif.params.expect("params must be present"))?;
|
||||
assert_eq!(started.thread, thread);
|
||||
let mut expected_started_thread = thread;
|
||||
expected_started_thread.turns.clear();
|
||||
assert_eq!(started.thread, expected_started_thread);
|
||||
|
||||
let list_id = mcp
|
||||
.send_thread_list_request(ThreadListParams {
|
||||
|
||||
Reference in New Issue
Block a user