fix: Executing cli-anything-browser --json fs ls yields no return res…#218
fix: Executing cli-anything-browser --json fs ls yields no return res…#218qingminglong wants to merge 1 commit intoHKUDS:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes the cli-anything-browser --json fs ls hang by changing the DOMShell MCP call lifecycle to avoid blocking during shutdown and by normalizing MCP CallToolResult objects into plain dicts that the CLI can JSON-serialize reliably.
Changes:
- Add configurable MCP call/close timeouts and a custom sync runner to reduce shutdown-related hangs/noise.
- Normalize MCP tool results (notably
domshell_ls) into structured dictionaries (entries,matches, etc.) for direct CLI consumption. - Add unit tests covering result normalization and safe async context close timeout behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| browser/agent-harness/cli_anything/browser/utils/domshell_backend.py | Adds timeouts, custom sync loop runner, result normalization, and altered MCP call flow to prevent --json fs ls from hanging. |
| browser/agent-harness/cli_anything/browser/tests/test_core.py | Adds tests for ls text parsing, normalization, and timeout-protected async context exits. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| loop = asyncio.new_event_loop() | ||
| old_loop = None | ||
| try: | ||
| old_loop = asyncio.get_event_loop_policy().get_event_loop() |
There was a problem hiding this comment.
_run_sync() calls get_event_loop_policy().get_event_loop() to capture the previous loop. In Python 3.11+, this can implicitly create a new event loop when none exists, and then you restore it at the end without closing it, leaking a loop for the rest of the process. Consider restoring to None unless there was an explicitly set loop, or using get_running_loop() to detect an active loop and otherwise leaving the policy’s current loop unset.
| old_loop = asyncio.get_event_loop_policy().get_event_loop() | |
| old_loop = asyncio.get_running_loop() |
| client_ctx = stdio_client(server_params) | ||
| read, write = await client_ctx.__aenter__() | ||
| session_ctx = ClientSession(read, write) | ||
| session = await session_ctx.__aenter__() | ||
| await session.initialize() | ||
| result = await asyncio.wait_for( | ||
| session.call_tool(tool_name, arguments), | ||
| timeout=MCP_CALL_TIMEOUT_SECONDS | ||
| ) | ||
| return _normalize_tool_result(tool_name, result, arguments) |
There was a problem hiding this comment.
This one-shot code path manually calls aenter on stdio_client / ClientSession but never calls aexit for either context. That can leak subprocesses/pipes (and potentially keep an orphaned npx/domshell-proxy running after the CLI prints output). Consider adding a finally block that closes both contexts using the new _safe_aexit() (with timeout) so you still avoid the observed hang while ensuring resources are released.
| def _should_ignore_loop_error(context: dict[str, Any]) -> bool: | ||
| """判断是否忽略已知的 anyio 关闭阶段噪声异常。""" | ||
| exc = context.get("exception") | ||
| if not exc: | ||
| return False | ||
| text = str(exc) | ||
| return "Attempted to exit cancel scope in a different task" in text | ||
|
|
||
|
|
||
| def _run_sync(coro): | ||
| """以可控事件循环执行协程，避免 asyncio.run 关闭阶段噪声。""" | ||
| loop = asyncio.new_event_loop() |
There was a problem hiding this comment.
The new docstrings/comments here are in Chinese while the rest of this module’s docstrings are in English. To keep the harness maintainable for the broader contributor base, please translate these docstrings/comments to English (or keep bilingual text consistently across the file).
| Example: | ||
| >>> ls("/") | ||
| {"path": "/", "entries": [{"name": "main", "role": "landmark", ...}]} | ||
| """ | ||
| result = asyncio.run(_call_tool("domshell_ls", {"options": path}, use_daemon)) | ||
| result = _run_sync(_call_tool("domshell_ls", {"options": path}, use_daemon)) | ||
| return result |
There was a problem hiding this comment.
Since the sync wrappers have been switched from asyncio.run() to _run_sync(), the NOTE above about daemon mode using asyncio.run() per tool call is now outdated/misleading. Please update the NOTE to reflect the current mechanism (and any remaining limitations) so future readers aren’t debugging the wrong thing.
| class TestBackendResultNormalization: | ||
| """测试后端返回结构标准化。""" | ||
|
|
||
| def test_parse_ls_text_to_entries(self): | ||
| """应把 ls 文本结果解析为 entries 列表。""" | ||
| text = " windows/ (1 windows)\n tabs/ (44 tabs)" |
There was a problem hiding this comment.
These new test docstrings are in Chinese while the surrounding test file uses English docstrings/comments. For consistency and easier maintenance by the rest of the team, please translate these docstrings to English (or adopt a consistent bilingual style across the file).
| result = await _daemon_session.call_tool(tool_name, arguments) | ||
| return result | ||
| return _normalize_tool_result(tool_name, result, arguments) | ||
| except Exception as e: |
There was a problem hiding this comment.
_call_tool() is now returning normalized dicts (via _normalize_tool_result) rather than the raw MCP CallToolResult, but its signature/docstring still imply it returns the MCP server result unchanged (-> Any). Please update the type hint/docstring to reflect the new return contract so callers and tests don’t have to guess.
| MCP_CALL_TIMEOUT_SECONDS = float(os.environ.get("DOMSHELL_MCP_CALL_TIMEOUT", "15")) | ||
| MCP_CLOSE_TIMEOUT_SECONDS = float(os.environ.get("DOMSHELL_MCP_CLOSE_TIMEOUT", "2")) | ||
|
|
||
|
|
There was a problem hiding this comment.
These timeout constants are parsed with float() at import time; an invalid env value (e.g., "15s") will raise ValueError and prevent the harness/CLI from importing. Consider parsing defensively (try/except with fallback to defaults) and/or validating that the value is > 0.
| MCP_CALL_TIMEOUT_SECONDS = float(os.environ.get("DOMSHELL_MCP_CALL_TIMEOUT", "15")) | |
| MCP_CLOSE_TIMEOUT_SECONDS = float(os.environ.get("DOMSHELL_MCP_CLOSE_TIMEOUT", "2")) | |
| def _get_timeout_seconds(env_var_name: str, default: float) -> float: | |
| """Parse a positive timeout value from the environment, with fallback.""" | |
| raw_value = os.environ.get(env_var_name) | |
| if raw_value is None: | |
| return default | |
| try: | |
| timeout = float(raw_value) | |
| except (TypeError, ValueError): | |
| return default | |
| if timeout <= 0: | |
| return default | |
| return timeout | |
| MCP_CALL_TIMEOUT_SECONDS = _get_timeout_seconds("DOMSHELL_MCP_CALL_TIMEOUT", 15.0) | |
| MCP_CLOSE_TIMEOUT_SECONDS = _get_timeout_seconds("DOMSHELL_MCP_CLOSE_TIMEOUT", 2.0) |
|
@qingminglong please review and resolve the issues reported above by copilot. Thx! |
|
Thanks for tackling the browser fs hang. The cleanup path still needs work: Please keep the bounded cleanup, but close the contexts in a |
…ults (fixes #217)
Description
Fixes #
Type of Change
For New Software CLIs (in-repo)
<SOFTWARE>.mdSOP document exists at<software>/agent-harness/<SOFTWARE>.mdSKILL.mdexists inside the Python package (cli_anything/<software>/SKILL.md)cli_anything/<software>/tests/test_core.pyare present and pass without backendcli_anything/<software>/tests/test_full_e2e.pyare presentREADME.mdincludes the new software (with link to harness directory)registry.jsonincludes an entry withsource_url: null(see Contributing guide)repl_skin.pyinutils/is an unmodified copy from the pluginFor New Software CLIs (standalone repo)
pip install <package-name>or apip install git+https://...URLSKILL.mdexists in the external reporegistry.jsonentry includessource_urlpointing to the external reporegistry.jsonentry includesskill_mdwith full URL to the external SKILL.mdinstall_cmdinregistry.jsonworks (tested locally)For Existing CLI Modifications
python3 -m pytest cli_anything/<software>/tests/test_core.py -vpython3 -m pytest cli_anything/<software>/tests/test_full_e2e.py -vregistry.jsonentry is updated if version, description, or requirements changedGeneral Checklist
--jsonflag is supported on any new commandsfeat:,fix:,docs:,test:)Test Results