[refactor]: Refactor sandbox configuration setup in IssueResolver class (#8414)

openhands/resolver/resolve_issue.py

@@ -71,20 +71,12 @@ class IssueResolver:
             base_domain: The base domain for the git server.
         """
 
-        base_container_image = args.base_container_image
-        runtime_container_image = args.runtime_container_image
-
-        if runtime_container_image is not None and base_container_image is not None:
-            raise ValueError('Cannot provide both runtime and base container images.')
-
-        if (
-            runtime_container_image is None
-            and base_container_image is None
-            and not args.is_experimental
-        ):
-            runtime_container_image = (
-                f'ghcr.io/all-hands-ai/runtime:{openhands.__version__}-nikolaik'
-            )
+        # Setup and validate container images
+        self.sandbox_config = self._setup_sandbox_config(
+            args.base_container_image,
+            args.runtime_container_image,
+            args.is_experimental,
+        )
 
         parts = args.selected_repo.rsplit('/', 1)
         if len(parts) < 2:
@@ -162,8 +154,6 @@ class IssueResolver:
         self.owner = owner
         self.repo = repo
         self.platform = platform
-        self.runtime_container_image = runtime_container_image
-        self.base_container_image = base_container_image
         self.max_iterations = args.max_iterations
         self.output_dir = args.output_dir
         self.llm_config = llm_config
@@ -172,7 +162,6 @@ class IssueResolver:
         self.repo_instruction = repo_instruction
         self.issue_number = args.issue_number
         self.comment_id = args.comment_id
-        self.platform = platform
 
         factory = IssueHandlerFactory(
             owner=self.owner,
@@ -186,6 +175,54 @@ class IssueResolver:
         )
         self.issue_handler = factory.create()
 
+    @classmethod
+    def _setup_sandbox_config(
+        cls,
+        base_container_image: str | None,
+        runtime_container_image: str | None,
+        is_experimental: bool,
+    ) -> SandboxConfig:
+        if runtime_container_image is not None and base_container_image is not None:
+            raise ValueError('Cannot provide both runtime and base container images.')
+
+        if (
+            runtime_container_image is None
+            and base_container_image is None
+            and not is_experimental
+        ):
+            runtime_container_image = (
+                f'ghcr.io/all-hands-ai/runtime:{openhands.__version__}-nikolaik'
+            )
+
+        # Convert container image values to string or None
+        container_base = (
+            str(base_container_image) if base_container_image is not None else None
+        )
+        container_runtime = (
+            str(runtime_container_image)
+            if runtime_container_image is not None
+            else None
+        )
+
+        sandbox_config = SandboxConfig(
+            base_container_image=container_base,
+            runtime_container_image=container_runtime,
+            enable_auto_lint=False,
+            use_host_network=False,
+            timeout=300,
+        )
+
+        # Configure sandbox for GitLab CI environment
+        if cls.GITLAB_CI:
+            sandbox_config.local_runtime_url = os.getenv(
+                'LOCAL_RUNTIME_URL', 'http://localhost'
+            )
+            user_id = os.getuid() if hasattr(os, 'getuid') else 1000
+            if user_id == 0:
+                sandbox_config.user_id = get_unique_uid()
+
+        return sandbox_config
+
     def initialize_runtime(
         self,
         runtime: Runtime,
@@ -324,32 +361,12 @@ class IssueResolver:
             shutil.rmtree(workspace_base)
         shutil.copytree(os.path.join(self.output_dir, 'repo'), workspace_base)
 
-        # This code looks unnecessary because these are default values in the config class
-        # they're set by default if nothing else overrides them
-        # FIXME we should remove them here
-        sandbox_config = SandboxConfig(
-            base_container_image=self.base_container_image,
-            runtime_container_image=self.runtime_container_image,
-            enable_auto_lint=False,
-            use_host_network=False,
-            # large enough timeout, since some testcases take very long to run
-            timeout=300,
-        )
-
-        if os.getenv('GITLAB_CI') == 'true':
-            sandbox_config.local_runtime_url = os.getenv(
-                'LOCAL_RUNTIME_URL', 'http://localhost'
-            )
-            user_id = os.getuid() if hasattr(os, 'getuid') else 1000
-            if user_id == 0:
-                sandbox_config.user_id = get_unique_uid()
-
         config = AppConfig(
             default_agent='CodeActAgent',
             runtime='docker',
             max_budget_per_task=4,
             max_iterations=self.max_iterations,
-            sandbox=sandbox_config,
+            sandbox=self.sandbox_config,
             # do not mount workspace
             workspace_base=workspace_base,
             workspace_mount_path=workspace_base,

tests/unit/resolver/test_resolve_issue.py

@@ -0,0 +1,119 @@
+import os
+from unittest import mock
+
+import pytest
+
+from openhands.core.config import SandboxConfig
+from openhands.resolver.resolve_issue import IssueResolver
+import openhands
+
+def assert_sandbox_config(
+    config: SandboxConfig,
+    base_container_image = SandboxConfig.model_fields["base_container_image"].default,
+    runtime_container_image = "ghcr.io/all-hands-ai/runtime:0.38.0-nikolaik",
+    local_runtime_url = SandboxConfig.model_fields["local_runtime_url"].default,
+    user_id = SandboxConfig.model_fields["user_id"].default,
+):
+    """Helper function to assert the properties of the SandboxConfig object."""
+    assert isinstance(config, SandboxConfig)
+    assert config.base_container_image == base_container_image
+    assert config.runtime_container_image == runtime_container_image
+    assert config.enable_auto_lint is False
+    assert config.use_host_network is False
+    assert config.timeout == 300
+    assert config.local_runtime_url == local_runtime_url
+    assert config.user_id == user_id
+
+def test_setup_sandbox_config_default():
+    """Test default configuration when no images provided and not experimental"""
+    config = IssueResolver._setup_sandbox_config(
+        base_container_image=None,
+        runtime_container_image=None,
+        is_experimental=False,
+    )
+
+    assert_sandbox_config(config)
+
+def test_setup_sandbox_config_both_images():
+    """Test that providing both container images raises ValueError"""
+    with pytest.raises(ValueError, match="Cannot provide both runtime and base container images."):
+        IssueResolver._setup_sandbox_config(
+            base_container_image="base-image",
+            runtime_container_image="runtime-image",
+            is_experimental=False,
+        )
+
+def test_setup_sandbox_config_base_only():
+    """Test configuration when only base_container_image is provided"""
+    base_image = "custom-base-image"
+    config = IssueResolver._setup_sandbox_config(
+        base_container_image=base_image,
+        runtime_container_image=None,
+        is_experimental=False,
+    )
+
+    assert_sandbox_config(
+        config,
+        base_container_image=base_image,
+        runtime_container_image=None
+    )
+
+def test_setup_sandbox_config_runtime_only():
+    """Test configuration when only runtime_container_image is provided"""
+    runtime_image = "custom-runtime-image"
+    config = IssueResolver._setup_sandbox_config(
+        base_container_image=None,
+        runtime_container_image=runtime_image,
+        is_experimental=False,
+    )
+
+    assert_sandbox_config(
+        config,
+        runtime_container_image=runtime_image
+    )
+ 
+def test_setup_sandbox_config_experimental():
+    """Test configuration when experimental mode is enabled"""
+    config = IssueResolver._setup_sandbox_config(
+        base_container_image=None,
+        runtime_container_image=None,
+        is_experimental=True,
+    )
+
+    assert_sandbox_config(
+        config,
+        runtime_container_image=None
+    )
+
+@mock.patch("openhands.resolver.resolve_issue.os.getuid", return_value=0)
+@mock.patch("openhands.resolver.resolve_issue.get_unique_uid", return_value=1001)
+def test_setup_sandbox_config_gitlab_ci(mock_get_unique_uid, mock_getuid):
+    """Test GitLab CI specific configuration when running as root"""
+    with mock.patch.object(IssueResolver, "GITLAB_CI", True):
+        config = IssueResolver._setup_sandbox_config(
+            base_container_image=None,
+            runtime_container_image=None,
+            is_experimental=False,
+        )
+        
+        assert_sandbox_config(
+            config,
+            local_runtime_url="http://localhost",
+            user_id=1001
+        )
+
+@mock.patch("openhands.resolver.resolve_issue.os.getuid", return_value=1000)
+def test_setup_sandbox_config_gitlab_ci_non_root(mock_getuid):
+    """Test GitLab CI configuration when not running as root"""
+    with mock.patch.object(IssueResolver, "GITLAB_CI", True):
+        config = IssueResolver._setup_sandbox_config(
+            base_container_image=None,
+            runtime_container_image=None,
+            is_experimental=False,
+        )
+
+        assert_sandbox_config(
+            config,
+            local_runtime_url="http://localhost",
+            user_id=1000
+        )