fix: address review feedback for health_check_timeout

- Add gt=0.0 validator to reject zero/negative timeouts
- Remove dead default from _wait_for_health methods (make timeout required)
- Fix test_docker_workspace_health_check_timeout_custom to actually verify
  _wait_for_health is called with custom timeout (blocker)
- Add test_docker_workspace_startup_uses_health_check_timeout for _start_container
  path (significant gap)
- Collapse redundant *_field_exists/*_default tests into parametrized tests
- Add validation tests for non-positive timeout values

Co-authored-by: openhands <openhands@all-hands.dev>

Author: openhands-agent

openhands-workspace/openhands/workspace/apptainer/workspace.py

@@ -130,6 +130,7 @@ class ApptainerWorkspace(RemoteWorkspace):
     )
     health_check_timeout: float = Field(
         default=120.0,
+        gt=0.0,
         description="Timeout in seconds to wait for container health check to pass.",
     )
 
@@ -326,7 +327,7 @@ def _stream_logs(self) -> None:
             except Exception:
                 pass
 
-    def _wait_for_health(self, timeout: float = 120.0) -> None:
+    def _wait_for_health(self, *, timeout: float) -> None:
         """Wait for the container to become healthy."""
         start = time.time()
         health_url = f"http://127.0.0.1:{self.host_port}/health"

openhands-workspace/openhands/workspace/docker/workspace.py

@@ -123,6 +123,7 @@ class DockerWorkspace(RemoteWorkspace):
     )
     health_check_timeout: float = Field(
         default=120.0,
+        gt=0.0,
         description="Timeout in seconds to wait for container health check to pass.",
     )
 
@@ -313,7 +314,7 @@ def _stream_docker_logs(self) -> None:
             except Exception:
                 pass
 
-    def _wait_for_health(self, timeout: float = 120.0) -> None:
+    def _wait_for_health(self, *, timeout: float) -> None:
         """Wait for the Docker container to become healthy."""
         start = time.time()
         # We can construct the health URL based on self.host if available,

tests/workspace/test_docker_workspace.py

@@ -7,6 +7,7 @@
 from unittest.mock import MagicMock, Mock, patch
 
 import pytest
+from pydantic import ValidationError
 
 from openhands.workspace import (
     ApptainerWorkspace,
@@ -177,40 +178,56 @@ def test_docker_network(mock_docker_workspace):
         assert run_cmd[network_index + 1] == network_name
 
 
-def test_docker_workspace_health_check_timeout_field_exists():
-    """Test that health_check_timeout is a recognised model field."""
-
-    assert "health_check_timeout" in DockerWorkspace.model_fields
+# ===========================================================================
+# health_check_timeout tests for DockerWorkspace and ApptainerWorkspace
+# ===========================================================================
 
 
-def test_docker_workspace_health_check_timeout_default():
+@pytest.mark.parametrize("cls", [DockerWorkspace, ApptainerWorkspace])
+def test_health_check_timeout_default(cls):
     """Test that health_check_timeout defaults to 120.0 seconds."""
+    assert cls.model_fields["health_check_timeout"].default == 120.0
+
 
-    field = DockerWorkspace.model_fields["health_check_timeout"]
-    assert field.default == 120.0
+@pytest.mark.parametrize("cls", [DockerWorkspace, ApptainerWorkspace])
+def test_health_check_timeout_rejects_non_positive(cls):
+    """Test that health_check_timeout rejects zero and negative values."""
+    with pytest.raises(ValidationError, match="greater than 0"):
+        # Attempt to create with invalid timeout - we need to mock startup
+        with patch.object(cls, "model_post_init"):
+            cls.model_validate(
+                {"server_image": "test:latest", "health_check_timeout": 0}
+            )
 
+    with pytest.raises(ValidationError, match="greater than 0"):
+        with patch.object(cls, "model_post_init"):
+            cls.model_validate(
+                {"server_image": "test:latest", "health_check_timeout": -10.0}
+            )
 
-def test_docker_workspace_health_check_timeout_custom(mock_docker_workspace):
-    """Test that a custom health_check_timeout is forwarded to _wait_for_health."""
 
-    with patch.object(DockerWorkspace, "_wait_for_health") as mock_wait:
-        with patch.object(DockerWorkspace, "_start_container"):
-            with patch(
-                "openhands.workspace.docker.workspace.execute_command"
-            ) as mock_exec:
-                mock_exec.return_value = Mock(returncode=0, stdout="", stderr="")
-                workspace = DockerWorkspace(
-                    server_image="test:latest", health_check_timeout=60.0
-                )
-        assert workspace.health_check_timeout == 60.0
-        # The field is set; _wait_for_health is called by _start_container which
-        # is mocked, so verify the value round-trips through the model.
-        _ = mock_wait  # referenced to satisfy linters
+def test_docker_workspace_startup_uses_health_check_timeout():
+    """Test that _start_container passes health_check_timeout to _wait_for_health."""
+    with (
+        patch(
+            "openhands.workspace.docker.workspace.check_port_available",
+            return_value=True,
+        ),
+        patch(
+            "openhands.workspace.docker.workspace.find_available_tcp_port",
+            return_value=8000,
+        ),
+        patch("openhands.workspace.docker.workspace.execute_command") as mock_exec,
+        patch.object(DockerWorkspace, "_wait_for_health") as mock_wait,
+        patch("openhands.workspace.docker.workspace.RemoteWorkspace.model_post_init"),
+    ):
+        mock_exec.return_value = Mock(returncode=0, stdout="container_123", stderr="")
+        DockerWorkspace(server_image="test:latest", health_check_timeout=60.0)
+        mock_wait.assert_called_once_with(timeout=60.0)
 
 
 def test_docker_workspace_resume_uses_health_check_timeout(mock_docker_workspace):
     """Test that resume() passes health_check_timeout to _wait_for_health."""
-
     with patch.object(DockerWorkspace, "_start_container"):
         with patch("openhands.workspace.docker.workspace.execute_command") as mock_exec:
             mock_exec.return_value = Mock(returncode=0, stdout="", stderr="")
@@ -226,27 +243,8 @@ def test_docker_workspace_resume_uses_health_check_timeout(mock_docker_workspace
         mock_wait.assert_called_once_with(timeout=30.0)
 
 
-# ===========================================================================
-# Apptainer health_check_timeout tests (co-located for compactness)
-# ===========================================================================
-
-
-def test_apptainer_workspace_health_check_timeout_field_exists():
-    """Test that health_check_timeout is a recognised model field."""
-
-    assert "health_check_timeout" in ApptainerWorkspace.model_fields
-
-
-def test_apptainer_workspace_health_check_timeout_default():
-    """Test that health_check_timeout defaults to 120.0 seconds."""
-
-    field = ApptainerWorkspace.model_fields["health_check_timeout"]
-    assert field.default == 120.0
-
-
-def test_apptainer_workspace_health_check_timeout_startup_call():
+def test_apptainer_workspace_startup_uses_health_check_timeout():
     """Test that model_post_init passes health_check_timeout to _wait_for_health."""
-
     with (
         patch("openhands.workspace.apptainer.workspace.execute_command") as mock_exec,
         patch(