From 338374f21cef8fa7a024b2f89c7fa57fb5173668 Mon Sep 17 00:00:00 2001 From: gt-oai Date: Thu, 5 Feb 2026 13:03:11 +0000 Subject: [PATCH] Add feedback flag to requirements --- codex-rs/app-server/src/config_api.rs | 1 + codex-rs/cloud-requirements/src/lib.rs | 3 + codex-rs/core/src/config/mod.rs | 80 +++++++++++++++++-- .../src/config_loader/config_requirements.rs | 69 ++++++++++++++++ codex-rs/core/src/config_loader/tests.rs | 3 + codex-rs/tui/src/debug_config.rs | 14 ++++ 6 files changed, 165 insertions(+), 5 deletions(-) diff --git a/codex-rs/app-server/src/config_api.rs b/codex-rs/app-server/src/config_api.rs index 5a43d6e528..59dd446c87 100644 --- a/codex-rs/app-server/src/config_api.rs +++ b/codex-rs/app-server/src/config_api.rs @@ -157,6 +157,7 @@ mod tests { mcp_servers: None, rules: None, enforce_residency: Some(CoreResidencyRequirement::Us), + feedback_enabled: None, }; let mapped = map_requirements_toml_to_api(requirements); diff --git a/codex-rs/cloud-requirements/src/lib.rs b/codex-rs/cloud-requirements/src/lib.rs index 9ca432dc7f..f113330408 100644 --- a/codex-rs/cloud-requirements/src/lib.rs +++ b/codex-rs/cloud-requirements/src/lib.rs @@ -384,6 +384,7 @@ mod tests { mcp_servers: None, rules: None, enforce_residency: None, + feedback_enabled: None, }) ); } @@ -424,6 +425,7 @@ mod tests { mcp_servers: None, rules: None, enforce_residency: None, + feedback_enabled: None, }) ); } @@ -467,6 +469,7 @@ mod tests { mcp_servers: None, rules: None, enforce_residency: None, + feedback_enabled: None, }) ); assert_eq!(fetcher.request_count.load(Ordering::SeqCst), 2); diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 94e36eb90b..6faecc1189 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -1585,6 +1585,7 @@ impl Config { mcp_servers, exec_policy: _, enforce_residency, + feedback_enabled: mut constrained_feedback_enabled, } = requirements; constrained_approval_policy @@ -1593,6 +1594,28 @@ impl Config { constrained_sandbox_policy .set(sandbox_policy) .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidInput, format!("{e}")))?; + let feedback_enabled_was_explicit = cfg + .feedback + .as_ref() + .and_then(|feedback| feedback.enabled) + .is_some(); + let mut feedback_enabled = cfg + .feedback + .as_ref() + .and_then(|feedback| feedback.enabled) + .unwrap_or(true); + if !feedback_enabled_was_explicit + && let Err(err) = constrained_feedback_enabled.can_set(&feedback_enabled) + { + tracing::warn!( + error = %err, + "default feedback.enabled is disallowed by requirements; falling back to required default" + ); + feedback_enabled = constrained_feedback_enabled.value(); + } + constrained_feedback_enabled + .set(feedback_enabled) + .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidInput, format!("{e}")))?; 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}")))?; @@ -1689,11 +1712,7 @@ impl Config { .as_ref() .and_then(|a| a.enabled) .or(cfg.analytics.as_ref().and_then(|a| a.enabled)), - feedback_enabled: cfg - .feedback - .as_ref() - .and_then(|feedback| feedback.enabled) - .unwrap_or(true), + feedback_enabled: constrained_feedback_enabled.value(), tui_notifications: cfg .tui .as_ref() @@ -4701,6 +4720,7 @@ mcp_oauth_callback_port = 5678 mcp_servers: None, rules: None, enforce_residency: None, + feedback_enabled: None, }; let err = ConfigBuilder::default() @@ -4779,6 +4799,56 @@ trust_level = "untrusted" assert!(message.contains("set by cloud requirements")); Ok(()) } + + #[tokio::test] + async fn requirements_disallowing_default_feedback_falls_back_to_required_default() + -> std::io::Result<()> { + let codex_home = TempDir::new()?; + + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .cloud_requirements(CloudRequirementsLoader::new(async { + Some(crate::config_loader::ConfigRequirementsToml { + feedback_enabled: Some(false), + ..Default::default() + }) + })) + .build() + .await?; + + assert!(!config.feedback_enabled); + Ok(()) + } + + #[tokio::test] + async fn explicit_feedback_enabled_still_errors_when_disallowed_by_requirements() + -> std::io::Result<()> { + let codex_home = TempDir::new()?; + std::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + r#"[feedback] +enabled = true +"#, + )?; + + let err = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .fallback_cwd(Some(codex_home.path().to_path_buf())) + .cloud_requirements(CloudRequirementsLoader::new(async { + Some(crate::config_loader::ConfigRequirementsToml { + feedback_enabled: Some(false), + ..Default::default() + }) + })) + .build() + .await + .expect_err("explicit feedback.enabled=true should fail"); + assert_eq!(err.kind(), std::io::ErrorKind::InvalidInput); + let message = err.to_string(); + assert!(message.contains("invalid value for `feedback.enabled`")); + assert!(message.contains("set by cloud requirements")); + Ok(()) + } } #[cfg(test)] diff --git a/codex-rs/core/src/config_loader/config_requirements.rs b/codex-rs/core/src/config_loader/config_requirements.rs index b3e6043288..1654f2faed 100644 --- a/codex-rs/core/src/config_loader/config_requirements.rs +++ b/codex-rs/core/src/config_loader/config_requirements.rs @@ -79,6 +79,7 @@ pub struct ConfigRequirements { pub mcp_servers: Option>>, pub(crate) exec_policy: Option>, pub enforce_residency: ConstrainedWithSource>, + pub feedback_enabled: ConstrainedWithSource, } impl Default for ConfigRequirements { @@ -95,6 +96,7 @@ impl Default for ConfigRequirements { mcp_servers: None, exec_policy: None, enforce_residency: ConstrainedWithSource::new(Constrained::allow_any(None), None), + feedback_enabled: ConstrainedWithSource::new(Constrained::allow_any(true), None), } } } @@ -125,6 +127,7 @@ pub struct ConfigRequirementsToml { pub mcp_servers: Option>, pub rules: Option, pub enforce_residency: Option, + pub feedback_enabled: Option, } /// Value paired with the requirement source it came from, for better error @@ -156,6 +159,7 @@ pub struct ConfigRequirementsWithSources { pub mcp_servers: Option>>, pub rules: Option>, pub enforce_residency: Option>, + pub feedback_enabled: Option>, } impl ConfigRequirementsWithSources { @@ -189,6 +193,7 @@ impl ConfigRequirementsWithSources { mcp_servers, rules, enforce_residency, + feedback_enabled, } ); } @@ -200,6 +205,7 @@ impl ConfigRequirementsWithSources { mcp_servers, rules, enforce_residency, + feedback_enabled, } = self; ConfigRequirementsToml { allowed_approval_policies: allowed_approval_policies.map(|sourced| sourced.value), @@ -207,6 +213,7 @@ impl ConfigRequirementsWithSources { mcp_servers: mcp_servers.map(|sourced| sourced.value), rules: rules.map(|sourced| sourced.value), enforce_residency: enforce_residency.map(|sourced| sourced.value), + feedback_enabled: feedback_enabled.map(|sourced| sourced.value), } } } @@ -251,6 +258,7 @@ impl ConfigRequirementsToml { && self.mcp_servers.is_none() && self.rules.is_none() && self.enforce_residency.is_none() + && self.feedback_enabled.is_none() } } @@ -264,6 +272,7 @@ impl TryFrom for ConfigRequirements { mcp_servers, rules, enforce_residency, + feedback_enabled, } = toml; let approval_policy = match allowed_approval_policies { @@ -380,12 +389,35 @@ impl TryFrom for ConfigRequirements { } None => ConstrainedWithSource::new(Constrained::allow_any(None), None), }; + let feedback_enabled = match feedback_enabled { + Some(Sourced { + value: feedback_enabled, + source: requirement_source, + }) => { + let requirement_source_for_error = requirement_source.clone(); + let constrained = Constrained::new(feedback_enabled, move |candidate| { + if candidate == &feedback_enabled { + Ok(()) + } else { + Err(ConstraintError::InvalidValue { + field_name: "feedback.enabled", + candidate: format!("{candidate:?}"), + allowed: format!("{feedback_enabled:?}"), + requirement_source: requirement_source_for_error.clone(), + }) + } + })?; + ConstrainedWithSource::new(constrained, Some(requirement_source)) + } + None => ConstrainedWithSource::new(Constrained::allow_any(true), None), + }; Ok(ConfigRequirements { approval_policy, sandbox_policy, mcp_servers, exec_policy, enforce_residency, + feedback_enabled, }) } } @@ -413,6 +445,7 @@ mod tests { mcp_servers, rules, enforce_residency, + feedback_enabled, } = toml; ConfigRequirementsWithSources { allowed_approval_policies: allowed_approval_policies @@ -423,6 +456,8 @@ mod tests { rules: rules.map(|value| Sourced::new(value, RequirementSource::Unknown)), enforce_residency: enforce_residency .map(|value| Sourced::new(value, RequirementSource::Unknown)), + feedback_enabled: feedback_enabled + .map(|value| Sourced::new(value, RequirementSource::Unknown)), } } @@ -437,7 +472,9 @@ mod tests { SandboxModeRequirement::DangerFullAccess, ]; let enforce_residency = ResidencyRequirement::Us; + let feedback_enabled = false; let enforce_source = source.clone(); + let feedback_source = source.clone(); // Intentionally constructed without `..Default::default()` so adding a new field to // `ConfigRequirementsToml` forces this test to be updated. @@ -447,6 +484,7 @@ mod tests { mcp_servers: None, rules: None, enforce_residency: Some(enforce_residency), + feedback_enabled: Some(feedback_enabled), }; target.merge_unset_fields(source.clone(), other); @@ -462,6 +500,7 @@ mod tests { mcp_servers: None, rules: None, enforce_residency: Some(Sourced::new(enforce_residency, enforce_source)), + feedback_enabled: Some(Sourced::new(feedback_enabled, feedback_source)), } ); } @@ -492,6 +531,7 @@ mod tests { mcp_servers: None, rules: None, enforce_residency: None, + feedback_enabled: None, } ); Ok(()) @@ -530,6 +570,7 @@ mod tests { mcp_servers: None, rules: None, enforce_residency: None, + feedback_enabled: None, } ); Ok(()) @@ -616,6 +657,7 @@ mod tests { allowed_approval_policies = ["on-request"] allowed_sandbox_modes = ["read-only"] enforce_residency = "us" + feedback_enabled = false "#, )?; @@ -633,6 +675,33 @@ mod tests { Some(source_location.clone()) ); assert_eq!(requirements.enforce_residency.source, Some(source_location)); + assert_eq!( + requirements.feedback_enabled.source, + Some(RequirementSource::CloudRequirements) + ); + + Ok(()) + } + + #[test] + fn deserialize_feedback_enabled() -> Result<()> { + let toml_str = r#" + feedback_enabled = false + "#; + let config: ConfigRequirementsToml = from_str(toml_str)?; + let requirements: ConfigRequirements = with_unknown_source(config).try_into()?; + + assert_eq!(requirements.feedback_enabled.value(), false); + assert_eq!( + requirements.feedback_enabled.can_set(&true), + Err(ConstraintError::InvalidValue { + field_name: "feedback.enabled", + candidate: "true".into(), + allowed: "false".into(), + requirement_source: RequirementSource::Unknown, + }) + ); + assert!(requirements.feedback_enabled.can_set(&false).is_ok()); Ok(()) } diff --git a/codex-rs/core/src/config_loader/tests.rs b/codex-rs/core/src/config_loader/tests.rs index d68093c30f..2dafa10075 100644 --- a/codex-rs/core/src/config_loader/tests.rs +++ b/codex-rs/core/src/config_loader/tests.rs @@ -539,6 +539,7 @@ allowed_approval_policies = ["on-request"] mcp_servers: None, rules: None, enforce_residency: None, + feedback_enabled: None, }) }), ) @@ -585,6 +586,7 @@ allowed_approval_policies = ["on-request"] mcp_servers: None, rules: None, enforce_residency: None, + feedback_enabled: None, }, ); load_requirements_toml(&mut config_requirements_toml, &requirements_file).await?; @@ -620,6 +622,7 @@ async fn load_config_layers_includes_cloud_requirements() -> anyhow::Result<()> mcp_servers: None, rules: None, enforce_residency: None, + feedback_enabled: None, }; let expected = requirements.clone(); let cloud_requirements = CloudRequirementsLoader::new(async move { Some(requirements) }); diff --git a/codex-rs/tui/src/debug_config.rs b/codex-rs/tui/src/debug_config.rs index 4202264fb2..0336a196d7 100644 --- a/codex-rs/tui/src/debug_config.rs +++ b/codex-rs/tui/src/debug_config.rs @@ -99,6 +99,14 @@ fn render_debug_config_lines(stack: &ConfigLayerStack) -> Vec> { )); } + if let Some(feedback_enabled) = requirements_toml.feedback_enabled { + requirement_lines.push(requirement_line( + "feedback_enabled", + feedback_enabled.to_string(), + requirements.feedback_enabled.source.as_ref(), + )); + } + if requirement_lines.is_empty() { lines.push(" ".dim().into()); } else { @@ -287,6 +295,10 @@ mod tests { Constrained::allow_any(Some(ResidencyRequirement::Us)), Some(RequirementSource::CloudRequirements), ); + requirements.feedback_enabled = ConstrainedWithSource::new( + Constrained::allow_any(false), + Some(RequirementSource::CloudRequirements), + ); let requirements_toml = ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::OnRequest]), @@ -301,6 +313,7 @@ mod tests { )])), rules: None, enforce_residency: Some(ResidencyRequirement::Us), + feedback_enabled: Some(false), }; let user_file = if cfg!(windows) { @@ -333,6 +346,7 @@ mod tests { ); assert!(rendered.contains("mcp_servers: docs (source: MDM managed_config.toml (legacy))")); assert!(rendered.contains("enforce_residency: us (source: cloud requirements)")); + assert!(rendered.contains("feedback_enabled: false (source: cloud requirements)")); assert!(!rendered.contains(" - rules:")); } }