fix: show user warning when using default fallback metadata (#11690)
### What It's currently unclear when the harness falls back to the default, generic `ModelInfo`. This happens when the `remote_models` feature is disabled or the model is truly unknown, and can lead to bad performance and issues in the harness. Add a user-facing warning when this happens so they are aware when their setup is broken. ### Tests Added tests, tested locally.
This commit is contained in:
parent
85034b189e
commit
060a320e7d
13 changed files with 76 additions and 8 deletions
|
|
@ -41,6 +41,7 @@ fn preset_to_info(preset: &ModelPreset, priority: i32) -> ModelInfo {
|
|||
experimental_supported_tools: Vec::new(),
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -89,6 +89,7 @@ async fn models_client_hits_models_endpoint() {
|
|||
experimental_supported_tools: Vec::new(),
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
}],
|
||||
};
|
||||
|
||||
|
|
|
|||
|
|
@ -1780,13 +1780,11 @@ impl Session {
|
|||
}
|
||||
}
|
||||
|
||||
let resolved_model_slug = session_configuration.collaboration_mode.model().to_string();
|
||||
let model_info = self
|
||||
.services
|
||||
.models_manager
|
||||
.get_model_info(
|
||||
session_configuration.collaboration_mode.model(),
|
||||
&per_turn_config,
|
||||
)
|
||||
.get_model_info(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)),
|
||||
|
|
@ -1794,7 +1792,7 @@ impl Session {
|
|||
session_configuration.provider.clone(),
|
||||
&session_configuration,
|
||||
per_turn_config,
|
||||
model_info,
|
||||
model_info.clone(),
|
||||
self.services
|
||||
.network_proxy
|
||||
.as_ref()
|
||||
|
|
@ -1807,6 +1805,17 @@ impl Session {
|
|||
turn_context.final_output_json_schema = final_schema;
|
||||
}
|
||||
let turn_context = Arc::new(turn_context);
|
||||
if model_info.used_fallback_model_metadata {
|
||||
self.send_event(
|
||||
turn_context.as_ref(),
|
||||
EventMsg::Warning(WarningEvent {
|
||||
message: format!(
|
||||
"Model metadata for `{resolved_model_slug}` not found. Defaulting to fallback metadata; this can degrade performance and cause issues."
|
||||
),
|
||||
}),
|
||||
)
|
||||
.await;
|
||||
}
|
||||
turn_context.turn_metadata_state.spawn_git_enrichment_task();
|
||||
turn_context
|
||||
}
|
||||
|
|
|
|||
|
|
@ -140,15 +140,16 @@ impl ModelsManager {
|
|||
let remote = self
|
||||
.find_remote_model_by_longest_prefix(model, config)
|
||||
.await;
|
||||
let model = if let Some(remote) = remote {
|
||||
let model_info = if let Some(remote) = remote {
|
||||
ModelInfo {
|
||||
slug: model.to_string(),
|
||||
used_fallback_model_metadata: false,
|
||||
..remote
|
||||
}
|
||||
} else {
|
||||
model_info::model_info_from_slug(model)
|
||||
};
|
||||
model_info::with_config_overrides(model, config)
|
||||
model_info::with_config_overrides(model_info, config)
|
||||
}
|
||||
|
||||
async fn find_remote_model_by_longest_prefix(
|
||||
|
|
@ -472,6 +473,37 @@ mod tests {
|
|||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn get_model_info_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(known_slug.as_str(), &config).await;
|
||||
assert!(!known.used_fallback_model_metadata);
|
||||
assert_eq!(known.slug, known_slug);
|
||||
|
||||
let unknown = manager
|
||||
.get_model_info("model-that-does-not-exist", &config)
|
||||
.await;
|
||||
assert!(unknown.used_fallback_model_metadata);
|
||||
assert_eq!(unknown.slug, "model-that-does-not-exist");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn refresh_available_models_sorts_by_priority() {
|
||||
let server = MockServer::start().await;
|
||||
|
|
|
|||
|
|
@ -81,6 +81,7 @@ pub(crate) fn model_info_from_slug(slug: &str) -> ModelInfo {
|
|||
experimental_supported_tools: Vec::new(),
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: true, // this is the fallback model metadata
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -228,6 +228,7 @@ async fn model_change_from_image_to_text_strips_prior_image_content() -> Result<
|
|||
supported_in_api: true,
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
priority: 1,
|
||||
upgrade: None,
|
||||
base_instructions: "base instructions".to_string(),
|
||||
|
|
@ -385,6 +386,7 @@ async fn model_switch_to_smaller_model_updates_token_context_window() -> Result<
|
|||
supported_in_api: true,
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
priority: 1,
|
||||
upgrade: None,
|
||||
base_instructions: "base instructions".to_string(),
|
||||
|
|
|
|||
|
|
@ -352,5 +352,6 @@ fn test_remote_model(slug: &str, priority: i32) -> ModelInfo {
|
|||
experimental_supported_tools: Vec::new(),
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -616,6 +616,7 @@ async fn ignores_remote_personality_if_remote_models_disabled() -> anyhow::Resul
|
|||
experimental_supported_tools: Vec::new(),
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
};
|
||||
|
||||
let _models_mock = mount_models_once(
|
||||
|
|
@ -733,6 +734,7 @@ async fn remote_model_friendly_personality_instructions_with_feature() -> anyhow
|
|||
experimental_supported_tools: Vec::new(),
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
};
|
||||
|
||||
let _models_mock = mount_models_once(
|
||||
|
|
@ -845,6 +847,7 @@ async fn user_turn_personality_remote_model_template_includes_update_message() -
|
|||
experimental_supported_tools: Vec::new(),
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
};
|
||||
|
||||
let _models_mock = mount_models_once(
|
||||
|
|
|
|||
|
|
@ -225,6 +225,7 @@ async fn remote_models_remote_model_uses_unified_exec() -> Result<()> {
|
|||
supported_in_api: true,
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
priority: 1,
|
||||
upgrade: None,
|
||||
base_instructions: "base instructions".to_string(),
|
||||
|
|
@ -464,6 +465,7 @@ async fn remote_models_apply_remote_base_instructions() -> Result<()> {
|
|||
supported_in_api: true,
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
priority: 1,
|
||||
upgrade: None,
|
||||
base_instructions: remote_base.to_string(),
|
||||
|
|
@ -948,6 +950,7 @@ fn test_remote_model_with_policy(
|
|||
supported_in_api: true,
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
priority,
|
||||
upgrade: None,
|
||||
base_instructions: "base instructions".to_string(),
|
||||
|
|
|
|||
|
|
@ -74,7 +74,14 @@ async fn emits_warning_when_resumed_model_differs() {
|
|||
.expect("resume conversation");
|
||||
|
||||
// Assert: a Warning event is emitted describing the model mismatch.
|
||||
let warning = wait_for_event(&conversation, |ev| matches!(ev, EventMsg::Warning(_))).await;
|
||||
let warning = wait_for_event(&conversation, |ev| {
|
||||
matches!(
|
||||
ev,
|
||||
EventMsg::Warning(WarningEvent { message })
|
||||
if message.contains("previous-model") && message.contains("current-model")
|
||||
)
|
||||
})
|
||||
.await;
|
||||
let EventMsg::Warning(WarningEvent { message }) = warning else {
|
||||
panic!("expected warning event");
|
||||
};
|
||||
|
|
|
|||
|
|
@ -410,6 +410,7 @@ async fn stdio_image_responses_are_sanitized_for_text_only_model() -> anyhow::Re
|
|||
experimental_supported_tools: Vec::new(),
|
||||
input_modalities: vec![InputModality::Text],
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
}],
|
||||
},
|
||||
)
|
||||
|
|
|
|||
|
|
@ -671,6 +671,7 @@ async fn view_image_tool_returns_unsupported_message_for_text_only_model() -> an
|
|||
supported_in_api: true,
|
||||
input_modalities: vec![InputModality::Text],
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
priority: 1,
|
||||
upgrade: None,
|
||||
base_instructions: "base instructions".to_string(),
|
||||
|
|
|
|||
|
|
@ -253,6 +253,11 @@ pub struct ModelInfo {
|
|||
/// When true, this model should use websocket transport even when websocket features are off.
|
||||
#[serde(default)]
|
||||
pub prefer_websockets: bool,
|
||||
/// Internal-only marker set by core when a model slug resolved to fallback metadata.
|
||||
#[serde(default, skip_serializing, skip_deserializing)]
|
||||
#[schemars(skip)]
|
||||
#[ts(skip)]
|
||||
pub used_fallback_model_metadata: bool,
|
||||
}
|
||||
|
||||
impl ModelInfo {
|
||||
|
|
@ -517,6 +522,7 @@ mod tests {
|
|||
experimental_supported_tools: vec![],
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue