mirror of
https://github.com/openai/codex.git
synced 2026-10-30 19:18:49 +00:00
Validate thread providers before resume and input (#57840)
## Why Existing chats retain their provider configuration when the selected provider changes. Reject resume and input until the configured provider matches the chat, while keeping history readable and allowing recovery after configuration is restored. ## What changed - Validate saved providers before applying resume overrides, preserving the provider when only model or reasoning settings change. - Check the current provider selection, including profiles and launch overrides, before accepting input. Reject input when configuration cannot be loaded. - Return structured `ModelProviderError` data for provider mismatches, missing definitions, and policy conflicts, with generated JSON Schema, TypeScript, and Python types. ## Testing Add coverage for override bypass prevention, provider restoration, readable history after rejection, and blocked requests sending no inference traffic. Verify schema fields and Python round trips match the error wire format. GitOrigin-RevId: 051f97c3815c547d8411c71b770df30eae969f68
This commit is contained in:
@@ -295,7 +295,7 @@ any files were compressed; failures are reported through existing logs and metri
|
||||
There are no progress notifications or cancellation API. Clients sharing this
|
||||
Codex home must support compressed rollout files, including shared histories.
|
||||
|
||||
## Managed model provider requirements
|
||||
## Model provider admission
|
||||
|
||||
`configRequirements/read` reports `supportsIndependentSpeedModes: true` when
|
||||
`features.fast_mode` and `features.ultrafast_mode` independently control Fast and
|
||||
@@ -307,8 +307,18 @@ Existing threads retain their provider configuration. Input RPCs reject requests
|
||||
`model_provider` or `model_providers` requirements no longer match that configuration, or cannot
|
||||
be loaded. This covers turn start/steer, review, compaction, manual queue start, and active goal
|
||||
updates. Realtime connections use separate routing configuration and are not checked here.
|
||||
Interrupt, realtime stop, and goal pause/clear remain available. User and project
|
||||
configuration changes alone do not invalidate existing threads.
|
||||
Interrupt, realtime stop, and goal pause/clear remain available.
|
||||
|
||||
Resume and input RPCs also reject a thread whose saved provider differs from the current
|
||||
configured provider, including profile and app-server launch overrides. The JSON-RPC error's
|
||||
`data` contains `ModelProviderError`: `modelProviderMismatch` has `modelProvider` and
|
||||
`currentModelProvider`, `modelProviderNotConfigured` identifies a missing provider, and
|
||||
`modelProviderPolicyConflict` identifies a mandatory-policy conflict. Clients can still use
|
||||
`thread/read` to display history and disable the composer with the error's explanation.
|
||||
Changing the resume request's provider does not bypass validation of the saved provider;
|
||||
switch the configured provider back to continue the chat. Input is also rejected when the
|
||||
current provider configuration cannot be loaded. Older clients retain their normal error
|
||||
fallback for unrecognized structured error variants.
|
||||
|
||||
## Managed network requirements
|
||||
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
use codex_app_server_protocol::ModelProviderError;
|
||||
use codex_arg0::Arg0DispatchPaths;
|
||||
use codex_cloud_config::managed_config_bundle_loader;
|
||||
use codex_config::CloudConfigBundleLoader;
|
||||
@@ -38,12 +39,6 @@ use tracing::warn;
|
||||
#[path = "application_network.rs"]
|
||||
pub(crate) mod application_network;
|
||||
|
||||
#[derive(Debug, thiserror::Error)]
|
||||
#[error(
|
||||
"Your organization's required model provider settings changed. Restart Codex to apply them; this request was not sent"
|
||||
)]
|
||||
pub(crate) struct ModelProviderRequirementsChanged;
|
||||
|
||||
/// Shared app-server entry point for loading effective Codex configuration.
|
||||
#[derive(Clone)]
|
||||
pub(crate) struct ConfigManager {
|
||||
@@ -408,7 +403,12 @@ impl ConfigManager {
|
||||
if selection_changed || definition_changed {
|
||||
return Err(std::io::Error::new(
|
||||
std::io::ErrorKind::PermissionDenied,
|
||||
ModelProviderRequirementsChanged,
|
||||
ModelProviderError::ModelProviderPolicyConflict {
|
||||
model_provider: current.model_provider_id.clone(),
|
||||
required_model_provider: requirements
|
||||
.model_provider
|
||||
.unwrap_or_else(|| current.model_provider_id.clone()),
|
||||
},
|
||||
));
|
||||
}
|
||||
Ok(())
|
||||
@@ -430,6 +430,27 @@ impl ConfigManager {
|
||||
Ok(CloudStartupConfig { bootstrap, config })
|
||||
}
|
||||
|
||||
/// Admits resumed threads and input only on the currently selected provider.
|
||||
pub(crate) async fn check_thread_model_provider_for_input(
|
||||
&self,
|
||||
current: &Config,
|
||||
) -> std::io::Result<()> {
|
||||
self.check_thread_model_provider(current).await?;
|
||||
let configured = self
|
||||
.load_latest_config(Some(current.cwd.to_path_buf()))
|
||||
.await?;
|
||||
if current.model_provider_id != configured.model_provider_id {
|
||||
return Err(std::io::Error::new(
|
||||
std::io::ErrorKind::InvalidInput,
|
||||
ModelProviderError::ModelProviderMismatch {
|
||||
model_provider: current.model_provider_id.clone(),
|
||||
current_model_provider: configured.model_provider_id,
|
||||
},
|
||||
));
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Installs local policy before cloud access; failed policy loads block traffic while
|
||||
/// non-strict startup remains available for configuration repair.
|
||||
pub(crate) async fn load_startup_config(
|
||||
@@ -514,6 +535,89 @@ impl ConfigManager {
|
||||
})
|
||||
}
|
||||
|
||||
/// Validates saved and requested providers before resume overrides or policy can replace them.
|
||||
pub(crate) async fn load_for_resume(
|
||||
&self,
|
||||
saved_provider: Option<&str>,
|
||||
request_overrides: Option<HashMap<String, serde_json::Value>>,
|
||||
typesafe_overrides: ConfigOverrides,
|
||||
cwd: Option<PathBuf>,
|
||||
) -> std::io::Result<Config> {
|
||||
let requested_provider = typesafe_overrides.model_provider.clone().or_else(|| {
|
||||
request_overrides
|
||||
.as_ref()
|
||||
.and_then(|values| values.get("model_provider"))
|
||||
.and_then(serde_json::Value::as_str)
|
||||
.map(str::to_owned)
|
||||
});
|
||||
// Resolve the selection before the saved provider is applied as a resume override.
|
||||
let current_provider = self
|
||||
.load_latest_config(typesafe_overrides.cwd.clone().or_else(|| cwd.clone()))
|
||||
.await
|
||||
.map(|config| config.model_provider_id);
|
||||
self.load_with_validation(
|
||||
&self.current_cli_overrides(),
|
||||
request_overrides,
|
||||
typesafe_overrides,
|
||||
cwd,
|
||||
|config, _overrides, layers| {
|
||||
let requested_provider = requested_provider
|
||||
.as_deref()
|
||||
.or(config.model_provider.as_deref())
|
||||
.unwrap_or("openai");
|
||||
// Validate the saved route even when a client sends a provider override.
|
||||
for provider in [
|
||||
saved_provider.unwrap_or(requested_provider),
|
||||
requested_provider,
|
||||
] {
|
||||
if let Some(required) = layers.required_model_provider()
|
||||
&& required != provider
|
||||
{
|
||||
return Err(std::io::Error::new(
|
||||
std::io::ErrorKind::PermissionDenied,
|
||||
ModelProviderError::ModelProviderPolicyConflict {
|
||||
model_provider: provider.to_owned(),
|
||||
required_model_provider: required.to_owned(),
|
||||
},
|
||||
));
|
||||
}
|
||||
if !config.model_providers.contains_key(provider)
|
||||
&& !built_in_model_providers(config.openai_base_url.clone())
|
||||
.contains_key(provider)
|
||||
{
|
||||
return Err(std::io::Error::new(
|
||||
std::io::ErrorKind::NotFound,
|
||||
ModelProviderError::ModelProviderNotConfigured {
|
||||
model_provider: provider.to_owned(),
|
||||
},
|
||||
));
|
||||
}
|
||||
}
|
||||
let current_provider = current_provider?;
|
||||
for provider in [
|
||||
saved_provider.unwrap_or(requested_provider),
|
||||
requested_provider,
|
||||
] {
|
||||
if provider != current_provider {
|
||||
return Err(std::io::Error::new(
|
||||
std::io::ErrorKind::InvalidInput,
|
||||
ModelProviderError::ModelProviderMismatch {
|
||||
model_provider: provider.to_owned(),
|
||||
current_model_provider: current_provider,
|
||||
},
|
||||
));
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
},
|
||||
)
|
||||
.await
|
||||
.and_then(|config| {
|
||||
config.validate_windows_mxc_requirement()?;
|
||||
Ok(config)
|
||||
})
|
||||
}
|
||||
|
||||
/// Reload sources using the task's session flags before materializing config.
|
||||
pub(crate) async fn load_permission_config_for_thread(
|
||||
&self,
|
||||
@@ -558,11 +662,33 @@ impl ConfigManager {
|
||||
|
||||
#[instrument(level = "trace", skip_all)]
|
||||
pub(crate) async fn load_with_cli_overrides(
|
||||
&self,
|
||||
cli_overrides: &[(String, TomlValue)],
|
||||
request_overrides: Option<HashMap<String, serde_json::Value>>,
|
||||
typesafe_overrides: ConfigOverrides,
|
||||
fallback_cwd: Option<PathBuf>,
|
||||
) -> std::io::Result<Config> {
|
||||
self.load_with_validation(
|
||||
cli_overrides,
|
||||
request_overrides,
|
||||
typesafe_overrides,
|
||||
fallback_cwd,
|
||||
|_, _, _| Ok(()),
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
async fn load_with_validation(
|
||||
&self,
|
||||
cli_overrides: &[(String, TomlValue)],
|
||||
request_overrides: Option<HashMap<String, serde_json::Value>>,
|
||||
mut typesafe_overrides: ConfigOverrides,
|
||||
fallback_cwd: Option<PathBuf>,
|
||||
validate: impl FnOnce(
|
||||
&codex_config::config_toml::ConfigToml,
|
||||
&ConfigOverrides,
|
||||
&ConfigLayerStack,
|
||||
) -> std::io::Result<()>,
|
||||
) -> std::io::Result<Config> {
|
||||
let mcp_ema_cancellation = self.capture_mcp_ema_cancellation();
|
||||
let policy_load = self.refresh_application_network_policy().await?;
|
||||
@@ -593,7 +719,7 @@ impl ConfigManager {
|
||||
.fallback_cwd(fallback_cwd)
|
||||
.cloud_config_bundle(policy_load.cloud_config.clone())
|
||||
.thread_config_loader(Arc::clone(&self.thread_config_loader))
|
||||
.build()
|
||||
.build_with_validation(validate)
|
||||
.await;
|
||||
let mut config = result?;
|
||||
self.check_application_policy_load(&policy_load)?;
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
//! Checks retained providers against managed policy without reloading session configuration.
|
||||
//! Checks retained providers against managed policy and the current provider selection.
|
||||
|
||||
use super::*;
|
||||
use anyhow::Result;
|
||||
@@ -261,3 +261,62 @@ async fn catalog_reload_uses_latest_model_provider() -> Result<()> {
|
||||
assert_eq!(refreshed.model_provider_id, "openai");
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Invocation overrides and selected profiles still define the current provider.
|
||||
#[test_case("model_provider", "gateway"; "explicit_provider")]
|
||||
#[test_case("profile", "gateway-profile"; "selected_profile")]
|
||||
#[tokio::test]
|
||||
async fn input_provider_check_uses_effective_selection(key: &str, value: &str) -> Result<()> {
|
||||
let home = tempdir()?;
|
||||
std::fs::write(
|
||||
home.path().join("config.toml"),
|
||||
r#"
|
||||
model_provider = "openai"
|
||||
[model_providers.gateway]
|
||||
name = "Gateway"
|
||||
"#,
|
||||
)?;
|
||||
let mut loader_overrides = LoaderOverrides::without_managed_config_for_tests();
|
||||
let cli_overrides = if key == "profile" {
|
||||
std::fs::write(
|
||||
home.path().join(format!("{value}.config.toml")),
|
||||
"model_provider = 'gateway'\n[model_providers.gateway]\nname = 'Gateway'\n",
|
||||
)?;
|
||||
loader_overrides.user_config_path = Some(AbsolutePathBuf::from_absolute_path(
|
||||
home.path().join(format!("{value}.config.toml")),
|
||||
)?);
|
||||
loader_overrides.user_config_profile = Some(value.parse()?);
|
||||
Vec::new()
|
||||
} else {
|
||||
vec![(key.into(), TomlValue::String(value.into()))]
|
||||
};
|
||||
let manager = ConfigManager::new_for_tests(
|
||||
home.path().to_path_buf(),
|
||||
cli_overrides,
|
||||
loader_overrides,
|
||||
CloudConfigBundleLoader::default(),
|
||||
);
|
||||
let current = manager
|
||||
.load_latest_config(Some(home.path().to_path_buf()))
|
||||
.await?;
|
||||
assert_eq!(current.model_provider_id, "gateway");
|
||||
manager
|
||||
.check_thread_model_provider_for_input(¤t)
|
||||
.await?;
|
||||
let default_manager =
|
||||
ConfigManager::without_managed_config_for_tests(home.path().to_path_buf());
|
||||
let error = default_manager
|
||||
.check_thread_model_provider_for_input(¤t)
|
||||
.await
|
||||
.unwrap_err();
|
||||
assert_eq!(
|
||||
error
|
||||
.get_ref()
|
||||
.and_then(|error| error.downcast_ref::<ModelProviderError>()),
|
||||
Some(&ModelProviderError::ModelProviderMismatch {
|
||||
model_provider: "gateway".into(),
|
||||
current_model_provider: "openai".into(),
|
||||
})
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -266,11 +266,23 @@ delivery = {{ kind = "header", name = "X-Gateway-Authorization" }}
|
||||
}
|
||||
})
|
||||
.await?;
|
||||
assert_eq!(error, JSONRPCErrorError {
|
||||
code: -32600,
|
||||
message: "failed to load configuration: Your organization's required model provider settings changed. Restart Codex to apply them; this request was not sent".to_string(),
|
||||
data: None,
|
||||
});
|
||||
assert_eq!(
|
||||
error,
|
||||
JSONRPCErrorError {
|
||||
code: -32600,
|
||||
message: codex_app_server_protocol::ModelProviderError::ModelProviderPolicyConflict {
|
||||
model_provider: "gateway".into(),
|
||||
required_model_provider: "openai".into(),
|
||||
}
|
||||
.to_string(),
|
||||
data: Some(serde_json::to_value(
|
||||
codex_app_server_protocol::ModelProviderError::ModelProviderPolicyConflict {
|
||||
model_provider: "gateway".into(),
|
||||
required_model_provider: "openai".into(),
|
||||
}
|
||||
)?),
|
||||
}
|
||||
);
|
||||
assert!(server.received_requests().await.unwrap().is_empty());
|
||||
processor.shutdown_threads().await;
|
||||
processor.drain_background_tasks().await;
|
||||
|
||||
@@ -14,6 +14,19 @@ fn cloud_config_bundle_load_error(err: &std::io::Error) -> Option<&CloudConfigBu
|
||||
}
|
||||
|
||||
pub(crate) fn config_load_error(err: &std::io::Error) -> JSONRPCErrorError {
|
||||
let mut current = err
|
||||
.get_ref()
|
||||
.map(|error| error as &(dyn std::error::Error + 'static));
|
||||
while let Some(source) = current {
|
||||
if let Some(provider_error) =
|
||||
source.downcast_ref::<codex_app_server_protocol::ModelProviderError>()
|
||||
{
|
||||
let mut error = invalid_request(provider_error.to_string());
|
||||
error.data = serde_json::to_value(provider_error).ok();
|
||||
return error;
|
||||
}
|
||||
current = source.source();
|
||||
}
|
||||
let data = cloud_config_bundle_load_error(err).map(|cloud_error| {
|
||||
let mut data = serde_json::json!({
|
||||
"reason": "cloudConfigBundle",
|
||||
|
||||
@@ -162,7 +162,7 @@ impl ThreadGoalRequestProcessor {
|
||||
&& let Ok(thread) = self.thread_manager.get_thread(thread_id).await
|
||||
{
|
||||
self.config_manager
|
||||
.check_thread_model_provider(thread.config().await.as_ref())
|
||||
.check_thread_model_provider_for_input(thread.config().await.as_ref())
|
||||
.await
|
||||
.map_err(|error| config_load_error(&error))?;
|
||||
}
|
||||
|
||||
@@ -247,12 +247,23 @@ fn merge_persisted_resume_metadata(
|
||||
typesafe_overrides: &mut ConfigOverrides,
|
||||
persisted_metadata: &ThreadMetadata,
|
||||
) {
|
||||
if has_model_resume_override(request_overrides.as_ref(), typesafe_overrides) {
|
||||
let has_model_override =
|
||||
has_model_resume_override(request_overrides.as_ref(), typesafe_overrides);
|
||||
|
||||
// Model and reasoning overrides do not imply a provider change.
|
||||
if typesafe_overrides.model_provider.is_none()
|
||||
&& !request_overrides
|
||||
.as_ref()
|
||||
.is_some_and(|overrides| overrides.contains_key("model_provider"))
|
||||
{
|
||||
typesafe_overrides.model_provider = Some(persisted_metadata.model_provider.clone());
|
||||
}
|
||||
|
||||
if has_model_override {
|
||||
return;
|
||||
}
|
||||
|
||||
typesafe_overrides.model = persisted_metadata.model.clone();
|
||||
typesafe_overrides.model_provider = Some(persisted_metadata.model_provider.clone());
|
||||
|
||||
if let Some(reasoning_effort) = persisted_metadata.reasoning_effort.as_ref() {
|
||||
request_overrides.get_or_insert_with(HashMap::new).insert(
|
||||
@@ -292,7 +303,9 @@ fn has_model_resume_override(
|
||||
) -> bool {
|
||||
typesafe_overrides.model.is_some()
|
||||
|| typesafe_overrides.model_provider.is_some()
|
||||
|| request_overrides.is_some_and(|overrides| overrides.contains_key("model"))
|
||||
|| request_overrides.is_some_and(|overrides| {
|
||||
overrides.contains_key("model") || overrides.contains_key("model_provider")
|
||||
})
|
||||
|| request_overrides
|
||||
.is_some_and(|overrides| overrides.contains_key("model_reasoning_effort"))
|
||||
}
|
||||
@@ -2564,7 +2577,7 @@ impl ThreadRequestProcessor {
|
||||
let (_, thread) = self.load_thread(&thread_id).await?;
|
||||
ensure_direct_input_allowed(thread.as_ref()).await?;
|
||||
self.config_manager
|
||||
.check_thread_model_provider(thread.config().await.as_ref())
|
||||
.check_thread_model_provider_for_input(thread.config().await.as_ref())
|
||||
.await
|
||||
.map_err(|error| config_load_error(&error))?;
|
||||
self.submit_core_op(request_id, thread.as_ref(), Op::Compact)
|
||||
@@ -4252,7 +4265,32 @@ impl ThreadRequestProcessor {
|
||||
drop(_thread_list_state_permit);
|
||||
let config = self
|
||||
.config_manager
|
||||
.load_for_cwd(request_overrides, typesafe_overrides, history_cwd)
|
||||
.load_for_resume(
|
||||
resume_source_thread
|
||||
.as_ref()
|
||||
.map(|thread| thread.model_provider.as_str())
|
||||
.filter(|provider| !provider.is_empty())
|
||||
.or_else(|| {
|
||||
config_state
|
||||
.persisted_metadata
|
||||
.as_ref()
|
||||
.map(|metadata| metadata.model_provider.as_str())
|
||||
})
|
||||
.or_else(|| match &thread_history {
|
||||
InitialHistory::Resumed(resumed) => {
|
||||
resumed.history.iter().find_map(|item| match item {
|
||||
RolloutItem::SessionMeta(meta) => {
|
||||
meta.meta.model_provider.as_deref()
|
||||
}
|
||||
_ => None,
|
||||
})
|
||||
}
|
||||
_ => None,
|
||||
}),
|
||||
request_overrides,
|
||||
typesafe_overrides,
|
||||
history_cwd,
|
||||
)
|
||||
.await
|
||||
.map_err(|err| config_load_error(&err))?;
|
||||
*prepared_config = Some(PreparedResumeConfig {
|
||||
@@ -4710,6 +4748,10 @@ impl ThreadRequestProcessor {
|
||||
mismatch_details.join("; ")
|
||||
);
|
||||
}
|
||||
self.config_manager
|
||||
.check_thread_model_provider_for_input(existing_thread.config().await.as_ref())
|
||||
.await
|
||||
.map_err(|err| config_load_error(&err))?;
|
||||
let redact_resume_payloads =
|
||||
should_redact_thread_resume_payloads(app_server_client_name.as_deref());
|
||||
let include_turns = !params.exclude_turns;
|
||||
|
||||
@@ -858,7 +858,10 @@ mod thread_processor_behavior_tests {
|
||||
);
|
||||
|
||||
assert_eq!(typesafe_overrides.model, Some("gpt-5.2-codex".to_string()));
|
||||
assert_eq!(typesafe_overrides.model_provider, None);
|
||||
assert_eq!(
|
||||
typesafe_overrides.model_provider,
|
||||
Some("mock_provider".to_string())
|
||||
);
|
||||
assert_eq!(
|
||||
request_overrides,
|
||||
Some(HashMap::from([(
|
||||
@@ -870,8 +873,7 @@ mod thread_processor_behavior_tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn merge_persisted_resume_metadata_skips_persisted_values_when_model_overridden() -> Result<()>
|
||||
{
|
||||
fn merge_persisted_resume_metadata_preserves_provider_when_model_overridden() -> Result<()> {
|
||||
let mut request_overrides = Some(HashMap::from([(
|
||||
"model".to_string(),
|
||||
serde_json::Value::String("gpt-5.2-codex".to_string()),
|
||||
@@ -887,7 +889,10 @@ mod thread_processor_behavior_tests {
|
||||
);
|
||||
|
||||
assert_eq!(typesafe_overrides.model, None);
|
||||
assert_eq!(typesafe_overrides.model_provider, None);
|
||||
assert_eq!(
|
||||
typesafe_overrides.model_provider,
|
||||
Some("mock_provider".to_string())
|
||||
);
|
||||
assert_eq!(
|
||||
request_overrides,
|
||||
Some(HashMap::from([(
|
||||
@@ -922,7 +927,7 @@ mod thread_processor_behavior_tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn merge_persisted_resume_metadata_skips_persisted_values_when_reasoning_effort_overridden()
|
||||
fn merge_persisted_resume_metadata_preserves_provider_when_reasoning_effort_overridden()
|
||||
-> Result<()> {
|
||||
let mut request_overrides = Some(HashMap::from([(
|
||||
"model_reasoning_effort".to_string(),
|
||||
@@ -939,7 +944,10 @@ mod thread_processor_behavior_tests {
|
||||
);
|
||||
|
||||
assert_eq!(typesafe_overrides.model, None);
|
||||
assert_eq!(typesafe_overrides.model_provider, None);
|
||||
assert_eq!(
|
||||
typesafe_overrides.model_provider,
|
||||
Some("mock_provider".to_string())
|
||||
);
|
||||
assert_eq!(
|
||||
request_overrides,
|
||||
Some(HashMap::from([(
|
||||
|
||||
@@ -193,7 +193,7 @@ impl ThreadQueueRequestProcessor {
|
||||
let thread = loaded_thread
|
||||
.ok_or_else(|| invalid_request("resume the thread before starting a queued message"))?;
|
||||
self.config_manager
|
||||
.check_thread_model_provider(thread.config().await.as_ref())
|
||||
.check_thread_model_provider_for_input(thread.config().await.as_ref())
|
||||
.await
|
||||
.map_err(|error| config_load_error(&error))?;
|
||||
let submission = self
|
||||
|
||||
@@ -338,7 +338,7 @@ impl TurnRequestProcessor {
|
||||
self.ensure_direct_input_allowed(request_id, thread.as_ref())
|
||||
.await?;
|
||||
self.config_manager
|
||||
.check_thread_model_provider(thread.config().await.as_ref())
|
||||
.check_thread_model_provider_for_input(thread.config().await.as_ref())
|
||||
.await
|
||||
.map_err(|error| config_load_error(&error))?;
|
||||
let (review_request, display_text) = Self::review_request_from_target(params.target)?;
|
||||
@@ -508,7 +508,7 @@ impl TurnRequestProcessor {
|
||||
self.ensure_direct_input_allowed(&request_id, thread.as_ref())
|
||||
.await?;
|
||||
self.config_manager
|
||||
.check_thread_model_provider(thread.config().await.as_ref())
|
||||
.check_thread_model_provider_for_input(thread.config().await.as_ref())
|
||||
.await
|
||||
.map_err(|error| config_load_error(&error))?;
|
||||
if let Some(tool_output) = ¶ms.tool_output {
|
||||
@@ -1037,7 +1037,7 @@ impl TurnRequestProcessor {
|
||||
self.ensure_direct_input_allowed(request_id, thread.as_ref())
|
||||
.await?;
|
||||
self.config_manager
|
||||
.check_thread_model_provider(thread.config().await.as_ref())
|
||||
.check_thread_model_provider_for_input(thread.config().await.as_ref())
|
||||
.await
|
||||
.map_err(|error| config_load_error(&error))?;
|
||||
|
||||
|
||||
@@ -14,9 +14,12 @@ use codex_app_server_protocol::ApprovalsReviewer;
|
||||
use codex_app_server_protocol::AskForApproval;
|
||||
use codex_app_server_protocol::ClientInfo;
|
||||
use codex_app_server_protocol::ClientRequest;
|
||||
use codex_app_server_protocol::ConfigValueWriteParams;
|
||||
use codex_app_server_protocol::ConfigWriteResponse;
|
||||
use codex_app_server_protocol::InitializeCapabilities;
|
||||
use codex_app_server_protocol::JSONRPCMessage;
|
||||
use codex_app_server_protocol::LoginAccountResponse;
|
||||
use codex_app_server_protocol::MergeStrategy;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
use codex_app_server_protocol::SandboxPolicy;
|
||||
use codex_app_server_protocol::ServerRequest;
|
||||
@@ -289,6 +292,16 @@ supports_websockets = false
|
||||
|
||||
let mut thread_ids = Vec::new();
|
||||
for provider in ["bearer_provider", "mock_provider"] {
|
||||
let config_id = app_server
|
||||
.send_config_value_write_request(ConfigValueWriteParams {
|
||||
file_path: None,
|
||||
key_path: "model_provider".into(),
|
||||
value: json!(provider),
|
||||
merge_strategy: MergeStrategy::Replace,
|
||||
expected_version: None,
|
||||
})
|
||||
.await?;
|
||||
let _: ConfigWriteResponse = app_server.read_response(config_id).await?;
|
||||
let thread = app_server
|
||||
.start_thread(ThreadStartParams {
|
||||
model_provider: Some(provider.into()),
|
||||
|
||||
@@ -1010,12 +1010,14 @@ async fn resume_revalidates_persisted_thread_after_config_load(
|
||||
(resumed.thread.git_info, resumed.thread.project_id),
|
||||
(expected_git_info.clone(), expected_project_id.clone())
|
||||
);
|
||||
// Each resume attempt loads the current provider selection and then the
|
||||
// saved thread config. Changed metadata must trigger another full attempt.
|
||||
if matches!(mutation, ResumeMutation::Unchanged) {
|
||||
assert_eq!(resume_config_loads, 1);
|
||||
assert_eq!(resume_config_loads, 2);
|
||||
} else {
|
||||
assert!(
|
||||
resume_config_loads >= 2,
|
||||
"changed persisted metadata must reload config"
|
||||
resume_config_loads >= 4,
|
||||
"changed persisted metadata must reload both provider and thread config"
|
||||
);
|
||||
}
|
||||
turn_result?;
|
||||
|
||||
@@ -1,17 +1,15 @@
|
||||
//! Verifies provider requirements at app-server input and queue admission boundaries.
|
||||
//! Verifies provider selection and requirements at app-server input and queue admission boundaries.
|
||||
|
||||
use anyhow::Result;
|
||||
use app_test_support::MockResponsesConfig;
|
||||
use app_test_support::TestAppServer;
|
||||
use codex_app_server::in_process;
|
||||
use codex_app_server::in_process::InProcessServerEvent;
|
||||
use codex_app_server::in_process::InProcessStartArgs;
|
||||
use codex_app_server_protocol::ClientInfo;
|
||||
use codex_app_server_protocol::ClientRequest;
|
||||
use codex_app_server_protocol::InitializeParams;
|
||||
use codex_app_server_protocol::JSONRPCErrorError;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
use codex_app_server_protocol::ServerNotification;
|
||||
use codex_app_server_protocol::ThreadQueueAddParams;
|
||||
use codex_app_server_protocol::ThreadQueueAddResponse;
|
||||
use codex_app_server_protocol::ThreadQueueListParams;
|
||||
@@ -44,6 +42,7 @@ use wiremock::MockServer;
|
||||
|
||||
#[test_case("other"; "selection_changes")]
|
||||
#[test_case("gateway"; "definition_changes")]
|
||||
#[test_case("default"; "default_selection_changes")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn provider_requirement_changes_reject_inputs_to_existing_threads(
|
||||
required_selection: &str,
|
||||
@@ -80,7 +79,17 @@ base_url = "{}/v1"
|
||||
other.uri(),
|
||||
);
|
||||
let requirements_path = home.path().join("requirements.toml");
|
||||
std::fs::write(&requirements_path, &requirements)?;
|
||||
if required_selection == "default" {
|
||||
std::fs::write(
|
||||
&requirements_path,
|
||||
requirements.replace("model_provider = \"gateway\"", ""),
|
||||
)?;
|
||||
let path = home.path().join("config.toml");
|
||||
let config = std::fs::read_to_string(&path)?;
|
||||
std::fs::write(path, config.replace("mock_provider", "gateway"))?;
|
||||
} else {
|
||||
std::fs::write(&requirements_path, &requirements)?;
|
||||
}
|
||||
let mut server = TestAppServer::builder()
|
||||
.with_codex_home(home.path())
|
||||
.with_json_logging("warn")
|
||||
@@ -127,13 +136,31 @@ base_url = "{}/v1"
|
||||
} else {
|
||||
requirements.replace(gateway.uri(), &other.uri())
|
||||
};
|
||||
std::fs::write(requirements_path, updated)?;
|
||||
let provider_error = if required_selection == "default" {
|
||||
let path = home.path().join("config.toml");
|
||||
let config = std::fs::read_to_string(&path)?;
|
||||
std::fs::write(
|
||||
path,
|
||||
config.replace("model_provider = \"gateway\"", "model_provider = \"other\""),
|
||||
)?;
|
||||
codex_app_server_protocol::ModelProviderError::ModelProviderMismatch {
|
||||
model_provider: "gateway".into(),
|
||||
current_model_provider: "other".into(),
|
||||
}
|
||||
} else {
|
||||
std::fs::write(requirements_path, updated)?;
|
||||
codex_app_server_protocol::ModelProviderError::ModelProviderPolicyConflict {
|
||||
model_provider: "gateway".into(),
|
||||
required_model_provider: required_selection.into(),
|
||||
}
|
||||
};
|
||||
let expected_error = JSONRPCErrorError {
|
||||
code: -32600,
|
||||
message: "failed to load configuration: Your organization's required model provider settings changed. Restart Codex to apply them; this request was not sent".to_string(),
|
||||
data: None,
|
||||
message: provider_error.to_string(),
|
||||
data: Some(serde_json::to_value(provider_error)?),
|
||||
};
|
||||
for (method, params) in [
|
||||
("thread/resume", json!({"threadId": started.thread.id})),
|
||||
(
|
||||
"thread/goal/set",
|
||||
json!({"threadId": started.thread.id, "objective": "Blocked objective"}),
|
||||
@@ -256,46 +283,71 @@ base_url = "{}/v1"
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn local_config_changes_do_not_block_existing_threads() -> Result<()> {
|
||||
async fn provider_selection_can_be_restored_for_existing_threads() -> Result<()> {
|
||||
let provider = MockServer::start().await;
|
||||
let home = TempDir::new()?;
|
||||
MockResponsesConfig::new(&provider.uri()).write(home.path())?;
|
||||
let path = home.path().join("config.toml");
|
||||
let original = std::fs::read_to_string(&path)?;
|
||||
let mut server = TestAppServer::builder()
|
||||
.with_codex_home(home.path())
|
||||
.build_initialized()
|
||||
.await?;
|
||||
let started = server.start_thread(ThreadStartParams::default()).await?;
|
||||
let requests = responses::mount_sse_sequence(
|
||||
let requests = responses::mount_sse_once(
|
||||
&provider,
|
||||
vec![
|
||||
responses::sse(vec![responses::ev_completed("first")]),
|
||||
responses::sse(vec![responses::ev_completed("second")]),
|
||||
],
|
||||
responses::sse(vec![responses::ev_completed("restored")]),
|
||||
)
|
||||
.await;
|
||||
for local_settings in ["model_provider = 'openai'", "invalid toml !!!"] {
|
||||
std::fs::write(home.path().join("config.toml"), local_settings)?;
|
||||
let id = server
|
||||
.send_turn_start_request(TurnStartParams {
|
||||
thread_id: started.thread.id.clone(),
|
||||
input: vec![UserInput::Text {
|
||||
text: "Continue on the original provider.".into(),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let _: TurnStartResponse = server.read_response(id).await?;
|
||||
server
|
||||
.read_stream_until_notification_message("turn/completed")
|
||||
.await?;
|
||||
}
|
||||
assert_eq!(requests.requests().len(), 2);
|
||||
std::fs::write(
|
||||
&path,
|
||||
original.replace(
|
||||
"model_provider = \"mock_provider\"",
|
||||
"model_provider = \"openai\"",
|
||||
),
|
||||
)?;
|
||||
let params = TurnStartParams {
|
||||
thread_id: started.thread.id.clone(),
|
||||
input: vec![UserInput::Text {
|
||||
text: "Continue on the original provider.".into(),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
..Default::default()
|
||||
};
|
||||
let id = server.send_turn_start_request(params.clone()).await?;
|
||||
let error = server
|
||||
.read_stream_until_error_message(RequestId::Integer(id))
|
||||
.await?;
|
||||
assert_eq!(
|
||||
error.error.data,
|
||||
Some(serde_json::to_value(
|
||||
codex_app_server_protocol::ModelProviderError::ModelProviderMismatch {
|
||||
model_provider: "mock_provider".into(),
|
||||
current_model_provider: "openai".into(),
|
||||
}
|
||||
)?)
|
||||
);
|
||||
assert!(requests.requests().is_empty());
|
||||
let id = server
|
||||
.send_raw_request(
|
||||
"thread/read",
|
||||
Some(json!({"threadId": started.thread.id, "includeTurns": false})),
|
||||
)
|
||||
.await?;
|
||||
let read: codex_app_server_protocol::ThreadReadResponse = server.read_response(id).await?;
|
||||
assert_eq!(read.thread.model_provider, "mock_provider");
|
||||
std::fs::write(path, original)?;
|
||||
let id = server.send_turn_start_request(params).await?;
|
||||
let _: TurnStartResponse = server.read_response(id).await?;
|
||||
server
|
||||
.read_stream_until_notification_message("turn/completed")
|
||||
.await?;
|
||||
assert_eq!(requests.requests().len(), 1);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn malformed_system_defaults_do_not_block_existing_thread_turn() -> Result<()> {
|
||||
async fn malformed_system_defaults_reject_input_until_provider_can_be_resolved() -> Result<()> {
|
||||
let provider = MockServer::start().await;
|
||||
let home = TempDir::new()?;
|
||||
MockResponsesConfig::new(&provider.uri()).write(home.path())?;
|
||||
@@ -310,7 +362,7 @@ async fn malformed_system_defaults_do_not_block_existing_thread_turn() -> Result
|
||||
.build()
|
||||
.await?,
|
||||
);
|
||||
let mut client = in_process::start(InProcessStartArgs {
|
||||
let client = in_process::start(InProcessStartArgs {
|
||||
arg0_paths: Arg0DispatchPaths::default(),
|
||||
config,
|
||||
cli_overrides: Vec::new(),
|
||||
@@ -365,23 +417,9 @@ async fn malformed_system_defaults_do_not_block_existing_thread_turn() -> Result
|
||||
},
|
||||
})
|
||||
.await?
|
||||
.expect("turn/start should succeed");
|
||||
let _: TurnStartResponse = serde_json::from_value(response)?;
|
||||
timeout(Duration::from_secs(/*secs*/ 60), async {
|
||||
loop {
|
||||
let Some(event) = client.next_event().await else {
|
||||
anyhow::bail!("app-server stopped before turn/completed");
|
||||
};
|
||||
if let InProcessServerEvent::ServerNotification(notification) = event
|
||||
&& let ServerNotification::TurnCompleted(completed) = notification.as_ref()
|
||||
&& completed.thread_id == started.thread.id
|
||||
{
|
||||
return Ok::<(), anyhow::Error>(());
|
||||
}
|
||||
}
|
||||
})
|
||||
.await??;
|
||||
assert_eq!(requests.requests().len(), 1);
|
||||
.expect_err("cannot verify the selected provider from malformed configuration");
|
||||
assert_eq!(response.code, -32600);
|
||||
assert!(requests.requests().is_empty());
|
||||
client.shutdown().await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -6782,3 +6782,221 @@ async fn setup_rollout_fixture(codex_home: &Path, server_uri: &str) -> Result<Ro
|
||||
rollout_file_path,
|
||||
})
|
||||
}
|
||||
|
||||
#[test_case::test_case(false, false; "missing_definition_config_override")]
|
||||
#[test_case::test_case(false, true; "missing_definition_typed_override")]
|
||||
#[test_case::test_case(true, false; "policy_conflict_config_override")]
|
||||
#[test_case::test_case(true, true; "policy_conflict_typed_override")]
|
||||
#[tokio::test]
|
||||
async fn thread_resume_returns_provider_error_even_with_provider_override(
|
||||
policy_conflict: bool,
|
||||
typed_override: bool,
|
||||
) -> Result<()> {
|
||||
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
||||
let home = TempDir::new()?;
|
||||
mock_responses_config(&server.uri()).write(home.path())?;
|
||||
let thread_id = create_fake_rollout(
|
||||
home.path(),
|
||||
"2025-01-05T12-00-00",
|
||||
"2025-01-05T12:00:00Z",
|
||||
"Saved history",
|
||||
Some("old_provider"),
|
||||
/*git_info*/ None,
|
||||
)?;
|
||||
if policy_conflict {
|
||||
let config_path = home.path().join("config.toml");
|
||||
let config = std::fs::read_to_string(&config_path)?;
|
||||
let server_uri = server.uri();
|
||||
std::fs::write(
|
||||
config_path,
|
||||
format!(
|
||||
"{config}\n[model_providers.old_provider]\nname = 'Original provider'\nbase_url = '{server_uri}/v1'\n"
|
||||
),
|
||||
)?;
|
||||
std::fs::write(
|
||||
home.path().join("requirements.toml"),
|
||||
"model_provider = 'mock_provider'",
|
||||
)?;
|
||||
}
|
||||
let mut client = TestAppServer::builder()
|
||||
.with_codex_home(home.path())
|
||||
.build_initialized()
|
||||
.await?;
|
||||
let expected = if policy_conflict {
|
||||
codex_app_server_protocol::ModelProviderError::ModelProviderPolicyConflict {
|
||||
model_provider: "old_provider".into(),
|
||||
required_model_provider: "mock_provider".into(),
|
||||
}
|
||||
} else {
|
||||
codex_app_server_protocol::ModelProviderError::ModelProviderNotConfigured {
|
||||
model_provider: "old_provider".into(),
|
||||
}
|
||||
};
|
||||
// Model and reasoning overrides must not discard the provider being validated.
|
||||
for mut params in [
|
||||
ThreadResumeParams::default(),
|
||||
ThreadResumeParams {
|
||||
model: Some("gpt-5.1-codex".into()),
|
||||
..Default::default()
|
||||
},
|
||||
ThreadResumeParams {
|
||||
config: Some(std::collections::HashMap::from([(
|
||||
"model".into(),
|
||||
json!("gpt-5.1-codex"),
|
||||
)])),
|
||||
..Default::default()
|
||||
},
|
||||
ThreadResumeParams {
|
||||
config: Some(std::collections::HashMap::from([(
|
||||
"model_reasoning_effort".into(),
|
||||
json!("high"),
|
||||
)])),
|
||||
..Default::default()
|
||||
},
|
||||
] {
|
||||
params.thread_id = thread_id.clone();
|
||||
let id = client.send_thread_resume_request(params).await?;
|
||||
let error = client
|
||||
.read_stream_until_error_message(RequestId::Integer(id))
|
||||
.await?;
|
||||
assert_eq!(
|
||||
error.error,
|
||||
JSONRPCErrorError {
|
||||
code: -32600,
|
||||
message: expected.to_string(),
|
||||
data: Some(serde_json::to_value(&expected)?),
|
||||
}
|
||||
);
|
||||
}
|
||||
assert!(
|
||||
server
|
||||
.received_requests()
|
||||
.await
|
||||
.expect("mock server should record received requests")
|
||||
.iter()
|
||||
.all(|request| !request.url.path().ends_with("/responses"))
|
||||
);
|
||||
// Failure leaves the history readable and does not create a live session with a new provider.
|
||||
let id = client
|
||||
.send_raw_request(
|
||||
"thread/read",
|
||||
Some(json!({"threadId": thread_id, "includeTurns": true})),
|
||||
)
|
||||
.await?;
|
||||
let read: codex_app_server_protocol::ThreadReadResponse = client.read_response(id).await?;
|
||||
assert_eq!(read.thread.model_provider, "old_provider");
|
||||
// Provider overrides must not bypass admission for the saved provider.
|
||||
let id = client
|
||||
.send_thread_resume_request(ThreadResumeParams {
|
||||
thread_id,
|
||||
model: Some("gpt-5.1-codex".into()),
|
||||
model_provider: typed_override.then(|| "mock_provider".into()),
|
||||
config: (!typed_override).then(|| {
|
||||
std::collections::HashMap::from([("model_provider".into(), json!("mock_provider"))])
|
||||
}),
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let error = client
|
||||
.read_stream_until_error_message(RequestId::Integer(id))
|
||||
.await?;
|
||||
assert_eq!(error.error.data, Some(serde_json::to_value(expected)?));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test_case::test_case(false; "missing_definition")]
|
||||
#[test_case::test_case(true; "provider_mismatch")]
|
||||
#[tokio::test]
|
||||
async fn thread_resume_recovers_after_provider_configuration_is_restored(
|
||||
configured: bool,
|
||||
) -> Result<()> {
|
||||
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
||||
let home = TempDir::new()?;
|
||||
mock_responses_config(&server.uri()).write(home.path())?;
|
||||
let thread_id = create_fake_rollout(
|
||||
home.path(),
|
||||
"2025-01-05T12-00-00",
|
||||
"2025-01-05T12:00:00Z",
|
||||
"Saved history",
|
||||
Some("old_provider"),
|
||||
/*git_info*/ None,
|
||||
)?;
|
||||
let config_path = home.path().join("config.toml");
|
||||
if configured {
|
||||
let config = std::fs::read_to_string(&config_path)?;
|
||||
let server_uri = server.uri();
|
||||
std::fs::write(
|
||||
&config_path,
|
||||
format!(
|
||||
"{config}\n[model_providers.old_provider]\nname = 'Original provider'\nbase_url = '{server_uri}/v1'\n"
|
||||
),
|
||||
)?;
|
||||
}
|
||||
let mut client = TestAppServer::builder()
|
||||
.with_codex_home(home.path())
|
||||
.build_initialized()
|
||||
.await?;
|
||||
let params = ThreadResumeParams {
|
||||
thread_id,
|
||||
..Default::default()
|
||||
};
|
||||
let id = client.send_thread_resume_request(params.clone()).await?;
|
||||
let error = client
|
||||
.read_stream_until_error_message(RequestId::Integer(id))
|
||||
.await?;
|
||||
assert_eq!(
|
||||
error
|
||||
.error
|
||||
.data
|
||||
.as_ref()
|
||||
.expect("provider error should include structured data")["type"],
|
||||
if configured {
|
||||
"modelProviderMismatch"
|
||||
} else {
|
||||
"modelProviderNotConfigured"
|
||||
}
|
||||
);
|
||||
if configured {
|
||||
for overrides in [
|
||||
ThreadResumeParams {
|
||||
model_provider: Some("mock_provider".into()),
|
||||
..params.clone()
|
||||
},
|
||||
ThreadResumeParams {
|
||||
config: Some(std::collections::HashMap::from([(
|
||||
"model_provider".into(),
|
||||
json!("mock_provider"),
|
||||
)])),
|
||||
..params.clone()
|
||||
},
|
||||
] {
|
||||
let id = client.send_thread_resume_request(overrides).await?;
|
||||
let overridden = client
|
||||
.read_stream_until_error_message(RequestId::Integer(id))
|
||||
.await?;
|
||||
assert_eq!(overridden.error, error.error);
|
||||
}
|
||||
}
|
||||
let id = client
|
||||
.send_raw_request(
|
||||
"thread/read",
|
||||
Some(json!({"threadId": params.thread_id, "includeTurns": true})),
|
||||
)
|
||||
.await?;
|
||||
let read: codex_app_server_protocol::ThreadReadResponse = client.read_response(id).await?;
|
||||
assert_eq!(read.thread.model_provider, "old_provider");
|
||||
let config = std::fs::read_to_string(&config_path)?;
|
||||
let restored = if configured {
|
||||
config.replace(
|
||||
"model_provider = \"mock_provider\"",
|
||||
"model_provider = \"old_provider\"",
|
||||
)
|
||||
} else {
|
||||
config.replace("mock_provider", "old_provider")
|
||||
};
|
||||
std::fs::write(config_path, restored)?;
|
||||
let id = client.send_thread_resume_request(params).await?;
|
||||
let resumed: ThreadResumeResponse = client.read_response(id).await?;
|
||||
assert_eq!(resumed.model_provider, "old_provider");
|
||||
Ok(())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user