mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
app-server: ignore persist_extended_history param (#21225)
## Why Taking a step to removing the `persistExtendedHistory` field. It's not scalable to be persisting so much data in the rollout file and returning it in the thread history. When a client explicitly sends `true`, the server now tells that client the parameter is deprecated and ignored so the caller has a clear migration signal via the `deprecationNotice` notification. ## What changed - Keep the `persist_extended_history` / `persistExtendedHistory` field in the v2 protocol for compatibility, but document it as deprecated and ignored. - Ignore the parameter in app-server `thread/start`, `thread/resume`, and `thread/fork`; those paths always use limited history persistence now. - Stop treating `persistExtendedHistory` as a running-thread resume override mismatch. - Emit a connection-scoped `deprecationNotice` when a request explicitly sets `persist_extended_history: true`. ## Verification - Added `thread_start_deprecates_persist_extended_history_true` to cover the deprecation notice. - `cargo test -p codex-app-server` - `cargo test -p codex-app-server-protocol`
This commit is contained in:
@@ -3841,9 +3841,9 @@ pub struct ThreadStartParams {
|
||||
#[experimental("thread/start.experimentalRawEvents")]
|
||||
#[serde(default)]
|
||||
pub experimental_raw_events: bool,
|
||||
/// If true, persist additional EventMsg variants to the rollout file.
|
||||
/// However, `thread/read`, `thread/resume`, and `thread/fork` still only
|
||||
/// return the limited form of thread history for scalability reasons.
|
||||
/// Deprecated and ignored by app-server. Kept only so older clients can
|
||||
/// continue sending the field while rollout persistence always uses the
|
||||
/// limited history policy.
|
||||
#[experimental("thread/start.persistFullHistory")]
|
||||
#[serde(default)]
|
||||
pub persist_extended_history: bool,
|
||||
@@ -3973,9 +3973,9 @@ pub struct ThreadResumeParams {
|
||||
#[experimental("thread/resume.excludeTurns")]
|
||||
#[serde(default, skip_serializing_if = "std::ops::Not::not")]
|
||||
pub exclude_turns: bool,
|
||||
/// If true, persist additional EventMsg variants to the rollout file.
|
||||
/// However, `thread/read`, `thread/resume`, and `thread/fork` still only
|
||||
/// return the limited form of thread history for scalability reasons.
|
||||
/// Deprecated and ignored by app-server. Kept only so older clients can
|
||||
/// continue sending the field while rollout persistence always uses the
|
||||
/// limited history policy.
|
||||
#[experimental("thread/resume.persistFullHistory")]
|
||||
#[serde(default)]
|
||||
pub persist_extended_history: bool,
|
||||
@@ -4079,9 +4079,9 @@ pub struct ThreadForkParams {
|
||||
#[experimental("thread/fork.excludeTurns")]
|
||||
#[serde(default, skip_serializing_if = "std::ops::Not::not")]
|
||||
pub exclude_turns: bool,
|
||||
/// If true, persist additional EventMsg variants to the rollout file.
|
||||
/// However, `thread/read`, `thread/resume`, and `thread/fork` still only
|
||||
/// return the limited form of thread history for scalability reasons.
|
||||
/// Deprecated and ignored by app-server. Kept only so older clients can
|
||||
/// continue sending the field while rollout persistence always uses the
|
||||
/// limited history policy.
|
||||
#[experimental("thread/fork.persistFullHistory")]
|
||||
#[serde(default)]
|
||||
pub persist_extended_history: bool,
|
||||
|
||||
@@ -52,6 +52,7 @@ use codex_app_server_protocol::CommandExecWriteParams;
|
||||
use codex_app_server_protocol::ConfigWarningNotification;
|
||||
use codex_app_server_protocol::ConversationGitInfo;
|
||||
use codex_app_server_protocol::ConversationSummary;
|
||||
use codex_app_server_protocol::DeprecationNoticeNotification;
|
||||
use codex_app_server_protocol::DynamicToolSpec as ApiDynamicToolSpec;
|
||||
use codex_app_server_protocol::ExperimentalFeature as ApiExperimentalFeature;
|
||||
use codex_app_server_protocol::ExperimentalFeatureListParams;
|
||||
|
||||
@@ -2,6 +2,10 @@ use super::*;
|
||||
|
||||
const THREAD_LIST_DEFAULT_LIMIT: usize = 25;
|
||||
const THREAD_LIST_MAX_LIMIT: usize = 100;
|
||||
const PERSIST_EXTENDED_HISTORY_DEPRECATION_SUMMARY: &str =
|
||||
"persistExtendedHistory is deprecated and ignored";
|
||||
const PERSIST_EXTENDED_HISTORY_DEPRECATION_DETAILS: &str =
|
||||
"Remove this parameter. App-server always uses limited history persistence.";
|
||||
|
||||
struct ThreadListFilters {
|
||||
model_providers: Option<Vec<String>>,
|
||||
@@ -121,12 +125,6 @@ fn collect_resume_override_mismatches(
|
||||
"developerInstructions override was provided and ignored while running".to_string(),
|
||||
);
|
||||
}
|
||||
if request.persist_extended_history {
|
||||
mismatch_details.push(
|
||||
"persistExtendedHistory override was provided and ignored while running".to_string(),
|
||||
);
|
||||
}
|
||||
|
||||
mismatch_details
|
||||
}
|
||||
|
||||
@@ -750,6 +748,10 @@ impl ThreadRequestProcessor {
|
||||
"`permissions` cannot be combined with `sandbox`",
|
||||
));
|
||||
}
|
||||
if persist_extended_history {
|
||||
self.send_persist_extended_history_deprecation_notice(request_id.connection_id)
|
||||
.await;
|
||||
}
|
||||
let environment_selections = self.parse_environment_selections(environments)?;
|
||||
let mut typesafe_overrides = self.build_thread_config_overrides(
|
||||
model,
|
||||
@@ -792,7 +794,6 @@ impl ThreadRequestProcessor {
|
||||
dynamic_tools,
|
||||
session_start_source,
|
||||
environment_selections,
|
||||
persist_extended_history,
|
||||
service_name,
|
||||
experimental_raw_events,
|
||||
request_trace,
|
||||
@@ -841,6 +842,18 @@ impl ThreadRequestProcessor {
|
||||
self.outgoing.request_trace_context(request_id).await
|
||||
}
|
||||
|
||||
async fn send_persist_extended_history_deprecation_notice(&self, connection_id: ConnectionId) {
|
||||
self.outgoing
|
||||
.send_server_notification_to_connections(
|
||||
&[connection_id],
|
||||
ServerNotification::DeprecationNotice(DeprecationNoticeNotification {
|
||||
summary: PERSIST_EXTENDED_HISTORY_DEPRECATION_SUMMARY.to_string(),
|
||||
details: Some(PERSIST_EXTENDED_HISTORY_DEPRECATION_DETAILS.to_string()),
|
||||
}),
|
||||
)
|
||||
.await;
|
||||
}
|
||||
|
||||
async fn submit_core_op(
|
||||
&self,
|
||||
request_id: &ConnectionRequestId,
|
||||
@@ -864,7 +877,6 @@ impl ThreadRequestProcessor {
|
||||
dynamic_tools: Option<Vec<ApiDynamicToolSpec>>,
|
||||
session_start_source: Option<codex_app_server_protocol::ThreadStartSource>,
|
||||
environments: Option<Vec<TurnEnvironmentSelection>>,
|
||||
persist_extended_history: bool,
|
||||
service_name: Option<String>,
|
||||
experimental_raw_events: bool,
|
||||
request_trace: Option<W3cTraceContext>,
|
||||
@@ -981,7 +993,7 @@ impl ThreadRequestProcessor {
|
||||
},
|
||||
session_source: None,
|
||||
dynamic_tools: core_dynamic_tools,
|
||||
persist_extended_history,
|
||||
persist_extended_history: false,
|
||||
metrics_service_name: service_name,
|
||||
parent_trace: request_trace,
|
||||
environments,
|
||||
@@ -990,7 +1002,7 @@ impl ThreadRequestProcessor {
|
||||
"app_server.thread_start.create_thread",
|
||||
otel.name = "app_server.thread_start.create_thread",
|
||||
thread_start.dynamic_tool_count = core_dynamic_tool_count,
|
||||
thread_start.persist_extended_history = persist_extended_history,
|
||||
thread_start.persist_extended_history = false,
|
||||
))
|
||||
.await
|
||||
.map_err(|err| match err {
|
||||
@@ -2202,6 +2214,10 @@ impl ThreadRequestProcessor {
|
||||
.await;
|
||||
return Ok(());
|
||||
}
|
||||
if params.persist_extended_history {
|
||||
self.send_persist_extended_history_deprecation_notice(request_id.connection_id)
|
||||
.await;
|
||||
}
|
||||
|
||||
let _thread_list_state_permit = match self.acquire_thread_list_state_permit().await {
|
||||
Ok(permit) => permit,
|
||||
@@ -2236,7 +2252,7 @@ impl ThreadRequestProcessor {
|
||||
developer_instructions,
|
||||
personality,
|
||||
exclude_turns,
|
||||
persist_extended_history,
|
||||
persist_extended_history: _persist_extended_history,
|
||||
} = params;
|
||||
let include_turns = !exclude_turns;
|
||||
|
||||
@@ -2300,7 +2316,7 @@ impl ThreadRequestProcessor {
|
||||
config.clone(),
|
||||
thread_history,
|
||||
self.auth_manager.clone(),
|
||||
persist_extended_history,
|
||||
/*persist_extended_history*/ false,
|
||||
self.request_trace_context(&request_id).await,
|
||||
)
|
||||
.await
|
||||
@@ -2836,6 +2852,10 @@ impl ThreadRequestProcessor {
|
||||
"`permissions` cannot be combined with `sandbox`",
|
||||
));
|
||||
}
|
||||
if persist_extended_history {
|
||||
self.send_persist_extended_history_deprecation_notice(request_id.connection_id)
|
||||
.await;
|
||||
}
|
||||
|
||||
let source_thread = self
|
||||
.read_stored_thread_for_resume(&thread_id, path.as_ref(), /*include_history*/ true)
|
||||
@@ -2913,7 +2933,7 @@ impl ThreadRequestProcessor {
|
||||
history: history_items.clone(),
|
||||
rollout_path: source_thread.rollout_path.clone(),
|
||||
}),
|
||||
persist_extended_history,
|
||||
/*persist_extended_history*/ false,
|
||||
self.request_trace_context(&request_id).await,
|
||||
)
|
||||
.await
|
||||
|
||||
@@ -6,6 +6,7 @@ use app_test_support::create_mock_responses_server_repeating_assistant;
|
||||
use app_test_support::to_response;
|
||||
use app_test_support::write_chatgpt_auth;
|
||||
use codex_app_server_protocol::AskForApproval;
|
||||
use codex_app_server_protocol::DeprecationNoticeNotification;
|
||||
use codex_app_server_protocol::JSONRPCError;
|
||||
use codex_app_server_protocol::JSONRPCMessage;
|
||||
use codex_app_server_protocol::JSONRPCResponse;
|
||||
@@ -50,6 +51,46 @@ use super::analytics::wait_for_analytics_payload;
|
||||
const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10);
|
||||
const INVALID_REQUEST_ERROR_CODE: i64 = -32600;
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_start_deprecates_persist_extended_history_true() -> Result<()> {
|
||||
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
||||
let codex_home = TempDir::new()?;
|
||||
create_config_toml_without_approval_policy(codex_home.path(), &server.uri())?;
|
||||
|
||||
let mut mcp = McpProcess::new(codex_home.path()).await?;
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||
|
||||
let req_id = mcp
|
||||
.send_thread_start_request(ThreadStartParams {
|
||||
persist_extended_history: true,
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
|
||||
let notification = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_notification_message("deprecationNotice"),
|
||||
)
|
||||
.await??;
|
||||
let notice: DeprecationNoticeNotification = serde_json::from_value(
|
||||
notification
|
||||
.params
|
||||
.expect("deprecationNotice params should be present"),
|
||||
)?;
|
||||
assert_eq!(
|
||||
notice.summary,
|
||||
"persistExtendedHistory is deprecated and ignored"
|
||||
);
|
||||
|
||||
timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(req_id)),
|
||||
)
|
||||
.await??;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_start_creates_thread_and_emits_started() -> Result<()> {
|
||||
// Provide a mock server and config so model wiring is valid.
|
||||
|
||||
Reference in New Issue
Block a user