mirror of
https://github.com/openai/codex.git
synced 2026-10-20 00:18:49 +00:00
Treat explicit provider model catalogs as authoritative (#49135)
## Why Providers with `model_catalog_url` could advertise bundled models absent from their catalog or reuse stale models after a failed refresh. Model metadata could also match by prefix or namespaced suffix instead of the provider's exact model ID. ## What changed - Suppress bundled model fallback and merging for explicit provider catalogs, including when discovery is disabled. - Require unique, non-empty catalog slugs and match metadata by exact model ID. - Serialize explicit catalog refreshes and invalidate in-memory and persisted catalog entries after fetch failures, preventing stale cache reuse. - Report a clear configuration error when startup has no available model, while allowing an explicitly configured `model`. ## Testing Add regression coverage for exact model matching, hidden catalog entries, cache invalidation across restart and recovery, invalid slugs, and empty-catalog startup with and without an explicit model. Update app-server model-list expectations and verify the TUI configuration error. GitOrigin-RevId: ac323a94178e3c48df8d4a159a86fdbac20b03a2
This commit is contained in:
@@ -131,7 +131,15 @@ requires_openai_auth = true
|
||||
})
|
||||
.await?;
|
||||
let enabled = user_enablement.unwrap_or(app_enablement) && catalog_opt_in;
|
||||
let expected = if enabled { &remote } else { &bundled };
|
||||
let expected: &[ModelPreset] = if enabled {
|
||||
&remote
|
||||
} else if catalog_opt_in {
|
||||
// The rollout gate suppresses discovery, but an explicit catalog still
|
||||
// prevents bundled models from being advertised for this provider.
|
||||
&[]
|
||||
} else {
|
||||
&bundled
|
||||
};
|
||||
assert_eq!(
|
||||
response,
|
||||
ModelListResponse {
|
||||
|
||||
@@ -659,6 +659,11 @@ impl Session {
|
||||
config.http_client_factory(),
|
||||
)
|
||||
.await;
|
||||
if model.trim().is_empty() {
|
||||
return Err(CodexErr::InvalidRequest(
|
||||
"No models are available. Set `model` explicitly or check your model catalog configuration.".to_string(),
|
||||
));
|
||||
}
|
||||
let trusted_guardian_reviewer = crate::guardian::is_basic_session_source(&session_source)
|
||||
&& !matches!(conversation_history, InitialHistory::Resumed(_));
|
||||
if config
|
||||
|
||||
@@ -946,6 +946,7 @@ async fn remote_models_apply_legacy_instructions(auth: CodexAuth) -> Result<()>
|
||||
cwd,
|
||||
config,
|
||||
thread_manager,
|
||||
home: _home,
|
||||
..
|
||||
} = builder.build(&server).await?;
|
||||
|
||||
@@ -1484,6 +1485,51 @@ fn test_remote_model_with_policy(
|
||||
}
|
||||
}
|
||||
|
||||
#[test_case(None; "without explicit model")]
|
||||
#[test_case(Some("gateway-conversation"); "with explicit model")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn empty_model_catalog_requires_explicit_model(configured_model: Option<&str>) -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
let server = MockServer::start().await;
|
||||
Mock::given(method("GET"))
|
||||
.and(path("/codex/models"))
|
||||
.respond_with(ResponseTemplate::new(/*s*/ 200).set_body_json(ModelsResponse::default()))
|
||||
.expect(1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
let response = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]),
|
||||
)
|
||||
.await;
|
||||
let catalog_url = format!("{}/codex/models", server.uri());
|
||||
let model = configured_model.map(str::to_string);
|
||||
let result = test_codex()
|
||||
.with_auth(CodexAuth::from_api_key("gateway-api-key"))
|
||||
.with_config(move |config| {
|
||||
config.model = model;
|
||||
config.model_provider.model_catalog_url = Some(catalog_url.into());
|
||||
config
|
||||
.features
|
||||
.enable(Feature::ApiKeyModelDiscovery)
|
||||
.expect("enable API-key discovery");
|
||||
})
|
||||
.build_with_auto_env(&server)
|
||||
.await;
|
||||
if let Some(model) = configured_model {
|
||||
let test = result?;
|
||||
test.submit_turn("hello").await?;
|
||||
assert_eq!(response.single_request().body_json()["model"], model);
|
||||
} else {
|
||||
let error = result.err().expect("startup requires a model");
|
||||
assert_eq!(
|
||||
error.to_string(),
|
||||
"No models are available. Set `model` explicitly or check your model catalog configuration."
|
||||
);
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn model_catalog_url_supplies_conversation_model_and_instructions() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
use std::collections::HashSet;
|
||||
use std::fmt;
|
||||
use std::future::Future;
|
||||
use std::pin::Pin;
|
||||
@@ -181,6 +182,17 @@ impl OpenAiModelsEndpoint {
|
||||
})
|
||||
.await
|
||||
.map_err(|_| CodexErr::RequestTimeout)??;
|
||||
if self.provider_info.model_catalog_url.is_some() {
|
||||
let mut slugs = HashSet::new();
|
||||
if models
|
||||
.iter()
|
||||
.any(|model| model.slug.trim().is_empty() || !slugs.insert(&model.slug))
|
||||
{
|
||||
return Err(CodexErr::InvalidRequest(
|
||||
"model catalog must contain unique, non-empty model slugs".to_string(),
|
||||
));
|
||||
}
|
||||
}
|
||||
Ok(ModelsEndpointResponse {
|
||||
models,
|
||||
etag,
|
||||
@@ -397,6 +409,7 @@ mod tests {
|
||||
use codex_models_manager::manager::ModelsManager;
|
||||
use codex_models_manager::manager::OpenAiModelsManager;
|
||||
use codex_models_manager::manager::RefreshStrategy;
|
||||
use codex_models_manager::model_info::model_info_from_slug;
|
||||
use codex_protocol::auth::AuthMode;
|
||||
use codex_protocol::config_types::ModelProviderAuthInfo;
|
||||
use codex_protocol::error::CodexErrorDetails;
|
||||
@@ -701,7 +714,7 @@ mod tests {
|
||||
.unwrap();
|
||||
let model = codex_protocol::openai_models::ModelInfo {
|
||||
used_fallback_model_metadata: false,
|
||||
..codex_models_manager::model_info::model_info_from_slug("command-auth-model")
|
||||
..model_info_from_slug("command-auth-model")
|
||||
};
|
||||
Mock::given(method("GET"))
|
||||
.and(path("/models"))
|
||||
@@ -784,8 +797,7 @@ mod tests {
|
||||
CodexAuth::from_api_key("test-key"),
|
||||
] {
|
||||
let server = MockServer::start().await;
|
||||
let mut model =
|
||||
codex_models_manager::model_info::model_info_from_slug("provider-model");
|
||||
let mut model = model_info_from_slug("provider-model");
|
||||
model.visibility = ModelVisibility::List;
|
||||
model.supported_in_api = true;
|
||||
model.used_fallback_model_metadata = false;
|
||||
@@ -834,7 +846,8 @@ mod tests {
|
||||
home.path().to_path_buf(),
|
||||
endpoint,
|
||||
Some(auth_manager.clone()),
|
||||
);
|
||||
)
|
||||
.with_provider_catalog();
|
||||
// SIWC does not require the API-key rollout flag.
|
||||
manager.set_api_key_model_discovery_enabled(
|
||||
auth_manager.auth_mode() == Some(codex_protocol::auth::AuthMode::ApiKey),
|
||||
@@ -853,6 +866,41 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn explicit_catalog_requires_unique_nonempty_slugs() {
|
||||
for slugs in [vec![" \t"], vec!["private-model", "private-model"]] {
|
||||
let server = MockServer::start().await;
|
||||
let catalog = ModelsResponse {
|
||||
models: slugs.into_iter().map(model_info_from_slug).collect(),
|
||||
};
|
||||
Mock::given(method("GET"))
|
||||
.and(path("/codex/models"))
|
||||
.respond_with(ResponseTemplate::new(/*s*/ 200).set_body_json(catalog))
|
||||
.expect(1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
let endpoint = OpenAiModelsEndpoint::new(
|
||||
ModelProviderInfo {
|
||||
model_catalog_url: Some(format!("{}/codex/models", server.uri()).into()),
|
||||
..ModelProviderInfo::default()
|
||||
},
|
||||
/*auth_manager*/ None,
|
||||
/*gateway_auth_manager*/ None,
|
||||
);
|
||||
let error = endpoint
|
||||
.list_models(
|
||||
"1.2.3",
|
||||
HttpClientFactory::new(OutboundProxyPolicy::ReqwestDefault),
|
||||
)
|
||||
.await
|
||||
.expect_err("ambiguous model identity must be rejected");
|
||||
assert_eq!(
|
||||
error.to_string(),
|
||||
"model catalog must contain unique, non-empty model slugs"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn explicit_catalog_rejects_oversized_response() {
|
||||
let server = MockServer::start().await;
|
||||
|
||||
@@ -399,6 +399,12 @@ struct ConfiguredModelProvider {
|
||||
gateway_auth_manager: Option<Result<Arc<GatewayAuthManager>, String>>,
|
||||
}
|
||||
|
||||
enum ModelsCacheConfig {
|
||||
Disk { codex_home: PathBuf },
|
||||
Disabled,
|
||||
Custom(Arc<dyn ModelsCache>),
|
||||
}
|
||||
|
||||
impl ConfiguredModelProvider {
|
||||
fn new(
|
||||
info: ModelProviderInfo,
|
||||
@@ -411,6 +417,40 @@ impl ConfiguredModelProvider {
|
||||
gateway_auth_manager,
|
||||
}
|
||||
}
|
||||
|
||||
fn create_models_manager(
|
||||
&self,
|
||||
config_model_catalog: Option<ModelsResponse>,
|
||||
cache: ModelsCacheConfig,
|
||||
) -> SharedModelsManager {
|
||||
if let Some(model_catalog) = config_model_catalog {
|
||||
return Arc::new(StaticModelsManager::new(
|
||||
self.auth_manager.clone(),
|
||||
model_catalog,
|
||||
));
|
||||
}
|
||||
let endpoint = Arc::new(OpenAiModelsEndpoint::new(
|
||||
self.info.clone(),
|
||||
self.auth_manager.clone(),
|
||||
self.gateway_auth_manager.clone(),
|
||||
));
|
||||
let auth_manager = self.auth_manager.clone();
|
||||
let manager = match cache {
|
||||
ModelsCacheConfig::Disk { codex_home } => {
|
||||
OpenAiModelsManager::new(codex_home, endpoint, auth_manager)
|
||||
}
|
||||
ModelsCacheConfig::Disabled => {
|
||||
OpenAiModelsManager::new_without_cache(endpoint, auth_manager)
|
||||
}
|
||||
ModelsCacheConfig::Custom(cache) => {
|
||||
OpenAiModelsManager::new_with_cache(cache, endpoint, auth_manager)
|
||||
}
|
||||
};
|
||||
match &self.info.model_catalog_url {
|
||||
Some(_) => Arc::new(manager.with_provider_catalog()),
|
||||
None => Arc::new(manager),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl ModelProvider for ConfiguredModelProvider {
|
||||
@@ -559,47 +599,14 @@ impl ModelProvider for ConfiguredModelProvider {
|
||||
codex_home: PathBuf,
|
||||
config_model_catalog: Option<ModelsResponse>,
|
||||
) -> SharedModelsManager {
|
||||
match config_model_catalog {
|
||||
Some(model_catalog) => Arc::new(StaticModelsManager::new(
|
||||
self.auth_manager.clone(),
|
||||
model_catalog,
|
||||
)),
|
||||
None => {
|
||||
let endpoint = Arc::new(OpenAiModelsEndpoint::new(
|
||||
self.info.clone(),
|
||||
self.auth_manager.clone(),
|
||||
self.gateway_auth_manager.clone(),
|
||||
));
|
||||
Arc::new(OpenAiModelsManager::new(
|
||||
codex_home,
|
||||
endpoint,
|
||||
self.auth_manager.clone(),
|
||||
))
|
||||
}
|
||||
}
|
||||
self.create_models_manager(config_model_catalog, ModelsCacheConfig::Disk { codex_home })
|
||||
}
|
||||
|
||||
fn models_manager_without_cache(
|
||||
&self,
|
||||
config_model_catalog: Option<ModelsResponse>,
|
||||
) -> SharedModelsManager {
|
||||
match config_model_catalog {
|
||||
Some(model_catalog) => Arc::new(StaticModelsManager::new(
|
||||
self.auth_manager.clone(),
|
||||
model_catalog,
|
||||
)),
|
||||
None => {
|
||||
let endpoint = Arc::new(OpenAiModelsEndpoint::new(
|
||||
self.info.clone(),
|
||||
self.auth_manager.clone(),
|
||||
self.gateway_auth_manager.clone(),
|
||||
));
|
||||
Arc::new(OpenAiModelsManager::new_without_cache(
|
||||
endpoint,
|
||||
self.auth_manager.clone(),
|
||||
))
|
||||
}
|
||||
}
|
||||
self.create_models_manager(config_model_catalog, ModelsCacheConfig::Disabled)
|
||||
}
|
||||
|
||||
fn models_manager_with_cache(
|
||||
@@ -607,24 +614,7 @@ impl ModelProvider for ConfiguredModelProvider {
|
||||
config_model_catalog: Option<ModelsResponse>,
|
||||
cache: Arc<dyn ModelsCache>,
|
||||
) -> SharedModelsManager {
|
||||
match config_model_catalog {
|
||||
Some(model_catalog) => Arc::new(StaticModelsManager::new(
|
||||
self.auth_manager.clone(),
|
||||
model_catalog,
|
||||
)),
|
||||
None => {
|
||||
let endpoint = Arc::new(OpenAiModelsEndpoint::new(
|
||||
self.info.clone(),
|
||||
self.auth_manager.clone(),
|
||||
self.gateway_auth_manager.clone(),
|
||||
));
|
||||
Arc::new(OpenAiModelsManager::new_with_cache(
|
||||
cache,
|
||||
endpoint,
|
||||
self.auth_manager.clone(),
|
||||
))
|
||||
}
|
||||
}
|
||||
self.create_models_manager(config_model_catalog, ModelsCacheConfig::Custom(cache))
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -12,11 +12,8 @@ async fn api_key_discovery_disabled_preserves_command_auth_discovery_and_merging
|
||||
)];
|
||||
let endpoint = Arc::new(TestModelsEndpoint {
|
||||
has_command_auth: true,
|
||||
uses_codex_backend: false,
|
||||
responses: Mutex::new(vec![models.clone()].into()),
|
||||
etag: None,
|
||||
fetch_count: AtomicUsize::new(0),
|
||||
observed_proxy_policy: Mutex::new(None),
|
||||
responses: Mutex::new(vec![Ok(models.clone())].into()),
|
||||
..TestModelsEndpoint::default()
|
||||
});
|
||||
let manager = OpenAiModelsManager::new_without_cache(
|
||||
endpoint.clone(),
|
||||
|
||||
@@ -64,6 +64,9 @@ pub type ModelsCacheFuture<'a, T> = Pin<Box<dyn Future<Output = T> + Send + 'a>>
|
||||
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
|
||||
pub struct ModelsCacheEntry {
|
||||
/// Time when the catalog was fetched or last revalidated with the models endpoint.
|
||||
///
|
||||
/// `DateTime::<Utc>::MIN_UTC` marks an invalidated catalog, both in memory and in storage.
|
||||
/// The manager rejects it even if the backend uses native TTL.
|
||||
pub fetched_at: DateTime<Utc>,
|
||||
/// Endpoint ETag used to revalidate the catalog without downloading an unchanged payload.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
|
||||
@@ -4,6 +4,7 @@ use crate::cache::ModelsCacheEntry;
|
||||
use crate::collaboration_mode_presets::builtin_collaboration_mode_presets;
|
||||
use crate::config::ModelsManagerConfig;
|
||||
use crate::model_info;
|
||||
use chrono::DateTime;
|
||||
use chrono::Utc;
|
||||
use codex_http_client::HttpClientFactory;
|
||||
use codex_login::AuthManager;
|
||||
@@ -22,6 +23,8 @@ use std::sync::Arc;
|
||||
use std::sync::atomic::AtomicBool;
|
||||
use std::sync::atomic::Ordering;
|
||||
use std::time::Duration;
|
||||
use tokio::sync::Mutex;
|
||||
use tokio::sync::MutexGuard;
|
||||
use tokio::sync::RwLock;
|
||||
use tokio::sync::TryLockError;
|
||||
use tracing::Instrument as _;
|
||||
@@ -145,7 +148,7 @@ pub trait ModelsManager: fmt::Debug + Send + Sync {
|
||||
) -> ModelsManagerFuture<'_, ModelsResponse>;
|
||||
|
||||
/// Best-effort refresh when the in-memory catalog belongs to different credentials.
|
||||
/// Static catalogs need no refresh. Failures leave the existing cache/default fallback.
|
||||
/// Static catalogs need no refresh. Failures follow the catalog's fallback policy.
|
||||
fn refresh_after_auth_change(
|
||||
&self,
|
||||
_http_client_factory: HttpClientFactory,
|
||||
@@ -255,12 +258,37 @@ pub type ModelsManagerFuture<'a, T> = Pin<Box<dyn Future<Output = T> + Send + 'a
|
||||
/// Shared model manager handle used across runtime services.
|
||||
pub type SharedModelsManager = Arc<dyn ModelsManager>;
|
||||
|
||||
/// Only explicit catalogs serialize refresh and suppress bundled fallback. Auth and discovery
|
||||
/// settings are still evaluated when models are requested.
|
||||
#[derive(Debug)]
|
||||
enum CatalogSource {
|
||||
Default,
|
||||
ExplicitProvider(Mutex<()>),
|
||||
}
|
||||
|
||||
impl CatalogSource {
|
||||
fn fallback_models(&self) -> Option<Vec<ModelInfo>> {
|
||||
match self {
|
||||
Self::Default => Some(load_remote_models_from_file().unwrap_or_default()),
|
||||
Self::ExplicitProvider(_) => None,
|
||||
}
|
||||
}
|
||||
|
||||
async fn refresh_lock(&self) -> Option<MutexGuard<'_, ()>> {
|
||||
match self {
|
||||
Self::Default => None,
|
||||
Self::ExplicitProvider(lock) => Some(lock.lock().await),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// OpenAI-compatible model manager backed by bundled models, cache, and `/models`.
|
||||
#[derive(Debug)]
|
||||
pub struct OpenAiModelsManager {
|
||||
remote_models: RwLock<ModelsCacheEntry>,
|
||||
cache: Option<Arc<dyn ModelsCache>>,
|
||||
endpoint_client: SharedModelsEndpointClient,
|
||||
catalog_source: CatalogSource,
|
||||
api_key_model_discovery_enabled: AtomicBool,
|
||||
auth_manager: Option<Arc<AuthManager>>,
|
||||
}
|
||||
@@ -315,21 +343,29 @@ impl OpenAiModelsManager {
|
||||
endpoint_client: Arc<dyn ModelsEndpointClient>,
|
||||
auth_manager: Option<Arc<AuthManager>>,
|
||||
) -> Self {
|
||||
let remote_models = load_remote_models_from_file().unwrap_or_default();
|
||||
Self {
|
||||
remote_models: RwLock::new(ModelsCacheEntry {
|
||||
fetched_at: Utc::now(),
|
||||
etag: None,
|
||||
client_version: Some(crate::client_version_to_whole()),
|
||||
identity: endpoint_client.identity(),
|
||||
models: remote_models,
|
||||
models: load_remote_models_from_file().unwrap_or_default(),
|
||||
}),
|
||||
cache,
|
||||
api_key_model_discovery_enabled: AtomicBool::new(false),
|
||||
endpoint_client,
|
||||
catalog_source: CatalogSource::Default,
|
||||
auth_manager,
|
||||
}
|
||||
}
|
||||
|
||||
/// Configure a newly constructed manager to use only the provider's explicit catalog.
|
||||
/// Clear the bundled seed before sharing it; the existing auth identity must be retained.
|
||||
pub fn with_provider_catalog(mut self) -> Self {
|
||||
self.catalog_source = CatalogSource::ExplicitProvider(Mutex::new(()));
|
||||
self.remote_models.get_mut().models.clear();
|
||||
self
|
||||
}
|
||||
}
|
||||
|
||||
impl StaticModelsManager {
|
||||
@@ -396,7 +432,7 @@ impl ModelsManager for OpenAiModelsManager {
|
||||
if entry.identity.is_some() && entry.identity == self.endpoint_client.identity() {
|
||||
entry.models.clone()
|
||||
} else {
|
||||
load_remote_models_from_file().unwrap_or_default()
|
||||
self.catalog_source.fallback_models().unwrap_or_default()
|
||||
}
|
||||
})
|
||||
}
|
||||
@@ -407,7 +443,7 @@ impl ModelsManager for OpenAiModelsManager {
|
||||
if entry.identity.is_some() && entry.identity == self.endpoint_client.identity() {
|
||||
entry.models.clone()
|
||||
} else {
|
||||
load_remote_models_from_file().unwrap_or_default()
|
||||
self.catalog_source.fallback_models().unwrap_or_default()
|
||||
},
|
||||
)
|
||||
}
|
||||
@@ -416,6 +452,27 @@ impl ModelsManager for OpenAiModelsManager {
|
||||
self.auth_manager.as_deref()
|
||||
}
|
||||
|
||||
#[tracing::instrument(skip_all, fields(model))]
|
||||
fn get_model_info<'a>(
|
||||
&'a self,
|
||||
model: &'a str,
|
||||
config: &'a ModelsManagerConfig,
|
||||
) -> ModelsManagerFuture<'a, ModelInfo> {
|
||||
Box::pin(async move {
|
||||
let models = self.get_remote_models().await;
|
||||
match &self.catalog_source {
|
||||
CatalogSource::Default => {
|
||||
construct_model_info_from_candidates(model, &models, config)
|
||||
}
|
||||
CatalogSource::ExplicitProvider(_) => construct_model_info(
|
||||
model,
|
||||
models.into_iter().find(|candidate| candidate.slug == model),
|
||||
config,
|
||||
),
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
fn list_collaboration_modes(&self) -> Vec<CollaborationModeMask> {
|
||||
builtin_collaboration_mode_presets()
|
||||
}
|
||||
@@ -451,6 +508,7 @@ impl OpenAiModelsManager {
|
||||
}
|
||||
|
||||
async fn refresh_if_new_etag(&self, etag: String, http_client_factory: HttpClientFactory) {
|
||||
let refresh_guard = self.catalog_source.refresh_lock().await;
|
||||
let (identity, current_etag) = {
|
||||
let entry = self.remote_models.read().await;
|
||||
(entry.identity.clone(), entry.etag.clone())
|
||||
@@ -468,6 +526,7 @@ impl OpenAiModelsManager {
|
||||
}
|
||||
return;
|
||||
}
|
||||
drop(refresh_guard);
|
||||
if let Err(err) = self
|
||||
.refresh_available_models(RefreshStrategy::Online, &http_client_factory)
|
||||
.await
|
||||
@@ -482,6 +541,7 @@ impl OpenAiModelsManager {
|
||||
refresh_strategy: RefreshStrategy,
|
||||
http_client_factory: &HttpClientFactory,
|
||||
) -> CoreResult<()> {
|
||||
let _refresh_guard = self.catalog_source.refresh_lock().await;
|
||||
// API-key discovery must be enabled and supported before reusing a remote catalog.
|
||||
// Otherwise even a matching cache from an earlier run would bypass bundled-only behavior.
|
||||
// Command-auth providers retain their existing discovery behavior.
|
||||
@@ -521,14 +581,54 @@ impl OpenAiModelsManager {
|
||||
http_client_factory: &HttpClientFactory,
|
||||
) -> CoreResult<()> {
|
||||
let client_version = crate::client_version_to_whole();
|
||||
let request_identity = self.endpoint_client.identity();
|
||||
let ModelsEndpointResponse {
|
||||
models,
|
||||
etag,
|
||||
identity,
|
||||
} = self
|
||||
} = match self
|
||||
.endpoint_client
|
||||
.list_models(&client_version, http_client_factory.clone())
|
||||
.await?;
|
||||
.await
|
||||
{
|
||||
Ok(response) => response,
|
||||
Err(error) => {
|
||||
if matches!(&self.catalog_source, CatalogSource::ExplicitProvider(_)) {
|
||||
{
|
||||
let mut current = self.remote_models.write().await;
|
||||
if request_identity != self.endpoint_client.identity() {
|
||||
return Err(error);
|
||||
}
|
||||
// Keep this failure in memory even if persistent invalidation fails.
|
||||
current.fetched_at = DateTime::<Utc>::MIN_UTC;
|
||||
current.models.clear();
|
||||
current.etag = None;
|
||||
current.identity.clone_from(&request_identity);
|
||||
}
|
||||
if let Some(identity) = request_identity
|
||||
&& let Some(cache) = self.cache.as_ref()
|
||||
{
|
||||
let invalidation = async {
|
||||
if let Some(mut entry) = cache.load(&client_version).await?
|
||||
&& entry.identity.as_deref() == Some(&identity)
|
||||
&& entry.client_version.as_deref() == Some(&client_version)
|
||||
{
|
||||
// Preserve backend lookup keys, but force a cache miss after restart.
|
||||
entry.fetched_at = DateTime::<Utc>::MIN_UTC;
|
||||
entry.etag = None;
|
||||
entry.models.clear();
|
||||
cache.store(&entry).await?;
|
||||
}
|
||||
Ok::<_, crate::cache::ModelsCacheError>(())
|
||||
};
|
||||
if let Err(err) = invalidation.await {
|
||||
error!("failed to invalidate models cache: {err}");
|
||||
}
|
||||
}
|
||||
}
|
||||
return Err(error);
|
||||
}
|
||||
};
|
||||
if Some(&identity) != self.endpoint_client.identity().as_ref() {
|
||||
return Ok(());
|
||||
}
|
||||
@@ -574,7 +674,6 @@ impl OpenAiModelsManager {
|
||||
if entry.identity != self.endpoint_client.identity() {
|
||||
return false;
|
||||
}
|
||||
// Visible ChatGPT and OpenAI API-key catalogs are authoritative.
|
||||
let remote_only = entry
|
||||
.models
|
||||
.iter()
|
||||
@@ -585,8 +684,7 @@ impl OpenAiModelsManager {
|
||||
.auth_mode()
|
||||
.is_some_and(AuthMode::has_chatgpt_account)
|
||||
}));
|
||||
if !remote_only {
|
||||
let mut models = load_remote_models_from_file().unwrap_or_default();
|
||||
if !remote_only && let Some(mut models) = self.catalog_source.fallback_models() {
|
||||
for model in entry.models {
|
||||
if let Some(index) = models
|
||||
.iter()
|
||||
@@ -615,6 +713,15 @@ impl OpenAiModelsManager {
|
||||
let Some(identity) = self.endpoint_client.identity() else {
|
||||
return false;
|
||||
};
|
||||
{
|
||||
let current = self.remote_models.read().await;
|
||||
// A failed fetch must not be replaced by stale cache data even if invalidation failed.
|
||||
if current.fetched_at == DateTime::<Utc>::MIN_UTC
|
||||
&& current.identity.as_ref() == Some(&identity)
|
||||
{
|
||||
return false;
|
||||
}
|
||||
}
|
||||
let cache_entry = match cache.load(&client_version).await {
|
||||
Ok(Some(cache_entry)) => cache_entry,
|
||||
Ok(None) => {
|
||||
@@ -626,6 +733,9 @@ impl OpenAiModelsManager {
|
||||
return false;
|
||||
}
|
||||
};
|
||||
if cache_entry.fetched_at == DateTime::<Utc>::MIN_UTC {
|
||||
return false;
|
||||
}
|
||||
if cache_entry.client_version.as_deref() != Some(client_version.as_str()) {
|
||||
info!(
|
||||
expected_version = client_version,
|
||||
@@ -788,6 +898,14 @@ pub(crate) fn construct_model_info_from_candidates(
|
||||
// retry for namespaced slugs like `custom/gpt-5.3-codex`.
|
||||
let remote = find_model_by_longest_prefix(model, candidates)
|
||||
.or_else(|| find_model_by_namespaced_suffix(model, candidates));
|
||||
construct_model_info(model, remote, config)
|
||||
}
|
||||
|
||||
fn construct_model_info(
|
||||
model: &str,
|
||||
remote: Option<ModelInfo>,
|
||||
config: &ModelsManagerConfig,
|
||||
) -> ModelInfo {
|
||||
let model_info = if let Some(remote) = remote {
|
||||
ModelInfo {
|
||||
slug: model.to_string(),
|
||||
|
||||
@@ -16,6 +16,7 @@ use codex_login::ExternalAuth;
|
||||
use codex_login::ExternalAuthRefreshContext;
|
||||
use codex_login::TokenData;
|
||||
use codex_protocol::auth::AuthMode;
|
||||
use codex_protocol::error::CodexErr;
|
||||
use codex_protocol::openai_models::ModelAccessPrograms;
|
||||
use codex_protocol::openai_models::ModelsResponse;
|
||||
use codex_protocol::turn_input::CyberAccessProgram;
|
||||
@@ -92,17 +93,17 @@ fn assert_models_contain(actual: &[ModelInfo], expected: &[ModelInfo]) {
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug)]
|
||||
#[derive(Debug, Default)]
|
||||
struct TestModelsEndpoint {
|
||||
has_command_auth: bool,
|
||||
uses_codex_backend: bool,
|
||||
responses: Mutex<VecDeque<Vec<ModelInfo>>>,
|
||||
responses: Mutex<VecDeque<CoreResult<Vec<ModelInfo>>>>,
|
||||
etag: Option<String>,
|
||||
fetch_count: AtomicUsize,
|
||||
observed_proxy_policy: Mutex<Option<OutboundProxyPolicy>>,
|
||||
}
|
||||
|
||||
#[derive(Debug)]
|
||||
#[derive(Debug, Default)]
|
||||
struct TestModelsCache {
|
||||
entry: Mutex<Option<ModelsCacheEntry>>,
|
||||
load_error: bool,
|
||||
@@ -162,6 +163,7 @@ impl ModelsCache for TestModelsCache {
|
||||
if self.store_error {
|
||||
return Err(ModelsCacheError::new("test store failure"));
|
||||
}
|
||||
*self.entry.lock().unwrap() = Some(entry.clone());
|
||||
self.stored_entries
|
||||
.lock()
|
||||
.expect("stored entries lock should not be poisoned")
|
||||
@@ -196,23 +198,24 @@ impl ModelsCache for TestModelsCache {
|
||||
impl TestModelsEndpoint {
|
||||
fn new(responses: Vec<Vec<ModelInfo>>) -> Arc<Self> {
|
||||
Arc::new(Self {
|
||||
has_command_auth: false,
|
||||
uses_codex_backend: true,
|
||||
responses: Mutex::new(responses.into()),
|
||||
etag: None,
|
||||
fetch_count: AtomicUsize::new(0),
|
||||
observed_proxy_policy: Mutex::new(None),
|
||||
responses: Mutex::new(responses.into_iter().map(Ok).collect()),
|
||||
..Self::default()
|
||||
})
|
||||
}
|
||||
|
||||
fn without_refresh(responses: Vec<Vec<ModelInfo>>) -> Arc<Self> {
|
||||
Arc::new(Self {
|
||||
has_command_auth: false,
|
||||
uses_codex_backend: false,
|
||||
responses: Mutex::new(responses.into_iter().map(Ok).collect()),
|
||||
..Self::default()
|
||||
})
|
||||
}
|
||||
|
||||
fn with_command_auth(responses: Vec<CoreResult<Vec<ModelInfo>>>) -> Arc<Self> {
|
||||
Arc::new(Self {
|
||||
has_command_auth: true,
|
||||
responses: Mutex::new(responses.into()),
|
||||
etag: None,
|
||||
fetch_count: AtomicUsize::new(0),
|
||||
observed_proxy_policy: Mutex::new(None),
|
||||
..Self::default()
|
||||
})
|
||||
}
|
||||
|
||||
@@ -234,7 +237,7 @@ impl TestModelsEndpoint {
|
||||
.lock()
|
||||
.expect("responses lock should not be poisoned")
|
||||
.pop_front()
|
||||
.unwrap_or_default();
|
||||
.unwrap_or_else(|| Ok(Vec::new()))?;
|
||||
Ok(ModelsEndpointResponse {
|
||||
models,
|
||||
etag: self.etag.clone(),
|
||||
@@ -1051,11 +1054,8 @@ async fn refresh_available_models_keeps_merging_for_custom_api_auth() {
|
||||
let codex_home = tempdir().expect("temp dir");
|
||||
let endpoint = Arc::new(TestModelsEndpoint {
|
||||
has_command_auth: true,
|
||||
uses_codex_backend: false,
|
||||
responses: Mutex::new(vec![remote_models.clone()].into()),
|
||||
etag: None,
|
||||
fetch_count: AtomicUsize::new(0),
|
||||
observed_proxy_policy: Mutex::new(None),
|
||||
responses: Mutex::new(vec![Ok(remote_models.clone())].into()),
|
||||
..TestModelsEndpoint::default()
|
||||
});
|
||||
let manager = openai_manager_for_tests_with_auth(
|
||||
codex_home.path().to_path_buf(),
|
||||
@@ -1079,6 +1079,86 @@ async fn refresh_available_models_keeps_merging_for_custom_api_auth() {
|
||||
assert_eq!(endpoint.fetch_count(), 1, "expected a single model fetch");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn authoritative_catalog_preserves_hidden_models_and_matches_exact_ids() {
|
||||
let model =
|
||||
remote_model_with_visibility("gpt-6-sol", "Provider Model", /*priority*/ 0, "hide");
|
||||
let endpoint = TestModelsEndpoint::with_command_auth(vec![Ok(vec![model.clone()])]);
|
||||
let manager = OpenAiModelsManager::new_without_cache(endpoint, /*auth_manager*/ None)
|
||||
.with_provider_catalog();
|
||||
assert_eq!(
|
||||
manager
|
||||
.raw_model_catalog(RefreshStrategy::Online, DEFAULT_HTTP_CLIENT_FACTORY)
|
||||
.await
|
||||
.models,
|
||||
vec![model]
|
||||
);
|
||||
for unknown in ["gpt-6-sol-other", "namespace/gpt-6-sol"] {
|
||||
assert_eq!(
|
||||
manager
|
||||
.get_model_info(unknown, &ModelsManagerConfig::default())
|
||||
.await,
|
||||
model_info::model_info_from_slug(unknown)
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn authoritative_catalog_failure_invalidates_cache_until_refresh_succeeds() {
|
||||
// Like a native TTL backend, this cache may return an invalidated stored entry.
|
||||
let cache = Arc::new(TestModelsCache::default());
|
||||
let models = vec![remote_model(
|
||||
"provider-model",
|
||||
"Provider Model",
|
||||
/*priority*/ 0,
|
||||
)];
|
||||
let endpoint = TestModelsEndpoint::with_command_auth(vec![
|
||||
Ok(models.clone()),
|
||||
Err(CodexErr::RequestTimeout),
|
||||
Ok(models.clone()),
|
||||
]);
|
||||
let manager = OpenAiModelsManager::new_with_cache(
|
||||
cache.clone(),
|
||||
endpoint.clone(),
|
||||
/*auth_manager*/ None,
|
||||
)
|
||||
.with_provider_catalog();
|
||||
assert_eq!(
|
||||
manager
|
||||
.raw_model_catalog(RefreshStrategy::Online, DEFAULT_HTTP_CLIENT_FACTORY)
|
||||
.await
|
||||
.models,
|
||||
models
|
||||
);
|
||||
for strategy in [RefreshStrategy::Online, RefreshStrategy::Offline] {
|
||||
assert_eq!(
|
||||
manager
|
||||
.raw_model_catalog(strategy, DEFAULT_HTTP_CLIENT_FACTORY)
|
||||
.await,
|
||||
ModelsResponse::default()
|
||||
);
|
||||
}
|
||||
let restarted =
|
||||
OpenAiModelsManager::new_with_cache(cache, endpoint, /*auth_manager*/ None)
|
||||
.with_provider_catalog();
|
||||
assert_eq!(
|
||||
restarted
|
||||
.raw_model_catalog(RefreshStrategy::Offline, DEFAULT_HTTP_CLIENT_FACTORY)
|
||||
.await,
|
||||
ModelsResponse::default()
|
||||
);
|
||||
assert_eq!(
|
||||
restarted
|
||||
.raw_model_catalog(
|
||||
RefreshStrategy::OnlineIfUncached,
|
||||
DEFAULT_HTTP_CLIENT_FACTORY,
|
||||
)
|
||||
.await
|
||||
.models,
|
||||
models
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn refresh_available_models_uses_cache_when_fresh() {
|
||||
let remote_models = vec![remote_model("cached", "Cached", /*priority*/ 5)];
|
||||
@@ -1129,12 +1209,10 @@ async fn online_refresh_updates_access_programs_with_unchanged_etag() {
|
||||
};
|
||||
let responses = vec![vec![granted_model.clone()], vec![revoked_model.clone()]];
|
||||
let endpoint = Arc::new(TestModelsEndpoint {
|
||||
has_command_auth: false,
|
||||
uses_codex_backend: true,
|
||||
responses: Mutex::new(responses.into()),
|
||||
responses: Mutex::new(responses.into_iter().map(Ok).collect()),
|
||||
etag: Some("stable-catalog-etag".to_string()),
|
||||
fetch_count: AtomicUsize::new(0),
|
||||
observed_proxy_policy: Mutex::new(None),
|
||||
..TestModelsEndpoint::default()
|
||||
});
|
||||
let manager = openai_manager_for_tests(codex_home.path().to_path_buf(), endpoint.clone());
|
||||
|
||||
|
||||
@@ -649,7 +649,7 @@ impl AppServerSession {
|
||||
.map(|model| model.model.clone())
|
||||
})
|
||||
.or_else(|| available_models.first().map(|model| model.model.clone()))
|
||||
.wrap_err("model/list returned no models for TUI bootstrap")?;
|
||||
.wrap_err("No models are available. Set `model` explicitly or check your model catalog configuration.")?;
|
||||
self.default_model = Some(default_model.clone());
|
||||
self.available_models = available_models.clone();
|
||||
|
||||
@@ -2580,6 +2580,25 @@ mod tests {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn bootstrap_empty_catalog_reports_configuration_error() -> Result<()> {
|
||||
let codex_home = tempfile::tempdir()?;
|
||||
let mut config = build_config(&codex_home).await;
|
||||
config.model = None;
|
||||
config.model_catalog = Some(codex_protocol::openai_models::ModelsResponse::default());
|
||||
let mut app_server = crate::start_embedded_app_server_for_picker(&config).await?;
|
||||
|
||||
let error = app_server
|
||||
.bootstrap(&config)
|
||||
.await
|
||||
.err()
|
||||
.expect("bootstrap requires a model");
|
||||
insta::assert_snapshot!(error.to_string(), @"No models are available. Set `model` explicitly or check your model catalog configuration.");
|
||||
|
||||
app_server.shutdown().await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn rate_limit_snapshot(limit_id: &str) -> RateLimitSnapshot {
|
||||
RateLimitSnapshot {
|
||||
limit_id: Some(limit_id.to_string()),
|
||||
|
||||
Reference in New Issue
Block a user