Merge 055f153e84 into sapling-pr-archive-bolinfest

codex-rs/core/src/config_loader/config_requirements.rs

@@ -25,6 +25,26 @@ pub struct ConfigRequirementsToml {
     pub allowed_approval_policies: Option<Vec<AskForApproval>>,
 }
 
+impl ConfigRequirementsToml {
+    /// For every field in `other` that is `Some`, if the corresponding field in
+    /// `self` is `None`, copy the value from `other` into `self`.
+    pub fn merge_unset_fields(&mut self, mut other: ConfigRequirementsToml) {
+        macro_rules! fill_missing_take {
+            ($base:expr, $other:expr, { $($field:ident),+ $(,)? }) => {
+                $(
+                    if $base.$field.is_none() {
+                        if let Some(value) = $other.$field.take() {
+                            $base.$field = Some(value);
+                        }
+                    }
+                )+
+            };
+        }
+
+        fill_missing_take!(self, other, { allowed_approval_policies });
+    }
+}
+
 impl TryFrom<ConfigRequirementsToml> for ConfigRequirements {
     type Error = ConstraintError;
 
@@ -45,3 +65,43 @@ impl TryFrom<ConfigRequirementsToml> for ConfigRequirements {
         Ok(ConfigRequirements { approval_policy })
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use anyhow::Result;
+    use pretty_assertions::assert_eq;
+    use toml::from_str;
+
+    #[test]
+    fn merge_unset_fields_only_fills_missing_values() -> Result<()> {
+        let source: ConfigRequirementsToml = from_str(
+            r#"
+                allowed_approval_policies = ["on-request"]
+            "#,
+        )?;
+
+        let mut empty_target: ConfigRequirementsToml = from_str(
+            r#"
+                # intentionally left unset
+            "#,
+        )?;
+        empty_target.merge_unset_fields(source.clone());
+        assert_eq!(
+            empty_target.allowed_approval_policies,
+            Some(vec![AskForApproval::OnRequest])
+        );
+
+        let mut populated_target: ConfigRequirementsToml = from_str(
+            r#"
+                allowed_approval_policies = ["never"]
+            "#,
+        )?;
+        populated_target.merge_unset_fields(source);
+        assert_eq!(
+            populated_target.allowed_approval_policies,
+            Some(vec![AskForApproval::Never])
+        );
+        Ok(())
+    }
+}

codex-rs/core/src/config_loader/mod.rs

@@ -11,6 +11,7 @@ mod state;
 mod tests;
 
 use crate::config::CONFIG_TOML_FILE;
+use crate::config_loader::config_requirements::ConfigRequirementsToml;
 use crate::config_loader::layer_io::LoadedConfigLayers;
 use codex_app_server_protocol::ConfigLayerSource;
 use codex_protocol::protocol::AskForApproval;
@@ -26,6 +27,9 @@ pub use state::ConfigLayerEntry;
 pub use state::ConfigLayerStack;
 pub use state::LoaderOverrides;
 
+/// On Unix systems, load requirements from this file path, if present.
+const DEFAULT_REQUIREMENTS_TOML_FILE_UNIX: &str = "/etc/codex/requirements.toml";
+
 /// To build up the set of admin-enforced constraints, we build up from multiple
 /// configuration layers in the following order, but a constraint defined in an
 /// earlier layer cannot be overridden by a later layer:
@@ -55,10 +59,28 @@ pub async fn load_config_layers_state(
     cli_overrides: &[(String, TomlValue)],
     overrides: LoaderOverrides,
 ) -> io::Result<ConfigLayerStack> {
-    let loaded_config_layers = layer_io::load_config_layers_internal(codex_home, overrides).await?;
-    let requirements = load_requirements_from_legacy_scheme(loaded_config_layers.clone()).await?;
+    let mut config_requirements_toml = ConfigRequirementsToml::default();
 
-    // TODO(mbolin): Honor /etc/codex/requirements.toml.
+    // TODO(mbolin): Support an entry in MDM for config requirements and use it
+    // with `config_requirements_toml.merge_unset_fields(...)`, if present.
+
+    // Honor /etc/codex/requirements.toml.
+    if cfg!(unix) {
+        load_requirements_toml(
+            &mut config_requirements_toml,
+            DEFAULT_REQUIREMENTS_TOML_FILE_UNIX,
+        )
+        .await?;
+    }
+
+    // Make a best-effort to support the legacy `managed_config.toml` as a
+    // requirements specification.
+    let loaded_config_layers = layer_io::load_config_layers_internal(codex_home, overrides).await?;
+    load_requirements_from_legacy_scheme(
+        &mut config_requirements_toml,
+        loaded_config_layers.clone(),
+    )
+    .await?;
 
     let mut layers = Vec::<ConfigLayerEntry>::new();
 
@@ -133,23 +155,61 @@ pub async fn load_config_layers_state(
         ));
     }
 
-    ConfigLayerStack::new(layers, requirements)
+    ConfigLayerStack::new(layers, config_requirements_toml.try_into()?)
+}
+
+/// If available, apply requirements from `/etc/codex/requirements.toml` to
+/// `config_requirements_toml` by filling in any unset fields.
+async fn load_requirements_toml(
+    config_requirements_toml: &mut ConfigRequirementsToml,
+    requirements_toml_file: impl AsRef<Path>,
+) -> io::Result<()> {
+    if cfg!(unix) {
+        match tokio::fs::read_to_string(&requirements_toml_file).await {
+            Ok(contents) => {
+                let requirements_config: ConfigRequirementsToml = toml::from_str(&contents)
+                    .map_err(|e| {
+                        io::Error::new(
+                            io::ErrorKind::InvalidData,
+                            format!(
+                                "Error parsing requirements file {}: {e}",
+                                requirements_toml_file.as_ref().display(),
+                            ),
+                        )
+                    })?;
+                config_requirements_toml.merge_unset_fields(requirements_config);
+            }
+            Err(e) => {
+                if e.kind() != io::ErrorKind::NotFound {
+                    return Err(io::Error::new(
+                        e.kind(),
+                        format!(
+                            "Failed to read requirements file {}: {e}",
+                            requirements_toml_file.as_ref().display(),
+                        ),
+                    ));
+                }
+            }
+        }
+    }
+
+    Ok(())
 }
 
 async fn load_requirements_from_legacy_scheme(
+    config_requirements_toml: &mut ConfigRequirementsToml,
     loaded_config_layers: LoadedConfigLayers,
-) -> io::Result<ConfigRequirements> {
-    let mut config_requirements = ConfigRequirements::default();
-
-    // In this implementation, later layers override earlier layers, so list
-    // managed_config_from_mdm last because it has the highest precedence.
+) -> io::Result<()> {
+    // In this implementation, earlier layers cannot be overwritten by later
+    // layers, so list managed_config_from_mdm first because it has the highest
+    // precedence.
     let LoadedConfigLayers {
         managed_config,
         managed_config_from_mdm,
     } = loaded_config_layers;
     for config in [
-        managed_config.map(|c| c.managed_config),
         managed_config_from_mdm,
+        managed_config.map(|c| c.managed_config),
     ]
     .into_iter()
     .flatten()
@@ -162,14 +222,11 @@ async fn load_requirements_from_legacy_scheme(
                 )
             })?;
 
-        let LegacyManagedConfigToml { approval_policy } = legacy_config;
-        if let Some(approval_policy) = approval_policy {
-            config_requirements.approval_policy =
-                crate::config::Constrained::allow_only(approval_policy);
-        }
+        let new_requirements_toml = ConfigRequirementsToml::from(legacy_config);
+        config_requirements_toml.merge_unset_fields(new_requirements_toml);
     }
 
-    Ok(config_requirements)
+    Ok(())
 }
 
 /// The legacy mechanism for specifying admin-enforced configuration is to read
@@ -184,3 +241,16 @@ async fn load_requirements_from_legacy_scheme(
 struct LegacyManagedConfigToml {
     approval_policy: Option<AskForApproval>,
 }
+
+impl From<LegacyManagedConfigToml> for ConfigRequirementsToml {
+    fn from(legacy: LegacyManagedConfigToml) -> Self {
+        let mut config_requirements_toml = ConfigRequirementsToml::default();
+
+        let LegacyManagedConfigToml { approval_policy } = legacy;
+        if let Some(approval_policy) = approval_policy {
+            config_requirements_toml.allowed_approval_policies = Some(vec![approval_policy]);
+        }
+
+        config_requirements_toml
+    }
+}

codex-rs/core/src/config_loader/tests.rs

@@ -1,6 +1,11 @@
 use super::LoaderOverrides;
 use super::load_config_layers_state;
 use crate::config::CONFIG_TOML_FILE;
+use crate::config_loader::ConfigRequirements;
+use crate::config_loader::config_requirements::ConfigRequirementsToml;
+use crate::config_loader::load_requirements_toml;
+use codex_protocol::protocol::AskForApproval;
+use pretty_assertions::assert_eq;
 use tempfile::tempdir;
 use toml::Value as TomlValue;
 
@@ -147,3 +152,40 @@ flag = true
     );
     assert_eq!(nested.get("flag"), Some(&TomlValue::Boolean(false)));
 }
+
+#[tokio::test(flavor = "current_thread")]
+async fn load_requirements_toml_produces_expected_constraints() -> anyhow::Result<()> {
+    let tmp = tempdir()?;
+    let requirements_file = tmp.path().join("requirements.toml");
+    tokio::fs::write(
+        &requirements_file,
+        r#"
+allowed_approval_policies = ["never", "on-request"]
+"#,
+    )
+    .await?;
+
+    let mut config_requirements_toml = ConfigRequirementsToml::default();
+    load_requirements_toml(&mut config_requirements_toml, &requirements_file).await?;
+
+    assert_eq!(
+        config_requirements_toml.allowed_approval_policies,
+        Some(vec![AskForApproval::Never, AskForApproval::OnRequest])
+    );
+
+    let config_requirements: ConfigRequirements = config_requirements_toml.try_into()?;
+    assert_eq!(
+        config_requirements.approval_policy.value(),
+        AskForApproval::OnRequest
+    );
+    config_requirements
+        .approval_policy
+        .can_set(&AskForApproval::Never)?;
+    assert!(
+        config_requirements
+            .approval_policy
+            .can_set(&AskForApproval::OnFailure)
+            .is_err()
+    );
+    Ok(())
+}