Refactor sandbox and security configurations (#6973)

openhands/core/config/sandbox_config.py

@@ -1,6 +1,6 @@
 import os
 
-from pydantic import BaseModel, Field
+from pydantic import BaseModel, Field, ValidationError
 
 
 class SandboxConfig(BaseModel):
@@ -74,3 +74,24 @@ class SandboxConfig(BaseModel):
     selected_repo: str | None = Field(default=None)
 
     model_config = {'extra': 'forbid'}
+
+    @classmethod
+    def from_toml_section(cls, data: dict) -> dict[str, 'SandboxConfig']:
+        """
+        Create a mapping of SandboxConfig instances from a toml dictionary representing the [sandbox] section.
+
+        The configuration is built from all keys in data.
+
+        Returns:
+            dict[str, SandboxConfig]: A mapping where the key "sandbox" corresponds to the [sandbox] configuration
+        """
+        # Initialize the result mapping
+        sandbox_mapping: dict[str, SandboxConfig] = {}
+
+        # Try to create the configuration instance
+        try:
+            sandbox_mapping['sandbox'] = cls.model_validate(data)
+        except ValidationError as e:
+            raise ValueError(f'Invalid sandbox configuration: {e}')
+
+        return sandbox_mapping

openhands/core/config/security_config.py

@@ -1,4 +1,4 @@
-from pydantic import BaseModel, Field
+from pydantic import BaseModel, Field, ValidationError
 
 
 class SecurityConfig(BaseModel):
@@ -11,3 +11,27 @@ class SecurityConfig(BaseModel):
 
     confirmation_mode: bool = Field(default=False)
     security_analyzer: str | None = Field(default=None)
+
+    model_config = {'extra': 'forbid'}
+
+    @classmethod
+    def from_toml_section(cls, data: dict) -> dict[str, 'SecurityConfig']:
+        """
+        Create a mapping of SecurityConfig instances from a toml dictionary representing the [security] section.
+
+        The configuration is built from all keys in data.
+
+        Returns:
+            dict[str, SecurityConfig]: A mapping where the key "security" corresponds to the [security] configuration
+        """
+
+        # Initialize the result mapping
+        security_mapping: dict[str, SecurityConfig] = {}
+
+        # Try to create the configuration instance
+        try:
+            security_mapping['security'] = cls.model_validate(data)
+        except ValidationError as e:
+            raise ValueError(f'Invalid security configuration: {e}')
+
+        return security_mapping

openhands/core/config/utils.py

@@ -166,28 +166,32 @@ def load_from_toml(cfg: AppConfig, toml_file: str = 'config.toml') -> None:
     # Process security section if present
     if 'security' in toml_config:
         try:
-            logger.openhands_logger.debug(
-                'Attempt to load security config from config toml'
-            )
-            security_config = SecurityConfig(**toml_config['security'])
-            cfg.security = security_config
+            security_mapping = SecurityConfig.from_toml_section(toml_config['security'])
+            # We only use the base security config for now
+            if 'security' in security_mapping:
+                cfg.security = security_mapping['security']
         except (TypeError, KeyError, ValidationError) as e:
             logger.openhands_logger.warning(
                 f'Cannot parse [security] config from toml, values have not been applied.\nError: {e}'
             )
+        except ValueError:
+            # Re-raise ValueError from SecurityConfig.from_toml_section
+            raise ValueError('Error in [security] section in config.toml')
 
     # Process sandbox section if present
     if 'sandbox' in toml_config:
         try:
-            logger.openhands_logger.debug(
-                'Attempt to load sandbox config from config toml'
-            )
-            sandbox_config = SandboxConfig(**toml_config['sandbox'])
-            cfg.sandbox = sandbox_config
+            sandbox_mapping = SandboxConfig.from_toml_section(toml_config['sandbox'])
+            # We only use the base sandbox config for now
+            if 'sandbox' in sandbox_mapping:
+                cfg.sandbox = sandbox_mapping['sandbox']
         except (TypeError, KeyError, ValidationError) as e:
             logger.openhands_logger.warning(
                 f'Cannot parse [sandbox] config from toml, values have not been applied.\nError: {e}'
             )
+        except ValueError:
+            # Re-raise ValueError from SandboxConfig.from_toml_section
+            raise ValueError('Error in [sandbox] section in config.toml')
 
     # Process extended section if present
     if 'extended' in toml_config:

tests/unit/test_config.py

@@ -460,11 +460,8 @@ def test_load_from_toml_partial_invalid(default_config, temp_toml_file, caplog):
 
     This ensures that:
     1. Valid configuration sections are properly loaded
-    2. Invalid fields are ignored gracefully
-    3. The config object maintains correct values for valid fields
-    4. Appropriate warnings are logged for invalid fields
-
-    See `openhands/core/schema/config.py` for the list of valid fields.
+    2. Invalid fields in security and sandbox sections raise ValueError
+    4. The config object maintains correct values for valid fields
     """
     with open(temp_toml_file, 'w', encoding='utf-8') as f:
         f.write("""
@@ -472,7 +469,7 @@ def test_load_from_toml_partial_invalid(default_config, temp_toml_file, caplog):
 debug = true
 
 [llm]
-# No set in `openhands/core/schema/config.py`
+# Not set in `openhands/core/schema/config.py`
 invalid_field = "test"
 model = "gpt-4"
 
@@ -484,7 +481,6 @@ invalid_field_in_sandbox = "test"
 """)
 
     # Create a string buffer to capture log output
-    # Referenced from test_logging.py and `mock_logger`
     log_output = StringIO()
     handler = logging.StreamHandler(log_output)
     handler.setLevel(logging.WARNING)
@@ -493,31 +489,42 @@ invalid_field_in_sandbox = "test"
     openhands_logger.addHandler(handler)
 
     try:
-        load_from_toml(default_config, temp_toml_file)
+        # Since sandbox_config.from_toml_section now raises ValueError for invalid fields,
+        # we need to catch that exception
+        with pytest.raises(ValueError) as excinfo:
+            load_from_toml(default_config, temp_toml_file)
+
+        # Verify the error message mentions the invalid sandbox field
+        assert 'Error in [sandbox] section in config.toml' in str(excinfo.value)
+
         log_content = log_output.getvalue()
 
-        # invalid [llm] config
-        # Verify that the appropriate warning was logged
+        # The LLM config should still log a warning but not raise an exception
         assert 'Cannot parse [llm] config from toml' in log_content
-        assert 'values have not been applied' in log_content
-        # Error: LLMConfig.__init__() got an unexpected keyword argume
-        assert 'Cannot parse [llm] config from toml' in log_content
-        assert 'invalid_field' in log_content
 
-        # invalid [sandbox] config
-        assert 'Cannot parse [sandbox] config from toml' in log_content
-        assert 'values have not been applied' in log_content
-        assert 'invalid_field_in_sandbox' in log_content
-
-        # Verify valid configurations are loaded. Load from default instead of `config.toml`
-        # assert default_config.debug is True
+        # Verify valid configurations are loaded before the error was raised
         assert default_config.debug is True
-        assert default_config.get_llm_config().model == 'claude-3-5-sonnet-20241022'
-        assert default_config.get_agent_config().memory_enabled is True
     finally:
         openhands_logger.removeHandler(handler)
 
 
+def test_load_from_toml_security_invalid(default_config, temp_toml_file):
+    """Test that invalid security configuration raises ValueError."""
+    with open(temp_toml_file, 'w', encoding='utf-8') as f:
+        f.write("""
+[core]
+debug = true
+
+[security]
+invalid_security_field = "test"
+""")
+
+    with pytest.raises(ValueError) as excinfo:
+        load_from_toml(default_config, temp_toml_file)
+
+    assert 'Error in [security] section in config.toml' in str(excinfo.value)
+
+
 def test_finalize_config(default_config):
     # Test finalize config
     assert default_config.workspace_mount_path is None