Skip to content

Commit bbfdb55

Browse files
committed
fix(browser): route AsyncPlaywrightTool page_obs through async_execute_action
AsyncPlaywrightTool overrode `execute_action` — the base Tool's SYNC bridge — with an async coroutine that did `await super().execute_action(...)`, which always raised `TypeError: object Observation can't be used in 'await' expression`. Meanwhile async callers (the gym-style `Task.step`, the harness parallel-tool path, and the MCP server) enter through `async_execute_action`, which fell through to the base implementation and returned a bare "Success", silently dropping the page observation for every async browser action. Move the page_obs override to `async_execute_action` (the native async entry, mirroring SyncPlaywrightTool's sync `execute_action` override) and correct the `super()` call. Update the async integration tests to use `async_execute_action`, and add two CI-runnable regression tests (no browser): a structural guard that the override lives on the async entry, and a behavioral check that it returns the page observation (not "Success") with `tool_call_id` propagated. Caught by scripts/smoke/tool_api.py, which now passes. Signed-off-by: Alexandre Lacoste <alex.lacoste.shmu@gmail.com>
1 parent d74c1f6 commit bbfdb55

2 files changed

Lines changed: 43 additions & 7 deletions

File tree

cube-tools/cube-browser-tool/src/cube_browser_tool/playwright_tool.py

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -447,8 +447,13 @@ async def close(self) -> None:
447447
"""Release all Playwright resources via the session."""
448448
await self._session.stop()
449449

450-
async def execute_action(self, action: Action) -> Observation | StepError:
451-
result = await super().execute_action(action)
450+
# Action dispatch override — appends page_obs() after every action.
451+
# Async callers (Task.step, the harness parallel-tool path, the MCP
452+
# server) enter through `async_execute_action`, the base's native async
453+
# entry — NOT `execute_action` (which is the sync bridge). Overriding the
454+
# wrong one left every async browser action returning a bare "Success".
455+
async def async_execute_action(self, action: Action) -> Observation | StepError:
456+
result = await super().async_execute_action(action)
452457
if isinstance(result, StepError):
453458
return result
454459
try:

cube-tools/cube-browser-tool/tests/test_playwright_tool.py

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212

1313
import pytest
1414
import pytest_asyncio
15-
from cube.core import Action, Observation, StepError
15+
from cube.core import Action, Content, Observation, StepError
1616
from cube.tools.browser import AsyncBrowserTool, BrowserTool
1717
from cube_browser_playwright import PlaywrightSessionConfig, Viewport
1818

@@ -118,6 +118,35 @@ def test_async_max_wait_validator_rejects_negative() -> None:
118118
AsyncPlaywrightConfig(max_wait=-1)
119119

120120

121+
def test_async_tool_overrides_async_execute_action_not_sync() -> None:
122+
"""Async callers (Task.step, the harness parallel-tool path, the MCP server)
123+
enter through ``async_execute_action`` — the page_obs override must live there,
124+
not on the sync ``execute_action`` bridge, or async browser actions silently
125+
return a bare "Success" (regression guard, needs no browser)."""
126+
assert "async_execute_action" in AsyncPlaywrightTool.__dict__
127+
assert "execute_action" not in AsyncPlaywrightTool.__dict__
128+
129+
130+
@pytest.mark.asyncio
131+
async def test_async_execute_action_returns_page_obs_not_success() -> None:
132+
"""``async_execute_action`` must discard the spurious "Success" item that
133+
null-returning browser actions produce and return the page observation,
134+
with ``tool_call_id`` propagated. Exercises the real dispatch override with
135+
a stubbed ``page_obs`` — no browser required."""
136+
tool = object.__new__(AsyncPlaywrightTool) # skip __init__; no live session needed
137+
sentinel = Observation(contents=[Content.from_data("PAGE_OBS_SENTINEL")])
138+
139+
async def fake_page_obs() -> Observation:
140+
return sentinel
141+
142+
tool.page_obs = fake_page_obs # type: ignore[method-assign]
143+
result = await tool.async_execute_action(Action(name="noop", arguments={}, id="call-1"))
144+
assert isinstance(result, Observation)
145+
datas = [c.data for c in result.contents if hasattr(c, "data")]
146+
assert "PAGE_OBS_SENTINEL" in datas and "Success" not in datas
147+
assert result.contents[0].tool_call_id == "call-1"
148+
149+
121150
# ---------------------------------------------------------------------------
122151
# Integration tests — require a live Playwright/Chromium install
123152
# ---------------------------------------------------------------------------
@@ -341,7 +370,7 @@ async def test_async_page_obs_contains_html(async_tool) -> None:
341370
@pytest.mark.asyncio
342371
async def test_async_execute_action_appends_page_obs(async_tool) -> None:
343372
await async_tool.goto(SIMPLE_PAGE)
344-
result = await async_tool.execute_action(Action(name="noop", arguments={}))
373+
result = await async_tool.async_execute_action(Action(name="noop", arguments={}))
345374
assert isinstance(result, Observation)
346375
assert len(result.contents) >= 1
347376

@@ -350,7 +379,9 @@ async def test_async_execute_action_appends_page_obs(async_tool) -> None:
350379
@pytest.mark.asyncio
351380
async def test_async_execute_action_returns_step_error_on_bad_selector(async_tool) -> None:
352381
await async_tool.goto(SIMPLE_PAGE)
353-
result = await async_tool.execute_action(Action(name="browser_click", arguments={"selector": "#does-not-exist"}))
382+
result = await async_tool.async_execute_action(
383+
Action(name="browser_click", arguments={"selector": "#does-not-exist"})
384+
)
354385
assert isinstance(result, StepError)
355386

356387

@@ -369,7 +400,7 @@ async def test_async_browser_select_option() -> None:
369400
tool = await AsyncPlaywrightConfig(use_html=True, use_screenshot=False, use_axtree=False).make()
370401
try:
371402
await tool.goto(SELECT_PAGE)
372-
result = await tool.execute_action(
403+
result = await tool.async_execute_action(
373404
Action(name="browser_select_option", arguments={"selector": "#sel", "value": "b"})
374405
)
375406
assert isinstance(result, Observation)
@@ -387,7 +418,7 @@ async def test_async_browser_wait_is_capped_at_max_wait() -> None:
387418
tool = await AsyncPlaywrightConfig(use_html=False, use_screenshot=False, use_axtree=False, max_wait=1).make()
388419
try:
389420
start = time.monotonic()
390-
await tool.execute_action(Action(name="browser_wait", arguments={"seconds": 9999}))
421+
await tool.async_execute_action(Action(name="browser_wait", arguments={"seconds": 9999}))
391422
elapsed = time.monotonic() - start
392423
assert elapsed < 3.0 # capped at max_wait=1, with 2s slack for CI
393424
finally:

0 commit comments

Comments
 (0)