mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Fix empty rollout path app-server handling (#23400)
## Summary - Coerce `path: ""` to `None` at the v2 protocol params deserialization boundary for `thread/resume` and `thread/fork`. - Restore the pre-ThreadStore running-thread resume behavior: if `threadId` is already running, rejoin it by id and treat a non-empty `path` only as a consistency check; otherwise cold resume keeps `history > path > threadId` precedence. - Add protocol, resume, and fork regression coverage for empty path payloads; refresh app-server schema fixtures for the clarified params docs. ## Tests - `just fmt` - `just write-app-server-schema` - `cargo test -p codex-app-server-protocol thread_path_params_deserialize_empty_path_as_none` - `cargo test -p codex-app-server-protocol --test schema_fixtures` - `cargo test -p codex-app-server empty_path` - `RUST_MIN_STACK=8388608 cargo test -p codex-app-server --test all thread_resume_rejects_mismatched_path_for_running_thread_id` - `RUST_MIN_STACK=8388608 cargo test -p codex-app-server --test all thread_resume_uses_path_over_non_running_thread_id`
This commit is contained in:
@@ -1,8 +1,18 @@
|
||||
use std::path::PathBuf;
|
||||
|
||||
use serde::Deserialize;
|
||||
use serde::Deserializer;
|
||||
use serde::Serialize;
|
||||
use serde::Serializer;
|
||||
|
||||
pub fn deserialize_empty_path_as_none<'de, D>(deserializer: D) -> Result<Option<PathBuf>, D::Error>
|
||||
where
|
||||
D: Deserializer<'de>,
|
||||
{
|
||||
let path = Option::<PathBuf>::deserialize(deserializer)?;
|
||||
Ok(path.filter(|path| !path.as_os_str().is_empty()))
|
||||
}
|
||||
|
||||
pub fn deserialize_double_option<'de, T, D>(deserializer: D) -> Result<Option<Option<T>>, D::Error>
|
||||
where
|
||||
T: Deserialize<'de>,
|
||||
|
||||
@@ -661,6 +661,33 @@ fn permission_profile_selection_uses_id_string() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn thread_path_params_deserialize_empty_path_as_none() {
|
||||
let resume: ThreadResumeParams = serde_json::from_value(json!({
|
||||
"threadId": "thread-1",
|
||||
"path": "",
|
||||
}))
|
||||
.expect("thread/resume params deserialize");
|
||||
assert_eq!(resume.path, None);
|
||||
|
||||
let fork: ThreadForkParams = serde_json::from_value(json!({
|
||||
"threadId": "thread-1",
|
||||
"path": "",
|
||||
}))
|
||||
.expect("thread/fork params deserialize");
|
||||
assert_eq!(fork.path, None);
|
||||
|
||||
let resume_with_path: ThreadResumeParams = serde_json::from_value(json!({
|
||||
"threadId": "thread-1",
|
||||
"path": "/tmp/resume-thread.jsonl",
|
||||
}))
|
||||
.expect("thread/resume params deserialize");
|
||||
assert_eq!(
|
||||
resume_with_path.path,
|
||||
Some(PathBuf::from("/tmp/resume-thread.jsonl"))
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn fs_get_metadata_response_round_trips_minimal_fields() {
|
||||
let response = FsGetMetadataResponse {
|
||||
|
||||
@@ -229,8 +229,13 @@ pub struct ThreadStartResponse {
|
||||
/// 2. By history: instantiate the thread from memory and resume it.
|
||||
/// 3. By path: load the thread from disk by path and resume it.
|
||||
///
|
||||
/// The precedence is: history > path > thread_id.
|
||||
/// If using history or path, the thread_id param will be ignored.
|
||||
/// For non-running threads, the precedence is: history > non-empty path > thread_id.
|
||||
/// If using history or a non-empty path for a non-running thread, the thread_id
|
||||
/// param will be ignored.
|
||||
///
|
||||
/// If thread_id identifies a running thread, app-server rejoins that thread and
|
||||
/// treats a non-empty path as a consistency check against the active rollout path.
|
||||
/// Empty string path values are treated as absent.
|
||||
///
|
||||
/// Prefer using thread_id whenever possible.
|
||||
pub struct ThreadResumeParams {
|
||||
@@ -244,8 +249,14 @@ pub struct ThreadResumeParams {
|
||||
pub history: Option<Vec<ResponseItem>>,
|
||||
|
||||
/// [UNSTABLE] Specify the rollout path to resume from.
|
||||
/// If specified, the thread_id param will be ignored.
|
||||
/// If specified for a non-running thread, the thread_id param will be ignored.
|
||||
/// If thread_id identifies a running thread, the path must match the active
|
||||
/// rollout path.
|
||||
#[experimental("thread/resume.path")]
|
||||
#[serde(
|
||||
default,
|
||||
deserialize_with = "crate::protocol::serde_helpers::deserialize_empty_path_as_none"
|
||||
)]
|
||||
#[ts(optional = nullable)]
|
||||
pub path: Option<PathBuf>,
|
||||
|
||||
@@ -346,7 +357,8 @@ pub struct ThreadResumeResponse {
|
||||
/// 1. By thread_id: load the thread from disk by thread_id and fork it into a new thread.
|
||||
/// 2. By path: load the thread from disk by path and fork it into a new thread.
|
||||
///
|
||||
/// If using path, the thread_id param will be ignored.
|
||||
/// If using a non-empty path, the thread_id param will be ignored.
|
||||
/// Empty string path values are treated as absent.
|
||||
///
|
||||
/// Prefer using thread_id whenever possible.
|
||||
pub struct ThreadForkParams {
|
||||
@@ -355,6 +367,10 @@ pub struct ThreadForkParams {
|
||||
/// [UNSTABLE] Specify the rollout path to fork from.
|
||||
/// If specified, the thread_id param will be ignored.
|
||||
#[experimental("thread/fork.path")]
|
||||
#[serde(
|
||||
default,
|
||||
deserialize_with = "crate::protocol::serde_helpers::deserialize_empty_path_as_none"
|
||||
)]
|
||||
#[ts(optional = nullable)]
|
||||
pub path: Option<PathBuf>,
|
||||
|
||||
|
||||
Reference in New Issue
Block a user