Print warning when config does not meet requirements (#10792)
<img width="1019" height="284" alt="Screenshot 2026-02-05 at 23 34 08" src="https://github.com/user-attachments/assets/19ec3ce1-3c3b-40f5-b251-a31d964bf3bb" /> Currently, if a config value is set that fails the requirements, we exit Codex. Now, instead of this, we print a warning and default to a requirements-permitting value.
This commit is contained in:
gt-oai
GitHub
parent
0d8b2b74c4
commit
d74fa8edd1
3 changed files with 112 additions and 24 deletions
|
|
@ -961,6 +961,14 @@ impl Session {
|
|||
}),
|
||||
});
|
||||
}
|
||||
for message in &config.startup_warnings {
|
||||
post_session_configured_events.push(Event {
|
||||
id: "".to_owned(),
|
||||
msg: EventMsg::Warning(WarningEvent {
|
||||
message: message.clone(),
|
||||
}),
|
||||
});
|
||||
}
|
||||
maybe_push_unstable_features_warning(&config, &mut post_session_configured_events);
|
||||
|
||||
let auth = auth.as_ref();
|
||||
|
|
|
|||
|
|
@ -21,6 +21,7 @@ use crate::config::types::UriBasedFileOpener;
|
|||
use crate::config_loader::CloudRequirementsLoader;
|
||||
use crate::config_loader::ConfigLayerStack;
|
||||
use crate::config_loader::ConfigRequirements;
|
||||
use crate::config_loader::ConstrainedWithSource;
|
||||
use crate::config_loader::LoaderOverrides;
|
||||
use crate::config_loader::McpServerIdentity;
|
||||
use crate::config_loader::McpServerRequirement;
|
||||
|
|
@ -116,6 +117,9 @@ pub struct Config {
|
|||
/// requirements).
|
||||
pub config_layer_stack: ConfigLayerStack,
|
||||
|
||||
/// Warnings collected during config load that should be shown on startup.
|
||||
pub startup_warnings: Vec<String>,
|
||||
|
||||
/// Optional override of model selection.
|
||||
pub model: Option<String>,
|
||||
|
||||
|
|
@ -601,6 +605,41 @@ fn constrain_mcp_servers(
|
|||
})
|
||||
}
|
||||
|
||||
fn apply_requirement_constrained_value<T>(
|
||||
field_name: &'static str,
|
||||
configured_value: T,
|
||||
constrained_value: &mut ConstrainedWithSource<T>,
|
||||
startup_warnings: &mut Vec<String>,
|
||||
) -> std::io::Result<()>
|
||||
where
|
||||
T: Clone + std::fmt::Debug + Send + Sync,
|
||||
{
|
||||
if let Err(err) = constrained_value.set(configured_value) {
|
||||
let fallback_value = constrained_value.get().clone();
|
||||
tracing::warn!(
|
||||
error = %err,
|
||||
?fallback_value,
|
||||
requirement_source = ?constrained_value.source,
|
||||
"configured value is disallowed by requirements; falling back to required value for {field_name}"
|
||||
);
|
||||
let message = format!(
|
||||
"Configured value for `{field_name}` is disallowed by requirements; falling back to required value {fallback_value:?}. Details: {err}"
|
||||
);
|
||||
startup_warnings.push(message);
|
||||
|
||||
constrained_value.set(fallback_value).map_err(|fallback_err| {
|
||||
std::io::Error::new(
|
||||
std::io::ErrorKind::InvalidInput,
|
||||
format!(
|
||||
"configured value for `{field_name}` is disallowed by requirements ({err}); fallback to a requirement-compliant value also failed ({fallback_err})"
|
||||
),
|
||||
)
|
||||
})?;
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn mcp_server_matches_requirement(
|
||||
requirement: &McpServerRequirement,
|
||||
server: &McpServerConfig,
|
||||
|
|
@ -1320,6 +1359,7 @@ impl Config {
|
|||
) -> std::io::Result<Self> {
|
||||
let requirements = config_layer_stack.requirements().clone();
|
||||
let user_instructions = Self::load_instructions(Some(&codex_home));
|
||||
let mut startup_warnings = Vec::new();
|
||||
|
||||
// Destructure ConfigOverrides fully to ensure all overrides are applied.
|
||||
let ConfigOverrides {
|
||||
|
|
@ -1586,12 +1626,18 @@ impl Config {
|
|||
enforce_residency,
|
||||
} = requirements;
|
||||
|
||||
constrained_approval_policy
|
||||
.set(approval_policy)
|
||||
.map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidInput, format!("{e}")))?;
|
||||
constrained_sandbox_policy
|
||||
.set(sandbox_policy)
|
||||
.map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidInput, format!("{e}")))?;
|
||||
apply_requirement_constrained_value(
|
||||
"approval_policy",
|
||||
approval_policy,
|
||||
&mut constrained_approval_policy,
|
||||
&mut startup_warnings,
|
||||
)?;
|
||||
apply_requirement_constrained_value(
|
||||
"sandbox_mode",
|
||||
sandbox_policy,
|
||||
&mut constrained_sandbox_policy,
|
||||
&mut startup_warnings,
|
||||
)?;
|
||||
|
||||
let mcp_servers = constrain_mcp_servers(cfg.mcp_servers.clone(), mcp_servers.as_ref())
|
||||
.map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidInput, format!("{e}")))?;
|
||||
|
|
@ -1604,6 +1650,7 @@ impl Config {
|
|||
model_provider_id,
|
||||
model_provider,
|
||||
cwd: resolved_cwd,
|
||||
startup_warnings,
|
||||
approval_policy: constrained_approval_policy.value,
|
||||
sandbox_policy: constrained_sandbox_policy.value,
|
||||
enforce_residency: enforce_residency.value,
|
||||
|
|
@ -3853,6 +3900,7 @@ model_verbosity = "high"
|
|||
codex_home: fixture.codex_home(),
|
||||
log_dir: fixture.codex_home().join("log"),
|
||||
config_layer_stack: Default::default(),
|
||||
startup_warnings: Vec::new(),
|
||||
history: History::default(),
|
||||
ephemeral: false,
|
||||
file_opener: UriBasedFileOpener::VsCode,
|
||||
|
|
@ -3940,6 +3988,7 @@ model_verbosity = "high"
|
|||
codex_home: fixture.codex_home(),
|
||||
log_dir: fixture.codex_home().join("log"),
|
||||
config_layer_stack: Default::default(),
|
||||
startup_warnings: Vec::new(),
|
||||
history: History::default(),
|
||||
ephemeral: false,
|
||||
file_opener: UriBasedFileOpener::VsCode,
|
||||
|
|
@ -4042,6 +4091,7 @@ model_verbosity = "high"
|
|||
codex_home: fixture.codex_home(),
|
||||
log_dir: fixture.codex_home().join("log"),
|
||||
config_layer_stack: Default::default(),
|
||||
startup_warnings: Vec::new(),
|
||||
history: History::default(),
|
||||
ephemeral: false,
|
||||
file_opener: UriBasedFileOpener::VsCode,
|
||||
|
|
@ -4130,6 +4180,7 @@ model_verbosity = "high"
|
|||
codex_home: fixture.codex_home(),
|
||||
log_dir: fixture.codex_home().join("log"),
|
||||
config_layer_stack: Default::default(),
|
||||
startup_warnings: Vec::new(),
|
||||
history: History::default(),
|
||||
ephemeral: false,
|
||||
file_opener: UriBasedFileOpener::VsCode,
|
||||
|
|
@ -4681,7 +4732,7 @@ mcp_oauth_callback_port = 5678
|
|||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn explicit_sandbox_mode_still_errors_when_disallowed_by_requirements()
|
||||
async fn explicit_sandbox_mode_falls_back_when_disallowed_by_requirements()
|
||||
-> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
std::fs::write(
|
||||
|
|
@ -4700,19 +4751,15 @@ mcp_oauth_callback_port = 5678
|
|||
enforce_residency: None,
|
||||
};
|
||||
|
||||
let err = ConfigBuilder::default()
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.fallback_cwd(Some(codex_home.path().to_path_buf()))
|
||||
.cloud_requirements(CloudRequirementsLoader::new(
|
||||
async move { Some(requirements) },
|
||||
))
|
||||
.build()
|
||||
.await
|
||||
.expect_err("explicit disallowed mode should still fail");
|
||||
assert_eq!(err.kind(), std::io::ErrorKind::InvalidInput);
|
||||
let message = err.to_string();
|
||||
assert!(message.contains("invalid value for `sandbox_mode`"));
|
||||
assert!(message.contains("set by cloud requirements"));
|
||||
.await?;
|
||||
assert_eq!(*config.sandbox_policy.get(), SandboxPolicy::ReadOnly);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
|
@ -4749,7 +4796,7 @@ trust_level = "untrusted"
|
|||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn explicit_approval_policy_still_errors_when_disallowed_by_requirements()
|
||||
async fn explicit_approval_policy_falls_back_when_disallowed_by_requirements()
|
||||
-> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
std::fs::write(
|
||||
|
|
@ -4758,7 +4805,7 @@ trust_level = "untrusted"
|
|||
"#,
|
||||
)?;
|
||||
|
||||
let err = ConfigBuilder::default()
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.fallback_cwd(Some(codex_home.path().to_path_buf()))
|
||||
.cloud_requirements(CloudRequirementsLoader::new(async {
|
||||
|
|
@ -4768,12 +4815,8 @@ trust_level = "untrusted"
|
|||
})
|
||||
}))
|
||||
.build()
|
||||
.await
|
||||
.expect_err("explicit disallowed approval policy should fail");
|
||||
assert_eq!(err.kind(), std::io::ErrorKind::InvalidInput);
|
||||
let message = err.to_string();
|
||||
assert!(message.contains("invalid value for `approval_policy`"));
|
||||
assert!(message.contains("set by cloud requirements"));
|
||||
.await?;
|
||||
assert_eq!(config.approval_policy.value(), AskForApproval::OnRequest);
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1168,6 +1168,37 @@ mod tests {
|
|||
|
||||
#[tokio::test]
|
||||
async fn build_agent_spawn_config_uses_turn_context_values() {
|
||||
fn pick_allowed_approval_policy(
|
||||
constraint: &crate::config::Constrained<AskForApproval>,
|
||||
base: AskForApproval,
|
||||
) -> AskForApproval {
|
||||
let candidates = [
|
||||
AskForApproval::Never,
|
||||
AskForApproval::UnlessTrusted,
|
||||
AskForApproval::OnRequest,
|
||||
AskForApproval::OnFailure,
|
||||
];
|
||||
candidates
|
||||
.into_iter()
|
||||
.find(|candidate| *candidate != base && constraint.can_set(candidate).is_ok())
|
||||
.unwrap_or(base)
|
||||
}
|
||||
|
||||
fn pick_allowed_sandbox_policy(
|
||||
constraint: &crate::config::Constrained<SandboxPolicy>,
|
||||
base: SandboxPolicy,
|
||||
) -> SandboxPolicy {
|
||||
let candidates = [
|
||||
SandboxPolicy::new_read_only_policy(),
|
||||
SandboxPolicy::new_workspace_write_policy(),
|
||||
SandboxPolicy::DangerFullAccess,
|
||||
];
|
||||
candidates
|
||||
.into_iter()
|
||||
.find(|candidate| *candidate != base && constraint.can_set(candidate).is_ok())
|
||||
.unwrap_or(base)
|
||||
}
|
||||
|
||||
let (_session, mut turn) = make_session_and_context().await;
|
||||
let base_instructions = BaseInstructions {
|
||||
text: "base".to_string(),
|
||||
|
|
@ -1181,8 +1212,14 @@ mod tests {
|
|||
let temp_dir = tempfile::tempdir().expect("temp dir");
|
||||
turn.cwd = temp_dir.path().to_path_buf();
|
||||
turn.codex_linux_sandbox_exe = Some(PathBuf::from("/bin/echo"));
|
||||
turn.approval_policy = AskForApproval::Never;
|
||||
turn.sandbox_policy = SandboxPolicy::DangerFullAccess;
|
||||
turn.approval_policy = pick_allowed_approval_policy(
|
||||
&turn.config.approval_policy,
|
||||
*turn.config.approval_policy.get(),
|
||||
);
|
||||
turn.sandbox_policy = pick_allowed_sandbox_policy(
|
||||
&turn.config.sandbox_policy,
|
||||
turn.config.sandbox_policy.get().clone(),
|
||||
);
|
||||
|
||||
let config = build_agent_spawn_config(&base_instructions, &turn, 0).expect("spawn config");
|
||||
let mut expected = (*turn.config).clone();
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue