mirror of
https://github.com/openai/codex.git
synced 2026-11-06 20:20:34 +00:00
show user warning when using default fallback metadata, as it can break things
This commit is contained in:
@@ -1628,6 +1628,34 @@ impl Session {
|
||||
state.set_previous_model(previous_model);
|
||||
}
|
||||
|
||||
async fn should_emit_unknown_model_warning(&self, model_slug: &str) -> bool {
|
||||
let mut state = self.state.lock().await;
|
||||
state.mark_unknown_model_warning_emitted(model_slug)
|
||||
}
|
||||
|
||||
async fn maybe_warn_for_unknown_model(
|
||||
&self,
|
||||
turn_context: &TurnContext,
|
||||
model_slug: &str,
|
||||
used_fallback_model_metadata: bool,
|
||||
) {
|
||||
if !used_fallback_model_metadata
|
||||
|| !self.should_emit_unknown_model_warning(model_slug).await
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
self.send_event(
|
||||
turn_context,
|
||||
EventMsg::Warning(WarningEvent {
|
||||
message: format!(
|
||||
"Defaulting to fallback model metadata for `{model_slug}`. This can degrade performance and cause issues."
|
||||
),
|
||||
}),
|
||||
)
|
||||
.await;
|
||||
}
|
||||
|
||||
fn maybe_refresh_shell_snapshot_for_cwd(
|
||||
&self,
|
||||
previous_cwd: &Path,
|
||||
@@ -1754,13 +1782,11 @@ impl Session {
|
||||
}
|
||||
}
|
||||
|
||||
let model_info = self
|
||||
let resolved_model_slug = session_configuration.collaboration_mode.model().to_string();
|
||||
let model_resolution = self
|
||||
.services
|
||||
.models_manager
|
||||
.get_model_info(
|
||||
session_configuration.collaboration_mode.model(),
|
||||
&per_turn_config,
|
||||
)
|
||||
.get_model_info_resolution(resolved_model_slug.as_str(), &per_turn_config)
|
||||
.await;
|
||||
let mut turn_context: TurnContext = Self::make_turn_context(
|
||||
Some(Arc::clone(&self.services.auth_manager)),
|
||||
@@ -1768,7 +1794,7 @@ impl Session {
|
||||
session_configuration.provider.clone(),
|
||||
&session_configuration,
|
||||
per_turn_config,
|
||||
model_info,
|
||||
model_resolution.model_info,
|
||||
self.services
|
||||
.network_proxy
|
||||
.as_ref()
|
||||
@@ -1781,6 +1807,12 @@ impl Session {
|
||||
turn_context.final_output_json_schema = final_schema;
|
||||
}
|
||||
let turn_context = Arc::new(turn_context);
|
||||
self.maybe_warn_for_unknown_model(
|
||||
turn_context.as_ref(),
|
||||
resolved_model_slug.as_str(),
|
||||
model_resolution.used_fallback_model_metadata,
|
||||
)
|
||||
.await;
|
||||
turn_context.spawn_turn_metadata_header_task();
|
||||
turn_context
|
||||
}
|
||||
|
||||
@@ -43,6 +43,13 @@ pub enum RefreshStrategy {
|
||||
OnlineIfUncached,
|
||||
}
|
||||
|
||||
/// Result of resolving model metadata for a requested model slug.
|
||||
#[derive(Debug, Clone)]
|
||||
pub(crate) struct ModelInfoResolution {
|
||||
pub(crate) model_info: ModelInfo,
|
||||
pub(crate) used_fallback_model_metadata: bool,
|
||||
}
|
||||
|
||||
/// Coordinates remote model discovery plus cached metadata on disk.
|
||||
#[derive(Debug)]
|
||||
pub struct ModelsManager {
|
||||
@@ -137,18 +144,35 @@ impl ModelsManager {
|
||||
// todo(aibrahim): look if we can tighten it to pub(crate)
|
||||
/// Look up model metadata, applying remote overrides and config adjustments.
|
||||
pub async fn get_model_info(&self, model: &str, config: &Config) -> ModelInfo {
|
||||
self.get_model_info_resolution(model, config)
|
||||
.await
|
||||
.model_info
|
||||
}
|
||||
|
||||
/// Look up model metadata, returning whether fallback metadata was used.
|
||||
pub(crate) async fn get_model_info_resolution(
|
||||
&self,
|
||||
model: &str,
|
||||
config: &Config,
|
||||
) -> ModelInfoResolution {
|
||||
let remote = self
|
||||
.find_remote_model_by_longest_prefix(model, config)
|
||||
.await;
|
||||
let model = if let Some(remote) = remote {
|
||||
ModelInfo {
|
||||
slug: model.to_string(),
|
||||
..remote
|
||||
}
|
||||
let (model_info, used_fallback_model_metadata) = if let Some(remote) = remote {
|
||||
(
|
||||
ModelInfo {
|
||||
slug: model.to_string(),
|
||||
..remote
|
||||
},
|
||||
false,
|
||||
)
|
||||
} else {
|
||||
model_info::model_info_from_slug(model)
|
||||
(model_info::model_info_from_slug(model), true)
|
||||
};
|
||||
model_info::with_config_overrides(model, config)
|
||||
ModelInfoResolution {
|
||||
model_info: model_info::with_config_overrides(model_info, config),
|
||||
used_fallback_model_metadata,
|
||||
}
|
||||
}
|
||||
|
||||
async fn find_remote_model_by_longest_prefix(
|
||||
@@ -472,6 +496,39 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn get_model_info_resolution_tracks_fallback_usage() {
|
||||
let codex_home = tempdir().expect("temp dir");
|
||||
let mut config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.build()
|
||||
.await
|
||||
.expect("load default test config");
|
||||
config.features.enable(Feature::RemoteModels);
|
||||
let auth_manager =
|
||||
AuthManager::from_auth_for_testing(CodexAuth::from_api_key("Test API Key"));
|
||||
let manager = ModelsManager::new(codex_home.path().to_path_buf(), auth_manager);
|
||||
let known_slug = manager
|
||||
.get_remote_models(&config)
|
||||
.await
|
||||
.first()
|
||||
.expect("bundled models should include at least one model")
|
||||
.slug
|
||||
.clone();
|
||||
|
||||
let known = manager
|
||||
.get_model_info_resolution(known_slug.as_str(), &config)
|
||||
.await;
|
||||
assert!(!known.used_fallback_model_metadata);
|
||||
assert_eq!(known.model_info.slug, known_slug);
|
||||
|
||||
let unknown = manager
|
||||
.get_model_info_resolution("model-that-does-not-exist", &config)
|
||||
.await;
|
||||
assert!(unknown.used_fallback_model_metadata);
|
||||
assert_eq!(unknown.model_info.slug, "model-that-does-not-exist");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn refresh_available_models_sorts_by_priority() {
|
||||
let server = MockServer::start().await;
|
||||
|
||||
@@ -31,6 +31,7 @@ pub(crate) struct SessionState {
|
||||
pub(crate) startup_regular_task: Option<RegularTask>,
|
||||
pub(crate) active_mcp_tool_selection: Option<Vec<String>>,
|
||||
pub(crate) active_connector_selection: HashSet<String>,
|
||||
warned_unknown_models: HashSet<String>,
|
||||
}
|
||||
|
||||
impl SessionState {
|
||||
@@ -49,6 +50,7 @@ impl SessionState {
|
||||
startup_regular_task: None,
|
||||
active_mcp_tool_selection: None,
|
||||
active_connector_selection: HashSet::new(),
|
||||
warned_unknown_models: HashSet::new(),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -196,6 +198,13 @@ impl SessionState {
|
||||
pub(crate) fn clear_connector_selection(&mut self) {
|
||||
self.active_connector_selection.clear();
|
||||
}
|
||||
|
||||
/// Marks an unknown model warning as emitted for the provided slug.
|
||||
///
|
||||
/// Returns `true` only the first time a slug is marked during this session.
|
||||
pub(crate) fn mark_unknown_model_warning_emitted(&mut self, model_slug: &str) -> bool {
|
||||
self.warned_unknown_models.insert(model_slug.to_string())
|
||||
}
|
||||
}
|
||||
|
||||
// Sometimes new snapshots don't include credits or plan information.
|
||||
@@ -322,6 +331,16 @@ mod tests {
|
||||
assert_eq!(state.get_connector_selection(), HashSet::new());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn unknown_model_warning_tracking_deduplicates_by_slug() {
|
||||
let session_configuration = make_session_configuration_for_tests().await;
|
||||
let mut state = SessionState::new(session_configuration);
|
||||
|
||||
assert!(state.mark_unknown_model_warning_emitted("gpt-alpha"));
|
||||
assert!(!state.mark_unknown_model_warning_emitted("gpt-alpha"));
|
||||
assert!(state.mark_unknown_model_warning_emitted("gpt-beta"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn set_rate_limits_defaults_limit_id_to_codex_when_missing() {
|
||||
let session_configuration = make_session_configuration_for_tests().await;
|
||||
|
||||
Reference in New Issue
Block a user