mirror of
https://github.com/openai/codex.git
synced 2026-11-08 20:40:33 +00:00
Preserve model catalogs when starting child agents (#60049)
## Why Child startup can lose remote model capabilities such as `tool_search` when the selected catalog is uninitialized or disk data is older than the parent's in-memory snapshot. ## What changed - Initialize the final provider, auth, and originator catalog before resolving a child's startup model, then read it from memory. - Preserve loaded catalogs, including empty responses and updates that failed to persist, without overwriting concurrent refreshes. - Reuse catalogs across retry and WebSocket transport overrides while retaining routing and header distinctions. ## Testing Add regression coverage for child tool search with warm, cold, and disabled caches; failed cache writes; concurrent refreshes; empty catalogs; and Guardian reviewer catalog reuse. GitOrigin-RevId: 01d995a65b623cc7d6f26e2dc9b6e8cf380de782
This commit is contained in:
@@ -605,15 +605,14 @@ impl Session {
|
||||
session_source: &SessionSource,
|
||||
allow_provider_model_fallback: bool,
|
||||
) -> CodexResult<(String, CatalogModel)> {
|
||||
// Child catalogs are initialized by their selector. Reading only memory here
|
||||
// preserves the loaded snapshot shared with the parent.
|
||||
let refresh_strategy = if session_source.is_non_root_agent() {
|
||||
RefreshStrategy::Offline
|
||||
RefreshStrategy::InMemory
|
||||
} else {
|
||||
RefreshStrategy::OnlineIfUncached
|
||||
};
|
||||
if config.features.enabled(Feature::RuntimeAgentInstances)
|
||||
|| config.model.is_none()
|
||||
|| refresh_strategy != RefreshStrategy::Offline
|
||||
{
|
||||
if !session_source.is_non_root_agent() {
|
||||
let _ = models_manager
|
||||
.list_models(refresh_strategy, config.http_client_factory())
|
||||
.await;
|
||||
|
||||
@@ -360,6 +360,10 @@ impl GatedModelsManager {
|
||||
}
|
||||
|
||||
impl ModelsManager for GatedModelsManager {
|
||||
fn ensure_initialized(&self, factory: HttpClientFactory) -> ModelsManagerFuture<'_, ()> {
|
||||
self.inner.ensure_initialized(factory)
|
||||
}
|
||||
|
||||
fn model_catalog_with_origins(
|
||||
&self,
|
||||
strategy: RefreshStrategy,
|
||||
|
||||
@@ -1116,12 +1116,16 @@ impl ThreadManager {
|
||||
/*parent_originator*/ None,
|
||||
)
|
||||
.await;
|
||||
let manager = self.state.models_for_thread(
|
||||
&options.config,
|
||||
&self.state.auth_manager,
|
||||
&self.state.runtime_auth,
|
||||
&originator,
|
||||
);
|
||||
let manager = self
|
||||
.state
|
||||
.models_for_thread(
|
||||
&options.config,
|
||||
&self.state.auth_manager,
|
||||
&self.state.runtime_auth,
|
||||
&originator,
|
||||
&source,
|
||||
)
|
||||
.await;
|
||||
let (_, info) = Session::resolve_startup_model(
|
||||
manager.as_ref(),
|
||||
&options.config,
|
||||
@@ -1919,12 +1923,13 @@ impl ThreadManager {
|
||||
}
|
||||
|
||||
impl ThreadManagerState {
|
||||
fn models_for_thread(
|
||||
async fn models_for_thread(
|
||||
&self,
|
||||
config: &Config,
|
||||
auth_manager: &Arc<AuthManager>,
|
||||
runtime_auth: &crate::runtime_auth::RuntimeAuthSlot,
|
||||
originator: &str,
|
||||
session_source: &SessionSource,
|
||||
) -> SharedModelsManager {
|
||||
let runtime = crate::runtime_auth::runtime_for_config(
|
||||
runtime_auth,
|
||||
@@ -1932,13 +1937,16 @@ impl ThreadManagerState {
|
||||
Arc::clone(auth_manager),
|
||||
&self.installation_id,
|
||||
);
|
||||
let manager = self.model_catalog.for_thread(
|
||||
config,
|
||||
Arc::clone(auth_manager),
|
||||
runtime,
|
||||
Arc::ptr_eq(auth_manager, &self.auth_manager),
|
||||
);
|
||||
manager.for_originator(originator).unwrap_or(manager)
|
||||
self.model_catalog
|
||||
.for_thread(
|
||||
config,
|
||||
Arc::clone(auth_manager),
|
||||
runtime,
|
||||
Arc::ptr_eq(auth_manager, &self.auth_manager),
|
||||
originator,
|
||||
session_source,
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
pub(crate) fn shared_thread_instructions_provider(
|
||||
@@ -2775,12 +2783,22 @@ impl ThreadManagerState {
|
||||
} else {
|
||||
Arc::new(mcp_manager.for_plugins_manager(Arc::clone(&plugins_manager)))
|
||||
};
|
||||
let models_manager = self.models_for_thread(
|
||||
&config,
|
||||
&auth_manager,
|
||||
&runtime_auth,
|
||||
&startup_context.originator,
|
||||
);
|
||||
let models_manager = tokio::select! {
|
||||
biased;
|
||||
_ = agent_control.runtime().shutdown.cancelled() => {
|
||||
if let Some(startup_guard) = startup_guard {
|
||||
startup_guard.cleanup().await;
|
||||
}
|
||||
return Err(CodexErr::TurnAborted);
|
||||
}
|
||||
manager = self.models_for_thread(
|
||||
&config,
|
||||
&auth_manager,
|
||||
&runtime_auth,
|
||||
&startup_context.originator,
|
||||
&session_source,
|
||||
) => manager,
|
||||
};
|
||||
let spawn_result = Session::spawn(SessionSpawnArgs {
|
||||
attached_thread,
|
||||
startup,
|
||||
|
||||
@@ -12,6 +12,7 @@ use codex_model_provider_info::ModelProviderInfo;
|
||||
use codex_models_manager::manager::ModelCatalogSettings;
|
||||
use codex_models_manager::manager::SharedModelsManager;
|
||||
use codex_protocol::openai_models::ModelsResponse;
|
||||
use codex_protocol::protocol::SessionSource;
|
||||
use codex_runtime_agent_instance::RuntimeAuthManager;
|
||||
use std::sync::Arc;
|
||||
|
||||
@@ -115,23 +116,35 @@ impl ThreadModelCatalog {
|
||||
}
|
||||
}
|
||||
|
||||
pub(super) fn for_thread(
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub(super) async fn for_thread(
|
||||
&self,
|
||||
config: &Config,
|
||||
credentials: Arc<AuthManager>,
|
||||
runtime: Option<Arc<RuntimeAuthManager>>,
|
||||
same_owner: bool,
|
||||
originator: &str,
|
||||
session_source: &SessionSource,
|
||||
) -> SharedModelsManager {
|
||||
let models = config.model_catalog.as_ref().or_else(|| {
|
||||
self.catalog
|
||||
.as_ref()
|
||||
.filter(|_| self.provider == config.model_provider)
|
||||
.filter(|_| self.matches_catalog_provider(&config.model_provider))
|
||||
});
|
||||
let auth = ProviderAuthContext {
|
||||
credentials,
|
||||
runtime_auth: runtime.map(|runtime| (runtime, config.runtime_auth_mode)),
|
||||
};
|
||||
self.for_config(config, models, auth, same_owner)
|
||||
let manager = self.for_config(config, models, auth, same_owner);
|
||||
let manager = manager.for_originator(originator).unwrap_or(manager);
|
||||
if session_source.is_non_root_agent() {
|
||||
// The final provider/auth/originator catalog may be new even when the base
|
||||
// manager was reused. Preserve loaded snapshots, including unsaved updates.
|
||||
manager
|
||||
.ensure_initialized(config.http_client_factory())
|
||||
.await;
|
||||
}
|
||||
manager
|
||||
}
|
||||
|
||||
pub(super) fn for_config(
|
||||
@@ -145,7 +158,7 @@ impl ThreadModelCatalog {
|
||||
config.features.enabled(Feature::ApiKeyModelDiscovery);
|
||||
if same_owner
|
||||
&& self.runtime_mode == auth.runtime_auth.as_ref().map(|(_, mode)| *mode)
|
||||
&& self.provider == config.model_provider
|
||||
&& self.matches_catalog_provider(&config.model_provider)
|
||||
&& self.catalog.as_ref() == models
|
||||
&& self.api_key_model_discovery_default == api_key_model_discovery_default
|
||||
{
|
||||
@@ -158,6 +171,18 @@ impl ThreadModelCatalog {
|
||||
)
|
||||
}
|
||||
|
||||
fn matches_catalog_provider(&self, provider: &ModelProviderInfo) -> bool {
|
||||
// Guardian overrides inference transport and retries without changing the catalog.
|
||||
// Keep all other provider fields, including routing and headers, in the comparison.
|
||||
self.provider
|
||||
== ModelProviderInfo {
|
||||
request_max_retries: self.provider.request_max_retries,
|
||||
stream_max_retries: self.provider.stream_max_retries,
|
||||
supports_websockets: self.provider.supports_websockets,
|
||||
..provider.clone()
|
||||
}
|
||||
}
|
||||
|
||||
pub(super) fn set_api_key_model_discovery_enabled(&self, enabled: bool) {
|
||||
self.factory.set_api_key_model_discovery_enabled(enabled);
|
||||
}
|
||||
|
||||
@@ -2248,6 +2248,8 @@ async fn start_thread_seeds_extension_data_for_mcp_and_lifecycle_contributors()
|
||||
let lifecycle_observed = Arc::new(std::sync::Mutex::new(Vec::new()));
|
||||
let mcp_observed = Arc::new(std::sync::Mutex::new(Vec::new()));
|
||||
let mcp_loaded = Arc::new(std::sync::atomic::AtomicUsize::new(0));
|
||||
// This fixture counts explicit MCP projections, excluding background websocket prewarming.
|
||||
config.model_provider.supports_websockets = false;
|
||||
let recorder = Arc::new(InitialDataRecorder {
|
||||
lifecycle_observed: Arc::clone(&lifecycle_observed),
|
||||
mcp_observed: Arc::clone(&mcp_observed),
|
||||
@@ -3432,8 +3434,10 @@ async fn metadata_update_without_result_reads_only_when_the_caller_needs_the_thr
|
||||
);
|
||||
}
|
||||
|
||||
#[test_case::test_case(true; "disk_cache")]
|
||||
#[test_case::test_case(false; "without_cache")]
|
||||
#[tokio::test]
|
||||
async fn new_uses_active_provider_for_model_refresh() {
|
||||
async fn new_uses_active_provider_for_model_refresh(with_cache: bool) {
|
||||
let server = MockServer::start().await;
|
||||
let models_mock = mount_models_once(&server, ModelsResponse { models: vec![] }).await;
|
||||
|
||||
@@ -3453,7 +3457,11 @@ async fn new_uses_active_provider_for_model_refresh() {
|
||||
&config,
|
||||
auth_manager.clone(),
|
||||
TEST_INSTALLATION_ID.to_string(),
|
||||
Arc::new(ModelCatalogSettings::disk(config.codex_home.to_path_buf())),
|
||||
Arc::new(if with_cache {
|
||||
ModelCatalogSettings::disk(config.codex_home.to_path_buf())
|
||||
} else {
|
||||
ModelCatalogSettings::default()
|
||||
}),
|
||||
),
|
||||
crate::CodexAppsToolsCache::default(),
|
||||
SessionSource::Exec,
|
||||
@@ -3476,6 +3484,16 @@ async fn new_uses_active_provider_for_model_refresh() {
|
||||
.await;
|
||||
assert_eq!(models_mock.requests().len(), 1);
|
||||
|
||||
let mut thread_config = config.clone();
|
||||
thread_config.model = Some("remote-search-model".to_string());
|
||||
thread_config.model_provider.request_max_retries = Some(1);
|
||||
thread_config.model_provider.stream_max_retries = Some(1);
|
||||
thread_config
|
||||
.model_provider
|
||||
.http_headers
|
||||
.get_or_insert_default()
|
||||
.insert("X-OpenAI-Product-Sku".to_string(), "test-product".into());
|
||||
|
||||
// Thread startup must select the originator catalog before resolving model metadata.
|
||||
for originator in [
|
||||
"codex_work_cca",
|
||||
@@ -3484,24 +3502,32 @@ async fn new_uses_active_provider_for_model_refresh() {
|
||||
"codex_work_mobile",
|
||||
"chatgpt_cca",
|
||||
] {
|
||||
let mut model = codex_models_manager::model_info::model_info_from_slug("gpt-5.5");
|
||||
let mut model =
|
||||
codex_models_manager::model_info::model_info_from_slug("remote-search-model");
|
||||
model.description = Some(originator.to_string());
|
||||
model.supports_search_tool = true;
|
||||
model.tool_mode = Some(codex_protocol::openai_models::ToolMode::CodeModeOnly);
|
||||
model.use_responses_lite = true;
|
||||
wiremock::Mock::given(wiremock::matchers::method("GET"))
|
||||
.and(wiremock::matchers::path("/models"))
|
||||
.and(wiremock::matchers::header("originator", originator))
|
||||
.and(wiremock::matchers::header(
|
||||
"X-OpenAI-Product-Sku",
|
||||
"test-product",
|
||||
))
|
||||
.respond_with(
|
||||
wiremock::ResponseTemplate::new(200).set_body_json(ModelsResponse {
|
||||
models: vec![model],
|
||||
}),
|
||||
)
|
||||
.expect(1)
|
||||
.expect(if with_cache { 1..=1 } else { 1..=2 })
|
||||
.mount(&server)
|
||||
.await;
|
||||
let started = manager
|
||||
.start_thread(StartThreadOptions {
|
||||
metrics_service_name: Some(originator.to_string()),
|
||||
environments: Some(Vec::new()),
|
||||
..StartThreadOptions::new(config.clone())
|
||||
..StartThreadOptions::new(thread_config.clone())
|
||||
})
|
||||
.await
|
||||
.expect("start scoped thread");
|
||||
@@ -3510,9 +3536,50 @@ async fn new_uses_active_provider_for_model_refresh() {
|
||||
.session
|
||||
.services
|
||||
.models_manager
|
||||
.get_model_info("gpt-5.5", &config.to_models_manager_config())
|
||||
.get_model_info(
|
||||
"remote-search-model",
|
||||
&thread_config.to_models_manager_config(),
|
||||
)
|
||||
.await;
|
||||
assert_eq!(model.description.as_deref(), Some(originator));
|
||||
assert!(model.supports_search_tool);
|
||||
|
||||
let child = manager
|
||||
.state
|
||||
.spawn_child_thread(
|
||||
StartThreadOptions {
|
||||
session_source: Some(SessionSource::SubAgent(SubAgentSource::ThreadSpawn {
|
||||
parent_thread_id: started.thread_id,
|
||||
depth: 1,
|
||||
agent_path: None,
|
||||
agent_nickname: None,
|
||||
agent_role: None,
|
||||
})),
|
||||
environments: Some(Vec::new()),
|
||||
..StartThreadOptions::new(thread_config.clone())
|
||||
},
|
||||
started
|
||||
.thread
|
||||
.session
|
||||
.services
|
||||
.local_agent_runtime
|
||||
.control(started.thread.session.session_id()),
|
||||
Some(started.thread_id),
|
||||
/*inherited_exec_policy*/ None,
|
||||
)
|
||||
.await
|
||||
.expect("spawn child with the parent's provider headers");
|
||||
let (turn, _) = child
|
||||
.thread
|
||||
.session
|
||||
.new_turn_with_sub_id(
|
||||
"child-turn".to_string(),
|
||||
SessionSettingsUpdate::default(),
|
||||
Default::default(),
|
||||
)
|
||||
.await
|
||||
.expect("prepare child's first turn");
|
||||
assert_eq!(turn.model_info().as_ref(), &model);
|
||||
}
|
||||
let legacy = manager
|
||||
.start_thread(StartThreadOptions {
|
||||
|
||||
Reference in New Issue
Block a user