mirror of
https://github.com/HKUDS/CLI-Anything.git
synced 2026-08-28 23:27:04 +08:00
fix(browser): R4 bot-cleanup — surface ls/grep errors, lock _daemon_lane_id, exc_info, HARNESS.md drift
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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}'")
|
||||
|
||||
@@ -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 <path>".
|
||||
"""
|
||||
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
|
||||
|
||||
@@ -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(
|
||||
|
||||
Reference in New Issue
Block a user