diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index ea897239537d..a8e086b4924b 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2507,6 +2507,7 @@ dependencies = [ "clap_complete", "codex-api", "codex-app-server", + "codex-app-server-client", "codex-app-server-daemon", "codex-app-server-protocol", "codex-app-server-test-client", diff --git a/codex-rs/app-server-protocol/schema/json/ClientRequest.json b/codex-rs/app-server-protocol/schema/json/ClientRequest.json index 458e63efca2f..156379baf5fd 100644 --- a/codex-rs/app-server-protocol/schema/json/ClientRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ClientRequest.json @@ -4409,6 +4409,17 @@ }, "ThreadGoalClearParams": { "properties": { + "origin": { + "anyOf": [ + { + "$ref": "#/definitions/ThreadGoalMutationOrigin" + }, + { + "type": "null" + } + ], + "description": "Missing provenance does not supply user authorization." + }, "threadId": { "type": "string" } @@ -4429,6 +4440,14 @@ ], "type": "object" }, + "ThreadGoalMutationOrigin": { + "description": "Distinguishes explicit user actions from automatic goal lifecycle mutations.", + "enum": [ + "user", + "automatic" + ], + "type": "string" + }, "ThreadGoalSetParams": { "properties": { "objective": { @@ -4437,6 +4456,17 @@ "null" ] }, + "origin": { + "anyOf": [ + { + "$ref": "#/definitions/ThreadGoalMutationOrigin" + }, + { + "type": "null" + } + ], + "description": "Missing provenance does not supply user authorization." + }, "status": { "anyOf": [ { diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index 663cc473da03..04ae2b92867b 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -21560,6 +21560,17 @@ "ThreadGoalClearParams": { "$schema": "http://json-schema.org/draft-07/schema#", "properties": { + "origin": { + "anyOf": [ + { + "$ref": "#/definitions/v2/ThreadGoalMutationOrigin" + }, + { + "type": "null" + } + ], + "description": "Missing provenance does not supply user authorization." + }, "threadId": { "type": "string" } @@ -21626,6 +21637,14 @@ "title": "ThreadGoalGetResponse", "type": "object" }, + "ThreadGoalMutationOrigin": { + "description": "Distinguishes explicit user actions from automatic goal lifecycle mutations.", + "enum": [ + "user", + "automatic" + ], + "type": "string" + }, "ThreadGoalSetParams": { "$schema": "http://json-schema.org/draft-07/schema#", "properties": { @@ -21635,6 +21654,17 @@ "null" ] }, + "origin": { + "anyOf": [ + { + "$ref": "#/definitions/v2/ThreadGoalMutationOrigin" + }, + { + "type": "null" + } + ], + "description": "Missing provenance does not supply user authorization." + }, "status": { "anyOf": [ { diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index 8856accc957b..fcae8cbdd3e5 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -19274,6 +19274,17 @@ "ThreadGoalClearParams": { "$schema": "http://json-schema.org/draft-07/schema#", "properties": { + "origin": { + "anyOf": [ + { + "$ref": "#/definitions/ThreadGoalMutationOrigin" + }, + { + "type": "null" + } + ], + "description": "Missing provenance does not supply user authorization." + }, "threadId": { "type": "string" } @@ -19340,6 +19351,14 @@ "title": "ThreadGoalGetResponse", "type": "object" }, + "ThreadGoalMutationOrigin": { + "description": "Distinguishes explicit user actions from automatic goal lifecycle mutations.", + "enum": [ + "user", + "automatic" + ], + "type": "string" + }, "ThreadGoalSetParams": { "$schema": "http://json-schema.org/draft-07/schema#", "properties": { @@ -19349,6 +19368,17 @@ "null" ] }, + "origin": { + "anyOf": [ + { + "$ref": "#/definitions/ThreadGoalMutationOrigin" + }, + { + "type": "null" + } + ], + "description": "Missing provenance does not supply user authorization." + }, "status": { "anyOf": [ { diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadGoalClearParams.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadGoalClearParams.json index 99b32a7316b1..ffef6d9d3256 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadGoalClearParams.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadGoalClearParams.json @@ -1,6 +1,27 @@ { "$schema": "http://json-schema.org/draft-07/schema#", + "definitions": { + "ThreadGoalMutationOrigin": { + "description": "Distinguishes explicit user actions from automatic goal lifecycle mutations.", + "enum": [ + "user", + "automatic" + ], + "type": "string" + } + }, "properties": { + "origin": { + "anyOf": [ + { + "$ref": "#/definitions/ThreadGoalMutationOrigin" + }, + { + "type": "null" + } + ], + "description": "Missing provenance does not supply user authorization." + }, "threadId": { "type": "string" } diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadGoalSetParams.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadGoalSetParams.json index 0087827c2138..b1aad0e2367c 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadGoalSetParams.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadGoalSetParams.json @@ -1,6 +1,14 @@ { "$schema": "http://json-schema.org/draft-07/schema#", "definitions": { + "ThreadGoalMutationOrigin": { + "description": "Distinguishes explicit user actions from automatic goal lifecycle mutations.", + "enum": [ + "user", + "automatic" + ], + "type": "string" + }, "ThreadGoalStatus": { "enum": [ "active", @@ -20,6 +28,17 @@ "null" ] }, + "origin": { + "anyOf": [ + { + "$ref": "#/definitions/ThreadGoalMutationOrigin" + }, + { + "type": "null" + } + ], + "description": "Missing provenance does not supply user authorization." + }, "status": { "anyOf": [ { diff --git a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-experimental.json.zst b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-experimental.json.zst index 2c53aa178308..9e6ab232b723 100644 Binary files a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-experimental.json.zst and b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-experimental.json.zst differ diff --git a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst index 1efda781c923..6de2c0c5460b 100644 Binary files a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst and b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst differ diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ThreadGoalClearParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ThreadGoalClearParams.ts index efefc2538af4..1e944736d45c 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ThreadGoalClearParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ThreadGoalClearParams.ts @@ -1,5 +1,10 @@ // GENERATED CODE! DO NOT MODIFY BY HAND! // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +import type { ThreadGoalMutationOrigin } from "./ThreadGoalMutationOrigin"; -export type ThreadGoalClearParams = { threadId: string, }; +export type ThreadGoalClearParams = { threadId: string, +/** + * Missing provenance does not supply user authorization. + */ +origin?: ThreadGoalMutationOrigin | null, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ThreadGoalMutationOrigin.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ThreadGoalMutationOrigin.ts new file mode 100644 index 000000000000..742419cba3d2 --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ThreadGoalMutationOrigin.ts @@ -0,0 +1,8 @@ +// GENERATED CODE! DO NOT MODIFY BY HAND! + +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. + +/** + * Distinguishes explicit user actions from automatic goal lifecycle mutations. + */ +export type ThreadGoalMutationOrigin = "user" | "automatic"; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/ThreadGoalSetParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/ThreadGoalSetParams.ts index b92720c17872..d6edcbddfd47 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/ThreadGoalSetParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/ThreadGoalSetParams.ts @@ -1,6 +1,11 @@ // GENERATED CODE! DO NOT MODIFY BY HAND! // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +import type { ThreadGoalMutationOrigin } from "./ThreadGoalMutationOrigin"; import type { ThreadGoalStatus } from "./ThreadGoalStatus"; -export type ThreadGoalSetParams = { threadId: string, objective?: string | null, status?: ThreadGoalStatus | null, tokenBudget?: number | null, }; +export type ThreadGoalSetParams = { threadId: string, +/** + * Missing provenance does not supply user authorization. + */ +origin?: ThreadGoalMutationOrigin | null, objective?: string | null, status?: ThreadGoalStatus | null, tokenBudget?: number | null, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts index b96f29ce44bb..24dc8f30a757 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts @@ -505,6 +505,7 @@ export type { ThreadGoalClearResponse } from "./ThreadGoalClearResponse"; export type { ThreadGoalClearedNotification } from "./ThreadGoalClearedNotification"; export type { ThreadGoalGetParams } from "./ThreadGoalGetParams"; export type { ThreadGoalGetResponse } from "./ThreadGoalGetResponse"; +export type { ThreadGoalMutationOrigin } from "./ThreadGoalMutationOrigin"; export type { ThreadGoalSetParams } from "./ThreadGoalSetParams"; export type { ThreadGoalSetResponse } from "./ThreadGoalSetResponse"; export type { ThreadGoalStatus } from "./ThreadGoalStatus"; diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index c6d6fad791b8..1aec8bbd4540 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -2522,6 +2522,7 @@ mod tests { let thread_goal_set = ClientRequest::ThreadGoalSet { request_id: request_id(), params: v2::ThreadGoalSetParams { + origin: None, thread_id: "goal-thread".to_string(), objective: Some("ship it".to_string()), status: None, @@ -4440,6 +4441,7 @@ mod tests { let set_request = ClientRequest::ThreadGoalSet { request_id: RequestId::Integer(1), params: v2::ThreadGoalSetParams { + origin: None, thread_id: "thr_123".to_string(), objective: Some("ship goal mode".to_string()), status: Some(v2::ThreadGoalStatus::Active), @@ -4455,6 +4457,7 @@ mod tests { let clear_request = ClientRequest::ThreadGoalClear { request_id: RequestId::Integer(3), params: v2::ThreadGoalClearParams { + origin: None, thread_id: "thr_123".to_string(), }, }; diff --git a/codex-rs/app-server-protocol/src/protocol/v2/thread.rs b/codex-rs/app-server-protocol/src/protocol/v2/thread.rs index 2e166d5506b1..9eee37dcac97 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/thread.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/thread.rs @@ -841,11 +841,24 @@ impl From for ThreadGoal { } } +/// Distinguishes explicit user actions from automatic goal lifecycle mutations. +#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub enum ThreadGoalMutationOrigin { + User, + Automatic, +} + #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] pub struct ThreadGoalSetParams { pub thread_id: String, + /// Missing provenance does not supply user authorization. + #[ts(optional = nullable)] + pub origin: Option, #[ts(optional = nullable)] pub objective: Option, #[ts(optional = nullable)] @@ -886,6 +899,9 @@ pub struct ThreadGoalGetResponse { #[ts(export_to = "v2/")] pub struct ThreadGoalClearParams { pub thread_id: String, + /// Missing provenance does not supply user authorization. + #[ts(optional = nullable)] + pub origin: Option, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] diff --git a/codex-rs/app-server/src/request_processors/thread_goal_processor.rs b/codex-rs/app-server/src/request_processors/thread_goal_processor.rs index 6d52f5b52f46..bada44de510d 100644 --- a/codex-rs/app-server/src/request_processors/thread_goal_processor.rs +++ b/codex-rs/app-server/src/request_processors/thread_goal_processor.rs @@ -2,6 +2,8 @@ use super::thread_input::DIRECT_INPUT_TO_MULTI_AGENT_V2_SUBAGENT_ERROR; use super::thread_input::can_accept_direct_input; use super::thread_input::ensure_direct_input_allowed; use super::*; +use codex_app_server_protocol::ThreadGoalMutationOrigin; +use codex_core::context::UserGoalUpdate; use codex_goal_extension::GoalObjectiveUpdate; use codex_goal_extension::GoalService; use codex_goal_extension::GoalServiceError; @@ -9,9 +11,11 @@ use codex_goal_extension::GoalSetRequest; use codex_goal_extension::GoalTokenBudgetUpdate; use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::SubAgentSource; -use codex_protocol::protocol::ThreadSettingsAppliedEvent; -use codex_protocol::protocol::ThreadSettingsSnapshot; use codex_rollout::RolloutRecorder; +use codex_rollout::WriterLockCoordinator; + +#[path = "thread_goal_user_context.rs"] +mod user_context; enum GoalAccess { Read, @@ -27,6 +31,7 @@ pub(crate) struct ThreadGoalRequestProcessor { state_db: Option, goal_service: Arc, config_manager: ConfigManager, + writer_locks: Arc, } impl ThreadGoalRequestProcessor { @@ -39,6 +44,7 @@ impl ThreadGoalRequestProcessor { goal_service: Arc, config_manager: ConfigManager, ) -> Self { + let writer_locks = Arc::new(WriterLockCoordinator::new(&config.codex_home)); Self { thread_manager, outgoing, @@ -47,6 +53,7 @@ impl ThreadGoalRequestProcessor { state_db, goal_service, config_manager, + writer_locks, } } @@ -133,6 +140,7 @@ impl ThreadGoalRequestProcessor { } let thread_id = parse_thread_id_for_request(params.thread_id.as_str())?; + let _goal_resume_guard = self.thread_state_manager.lock_goal_resume(thread_id).await; let state_db = self .state_db_for_materialized_thread(thread_id, GoalAccess::Mutate) .await?; @@ -169,7 +177,7 @@ impl ThreadGoalRequestProcessor { thread_state.listener_command_tx() }; let status = params.status.map(ThreadGoalStatus::to_core); - let objective = params.objective.as_deref(); + let objective = params.objective.as_deref().map(str::trim); let outcome = self .goal_service @@ -187,45 +195,44 @@ impl ThreadGoalRequestProcessor { }, max_goal_token_budget, }, + Box::pin(async { + if params.origin == Some(ThreadGoalMutationOrigin::User) + && (objective.is_some() || status.is_some()) + { + self.record_user_goal_update( + thread_id, + UserGoalUpdate::Set { + objective: objective.map(str::to_owned), + status, + }, + ) + .await?; + } + Ok(()) + }), ) .await .map_err(goal_service_error)?; let goal = ThreadGoal::from(outcome.goal.clone()); let persist_result = match self.thread_manager.get_thread(thread_id).await { - Ok(thread) => match thread.rollout_path() { - Some(path) if codex_rollout::existing_rollout_path(&path).await.is_none() => { - // Goal-first threads need their settings captured when the goal creates the - // rollout. Once materialized, normal settings updates own this event. - let persisted_settings = thread.thread_settings_snapshot().await; - let items = [ - thread_settings_applied_item(thread_id, persisted_settings.clone()), - outcome.thread_goal_updated_item(), - ]; - match thread.append_rollout_items(&items).await { - Err(err) => Err(err), - Ok(()) => { - // Catch up a settings update queued while the rollout materialized. - let current_settings = thread.thread_settings_snapshot().await; - if current_settings == persisted_settings { - Ok(()) - } else { - thread - .append_rollout_items(&[thread_settings_applied_item( - thread_id, - current_settings, - )]) - .await - } - } + Ok(thread) => { + let needs_settings_checkpoint = match thread.rollout_path() { + Some(path) => codex_rollout::existing_rollout_path(&path).await.is_none(), + None => false, + }; + match thread + .append_rollout_items(&[outcome.thread_goal_updated_item()]) + .await + { + // Automatic and legacy callers still need goal-first settings persisted, + // even though they do not record a user instruction. + Ok(()) if needs_settings_checkpoint => { + thread.checkpoint_thread_settings().await } + result => result, } - Some(_) | None => { - thread - .append_rollout_items(&[outcome.thread_goal_updated_item()]) - .await - } - }, + } Err(_) => Ok(()), }; if let Err(err) = persist_result { @@ -275,6 +282,7 @@ impl ThreadGoalRequestProcessor { } let thread_id = parse_thread_id_for_request(params.thread_id.as_str())?; + let _goal_resume_guard = self.thread_state_manager.lock_goal_resume(thread_id).await; let state_db = self .state_db_for_materialized_thread(thread_id, GoalAccess::Mutate) .await?; @@ -288,7 +296,17 @@ impl ThreadGoalRequestProcessor { }; let cleared = self .goal_service - .clear_thread_goal(&state_db, thread_id) + .clear_thread_goal( + &state_db, + thread_id, + Box::pin(async { + if params.origin == Some(ThreadGoalMutationOrigin::User) { + self.record_user_goal_update(thread_id, UserGoalUpdate::Clear) + .await?; + } + Ok(()) + }), + ) .await .map_err(goal_service_error)?; @@ -504,18 +522,6 @@ impl ThreadGoalRequestProcessor { } } -fn thread_settings_applied_item( - thread_id: ThreadId, - thread_settings: ThreadSettingsSnapshot, -) -> RolloutItem { - RolloutItem::EventMsg(EventMsg::ThreadSettingsApplied( - ThreadSettingsAppliedEvent { - thread_id: Some(thread_id), - thread_settings, - }, - )) -} - pub(super) fn api_thread_goal_from_state(goal: codex_state::ThreadGoal) -> ThreadGoal { ThreadGoal { thread_id: goal.thread_id.to_string(), diff --git a/codex-rs/app-server/src/request_processors/thread_goal_user_context.rs b/codex-rs/app-server/src/request_processors/thread_goal_user_context.rs new file mode 100644 index 000000000000..c27c4a21ce59 --- /dev/null +++ b/codex-rs/app-server/src/request_processors/thread_goal_user_context.rs @@ -0,0 +1,51 @@ +//! Persists explicit user goal mutations with user provenance before goal work resumes. +//! Unloaded threads receive the same item on replay; tool-created goals never use this path. + +use super::*; +use codex_core::context::ContextualUserFragment; +use codex_core::context::UserGoalUpdate; +use codex_rollout::RolloutRecorderParams; + +impl ThreadGoalRequestProcessor { + pub(super) async fn record_user_goal_update( + &self, + thread_id: ThreadId, + update: UserGoalUpdate, + ) -> Result<(), GoalServiceError> { + if let Ok(thread) = self.thread_manager.get_thread(thread_id).await { + return thread.record_user_goal_update(update).await.map_err(|err| { + GoalServiceError::Internal(format!("failed to record goal instruction: {err}")) + }); + } + let item = ContextualUserFragment::into(update); + let record_error = + |err| GoalServiceError::Internal(format!("failed to record goal instruction: {err}")); + let writer_lock = Arc::new(self.writer_locks.acquire(thread_id).map_err(record_error)?); + let path = codex_rollout::find_thread_path_by_id_str( + &self.config.codex_home, + &thread_id.to_string(), + self.state_db.as_deref(), + ) + .await + .map_err(|err| { + GoalServiceError::Internal(format!("failed to locate thread id {thread_id}: {err}")) + })? + .ok_or_else(|| { + GoalServiceError::InvalidRequest(format!("thread not found: {thread_id}")) + })?; + // The writer owns the lock through background IO, even if this RPC is cancelled. + let recorder = RolloutRecorder::new_with_writer_lock( + self.config.as_ref(), + RolloutRecorderParams::resume(path), + writer_lock, + ) + .await + .map_err(record_error)?; + recorder + .record_canonical_items(&[RolloutItem::ResponseItem(item.into())]) + .await + .map_err(record_error)?; + // Shutdown flushes queued items before acknowledging the goal mutation. + recorder.shutdown().await.map_err(record_error) + } +} diff --git a/codex-rs/app-server/src/request_processors/thread_processor.rs b/codex-rs/app-server/src/request_processors/thread_processor.rs index 289cb6395366..e36ca147e741 100644 --- a/codex-rs/app-server/src/request_processors/thread_processor.rs +++ b/codex-rs/app-server/src/request_processors/thread_processor.rs @@ -3749,6 +3749,17 @@ impl ThreadRequestProcessor { } }; let (thread_history, resume_source_thread) = resume_result?; + // Path-based resume can use an empty request thread ID. Coordinate once its real + // identity is known; unrelated loaded threads never wait for this cold startup. + let _goal_resume_guard = if let InitialHistory::Resumed(resumed) = &thread_history { + Some( + self.thread_state_manager + .lock_goal_resume(resumed.conversation_id) + .await, + ) + } else { + None + }; if let InitialHistory::Resumed(resumed) = &thread_history && self .pending_thread_unloads @@ -3948,7 +3959,8 @@ impl ThreadRequestProcessor { let mut config = match prepared_config.take() { Some(prepared) if prepared.state == config_state => prepared.config, _ => { - // Config loading can call back into Desktop; release the permit during host work. + // Config loading can call back into Desktop; release both locks during host work. + drop(_goal_resume_guard); drop(_thread_list_state_permit); let config = self .config_manager @@ -4586,6 +4598,7 @@ impl ThreadRequestProcessor { .await .map_err(thread_store_resume_read_error)?; let history = InitialHistory::Resumed(ResumedHistory { + history_revision: model_context.revision, conversation_id: model_context.thread_id, history: Arc::new(model_context.items), rollout_path: stored_thread.rollout_path.clone(), @@ -4680,18 +4693,15 @@ impl ThreadRequestProcessor { stored_thread: &mut StoredThread, ) -> Result { let thread_id = stored_thread.thread_id; - let history = stored_thread - .history - .take() - .map(|history| history.items) - .ok_or_else(|| { - internal_error(format!( - "thread {thread_id} did not include persisted history" - )) - })?; + let history = stored_thread.history.take().ok_or_else(|| { + internal_error(format!( + "thread {thread_id} did not include persisted history" + )) + })?; Ok(InitialHistory::Resumed(ResumedHistory { + history_revision: history.revision, conversation_id: thread_id, - history: Arc::new(history), + history: Arc::new(history.items), rollout_path: stored_thread.rollout_path.clone(), })) } @@ -5064,6 +5074,7 @@ impl ThreadRequestProcessor { // The fork cutoff can remove the only TurnContext that records the selected version. // Recover it from the untrimmed source or live parent, independently of permission overrides. let source_multi_agent_version = InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: source_thread_id, history: Arc::clone(latest_context.as_ref().unwrap_or(&source_history_items)), rollout_path: source_thread.rollout_path.clone(), @@ -5191,6 +5202,7 @@ impl ThreadRequestProcessor { ForkSnapshot::Interrupted, fork_options, InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: source_thread_id, history: history_items, rollout_path: source_thread.rollout_path.clone(), diff --git a/codex-rs/app-server/src/thread_state.rs b/codex-rs/app-server/src/thread_state.rs index 8cfbc00983aa..a19c41b19ff8 100644 --- a/codex-rs/app-server/src/thread_state.rs +++ b/codex-rs/app-server/src/thread_state.rs @@ -99,6 +99,7 @@ pub(crate) struct TurnSummary { #[derive(Default)] pub(crate) struct ThreadState { + goal_resume_lock: Arc>, pub(crate) pending_interrupts: PendingInterruptQueue, pub(crate) turn_summary: TurnSummary, pub(crate) last_terminal_turn_id: Option, @@ -361,6 +362,21 @@ pub(crate) struct ThreadStateManager { } impl ThreadStateManager { + /// Coordinates goal edits with cold/path-based resume for just this thread. + pub(crate) async fn lock_goal_resume( + &self, + thread_id: ThreadId, + ) -> tokio::sync::OwnedMutexGuard<()> { + let lock = self + .thread_state(thread_id) + .await + .lock() + .await + .goal_resume_lock + .clone(); + lock.lock_owned().await + } + pub(crate) fn new() -> Self { Self::default() } diff --git a/codex-rs/app-server/tests/suite/v2/guardian_goal_tests.rs b/codex-rs/app-server/tests/suite/v2/guardian_goal_tests.rs new file mode 100644 index 000000000000..358114e4de5b --- /dev/null +++ b/codex-rs/app-server/tests/suite/v2/guardian_goal_tests.rs @@ -0,0 +1,131 @@ +//! Goal API edits and clears enter model history across loaded and cold-resume paths. + +use super::*; +use codex_app_server_protocol::ThreadGoalSetResponse; +use pretty_assertions::assert_eq; +use test_case::test_case; + +#[test_case(false; "loaded")] +#[test_case(true; "unloaded with path resume")] +#[tokio::test] +async fn user_goal_updates_survive_resume_and_clear(unloaded: bool) -> Result<()> { + skip_if_no_network!(Ok(())); + let server = responses::start_mock_server().await; + let mock = responses::mount_sse_once( + &server, + responses::sse(vec![responses::ev_completed("done")]), + ) + .await; + let codex_home = TempDir::new()?; + MockResponsesConfig::new(&server.uri()) + .with_provider_config("supports_websockets = false") + .enable_feature(Feature::Goals) + .disable_feature(Feature::EnableRequestCompression) + .write(codex_home.path())?; + let mut app = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .without_managed_config() + .build_initialized_with_timeout(TIMEOUT) + .await?; + let thread_id = if unloaded { + create_fake_rollout( + codex_home.path(), + "2025-01-05T12-00-00", + "2025-01-05T12:00:00Z", + "", + Some("mock_provider"), + /*git_info*/ None, + )? + } else { + app.start_thread(ThreadStartParams::default()) + .await? + .thread + .id + }; + // Automatic lifecycle calls mutate goal state without creating user authorization. + let id = app + .send_raw_request( + "thread/goal/set", + Some(json!({ + "threadId": thread_id, "objective": "Automatically generated goal.", + "status": "paused", "origin": "automatic", + })), + ) + .await?; + let _: ThreadGoalSetResponse = app.read_response(id).await?; + let mut resume_request = None; + for objective in [ + "Send the approved report.", + "Draft the report, but do not send it.", + ] { + if unloaded && objective.starts_with("Draft") { + let path = codex_rollout::find_thread_path_by_id_str( + codex_home.path(), + &thread_id, + /*state_db_ctx*/ None, + ) + .await? + .expect("unloaded rollout"); + resume_request = Some( + app.send_thread_resume_request(ThreadResumeParams { + thread_id: String::new(), + path: Some(path), + ..Default::default() + }) + .await?, + ); + } + let id = app + .send_raw_request( + "thread/goal/set", + Some(json!({ + "threadId": thread_id, "objective": objective, "status": "paused", "origin": "user", + })), + ) + .await?; + let _: ThreadGoalSetResponse = app.read_response(id).await?; + } + if let Some(id) = resume_request { + let _: ThreadResumeResponse = app.read_response(id).await?; + } + let id = app + .send_raw_request( + "thread/goal/clear", + Some(json!({"threadId": thread_id, "origin": "user"})), + ) + .await?; + let _: codex_app_server_protocol::ThreadGoalClearResponse = app.read_response(id).await?; + let id = app + .send_turn_start_request(TurnStartParams { + thread_id, + input: vec![UserInput::Text { + text: "What is the status?".to_owned(), + text_elements: vec![], + }], + ..Default::default() + }) + .await?; + let _: TurnStartResponse = app.read_response(id).await?; + let completed: TurnCompletedNotification = + timeout(TIMEOUT, app.read_notification("turn/completed")).await??; + assert_eq!(completed.turn.status, TurnStatus::Completed); + let history = mock.single_request().message_input_texts("user").join("\n"); + assert!( + !history.contains("Automatically generated goal."), + "{history}" + ); + let old = history + .find("User set the goal: \"Send the approved report.\"") + .expect("original user goal"); + let new = history + .find("User set the goal: \"Draft the report, but do not send it.\"") + .expect("updated user goal"); + assert!(old < new, "{history}"); + assert!( + history.contains("User set goal status: \"paused\"."), + "{history}" + ); + assert!(history.contains("User cleared the goal."), "{history}"); + app.shutdown_gracefully().await?; + Ok(()) +} diff --git a/codex-rs/app-server/tests/suite/v2/guardian_v2.rs b/codex-rs/app-server/tests/suite/v2/guardian_v2.rs index b769a1682cc2..b084707a4ecd 100644 --- a/codex-rs/app-server/tests/suite/v2/guardian_v2.rs +++ b/codex-rs/app-server/tests/suite/v2/guardian_v2.rs @@ -78,6 +78,9 @@ use super::mcp_tool::start_mcp_server_with_tools; #[path = "guardian_sync_session_tests.rs"] mod sync_sessions; +#[path = "guardian_goal_tests.rs"] +mod goals; + #[path = "guardian_v2_history_tests.rs"] mod history; diff --git a/codex-rs/cli/Cargo.toml b/codex-rs/cli/Cargo.toml index 399230d3f4d1..0c67a20cd206 100644 --- a/codex-rs/cli/Cargo.toml +++ b/codex-rs/cli/Cargo.toml @@ -123,6 +123,7 @@ windows-sys = { version = "0.52", features = [ app_test_support = { workspace = true } assert_cmd = { workspace = true } assert_matches = { workspace = true } +codex-app-server-client = { workspace = true } codex-utils-cargo-bin = { workspace = true } codex-utils-pty = { workspace = true } flate2 = { workspace = true } diff --git a/codex-rs/cli/tests/features.rs b/codex-rs/cli/tests/features.rs index 4beebb0d3f2d..d55a3f4e77b0 100644 --- a/codex-rs/cli/tests/features.rs +++ b/codex-rs/cli/tests/features.rs @@ -363,6 +363,8 @@ fn no_daemon_rejects_agents_and_explicit_remote_targets() -> Result<()> { "--no-daemon --remote ws://localhost:9999 agents", "--no-daemon --remote ws://localhost:9999", "--no-daemon --remote ws://localhost:9999 archive example", + "--no-daemon --remote ws://localhost:9999 unarchive example", + "--no-daemon --remote ws://localhost:9999 delete --force 123e4567-e89b-12d3-a456-426614174000", "--no-daemon --remote ws://localhost:9999 queue --thread example --message hello", "--remote ws://localhost:9999 resume --no-daemon --last", "--no-daemon fork --remote ws://localhost:9999 session-name", diff --git a/codex-rs/cli/tests/queue.rs b/codex-rs/cli/tests/queue.rs index 4275ed7b94af..b4f5f77a59a8 100644 --- a/codex-rs/cli/tests/queue.rs +++ b/codex-rs/cli/tests/queue.rs @@ -22,6 +22,16 @@ enum QueueResponse { UnknownMethodVariant, } +#[cfg(unix)] +fn socket_test_home() -> Result { + // macOS temporary paths can exceed the Unix socket path limit. + #[cfg(target_os = "macos")] + let home = tempfile::tempdir_in("/tmp")?; + #[cfg(not(target_os = "macos"))] + let home = TempDir::new()?; + Ok(home) +} + async fn respond_to_queue_request( stream: S, codex_home: &Path, @@ -188,10 +198,288 @@ async fn run_remote_queue_command(response: QueueResponse) -> Result<(Output, Va Ok((output, server.await??)) } +#[cfg(unix)] +#[tokio::test] +async fn remote_session_commands_with_workload_identity_use_server_auth() -> Result<()> { + use std::time::Duration; + + use app_test_support::MockResponsesConfig; + use app_test_support::TestAppServer; + use app_test_support::create_final_assistant_message_sse_response; + use app_test_support::create_mock_responses_server_sequence_unchecked; + use codex_app_server_client::AppServerEvent; + use codex_app_server_client::DEFAULT_IN_PROCESS_CHANNEL_CAPACITY; + use codex_app_server_client::RemoteAppServerClient; + use codex_app_server_client::RemoteAppServerConnectArgs; + use codex_app_server_client::RemoteAppServerEndpoint; + use codex_app_server_protocol::ClientRequest; + use codex_app_server_protocol::RequestId; + use codex_app_server_protocol::ServerNotification; + use codex_app_server_protocol::ThreadListParams; + use codex_app_server_protocol::ThreadListResponse; + use codex_app_server_protocol::ThreadQueueListParams; + use codex_app_server_protocol::ThreadQueueListResponse; + use codex_app_server_protocol::ThreadSetNameParams; + use codex_app_server_protocol::ThreadSetNameResponse; + use codex_app_server_protocol::ThreadStartParams; + use codex_app_server_protocol::ThreadStartResponse; + use codex_app_server_protocol::TurnStatus; + use codex_protocol::shell_environment::OPENAI_FEDERATION_RULE_ID_ENV_VAR; + use codex_protocol::shell_environment::OPENAI_IDENTITY_TOKEN_FILE_ENV_VAR; + use tokio::process::Command; + use tokio::time::timeout; + + let codex_home = socket_test_home()?; + let codex = codex_utils_cargo_bin::cargo_bin("codex")?; + let model = create_mock_responses_server_sequence_unchecked(vec![ + create_final_assistant_message_sse_response("queued message consumed")?, + ]) + .await; + MockResponsesConfig::new(&model.uri()) + .with_root_config("features.plugins = false\nanalytics.enabled = false") + .with_provider_config("env_key = \"CODEX_QUEUE_SERVER_API_KEY\"") + .write(codex_home.path())?; + let _server = TestAppServer::builder() + .with_program(&codex) + .with_codex_home(codex_home.path()) + .with_plugin_startup_tasks() + .without_managed_config() + .with_args(&["app-server", "--listen", "unix://"]) + .with_env_overrides(&[ + ("OPENAI_API_KEY", None), + ("CODEX_API_KEY", None), + ("CODEX_ACCESS_TOKEN", None), + (OPENAI_FEDERATION_RULE_ID_ENV_VAR, None), + (OPENAI_IDENTITY_TOKEN_FILE_ENV_VAR, None), + ("CODEX_QUEUE_SERVER_API_KEY", Some("server-only-test-key")), + ]) + .build() + .await?; + let socket_path = codex_app_server::app_server_control_socket_path(codex_home.path())?; + timeout(Duration::from_secs(30), async { + while !socket_path.as_path().try_exists()? { + tokio::time::sleep(Duration::from_millis(10)).await; + } + Ok::<_, anyhow::Error>(()) + }) + .await??; + + let endpoint = format!("unix://{}", socket_path.as_path().display()); + let mut app = RemoteAppServerClient::connect(RemoteAppServerConnectArgs { + endpoint: RemoteAppServerEndpoint::UnixSocket { socket_path }, + client_name: "queue-e2e-test".to_string(), + client_version: "0.1.0".to_string(), + experimental_api: true, + mcp_server_openai_form_elicitation: false, + opt_out_notification_methods: Vec::new(), + channel_capacity: DEFAULT_IN_PROCESS_CHANNEL_CAPACITY, + }) + .await?; + let started: ThreadStartResponse = app + .request_typed(ClientRequest::ThreadStart { + request_id: RequestId::Integer(1), + params: ThreadStartParams::default(), + }) + .await?; + let thread_id = started.thread.id; + let message = "synthetic WIF queue end-to-end message"; + let output = timeout( + Duration::from_secs(30), + Command::new(&codex) + .env("CODEX_HOME", codex_home.path()) + .env(OPENAI_FEDERATION_RULE_ID_ENV_VAR, "rule-test") + .env( + OPENAI_IDENTITY_TOKEN_FILE_ENV_VAR, + codex_home.path().join("missing-identity-token"), + ) + .env_remove("CODEX_QUEUE_SERVER_API_KEY") + .kill_on_drop(true) + .args(["queue", "--remote", "unix://"]) + .args(["--thread", &thread_id, "--message", message]) + .output(), + ) + .await??; + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let completed = timeout(Duration::from_secs(30), async { + while let Some(event) = app.next_event().await { + if let AppServerEvent::ServerNotification(notification) = event + && let ServerNotification::TurnCompleted(completed) = *notification + { + return Ok::<_, anyhow::Error>(completed); + } + } + anyhow::bail!("app server disconnected before the queued turn completed") + }) + .await??; + assert_eq!(completed.thread_id, thread_id); + assert_eq!(completed.turn.status, TurnStatus::Completed); + let queue: ThreadQueueListResponse = app + .request_typed(ClientRequest::ThreadQueueList { + request_id: RequestId::Integer(2), + params: ThreadQueueListParams { + thread_id: thread_id.clone(), + cursor: None, + limit: None, + }, + }) + .await?; + assert!(queue.data.is_empty()); + + let requests = model + .received_requests() + .await + .context("model request capture unavailable")?; + let request = requests + .iter() + .find(|request| request.url.path().ends_with("/responses")) + .context("queued message did not reach the model")?; + assert!( + request.body_json::()?["input"] + .to_string() + .contains(message) + ); + assert_eq!( + request.headers["authorization"].to_str()?, + "Bearer server-only-test-key" + ); + let metadata: Value = serde_json::from_str(request.headers["x-codex-turn-metadata"].to_str()?)?; + assert_eq!(metadata["turn_trigger"], "queue"); + + let name = "remote-session-lifecycle"; + let _: ThreadSetNameResponse = app + .request_typed(ClientRequest::ThreadSetName { + request_id: RequestId::Integer(3), + params: ThreadSetNameParams { + thread_id: thread_id.clone(), + name: name.to_string(), + }, + }) + .await?; + // A separate caller home makes name resolution rely on the selected server. + let caller_home = TempDir::new()?; + let remote_command = || { + let mut command = Command::new(&codex); + command + .env("CODEX_HOME", caller_home.path()) + .current_dir(caller_home.path()) + .env(OPENAI_FEDERATION_RULE_ID_ENV_VAR, "rule-test") + .env( + OPENAI_IDENTITY_TOKEN_FILE_ENV_VAR, + caller_home.path().join("missing-identity-token"), + ) + .env_remove("CODEX_QUEUE_SERVER_API_KEY") + .kill_on_drop(true) + .args(["--remote", &endpoint]); + command + }; + let output = timeout( + Duration::from_secs(30), + remote_command().args(["delete", &thread_id]).output(), + ) + .await??; + assert!(!output.status.success()); + assert!(String::from_utf8(output.stderr)?.contains("cannot confirm session deletion")); + + let cases: &[(&[&str], Option)] = &[ + (&["archive", name], Some(true)), + (&["unarchive", name], Some(false)), + (&["delete", "--force", &thread_id], None), + ]; + for &(args, expected_archived) in cases { + let output = timeout( + Duration::from_secs(30), + remote_command().args(args).output(), + ) + .await??; + assert!( + output.status.success(), + "{args:?}: {}", + String::from_utf8_lossy(&output.stderr) + ); + for archived in [false, true] { + let threads: ThreadListResponse = app + .request_typed(ClientRequest::ThreadList { + request_id: RequestId::Integer(4), + params: ThreadListParams { + cursor: None, + limit: None, + sort_key: None, + sort_direction: None, + model_providers: None, + source_kinds: None, + originators: None, + archived: Some(archived), + section_id: None, + project_id: None, + cwd: None, + use_state_db_only: false, + search_term: None, + parent_thread_id: None, + ancestor_thread_id: None, + }, + }) + .await?; + assert_eq!( + threads.data.iter().any(|thread| thread.id == thread_id), + expected_archived == Some(archived), + "{args:?}: archived={archived}" + ); + } + } + Ok(()) +} + +#[test] +fn remote_session_commands_validate_config() -> Result<()> { + let cases: &[(&[&str], &str, &str)] = &[ + ( + &["--strict-config"], + "config.toml", + "unknown_queue_option = true\n", + ), + (&["--profile", "queue"], "queue.config.toml", "model = 42\n"), + ( + &[], + "config.toml", + "experimental_thread_store_endpoint = \"https://example.com\"\n", + ), + ]; + let commands: &[&[&str]] = &[ + &["queue", "--thread", THREAD_ID, "--message", "do the thing"], + &["archive", THREAD_ID], + &["unarchive", THREAD_ID], + &["delete", "--force", THREAD_ID], + ]; + for &command in commands { + for &(args, config_file, config) in cases { + let codex_home = TempDir::new()?; + std::fs::write(codex_home.path().join(config_file), config)?; + let output = std::process::Command::new(codex_utils_cargo_bin::cargo_bin("codex")?) + .env("CODEX_HOME", codex_home.path()) + .current_dir(codex_home.path()) + .args(args) + .args(["--remote", "ws://127.0.0.1:1"]) + .args(command) + .output()?; + let stderr = String::from_utf8(output.stderr)?; + assert!(!output.status.success()); + assert!( + stderr.contains("failed to load config.toml"), + "{command:?}, {config:?}: {stderr}" + ); + } + } + Ok(()) +} + #[cfg(unix)] #[tokio::test] async fn queue_rejects_local_daemon_that_does_not_support_queueing() -> Result<()> { - let codex_home = tempfile::tempdir_in("/tmp")?; + let codex_home = socket_test_home()?; let socket_path = codex_app_server::app_server_control_socket_path(codex_home.path())?; std::fs::create_dir_all( socket_path @@ -232,7 +520,7 @@ async fn queue_rejects_local_daemon_that_does_not_support_queueing() -> Result<( #[cfg(unix)] #[tokio::test] async fn queue_rejects_overrides_that_bypass_local_daemon() -> Result<()> { - let codex_home = tempfile::tempdir_in("/tmp")?; + let codex_home = socket_test_home()?; let socket_path = codex_app_server::app_server_control_socket_path(codex_home.path())?; std::fs::create_dir_all( socket_path diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index bcc4a8ecceb2..43a322feacdd 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -975,6 +975,9 @@ "mentions_v2": { "type": "boolean" }, + "model_catalog_in_context": { + "type": "boolean" + }, "multi_agent": { "type": "boolean" }, @@ -7015,6 +7018,9 @@ "mentions_v2": { "type": "boolean" }, + "model_catalog_in_context": { + "type": "boolean" + }, "multi_agent": { "type": "boolean" }, diff --git a/codex-rs/core/src/agent/control/spawn.rs b/codex-rs/core/src/agent/control/spawn.rs index 277ce112bd84..7ca849717bb3 100644 --- a/codex-rs/core/src/agent/control/spawn.rs +++ b/codex-rs/core/src/agent/control/spawn.rs @@ -160,7 +160,7 @@ async fn load_agent_model_context( state: &ThreadManagerState, thread_id: ThreadId, history_mode: ThreadHistoryMode, -) -> CodexResult>> { +) -> CodexResult> { match history_mode { ThreadHistoryMode::Legacy => Ok(state .read_stored_thread(ReadThreadParams { @@ -170,15 +170,18 @@ async fn load_agent_model_context( }) .await? .history - .map(|history| history.items)), + .map(|history| codex_thread_store::StoredModelContext { + thread_id, + revision: history.revision, + items: history.items, + })), ThreadHistoryMode::Paginated => Ok(Some( state .load_latest_model_context(LoadThreadHistoryParams { thread_id, include_archived: true, }) - .await? - .items, + .await?, )), } } @@ -365,8 +368,9 @@ impl LocalAgentControl { .await? .ok_or(CodexErr::ThreadNotFound(thread_id))?; let initial_history = InitialHistory::Resumed(ResumedHistory { + history_revision: history.revision, conversation_id: thread_id, - history: Arc::new(history), + history: Arc::new(history.items), rollout_path: stored_thread.rollout_path, }); if initial_history.get_multi_agent_version() != Some(MultiAgentVersion::V2) { @@ -992,7 +996,8 @@ impl LocalAgentControl { CodexErr::Fatal(format!( "parent thread history unavailable for fork: {parent_thread_id}" )) - })?; + })? + .items; let selected_capability_roots = forked_rollout_items .iter() @@ -1351,8 +1356,9 @@ impl LocalAgentControl { .await? .ok_or(CodexErr::ThreadNotFound(thread_id))?; let initial_history = InitialHistory::Resumed(ResumedHistory { + history_revision: history.revision, conversation_id: thread_id, - history: Arc::new(history), + history: Arc::new(history.items), rollout_path: stored_thread.rollout_path, }); let parent_thread_id = stored_thread.parent_thread_id; diff --git a/codex-rs/core/src/agent/control_tests.rs b/codex-rs/core/src/agent/control_tests.rs index dbd89a5ccfe2..5d483f385389 100644 --- a/codex-rs/core/src/agent/control_tests.rs +++ b/codex-rs/core/src/agent/control_tests.rs @@ -1347,6 +1347,7 @@ async fn cold_resume_with_thread_instructions_preserves_lazy_v2_child_inheritanc .await .expect("read parent history"); let initial_history = InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: parent_thread_id, history: Arc::new(stored_parent.history.expect("parent history").items), rollout_path: stored_parent.rollout_path, diff --git a/codex-rs/core/src/codex_thread.rs b/codex-rs/core/src/codex_thread.rs index c54fd5454d18..7a002472217c 100644 --- a/codex-rs/core/src/codex_thread.rs +++ b/codex-rs/core/src/codex_thread.rs @@ -755,8 +755,7 @@ impl CodexThread { &self, update: crate::context::UserGoalUpdate, ) -> CodexResult<()> { - self.session.record_user_goal_update(update).await; - self.checkpoint_preparation().await?; + self.session.record_user_goal_update(update).await?; Ok(()) } diff --git a/codex-rs/core/src/compact.rs b/codex-rs/core/src/compact.rs index a937b39db0e4..6de37b1e0ae4 100644 --- a/codex-rs/core/src/compact.rs +++ b/codex-rs/core/src/compact.rs @@ -1,3 +1,5 @@ +use crate::context::UserGoalUpdate; +use std::collections::HashSet; use std::sync::Arc; use std::time::Instant; @@ -30,6 +32,7 @@ use codex_analytics::CompactionTrigger; use codex_analytics::now_unix_seconds; use codex_history::CodexHarnessMetadata; use codex_history::ResponseItemEnvelope; +use codex_protocol::ResponseItemId; use codex_protocol::error::CodexErr; use codex_protocol::error::CodexErrorDetails; use codex_protocol::error::Result as CodexResult; @@ -76,6 +79,7 @@ pub(crate) enum InitialContextInjection { /// `Session::replace_compacted_history` assigns missing item IDs before constructing the persisted /// `CompactedItem`, ensuring the live and persisted histories remain identical. pub(crate) struct CompactedHistoryMetadata { + pub(crate) input_goal_ids: HashSet, pub(crate) message: String, pub(crate) window_number: u64, pub(crate) window_ids: AutoCompactWindowIds, @@ -255,6 +259,7 @@ async fn run_compact_task_inner_impl( let initial_input_for_turn: ResponseInputItem = ResponseInputItem::from(input); let mut history = sess.clone_history().await; + let input_goal_ids = UserGoalUpdate::message_ids(history.raw_items()); history.record_items( &[initial_input_for_turn.into()], turn_context.model_info().truncation_policy.into(), @@ -381,6 +386,7 @@ async fn run_compact_task_inner_impl( reference_context_item, world_state_baseline, CompactedHistoryMetadata { + input_goal_ids, message: summary_text, window_number, window_ids, diff --git a/codex-rs/core/src/compact_remote_v2.rs b/codex-rs/core/src/compact_remote_v2.rs index 013a4faca473..56e4aace1b83 100644 --- a/codex-rs/core/src/compact_remote_v2.rs +++ b/codex-rs/core/src/compact_remote_v2.rs @@ -293,6 +293,7 @@ async fn run_remote_compact_task_inner_impl( } }; let RemoteCompactV2Attempt { + input_goal_ids, trace_input_history, prompt_input, prompt_input_metadata, @@ -361,6 +362,7 @@ async fn run_remote_compact_task_inner_impl( reference_context_item, world_state_baseline, CompactedHistoryMetadata { + input_goal_ids, message: String::new(), window_number: new_window_number, window_ids: new_window_ids, diff --git a/codex-rs/core/src/compact_remote_v2_attempt.rs b/codex-rs/core/src/compact_remote_v2_attempt.rs index 60b5b1b7721c..6f881e2868a0 100644 --- a/codex-rs/core/src/compact_remote_v2_attempt.rs +++ b/codex-rs/core/src/compact_remote_v2_attempt.rs @@ -1,3 +1,6 @@ +use crate::context::UserGoalUpdate; +use codex_protocol::ResponseItemId; +use std::collections::HashSet; use std::sync::Arc; use super::RemoteCompactionV2Output; @@ -17,6 +20,7 @@ use codex_rollout_trace::CompactionTraceContext; use tracing::info; pub(super) struct RemoteCompactV2Attempt { + pub(super) input_goal_ids: HashSet, pub(super) trace_input_history: Option>, pub(super) prompt_input: Vec, pub(super) prompt_input_metadata: Vec>, @@ -37,6 +41,7 @@ pub(super) async fn run_remote_compact_v2_attempt( ) -> CodexResult { let turn_context = &step_context.turn; let mut history = sess.clone_history().await; + let input_goal_ids = UserGoalUpdate::message_ids(history.raw_items()); let base_instructions = sess.get_prompt_base_instructions().await; let (rewritten_outputs, estimated_deleted_tokens) = trim_function_call_history_to_fit_context_window( @@ -122,6 +127,7 @@ pub(super) async fn run_remote_compact_v2_attempt( let mut prompt_input = prompt.input; prompt_input.pop(); Ok(RemoteCompactV2Attempt { + input_goal_ids, trace_input_history, prompt_input, prompt_input_metadata, diff --git a/codex-rs/core/src/context/user_goal.rs b/codex-rs/core/src/context/user_goal.rs index d20504159ff6..76d7c1c53281 100644 --- a/codex-rs/core/src/context/user_goal.rs +++ b/codex-rs/core/src/context/user_goal.rs @@ -3,10 +3,12 @@ //! Oversized objectives are omitted whole so truncation cannot turn a restriction into a grant. use super::ContextualUserFragment; +use codex_protocol::ResponseItemId; use codex_protocol::models::ContentItem; use codex_protocol::models::ContentItemKind; use codex_protocol::models::ResponseItem; use codex_protocol::protocol::ThreadGoalStatus; +use std::collections::HashSet; const MAX_OBJECTIVE_BYTES: usize = 700; @@ -23,6 +25,17 @@ pub enum UserGoalUpdate { impl UserGoalUpdate { pub(crate) const OMITTED_OBJECTIVE_KIND: &str = "user.goal.omitted"; + /// Captures accepted goal instructions at a compaction input boundary. + pub(crate) fn message_ids<'a>( + items: impl Iterator, + ) -> HashSet { + items + .filter(|item| Self::message_text(item).is_some()) + .filter_map(ResponseItem::id) + .cloned() + .collect() + } + /// Reads only a host-annotated goal instruction, never a matching text wrapper alone. pub(crate) fn message_text(item: &ResponseItem) -> Option<&str> { let ResponseItem::Message { diff --git a/codex-rs/core/src/context/world_state/mod.rs b/codex-rs/core/src/context/world_state/mod.rs index 1c4441f80e3a..62e5dcd7b907 100644 --- a/codex-rs/core/src/context/world_state/mod.rs +++ b/codex-rs/core/src/context/world_state/mod.rs @@ -7,6 +7,7 @@ mod environment; mod environments_instructions; mod managed_developer_instructions; mod model; +mod model_catalog; mod multi_agent_mode; mod multi_agent_usage_hint; mod permissions; @@ -46,6 +47,7 @@ pub(crate) use managed_developer_instructions::ManagedDeveloperInstructions; pub(crate) use managed_developer_instructions::ManagedDeveloperInstructionsState; pub(crate) use managed_developer_instructions::validate_managed_developer_instructions; pub(crate) use model::ModelInstructionsState; +pub(crate) use model_catalog::ModelCatalogState; pub(crate) use multi_agent_mode::MultiAgentModeState; pub(crate) use multi_agent_usage_hint::MultiAgentUsageHintState; pub(crate) use permissions::PermissionsState; diff --git a/codex-rs/core/src/context/world_state/model_catalog.rs b/codex-rs/core/src/context/world_state/model_catalog.rs new file mode 100644 index 000000000000..db685166a5d0 --- /dev/null +++ b/codex-rs/core/src/context/world_state/model_catalog.rs @@ -0,0 +1,144 @@ +//! Keeps spawn model choices in append-only context instead of the tool schema. +//! Snapshots compare the bounded text actually shown to the model; catalog refreshes +//! never rewrite earlier messages, and missing retained context gets a fresh catalog. + +use super::PreviousSectionState; +use super::WorldStateHash; +use super::WorldStateSection; +use crate::agent::child_config::MAX_SPAWN_AGENT_MODEL_OVERRIDES; +use crate::agent::child_config::model_supports_multi_agent_backend; +use crate::context::ContextualUserFragment; +use crate::context::environment_context::push_xml_escaped_text; +use codex_protocol::models::ContentItemKind; +use codex_protocol::openai_models::ModelPreset; +use codex_protocol::protocol::MultiAgentVersion; + +// Includes XML escaping and markers; at most 1,000 tokens even with byte fallback. +const MAX_RENDERED_BYTES: usize = 1_000; +const MAX_DESCRIPTION_CHARS: usize = 250; +const OMITTED_NOTICE: &str = "Additional model choices omitted.\n"; + +/// The current model choices, or an empty catalog to invalidate a retained listing. +#[derive(Clone, Default)] +pub(crate) struct ModelCatalogState { + catalog: String, +} + +impl ModelCatalogState { + pub(crate) fn new(models: &[ModelPreset], multi_agent_version: MultiAgentVersion) -> Self { + let mut catalog = String::new(); + let mut models = models + .iter() + .filter(|model| model.show_in_picker) + .filter(|model| model_supports_multi_agent_backend(model, multi_agent_version)) + .take(MAX_SPAWN_AGENT_MODEL_OVERRIDES) + .collect::>(); + // Stabilize the selected choices independently of their picker ordering. + models.sort_by(|left, right| left.model.cmp(&right.model)); + if models.is_empty() { + catalog.push_str("No picker-visible model overrides are currently loaded.\n"); + } + let (open, close) = Self::type_markers(); + let body_budget = MAX_RENDERED_BYTES - open.len() - close.len() - 2; + for model in models { + let mut line = format!("- `{}`: ", model.model); + line.extend(model.description.chars().take(MAX_DESCRIPTION_CHARS)); + if model.description.chars().count() > MAX_DESCRIPTION_CHARS { + line.push('…'); + } + if !model.supported_reasoning_efforts.is_empty() { + line.push_str(" Reasoning efforts: "); + for (index, preset) in model.supported_reasoning_efforts.iter().enumerate() { + if index > 0 { + line.push_str(", "); + } + line.push_str(preset.effort.as_str()); + if preset.effort == model.default_reasoning_effort { + line.push_str(" (default)"); + } + } + line.push('.'); + } + if !model.service_tiers.is_empty() { + line.push_str(" Service tiers: "); + for (index, tier) in model.service_tiers.iter().enumerate() { + if index > 0 { + line.push_str(", "); + } + line.push_str(tier.id.as_str()); + } + line.push('.'); + } + let mut escaped = String::new(); + push_xml_escaped_text(&mut escaped, &line); + if catalog.len() + escaped.len() + 1 + OMITTED_NOTICE.len() > body_budget { + // Keep identifiers and capability lists intact instead of advertising + // a truncated model ID, reasoning effort, or service tier. + catalog.push_str(OMITTED_NOTICE); + break; + } + catalog.push_str(&escaped); + catalog.push('\n'); + } + Self { catalog } + } +} + +impl ContextualUserFragment for ModelCatalogState { + fn content_kind(&self) -> ContentItemKind { + ContentItemKind("multi_agent.model_catalog".to_string()) + } + + fn role(&self) -> &'static str { + "developer" + } + + fn markers(&self) -> (&'static str, &'static str) { + Self::type_markers() + } + + fn type_markers() -> (&'static str, &'static str) { + ("", "") + } + + fn body(&self) -> String { + if self.catalog.is_empty() { + "\nThe previous spawn_agent model catalog no longer applies.\n".to_string() + } else { + format!("\n{}", self.catalog) + } + } +} + +impl WorldStateSection for ModelCatalogState { + const ID: &'static str = "model_catalog"; + type Snapshot = WorldStateHash; + + fn snapshot(&self) -> Self::Snapshot { + WorldStateHash::from_fragment(self) + } + + fn matches_legacy_fragment(role: &str, text: &str) -> bool { + role == "developer" && Self::matches_text(text) + } + + fn has_retained_fragment_matcher() -> bool { + true + } + + fn matches_retained_fragment(role: &str, text: &str) -> bool { + Self::matches_legacy_fragment(role, text) + } + + fn render_diff( + &self, + previous: PreviousSectionState<'_, Self::Snapshot>, + ) -> Option> { + if matches!(previous, PreviousSectionState::Known(previous) if previous == &self.snapshot()) + || self.catalog.is_empty() && matches!(previous, PreviousSectionState::Absent) + { + return None; + } + Some(Box::new(self.clone())) + } +} diff --git a/codex-rs/core/src/guardian/review_session_tests.rs b/codex-rs/core/src/guardian/review_session_tests.rs index 0a2eaf9445f6..7414760c66a2 100644 --- a/codex-rs/core/src/guardian/review_session_tests.rs +++ b/codex-rs/core/src/guardian/review_session_tests.rs @@ -85,6 +85,7 @@ async fn run_review_preserves_evidence_during_parent_compaction() { /*reference_context_item*/ None, /*world_state_baseline*/ None, crate::compact::CompactedHistoryMetadata { + input_goal_ids: Default::default(), message: String::new(), window_number, window_ids, diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 5e9ec6813fa6..88792b6ddadd 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -2657,6 +2657,7 @@ async fn guardian_reuses_prompt_cache_key_and_appends_prior_reviews() -> anyhow: /*reference_context_item*/ None, /*world_state_baseline*/ None, crate::compact::CompactedHistoryMetadata { + input_goal_ids: Default::default(), message: String::new(), window_number, window_ids, diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index b87697684690..4708d2921e76 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -4055,7 +4055,6 @@ impl Session { .get_or_insert_default() .compaction_model_hash = metadata.compaction_model_hash; } - let replacement_history = items.clone(); // Wait for accepted updates to finish persisting, then keep later updates from // overtaking the current settings snapshot while its checkpoint is written. let _settings_guard = thread_settings::acquire_persistence_lock(self).await; @@ -4076,6 +4075,26 @@ impl Session { } (!retained.is_empty()).then(|| WorldStateSnapshot::from(&retained)) }); + // Goal edits are published outside the running task. Keep edits accepted after + // the compaction input snapshot, in their original order, after its replacement. + let replacement_goal_ids = crate::context::UserGoalUpdate::message_ids( + items.iter().map(|envelope| &envelope.item), + ); + items.extend( + state + .history + .annotated_items() + .iter() + .filter(|envelope| { + crate::context::UserGoalUpdate::message_text(&envelope.item).is_some() + && envelope.item.id().is_some_and(|id| { + !metadata.input_goal_ids.contains(id) + && !replacement_goal_ids.contains(id) + }) + }) + .cloned(), + ); + let replacement_history = items.clone(); state.replace_annotated_history( items, reference_context_item.clone(), @@ -4530,9 +4549,10 @@ impl Session { world_state: Arc, ) -> u64 { let turn_context = step_context.turn.as_ref(); + let history = self.clone_history().await; + let input_goal_ids = crate::context::UserGoalUpdate::message_ids(history.raw_items()); let retained_client_developer_messages = if self.enabled(Feature::RetainClientDeveloperMessages) { - let history = self.clone_history().await; crate::compact_remote_v2::truncate_retained_messages_for_remote_compaction( history .annotated_items() @@ -4565,6 +4585,7 @@ impl Session { Some(turn_context_item), Some(world_state), CompactedHistoryMetadata { + input_goal_ids, message: String::new(), window_number, window_ids, diff --git a/codex-rs/core/src/session/reasoning_effort_tests.rs b/codex-rs/core/src/session/reasoning_effort_tests.rs index bee5f9f2a315..d9ff4ba24ef8 100644 --- a/codex-rs/core/src/session/reasoning_effort_tests.rs +++ b/codex-rs/core/src/session/reasoning_effort_tests.rs @@ -15,6 +15,7 @@ use test_case::test_case; #[test_case(InitialHistory::Forked(Vec::new()); "fork")] #[test_case(InitialHistory::Resumed(ResumedHistory { +history_revision: None, conversation_id: ThreadId::default(), history: Arc::new(Vec::new()), rollout_path: None, diff --git a/codex-rs/core/src/session/retained_context.rs b/codex-rs/core/src/session/retained_context.rs index c1724370e131..8c4203bf1335 100644 --- a/codex-rs/core/src/session/retained_context.rs +++ b/codex-rs/core/src/session/retained_context.rs @@ -1,5 +1,6 @@ //! Orders retained inputs at acceptance, assistant messages at stream start, //! and Code Mode messages at confirmed delivery. +//! Explicit goal authorization becomes live only after its persistence barrier succeeds. use std::collections::HashMap; use std::future::Future; @@ -11,10 +12,13 @@ use std::task::Poll; use crate::context::ContextualUserFragment; use crate::context::UserGoalUpdate; +use codex_history::CodexHarnessMetadata; +use codex_history::ResponseItemEnvelope; use codex_history::RetainedContextEvent; use codex_history::RetainedUserMessage; use codex_history::RolloutItem; use codex_protocol::models::ResponseItem; +use codex_thread_store::PersistContext; use tokio::sync::Semaphore; use tokio::sync::watch; use tokio::task::JoinHandle; @@ -77,17 +81,47 @@ impl Session { } /// Records authorization without adding pending input or reopening an active turn. - pub(crate) async fn record_user_goal_update(&self, update: UserGoalUpdate) { + pub(crate) async fn record_user_goal_update( + &self, + update: UserGoalUpdate, + ) -> std::io::Result<()> { // Goal metadata must not initialize a model step, even on a goal-first thread. // Keep context construction off callers' stacks, including the TUI RPC dispatcher. let context = Box::pin(self.new_inject_items_context()).await; + // Earlier Code Mode messages may need the persistence lock before this can return. + let user_input_order = self.reserve_user_input_order().await; let _guard = thread_settings::acquire_persistence_lock(self).await; - self.record_conversation_items( - &context, - context.model_info(), - &[ContextualUserFragment::into(update)], - ) - .await; + let mut item = ContextualUserFragment::into(update); + Self::stamp_response_item_for_history(&mut item, &context.sub_id); + Self::assign_missing_response_item_id(&mut item); + let mut item = ResponseItemEnvelope { + item, + metadata: Some(CodexHarnessMetadata { + user_input_order: Some(user_input_order), + ..Default::default() + }), + }; + if let Some(live_thread) = self.live_thread() { + // Capture settings under the same permit as the instruction, including when a + // goal creates the rollout. No fallible settings catch-up runs after publication. + live_thread + .append_items(&[ + RolloutItem::EventMsg(thread_settings::applied_event(self).await), + RolloutItem::ResponseItem(item.clone()), + ]) + .await + .map_err(std::io::Error::other)?; + } + // A failed append or checkpoint must not change live authorization or its revision. + self.try_ensure_rollout_materialized(PersistContext::ThreadPreparation) + .await?; + self.state.lock().await.history.record_annotated_items( + std::slice::from_mut(&mut item), + context.model_info().truncation_policy.into(), + ); + self.send_raw_response_items(&context, std::slice::from_ref(&item.item)) + .await; + Ok(()) } pub(crate) async fn reserve_user_input_order(&self) -> u64 { diff --git a/codex-rs/core/src/session/rollout_reconstruction_tests.rs b/codex-rs/core/src/session/rollout_reconstruction_tests.rs index 5d750651f39c..05a7d37f9170 100644 --- a/codex-rs/core/src/session/rollout_reconstruction_tests.rs +++ b/codex-rs/core/src/session/rollout_reconstruction_tests.rs @@ -98,6 +98,7 @@ async fn recorded_questions_share_queued_input_order_across_resume() { let saved = serde_json::from_value(serde_json::to_value(saved).unwrap()).unwrap(); session .record_initial_history(InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: session.thread_id, history: Arc::new(saved), rollout_path: None, @@ -307,6 +308,7 @@ async fn record_initial_history_reconstructs_typed_inter_agent_message() { session .record_initial_history(InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: ThreadId::default(), history: Arc::new(vec![RolloutItem::InterAgentCommunication( communication.clone(), @@ -334,6 +336,7 @@ async fn record_initial_history_ignores_security_risk_scores() { session .record_initial_history(InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: ThreadId::default(), history: Arc::new(vec![ RolloutItem::ResponseItem(ResponseItemEnvelope::new(user_item.clone())), @@ -394,6 +397,7 @@ async fn record_initial_history_restores_world_state_baseline(input: BaselineTur session .record_initial_history(InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: ThreadId::default(), history: Arc::new(rollout_items), rollout_path: Some(PathBuf::from("/tmp/resume.jsonl")), @@ -467,6 +471,7 @@ async fn record_initial_history_resumed_bare_turn_context_does_not_hydrate_previ session .record_initial_history(InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: ThreadId::default(), history: Arc::new(rollout_items), rollout_path: Some(PathBuf::from("/tmp/resume.jsonl")), @@ -552,6 +557,7 @@ async fn record_initial_history_resumed_hydrates_previous_turn_settings_from_lif session .record_initial_history(InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: ThreadId::default(), history: Arc::new(rollout_items), rollout_path: Some(PathBuf::from("/tmp/resume.jsonl")), @@ -1180,6 +1186,7 @@ async fn record_initial_history_resumed_rollback_skips_only_user_turns() { session .record_initial_history(InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: ThreadId::default(), history: Arc::new(rollout_items), rollout_path: Some(PathBuf::from("/tmp/resume.jsonl")), @@ -1274,6 +1281,7 @@ async fn record_initial_history_resumed_rollback_drops_incomplete_user_turn_comp session .record_initial_history(InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: ThreadId::default(), history: Arc::new(rollout_items), rollout_path: Some(PathBuf::from("/tmp/resume.jsonl")), @@ -1341,6 +1349,7 @@ async fn record_initial_history_requires_surviving_full_snapshot_without_user_tu session .record_initial_history(InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: ThreadId::default(), history: Arc::new(rollout_items), rollout_path: Some(PathBuf::from("/tmp/resume.jsonl")), @@ -1374,6 +1383,7 @@ async fn record_initial_history_resumed_does_not_seed_reference_context_item_aft session .record_initial_history(InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: ThreadId::default(), history: Arc::new(rollout_items), rollout_path: Some(PathBuf::from("/tmp/resume.jsonl")), @@ -1986,6 +1996,7 @@ async fn record_initial_history_resumed_turn_context_after_compaction_reestablis session .record_initial_history(InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: ThreadId::default(), history: Arc::new(rollout_items), rollout_path: Some(PathBuf::from("/tmp/resume.jsonl")), @@ -2154,6 +2165,7 @@ async fn record_initial_history_resumed_aborted_turn_without_id_clears_active_tu session .record_initial_history(InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: ThreadId::default(), history: Arc::new(rollout_items), rollout_path: Some(PathBuf::from("/tmp/resume.jsonl")), @@ -2291,6 +2303,7 @@ async fn record_initial_history_resumed_unmatched_abort_preserves_active_turn_fo session .record_initial_history(InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: ThreadId::default(), history: Arc::new(rollout_items), rollout_path: Some(PathBuf::from("/tmp/resume.jsonl")), @@ -2423,6 +2436,7 @@ async fn record_initial_history_resumed_trailing_incomplete_turn_compaction_clea session .record_initial_history(InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: ThreadId::default(), history: Arc::new(rollout_items), rollout_path: Some(PathBuf::from("/tmp/resume.jsonl")), @@ -2476,6 +2490,7 @@ async fn record_initial_history_resumed_trailing_incomplete_turn_preserves_turn_ session .record_initial_history(InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: ThreadId::default(), history: Arc::new(rollout_items), rollout_path: Some(PathBuf::from("/tmp/resume.jsonl")), @@ -2621,6 +2636,7 @@ async fn record_initial_history_resumed_replaced_incomplete_compacted_turn_clear session .record_initial_history(InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: ThreadId::default(), history: Arc::new(rollout_items), rollout_path: Some(PathBuf::from("/tmp/resume.jsonl")), diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index e92aaeef8ae5..b85caf05813c 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -619,7 +619,12 @@ async fn warm_plugins_and_skills_for_session_init( skills_service: Arc, turn_environments: &TurnEnvironmentSnapshot, extensions: &codex_extension_api::ExtensionRegistry, + session_source: &SessionSource, ) -> Vec { + // Guardian does not consume skills, including the legacy empty-registry catalog. + if crate::guardian::is_basic_session_source(session_source) { + return Vec::new(); + } let plugins_input = config.plugins_config_input(); let plugin_outcome = plugins_manager.plugins_for_config(&plugins_input).await; if config.features.enabled(Feature::SkipHostSkillDiscovery) @@ -1006,9 +1011,10 @@ impl Session { let mcp_auth = persistence_auth.clone(); let thread_persistence_fut = async { if config.ephemeral { - Ok::<_, anyhow::Error>((None, LiveThreadInitGuard::new(/*live_thread*/ None))) + Ok::<_, anyhow::Error>((None, LiveThreadInitGuard::new(/*live_thread*/ None), None)) } else { let mut local_guard = LiveThreadInitGuard::default(); + let mut resume_context = None; let mut managed_guard = match &startup { Some(startup) => Some(startup.persistence.lock().await), None => None, @@ -1077,6 +1083,7 @@ impl Session { } InitialHistory::Resumed(resumed_history) => { let params = ResumeThreadParams { + history_revision: resumed_history.history_revision.clone(), thread_id: resumed_history.conversation_id, rollout_path: resumed_history.rollout_path.clone(), history: Some(resumed_history.history.clone()), @@ -1091,16 +1098,23 @@ impl Session { }, }, }; - guard - .acquire(LiveThread::resume( - Arc::clone(&thread_store), - session_configuration.history_mode, - params, - )) - .await? + let store = Arc::clone(&thread_store); + let history_mode = session_configuration.history_mode; + let (context_tx, context_rx) = tokio::sync::oneshot::channel(); + let live_thread = guard + .acquire(async move { + let (live_thread, history) = + LiveThread::resume(store, history_mode, params).await?; + // Cancellation still leaves the writer with the acquisition guard. + let _ = context_tx.send(history); + Ok(live_thread) + }) + .await?; + resume_context = Some(context_rx.await?); + live_thread } }; - Ok((Some(live_thread), local_guard)) + Ok((Some(live_thread), local_guard, resume_context)) } } .instrument(info_span!( @@ -1182,11 +1196,18 @@ impl Session { let (thread_persistence_result, state_db_ctx, (auth, mcp_projection)) = tokio::join!(thread_persistence_fut, state_db_fut, auth_and_mcp_fut); - let (live_thread, mut live_thread_init) = thread_persistence_result.map_err(|e| { - error!("failed to initialize thread persistence: {e:#}"); - e - })?; + let (live_thread, mut live_thread_init, resume_context) = thread_persistence_result + .map_err(|e| { + error!("failed to initialize thread persistence: {e:#}"); + e + })?; let session_result: anyhow::Result> = async { + if let InitialHistory::Resumed(resumed) = &mut initial_history + && let Some(history) = resume_context + { + resumed.history = history; + resumed.history_revision = None; + } let rollout_path = if let Some(live_thread) = live_thread.as_ref() { live_thread.local_rollout_path().await? } else { @@ -1450,6 +1471,7 @@ impl Session { Arc::clone(&skills_service), &resolved_environments, extensions.as_ref(), + &session_configuration.session_source, ) .instrument(info_span!( "session_init.plugin_skill_warmup", diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 8221619a21c5..b3b682793608 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -2484,6 +2484,7 @@ async fn record_initial_history_reconstructs_resumed_transcript() { session .record_initial_history(InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: ThreadId::default(), history: Arc::new(rollout_items), rollout_path: Some(PathBuf::from("/tmp/resume.jsonl")), @@ -3101,6 +3102,7 @@ async fn prepares_resumed_history_before_installing_it() { session .record_initial_history(InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: ThreadId::default(), history: Arc::new(vec![RolloutItem::ResponseItem(ResponseItemEnvelope { item: resumed_item, @@ -3161,6 +3163,7 @@ fn resolve_multi_agent_version_handles_unset_and_legacy_history() { assert_eq!( resolve_multi_agent_version( &InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: thread_id, history: Arc::new(Vec::new()), rollout_path: None, @@ -3172,6 +3175,7 @@ fn resolve_multi_agent_version_handles_unset_and_legacy_history() { assert_eq!( resolve_multi_agent_version( &InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: thread_id, history: Arc::new(Vec::new()), rollout_path: None, @@ -3183,6 +3187,7 @@ fn resolve_multi_agent_version_handles_unset_and_legacy_history() { assert_eq!( resolve_multi_agent_version( &InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: thread_id, history: Arc::new(vec![session_meta_item( thread_id, @@ -3248,6 +3253,7 @@ async fn resumed_history_injects_initial_context_on_first_context_update_only() session .record_initial_history(InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: ThreadId::default(), history: Arc::new(rollout_items), rollout_path: Some(PathBuf::from("/tmp/resume.jsonl")), @@ -3358,6 +3364,7 @@ async fn record_initial_history_seeds_token_info_from_rollout() { session .record_initial_history(InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: ThreadId::default(), history: Arc::new(rollout_items), rollout_path: Some(PathBuf::from("/tmp/resume.jsonl")), @@ -5869,6 +5876,7 @@ async fn settings_checkpoint_waits_for_accepted_settings_persistence() { /*reference_context_item*/ None, /*world_state_baseline*/ None, CompactedHistoryMetadata { + input_goal_ids: Default::default(), message: "summary".to_string(), window_number, window_ids, @@ -6002,6 +6010,7 @@ async fn mcp_attribution_checkpoints_cover_batch_prefixes_compaction_and_restore /*reference_context_item*/ None, /*world_state_baseline*/ None, CompactedHistoryMetadata { + input_goal_ids: Default::default(), message: "summary".to_string(), window_number, window_ids, @@ -6125,7 +6134,25 @@ async fn compaction_persists_resume_metadata_and_companion_records() { thread_settings: session.thread_settings_snapshot().await, }; + let mut first_live_history = None; for with_baselines in [true, false] { + let input_goal_ids = + crate::context::UserGoalUpdate::message_ids(session.clone_history().await.raw_items()); + // The goal edit is accepted after the compaction input was captured. + let accepted_goal = if with_baselines { + session + .record_user_goal_update(crate::context::UserGoalUpdate::Clear) + .await + .expect("record concurrent clear"); + session + .clone_history() + .await + .annotated_items() + .last() + .cloned() + } else { + None + }; let (window_number, window_ids) = session.advance_auto_compact_window().await; session .replace_compacted_history( @@ -6133,6 +6160,7 @@ async fn compaction_persists_resume_metadata_and_companion_records() { with_baselines.then_some(turn_context_baseline.clone()), with_baselines.then_some(Arc::clone(&world_state)), CompactedHistoryMetadata { + input_goal_ids, message: String::new(), window_number, window_ids, @@ -6142,6 +6170,11 @@ async fn compaction_persists_resume_metadata_and_companion_records() { }, ) .await; + if let Some(accepted_goal) = accepted_goal { + let live_history = session.clone_history().await.annotated_items().to_vec(); + assert_eq!(&live_history[1..], &[accepted_goal]); + first_live_history = Some(live_history); + } } session.flush_rollout().await.expect("flush checkpoints"); @@ -6163,6 +6196,7 @@ async fn compaction_persists_resume_metadata_and_companion_records() { else { panic!("unexpected compaction records: {compaction_items:#?}"); }; + assert_eq!(first.replacement_history, first_live_history); assert_eq!(first.resume_metadata.as_ref(), Some(&expected)); assert_eq!(second.resume_metadata.as_ref(), Some(&expected)); assert_eq!( @@ -7269,6 +7303,7 @@ async fn resumed_root_session_uses_thread_id_as_session_id() { let thread_id = ThreadId::new(); let (session, rx_event) = make_session_with_history_source_and_agent_control_and_rx( InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: thread_id, history: Arc::new(Vec::new()), rollout_path: None, @@ -7304,6 +7339,7 @@ async fn resumed_subagent_session_restores_persisted_session_id() { }); let (session, rx_event) = make_session_with_history_source_and_agent_control_and_rx( InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: thread_id, history: Arc::new(vec![RolloutItem::SessionMeta(SessionMetaLine { meta: SessionMeta { @@ -7365,6 +7401,7 @@ async fn resumed_copied_fork_ignores_source_history_base() { ]; let (session, _rx_event) = make_session_with_history_source_and_agent_control_and_rx( InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: thread_id, history: Arc::new(history), rollout_path: None, diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index 0d7b59d77ea6..777da7411bbe 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -1205,6 +1205,7 @@ impl Session { per_turn_config.codex_home.as_path(), ); let skills_snapshot = if matches!(build_mode, TurnContextBuildMode::InjectItems) + || crate::guardian::is_basic_session_source(&session_configuration.session_source) || (per_turn_config .features .enabled(Feature::SkipHostSkillDiscovery) diff --git a/codex-rs/core/src/session/world_state.rs b/codex-rs/core/src/session/world_state.rs index c0d7dea64d78..1dc5b0f2191e 100644 --- a/codex-rs/core/src/session/world_state.rs +++ b/codex-rs/core/src/session/world_state.rs @@ -12,6 +12,7 @@ use crate::context::world_state::ContextWindowGuidanceState; use crate::context::world_state::EnvironmentsInstructionsState; use crate::context::world_state::EnvironmentsState; use crate::context::world_state::ManagedDeveloperInstructionsState; +use crate::context::world_state::ModelCatalogState; use crate::context::world_state::ModelInstructionsState; use crate::context::world_state::MultiAgentModeState; use crate::context::world_state::MultiAgentUsageHintState; @@ -21,6 +22,7 @@ use crate::context::world_state::PluginsInstructionsState; use crate::context::world_state::RealtimeState; use crate::context::world_state::ToolsState; use crate::context::world_state::WorldState; +use crate::tools::handlers::multi_agents_spec::MULTI_AGENT_V1_NAMESPACE; use codex_connectors::AppToolPolicyEvaluator; use codex_extension_api::WorldStateContributionInput; use codex_features::Feature; @@ -31,6 +33,7 @@ use codex_prompts::render_model_instructions; use codex_protocol::error::Result as CodexResult; use codex_protocol::models::BaseInstructionsProvenance; use codex_protocol::protocol::MultiAgentVersion; +use codex_tools::ToolName; const MAX_ENVIRONMENT_SUBAGENTS: usize = 8; const MAX_ENVIRONMENT_SUBAGENT_BYTES: usize = 1_024; @@ -324,6 +327,45 @@ impl Session { world_state.add_section(usage_hint); } world_state.add_section(multi_agent_mode); + let spawn_tool = match turn_context.multi_agent_version { + MultiAgentVersion::Disabled => None, + MultiAgentVersion::V1 => Some(ToolName::new( + Some(MULTI_AGENT_V1_NAMESPACE.to_string()), + "spawn_agent", + )), + MultiAgentVersion::V2 => turn_context + .config + .multi_agent_v2 + .expose_spawn_agent_model_overrides + .then(|| { + ToolName::new( + turn_context + .provider + .capabilities() + .namespace_tools + .then(|| turn_context.config.multi_agent_v2.tool_namespace.clone()) + .flatten(), + "spawn_agent", + ) + }), + }; + let model_overrides_available = + spawn_tool.is_some_and(|name| step_context.tool_router.exposes_tool(&name)); + world_state.add_section( + if model_overrides_available + && turn_context + .config + .features + .enabled(Feature::ModelCatalogInContext) + { + ModelCatalogState::new( + &turn_context.available_models, + turn_context.multi_agent_version, + ) + } else { + ModelCatalogState::default() + }, + ); if !crate::guardian::is_basic_session_source(&turn_context.session_source) { world_state.add_section(ManagedDeveloperInstructionsState::new( turn_context diff --git a/codex-rs/core/src/thread_manager.rs b/codex-rs/core/src/thread_manager.rs index ba195d8e0cdb..187e1d793e56 100644 --- a/codex-rs/core/src/thread_manager.rs +++ b/codex-rs/core/src/thread_manager.rs @@ -1492,6 +1492,7 @@ impl ThreadManager { prepared: PreparedFork, ) -> CodexResult { let history = InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: prepared.source_thread_id, history: Arc::clone(&prepared.model_context), rollout_path: None, @@ -2466,6 +2467,7 @@ fn stored_thread_to_initial_history( )) })?; Ok(InitialHistory::Resumed(ResumedHistory { + history_revision: history.revision, conversation_id: thread_id, history: Arc::new(history.items), rollout_path: rollout_path.or(stored_thread.rollout_path), diff --git a/codex-rs/core/src/thread_manager_tests.rs b/codex-rs/core/src/thread_manager_tests.rs index 99321ce694e7..af0848466e42 100644 --- a/codex-rs/core/src/thread_manager_tests.rs +++ b/codex-rs/core/src/thread_manager_tests.rs @@ -109,6 +109,7 @@ async fn live_fork_keeps_instructions_when_source_is_unloaded_during_setup() { .await .expect("start source"); let history = InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: source.thread_id, history: Arc::new(Vec::new()), rollout_path: None, @@ -323,6 +324,7 @@ async fn reserved_thread_id_is_used_without_changing_normal_id_generation() { .expect("start reserved thread"); let mut resumed_options = StartThreadOptions::new(config.clone()); resumed_options.initial_history = InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: reserved.thread_id, history: Arc::new(Vec::new()), rollout_path: None, @@ -2459,6 +2461,7 @@ async fn rollout_path_resume_and_fork_read_history_through_thread_store() { .resume_thread_with_history( config.clone(), InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: source.thread_id, history: Arc::new(vec![RolloutItem::ResponseItem(user_msg("hello").into())]), rollout_path: Some(rollout_path.clone()), diff --git a/codex-rs/core/src/tools/handlers/multi_agents_spec.rs b/codex-rs/core/src/tools/handlers/multi_agents_spec.rs index 52c24f8f0611..27a7ab9a2606 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_spec.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_spec.rs @@ -15,6 +15,8 @@ pub const MULTI_AGENT_V1_NAMESPACE: &str = "multi_agent_v1"; const MULTI_AGENT_V1_NAMESPACE_DESCRIPTION: &str = "Tools for spawning and managing sub-agents."; const SPAWN_AGENT_INHERITED_MODEL_GUIDANCE: &str = "Spawned agents inherit your current model by default. Omit `model` to use that preferred default; set `model` only when an explicit override is needed."; +const SPAWN_AGENT_MODEL_CATALOG_GUIDANCE: &str = + "Pick model overrides from the latest listing."; const SPAWN_AGENT_TYPE_OVERRIDE_DESCRIPTION_V1: &str = "Agent type override for the new agent. Omit to inherit the parent agent type with a full-history fork; otherwise, `default` is used."; const SPAWN_AGENT_MODEL_OVERRIDE_DESCRIPTION: &str = "Model override for the new agent. Omit unless an explicit override is needed."; @@ -28,6 +30,7 @@ pub struct SpawnAgentToolOptions { pub hide_agent_type_model_reasoning: bool, pub expose_spawn_agent_model_overrides: bool, pub multi_agent_version: MultiAgentVersion, + pub model_catalog_in_context: bool, pub usage_hint_text: Option, } @@ -40,6 +43,7 @@ impl Default for SpawnAgentToolOptions { hide_agent_type_model_reasoning: false, expose_spawn_agent_model_overrides: false, multi_agent_version: MultiAgentVersion::Disabled, + model_catalog_in_context: false, usage_hint_text: None, } } @@ -63,9 +67,11 @@ impl Default for WaitAgentTimeoutOptions { } pub fn create_spawn_agent_tool_v1(options: SpawnAgentToolOptions) -> ToolSpec { - let available_models_description = (!options.hide_agent_type_model_reasoning).then(|| { - spawn_agent_models_description(&options.available_models, options.multi_agent_version) - }); + let available_models_description = (!options.model_catalog_in_context + && !options.hide_agent_type_model_reasoning) + .then(|| { + spawn_agent_models_description(&options.available_models, options.multi_agent_version) + }); let inherited_model_guidance = (!options.hide_agent_type_model_reasoning).then_some(SPAWN_AGENT_INHERITED_MODEL_GUIDANCE); let return_value_description = @@ -85,6 +91,7 @@ pub fn create_spawn_agent_tool_v1(options: SpawnAgentToolOptions) -> ToolSpec { name: "spawn_agent".to_string(), description: spawn_agent_tool_description( available_models_description.as_deref(), + options.model_catalog_in_context, inherited_model_guidance, return_value_description, options.usage_hint_text, @@ -101,9 +108,11 @@ pub fn create_spawn_agent_tool_v2( options: SpawnAgentToolOptions, description_override: Option<&str>, ) -> ToolSpec { - let available_models_description = options.expose_spawn_agent_model_overrides.then(|| { - spawn_agent_models_description(&options.available_models, options.multi_agent_version) - }); + let available_models_description = (!options.model_catalog_in_context + && options.expose_spawn_agent_model_overrides) + .then(|| { + spawn_agent_models_description(&options.available_models, options.multi_agent_version) + }); let inherited_model_guidance = (options.expose_spawn_agent_model_overrides && !options.hide_agent_type_model_reasoning) .then_some(SPAWN_AGENT_INHERITED_MODEL_GUIDANCE); @@ -127,6 +136,10 @@ pub fn create_spawn_agent_tool_v2( name: "spawn_agent".to_string(), description: spawn_agent_tool_description_v2( available_models_description.as_deref(), + options.model_catalog_in_context, + options + .expose_spawn_agent_model_overrides + .then_some(SPAWN_AGENT_MODEL_CATALOG_GUIDANCE), inherited_model_guidance, options.usage_hint_text, description_override, @@ -673,18 +686,30 @@ fn hide_spawn_agent_metadata_options(properties: &mut BTreeMap, + model_catalog_in_context: bool, inherited_model_guidance: Option<&str>, return_value_description: &str, usage_hint_text: Option, ) -> String { - let agent_role_guidance = available_models_description.unwrap_or_default(); + let model_catalog_guidance = inherited_model_guidance + .map(|_| SPAWN_AGENT_MODEL_CATALOG_GUIDANCE) + .unwrap_or_default(); let inherited_model_guidance = inherited_model_guidance.unwrap_or_default(); - let tool_description = format!( - r#" - {agent_role_guidance} + let tool_description = if model_catalog_in_context { + format!( + r#" + Spawn a sub-agent for a well-scoped task. {return_value_description} {inherited_model_guidance} +{model_catalog_guidance}"# + ) + } else { + let available_models_description = available_models_description.unwrap_or_default(); + format!( + r#" + {available_models_description} Spawn a sub-agent for a well-scoped task. {return_value_description} {inherited_model_guidance}"# - ); + ) + }; if let Some(usage_hint_text) = usage_hint_text { return format!( @@ -693,19 +718,20 @@ fn spawn_agent_tool_description( {usage_hint_text}"# ); } - let agent_role_usage_hint = available_models_description - .map(|_| { - "Agent-role guidance below only helps choose which agent to use after spawning is already authorized; it never authorizes spawning by itself." - }) - .unwrap_or_default(); + let agent_role_usage_hint = if model_catalog_in_context { + "" + } else if available_models_description.is_some() { + "\nAgent-role guidance below only helps choose which agent to use after spawning is already authorized; it never authorizes spawning by itself." + } else { + "\n" + }; format!( r#" {tool_description} This spawn_agent tool provides you access to sub-agents that inherit your current model by default. Do not set the `model` field unless the user explicitly asks for a different model. You should follow the rules and guidelines below to use this tool. Do not spawn sub-agents unless the user or applicable AGENTS.md/skill instructions explicitly ask for sub-agents, delegation, or parallel agent work. -Requests for depth, thoroughness, research, investigation, or detailed codebase analysis do not count as permission to spawn. -{agent_role_usage_hint} +Requests for depth, thoroughness, research, investigation, or detailed codebase analysis do not count as permission to spawn.{agent_role_usage_hint} ### When to delegate vs. do the subtask yourself - First, quickly analyze the overall user task and form a succinct high-level plan. Identify which tasks are immediate blockers on the critical path, and which tasks are sidecar tasks that are needed but can run in parallel without blocking the next local step. As part of that plan, explicitly decide what immediate task you should do locally right now. Do this planning step before delegating to agents so you do not hand off the immediate blocking task to a submodel and then waste time waiting on it. @@ -740,28 +766,37 @@ Requests for depth, thoroughness, research, investigation, or detailed codebase fn spawn_agent_tool_description_v2( available_models_description: Option<&str>, + model_catalog_in_context: bool, + model_catalog_guidance: Option<&str>, inherited_model_guidance: Option<&str>, usage_hint_text: Option, description: Option<&str>, ) -> String { - let agent_role_guidance = available_models_description.unwrap_or_default(); + let model_catalog_guidance = model_catalog_guidance.unwrap_or_default(); let inherited_model_guidance = inherited_model_guidance.unwrap_or_default(); + let (catalog_prefix, catalog_suffix) = if model_catalog_in_context { + (String::new(), format!("\n{model_catalog_guidance}")) + } else { + let available_models_description = available_models_description.unwrap_or_default(); + ( + format!(" {available_models_description}\n"), + String::new(), + ) + }; let tool_description = if let Some(description) = description { format!( r#" - {agent_role_guidance} - {description} -{inherited_model_guidance}"# +{catalog_prefix} {description} +{inherited_model_guidance}{catalog_suffix}"# ) } else { format!( r#" - {agent_role_guidance} - Spawns an agent to work on the specified task. If your current task is `/root/task1` and you spawn_agent with task_name "task_3" the agent will have canonical task name `/root/task1/task_3`. +{catalog_prefix} Spawns an agent to work on the specified task. If your current task is `/root/task1` and you spawn_agent with task_name "task_3" the agent will have canonical task name `/root/task1/task_3`. You are then able to refer to this agent as `task_3` or `/root/task1/task_3` interchangeably. However an agent `/root/task2/task_3` would only be able to communicate with this agent via its canonical name `/root/task1/task_3`. The spawned agent will have the same tools as you and the ability to spawn its own subagents. -{inherited_model_guidance} +{inherited_model_guidance}{catalog_suffix} It will be able to send you and other running agents messages, and its final answer will be provided to you when it finishes. The new agent's canonical task name will be provided to it along with the message. diff --git a/codex-rs/core/src/tools/handlers/multi_agents_spec_tests.rs b/codex-rs/core/src/tools/handlers/multi_agents_spec_tests.rs index e2f943c7ab86..493ba3f02afc 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_spec_tests.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_spec_tests.rs @@ -1,64 +1,19 @@ use super::*; -use codex_protocol::openai_models::ModelPreset; -use codex_protocol::openai_models::ModelServiceTier; -use codex_protocol::openai_models::ReasoningEffort; -use codex_protocol::openai_models::ReasoningEffortPreset; use codex_tools::JsonSchemaPrimitiveType; use codex_tools::JsonSchemaType; use pretty_assertions::assert_eq; use serde_json::json; -fn model_preset(id: &str, show_in_picker: bool) -> ModelPreset { - ModelPreset { - id: id.to_string(), - model: format!("{id}-model"), - display_name: format!("{id} display"), - description: format!("{id} description"), - model_specialty: None, - default_reasoning_effort: ReasoningEffort::Medium, - supported_reasoning_efforts: vec![ReasoningEffortPreset { - effort: ReasoningEffort::Medium, - description: "Balanced".to_string(), - }], - supports_personality: false, - additional_speed_tiers: Vec::new(), - service_tiers: vec![ModelServiceTier { - id: "priority".to_string(), - name: "Fast".to_string(), - description: "1.5x speed, increased usage".to_string(), - }], - default_service_tier: None, - available_access_programs: None, - is_default: false, - upgrade: None, - show_in_picker, - multi_agent_version: Some(MultiAgentVersion::V2), - availability_nux: None, - supported_in_api: true, - input_modalities: Vec::new(), - } -} - #[test] -fn spawn_agent_tool_v2_requires_task_name_and_lists_visible_models() { - let mut legacy = model_preset("legacy", /*show_in_picker*/ true); - legacy.multi_agent_version = Some(MultiAgentVersion::V1); - let mut disabled = model_preset("disabled", /*show_in_picker*/ true); - disabled.multi_agent_version = Some(MultiAgentVersion::Disabled); +fn spawn_agent_tool_v2_requires_task_name() { let tool = create_spawn_agent_tool_v2( SpawnAgentToolOptions { - available_models: vec![ - model_preset("visible", /*show_in_picker*/ true), - model_preset("hidden", /*show_in_picker*/ false), - legacy, - disabled, - ], agent_type_description: "role help".to_string(), expose_agent_type: true, hide_agent_type_model_reasoning: false, expose_spawn_agent_model_overrides: true, - multi_agent_version: MultiAgentVersion::V2, usage_hint_text: None, + ..Default::default() }, /*description_override*/ None, ); @@ -84,18 +39,6 @@ fn spawn_agent_tool_v2_requires_task_name_and_lists_visible_models() { assert!(description.contains("The spawned agent will have the same tools as you")); assert!(!description.contains("max_concurrent_threads_per_session")); assert!(description.contains(SPAWN_AGENT_INHERITED_MODEL_GUIDANCE)); - assert!( - description - .contains("Available model overrides (optional; inherited parent model is preferred):") - ); - assert!(description.contains( - "- `visible-model`: visible description Reasoning efforts: medium (default). Service tiers: priority." - )); - assert!(description.contains( - "- `legacy-model`: legacy description Reasoning efforts: medium (default). Service tiers: priority." - )); - assert!(!description.contains("hidden-model")); - assert!(!description.contains("disabled-model")); assert!(properties.contains_key("task_name")); assert!(properties.contains_key("message")); assert_eq!( @@ -133,10 +76,8 @@ fn spawn_agent_tool_v2_requires_task_name_and_lists_visible_models() { #[test] fn spawn_agent_catalog_description_preserves_generated_context() { let options = SpawnAgentToolOptions { - available_models: vec![model_preset("visible", /*show_in_picker*/ true)], agent_type_description: "Available agent roles: explorer".to_string(), expose_spawn_agent_model_overrides: true, - multi_agent_version: MultiAgentVersion::V2, usage_hint_text: Some("Local usage hint.".to_string()), ..Default::default() }; @@ -155,7 +96,6 @@ fn spawn_agent_catalog_description_preserves_generated_context() { .description .contains("Catalog spawning guidance.") ); - assert!(configured_tool.description.contains("`visible-model`")); assert!( configured_tool .description @@ -174,13 +114,12 @@ fn spawn_agent_catalog_description_preserves_generated_context() { #[test] fn spawn_agent_tool_v1_keeps_legacy_fork_context_field() { let tool = create_spawn_agent_tool_v1(SpawnAgentToolOptions { - available_models: Vec::new(), agent_type_description: "role help".to_string(), expose_agent_type: true, hide_agent_type_model_reasoning: false, expose_spawn_agent_model_overrides: true, - multi_agent_version: MultiAgentVersion::V1, usage_hint_text: None, + ..Default::default() }); let ToolSpec::Namespace(namespace) = tool else { @@ -224,73 +163,16 @@ fn spawn_agent_tool_v1_keeps_legacy_fork_context_field() { assert!(!properties.contains_key("service_tier")); } -#[test] -fn spawn_agent_tool_caps_visible_model_summaries() { - let tool = create_spawn_agent_tool_v2( - SpawnAgentToolOptions { - available_models: vec![ - model_preset("first", /*show_in_picker*/ true), - model_preset("second", /*show_in_picker*/ true), - model_preset("third", /*show_in_picker*/ true), - model_preset("fourth", /*show_in_picker*/ true), - model_preset("fifth", /*show_in_picker*/ true), - model_preset("sixth", /*show_in_picker*/ true), - ], - agent_type_description: "role help".to_string(), - expose_agent_type: true, - hide_agent_type_model_reasoning: false, - expose_spawn_agent_model_overrides: true, - multi_agent_version: MultiAgentVersion::V2, - usage_hint_text: None, - }, - /*description_override*/ None, - ); - - let ToolSpec::Function(ResponsesApiTool { description, .. }) = tool else { - panic!("spawn_agent should be a function tool"); - }; - - for model in ["first", "second", "third", "fourth", "fifth"] { - assert!( - description.contains(&format!("`{model}-model`")), - "expected {model} model summary in spawn_agent description: {description:?}" - ); - } - assert!(!description.contains("`sixth-model`")); -} - -#[test] -fn spawn_agent_tool_caps_reasoning_effort_value_length() { - let mut model = model_preset("visible", /*show_in_picker*/ true); - let custom_effort = ReasoningEffort::Custom( - "é".repeat(MAX_REASONING_EFFORT_CHARS_IN_SPAWN_AGENT_DESCRIPTION + 1), - ); - model.default_reasoning_effort = custom_effort.clone(); - model.supported_reasoning_efforts = vec![ReasoningEffortPreset { - effort: custom_effort, - description: "Model-defined".to_string(), - }]; - - assert_eq!( - spawn_agent_models_description(&[model], MultiAgentVersion::V2), - format!( - "Available model overrides (optional; inherited parent model is preferred):\n- `visible-model`: visible description Reasoning efforts: {} (default). Service tiers: priority.", - "é".repeat(MAX_REASONING_EFFORT_CHARS_IN_SPAWN_AGENT_DESCRIPTION) - ) - ); -} - #[test] fn spawn_agent_tool_keeps_model_controls_when_spawn_metadata_is_hidden() { let tool = create_spawn_agent_tool_v2( SpawnAgentToolOptions { - available_models: vec![model_preset("visible", /*show_in_picker*/ true)], agent_type_description: "role help".to_string(), expose_agent_type: false, hide_agent_type_model_reasoning: true, expose_spawn_agent_model_overrides: true, - multi_agent_version: MultiAgentVersion::V2, usage_hint_text: None, + ..Default::default() }, /*description_override*/ None, ); @@ -313,20 +195,18 @@ fn spawn_agent_tool_keeps_model_controls_when_spawn_metadata_is_hidden() { assert!(properties.contains_key("reasoning_effort")); assert!(!properties.contains_key("service_tier")); assert!(!description.contains(SPAWN_AGENT_INHERITED_MODEL_GUIDANCE)); - assert!(description.contains("Available model overrides")); } #[test] fn spawn_agent_tool_hides_model_controls_without_override_exposure() { let tool = create_spawn_agent_tool_v2( SpawnAgentToolOptions { - available_models: vec![model_preset("visible", /*show_in_picker*/ true)], agent_type_description: "role help".to_string(), expose_agent_type: false, hide_agent_type_model_reasoning: true, expose_spawn_agent_model_overrides: false, - multi_agent_version: MultiAgentVersion::V2, usage_hint_text: None, + ..Default::default() }, Some(""), ); diff --git a/codex-rs/core/src/tools/router.rs b/codex-rs/core/src/tools/router.rs index d9f1f63eed9c..fbf0e773be5b 100644 --- a/codex-rs/core/src/tools/router.rs +++ b/codex-rs/core/src/tools/router.rs @@ -173,7 +173,7 @@ impl ToolRouter { } // Answers if the tool plan lets the model invoke the tool directly, through code mode, or deferred tool search. - pub(super) fn exposes_tool(&self, name: &ToolName) -> bool { + pub(crate) fn exposes_tool(&self, name: &ToolName) -> bool { let name = name.clone().with_default_namespace(); if self .code_mode_tool_names diff --git a/codex-rs/core/src/tools/spec_plan.rs b/codex-rs/core/src/tools/spec_plan.rs index bb1474feb528..379facebdc62 100644 --- a/codex-rs/core/src/tools/spec_plan.rs +++ b/codex-rs/core/src/tools/spec_plan.rs @@ -1320,6 +1320,11 @@ fn add_collaboration_tools(context: &CoreToolPlanContext<'_>, registry: &mut Too SpawnAgentHandlerV2::new( SpawnAgentToolOptions { available_models: turn_context.available_models.clone(), + multi_agent_version: turn_context.multi_agent_version, + model_catalog_in_context: turn_context + .config + .features + .enabled(Feature::ModelCatalogInContext), agent_type_description, expose_agent_type: !turn_context.config.agent_roles.is_empty(), hide_agent_type_model_reasoning: hide_spawn_agent_metadata, @@ -1327,7 +1332,6 @@ fn add_collaboration_tools(context: &CoreToolPlanContext<'_>, registry: &mut Too .config .multi_agent_v2 .expose_spawn_agent_model_overrides, - multi_agent_version: turn_context.multi_agent_version, usage_hint_text: turn_context .config .multi_agent_v2 @@ -1337,7 +1341,7 @@ fn add_collaboration_tools(context: &CoreToolPlanContext<'_>, registry: &mut Too spawn_agent_description.map(str::to_owned), ), tool_namespace, - // Spawn composes the selected description with runtime model and usage guidance. + // Spawn composes the selected description with inheritance and usage guidance. /*description_override*/ None, model_messages.multi_agent_tool_parameters_override("spawn_agent"), @@ -1404,11 +1408,15 @@ fn add_collaboration_tools(context: &CoreToolPlanContext<'_>, registry: &mut Too registry.add_with_exposure( SpawnAgentHandler::new(SpawnAgentToolOptions { available_models: turn_context.available_models.clone(), + multi_agent_version: turn_context.multi_agent_version, + model_catalog_in_context: turn_context + .config + .features + .enabled(Feature::ModelCatalogInContext), agent_type_description, expose_agent_type: !turn_context.config.agent_roles.is_empty(), hide_agent_type_model_reasoning: false, expose_spawn_agent_model_overrides: true, - multi_agent_version: turn_context.multi_agent_version, usage_hint_text: turn_context.config.multi_agent_v2.usage_hint_text.clone(), }), exposure, diff --git a/codex-rs/core/tests/common/context_snapshot/normalize.rs b/codex-rs/core/tests/common/context_snapshot/normalize.rs index b9d82891ff1a..a4da82ad46dc 100644 --- a/codex-rs/core/tests/common/context_snapshot/normalize.rs +++ b/codex-rs/core/tests/common/context_snapshot/normalize.rs @@ -330,6 +330,7 @@ fn known_segment_name(text: &str, source: TextSource<'_>) -> Option { | "collaboration_mode" | "multi_agent_role" | "multi_agent_mode" + | "model_catalog" | "apps_instructions" | "skills_instructions" | "plugins_instructions" diff --git a/codex-rs/core/tests/suite/agent_control_tests.rs b/codex-rs/core/tests/suite/agent_control_tests.rs index 551b2cedb367..8e12fad0de29 100644 --- a/codex-rs/core/tests/suite/agent_control_tests.rs +++ b/codex-rs/core/tests/suite/agent_control_tests.rs @@ -405,6 +405,7 @@ async fn host_factory_follows_thread_lifecycle() -> anyhow::Result<()> { test.codex.flush_rollout().await?; let saved = test.codex.load_history(/*include_archived*/ false).await?; let history = InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: root_id, history: Arc::new(saved.items), rollout_path: test.codex.rollout_path(), diff --git a/codex-rs/core/tests/suite/agents_md.rs b/codex-rs/core/tests/suite/agents_md.rs index 04f4d023f9cc..f2f72613605c 100644 --- a/codex-rs/core/tests/suite/agents_md.rs +++ b/codex-rs/core/tests/suite/agents_md.rs @@ -279,6 +279,7 @@ pub(super) async fn persisted_resume_history( Ok(( thread_id, InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: thread_id, history: Arc::new( store @@ -1588,6 +1589,7 @@ async fn fork_preserves_thread_instructions( }) .await?; let history = InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: parent_id, history: Arc::new(stored.items), rollout_path: None, diff --git a/codex-rs/core/tests/suite/compact.rs b/codex-rs/core/tests/suite/compact.rs index 2dced3650df9..6549a92c8f55 100644 --- a/codex-rs/core/tests/suite/compact.rs +++ b/codex-rs/core/tests/suite/compact.rs @@ -4690,6 +4690,7 @@ async fn paginated_compaction_cold_resume_from_bounded_suffix() -> Result<()> { .resume_thread_with_history( test.config.clone(), codex_history::InitialHistory::Resumed(codex_history::ResumedHistory { + history_revision: None, conversation_id: context.thread_id, history: Arc::new(context.items), rollout_path: Some(path), diff --git a/codex-rs/core/tests/suite/fork_thread.rs b/codex-rs/core/tests/suite/fork_thread.rs index 4f5708b2204a..c90fb922467a 100644 --- a/codex-rs/core/tests/suite/fork_thread.rs +++ b/codex-rs/core/tests/suite/fork_thread.rs @@ -50,6 +50,7 @@ async fn ephemeral_fork_skips_stored_title_lookup() -> anyhow::Result<()> { ForkSnapshot::Interrupted, codex_core::StartThreadOptions::new(config), InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: test.session_configured.thread_id, history: Arc::new(Vec::new()), rollout_path: None, @@ -218,6 +219,7 @@ async fn fork_thread_restores_history_selection_and_preserves_explicit_clear() - ) .await?; let history = InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: test.session_configured.thread_id, history: Arc::new(vec![thread_settings_applied_item( test.session_configured.thread_id, @@ -332,6 +334,7 @@ async fn assert_copied_fork_persists_inherited_history(history_mode: ThreadHisto ForkSnapshot::Interrupted, codex_core::StartThreadOptions::new(test.config.clone()), InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: test.session_configured.thread_id, history: Arc::new(supplied_history), rollout_path: None, diff --git a/codex-rs/core/tests/suite/guardian_checkpoint_migration_tests.rs b/codex-rs/core/tests/suite/guardian_checkpoint_migration_tests.rs index 6d3130968fee..2f77de01eb71 100644 --- a/codex-rs/core/tests/suite/guardian_checkpoint_migration_tests.rs +++ b/codex-rs/core/tests/suite/guardian_checkpoint_migration_tests.rs @@ -75,6 +75,7 @@ pub(super) async fn resume( .start_thread(StartThreadOptions { environments: Some(environments), initial_history: InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: thread_id, history: Arc::new(history), rollout_path: None, diff --git a/codex-rs/core/tests/suite/guardian_environments_tests.rs b/codex-rs/core/tests/suite/guardian_environments_tests.rs index 2fceca3f12cb..452387a53e43 100644 --- a/codex-rs/core/tests/suite/guardian_environments_tests.rs +++ b/codex-rs/core/tests/suite/guardian_environments_tests.rs @@ -291,6 +291,174 @@ async fn guardian_reviews_target_environment_and_reuses_prefix(tool: &str) -> Re Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn guardian_reviews_with_offline_primary_executor() -> Result<()> { + skip_if_no_network!(Ok(())); + let listener = TcpListener::bind("127.0.0.1:0").await?; + let executor_url = format!("ws://{}", listener.local_addr()?); + let (attach, connection) = tokio::sync::oneshot::channel(); + let (disconnect, stop) = tokio::sync::oneshot::channel(); + let executor = AbortOnDropHandle::new(tokio::spawn(serve_environment_with_instruction_files( + listener, + "", + Some( + "---\nname: guardian-fixture-skill\ndescription: Source executor skill catalog entry.\n---\nUse the source skill.\n", + ), + connection, + stop, + ))); + attach.send(()).expect("attach primary executor"); + + let environment_id = "guardian-primary"; + let (request_permission, pending_permission) = tokio::sync::oneshot::channel(); + let (model, _) = start_streaming_sse_server(vec![ + vec![StreamingSseChunk { + gate: Some(pending_permission), + body: sse(vec![ + ev_function_call( + "permission", + "request_permissions", + &json!({ + "environment_id": environment_id, + "permissions": {"network": {"enabled": true}}, + "reason": "Review permission while the executor is offline.", + }) + .to_string(), + ), + ev_completed("parent"), + ]), + }], + vec![StreamingSseChunk { + gate: None, + body: sse(vec![ + ev_assistant_message("deny", r#"{"outcome":"deny"}"#), + ev_completed("review"), + ]), + }], + vec![StreamingSseChunk { + gate: None, + body: sse(vec![ev_completed("done")]), + }], + ]) + .await; + + let mut extensions = ExtensionRegistryBuilder::::new(); + codex_skills_extension::install(&mut extensions, |config: &Config| { + codex_skills_extension::SkillsExtensionConfig { + include_instructions: config.include_skill_instructions, + max_context_tokens: config.skill_max_context_tokens, + bundled_skills_enabled: false, + cloud_skill_enabled: false, + shadow_selection_enabled: false, + } + }); + let test = test_codex() + .with_extensions(Arc::new(extensions.build())) + .with_model_info_override("guardian-offline-skills-parent", |info| { + info.guardian = None; + info.auto_review_model_override = Some(info.slug.clone()); + }) + .with_config(|config| { + // Isolate skill discovery from the unchanged repository-instruction discovery path. + config.project_doc_max_bytes = 0; + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); + // Start the reviewer only after the executor goes offline, not during prewarm. + config.approvals_reviewer = ApprovalsReviewer::User; + config + .permissions + .set_permission_profile(PermissionProfile::read_only()) + .expect("set read-only permissions"); + config + .features + .enable(Feature::RequestPermissionsTool) + .expect("enable permission requests"); + config + .features + .disable(Feature::SkipHostSkillDiscovery) + .expect("keep host skill discovery enabled for the parent"); + config + .features + .disable(Feature::DeferredExecutor) + .expect("disable deferred executor"); + }) + // This test supplies its own executor mock, using host-native paths. + .build_with_streaming_server(&model) + .await?; + let manager = test.thread_manager.environment_manager(); + manager.upsert_environment( + environment_id.to_owned(), + executor_url, + /*connect_timeout*/ None, + )?; + manager + .get_environment(environment_id) + .context("primary executor")? + .wait_until_ready() + .await?; + let mut primary = test.executor_environment().selection().clone(); + primary.environment_id = environment_id.to_owned(); + primary.config = + EnvironmentConfigState::Ready(environment_config_for_selection(&test.config, &primary)); + + test.codex + .start_or_steer_turn( + TurnInputRequest::user_input(vec![UserInput::Text { + text: "Do not grant network permission.".to_owned(), + text_elements: Vec::new(), + }]) + .with_thread_settings(ThreadSettingsOverrides { + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + environments: Some(TurnEnvironmentSelections::new( + test.config.cwd.clone(), + vec![primary], + )), + ..Default::default() + }), + ) + .await?; + timeout( + Duration::from_secs(/*secs*/ 10), + model.wait_for_request_count(/*count*/ 1), + ) + .await + .context("parent must discover skills and reach the model while online")?; + disconnect.send(()).expect("disconnect primary executor"); + executor.await?; + request_permission.send(()).expect("request permission"); + + timeout( + Duration::from_secs(/*secs*/ 10), + model.wait_for_request_count(/*count*/ 2), + ) + .await + .context("Guardian startup and turn creation must not wait for host skill discovery")?; + let requests = model.requests().await; + let parent: Value = serde_json::from_slice(&requests[0])?; + let review: Value = serde_json::from_slice(&requests[1])?; + assert!( + parent["input"] + .to_string() + .contains("guardian-fixture-skill"), + "parent must advertise the skill discovered on its executor" + ); + assert_eq!( + review["client_metadata"]["x-openai-subagent"], + json!("guardian") + ); + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::GuardianAssessment(assessment) + if assessment.status == GuardianAssessmentStatus::Denied) + }) + .await; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + test.codex.shutdown_and_wait().await?; + model.shutdown().await; + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn guardian_revalidates_allow_with_offline_secondary_executor() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/core/tests/suite/guardian_history.rs b/codex-rs/core/tests/suite/guardian_history.rs index 6500826ba103..e0ad6f253fb2 100644 --- a/codex-rs/core/tests/suite/guardian_history.rs +++ b/codex-rs/core/tests/suite/guardian_history.rs @@ -110,6 +110,7 @@ async fn guardian_history_survives_restart_and_user_fork( let items: Vec = serde_json::from_value(serde_json::to_value(model_context.items)?)?; let history = InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: thread_id, history: Arc::new(items), rollout_path: None, diff --git a/codex-rs/core/tests/suite/guardian_persistence_tests.rs b/codex-rs/core/tests/suite/guardian_persistence_tests.rs index f306967d3aa2..60a072856a5f 100644 --- a/codex-rs/core/tests/suite/guardian_persistence_tests.rs +++ b/codex-rs/core/tests/suite/guardian_persistence_tests.rs @@ -73,7 +73,7 @@ impl ThreadStore for GatedReviewerStore { } delegate_store_methods! { - fn resume_thread(params: ResumeThreadParams) -> (); + fn resume_thread(params: ResumeThreadParams) -> Arc>; fn append_items(params: AppendThreadItemsParams) -> (); fn discard_thread(thread_id: ThreadId) -> (); fn load_history(params: LoadThreadHistoryParams) -> StoredThreadHistory; diff --git a/codex-rs/core/tests/suite/guardian_retained_context.rs b/codex-rs/core/tests/suite/guardian_retained_context.rs index f4d8f0a55915..eb627c135c0d 100644 --- a/codex-rs/core/tests/suite/guardian_retained_context.rs +++ b/codex-rs/core/tests/suite/guardian_retained_context.rs @@ -226,6 +226,7 @@ async fn resume(test: &TestCodex, thread: &CodexThread) -> Result Result> { let checkpoint: CompactedItem = serde_json::from_value(json!({ "message":"Root checkpoint.", "replacement_history":[], "retained_context":history.retained_context(), }))?; + test.codex + .append_rollout_items(&[RolloutItem::Compacted(checkpoint)]) + .await?; test.codex.flush_rollout().await?; - let mut rollout = test + let rollout = test .codex .load_history(/*include_archived*/ false) .await? .items; - rollout.push(RolloutItem::Compacted(checkpoint)); test.codex = crate::suite::guardian_checkpoint_migration::resume(&test, &test.codex, rollout).await?; expected.insert( diff --git a/codex-rs/core/tests/suite/guardian_subagent_authorization.rs b/codex-rs/core/tests/suite/guardian_subagent_authorization.rs index b06ee02ba88d..cf3f57075f5f 100644 --- a/codex-rs/core/tests/suite/guardian_subagent_authorization.rs +++ b/codex-rs/core/tests/suite/guardian_subagent_authorization.rs @@ -1686,6 +1686,7 @@ async fn run_guardian_subagent_review( .resume_thread_with_history( test.config.clone(), InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: root_thread_id, history: Arc::new(saved.items), rollout_path: None, diff --git a/codex-rs/core/tests/suite/multi_agent_restore_tests.rs b/codex-rs/core/tests/suite/multi_agent_restore_tests.rs index 6e7abcc142d2..e616b4a922ed 100644 --- a/codex-rs/core/tests/suite/multi_agent_restore_tests.rs +++ b/codex-rs/core/tests/suite/multi_agent_restore_tests.rs @@ -1,4 +1,4 @@ -//! Exercises read overlap and ordered identity registration when a V2 root resumes cold. +//! Exercises read overlap, ordered identities, and revision-aware history on cold V2 resumes. use super::ROLE_MODEL; use super::ROLE_NAME; @@ -10,8 +10,10 @@ use super::request_has_model; use anyhow::Context; use anyhow::Result; use codex_features::Feature; +use codex_history::RolloutItem; use codex_protocol::ThreadId; use codex_protocol::error::CodexErrorDetails; +use codex_protocol::models::ResponseItem; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::ThreadHistoryMode; use codex_thread_store::AppendThreadItemsParams; @@ -49,10 +51,13 @@ use tokio::sync::mpsc; use tokio::sync::oneshot; use tokio::time::timeout; +const LATE_EDIT: &str = "Durable edit after the child history snapshot"; + struct GatedChildMetadataStore { inner: Arc, gates: Mutex>>, failed_child: ThreadId, + edited_child: ThreadId, started: mpsc::UnboundedSender, completed: mpsc::UnboundedSender, } @@ -76,7 +81,6 @@ impl ThreadStore for GatedChildMetadataStore { delegate_store_methods! { fn create_thread(params: CreateThreadParams) -> (); - fn resume_thread(params: ResumeThreadParams) -> (); fn append_items(params: AppendThreadItemsParams) -> (); fn flush_thread(thread_id: ThreadId) -> (); fn shutdown_thread(thread_id: ThreadId) -> (); @@ -91,6 +95,41 @@ impl ThreadStore for GatedChildMetadataStore { fn delete_thread(params: DeleteThreadParams) -> (); } + fn resume_thread( + &self, + params: ResumeThreadParams, + ) -> ThreadStoreFuture<'_, Arc>> { + Box::pin(async move { + assert!( + params.history_revision.is_some(), + "core must forward the revision" + ); + let supplied = params.history.as_ref().expect("preloaded history"); + let unchanged = self.inner.resume_thread(params.clone()).await?; + assert!(Arc::ptr_eq(supplied, &unchanged)); + if params.thread_id != self.edited_child { + return Ok(unchanged); + } + + // Commit an edit after core preloads history, before its writer acquisition finishes. + let edit: ResponseItem = serde_json::from_value(json!({ + "type": "message", "role": "user", + "content": [{"type": "input_text", "text": LATE_EDIT}], + })) + .expect("late user message"); + self.inner + .append_items(AppendThreadItemsParams { + thread_id: params.thread_id, + items: vec![RolloutItem::ResponseItem(edit.into())], + }) + .await?; + self.inner.shutdown_thread(params.thread_id).await?; + let refreshed = self.inner.resume_thread(params).await?; + assert!(!Arc::ptr_eq(&unchanged, &refreshed)); + Ok(refreshed) + }) + } + fn persist_thread( &self, thread_id: ThreadId, @@ -214,6 +253,7 @@ async fn cold_root_resume_overlaps_child_reads_and_applies_identities_in_graph_o (last, last_gate), ])), failed_child: failed, + edited_child: first, started: started_tx, completed: completed_tx, }); @@ -265,5 +305,27 @@ async fn cold_root_resume_overlaps_child_reads_and_applies_identities_in_graph_o assert!(resumed.thread_manager.get_thread(first).await.is_ok()); assert!(resumed.thread_manager.get_thread(last).await.is_err()); assert!(resumed.thread_manager.get_thread(failed).await.is_err()); + + mount_root_collaboration_call( + &server, + "continue restored child", + "resume-child", + "followup_task", + &json!({ "target": first.to_string(), "message": "continue after resume" }).to_string(), + ) + .await; + let followup = mount_sse_once_match( + &server, + |request: &wiremock::Request| request_has_model(request, ROLE_MODEL), + sse(vec![ev_completed("restored-child-followup")]), + ) + .await; + resumed.submit_turn("continue restored child").await?; + let child = resumed.thread_manager.get_thread(first).await?; + wait_for_event(child.as_ref(), |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + assert!(followup.single_request().body_contains_text(LATE_EDIT)); Ok(()) } diff --git a/codex-rs/core/tests/suite/network_approval.rs b/codex-rs/core/tests/suite/network_approval.rs index 4546185ed409..5dc35601a1fe 100644 --- a/codex-rs/core/tests/suite/network_approval.rs +++ b/codex-rs/core/tests/suite/network_approval.rs @@ -299,54 +299,81 @@ async fn strict_auto_review_routes_network_approval_to_guardian_when_user_review }), ..Default::default() }; - let responses = mount_sse_sequence( - &server, - vec![ - sse(vec![ - ev_response_created("resp-strict-network-permissions"), - ev_function_call( + // Guardian and parent requests can arrive in either order after the command yields. + // Only Guardian requests may consume these decisions. + let mut guardian_responses = Vec::new(); + for response_id in ["strict-command-guardian", "strict-network-guardian"] { + guardian_responses.push( + mount_sse_once_match( + &server, + is_guardian_request, + sse(vec![ + ev_response_created(response_id), + ev_assistant_message( + response_id, + r#"{"risk_level":"low","user_authorization":"high","outcome":"allow","rationale":"The strict-review request is safe."}"#, + ), + ev_completed(response_id), + ]), + ) + .await, + ); + } + let _parent = wiremock::Mock::given(wiremock::matchers::method("POST")) + .and(wiremock::matchers::path("/v1/responses")) + .and(|request: &wiremock::Request| !is_guardian_request(request)) + .respond_with(move |request: &wiremock::Request| { + let body: Value = serde_json::from_slice( + &decoded_request_body(request).expect("decode parent request"), + ) + .expect("parse parent request"); + let input = body["input"].as_array().expect("parent input"); + let output = input + .iter() + .rev() + .find(|item| item["type"] == "function_call_output"); + let response_id = format!("strict-parent-{}", input.len()); + let event = match output { + None => ev_function_call( permission_call_id, "request_permissions", - &serde_json::to_string(&json!({ + &json!({ "reason": "Require automatic review for the rest of this turn", "permissions": requested_permissions, - }))?, + }) + .to_string(), ), - ev_completed("resp-strict-network-permissions"), - ]), - sse(vec![ - ev_response_created("resp-strict-network-command"), - ev_function_call( + Some(output) if output["call_id"] == permission_call_id => ev_function_call( network_call_id, "exec_command", - &serde_json::to_string(&network_fetch_args(LOCAL_ENVIRONMENT_ID))?, - ), - ev_completed("resp-strict-network-command"), - ]), - sse(vec![ - ev_response_created("resp-strict-command-guardian"), - ev_assistant_message( - "msg-strict-command-guardian", - r#"{"risk_level":"low","user_authorization":"high","outcome":"allow","rationale":"The strict-review command is safe."}"#, - ), - ev_completed("resp-strict-command-guardian"), - ]), - sse(vec![ - ev_response_created("resp-strict-network-guardian"), - ev_assistant_message( - "msg-strict-network-guardian", - r#"{"risk_level":"low","user_authorization":"high","outcome":"allow","rationale":"The strict-review network request is safe."}"#, + &network_fetch_args(LOCAL_ENVIRONMENT_ID).to_string(), ), - ev_completed("resp-strict-network-guardian"), - ]), - sse(vec![ - ev_response_created("resp-strict-network-complete"), - ev_assistant_message("msg-strict-network-complete", "reviewed"), - ev_completed("resp-strict-network-complete"), - ]), - ], - ) - .await; + Some(output) => { + let output = output["output"].as_str().expect("process output"); + if let Some(session_id) = output + .lines() + .find_map(|line| line.strip_prefix("Process running with session ID ")) + { + ev_function_call( + &response_id, + "write_stdin", + &json!({ + "session_id": session_id.parse::().expect("session id"), + "chars": "", + "yield_time_ms": 1_000, + }) + .to_string(), + ) + } else { + assert!(output.contains("Process exited with code"), "{output}"); + ev_assistant_message("strict-parent-complete", "reviewed") + } + } + }; + sse_response(sse(vec![event, ev_completed(&response_id)])) + }) + .mount_as_scoped(&server) + .await; submit_managed_network_turn( &test, @@ -376,7 +403,24 @@ async fn strict_auto_review_routes_network_approval_to_guardian_when_user_review .await?; wait_for_completion_without_network_prompt(&test).await; - let actions = guardian_network_actions(&responses)?; + assert_eq!( + guardian_responses + .iter() + .flat_map(ResponseMock::requests) + .filter(|request| { + request.body_json()["client_metadata"]["x-openai-subagent"] == "guardian" + }) + .count(), + 2, + "the command and network request must each receive Guardian review", + ); + let actions = guardian_responses + .iter() + .map(guardian_network_actions) + .collect::>>()? + .into_iter() + .flatten() + .collect::>(); assert_eq!(actions.len(), 1); assert_eq!( actions[0] diff --git a/codex-rs/core/tests/suite/pending_input_persistence.rs b/codex-rs/core/tests/suite/pending_input_persistence.rs index e94db8194660..e84680b572aa 100644 --- a/codex-rs/core/tests/suite/pending_input_persistence.rs +++ b/codex-rs/core/tests/suite/pending_input_persistence.rs @@ -104,7 +104,7 @@ impl ThreadStore for GatedCheckpointStore { delegate_store_methods! { fn create_thread(params: CreateThreadParams) -> (); - fn resume_thread(params: ResumeThreadParams) -> (); + fn resume_thread(params: ResumeThreadParams) -> Arc>; fn append_items(params: AppendThreadItemsParams) -> (); fn discard_thread(thread_id: ThreadId) -> (); fn load_history(params: LoadThreadHistoryParams) -> StoredThreadHistory; @@ -509,6 +509,7 @@ async fn local_preparation_is_durable_before_first_input_and_survives_restart( .resume_thread_with_history( initial.config.clone(), InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: context.thread_id, history: Arc::new(context.items), rollout_path: Some(rollout_path), diff --git a/codex-rs/core/tests/suite/remote_env.rs b/codex-rs/core/tests/suite/remote_env.rs index 4b8c9192402b..622a031e5222 100644 --- a/codex-rs/core/tests/suite/remote_env.rs +++ b/codex-rs/core/tests/suite/remote_env.rs @@ -1446,6 +1446,19 @@ async fn serve_environment_with_agents_md( listener: TcpListener, contents: &str, attach: tokio::sync::oneshot::Receiver<()>, + shutdown: tokio::sync::oneshot::Receiver<()>, +) -> usize { + serve_environment_with_instruction_files( + listener, contents, /*skill*/ None, attach, shutdown, + ) + .await +} + +async fn serve_environment_with_instruction_files( + listener: TcpListener, + contents: &str, + skill: Option<&str>, + attach: tokio::sync::oneshot::Receiver<()>, mut shutdown: tokio::sync::oneshot::Receiver<()>, ) -> usize { let mut websocket = accept_initialized_exec_server(listener).await; @@ -1461,6 +1474,10 @@ async fn serve_environment_with_agents_md( let is_agents_md = request["params"]["path"] .as_str() .is_some_and(|path| path.ends_with("/AGENTS.md")); + let is_skill_root = skill.is_some() + && request["params"]["path"] + .as_str() + .is_some_and(|path| path.ends_with("/.agents/skills")); let response = match request["method"].as_str() { Some("environment/info") => json!({ "id": request["id"], @@ -1470,16 +1487,38 @@ async fn serve_environment_with_agents_md( "id": request["id"], "result": { "path": request["params"]["path"] } }), - Some("fs/walk") => json!({ - "id": request["id"], - "result": { "entries": [], "errors": [], "truncated": false } - }), - Some("fs/getMetadata") if is_agents_md => { + Some("fs/walk") => { + let root = request["params"]["path"].as_str().expect("walk root"); + let entries = if is_skill_root { + vec![json!({ + "path": format!("{root}/guardian-fixture-skill/SKILL.md"), + "kind": "file", + })] + } else { + Vec::new() + }; + json!({ + "id": request["id"], + "result": { "entries": entries, "errors": [], "truncated": false } + }) + } + Some("fs/readFile") + if skill.is_some() + && request["params"]["path"] + .as_str() + .is_some_and(|path| path.ends_with("/guardian-fixture-skill/SKILL.md")) => + { + json!({ + "id": request["id"], + "result": { "dataBase64": BASE64_STANDARD.encode(skill.expect("skill contents")) } + }) + } + Some("fs/getMetadata") if is_agents_md || is_skill_root => { json!({ "id": request["id"], "result": { - "isDirectory": false, - "isFile": true, + "isDirectory": is_skill_root, + "isFile": is_agents_md, "isSymlink": false, "size": contents.len(), "createdAtMs": 0, diff --git a/codex-rs/core/tests/suite/resume.rs b/codex-rs/core/tests/suite/resume.rs index 95d763644d13..a3bdb7257ae5 100644 --- a/codex-rs/core/tests/suite/resume.rs +++ b/codex-rs/core/tests/suite/resume.rs @@ -61,6 +61,7 @@ async fn local_thread_history_mode_survives_restart( .resume_thread_with_history( initial.config.clone(), InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: history.thread_id, history: Arc::new(history.items), rollout_path: initial.session_configured.rollout_path.clone(), diff --git a/codex-rs/core/tests/suite/resume_warning.rs b/codex-rs/core/tests/suite/resume_warning.rs index 3e4f6c199e23..fd53c8cfe22a 100644 --- a/codex-rs/core/tests/suite/resume_warning.rs +++ b/codex-rs/core/tests/suite/resume_warning.rs @@ -58,6 +58,7 @@ fn resume_history( }; InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: ThreadId::default(), history: Arc::new(vec![ RolloutItem::EventMsg(EventMsg::TurnStarted(TurnStartedEvent { diff --git a/codex-rs/core/tests/suite/rollout_compression.rs b/codex-rs/core/tests/suite/rollout_compression.rs index 1ab5986ae267..03f01be0b440 100644 --- a/codex-rs/core/tests/suite/rollout_compression.rs +++ b/codex-rs/core/tests/suite/rollout_compression.rs @@ -169,6 +169,7 @@ async fn compressed_shared_fork_resume_preserves_checkpoint_and_frozen_history() .resume_thread_with_history( config, InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: context.thread_id, history: Arc::new(context.items), rollout_path: Some(child_path), diff --git a/codex-rs/core/tests/suite/scenarios.rs b/codex-rs/core/tests/suite/scenarios.rs index 9f2e33dbfbb0..6c143ee22694 100644 --- a/codex-rs/core/tests/suite/scenarios.rs +++ b/codex-rs/core/tests/suite/scenarios.rs @@ -44,6 +44,8 @@ use codex_protocol::openai_models::ToolMessage; use codex_protocol::openai_models::ToolMode; use codex_protocol::protocol::EnvironmentConfigState; use codex_protocol::protocol::EventMsg; +#[cfg(not(target_os = "windows"))] +use codex_protocol::protocol::MultiAgentVersion; use codex_protocol::protocol::Op; use codex_protocol::protocol::TurnSettingsUpdate; use codex_protocol::protocol::TurnSettingsUpdateOutcome; @@ -1671,3 +1673,19 @@ async fn app_tool_exposure_request_history() -> Result<()> { ); Ok(()) } + +#[cfg(not(target_os = "windows"))] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn model_catalog_refresh_preserves_tools_and_history() -> Result<()> { + let requests = super::spawn_agent_description::model_catalog_refresh_requests( + MultiAgentVersion::V1, + /*model_catalog_in_context*/ true, + ) + .await?; + insta::assert_snapshot!(context_snapshot::format_request_history_snapshot( + "Catalog refreshes append the updated listing once while preserving tools and prior input.", + &requests, + &ContextSnapshotOptions::default().include_request_settings(), + )); + Ok(()) +} diff --git a/codex-rs/core/tests/suite/skills_extension.rs b/codex-rs/core/tests/suite/skills_extension.rs index 5f116a99d15a..72462e1e62e0 100644 --- a/codex-rs/core/tests/suite/skills_extension.rs +++ b/codex-rs/core/tests/suite/skills_extension.rs @@ -3089,6 +3089,7 @@ async fn production_turn_keeps_rebalanced_catalogs_stable_after_compaction_and_r .thread_manager .start_thread(StartThreadOptions { initial_history: InitialHistory::Resumed(ResumedHistory { + history_revision: history.revision, conversation_id: history.thread_id, history: Arc::new(history.items), rollout_path: cloud_thread.session_configured.rollout_path.clone(), diff --git a/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__model_catalog_refresh_preserves_tools_and_history.snap b/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__model_catalog_refresh_preserves_tools_and_history.snap new file mode 100644 index 000000000000..682d909fb2e1 --- /dev/null +++ b/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__model_catalog_refresh_preserves_tools_and_history.snap @@ -0,0 +1,50 @@ +--- +source: core/tests/suite/scenarios.rs +expression: "context_snapshot::format_request_history_snapshot(\"Catalog refreshes append the updated listing once while preserving tools and prior input.\",\n&requests, &ContextSnapshotOptions::default().include_request_settings(),)" +--- +Scenario: Catalog refreshes append the updated listing once while preserving tools and prior input. + +## Window 1 +Settings: + include: ["reasoning.encrypted_content"] + instructions: Test model instructions. + model: "visible-model" + parallel_tool_calls: true + prompt_cache_key: "" + reasoning: {"effort":"medium","summary":"auto"} + store: false + stream: true + tool_choice: "auto" + tools (6; hash=1ea0fa9abde186d4): + - function/exec_command: Runs a command in a PTY, returning output or a session ID for ongoing interaction.; args=[cmd, justification, login, max_output_tokens, prefix_rule, sandbox_permissions, shell, tty, workdir, yield_time_ms]; hash=c04108e51a8548c5 + - function/write_stdin: Writes characters to an existing unified exec session and returns recent output.; args=[chars, max_output_tokens, session_id, yield_time_ms]; hash=7fb62d8a3e2df4e2 + - function/request_user_input: Request user input for one to three short questions and wait for the response. This tool is only ...; args=[questions]; hash=64a729b25c424fc6 + - function/view_image: View a local image file from the filesystem when visual inspection is needed. Use this for images...; args=[path]; hash=1908de9c0be7118a + - namespace/multi_agent_v1: Tools for spawning and managing sub-agents.; hash=94c598b3c6ca141b + - function/close_agent + - function/resume_agent + - function/send_input + - function/spawn_agent + - function/wait_agent + - web_search; hash=8586237e354ee5ed +-- request 1 (turn) -- +00:message/developer[2]: + [01] + Filesystem sandboxing defines which files can be read or written. `sandbox_mode` is `danger-full-access`...ll commands are permitted. Network access is enabled. [hash=c134061cebb94a1b] + Approval policy is currently never. Do not provide the `sandbox_permissions` for any reason, commands will be rejected. + + [02] + - `visible-model`: Fast and capable Reasoning efforts: low, medium (default), high. Service tiers: priority. + +01:message/user: + hello +-- request 2 (turn) -- +02:message/developer: + + - `visible-model`: Updated picker copy Reasoning efforts: low, medium (default), high. Service tiers: priority. + +03:message/user: + after catalog refresh +-- request 3 (turn) -- +04:message/user: + without another catalog change diff --git a/codex-rs/core/tests/suite/spawn_agent_description.rs b/codex-rs/core/tests/suite/spawn_agent_description.rs index bc0eaf75806e..68bef0e2eae3 100644 --- a/codex-rs/core/tests/suite/spawn_agent_description.rs +++ b/codex-rs/core/tests/suite/spawn_agent_description.rs @@ -20,6 +20,7 @@ use codex_protocol::openai_models::ReasoningEffortPreset; use codex_protocol::openai_models::TruncationPolicyConfig; use codex_protocol::openai_models::default_input_modalities; use codex_protocol::protocol::MULTI_AGENT_MODE_OPEN_TAG; +use codex_protocol::protocol::MultiAgentVersion; use core_test_support::responses::ResponsesRequest; use core_test_support::responses::ev_completed; use core_test_support::responses::ev_response_created; @@ -30,6 +31,7 @@ use core_test_support::responses::namespace_child_tool; use core_test_support::responses::sse; use core_test_support::responses::start_mock_server; use core_test_support::test_codex::test_codex; +use pretty_assertions::assert_eq; use serde_json::Value; use std::time::Duration; use std::time::Instant; @@ -40,8 +42,8 @@ const MULTI_AGENT_V1_NAMESPACE: &str = "multi_agent_v1"; const MULTI_AGENT_V2_NAMESPACE: &str = "collaboration"; const SPAWN_AGENT_TOOL_NAME: &str = "spawn_agent"; -fn spawn_agent_description(body: &Value) -> Option { - namespace_child_tool(body, MULTI_AGENT_V1_NAMESPACE, SPAWN_AGENT_TOOL_NAME) +fn spawn_agent_description(body: &Value, namespace: &str) -> Option { + namespace_child_tool(body, namespace, SPAWN_AGENT_TOOL_NAME) .and_then(|tool| tool.get("description")) .and_then(Value::as_str) .map(str::to_string) @@ -148,8 +150,23 @@ async fn wait_for_model_available(manager: &SharedModelsManager, slug: &str) { } } +#[test_case(MultiAgentVersion::V1, false; "v1_default")] +#[test_case(MultiAgentVersion::V2, false; "v2_default")] +#[test_case(MultiAgentVersion::V2, true; "v2_context")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn spawn_agent_description_lists_visible_models_and_reasoning_efforts() -> Result<()> { +async fn model_catalog_flag_controls_refresh( + multi_agent_version: MultiAgentVersion, + model_catalog_in_context: bool, +) -> Result<()> { + // The V1 opt-in case also snapshots the full request history in scenarios.rs. + model_catalog_refresh_requests(multi_agent_version, model_catalog_in_context).await?; + Ok(()) +} + +pub(super) async fn model_catalog_refresh_requests( + multi_agent_version: MultiAgentVersion, + model_catalog_in_context: bool, +) -> Result> { let server = start_mock_server().await; mount_models_once( &server, @@ -206,30 +223,64 @@ async fn spawn_agent_description_lists_visible_models_and_reasoning_efforts() -> let mut builder = test_codex() .with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing()) .with_model("visible-model") - .with_config(|config| { + .with_config(move |config| { config .features .enable(Feature::Collab) .expect("test config should allow feature update"); + config + .features + .set_enabled( + Feature::MultiAgentV2, + multi_agent_version == MultiAgentVersion::V2, + ) + .expect("set multi-agent version"); + if model_catalog_in_context { + config + .features + .enable(Feature::ModelCatalogInContext) + .expect("enable context catalogs"); + } config.multi_agent_v2.hide_spawn_agent_metadata = false; + config.base_instructions = Some("Test model instructions.".to_string()); + config.include_environment_context = false; }); - let test = builder.build(&server).await?; + let test = builder.build_with_auto_env(&server).await?; wait_for_model_available(&test.thread_manager.get_models_manager(), "visible-model").await; test.submit_turn("hello").await?; - let body = resp_mock.single_request().body_json(); - let description = - spawn_agent_description(&body).expect("spawn_agent description should be present"); + let request = resp_mock.single_request(); + let body = request.body_json(); + let namespace = match multi_agent_version { + MultiAgentVersion::V1 => MULTI_AGENT_V1_NAMESPACE, + MultiAgentVersion::V2 => MULTI_AGENT_V2_NAMESPACE, + MultiAgentVersion::Disabled => unreachable!("this scenario requires spawning"), + }; + let description = spawn_agent_description(&body, namespace) + .expect("spawn_agent description should be present"); + let catalog = request.message_input_texts("developer").join("\n"); + let listing = if model_catalog_in_context { + &catalog + } else { + &description + }; assert!( - description.contains("- `visible-model`: Fast and capable"), - "expected visible model summary in spawn_agent description: {description:?}" + listing.contains("- `visible-model`: Fast and capable"), + "expected visible model summary in model catalog: {catalog:?}" ); - assert!( - description - .contains("Available model overrides (optional; inherited parent model is preferred):"), - "expected model choices to be framed as overrides in spawn_agent description: {description:?}" + assert_eq!( + description.contains("Pick model overrides from the latest listing."), + model_catalog_in_context + ); + assert_eq!( + catalog.contains(""), + model_catalog_in_context + ); + assert_eq!( + description.contains("- `visible-model`: Fast and capable"), + !model_catalog_in_context ); assert!( description.contains( @@ -237,48 +288,106 @@ async fn spawn_agent_description_lists_visible_models_and_reasoning_efforts() -> ), "expected inherited-model guidance in spawn_agent description: {description:?}" ); - assert!( + if multi_agent_version == MultiAgentVersion::V1 { + assert!( description.contains( "Do not set the `model` field unless the user explicitly asks for a different model." ), "expected model override usage guidance in spawn_agent description: {description:?}" ); + } assert!( - description.contains("Reasoning efforts: low, medium (default), high."), - "expected default reasoning effort in spawn_agent description: {description:?}" + listing.contains("Reasoning efforts: low, medium (default), high."), + "expected default reasoning effort in model catalog: {catalog:?}" ); assert!( - description.contains("Service tiers: priority."), - "expected service tier guidance in spawn_agent description: {description:?}" + listing.contains("Service tiers: priority."), + "expected service tier guidance in model catalog: {catalog:?}" ); assert!( - !description.contains("hidden-model"), - "hidden picker model should be omitted from spawn_agent description: {description:?}" + !listing.contains("hidden-model"), + "hidden picker model should be omitted from model catalog: {catalog:?}" ); - assert!( + if multi_agent_version == MultiAgentVersion::V1 { + assert!( description.contains( "Do not spawn sub-agents unless the user or applicable AGENTS.md/skill instructions explicitly ask for sub-agents, delegation, or parallel agent work." ), "expected explicit authorization rule in spawn_agent description: {description:?}" ); - assert!( + assert!( description.contains( "Requests for depth, thoroughness, research, investigation, or detailed codebase analysis do not count as permission to spawn." ) && description.contains("### When to delegate vs. do the subtask yourself"), "expected delegation decision guidance in spawn_agent description: {description:?}" ); - assert!( - description.contains( - "Agent-role guidance below only helps choose which agent to use after spawning is already authorized; it never authorizes spawning by itself." - ), - "expected agent-role clarification in spawn_agent description: {description:?}" - ); - assert!( - !description.contains("A mini model can solve many tasks faster than the main model."), - "spawn_agent description should not encourage choosing a smaller model by default: {description:?}" - ); + assert!( + !description.contains("A mini model can solve many tasks faster than the main model."), + "spawn_agent description should not encourage choosing a smaller model by default: {description:?}" + ); + } - Ok(()) + // Only the opt-in path keeps the tool schema stable across a catalog refresh. + let manager = test.thread_manager.get_models_manager(); + let mut refreshed = manager + .raw_model_catalog(RefreshStrategy::Offline, test.config.http_client_factory()) + .await; + refreshed + .models + .iter_mut() + .find(|model| model.slug == "visible-model") + .expect("visible model") + .description = Some("Updated picker copy".to_string()); + mount_models_once(&server, refreshed.clone()).await; + assert_eq!( + manager + .raw_model_catalog(RefreshStrategy::Online, test.config.http_client_factory()) + .await + .models, + refreshed.models + ); + let expected_catalogs = request + .message_input_texts("developer") + .into_iter() + .filter(|text| text.starts_with("")) + .flat_map(|catalog| { + [ + catalog.clone(), + catalog.replace("Fast and capable", "Updated picker copy"), + ] + }) + .collect::>(); + let mut requests = vec![request]; + for turn in ["after catalog refresh", "without another catalog change"] { + let response = mount_sse_once(&server, sse(vec![ev_completed(turn)])).await; + test.submit_turn(turn).await?; + let current = response.single_request(); + if model_catalog_in_context { + assert_eq!(current.body_json()["tools"], body["tools"]); + } else { + let expected_tools = serde_json::to_string(&body["tools"])? + .replace("Fast and capable", "Updated picker copy"); + assert_eq!( + current.body_json()["tools"], + serde_json::from_str::(&expected_tools)? + ); + } + assert!( + current + .input() + .starts_with(&requests.last().unwrap().input()) + ); + assert_eq!( + current + .message_input_texts("developer") + .into_iter() + .filter(|text| text.starts_with("")) + .collect::>(), + expected_catalogs, + ); + requests.push(current); + } + Ok(requests) } #[test_case(false, false, MULTI_AGENT_V1_NAMESPACE; "v1 hides agent type without roles")] diff --git a/codex-rs/ext/goal/src/api.rs b/codex-rs/ext/goal/src/api.rs index b414e9ccfb04..a2bd783dc8b0 100644 --- a/codex-rs/ext/goal/src/api.rs +++ b/codex-rs/ext/goal/src/api.rs @@ -141,10 +141,14 @@ impl GoalService { .map_err(|err| GoalServiceError::Internal(format!("failed to read thread goal: {err}"))) } + /// Records the user's instruction after validation and before changing goal state. + /// The recording future runs under the runtime's goal mutation permit; failure leaves + /// the goal unchanged. Recorded user intent remains valid if the database write fails. pub async fn set_thread_goal( &self, state_db: &codex_state::StateRuntime, request: GoalSetRequest<'_>, + record_user_instruction: impl Future> + Send, ) -> Result { let GoalSetRequest { thread_id, @@ -191,14 +195,21 @@ impl GoalService { tracing::warn!("failed to prepare external goal mutation: {err}"); } + let existing_goal = state_db + .thread_goals() + .get_thread_goal(thread_id) + .await + .map_err(|err| { + GoalServiceError::Internal(format!("failed to read thread goal: {err}")) + })?; + if objective.is_none() && existing_goal.is_none() { + return Err(GoalServiceError::InvalidRequest(format!( + "cannot update goal for thread {thread_id}: no goal exists" + ))); + } + record_user_instruction.await?; + let (goal, previous_goal) = if let Some(objective) = objective { - let existing_goal = state_db - .thread_goals() - .get_thread_goal(thread_id) - .await - .map_err(|err| { - GoalServiceError::Internal(format!("failed to read thread goal: {err}")) - })?; if let Some(existing_goal) = existing_goal.as_ref() { let previous_goal = PreviousGoalSnapshot::from(existing_goal); state_db @@ -238,18 +249,11 @@ impl GoalService { .map(|goal| (goal, None))? } } else { - let existing_goal = state_db - .thread_goals() - .get_thread_goal(thread_id) - .await - .map_err(|err| { - GoalServiceError::Internal(format!("failed to read thread goal: {err}")) - })? - .ok_or_else(|| { - GoalServiceError::InvalidRequest(format!( - "cannot update goal for thread {thread_id}: no goal exists" - )) - })?; + let existing_goal = existing_goal.ok_or_else(|| { + GoalServiceError::InvalidRequest(format!( + "cannot update goal for thread {thread_id}: no goal exists" + )) + })?; let previous_goal = PreviousGoalSnapshot::from(&existing_goal); let expected_goal_id = existing_goal.goal_id.clone(); state_db @@ -289,10 +293,12 @@ impl GoalService { }) } + /// Records a clear instruction even when no goal exists, then deletes goal state under the mutation permit. pub async fn clear_thread_goal( &self, state_db: &codex_state::StateRuntime, thread_id: ThreadId, + record_user_instruction: impl Future> + Send, ) -> Result { let runtime = self.runtime_for_thread(thread_id); // Hold this through the prepare/write window so idle continuation cannot @@ -312,6 +318,9 @@ impl GoalService { tracing::warn!("failed to prepare external goal mutation: {err}"); } + // A failed set can leave authorization in history without a goal row. + record_user_instruction.await?; + let cleared_goal = state_db .thread_goals() .delete_thread_goal(thread_id) diff --git a/codex-rs/ext/goal/tests/goal_extension_backend.rs b/codex-rs/ext/goal/tests/goal_extension_backend.rs index e0a952d1a8b0..c9a855b5552c 100644 --- a/codex-rs/ext/goal/tests/goal_extension_backend.rs +++ b/codex-rs/ext/goal/tests/goal_extension_backend.rs @@ -31,6 +31,7 @@ use codex_goal_extension::GoalExtensionConfig; use codex_goal_extension::GoalObjectiveUpdate; use codex_goal_extension::GoalRuntimeHandle; use codex_goal_extension::GoalService; +use codex_goal_extension::GoalServiceError; use codex_goal_extension::GoalSetRequest; use codex_goal_extension::GoalTokenBudgetUpdate; use codex_goal_extension::install_with_backend; @@ -1342,6 +1343,7 @@ async fn goal_service_external_set_active_preserves_concurrent_usage() -> anyhow token_budget: GoalTokenBudgetUpdate::Keep, max_goal_token_budget: None, }, + std::future::ready(Ok(())), ) .await?; harness @@ -1416,7 +1418,7 @@ async fn thread_stop_unregisters_goal_runtime_from_service() -> anyhow::Result<( assert!( harness .goal_service - .clear_thread_goal(runtime.as_ref(), thread_id) + .clear_thread_goal(runtime.as_ref(), thread_id, std::future::ready(Ok(()))) .await? ); assert_eq!(Vec::::new(), harness.sink.goal_events()); @@ -1483,17 +1485,28 @@ async fn goal_service_sets_gets_and_clears_thread_goal() -> anyhow::Result<()> { seed_thread_metadata(runtime.as_ref(), thread_id).await?; let api = GoalService::new(); - let set = api + let request = GoalSetRequest { + thread_id, + objective: GoalObjectiveUpdate::Set(" ship goal API ownership "), + status: None, + token_budget: GoalTokenBudgetUpdate::Set(Some(123)), + max_goal_token_budget: None, + }; + let failure = GoalServiceError::Internal("instruction recording failed".to_owned()); + let result = api .set_thread_goal( runtime.as_ref(), - GoalSetRequest { - thread_id, - objective: GoalObjectiveUpdate::Set(" ship goal API ownership "), - status: None, - token_budget: GoalTokenBudgetUpdate::Set(Some(123)), - max_goal_token_budget: None, - }, + request, + std::future::ready(Err(failure.clone())), ) + .await; + assert_eq!(result.unwrap_err(), failure); + assert_eq!( + api.get_thread_goal(runtime.as_ref(), thread_id).await?, + None + ); + let set = api + .set_thread_goal(runtime.as_ref(), request, std::future::ready(Ok(()))) .await?; let get = api .get_thread_goal(runtime.as_ref(), thread_id) @@ -1510,12 +1523,32 @@ async fn goal_service_sets_gets_and_clears_thread_goal() -> anyhow::Result<()> { assert_eq!(Some(123), get.token_budget); assert_eq!(Some("ship goal API ownership"), metadata.preview.as_deref()); - assert!(api.clear_thread_goal(runtime.as_ref(), thread_id).await?); + assert_eq!( + api.clear_thread_goal( + runtime.as_ref(), + thread_id, + std::future::ready(Err(failure.clone())) + ) + .await, + Err(failure), + ); + assert_eq!( + api.get_thread_goal(runtime.as_ref(), thread_id).await?, + Some(get) + ); + + assert!( + api.clear_thread_goal(runtime.as_ref(), thread_id, std::future::ready(Ok(()))) + .await? + ); assert_eq!( None, api.get_thread_goal(runtime.as_ref(), thread_id).await? ); - assert!(!api.clear_thread_goal(runtime.as_ref(), thread_id).await?); + assert!( + !api.clear_thread_goal(runtime.as_ref(), thread_id, std::future::ready(Ok(()))) + .await? + ); Ok(()) } @@ -1537,6 +1570,7 @@ async fn goal_service_enforces_maximum_token_budget_on_creation_and_updates() -> token_budget: GoalTokenBudgetUpdate::Keep, max_goal_token_budget: Some(100), }, + std::future::ready(Ok(())), ) .await?; assert_eq!(goal.goal.token_budget, Some(100)); @@ -1551,6 +1585,7 @@ async fn goal_service_enforces_maximum_token_budget_on_creation_and_updates() -> token_budget: GoalTokenBudgetUpdate::Set(Some(101)), max_goal_token_budget: Some(100), }, + std::future::ready(Ok(())), ) .await .expect_err("goal budget above the configured maximum should fail"); @@ -1577,6 +1612,7 @@ async fn goal_service_enforces_maximum_token_budget_on_creation_and_updates() -> token_budget: GoalTokenBudgetUpdate::Set(Some(99)), max_goal_token_budget: Some(100), }, + std::future::ready(Ok(())), ) .await?; assert_eq!(goal.goal.token_budget, Some(99)); @@ -1591,6 +1627,7 @@ async fn goal_service_enforces_maximum_token_budget_on_creation_and_updates() -> token_budget: GoalTokenBudgetUpdate::Set(None), max_goal_token_budget: Some(100), }, + std::future::ready(Ok(())), ) .await?; assert_eq!(goal.goal.token_budget, Some(100)); diff --git a/codex-rs/external-agent-migration/src/sessions/append.rs b/codex-rs/external-agent-migration/src/sessions/append.rs index 70b4b30e94da..b77481afd3b2 100644 --- a/codex-rs/external-agent-migration/src/sessions/append.rs +++ b/codex-rs/external-agent-migration/src/sessions/append.rs @@ -71,6 +71,7 @@ pub async fn append_existing_session( } if thread_store .resume_thread(ResumeThreadParams { + history_revision: None, thread_id, rollout_path: Some(rollout_path), history: None, diff --git a/codex-rs/features/src/lib.rs b/codex-rs/features/src/lib.rs index 7f61ef3aee9a..a42a4b5db2e6 100644 --- a/codex-rs/features/src/lib.rs +++ b/codex-rs/features/src/lib.rs @@ -211,6 +211,8 @@ pub enum Feature { Collab, /// Enable task-path-based multi-agent routing. MultiAgentV2, + /// Keep spawn model choices in append-only context instead of tool descriptions. + ModelCatalogInContext, /// Keep sampling through reasoning and commentary boundaries when agent mail arrives. /// Pending mail is delivered at the next normal input boundary instead. DeferMailboxPreemption, @@ -1360,6 +1362,12 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::Stable, default_enabled: false, }, + FeatureSpec { + id: Feature::ModelCatalogInContext, + key: "model_catalog_in_context", + stage: Stage::UnderDevelopment, + default_enabled: false, + }, FeatureSpec { id: Feature::DeferMailboxPreemption, key: "defer_mailbox_preemption", diff --git a/codex-rs/history/src/lib.rs b/codex-rs/history/src/lib.rs index 090ca9178216..b117bfa558fd 100644 --- a/codex-rs/history/src/lib.rs +++ b/codex-rs/history/src/lib.rs @@ -359,6 +359,9 @@ pub struct RolloutLine { pub struct ResumedHistory { pub conversation_id: ThreadId, pub history: Arc>, + /// Store-issued revision for this exact snapshot; clear it when modifying the history. + #[serde(skip)] + pub history_revision: Option, pub rollout_path: Option, } diff --git a/codex-rs/history/src/tests.rs b/codex-rs/history/src/tests.rs index e0e46cfe67aa..0b04ff6fd1e3 100644 --- a/codex-rs/history/src/tests.rs +++ b/codex-rs/history/src/tests.rs @@ -959,6 +959,7 @@ fn copied_history_uses_persisted_history_mode() -> Result<()> { git: None, }); let history = InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: thread_id, history: Arc::new(vec![session_meta.clone()]), rollout_path: None, @@ -978,6 +979,7 @@ fn copied_history_uses_persisted_history_mode() -> Result<()> { ); assert_eq!( InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id: thread_id, history: Arc::new(Vec::new()), rollout_path: None, diff --git a/codex-rs/rollout/src/recorder.rs b/codex-rs/rollout/src/recorder.rs index cd05f6149b3d..7422c7f4e761 100644 --- a/codex-rs/rollout/src/recorder.rs +++ b/codex-rs/rollout/src/recorder.rs @@ -1164,6 +1164,7 @@ impl RolloutRecorder { info!("Resumed rollout successfully from {path:?}"); Ok(InitialHistory::Resumed(ResumedHistory { + history_revision: None, conversation_id, history: Arc::new(items), rollout_path: Some(compression::plain_rollout_path(path)), diff --git a/codex-rs/thread-store/src/in_memory.rs b/codex-rs/thread-store/src/in_memory.rs index b7a38992602f..1e670fee7730 100644 --- a/codex-rs/thread-store/src/in_memory.rs +++ b/codex-rs/thread-store/src/in_memory.rs @@ -335,6 +335,7 @@ mod tests { .expect("create paginated thread"); store .resume_thread(ResumeThreadParams { + history_revision: None, thread_id, rollout_path: Some(rollout_path.clone()), history: None, @@ -397,6 +398,7 @@ mod tests { ); store .resume_thread(ResumeThreadParams { + history_revision: None, thread_id, rollout_path: None, history: None, @@ -616,20 +618,33 @@ impl InMemoryThreadStore { Ok(()) } - async fn resume_thread(&self, params: ResumeThreadParams) -> ThreadStoreResult<()> { + async fn resume_thread( + &self, + params: ResumeThreadParams, + ) -> ThreadStoreResult>> { let mut state = self.state.lock().await; state.calls.resume_thread += 1; if let Some(history) = params.history { - state - .histories - .insert(params.thread_id, Arc::unwrap_or_clone(history)); + if matches!( + history.first(), + Some(RolloutItem::SessionMeta(meta)) if meta.meta.id == params.thread_id + ) { + state + .histories + .entry(params.thread_id) + .or_insert_with(|| Arc::unwrap_or_clone(history)); + } else { + state + .histories + .insert(params.thread_id, Arc::unwrap_or_clone(history)); + } } else { state.histories.entry(params.thread_id).or_default(); } if let Some(rollout_path) = params.rollout_path { state.rollout_paths.insert(rollout_path, params.thread_id); } - Ok(()) + Ok(Arc::new(state.histories[¶ms.thread_id].clone())) } async fn append_items(&self, params: AppendThreadItemsParams) -> ThreadStoreResult<()> { @@ -667,6 +682,7 @@ impl InMemoryThreadStore { let history_mode = history_mode_from_state(&state, params.thread_id); reject_paginated_history_mode(history_mode)?; Ok(StoredThreadHistory { + revision: None, thread_id: params.thread_id, items: items.clone(), }) @@ -686,6 +702,7 @@ impl InMemoryThreadStore { thread_id: params.thread_id, })?; Ok(StoredModelContext { + revision: None, thread_id: params.thread_id, items: items.clone(), }) @@ -900,7 +917,10 @@ impl ThreadStore for InMemoryThreadStore { Box::pin(InMemoryThreadStore::create_thread(self, params)) } - fn resume_thread(&self, params: ResumeThreadParams) -> ThreadStoreFuture<'_, ()> { + fn resume_thread( + &self, + params: ResumeThreadParams, + ) -> ThreadStoreFuture<'_, Arc>> { Box::pin(InMemoryThreadStore::resume_thread(self, params)) } @@ -1066,6 +1086,7 @@ fn stored_thread_from_state( .ok_or(ThreadStoreError::ThreadNotFound { thread_id })?; let history_items = state.histories.get(&thread_id).cloned().unwrap_or_default(); let history = include_history.then(|| StoredThreadHistory { + revision: None, thread_id, items: history_items.clone(), }); diff --git a/codex-rs/thread-store/src/live_thread.rs b/codex-rs/thread-store/src/live_thread.rs index f6f57e6dd0a2..f9bb507fbc2b 100644 --- a/codex-rs/thread-store/src/live_thread.rs +++ b/codex-rs/thread-store/src/live_thread.rs @@ -179,14 +179,13 @@ impl LiveThread { Ok(live_thread) } + /// Reopens persistence and returns the replay history selected under writer ownership. pub async fn resume( thread_store: Arc, history_mode: ThreadHistoryMode, - params: ResumeThreadParams, - ) -> ThreadStoreResult { + mut params: ResumeThreadParams, + ) -> ThreadStoreResult<(Self, Arc>)> { let thread_id = params.thread_id; - let should_load_history = params.history.is_none(); - let include_archived = params.include_archived; let metadata = if history_mode == ThreadHistoryMode::Paginated && let Some(local_store) = thread_store.as_any().downcast_ref::() && let Some(state_db) = local_store.state_db().await @@ -200,34 +199,19 @@ impl LiveThread { } else { None }; - let mut metadata_sync = ThreadMetadataSync::for_resume(¶ms, metadata.as_ref()); - thread_store.resume_thread(params).await?; - if should_load_history { - match thread_store - .load_history(LoadThreadHistoryParams { - thread_id, - include_archived, - }) - .await - { - Ok(history) => metadata_sync.record_resume_history(&history.items), - Err(err) => { - if let Err(discard_err) = thread_store.discard_thread(thread_id).await { - warn!( - "failed to discard thread persistence after resume history load failed: {discard_err}" - ); - } - return Err(err); - } - } - } - Ok(Self { - thread_id, - history_mode, - thread_store, - metadata_sync: Arc::new(Mutex::new(metadata_sync)), - persistence_telemetry: RolloutPersistenceTelemetry::new(thread_id), - }) + let history = thread_store.resume_thread(params.clone()).await?; + params.history = Some(Arc::clone(&history)); + let metadata_sync = ThreadMetadataSync::for_resume(¶ms, metadata.as_ref()); + Ok(( + Self { + thread_id, + history_mode, + thread_store, + metadata_sync: Arc::new(Mutex::new(metadata_sync)), + persistence_telemetry: RolloutPersistenceTelemetry::new(thread_id), + }, + history, + )) } #[tracing::instrument( diff --git a/codex-rs/thread-store/src/local/compression_writer_tests.rs b/codex-rs/thread-store/src/local/compression_writer_tests.rs index 448b0338a44d..1ea0161bbd0a 100644 --- a/codex-rs/thread-store/src/local/compression_writer_tests.rs +++ b/codex-rs/thread-store/src/local/compression_writer_tests.rs @@ -22,6 +22,7 @@ use uuid::Uuid; use super::LocalThreadStore; use super::test_support::test_config; use super::test_support::write_session_file; +use crate::LoadThreadHistoryParams; use crate::ResumeThreadParams; use crate::ThreadMetadataPatch; use crate::ThreadPersistenceMetadata; @@ -46,6 +47,7 @@ fn age(path: &Path) -> TestResult<()> { fn resume(thread_id: ThreadId, path: &Path, home: &Path) -> ResumeThreadParams { ResumeThreadParams { + history_revision: None, thread_id, rollout_path: Some(path.to_path_buf()), history: None, @@ -186,6 +188,24 @@ async fn metadata_updates_share_ownership_and_resume_compressed_rollouts() -> Te json!(Some(RolloutItem::SessionMeta(expected.clone()))) ); + age(&path)?; + compress(home.path()).await?; + let snapshot = competitor + .load_history(LoadThreadHistoryParams { + thread_id, + include_archived: true, + }) + .await?; + let history = Arc::new(snapshot.items); + let resumed = competitor + .resume_thread(ResumeThreadParams { + history: Some(Arc::clone(&history)), + history_revision: snapshot.revision, + ..resume(thread_id, &path, home.path()) + }) + .await?; + assert!(Arc::ptr_eq(&history, &resumed)); + competitor.shutdown_thread(thread_id).await?; age(&path)?; compress(home.path()).await?; patch.patch.memory_mode = Some(ThreadMemoryMode::Enabled); diff --git a/codex-rs/thread-store/src/local/delete_thread.rs b/codex-rs/thread-store/src/local/delete_thread.rs index 103b12d49fd9..75e55ef84f1b 100644 --- a/codex-rs/thread-store/src/local/delete_thread.rs +++ b/codex-rs/thread-store/src/local/delete_thread.rs @@ -794,6 +794,7 @@ SELECT store .resume_thread(ResumeThreadParams { + history_revision: None, thread_id, rollout_path: Some(rollout_path), history: None, diff --git a/codex-rs/thread-store/src/local/history_revision.rs b/codex-rs/thread-store/src/local/history_revision.rs new file mode 100644 index 000000000000..ad241a49a24f --- /dev/null +++ b/codex-rs/thread-store/src/local/history_revision.rs @@ -0,0 +1,28 @@ +//! Validates snapshots of append-only local rollouts before reusing them for resume. +//! +//! Capture before and after reading; any change or unavailable metadata disables reuse. The +//! selected path prevents reuse for a different rollout. Validate under writer ownership, +//! before opening the recorder can materialize a compressed file. + +use std::path::Path; + +pub(super) async fn read(path: &Path) -> Option { + let path = codex_rollout::existing_rollout_path(path).await?; + let path = tokio::fs::canonicalize(path).await.ok()?; + let metadata = tokio::fs::metadata(&path).await.ok()?; + let modified = metadata.modified().ok()?; + let created = metadata.created().ok(); + #[cfg(unix)] + let identity = { + use std::os::unix::fs::MetadataExt; + Some(( + metadata.dev(), + metadata.ino(), + metadata.ctime(), + metadata.ctime_nsec(), + )) + }; + #[cfg(not(unix))] + let identity: Option<(u64, u64, i64, i64)> = None; + serde_json::to_string(&("local", path, metadata.len(), modified, created, identity)).ok() +} diff --git a/codex-rs/thread-store/src/local/live_writer.rs b/codex-rs/thread-store/src/local/live_writer.rs index 6a2d95f2b068..f78dba45313d 100644 --- a/codex-rs/thread-store/src/local/live_writer.rs +++ b/codex-rs/thread-store/src/local/live_writer.rs @@ -1,4 +1,5 @@ use std::path::PathBuf; +use std::sync::Arc; use codex_protocol::ThreadId; use codex_protocol::protocol::ThreadHistoryMode; @@ -14,7 +15,6 @@ use super::LocalThreadStore; use super::create_thread; use crate::AppendThreadItemsParams; use crate::CreateThreadParams; -use crate::ReadThreadParams; use crate::ResumeThreadParams; use crate::ThreadStoreError; use crate::ThreadStoreResult; @@ -40,52 +40,61 @@ pub(super) async fn create_thread( pub(super) async fn resume_thread( store: &LocalThreadStore, params: ResumeThreadParams, -) -> ThreadStoreResult<()> { +) -> ThreadStoreResult>> { let _live_writer_guard = store.live_writer_locks.lock(params.thread_id).await; store.ensure_live_recorder_absent(params.thread_id).await?; let writer_lock = store.acquire_writer_lock(params.thread_id)?; - let history_mode = if let Some(history) = params.history.as_deref() { - canonical_history_mode_from_rollout_items(history) - } else if let Some(rollout_path) = params.rollout_path.as_ref() { - super::read_thread::read_thread_by_rollout_path( - store, - rollout_path.clone(), - params.include_archived, - /*include_history*/ false, - ) - .await? - .history_mode - } else { - super::read_thread::read_thread( - store, - ReadThreadParams { - thread_id: params.thread_id, - include_archived: params.include_archived, - include_history: false, - }, - ) - .await? - .history_mode - }; - let rollout_path = match (params.rollout_path, params.history) { - (Some(rollout_path), _history) => rollout_path, - (None, history) => { - let thread = super::read_thread::read_thread( - store, - ReadThreadParams { + let rollout_path = match params.rollout_path { + Some(rollout_path) => rollout_path, + None => { + let resolved = if params.include_archived { + super::thread_rollout_resolver::resolve_current_including_archived( + store, + params.thread_id, + ) + .await? + } else { + super::thread_rollout_resolver::resolve_current(store, params.thread_id).await? + }; + resolved + .ok_or(ThreadStoreError::ThreadNotFound { thread_id: params.thread_id, - include_archived: params.include_archived, - include_history: history.is_none(), - }, - ) - .await?; - thread - .rollout_path - .ok_or_else(|| ThreadStoreError::Internal { - message: format!("thread {} does not have a rollout path", params.thread_id), })? + .path } }; + let rollout_path = + super::read_thread::resolve_requested_rollout_path(store, rollout_path).await?; + if !params.include_archived + && super::helpers::rollout_path_is_archived(&store.config.codex_home, &rollout_path) + { + return Err(ThreadStoreError::InvalidRequest { + message: format!("thread {} is archived", params.thread_id), + }); + } + let history = match params.history { + Some(history) + if !matches!( + history.first(), + Some(RolloutItem::SessionMeta(meta)) if meta.meta.id == params.thread_id + ) => + { + history + } + Some(history) + if params.history_revision.is_some() + && params.history_revision + == super::history_revision::read(&rollout_path).await => + { + history + } + _ => Arc::new( + super::model_context::load_from_rollout_path(store, params.thread_id, &rollout_path) + .await? + .items, + ), + }; + let history_mode = canonical_history_mode_from_rollout_items(&history); let cwd = params .metadata .cwd @@ -122,7 +131,8 @@ pub(super) async fn resume_thread( history_mode, writer_lock, ) - .await + .await?; + Ok(history) } #[tracing::instrument( diff --git a/codex-rs/thread-store/src/local/mod.rs b/codex-rs/thread-store/src/local/mod.rs index 05f2d9061da5..e492b9ab664b 100644 --- a/codex-rs/thread-store/src/local/mod.rs +++ b/codex-rs/thread-store/src/local/mod.rs @@ -2,6 +2,7 @@ mod archive_thread; mod create_thread; mod delete_thread; mod helpers; +mod history_revision; mod list_threads; mod live_writer; mod model_context; @@ -44,6 +45,7 @@ mod timestamp_metadata_tests; use codex_protocol::ThreadId; use codex_protocol::protocol::ThreadHistoryMode; +use codex_rollout::RolloutItem; use codex_rollout::RolloutRecorder; use codex_rollout::StateDbHandle; use codex_rollout::WriterLockCoordinator; @@ -526,7 +528,10 @@ impl ThreadStore for LocalThreadStore { }) } - fn resume_thread(&self, params: ResumeThreadParams) -> ThreadStoreFuture<'_, ()> { + fn resume_thread( + &self, + params: ResumeThreadParams, + ) -> ThreadStoreFuture<'_, Arc>> { Box::pin(async move { live_writer::resume_thread(self, params).await }) } @@ -804,6 +809,7 @@ mod tests { use codex_protocol::items::TurnItem; use codex_protocol::items::UserMessageItem; use codex_protocol::models::BaseInstructions; + use codex_protocol::models::ContentItem; use codex_protocol::models::FunctionCallOutputPayload; use codex_protocol::models::MessagePhase; use codex_protocol::models::ResponseItem; @@ -992,13 +998,15 @@ mod tests { .await .expect("update stale sqlite rollout path"); - let resumed = LiveThread::resume( + let history = Arc::new(vec![user_message_item("bounded suffix")]); + let (resumed, supplied_history) = LiveThread::resume( store, ThreadHistoryMode::Paginated, ResumeThreadParams { + history_revision: None, thread_id, rollout_path: Some(rollout_path.clone()), - history: Some(Arc::new(vec![user_message_item("bounded suffix")])), + history: Some(Arc::clone(&history)), include_archived: false, metadata: ThreadPersistenceMetadata { cwd: Some(home.path().to_path_buf()), @@ -1009,9 +1017,10 @@ mod tests { ) .await .expect("resume paginated thread from its requested rollout"); + assert!(Arc::ptr_eq(&supplied_history, &history)); assert_eq!( resumed.local_rollout_path().await.expect("live rollout"), - Some(rollout_path) + Some(std::fs::canonicalize(rollout_path).expect("canonical rollout path")) ); resumed.shutdown().await.expect("shutdown resumed writer"); @@ -1344,10 +1353,11 @@ mod tests { let thread_id = ThreadId::from_string(&uuid.to_string()).expect("valid thread id"); let rollout_path = write_session_file(home.path(), "2025-01-03T17-00-00", uuid).expect("session file"); - let live_thread = LiveThread::resume( + let (live_thread, _history) = LiveThread::resume( store, ThreadHistoryMode::Legacy, ResumeThreadParams { + history_revision: None, thread_id, rollout_path: Some(rollout_path), history: None, @@ -1399,10 +1409,11 @@ mod tests { let thread_id = ThreadId::from_string(&uuid.to_string()).expect("valid thread id"); let rollout_path = write_session_file(external_home.path(), "2025-01-03T17-30-00", uuid) .expect("external session file"); - let live_thread = LiveThread::resume( + let (live_thread, _history) = LiveThread::resume( store, ThreadHistoryMode::Legacy, ResumeThreadParams { + history_revision: None, thread_id, rollout_path: Some(rollout_path), history: None, @@ -1529,12 +1540,18 @@ mod tests { } #[tokio::test] - async fn resume_thread_reopens_live_writer_and_appends() { + async fn resume_thread_reopens_live_writer_and_appends_with_stale_sqlite_path() { let home = TempDir::new().expect("temp dir"); let config = test_config(home.path()); + let runtime = codex_state::StateRuntime::init( + config.sqlite.clone(), + config.default_model_provider_id.clone(), + ) + .await + .expect("state db should initialize"); let thread_id = ThreadId::default(); - let first_store = LocalThreadStore::new(config.clone(), /*state_db*/ None); + let first_store = LocalThreadStore::new(config.clone(), Some(runtime.clone())); first_store .create_thread(create_thread_params(thread_id)) .await @@ -1563,9 +1580,24 @@ mod tests { .await .expect("shutdown initial writer"); - let resumed_store = LocalThreadStore::new(config, /*state_db*/ None); + let mut builder = codex_state::ThreadMetadataBuilder::new( + thread_id, + home.path().join("missing-rollout.jsonl"), + chrono::Utc::now(), + SessionSource::Exec, + ); + builder.history_mode = ThreadHistoryMode::Legacy; + builder.cwd = home.path().to_path_buf(); + let metadata = builder.build("test-provider"); + runtime + .upsert_thread(&metadata) + .await + .expect("store stale sqlite path"); + + let resumed_store = LocalThreadStore::new(config, Some(runtime)); resumed_store .resume_thread(ResumeThreadParams { + history_revision: None, thread_id, rollout_path: None, history: None, @@ -1614,10 +1646,19 @@ mod tests { .live_rollout_path(thread_id) .await .expect("load rollout path"); + let snapshot = primary + .load_latest_model_context(LoadThreadHistoryParams { + thread_id, + include_archived: true, + }) + .await + .expect("load initial snapshot"); + let mut expected = snapshot.items; let resume_params = ResumeThreadParams { + history_revision: snapshot.revision, thread_id, rollout_path: Some(rollout_path), - history: None, + history: Some(Arc::new(expected.clone())), include_archived: true, metadata: thread_metadata(), }; @@ -1637,11 +1678,45 @@ mod tests { primary .shutdown_thread(thread_id) .await - .expect("shutdown should release writer ownership"); + .expect("release writer"); + let unchanged = secondary + .resume_thread(resume_params.clone()) + .await + .expect("resume unchanged snapshot"); + assert!(Arc::ptr_eq( + &unchanged, + resume_params.history.as_ref().unwrap() + )); + + let late_edit = RolloutItem::ResponseItem( + ResponseItem::Message { + id: None, + role: "user".to_string(), + content: vec![ContentItem::InputText { + text: "edit after the initial snapshot".to_string(), + }], + phase: None, + internal_chat_message_metadata_passthrough: None, + } + .into(), + ); secondary + .append_items(AppendThreadItemsParams { + thread_id, + items: vec![late_edit.clone()], + }) + .await + .expect("append before handing off writer ownership"); + expected.push(late_edit); + secondary + .shutdown_thread(thread_id) + .await + .expect("shutdown should release writer ownership"); + let resumed = primary .resume_thread(resume_params) .await .expect("resume after shutdown should acquire writer ownership"); + assert_eq!(serde_json::json!(*resumed), serde_json::json!(expected)); } } @@ -1681,6 +1756,7 @@ mod tests { .expect("live rollout path"); let err = store .resume_thread(ResumeThreadParams { + history_revision: None, thread_id, rollout_path: Some(rollout_path), history: None, @@ -1710,6 +1786,7 @@ mod tests { .expect("session file"); let err = store .resume_thread(ResumeThreadParams { + history_revision: None, thread_id, rollout_path: Some(rollout_path.clone()), history: None, @@ -1728,6 +1805,7 @@ mod tests { competing_store .resume_thread(ResumeThreadParams { + history_revision: None, thread_id, rollout_path: Some(rollout_path), history: None, @@ -1750,6 +1828,7 @@ mod tests { store .resume_thread(ResumeThreadParams { + history_revision: None, thread_id, rollout_path: Some(rollout_path), history: None, @@ -1798,6 +1877,7 @@ mod tests { store .resume_thread(ResumeThreadParams { + history_revision: None, thread_id, rollout_path: Some(rollout_path.clone()), history: None, @@ -1816,7 +1896,10 @@ mod tests { .await .expect("read external live thread"); - assert_eq!(thread.rollout_path, Some(rollout_path)); + assert_eq!( + thread.rollout_path, + Some(std::fs::canonicalize(rollout_path).expect("canonical rollout path")) + ); assert!(thread.history.expect("history").items.iter().any(|item| { matches!( item, @@ -1842,15 +1925,30 @@ mod tests { let thread_id = ThreadId::from_string(&uuid.to_string()).expect("valid thread id"); let rollout_path = write_archived_session_file(home.path(), "2025-01-04T10-30-00", uuid) .expect("archived session file"); + #[cfg(unix)] + let rollout_path = { + let alias = home.path().join("active-alias.jsonl"); + std::os::unix::fs::symlink(&rollout_path, &alias).expect("symlink archived rollout"); + alias + }; + let mut resume_params = ResumeThreadParams { + history_revision: None, + thread_id, + rollout_path: Some(rollout_path), + history: None, + include_archived: false, + metadata: thread_metadata(), + }; + let err = store + .resume_thread(resume_params.clone()) + .await + .expect_err("active-only resume should reject archived rollout targets"); + assert!(matches!(err, ThreadStoreError::InvalidRequest { .. })); + assert!(err.to_string().contains("archived")); + resume_params.include_archived = true; store - .resume_thread(ResumeThreadParams { - thread_id, - rollout_path: Some(rollout_path), - history: None, - include_archived: true, - metadata: thread_metadata(), - }) + .resume_thread(resume_params) .await .expect("resume live archived thread"); store @@ -2015,6 +2113,7 @@ mod tests { ); store .resume_thread(ResumeThreadParams { + history_revision: None, thread_id, rollout_path: Some(rollout_path), history: None, diff --git a/codex-rs/thread-store/src/local/model_context.rs b/codex-rs/thread-store/src/local/model_context.rs index 7e898add6dff..13a81b71ad8d 100644 --- a/codex-rs/thread-store/src/local/model_context.rs +++ b/codex-rs/thread-store/src/local/model_context.rs @@ -1,6 +1,7 @@ //! Reconstructs model context and preserves source runtime metadata across fork cutoffs. use std::io; +use std::path::Path; use codex_protocol::protocol::HistoryPosition; use codex_protocol::protocol::SessionMetaLine; @@ -48,31 +49,44 @@ pub(super) async fn load_latest_model_context( message: format!("no rollout found for thread id {}", params.thread_id), })?; - let session_meta = codex_rollout::read_session_meta_line(path.as_path()) + load_from_rollout_path(store, params.thread_id, &path).await +} + +pub(super) async fn load_from_rollout_path( + store: &LocalThreadStore, + thread_id: codex_protocol::ThreadId, + path: &Path, +) -> ThreadStoreResult { + let before = super::history_revision::read(path).await; + let session_meta = codex_rollout::read_session_meta_line(path) .await .map_err(|err| ThreadStoreError::Internal { message: format!("failed to read session metadata {}: {err}", path.display()), })?; - if session_meta.meta.id != params.thread_id { + if session_meta.meta.id != thread_id { return Err(ThreadStoreError::InvalidRequest { message: format!( "rollout at {} belongs to thread {}, not {}", path.display(), session_meta.meta.id, - params.thread_id + thread_id ), }); } let items = if matches!(session_meta.meta.history_mode, ThreadHistoryMode::Paginated) { - let lineage = store.resolve_rollout_lineage(params.thread_id).await?; + let lineage = store + .resolve_rollout_lineage(thread_id, Some(path.to_path_buf())) + .await?; scan_model_context_from_lineage(lineage, session_meta).await? } else { - read_thread::load_history_items(path.as_path()).await? + read_thread::load_history_items(path).await? }; + let after = super::history_revision::read(path).await; Ok(StoredModelContext { - thread_id: params.thread_id, + revision: before.filter(|revision| Some(revision) == after.as_ref()), + thread_id, items, }) } diff --git a/codex-rs/thread-store/src/local/model_context_tests.rs b/codex-rs/thread-store/src/local/model_context_tests.rs index eeef222d75d5..7be08ce485a8 100644 --- a/codex-rs/thread-store/src/local/model_context_tests.rs +++ b/codex-rs/thread-store/src/local/model_context_tests.rs @@ -153,7 +153,7 @@ async fn fork_context_excludes_items_after_frozen_cutoff() { append_items(path.as_path(), [user_message("later message")]); let store = LocalThreadStore::new(test_config(home.path()), /*state_db*/ None); let lineage = store - .resolve_rollout_lineage(thread_id) + .resolve_rollout_lineage(thread_id, /*initial_path*/ None) .await .expect("resolve source lineage"); let session_meta = codex_rollout::read_session_meta_line(path.as_path()) @@ -230,7 +230,7 @@ async fn fork_version_stops_before_older_segments_once_resolved() { ); let store = LocalThreadStore::new(test_config(home.path()), /*state_db*/ None); let lineage = store - .resolve_rollout_lineage(child_id) + .resolve_rollout_lineage(child_id, /*initial_path*/ None) .await .expect("resolve source lineage"); // The usable compaction should stop the scan before it reaches this missing segment. @@ -282,7 +282,7 @@ async fn fork_version_respects_inherited_segment_cutoffs() { ); let store = LocalThreadStore::new(test_config(home.path()), /*state_db*/ None); let lineage = store - .resolve_rollout_lineage(child_id) + .resolve_rollout_lineage(child_id, /*initial_path*/ None) .await .expect("resolve source lineage"); let mut source_meta = codex_rollout::read_session_meta_line(&child_path) diff --git a/codex-rs/thread-store/src/local/read_thread.rs b/codex-rs/thread-store/src/local/read_thread.rs index 2c613b0f0d11..3401702d75d5 100644 --- a/codex-rs/thread-store/src/local/read_thread.rs +++ b/codex-rs/thread-store/src/local/read_thread.rs @@ -199,7 +199,7 @@ fn reject_paginated_history(thread: &StoredThread, include_history: bool) -> Thr Ok(()) } -async fn resolve_requested_rollout_path( +pub(super) async fn resolve_requested_rollout_path( store: &LocalThreadStore, rollout_path: std::path::PathBuf, ) -> ThreadStoreResult { @@ -253,8 +253,14 @@ async fn attach_history_if_requested( message: format!("failed to load thread history for thread {thread_id}"), }); }; + let before = super::history_revision::read(&path).await; let items = load_history_items(&path).await?; - thread.history = Some(StoredThreadHistory { thread_id, items }); + let after = super::history_revision::read(&path).await; + thread.history = Some(StoredThreadHistory { + revision: before.filter(|revision| Some(revision) == after.as_ref()), + thread_id, + items, + }); Ok(()) } diff --git a/codex-rs/thread-store/src/local/revert_thread.rs b/codex-rs/thread-store/src/local/revert_thread.rs index 5aeebce31ee4..424a0f54c165 100644 --- a/codex-rs/thread-store/src/local/revert_thread.rs +++ b/codex-rs/thread-store/src/local/revert_thread.rs @@ -79,7 +79,9 @@ pub(super) async fn revert( // Preserve old-reader compatibility when introducing the first reference to a standalone // source. Already-shared ancestors stay read-only; their offsets address decoded JSONL bytes. - let mut lineage = store.resolve_rollout_lineage(thread_id).await?; + let mut lineage = store + .resolve_rollout_lineage(thread_id, /*initial_path*/ None) + .await?; for segment in &mut lineage.segments { if segment.rollout_id() == current_rollout.rollout_id && source_meta.history_base.is_none() { diff --git a/codex-rs/thread-store/src/local/rollout_lineage.rs b/codex-rs/thread-store/src/local/rollout_lineage.rs index ae3e1bec62f9..556ca03767bc 100644 --- a/codex-rs/thread-store/src/local/rollout_lineage.rs +++ b/codex-rs/thread-store/src/local/rollout_lineage.rs @@ -33,10 +33,12 @@ impl LocalThreadStore { pub(super) async fn resolve_rollout_lineage( &self, requested_thread_id: ThreadId, + initial_path: Option, ) -> ThreadStoreResult { self.resolve_rollout_lineage_with_representation( requested_thread_id, LineageRepresentation::Existing, + initial_path, ) .await } @@ -48,6 +50,7 @@ impl LocalThreadStore { self.resolve_rollout_lineage_with_representation( requested_thread_id, LineageRepresentation::PlainForReference, + /*initial_path*/ None, ) .await } @@ -56,6 +59,7 @@ impl LocalThreadStore { &self, requested_thread_id: ThreadId, representation: LineageRepresentation, + mut initial_path: Option, ) -> ThreadStoreResult { let mut segments = Vec::new(); let mut seen = HashSet::new(); @@ -70,14 +74,23 @@ impl LocalThreadStore { Some(self.live_writer_locks.lock(coordination_id).await) } }; - let (rollout_id, rollout_path) = match next_rollout_id { - Some(rollout_id) => { + let (rollout_id, rollout_path) = match (next_rollout_id, initial_path.take()) { + (Some(rollout_id), _) => { let rollout_path = resolve_rollout_path_by_id(self, rollout_id) .await? .ok_or_else(|| malformed_lineage(rollout_id, "missing source rollout"))?; (rollout_id, rollout_path) } - None => { + (None, Some(path)) => { + let rollout_id = + thread_rollout_resolver::rollout_id_from_path_or_legacy_thread_id( + &path, + requested_thread_id, + ThreadHistoryMode::Paginated, + )?; + (rollout_id, path) + } + (None, None) => { let resolved = thread_rollout_resolver::resolve_current_including_archived( self, requested_thread_id, diff --git a/codex-rs/thread-store/src/local/rollout_lineage_tests.rs b/codex-rs/thread-store/src/local/rollout_lineage_tests.rs index 6b8c88d118e8..be93f4a38cc1 100644 --- a/codex-rs/thread-store/src/local/rollout_lineage_tests.rs +++ b/codex-rs/thread-store/src/local/rollout_lineage_tests.rs @@ -74,7 +74,7 @@ async fn resolves_nested_lineage_with_empty_intermediate_segments() { ); let lineage = store - .resolve_rollout_lineage(child) + .resolve_rollout_lineage(child, /*initial_path*/ None) .await .expect("resolve nested lineage"); @@ -127,7 +127,7 @@ async fn resolves_archived_ancestors() { ); let lineage = store - .resolve_rollout_lineage(child) + .resolve_rollout_lineage(child, /*initial_path*/ None) .await .expect("resolve archived ancestor"); @@ -155,7 +155,7 @@ async fn resolves_lineage_at_explicit_history_position() { ); let lineage = store - .resolve_rollout_lineage(child) + .resolve_rollout_lineage(child, /*initial_path*/ None) .await .expect("resolve child lineage") .truncate_at(end) @@ -246,7 +246,7 @@ async fn rejects_missing_cycles_and_out_of_bounds_offsets() { async fn assert_invalid_lineage(store: &LocalThreadStore, thread_id: ThreadId, detail: &str) { let err = store - .resolve_rollout_lineage(thread_id) + .resolve_rollout_lineage(thread_id, /*initial_path*/ None) .await .expect_err("lineage should be invalid"); assert!(err.to_string().contains(detail), "{err}"); diff --git a/codex-rs/thread-store/src/local/thread_history/read.rs b/codex-rs/thread-store/src/local/thread_history/read.rs index dad2a2646778..49c326c1a4bb 100644 --- a/codex-rs/thread-store/src/local/thread_history/read.rs +++ b/codex-rs/thread-store/src/local/thread_history/read.rs @@ -110,7 +110,9 @@ pub(in crate::local) async fn list_turns( ) .await?; validate_page_size(params.page_size)?; - let lineage = store.resolve_rollout_lineage(params.thread_id).await?; + let lineage = store + .resolve_rollout_lineage(params.thread_id, /*initial_path*/ None) + .await?; let pool = store.thread_history_db().await?; let page = page_turn_rows( pool, @@ -168,7 +170,9 @@ pub(in crate::local) async fn list_items( ) .await?; validate_page_size(params.page_size)?; - let lineage = store.resolve_rollout_lineage(params.thread_id).await?; + let lineage = store + .resolve_rollout_lineage(params.thread_id, /*initial_path*/ None) + .await?; let pool = store.thread_history_db().await?; let page = page_item_rows(pool, &lineage, ¶ms).await?; diff --git a/codex-rs/thread-store/src/local/thread_history/realtime.rs b/codex-rs/thread-store/src/local/thread_history/realtime.rs index 0dd233b77670..cfb897caf349 100644 --- a/codex-rs/thread-store/src/local/thread_history/realtime.rs +++ b/codex-rs/thread-store/src/local/thread_history/realtime.rs @@ -52,7 +52,9 @@ pub(in crate::local) async fn list_timeline( .await?; validate_page_size(params.page_size)?; - let lineage = store.resolve_rollout_lineage(params.thread_id).await?; + let lineage = store + .resolve_rollout_lineage(params.thread_id, /*initial_path*/ None) + .await?; let pool = store.thread_history_db().await?; let cursor = params .cursor diff --git a/codex-rs/thread-store/src/local/thread_history/search.rs b/codex-rs/thread-store/src/local/thread_history/search.rs index f93fe61eb30d..7fa10e2419d1 100644 --- a/codex-rs/thread-store/src/local/thread_history/search.rs +++ b/codex-rs/thread-store/src/local/thread_history/search.rs @@ -74,7 +74,9 @@ pub(in crate::local) async fn search_thread_occurrences( ¶ms.search_term, )?; let matcher = LiteralMatcher::new(params.search_term.as_str()); - let lineage = store.resolve_rollout_lineage(params.thread_id).await?; + let lineage = store + .resolve_rollout_lineage(params.thread_id, /*initial_path*/ None) + .await?; let cursor_segment = cursor .as_ref() .map(|cursor| { diff --git a/codex-rs/thread-store/src/local/thread_history_materialization_tests.rs b/codex-rs/thread-store/src/local/thread_history_materialization_tests.rs index d9034acd56be..07713ea56c01 100644 --- a/codex-rs/thread-store/src/local/thread_history_materialization_tests.rs +++ b/codex-rs/thread-store/src/local/thread_history_materialization_tests.rs @@ -195,6 +195,7 @@ async fn split_homes_support_backfill_listing_and_paginated_history() { store .resume_thread(ResumeThreadParams { + history_revision: None, thread_id, rollout_path: Some(rollout_path), history: None, @@ -1091,6 +1092,7 @@ async fn paginated_fork_reads_compressed_shared_lineage_without_materializing() fs::rename(&source_compressed_path, &external_path).expect("move shared source outside home"); store .resume_thread(ResumeThreadParams { + history_revision: None, thread_id: source_thread_id, rollout_path: Some(external_path), history: None, diff --git a/codex-rs/thread-store/src/local/update_thread_metadata.rs b/codex-rs/thread-store/src/local/update_thread_metadata.rs index 76bc877d5ea5..d8074171c194 100644 --- a/codex-rs/thread-store/src/local/update_thread_metadata.rs +++ b/codex-rs/thread-store/src/local/update_thread_metadata.rs @@ -1287,6 +1287,7 @@ mod tests { let path = write_session_file(home.path(), "2025-01-03T12-00-00", uuid)?; store .resume_thread(ResumeThreadParams { + history_revision: None, thread_id, rollout_path: Some(path.clone()), history: None, @@ -1506,6 +1507,7 @@ mod tests { store .resume_thread(ResumeThreadParams { + history_revision: None, thread_id, rollout_path: Some(path.clone()), history: None, @@ -2393,6 +2395,7 @@ mod tests { .await; store .resume_thread(ResumeThreadParams { + history_revision: None, thread_id, rollout_path: Some(archived_path.clone()), history: None, diff --git a/codex-rs/thread-store/src/store.rs b/codex-rs/thread-store/src/store.rs index d67c3b766a89..87f6d40c0ab3 100644 --- a/codex-rs/thread-store/src/store.rs +++ b/codex-rs/thread-store/src/store.rs @@ -1,8 +1,10 @@ use codex_protocol::ThreadId; use codex_protocol::protocol::ThreadHistoryMode; +use codex_rollout::RolloutItem; use std::any::Any; use std::future::Future; use std::pin::Pin; +use std::sync::Arc; use crate::AddThreadAttachmentOutcome; use crate::AddThreadAttachmentParams; @@ -138,8 +140,18 @@ pub trait ThreadStore: Any + Send + Sync { }) } - /// Reopens an existing thread for live appends. - fn resume_thread(&self, params: ResumeThreadParams) -> ThreadStoreFuture<'_, ()>; + /// Reopens an existing thread for live appends and returns its authoritative replay context. + /// + /// Stored snapshots supplied by callers may predate writer acquisition. Implementations must + /// include writes committed before acquisition in the returned context. Explicitly supplied + /// histories without a canonical session header retain their import/override semantics. + /// A supplied revision may enable reuse of the shared snapshot after validation; missing or + /// unrecognized revisions must not be treated as proof that stored history is unchanged. + /// On failure, implementations must release any writer acquired by this operation. + fn resume_thread( + &self, + params: ResumeThreadParams, + ) -> ThreadStoreFuture<'_, Arc>>; /// Appends raw rollout items to a live thread. /// diff --git a/codex-rs/thread-store/src/thread_metadata_sync.rs b/codex-rs/thread-store/src/thread_metadata_sync.rs index 8e1fc670409a..55edd7472eda 100644 --- a/codex-rs/thread-store/src/thread_metadata_sync.rs +++ b/codex-rs/thread-store/src/thread_metadata_sync.rs @@ -872,6 +872,7 @@ mod tests { fn resume_params(thread_id: ThreadId, history: Vec) -> ResumeThreadParams { ResumeThreadParams { + history_revision: None, thread_id, rollout_path: None, history: Some(Arc::new(history)), diff --git a/codex-rs/thread-store/src/types.rs b/codex-rs/thread-store/src/types.rs index 5030f8864d3c..7cc8a70b2150 100644 --- a/codex-rs/thread-store/src/types.rs +++ b/codex-rs/thread-store/src/types.rs @@ -125,8 +125,12 @@ pub struct ResumeThreadParams { pub thread_id: ThreadId, /// Known local rollout path when the caller resumed from a specific file. pub rollout_path: Option, - /// Known replay history for the resumed thread, if already loaded by the caller. + /// Previously loaded replay history. A canonical session header identifies a stored snapshot + /// that may need refreshing under writer ownership; other histories are explicit overrides. pub history: Option>>, + /// Opaque revision paired with `history`, validated by the store under writer ownership. + #[serde(skip)] + pub history_revision: Option, /// Whether archived threads may be reopened. pub include_archived: bool, /// Metadata for future writes appended to the resumed live thread. @@ -175,6 +179,9 @@ pub struct StoredThreadHistory { pub thread_id: ThreadId, /// Persisted rollout items in replay order. pub items: Vec, + /// Optional revision for reusing this exact snapshot within the current process. + #[serde(skip)] + pub revision: Option, } /// Persisted rollout items needed to reconstruct the latest model-visible context. @@ -188,6 +195,9 @@ pub struct StoredModelContext { pub thread_id: ThreadId, /// Persisted rollout items in replay order. pub items: Vec, + /// Optional revision for reusing this exact snapshot within the current process. + #[serde(skip)] + pub revision: Option, } /// Requested boundary for inheriting a paginated thread's history. diff --git a/codex-rs/tui/src/app_server_session.rs b/codex-rs/tui/src/app_server_session.rs index f9c6f5fd7793..fac4b637022f 100644 --- a/codex-rs/tui/src/app_server_session.rs +++ b/codex-rs/tui/src/app_server_session.rs @@ -1493,6 +1493,7 @@ impl AppServerSession { .request_typed(ClientRequest::ThreadGoalSet { request_id, params: ThreadGoalSetParams { + origin: Some(codex_app_server_protocol::ThreadGoalMutationOrigin::User), thread_id: thread_id.to_string(), objective, status, @@ -1512,6 +1513,7 @@ impl AppServerSession { .request_typed(ClientRequest::ThreadGoalClear { request_id, params: ThreadGoalClearParams { + origin: Some(codex_app_server_protocol::ThreadGoalMutationOrigin::User), thread_id: thread_id.to_string(), }, }) diff --git a/codex-rs/tui/src/markdown_copy.rs b/codex-rs/tui/src/markdown_copy.rs index d2513211a828..ebc933a76b9f 100644 --- a/codex-rs/tui/src/markdown_copy.rs +++ b/codex-rs/tui/src/markdown_copy.rs @@ -26,6 +26,8 @@ pub(crate) enum Inline { Link(Arc), Delimiter(&'static str), Code, + /// Copy a generated file target literally without changing mixed-selection Markdown. + Literal, /// Balance parser events for links whose visible text has no copied wrapper. Ignored, } @@ -66,6 +68,7 @@ pub(crate) struct CopyLine { /// A visual separator inserted between sibling list items, never source content. pub(crate) omit: bool, runs: Vec<(Range, Vec)>, + literal_ranges: Vec>, } impl CopyLine { @@ -73,9 +76,13 @@ impl CopyLine { if length == 0 { return; } - let mut inline = inline[..inline.len().min(MAX_COPY_DEPTH)] + let inline = &inline[..inline.len().min(MAX_COPY_DEPTH)]; + let literal = inline .iter() - .filter(|mark| **mark != Inline::Ignored) + .any(|mark| matches!(mark, Inline::Code | Inline::Literal)); + let mut inline = inline + .iter() + .filter(|mark| !matches!(mark, Inline::Ignored | Inline::Literal)) .cloned() .collect::>(); inline.sort(); @@ -84,6 +91,16 @@ impl CopyLine { .runs .last() .map_or(/*default*/ 0, |(range, _)| range.end); + // Keep literal spans separate so they never split context-sensitive prose escaping. + if literal { + if let Some(range) = self.literal_ranges.last_mut() + && range.end == start + { + range.end += length; + } else { + self.literal_ranges.push(start..start + length); + } + } if let Some((range, previous)) = self.runs.last_mut() && previous == &inline { @@ -93,11 +110,12 @@ impl CopyLine { } } - fn is_inline_code(&self, range: &Range) -> bool { + fn is_literal(&self, range: &Range) -> bool { !range.is_empty() - && self.runs.iter().any(|(run, inline)| { - run.start <= range.start && range.end <= run.end && inline.contains(&Inline::Code) - }) + && self + .literal_ranges + .iter() + .any(|literal| literal.start <= range.start && range.end <= literal.end) } fn render(&self, text: &str, range: Range, depth: usize) -> String { @@ -172,7 +190,7 @@ impl CopyLine { .replace('\r', "%0D"); append_inline(&mut out, &format!("[{trimmed}](<{destination}>)")); } - Inline::Code | Inline::Ignored => unreachable!(), + Inline::Code | Inline::Literal | Inline::Ignored => unreachable!(), } out.push_str(&body[body.trim_end().len()..]); } @@ -267,7 +285,7 @@ pub(crate) fn selection(lines: &[SelectedLine], plain: &str) -> (String, CopyFor .source .copy .as_ref() - .is_some_and(|copy| copy.is_inline_code(&line.range)) + .is_some_and(|copy| copy.is_literal(&line.range)) { return (plain.to_owned(), CopyFormat::PlainText); } diff --git a/codex-rs/tui/src/markdown_copy/table.rs b/codex-rs/tui/src/markdown_copy/table.rs index affc6dcbfd24..54ff6d11b851 100644 --- a/codex-rs/tui/src/markdown_copy/table.rs +++ b/codex-rs/tui/src/markdown_copy/table.rs @@ -167,7 +167,7 @@ pub(super) fn render( && source .copy .as_ref() - .is_some_and(|copy| copy.is_inline_code(&source.range)) + .is_some_and(|copy| copy.is_literal(&source.range)) { return ( source.text[source.range.clone()].to_owned(), diff --git a/codex-rs/tui/src/markdown_render.rs b/codex-rs/tui/src/markdown_render.rs index c5033f8045b8..e07cb5686854 100644 --- a/codex-rs/tui/src/markdown_render.rs +++ b/codex-rs/tui/src/markdown_render.rs @@ -2211,7 +2211,9 @@ impl<'a, 'policy> Writer<'a, 'policy> { } self.push_span_to_table_cell(" (".into()); } + self.copy_inline.push(crate::markdown_copy::Inline::Literal); self.push_span_to_table_cell(span); + self.copy_inline.pop(); if show_label { self.push_span_to_table_cell(")".into()); } @@ -2231,7 +2233,9 @@ impl<'a, 'policy> Writer<'a, 'policy> { } self.push_span(" (".into()); } + self.copy_inline.push(crate::markdown_copy::Inline::Literal); self.push_span(span); + self.copy_inline.pop(); if show_label { self.push_span(")".into()); } diff --git a/codex-rs/tui/src/session_archive_commands.rs b/codex-rs/tui/src/session_archive_commands.rs index 0da6c2a4959f..ea37b0fe209c 100644 --- a/codex-rs/tui/src/session_archive_commands.rs +++ b/codex-rs/tui/src/session_archive_commands.rs @@ -2,6 +2,8 @@ //! //! The CLI commands are thin app-server clients: resolve a user-provided UUID or exact session //! name, then call the corresponding app-server RPC. +//! Explicit remote session commands, including queue, leave authentication and execution to +//! the selected server without initializing the caller's credentials or runtime. use std::io::IsTerminal; use std::io::Write; @@ -11,6 +13,7 @@ use std::sync::Arc; use crate::Cli; use crate::app_server_session::AppServerSession; +use crate::app_server_session::ThreadParamsMode; use crate::legacy_core::config::ConfigBuilder; use crate::legacy_core::config::ConfigOverrides; use crate::legacy_core::config::load_config_toml_with_layer_stack; @@ -247,6 +250,25 @@ pub(super) async fn start_app_server_for_session_command( launch_loader_overrides.user_config_profile = Some(profile_v2.clone()); } + if let Some(endpoint) = explicit_remote_endpoint { + // Validate config before connecting, but leave authentication and execution + // to the selected server even when this caller has workload identity set. + launch_loader_overrides.ignore_login_requirements = true; + ConfigBuilder::default() + .codex_home(codex_home) + .cli_overrides(cli_kv_overrides) + .loader_overrides(launch_loader_overrides) + .strict_config(strict_config) + .build() + .await + .wrap_err("failed to load config.toml")?; + return Ok(AppServerSession::new( + super::connect_remote_app_server(endpoint).await?, + ThreadParamsMode::Remote, + ) + .with_remote_cwd_override(cli.cwd.clone())); + } + let workload_identity_selected = codex_login::is_workload_identity_selected(); let reuse_implicit_local_daemon = !cli.no_daemon && !workload_identity_selected diff --git a/codex-rs/tui/src/transcript_view/markdown_copy_tests.rs b/codex-rs/tui/src/transcript_view/markdown_copy_tests.rs index 6d7d5304a0f9..9c965bae0257 100644 --- a/codex-rs/tui/src/transcript_view/markdown_copy_tests.rs +++ b/codex-rs/tui/src/transcript_view/markdown_copy_tests.rs @@ -128,6 +128,23 @@ fn inline_code_selection_copies_only_selected_content() { ("before ``a`b`` after", "a`"), ("- **`café界_*`**", "café界_*"), ("before ` leading and trailing ` after", " leading"), + (r"before ``real\_literal\!`` after", r"real\_literal\!"), + ("before [`foo_bar!`](/repo/foo_bar!) after", "repo/foo_bar!"), + ("before [`foo_bar!`](/repo/foo_bar!) after", "bar!"), + ( + "[foo_bar!](/foo_bar!)[baz_qux!](/baz_qux!)", + "foo_bar!baz_qux!", + ), + ("`foo_bar!`[baz_qux!](/baz_qux!)", "foo_bar!baz_qux!"), + ("before **[file](/repo/foo_bar!)** after", "foo_bar!"), + ( + "| File |\n|---|\n| [`foo_bar!`](/repo/foo_bar!) |", + "foo_bar!", + ), + ( + "before [file]() after", + "repo/`*_[]<>~#|&!=-+.)", + ), ] { for width in [18, 80] { let layout = markdown_layout(source, width); @@ -146,6 +163,25 @@ fn inline_code_selection_copies_only_selected_content() { insta::assert_snapshot!(copies.join("\n")); } +#[test] +fn mixed_file_targets_preserve_markdown_escaping() { + for (source, expected) in [ + ("before [+](/+) after", "before + after"), + ("before [1.)]() after", "before 1.) after"), + ( + "| File |\n|---|\n| [`foo_bar!`](/repo/foo_bar!) |", + "| File |\n|---|\n| repo/foo\\_bar\\! |", + ), + ] { + let layout = markdown_layout(source, /*width*/ 80); + assert_eq!( + payload(&layout, 0..layout.text().len()), + (expected.into(), CopyFormat::Markdown), + "{source}" + ); + } +} + #[test] fn inline_boundaries_preserve_selected_text_and_formatting() { let mut copies = Vec::new(); diff --git a/codex-rs/tui/src/transcript_view/snapshots/codex_tui__transcript_view__markdown_copy_tests__inline_code_selection_copies_only_selected_content.snap b/codex-rs/tui/src/transcript_view/snapshots/codex_tui__transcript_view__markdown_copy_tests__inline_code_selection_copies_only_selected_content.snap index 9feeaa9dd6f8..8c2c1a888644 100644 --- a/codex-rs/tui/src/transcript_view/snapshots/codex_tui__transcript_view__markdown_copy_tests__inline_code_selection_copies_only_selected_content.snap +++ b/codex-rs/tui/src/transcript_view/snapshots/codex_tui__transcript_view__markdown_copy_tests__inline_code_selection_copies_only_selected_content.snap @@ -16,3 +16,21 @@ Selected "a`" → ("a`", PlainText) Selected "café界_*" → ("café界_*", PlainText) before ` leading and trailing ` after Selected " leading" → (" leading", PlainText) +before ``real\_literal\!`` after +Selected "real\\_literal\\!" → ("real\\_literal\\!", PlainText) +before [`foo_bar!`](/repo/foo_bar!) after +Selected "repo/foo_bar!" → ("repo/foo_bar!", PlainText) +before [`foo_bar!`](/repo/foo_bar!) after +Selected "bar!" → ("bar!", PlainText) +[foo_bar!](/foo_bar!)[baz_qux!](/baz_qux!) +Selected "foo_bar!baz_qux!" → ("foo_bar!baz_qux!", PlainText) +`foo_bar!`[baz_qux!](/baz_qux!) +Selected "foo_bar!baz_qux!" → ("foo_bar!baz_qux!", PlainText) +before **[file](/repo/foo_bar!)** after +Selected "foo_bar!" → ("foo_bar!", PlainText) +| File | +|---| +| [`foo_bar!`](/repo/foo_bar!) | +Selected "foo_bar!" → ("foo_bar!", PlainText) +before [file]() after +Selected "repo/`*_[]<>~#|&!=-+.)" → ("repo/`*_[]<>~#|&!=-+.)", PlainText) diff --git a/sdk/python/src/openai_codex/generated/v2_all.py b/sdk/python/src/openai_codex/generated/v2_all.py index 6a2edc47255b..e2fef8e38359 100644 --- a/sdk/python/src/openai_codex/generated/v2_all.py +++ b/sdk/python/src/openai_codex/generated/v2_all.py @@ -5247,13 +5247,6 @@ class ThreadExtra(BaseModel): ) -class ThreadGoalClearParams(BaseModel): - model_config = ConfigDict( - populate_by_name=True, - ) - thread_id: Annotated[str, Field(alias="threadId")] - - class ThreadGoalClearResponse(BaseModel): model_config = ConfigDict( populate_by_name=True, @@ -5275,6 +5268,11 @@ class ThreadGoalGetParams(BaseModel): thread_id: Annotated[str, Field(alias="threadId")] +class ThreadGoalMutationOrigin(Enum): + user = "user" + automatic = "automatic" + + class ThreadGoalStatus(Enum): active = "active" paused = "paused" @@ -6867,15 +6865,6 @@ class ThreadGoalGetRequest(BaseModel): params: ThreadGoalGetParams -class ThreadGoalClearRequest(BaseModel): - model_config = ConfigDict( - populate_by_name=True, - ) - id: RequestId - method: Annotated[Literal["thread/goal/clear"], Field(title="Thread/goal/clearRequestMethod")] - params: ThreadGoalClearParams - - class ThreadMetadataUpdateRequest(BaseModel): model_config = ConfigDict( populate_by_name=True, @@ -9812,6 +9801,17 @@ class ThreadGoal(BaseModel): updated_at: Annotated[int, Field(alias="updatedAt")] +class ThreadGoalClearParams(BaseModel): + model_config = ConfigDict( + populate_by_name=True, + ) + origin: Annotated[ + ThreadGoalMutationOrigin | None, + Field(description="Missing provenance does not supply user authorization."), + ] = None + thread_id: Annotated[str, Field(alias="threadId")] + + class ThreadGoalGetResponse(BaseModel): model_config = ConfigDict( populate_by_name=True, @@ -9824,6 +9824,10 @@ class ThreadGoalSetParams(BaseModel): populate_by_name=True, ) objective: str | None = None + origin: Annotated[ + ThreadGoalMutationOrigin | None, + Field(description="Missing provenance does not supply user authorization."), + ] = None status: ThreadGoalStatus | None = None thread_id: Annotated[str, Field(alias="threadId")] token_budget: Annotated[int | None, Field(alias="tokenBudget")] = None @@ -10524,6 +10528,15 @@ class ThreadGoalSetRequest(BaseModel): params: ThreadGoalSetParams +class ThreadGoalClearRequest(BaseModel): + model_config = ConfigDict( + populate_by_name=True, + ) + id: RequestId + method: Annotated[Literal["thread/goal/clear"], Field(title="Thread/goal/clearRequestMethod")] + params: ThreadGoalClearParams + + class ThreadListRequest(BaseModel): model_config = ConfigDict( populate_by_name=True,