fix: address review findings in verl-agent adapter (#88)

abrichr · claude · web-flow · commit 879c53c18285 · 2026-03-02T23:40:39.000-05:00
- Fix SCROLL direction not forwarded to BenchmarkAction.scroll_direction
- Fix DRAG parsing to include end_x/end_y coordinates
- Fix is_action_valid logic: use pattern match instead of inverted condition
- Fix fractional coord conversion: trust _use_fractional flag instead of
  checking value ranges (0 and 1 are ambiguous between frac and pixel)
- Convert drag end coordinates (end_x/end_y) from fractional to pixel
- Add health_check() method returning ready/busy/needs_recovery/not_initialized
- Add DRAG to system prompt DSL documentation
- Fix vendored VAGEN source URL (mll-lab-nu -&gt; RAGEN-AI)
- Add 12 new tests: scroll direction, drag coords, health_check, is_action_valid

Co-authored-by: Claude Opus 4.6 &lt;noreply@anthropic.com&gt;
diff --git a/openadapt_evals/adapters/_vendored/gym_base_env.py b/openadapt_evals/adapters/_vendored/gym_base_env.py
@@ -1,4 +1,4 @@
-# Vendored from https://github.com/mll-lab-nu/VAGEN
+# Vendored from https://github.com/RAGEN-AI/VAGEN
 # These are pure abstract base classes with no heavy dependencies.
 # Vendored to avoid requiring the full VAGEN installation.
 # Last synced: 2026-03-02
diff --git a/openadapt_evals/adapters/_vendored/gym_image_env.py b/openadapt_evals/adapters/_vendored/gym_image_env.py
@@ -1,4 +1,4 @@
-# Vendored from https://github.com/mll-lab-nu/VAGEN
+# Vendored from https://github.com/RAGEN-AI/VAGEN
 # These are pure abstract base classes with no heavy dependencies.
 # Vendored to avoid requiring the full VAGEN installation.
 # Last synced: 2026-03-02
diff --git a/openadapt_evals/adapters/verl_env.py b/openadapt_evals/adapters/verl_env.py
@@ -81,6 +81,7 @@ def _parse_action_str(action_str: str) -> BenchmarkAction:
         TYPE(text="hello world")
         KEY(key="enter")
         SCROLL(x=0.50, y=0.50, direction="down")
+        DRAG(x=0.20, y=0.30, end_x=0.80, end_y=0.70)
         WAIT()
         DONE()
     """
@@ -113,6 +114,7 @@ def _parse_action_str(action_str: str) -> BenchmarkAction:
             type="scroll",
             x=float(kwargs.get("x", 0.5)),
             y=float(kwargs.get("y", 0.5)),
+            scroll_direction=kwargs.get("direction", "down"),
         )
     elif cmd == "WAIT":
         return BenchmarkAction(type="wait")
@@ -123,6 +125,8 @@ def _parse_action_str(action_str: str) -> BenchmarkAction:
             type="drag",
             x=float(kwargs.get("x", 0.5)),
             y=float(kwargs.get("y", 0.5)),
+            end_x=float(kwargs["end_x"]) if "end_x" in kwargs else None,
+            end_y=float(kwargs["end_y"]) if "end_y" in kwargs else None,
         )
     else:
         return BenchmarkAction(type="done")
@@ -163,6 +167,7 @@ def _build_obs_dict(
     "  TYPE(text=\"<text>\") - type text\n"
     "  KEY(key=\"<name>\") - press a key (enter, tab, escape, ctrl+a, etc.)\n"
     "  SCROLL(x=<frac>, y=<frac>, direction=\"up\"|\"down\") - scroll\n"
+    "  DRAG(x=<frac>, y=<frac>, end_x=<frac>, end_y=<frac>) - drag\n"
     "  WAIT() - wait for the screen to update\n"
     "  DONE() - task is complete\n"
     "\n"
@@ -216,6 +221,42 @@ def _ensure_env(self) -> RLEnvironment:
         self._rl_env = RLEnvironment(adapter, default_task_id=self._task_id)
         return self._rl_env
 
+    async def health_check(self) -> dict[str, Any]:
+        """Check environment health status.
+
+        Returns a dict with:
+            status: "ready" | "busy" | "needs_recovery" | "not_initialized"
+            server_url: The WAA server URL
+            step_count: Current step count in episode
+
+        Use this from a pool controller to decide whether to send work
+        to this environment or retire/restart it.
+        """
+        if self._rl_env is None:
+            return {"status": "not_initialized", "server_url": self._server_url}
+
+        adapter = self._rl_env.adapter
+        # Check if the WAA server is reachable
+        try:
+            check_fn = getattr(adapter, "check_connection", None)
+            reachable = await asyncio.to_thread(check_fn) if check_fn else True
+        except Exception:
+            reachable = False
+
+        if not reachable:
+            return {
+                "status": "needs_recovery",
+                "server_url": self._server_url,
+                "step_count": self._step_count,
+            }
+
+        status = "busy" if self._step_count > 0 and not self._rl_env.done else "ready"
+        return {
+            "status": status,
+            "server_url": self._server_url,
+            "step_count": self._step_count,
+        }
+
     async def close(self) -> None:
         """Release resources."""
         if self._rl_env is not None:
@@ -270,13 +311,20 @@ async def step(
         # Parse the LLM output into a BenchmarkAction
         action = _parse_action_str(action_str)
 
-        # Handle fractional → pixel coordinate conversion
+        # Handle fractional → pixel coordinate conversion.
+        # When _use_fractional is True, all coordinates from the parser are
+        # fractions (0.0-1.0). We convert unconditionally rather than checking
+        # value ranges, since pixel values 0 and 1 would be ambiguous.
         if self._use_fractional and action.type in ("click", "scroll", "drag"):
             w, h = env.screen_size
-            if action.x is not None and 0.0 <= action.x <= 1.0:
+            if action.x is not None:
                 action.x = int(action.x * w)
-            if action.y is not None and 0.0 <= action.y <= 1.0:
+            if action.y is not None:
                 action.y = int(action.y * h)
+            if action.end_x is not None:
+                action.end_x = int(action.end_x * w)
+            if action.end_y is not None:
+                action.end_y = int(action.end_y * h)
 
         # Execute action in a thread
         rollout_step = await asyncio.to_thread(env.step, action)
@@ -287,7 +335,8 @@ async def step(
         # Compute reward
         reward = 0.0
         info: dict[str, Any] = rollout_step.info
-        info["is_action_valid"] = action.type != "done" or action_str.strip() == ""
+        # Action is valid if it was explicitly parsed (not the fallback for unparseable input)
+        info["is_action_valid"] = _ACTION_PATTERN.search(action_str) is not None
 
         if done and self._evaluate_at_done:
             try:
diff --git a/tests/test_verl_env.py b/tests/test_verl_env.py
@@ -13,6 +13,7 @@
 from openadapt_evals.adapters.rl_env import RLEnvironment
 from openadapt_evals.adapters.verl_env import (
     WAADesktopEnv,
+    _ACTION_PATTERN,
     _build_obs_dict,
     _parse_action_str,
 )
@@ -52,14 +53,37 @@ def test_done(self):
         action = _parse_action_str("DONE()")
         assert action.type == "done"
 
-    def test_scroll(self):
+    def test_scroll_with_direction(self):
         action = _parse_action_str('SCROLL(x=0.50, y=0.50, direction="down")')
         assert action.type == "scroll"
+        assert action.scroll_direction == "down"
+
+    def test_scroll_up(self):
+        action = _parse_action_str('SCROLL(x=0.50, y=0.50, direction="up")')
+        assert action.scroll_direction == "up"
+
+    def test_scroll_default_direction(self):
+        action = _parse_action_str("SCROLL(x=0.50, y=0.50)")
+        assert action.scroll_direction == "down"
 
     def test_invalid_returns_done(self):
         action = _parse_action_str("random garbage text")
         assert action.type == "done"
 
+    def test_drag_with_end_coords(self):
+        action = _parse_action_str("DRAG(x=0.20, y=0.30, end_x=0.80, end_y=0.70)")
+        assert action.type == "drag"
+        assert action.x == pytest.approx(0.20)
+        assert action.y == pytest.approx(0.30)
+        assert action.end_x == pytest.approx(0.80)
+        assert action.end_y == pytest.approx(0.70)
+
+    def test_drag_without_end_coords(self):
+        action = _parse_action_str("DRAG(x=0.20, y=0.30)")
+        assert action.type == "drag"
+        assert action.end_x is None
+        assert action.end_y is None
+
     def test_with_thinking(self):
         action = _parse_action_str(
             "<think>I need to click the button</think>\nCLICK(x=0.25, y=0.75)"
@@ -68,14 +92,21 @@ def test_with_thinking(self):
         assert action.x == pytest.approx(0.25)
         assert action.y == pytest.approx(0.75)
 
+    def test_invalid_action_not_matched(self):
+        """Unparseable input should not match the action pattern."""
+        assert _ACTION_PATTERN.search("random garbage") is None
+
+    def test_explicit_done_is_matched(self):
+        """Explicit DONE() should match the action pattern."""
+        assert _ACTION_PATTERN.search("DONE()") is not None
+
 
 # --- Observation building tests ---
 
 
 class TestBuildObsDict:
     def test_with_screenshot(self):
         """Test obs dict with PNG bytes."""
-        # Create a minimal valid PNG (1x1 red pixel)
         from PIL import Image
         import io
 
@@ -137,6 +168,7 @@ def test_system_prompt(self):
         assert "obs_str" in result
         assert "CLICK" in result["obs_str"]
         assert "TYPE" in result["obs_str"]
+        assert "DRAG" in result["obs_str"]
         assert "DONE" in result["obs_str"]
 
     def test_reset_returns_obs_dict(self):
@@ -165,7 +197,6 @@ def test_step_done_triggers_eval(self):
         asyncio.run(env.reset(seed=42))
         obs, reward, done, info = asyncio.run(env.step("DONE()"))
         assert done is True
-        # Reward should be a float from evaluation (mock evaluator)
         assert isinstance(reward, float)
 
     def test_max_steps_triggers_done(self):
@@ -186,15 +217,13 @@ def test_close(self):
         assert env._rl_env is None
 
     def test_full_episode_flow(self):
-        """Test a complete episode: reset → multiple steps → done → evaluate."""
+        """Test a complete episode: reset -> multiple steps -> done -> evaluate."""
         env = _make_mock_env()
         env._max_steps = 5
 
-        # Reset
         obs, info = asyncio.run(env.reset(seed=1))
         assert "obs_str" in obs
 
-        # Take some actions
         obs, r, done, _ = asyncio.run(env.step("CLICK(x=0.05, y=0.08)"))
         assert not done
         assert r == 0.0
@@ -203,28 +232,70 @@ def test_full_episode_flow(self):
         assert not done
         assert r == 0.0
 
-        # Finish
         obs, r, done, info = asyncio.run(env.step("DONE()"))
         assert done
         assert isinstance(r, float)
 
     def test_protocol_has_required_methods(self):
         """Verify WAADesktopEnv has all GymImageEnv protocol methods."""
         env = _make_mock_env()
-        assert hasattr(env, "reset")
-        assert hasattr(env, "step")
-        assert hasattr(env, "close")
-        assert hasattr(env, "system_prompt")
-        assert callable(env.reset)
-        assert callable(env.step)
-        assert callable(env.close)
-        assert callable(env.system_prompt)
+        for method in ("reset", "step", "close", "system_prompt", "health_check"):
+            assert hasattr(env, method)
+            assert callable(getattr(env, method))
 
     def test_obs_contains_image_placeholder(self):
         """Test that observations with screenshots include <image> placeholder."""
         env = _make_mock_env()
         obs, _ = asyncio.run(env.reset(seed=42))
-        # Mock adapter returns observations that may or may not have screenshots
-        # At minimum, obs_str should be present
         assert "obs_str" in obs
         assert isinstance(obs["obs_str"], str)
+
+    # --- health_check tests ---
+
+    def test_health_check_not_initialized(self):
+        """Health check before reset returns not_initialized."""
+        env = WAADesktopEnv.__new__(WAADesktopEnv)
+        env._rl_env = None
+        env._server_url = "mock"
+        env._step_count = 0
+        result = asyncio.run(env.health_check())
+        assert result["status"] == "not_initialized"
+
+    def test_health_check_ready_after_episode(self):
+        """Health check after completed episode returns ready."""
+        env = _make_mock_env()
+        asyncio.run(env.reset(seed=42))
+        asyncio.run(env.step("DONE()"))
+        result = asyncio.run(env.health_check())
+        assert result["status"] == "ready"
+
+    def test_health_check_busy_mid_episode(self):
+        """Health check mid-episode returns busy."""
+        env = _make_mock_env()
+        asyncio.run(env.reset(seed=42))
+        asyncio.run(env.step("CLICK(x=0.5, y=0.5)"))
+        result = asyncio.run(env.health_check())
+        assert result["status"] == "busy"
+
+    # --- is_action_valid tests ---
+
+    def test_is_action_valid_for_parsed_action(self):
+        """Actions that parse successfully should be marked valid."""
+        env = _make_mock_env()
+        asyncio.run(env.reset(seed=42))
+        _, _, _, info = asyncio.run(env.step("CLICK(x=0.5, y=0.5)"))
+        assert info["is_action_valid"] is True
+
+    def test_is_action_valid_for_done(self):
+        """Explicit DONE() should be marked valid."""
+        env = _make_mock_env()
+        asyncio.run(env.reset(seed=42))
+        _, _, _, info = asyncio.run(env.step("DONE()"))
+        assert info["is_action_valid"] is True
+
+    def test_is_action_invalid_for_garbage(self):
+        """Unparseable input should be marked invalid."""
+        env = _make_mock_env()
+        asyncio.run(env.reset(seed=42))
+        _, _, _, info = asyncio.run(env.step("random garbage"))
+        assert info["is_action_valid"] is False

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-# Vendored from https://github.com/mll-lab-nu/VAGEN`
	`1`	`+# Vendored from https://github.com/RAGEN-AI/VAGEN`
`2`	`2`	`# These are pure abstract base classes with no heavy dependencies.`
`3`	`3`	`# Vendored to avoid requiring the full VAGEN installation.`
`4`	`4`	`# Last synced: 2026-03-02`