From a87647e07ce6af1b3e94d8b4dce8d3b40e760489 Mon Sep 17 00:00:00 2001 From: apireno Date: Thu, 4 Jun 2026 12:43:36 -0400 Subject: [PATCH] =?UTF-8?q?fix(browser):=20R4=20bot-cleanup=20=E2=80=94=20?= =?UTF-8?q?surface=20ls/grep=20errors,=20lock=20=5Fdaemon=5Flane=5Fid,=20e?= =?UTF-8?q?xc=5Finfo,=20HARNESS.md=20drift?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex P2 + Copilot review of 5790651. One real correctness bug (error visibility in non-JSON CLI), three defensive cleanups (thread-safety, log traceback, doc drift). Pre-existing items already tracked as #327, #328; two more filed as new follow-ups. 1. browser_cli.py fs_ls + fs_grep non-JSON branches now check "error" in result before falling into the empty-collection "No elements"/"No matches" path. Mirrors fs_cd. Without this, `fs ls /nonexistent` displayed "No elements at /nonexistent" instead of DOMShell's "No such directory" error. (Codex P2) 2. domshell_backend.py _daemon_lane_id capture wrapped in a threading.Lock; the test-and-set race on the `is None` check moved inside the lock. Cheap defensive measure for any future concurrent caller of _call_execute. (Copilot ×3) 3. domshell_backend.py daemon-failure log.warning now carries exc_info=True so the traceback shows up in diagnosability output. (Copilot) 4. HARNESS.md dropped the stale `group_id="shared"` reference (removed in 5790651's R3 fix) and replaced with accurate _daemon_lane_id-based wording for daemon-no-session callers. The split-and-check table accuracy (separate pre-existing item) stays scoped to #328. (Copilot) Tests: 193 passing locally (was 191 + 2 new for ls/grep error visibility). Cc @yuh-yang Co-Authored-By: Claude Opus 4.7 (1M context) --- browser/agent-harness/HARNESS.md | 12 +++-- .../cli_anything/browser/browser_cli.py | 14 +++++ .../cli_anything/browser/tests/test_core.py | 53 +++++++++++++++++++ .../browser/utils/domshell_backend.py | 34 ++++++++---- 4 files changed, 101 insertions(+), 12 deletions(-) diff --git a/browser/agent-harness/HARNESS.md b/browser/agent-harness/HARNESS.md index 7b194e1b7..2cd1ade02 100644 --- a/browser/agent-harness/HARNESS.md +++ b/browser/agent-harness/HARNESS.md @@ -38,9 +38,15 @@ DOMShell is an npm package that exposes Chrome's Accessibility Tree via MCP: ### Installation **Requires `@apireno/domshell` 2.0.2 or newer.** The harness uses -`group_id="shared"` and `group_id="new"` keywords introduced in 2.0.2 to -declare lane intent explicitly (silences the deprecation warning that 2.0.2 -added for omitted `group_id`; will become a hard error in DOMShell 3.0.0). +`group_id="new"` to declare lane intent explicitly on the first call of +each session and reuses the captured lane id on subsequent calls (silences +the deprecation warning that 2.0.2 added for omitted `group_id`; will +become a hard error in DOMShell 3.0.0). For direct daemon-mode callers +without a harness `Session`, the same scheme runs at module level — the +first call captures a lane id into `_daemon_lane_id` and subsequent calls +reuse it, preserving browser state across daemon-mode no-session calls +(see `domshell_backend.py`'s `_daemon_lane_id` declaration for the +stale-lane failure mode). ```bash npx @apireno/domshell --version # should report 2.0.2 or higher diff --git a/browser/agent-harness/cli_anything/browser/browser_cli.py b/browser/agent-harness/cli_anything/browser/browser_cli.py index d0751a953..264a1f9c9 100644 --- a/browser/agent-harness/cli_anything/browser/browser_cli.py +++ b/browser/agent-harness/cli_anything/browser/browser_cli.py @@ -226,6 +226,13 @@ def fs_ls(path): if _json_output: output(result) else: + # Surface DOMShell errors (e.g. `fs ls /nonexistent`) before + # falling into the empty-entries "No elements" branch — without + # this, an error from the kernel was hidden behind a misleading + # "No elements at …" message. (Codex P2 R4 on commit 5790651.) + if "error" in result: + click.echo(result["error"], err=True) + return entries = result.get("entries", []) if not entries: click.echo(f"No elements at {path or sess.working_dir}") @@ -273,6 +280,13 @@ def fs_grep(pattern, path): if _json_output: output(result) else: + # Surface DOMShell errors before the empty-matches "No matches" + # branch — same fix shape as fs_ls. Without this, an anchor cd + # failure on rooted grep was hidden behind "No matches". + # (Codex P2 R4 on commit 5790651.) + if "error" in result: + click.echo(result["error"], err=True) + return matches = result.get("matches", []) if not matches: click.echo(f"No matches for '{pattern}'") diff --git a/browser/agent-harness/cli_anything/browser/tests/test_core.py b/browser/agent-harness/cli_anything/browser/tests/test_core.py index ae44a613e..18c722582 100644 --- a/browser/agent-harness/cli_anything/browser/tests/test_core.py +++ b/browser/agent-harness/cli_anything/browser/tests/test_core.py @@ -386,3 +386,56 @@ class TestDaemonMode: result = fs.list_elements(sess) mock_ls.assert_called_once_with("/", use_daemon=False, session=sess) + + +# ── CLI-layer error surfacing (Codex P2 R4) ───────────────────────── + + +class TestCLIErrorSurfacing: + """The non-JSON branches of `fs ls` and `fs grep` previously fell + straight into ``result.get("entries"/"matches", [])`` and surfaced + "No elements at …" / "No matches for …" for DOMShell errors. Codex + P2 R4 on PR #308 commit 5790651 required surfacing the error + message instead. + """ + + def _invoke(self, mod_target, error_result, argv): + """Mock the dependency check + the fs_mod target, invoke CLI.""" + from click.testing import CliRunner + from cli_anything.browser.browser_cli import cli + + with patch( + "cli_anything.browser.browser_cli.backend.is_available", + return_value=(True, "ok"), + ), patch( + f"cli_anything.browser.browser_cli.fs_mod.{mod_target}", + return_value=error_result, + ): + return CliRunner().invoke(cli, argv) + + def test_fs_ls_surfaces_error_in_non_json_output(self): + """`fs ls` on a path DOMShell errors against should display the + error message — not the misleading "No elements at ". + """ + error_result = { + "error": "ls: nonexistent: No such directory", + "output": "ls: nonexistent: No such directory", + } + result = self._invoke( + "list_elements", error_result, ["fs", "ls", "/nonexistent"], + ) + # click.echo(err=True) goes to stderr; CliRunner captures both. + assert "No such directory" in result.output + assert "No elements" not in result.output + + def test_fs_grep_surfaces_error_in_non_json_output(self): + """Mirror for `fs grep`. Codex P2 R4 regression test.""" + error_result = { + "error": "cd: /nonexistent: No such directory", + "output": "cd: /nonexistent: No such directory", + } + result = self._invoke( + "grep_elements", error_result, ["fs", "grep", "Login", "/nonexistent"], + ) + assert "No such directory" in result.output + assert "No matches" not in result.output diff --git a/browser/agent-harness/cli_anything/browser/utils/domshell_backend.py b/browser/agent-harness/cli_anything/browser/utils/domshell_backend.py index d4e513a6e..674f63809 100644 --- a/browser/agent-harness/cli_anything/browser/utils/domshell_backend.py +++ b/browser/agent-harness/cli_anything/browser/utils/domshell_backend.py @@ -24,6 +24,7 @@ import re import shlex import subprocess import shutil +import threading from typing import Any, Optional from mcp import ClientSession, StdioServerParameters from mcp.client.stdio import stdio_client @@ -81,6 +82,13 @@ _daemon_client_context: Optional[Any] = None # Store stdio_client context manag # to clear. _daemon_lane_id: Optional[str] = None +# Synchronizes the first-call capture of _daemon_lane_id. Concurrent +# callers of _call_execute in daemon mode without a session would +# otherwise race on the `is None` check below. Lock spans only the +# check + assignment (microsecond contention), not the MCP call. +# (Copilot R4 on commit 5790651.) +_daemon_lane_lock = threading.Lock() + def _check_npx() -> bool: """Check if npx is available.""" @@ -553,10 +561,14 @@ async def _call_execute( # Daemon-level capture on first daemon-no-session call so # subsequent calls reuse the same lane regardless of whether # the daemon stays alive or we fall back to fresh spawns. - if use_daemon and session is None and _daemon_lane_id is None: - captured = _extract_lane_id(result) - if captured: - _daemon_lane_id = captured + # `is None` check moved INSIDE the lock to fix the + # test-and-set race between concurrent callers. + if use_daemon and session is None: + with _daemon_lane_lock: + if _daemon_lane_id is None: + captured = _extract_lane_id(result) + if captured: + _daemon_lane_id = captured return result except Exception as e: # Daemon died — log diagnosability and fall back to spawning @@ -564,6 +576,7 @@ async def _call_execute( # failure mode invisible in user reports. log.warning( "DOMShell daemon call failed, respawning per-command: %s", e, + exc_info=True, ) await _stop_daemon() @@ -582,11 +595,14 @@ async def _call_execute( ) _capture_lane(session, result) # See daemon-path capture above for rationale — same - # logic applies on the fresh-spawn fall-back path. - if use_daemon and session is None and _daemon_lane_id is None: - captured = _extract_lane_id(result) - if captured: - _daemon_lane_id = captured + # logic applies on the fresh-spawn fall-back path, + # including the lock-guarded test-and-set. + if use_daemon and session is None: + with _daemon_lane_lock: + if _daemon_lane_id is None: + captured = _extract_lane_id(result) + if captured: + _daemon_lane_id = captured return result except Exception as e: raise RuntimeError(