fix: address browser challenge review nits
This commit is contained in:
@@ -90,7 +90,7 @@ def _event_text(event: dict[str, Any]) -> str | None:
|
|||||||
return text if isinstance(text, str) else None
|
return text if isinstance(text, str) else None
|
||||||
|
|
||||||
|
|
||||||
def _result_text(parsed: dict[str, Any]) -> str:
|
def result_text(parsed: dict[str, Any]) -> str:
|
||||||
for key in ("text", "message", "content", "output"):
|
for key in ("text", "message", "content", "output"):
|
||||||
value = parsed.get(key)
|
value = parsed.get(key)
|
||||||
if isinstance(value, str):
|
if isinstance(value, str):
|
||||||
|
|||||||
@@ -9,8 +9,8 @@ from examples.agent_challenges.browser_click_challenge.classification import (
|
|||||||
extract_challenge_report,
|
extract_challenge_report,
|
||||||
)
|
)
|
||||||
from examples.agent_challenges.browser_click_challenge.opencode_io import (
|
from examples.agent_challenges.browser_click_challenge.opencode_io import (
|
||||||
_result_text,
|
|
||||||
parse_opencode_output,
|
parse_opencode_output,
|
||||||
|
result_text,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
@@ -61,7 +61,7 @@ def save_report_from_result_payload(
|
|||||||
def _report_text_from_result(result: dict[str, object]) -> str:
|
def _report_text_from_result(result: dict[str, object]) -> str:
|
||||||
parsed = result.get("parsed")
|
parsed = result.get("parsed")
|
||||||
if isinstance(parsed, dict):
|
if isinstance(parsed, dict):
|
||||||
return _result_text(parsed)
|
return result_text(parsed)
|
||||||
|
|
||||||
stdout = result.get("stdout")
|
stdout = result.get("stdout")
|
||||||
if isinstance(stdout, str) and stdout.strip():
|
if isinstance(stdout, str) and stdout.strip():
|
||||||
@@ -71,7 +71,7 @@ def _report_text_from_result(result: dict[str, object]) -> str:
|
|||||||
raise ValueError(
|
raise ValueError(
|
||||||
"result file is missing parsed output and stdout has no report text"
|
"result file is missing parsed output and stdout has no report text"
|
||||||
) from exc
|
) from exc
|
||||||
return _result_text(recovered)
|
return result_text(recovered)
|
||||||
|
|
||||||
raise ValueError("result file is missing parsed output")
|
raise ValueError("result file is missing parsed output")
|
||||||
|
|
||||||
|
|||||||
@@ -46,9 +46,9 @@ from examples.agent_challenges.browser_click_challenge.classification import (
|
|||||||
from examples.agent_challenges.browser_click_challenge.opencode_io import ( # noqa: E402
|
from examples.agent_challenges.browser_click_challenge.opencode_io import ( # noqa: E402
|
||||||
_event_text,
|
_event_text,
|
||||||
_parse_jsonl_tail,
|
_parse_jsonl_tail,
|
||||||
_result_text,
|
|
||||||
build_opencode_command,
|
build_opencode_command,
|
||||||
parse_opencode_output,
|
parse_opencode_output,
|
||||||
|
result_text,
|
||||||
)
|
)
|
||||||
from examples.agent_challenges.browser_click_challenge.reports import ( # noqa: E402
|
from examples.agent_challenges.browser_click_challenge.reports import ( # noqa: E402
|
||||||
save_report_from_result_payload,
|
save_report_from_result_payload,
|
||||||
@@ -76,7 +76,6 @@ __all__ = [
|
|||||||
"_contains_bool_marker",
|
"_contains_bool_marker",
|
||||||
"_event_text",
|
"_event_text",
|
||||||
"_parse_jsonl_tail",
|
"_parse_jsonl_tail",
|
||||||
"_result_text",
|
|
||||||
"build_opencode_command",
|
"build_opencode_command",
|
||||||
"challenge_report_schema_errors",
|
"challenge_report_schema_errors",
|
||||||
"classify_challenge_report",
|
"classify_challenge_report",
|
||||||
@@ -86,6 +85,7 @@ __all__ = [
|
|||||||
"parse_opencode_output",
|
"parse_opencode_output",
|
||||||
"prepare_trial_workspace",
|
"prepare_trial_workspace",
|
||||||
"render_prompt",
|
"render_prompt",
|
||||||
|
"result_text",
|
||||||
"rpc_url_for_port",
|
"rpc_url_for_port",
|
||||||
"run_trial",
|
"run_trial",
|
||||||
"save_report_from_result_payload",
|
"save_report_from_result_payload",
|
||||||
@@ -300,7 +300,7 @@ def run_trial(config: TrialConfig, *, index: int, results_dir: Path) -> dict[str
|
|||||||
parsed: dict[str, Any] | None
|
parsed: dict[str, Any] | None
|
||||||
try:
|
try:
|
||||||
parsed = parse_opencode_output(completed.stdout)
|
parsed = parse_opencode_output(completed.stdout)
|
||||||
text = _result_text(parsed)
|
text = result_text(parsed)
|
||||||
classification = classify_output(text)
|
classification = classify_output(text)
|
||||||
except Exception:
|
except Exception:
|
||||||
parsed = None
|
parsed = None
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ from __future__ import annotations
|
|||||||
|
|
||||||
import html
|
import html
|
||||||
import json
|
import json
|
||||||
|
import logging
|
||||||
import threading
|
import threading
|
||||||
import uuid
|
import uuid
|
||||||
import webbrowser
|
import webbrowser
|
||||||
@@ -9,12 +10,16 @@ from dataclasses import dataclass
|
|||||||
from http import HTTPStatus
|
from http import HTTPStatus
|
||||||
from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer
|
from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer
|
||||||
from typing import ClassVar
|
from typing import ClassVar
|
||||||
|
from urllib.error import HTTPError, URLError
|
||||||
from urllib.request import Request, urlopen
|
from urllib.request import Request, urlopen
|
||||||
|
|
||||||
from pydantic import BaseModel, Field
|
from pydantic import BaseModel, Field
|
||||||
|
|
||||||
from wf_authoring import node
|
from wf_authoring import node
|
||||||
|
|
||||||
|
_LOGGER = logging.getLogger(__name__)
|
||||||
|
_MAX_SESSION_EVENTS = 10
|
||||||
|
|
||||||
|
|
||||||
class Snapshot(BaseModel):
|
class Snapshot(BaseModel):
|
||||||
title: str
|
title: str
|
||||||
@@ -91,10 +96,21 @@ class _ClickSession:
|
|||||||
events=list(self.events[-10:]),
|
events=list(self.events[-10:]),
|
||||||
)
|
)
|
||||||
|
|
||||||
|
def add_event(self, event: str) -> None:
|
||||||
|
"""Record bounded click-session events for compact snapshots."""
|
||||||
|
self.events.append(event)
|
||||||
|
if len(self.events) > _MAX_SESSION_EVENTS:
|
||||||
|
del self.events[: len(self.events) - _MAX_SESSION_EVENTS]
|
||||||
|
|
||||||
def close(self) -> None:
|
def close(self) -> None:
|
||||||
self.server.shutdown()
|
self.server.shutdown()
|
||||||
self.server.server_close()
|
self.server.server_close()
|
||||||
self.thread.join(timeout=2)
|
self.thread.join(timeout=2)
|
||||||
|
if self.thread.is_alive():
|
||||||
|
_LOGGER.warning(
|
||||||
|
"click session %s server thread did not stop within timeout",
|
||||||
|
self.session_id,
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
_SESSIONS: dict[str, _ClickSession] = {}
|
_SESSIONS: dict[str, _ClickSession] = {}
|
||||||
@@ -122,7 +138,7 @@ class _ClickHandler(BaseHTTPRequestHandler):
|
|||||||
return
|
return
|
||||||
session = self.server.session
|
session = self.server.session
|
||||||
session.clicked.set()
|
session.clicked.set()
|
||||||
session.events.append("click")
|
session.add_event("click")
|
||||||
self._send_json(session.snapshot().model_dump(mode="json"))
|
self._send_json(session.snapshot().model_dump(mode="json"))
|
||||||
|
|
||||||
def _send_html(self) -> None:
|
def _send_html(self) -> None:
|
||||||
@@ -206,7 +222,13 @@ def _open_click_page(payload: OpenPageInput) -> OpenPageOutput:
|
|||||||
_SESSIONS[session_id] = session
|
_SESSIONS[session_id] = session
|
||||||
session.thread.start()
|
session.thread.start()
|
||||||
if payload.open_browser:
|
if payload.open_browser:
|
||||||
webbrowser.open(session.url)
|
try:
|
||||||
|
opened = webbrowser.open(session.url)
|
||||||
|
except Exception:
|
||||||
|
_LOGGER.exception("failed to open browser for click session %s", session_id)
|
||||||
|
else:
|
||||||
|
if not opened:
|
||||||
|
_LOGGER.warning("browser did not report opening %s", session.url)
|
||||||
return OpenPageOutput(
|
return OpenPageOutput(
|
||||||
session_id=session_id,
|
session_id=session_id,
|
||||||
url=session.url,
|
url=session.url,
|
||||||
@@ -218,8 +240,13 @@ def _wait_for_click(payload: WaitForClickInput) -> WaitForClickOutput:
|
|||||||
session = _get_session(payload.session_id)
|
session = _get_session(payload.session_id)
|
||||||
if payload.simulate:
|
if payload.simulate:
|
||||||
request = Request(f"{session.url}click", method="POST")
|
request = Request(f"{session.url}click", method="POST")
|
||||||
with urlopen(request, timeout=payload.timeout_seconds) as response:
|
try:
|
||||||
response.read()
|
with urlopen(request, timeout=payload.timeout_seconds) as response:
|
||||||
|
response.read()
|
||||||
|
except (HTTPError, URLError) as exc:
|
||||||
|
raise RuntimeError(
|
||||||
|
f"simulated click failed for session {payload.session_id!r}"
|
||||||
|
) from exc
|
||||||
elif not session.clicked.wait(timeout=payload.timeout_seconds):
|
elif not session.clicked.wait(timeout=payload.timeout_seconds):
|
||||||
raise TimeoutError("timed out waiting for click")
|
raise TimeoutError("timed out waiting for click")
|
||||||
return WaitForClickOutput(
|
return WaitForClickOutput(
|
||||||
|
|||||||
@@ -10,6 +10,7 @@ from examples.browser_click_workflow.ops import (
|
|||||||
WaitForClickInput,
|
WaitForClickInput,
|
||||||
_active_session_count,
|
_active_session_count,
|
||||||
_collect_snapshots,
|
_collect_snapshots,
|
||||||
|
_get_session,
|
||||||
_open_click_page,
|
_open_click_page,
|
||||||
_wait_for_click,
|
_wait_for_click,
|
||||||
)
|
)
|
||||||
@@ -68,6 +69,47 @@ def test_browser_click_source_human_timeout_cleans_up() -> None:
|
|||||||
assert _active_session_count() == 0
|
assert _active_session_count() == 0
|
||||||
|
|
||||||
|
|
||||||
|
def test_browser_click_session_events_are_bounded() -> None:
|
||||||
|
opened = _open_click_page(OpenPageInput(open_browser=False))
|
||||||
|
try:
|
||||||
|
session = _get_session(opened.session_id)
|
||||||
|
for index in range(20):
|
||||||
|
session.add_event(f"event-{index}")
|
||||||
|
|
||||||
|
snapshot = session.snapshot()
|
||||||
|
|
||||||
|
assert len(snapshot.events) == 10
|
||||||
|
assert snapshot.events == [f"event-{index}" for index in range(10, 20)]
|
||||||
|
finally:
|
||||||
|
_collect_snapshots(
|
||||||
|
CollectSnapshotsInput(
|
||||||
|
session_id=opened.session_id,
|
||||||
|
before=opened.before,
|
||||||
|
after=opened.before,
|
||||||
|
)
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_browser_click_open_browser_failure_does_not_fail_workflow(monkeypatch) -> None:
|
||||||
|
def fail_open(_url: str) -> bool:
|
||||||
|
raise RuntimeError("no browser")
|
||||||
|
|
||||||
|
monkeypatch.setattr("examples.browser_click_workflow.ops.webbrowser.open", fail_open)
|
||||||
|
|
||||||
|
opened = _open_click_page(OpenPageInput(open_browser=True))
|
||||||
|
try:
|
||||||
|
assert opened.url.startswith("http://127.0.0.1:")
|
||||||
|
assert opened.before.clicked is False
|
||||||
|
finally:
|
||||||
|
_collect_snapshots(
|
||||||
|
CollectSnapshotsInput(
|
||||||
|
session_id=opened.session_id,
|
||||||
|
before=opened.before,
|
||||||
|
after=opened.before,
|
||||||
|
)
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
EXAMPLE_DIR = (
|
EXAMPLE_DIR = (
|
||||||
Path(__file__).resolve().parents[2] / "examples" / "browser_click_workflow"
|
Path(__file__).resolve().parents[2] / "examples" / "browser_click_workflow"
|
||||||
)
|
)
|
||||||
|
|||||||
Reference in New Issue
Block a user