Merge branch 'main' of github.com:NousResearch/hermes-agent into bb/gui
# Conflicts: # tui_gateway/server.py
This commit is contained in:
Brooklyn Nicholson
@@ -189,6 +189,32 @@ class TestBrowserEvalSupervisorPath:
|
||||
json.loads(bt._browser_eval("1+1"))
|
||||
assert called["subprocess"] is True
|
||||
|
||||
def test_subprocess_reference_chain_error_becomes_guidance(self, monkeypatch):
|
||||
"""The CLI subprocess can't retry with returnByValue=False, so the
|
||||
cryptic 'Object reference chain is too long' CDP error must be turned
|
||||
into actionable guidance instead of surfaced raw."""
|
||||
import tools.browser_tool as bt
|
||||
|
||||
# No supervisor → subprocess path runs.
|
||||
_patch_supervisor(monkeypatch, None)
|
||||
|
||||
def _fake_subprocess(task_id, cmd, args):
|
||||
assert cmd == "eval"
|
||||
return {
|
||||
"success": False,
|
||||
"error": "Runtime.evaluate failed: Object reference chain is too long",
|
||||
}
|
||||
|
||||
monkeypatch.setattr(bt, "_run_browser_command", _fake_subprocess)
|
||||
|
||||
out = json.loads(bt._browser_eval("document.body"))
|
||||
assert out["success"] is False
|
||||
# Raw protocol error must NOT leak through.
|
||||
assert "reference chain" not in out["error"].lower()
|
||||
# Actionable guidance instead.
|
||||
assert "primitive" in out["error"].lower()
|
||||
assert "DOM node" in out["error"] or "dom node" in out["error"].lower()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Response shaping: CDPSupervisor.evaluate_runtime
|
||||
@@ -361,3 +387,91 @@ class TestEvaluateRuntimeResponseShaping:
|
||||
finally:
|
||||
loop.call_soon_threadsafe(loop.stop)
|
||||
thread.join(timeout=2)
|
||||
|
||||
|
||||
def _make_supervisor_with_cdp_fn(cdp_fn):
|
||||
"""Like ``_make_supervisor_with_cdp`` but lets the test supply a coroutine
|
||||
function as ``_cdp`` so behaviour can vary by params (e.g. returnByValue).
|
||||
"""
|
||||
import asyncio
|
||||
import threading
|
||||
|
||||
from tools.browser_supervisor import CDPSupervisor
|
||||
|
||||
sup = object.__new__(CDPSupervisor)
|
||||
sup._state_lock = threading.Lock()
|
||||
sup._active = True
|
||||
sup._page_session_id = "test-session-id"
|
||||
|
||||
loop = asyncio.new_event_loop()
|
||||
|
||||
def _runner():
|
||||
asyncio.set_event_loop(loop)
|
||||
loop.run_forever()
|
||||
|
||||
thread = threading.Thread(target=_runner, daemon=True)
|
||||
thread.start()
|
||||
|
||||
sup._cdp = cdp_fn # type: ignore[method-assign]
|
||||
sup._loop = loop
|
||||
sup._thread = thread
|
||||
return sup
|
||||
|
||||
|
||||
class TestEvaluateRuntimeDomNodeCrashRetry:
|
||||
"""returnByValue=True on a DOM node fails CDP serialization with 'Object
|
||||
reference chain is too long'. evaluate_runtime must retry with
|
||||
returnByValue=False and return the node's description instead of crashing.
|
||||
"""
|
||||
|
||||
def test_reference_chain_crash_retries_without_by_value(self):
|
||||
calls = []
|
||||
|
||||
async def _fake_cdp(method, params=None, *, session_id=None, timeout=10.0):
|
||||
by_value = (params or {}).get("returnByValue")
|
||||
calls.append(by_value)
|
||||
if by_value:
|
||||
# Mirror _read_loop turning a top-level CDP error into a RuntimeError.
|
||||
raise RuntimeError(
|
||||
"CDP error on id=7: {'code': -32000, "
|
||||
"'message': 'Object reference chain is too long'}"
|
||||
)
|
||||
# returnByValue=False: Chrome returns the node's description, no value.
|
||||
return {
|
||||
"id": 8,
|
||||
"result": {
|
||||
"result": {
|
||||
"type": "object",
|
||||
"subtype": "node",
|
||||
"description": "body",
|
||||
}
|
||||
},
|
||||
}
|
||||
|
||||
sup = _make_supervisor_with_cdp_fn(_fake_cdp)
|
||||
try:
|
||||
out = sup.evaluate_runtime("document.body")
|
||||
assert out["ok"] is True
|
||||
assert out["result"] == "body"
|
||||
assert out["result_type"] == "object"
|
||||
# First call by_value=True (crashed), retried with by_value=False.
|
||||
assert calls == [True, False]
|
||||
finally:
|
||||
_stop_supervisor(sup)
|
||||
|
||||
def test_unrelated_error_does_not_retry(self):
|
||||
calls = []
|
||||
|
||||
async def _fake_cdp(method, params=None, *, session_id=None, timeout=10.0):
|
||||
calls.append((params or {}).get("returnByValue"))
|
||||
raise RuntimeError("CDP error on id=3: {'message': 'Target closed'}")
|
||||
|
||||
sup = _make_supervisor_with_cdp_fn(_fake_cdp)
|
||||
try:
|
||||
out = sup.evaluate_runtime("document.body")
|
||||
assert out["ok"] is False
|
||||
assert "Target closed" in out["error"]
|
||||
# No retry for unrelated failures — exactly one call.
|
||||
assert calls == [True]
|
||||
finally:
|
||||
_stop_supervisor(sup)
|
||||
|
||||
@@ -345,15 +345,23 @@ class TestShellFileOpsHelpers:
|
||||
def test_add_line_numbers(self, file_ops):
|
||||
content = "line one\nline two\nline three"
|
||||
result = file_ops._add_line_numbers(content)
|
||||
assert " 1|line one" in result
|
||||
assert " 2|line two" in result
|
||||
assert " 3|line three" in result
|
||||
# Compact gutter: "<n>|content" (no fixed-width padding).
|
||||
assert "1|line one" in result
|
||||
assert "2|line two" in result
|
||||
assert "3|line three" in result
|
||||
|
||||
def test_add_line_numbers_with_offset(self, file_ops):
|
||||
content = "continued\nmore"
|
||||
result = file_ops._add_line_numbers(content, start_line=50)
|
||||
assert " 50|continued" in result
|
||||
assert " 51|more" in result
|
||||
assert "50|continued" in result
|
||||
assert "51|more" in result
|
||||
|
||||
def test_add_line_numbers_padded_env_override(self, file_ops, monkeypatch):
|
||||
# Legacy fixed-width format available via HERMES_READ_GUTTER=padded.
|
||||
monkeypatch.setenv("HERMES_READ_GUTTER", "padded")
|
||||
result = file_ops._add_line_numbers("line one\nline two")
|
||||
assert " 1|line one" in result
|
||||
assert " 2|line two" in result
|
||||
|
||||
def test_add_line_numbers_truncates_long_lines(self, file_ops):
|
||||
long_line = "x" * (MAX_LINE_LENGTH + 100)
|
||||
@@ -405,7 +413,7 @@ class TestShellFileOpsHelpers:
|
||||
assert "HERMES_FENCE" not in result.content
|
||||
assert "\x1b]" not in result.content
|
||||
assert "\x07" not in result.content
|
||||
assert " 1|print('ok')" in result.content
|
||||
assert "1|print('ok')" in result.content
|
||||
|
||||
def test_read_file_raw_strips_leaked_terminal_fence_markers(self, mock_env):
|
||||
leaked = (
|
||||
@@ -638,12 +646,14 @@ class TestPatchReplacePostWriteVerification:
|
||||
state = {"content": "hello world\n"}
|
||||
|
||||
def side_effect(command, stdin_data=None, **kwargs):
|
||||
# Write is `cat > path` — detect by the `>` redirect, NOT just `cat `
|
||||
if command.startswith("cat >"):
|
||||
if stdin_data is not None:
|
||||
state["content"] = stdin_data
|
||||
# A write is the only call that pipes content over stdin — key
|
||||
# on that behavioral signal rather than the exact write command,
|
||||
# which is an atomic temp-file + mv script (`set -e; ... mv ...`),
|
||||
# not a bare `cat > path`.
|
||||
if stdin_data is not None:
|
||||
state["content"] = stdin_data
|
||||
return {"output": "", "returncode": 0}
|
||||
if command.startswith("cat "): # read
|
||||
if command.startswith("cat "): # read / verify
|
||||
return {"output": state["content"], "returncode": 0}
|
||||
if command.startswith("mkdir "):
|
||||
return {"output": "", "returncode": 0}
|
||||
@@ -664,9 +674,8 @@ class TestPatchReplacePostWriteVerification:
|
||||
state = {"content": "hello world\n"}
|
||||
|
||||
def side_effect(command, stdin_data=None, **kwargs):
|
||||
if command.startswith("cat >"): # write
|
||||
if stdin_data is not None:
|
||||
state["content"] = stdin_data
|
||||
if stdin_data is not None: # write (atomic temp-file + mv script)
|
||||
state["content"] = stdin_data
|
||||
return {"output": "", "returncode": 0}
|
||||
if command.startswith("cat "): # read
|
||||
call_count["cat"] += 1
|
||||
|
||||
@@ -292,7 +292,7 @@ class TestPaginationBounds:
|
||||
result = ops.read_file("notes.txt", offset=0, limit=0)
|
||||
|
||||
assert result.error is None
|
||||
assert " 1|line1" in result.content
|
||||
assert "1|line1" in result.content
|
||||
sed_commands = [cmd for cmd in commands if cmd.startswith("sed -n")]
|
||||
assert sed_commands == ["sed -n '1,1p' 'notes.txt'"]
|
||||
|
||||
|
||||
@@ -0,0 +1,197 @@
|
||||
"""Regression tests for file-tool path resolution base correctness.
|
||||
|
||||
The bug (observed in a worktree dev session, May 2026): when the resolution
|
||||
base for a relative path is itself RELATIVE — e.g. ``TERMINAL_CWD="."`` from a
|
||||
stale config — ``_resolve_path_for_task`` resolved the path against the agent's
|
||||
PROCESS cwd instead of the intended workspace. In a git-worktree session this
|
||||
silently routed ``patch``/``write_file`` edits into the *main* checkout: the
|
||||
write landed, self-verified, and reported success — against the wrong file.
|
||||
The agent then grepped the worktree, saw nothing, and concluded the patch tool
|
||||
had silently no-op'd. It hadn't; it wrote to the wrong place.
|
||||
|
||||
Core invariant these tests pin:
|
||||
The resolution base for a relative path MUST always be absolute. A relative
|
||||
``TERMINAL_CWD`` (``.``, ``./sub``, ``..``) must be anchored deterministically,
|
||||
never left to resolve against whatever the process cwd happens to be.
|
||||
"""
|
||||
|
||||
import os
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
import tools.file_tools as ft
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def _isolated_cwd(tmp_path, monkeypatch):
|
||||
"""Two checkouts: workspace (intended) + decoy (process cwd)."""
|
||||
workspace = tmp_path / "workspace"
|
||||
decoy = tmp_path / "decoy"
|
||||
workspace.mkdir()
|
||||
decoy.mkdir()
|
||||
(workspace / "target.py").write_text("WORKSPACE_ORIGINAL\n")
|
||||
(decoy / "target.py").write_text("DECOY_ORIGINAL\n")
|
||||
# Process cwd = decoy, analogous to "main repo" while the terminal is in
|
||||
# the worktree.
|
||||
monkeypatch.chdir(decoy)
|
||||
# No live-terminal-cwd tracking recorded yet (fresh-session condition).
|
||||
monkeypatch.setattr(ft, "_get_live_tracking_cwd", lambda task_id="default": None)
|
||||
return workspace, decoy
|
||||
|
||||
|
||||
def test_relative_terminal_cwd_anchors_to_absolute_not_process_cwd(_isolated_cwd, monkeypatch):
|
||||
"""TERMINAL_CWD='.' must NOT silently mean 'the agent process cwd'.
|
||||
|
||||
A relative base is meaningless as a resolution anchor. The resolver must
|
||||
make it absolute deterministically. We assert the resolved path is
|
||||
absolute and stable regardless of where os.getcwd() points.
|
||||
"""
|
||||
workspace, decoy = _isolated_cwd
|
||||
# Poison config: literal relative '.'
|
||||
monkeypatch.setenv("TERMINAL_CWD", ".")
|
||||
|
||||
resolved = ft._resolve_path_for_task("target.py", task_id="default")
|
||||
|
||||
assert resolved.is_absolute(), f"resolution base leaked a relative path: {resolved}"
|
||||
# The exact anchor for a bare '.' is the process cwd resolved to absolute —
|
||||
# that is acceptable as long as it is ABSOLUTE and stable. The bug was that
|
||||
# a relative base produced surprising results; the fix is that the base is
|
||||
# always absolutised. (We do not require it to point at the workspace here —
|
||||
# that's what live-cwd tracking is for; see the next test.)
|
||||
assert str(resolved) == str((Path(os.getcwd()) / "target.py").resolve())
|
||||
|
||||
|
||||
def test_live_tracking_cwd_wins_over_relative_terminal_cwd(_isolated_cwd, monkeypatch):
|
||||
"""When the terminal reports its absolute cwd, that is authoritative.
|
||||
|
||||
This is the real-world fix: the terminal's tracked absolute cwd (the
|
||||
worktree) must override a stale relative TERMINAL_CWD so edits land where
|
||||
the agent is actually working.
|
||||
"""
|
||||
workspace, decoy = _isolated_cwd
|
||||
monkeypatch.setenv("TERMINAL_CWD", ".")
|
||||
monkeypatch.setattr(ft, "_get_live_tracking_cwd", lambda task_id="default": str(workspace))
|
||||
|
||||
resolved = ft._resolve_path_for_task("target.py", task_id="default")
|
||||
|
||||
assert resolved == (workspace / "target.py")
|
||||
|
||||
|
||||
def test_absolute_terminal_cwd_used_verbatim(_isolated_cwd, monkeypatch):
|
||||
"""An absolute TERMINAL_CWD is the resolution base (no live tracking)."""
|
||||
workspace, decoy = _isolated_cwd
|
||||
monkeypatch.setenv("TERMINAL_CWD", str(workspace))
|
||||
|
||||
resolved = ft._resolve_path_for_task("target.py", task_id="default")
|
||||
|
||||
assert resolved == (workspace / "target.py")
|
||||
|
||||
|
||||
def test_absolute_input_path_ignores_base(_isolated_cwd, monkeypatch):
|
||||
"""An absolute input path is never re-anchored."""
|
||||
workspace, decoy = _isolated_cwd
|
||||
monkeypatch.setenv("TERMINAL_CWD", ".")
|
||||
abs_target = str(workspace / "target.py")
|
||||
|
||||
resolved = ft._resolve_path_for_task(abs_target, task_id="default")
|
||||
|
||||
assert resolved == Path(abs_target).resolve()
|
||||
|
||||
|
||||
def test_resolution_base_always_absolute_no_terminal_cwd(_isolated_cwd, monkeypatch):
|
||||
"""With TERMINAL_CWD unset, the base falls back to an ABSOLUTE process cwd."""
|
||||
workspace, decoy = _isolated_cwd
|
||||
monkeypatch.delenv("TERMINAL_CWD", raising=False)
|
||||
|
||||
resolved = ft._resolve_path_for_task("target.py", task_id="default")
|
||||
|
||||
assert resolved.is_absolute()
|
||||
assert str(resolved) == str((Path(os.getcwd()) / "target.py").resolve())
|
||||
|
||||
|
||||
# ── B-(ii): workspace-divergence warning ────────────────────────────────────
|
||||
|
||||
|
||||
def test_warning_fires_when_relative_path_escapes_workspace(_isolated_cwd, monkeypatch):
|
||||
"""Relative path resolving outside the live workspace must warn."""
|
||||
workspace, decoy = _isolated_cwd
|
||||
# Live cwd = workspace, but the relative path resolves to decoy (process cwd)
|
||||
# because TERMINAL_CWD is the poison '.'. Simulate by pointing live tracking
|
||||
# at workspace while the resolved path is under decoy.
|
||||
monkeypatch.setattr(ft, "_get_live_tracking_cwd", lambda task_id="default": str(workspace))
|
||||
resolved_in_decoy = decoy / "target.py"
|
||||
|
||||
warn = ft._path_resolution_warning("target.py", resolved_in_decoy, task_id="default")
|
||||
|
||||
assert warn is not None
|
||||
assert "OUTSIDE the active workspace" in warn
|
||||
assert str(decoy) in warn
|
||||
assert str(workspace) in warn
|
||||
|
||||
|
||||
def test_no_warning_when_relative_path_inside_workspace(_isolated_cwd, monkeypatch):
|
||||
workspace, decoy = _isolated_cwd
|
||||
monkeypatch.setattr(ft, "_get_live_tracking_cwd", lambda task_id="default": str(workspace))
|
||||
resolved_in_workspace = workspace / "target.py"
|
||||
|
||||
warn = ft._path_resolution_warning("target.py", resolved_in_workspace, task_id="default")
|
||||
|
||||
assert warn is None
|
||||
|
||||
|
||||
def test_no_warning_for_absolute_input(_isolated_cwd, monkeypatch):
|
||||
workspace, decoy = _isolated_cwd
|
||||
monkeypatch.setattr(ft, "_get_live_tracking_cwd", lambda task_id="default": str(workspace))
|
||||
|
||||
warn = ft._path_resolution_warning(str(decoy / "target.py"), decoy / "target.py", task_id="default")
|
||||
|
||||
assert warn is None
|
||||
|
||||
|
||||
def test_no_warning_when_no_live_cwd(_isolated_cwd, monkeypatch):
|
||||
workspace, decoy = _isolated_cwd
|
||||
monkeypatch.setattr(ft, "_get_live_tracking_cwd", lambda task_id="default": None)
|
||||
|
||||
warn = ft._path_resolution_warning("target.py", decoy / "target.py", task_id="default")
|
||||
|
||||
assert warn is None
|
||||
|
||||
|
||||
# ── Fix A: write_file / patch report the resolved ABSOLUTE path ──────────────
|
||||
|
||||
|
||||
def test_write_file_reports_resolved_absolute_path(_isolated_cwd, monkeypatch):
|
||||
"""write_file_tool must put the absolute on-disk path in files_modified."""
|
||||
workspace, decoy = _isolated_cwd
|
||||
monkeypatch.setattr(ft, "_get_live_tracking_cwd", lambda task_id="default": str(workspace))
|
||||
|
||||
import json
|
||||
out = json.loads(ft.write_file_tool("newfile.txt", "hello\n", task_id="t1"))
|
||||
|
||||
expected = str((workspace / "newfile.txt").resolve())
|
||||
assert out.get("resolved_path") == expected
|
||||
assert out.get("files_modified") == [expected]
|
||||
assert (workspace / "newfile.txt").read_text() == "hello\n"
|
||||
|
||||
|
||||
def test_patch_reports_resolved_absolute_path(_isolated_cwd, monkeypatch):
|
||||
"""patch_tool (replace mode) must put the absolute on-disk path in files_modified."""
|
||||
workspace, decoy = _isolated_cwd
|
||||
monkeypatch.setattr(ft, "_get_live_tracking_cwd", lambda task_id="default": str(workspace))
|
||||
|
||||
import json
|
||||
out = json.loads(ft.patch_tool(
|
||||
mode="replace", path="target.py",
|
||||
old_string="WORKSPACE_ORIGINAL", new_string="WORKSPACE_PATCHED",
|
||||
task_id="t1",
|
||||
))
|
||||
|
||||
expected = str((workspace / "target.py").resolve())
|
||||
assert not out.get("error"), out
|
||||
assert out.get("resolved_path") == expected
|
||||
assert out.get("files_modified") == [expected]
|
||||
assert "WORKSPACE_PATCHED" in (workspace / "target.py").read_text()
|
||||
# And the decoy copy is untouched.
|
||||
assert (decoy / "target.py").read_text() == "DECOY_ORIGINAL\n"
|
||||
|
||||
@@ -107,5 +107,177 @@ class TestCheckSensitivePathMacOSBypass:
|
||||
assert _check_sensitive_path("/tmp/safe_file.txt") is None
|
||||
|
||||
|
||||
class TestAtomicWrite:
|
||||
"""write_file / patch land via a temp-file + atomic rename.
|
||||
|
||||
The invariant: a write that fails partway NEVER corrupts the existing
|
||||
file, and the swap is a real rename (so a reader either sees the full
|
||||
old content or the full new content, never a half-written file). These
|
||||
run against a real LocalEnvironment so the actual shell script executes.
|
||||
"""
|
||||
|
||||
@pytest.fixture
|
||||
def ops(self, tmp_path: Path):
|
||||
from tools.environments.local import LocalEnvironment
|
||||
from tools.file_operations import ShellFileOperations
|
||||
env = LocalEnvironment(cwd=str(tmp_path))
|
||||
return ShellFileOperations(env, cwd=str(tmp_path))
|
||||
|
||||
def test_overwrite_changes_inode(self, ops, tmp_path: Path):
|
||||
# A real rename allocates a new inode for the target; an in-place
|
||||
# rewrite would keep the same inode. This proves the swap is atomic.
|
||||
target = tmp_path / "f.txt"
|
||||
target.write_text("v1")
|
||||
ino_before = os.stat(target).st_ino
|
||||
res = ops.write_file(str(target), "v2 content")
|
||||
assert res.error is None, res.error
|
||||
assert target.read_text() == "v2 content"
|
||||
assert os.stat(target).st_ino != ino_before
|
||||
|
||||
def test_overwrite_preserves_mode(self, ops, tmp_path: Path):
|
||||
target = tmp_path / "perms.txt"
|
||||
target.write_text("old")
|
||||
os.chmod(target, 0o640)
|
||||
res = ops.write_file(str(target), "new")
|
||||
assert res.error is None, res.error
|
||||
assert (os.stat(target).st_mode & 0o777) == 0o640
|
||||
|
||||
def test_failed_write_leaves_original_intact(self, ops, tmp_path: Path):
|
||||
# A read-only parent directory means the temp file can't be created,
|
||||
# so the write fails BEFORE any rename. The original must survive
|
||||
# byte-for-byte and no temp file may be left behind.
|
||||
if hasattr(os, "geteuid") and os.geteuid() == 0:
|
||||
pytest.skip("root bypasses directory permission bits")
|
||||
locked = tmp_path / "locked"
|
||||
locked.mkdir()
|
||||
target = locked / "f.txt"
|
||||
target.write_text("ORIGINAL\n")
|
||||
os.chmod(locked, 0o500) # r-x: cannot create entries inside
|
||||
try:
|
||||
res = ops.write_file(str(target), "SHOULD NOT LAND")
|
||||
finally:
|
||||
os.chmod(locked, 0o700) # restore for cleanup
|
||||
assert res.error is not None
|
||||
assert target.read_text() == "ORIGINAL\n"
|
||||
assert [p for p in os.listdir(locked) if ".hermes-tmp" in p] == []
|
||||
|
||||
def test_no_temp_file_leaked_on_success(self, ops, tmp_path: Path):
|
||||
target = tmp_path / "f.txt"
|
||||
ops.write_file(str(target), "hello\n")
|
||||
assert [p for p in os.listdir(tmp_path) if ".hermes-tmp" in p] == []
|
||||
|
||||
def test_special_chars_roundtrip(self, ops, tmp_path: Path):
|
||||
target = tmp_path / "special.txt"
|
||||
tricky = "q 'single' \"double\" $VAR `cmd` \\back\nünïcödé 日本語\n"
|
||||
res = ops.write_file(str(target), tricky)
|
||||
assert res.error is None, res.error
|
||||
assert target.read_text(encoding="utf-8") == tricky
|
||||
|
||||
def test_patch_routes_through_atomic_write(self, ops, tmp_path: Path):
|
||||
target = tmp_path / "edit.py"
|
||||
target.write_text("a = 1\nb = 2\nc = 3\n")
|
||||
os.chmod(target, 0o600)
|
||||
res = ops.patch_replace(str(target), "b = 2", "b = 22")
|
||||
assert res.success, res.error
|
||||
assert target.read_text() == "a = 1\nb = 22\nc = 3\n"
|
||||
assert (os.stat(target).st_mode & 0o777) == 0o600
|
||||
|
||||
|
||||
class TestBomHandling:
|
||||
"""UTF-8 BOM is stripped on read and preserved across write/patch.
|
||||
|
||||
A BOM (U+FEFF, bytes EF BB BF) is an invisible leading marker some
|
||||
Windows editors prepend. The agent should never see it in read output,
|
||||
but a file that had one on disk must keep it after an edit so the byte
|
||||
signature is preserved.
|
||||
"""
|
||||
|
||||
BOM = "\ufeff"
|
||||
|
||||
@pytest.fixture
|
||||
def ops(self, tmp_path: Path):
|
||||
from tools.environments.local import LocalEnvironment
|
||||
from tools.file_operations import ShellFileOperations
|
||||
env = LocalEnvironment(cwd=str(tmp_path))
|
||||
return ShellFileOperations(env, cwd=str(tmp_path))
|
||||
|
||||
def test_helpers(self):
|
||||
from tools.file_operations import _strip_bom, _has_bom
|
||||
assert _strip_bom("\ufeffhello") == ("hello", True)
|
||||
assert _strip_bom("hello") == ("hello", False)
|
||||
assert _strip_bom("") == ("", False)
|
||||
# mid-string BOM is data, not a marker — left alone
|
||||
assert _strip_bom("a\ufeffb") == ("a\ufeffb", False)
|
||||
assert _has_bom("\ufeffx") is True
|
||||
assert _has_bom("x") is False
|
||||
assert _has_bom(None) is False
|
||||
|
||||
def test_read_strips_bom(self, ops, tmp_path: Path):
|
||||
target = tmp_path / "bom.py"
|
||||
# Write raw bytes with a real UTF-8 BOM prefix.
|
||||
target.write_bytes(self.BOM.encode("utf-8") + b"import os\nx = 1\n")
|
||||
res = ops.read_file(str(target))
|
||||
assert res.error is None, res.error
|
||||
# Line 1 content must NOT carry the phantom U+FEFF.
|
||||
first_line = res.content.split("\n", 1)[0]
|
||||
assert self.BOM not in first_line
|
||||
assert first_line.endswith("import os")
|
||||
|
||||
def test_read_raw_strips_bom(self, ops, tmp_path: Path):
|
||||
target = tmp_path / "bom.txt"
|
||||
target.write_bytes(self.BOM.encode("utf-8") + b"hello\nworld\n")
|
||||
res = ops.read_file_raw(str(target))
|
||||
assert res.error is None, res.error
|
||||
assert not res.content.startswith(self.BOM)
|
||||
assert res.content == "hello\nworld\n"
|
||||
|
||||
def test_write_preserves_bom(self, ops, tmp_path: Path):
|
||||
# Existing file has a BOM; agent rewrites with BOM-less content.
|
||||
target = tmp_path / "config.txt"
|
||||
target.write_bytes(self.BOM.encode("utf-8") + b"old\n")
|
||||
res = ops.write_file(str(target), "new content\n")
|
||||
assert res.error is None, res.error
|
||||
raw = target.read_bytes()
|
||||
assert raw.startswith(self.BOM.encode("utf-8")) # BOM restored
|
||||
assert raw == self.BOM.encode("utf-8") + b"new content\n"
|
||||
|
||||
def test_write_no_bom_when_original_had_none(self, ops, tmp_path: Path):
|
||||
target = tmp_path / "plain.txt"
|
||||
target.write_text("old\n")
|
||||
res = ops.write_file(str(target), "new\n")
|
||||
assert res.error is None, res.error
|
||||
assert not target.read_bytes().startswith(self.BOM.encode("utf-8"))
|
||||
|
||||
def test_write_does_not_double_bom(self, ops, tmp_path: Path):
|
||||
# If content already carries a BOM and the file had one, don't add a
|
||||
# second.
|
||||
target = tmp_path / "config.txt"
|
||||
target.write_bytes(self.BOM.encode("utf-8") + b"old\n")
|
||||
res = ops.write_file(str(target), self.BOM + "new\n")
|
||||
assert res.error is None, res.error
|
||||
raw = target.read_bytes()
|
||||
# exactly one BOM
|
||||
assert raw == self.BOM.encode("utf-8") + b"new\n"
|
||||
|
||||
def test_patch_roundtrip_preserves_bom(self, ops, tmp_path: Path):
|
||||
target = tmp_path / "edit.py"
|
||||
target.write_bytes(self.BOM.encode("utf-8") + b"a = 1\nb = 2\nc = 3\n")
|
||||
res = ops.patch_replace(str(target), "b = 2", "b = 22")
|
||||
assert res.success, res.error
|
||||
raw = target.read_bytes()
|
||||
assert raw.startswith(self.BOM.encode("utf-8")) # marker survived
|
||||
assert raw == self.BOM.encode("utf-8") + b"a = 1\nb = 22\nc = 3\n"
|
||||
|
||||
def test_patch_matches_first_line_through_bom(self, ops, tmp_path: Path):
|
||||
# The whole point: an edit targeting the BOM-prefixed first line
|
||||
# must match cleanly (the matcher sees BOM-stripped content).
|
||||
target = tmp_path / "mod.py"
|
||||
target.write_bytes(self.BOM.encode("utf-8") + b"import os\nimport sys\n")
|
||||
res = ops.patch_replace(str(target), "import os", "import os, json")
|
||||
assert res.success, res.error
|
||||
raw = target.read_bytes()
|
||||
assert raw == self.BOM.encode("utf-8") + b"import os, json\nimport sys\n"
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
pytest.main([__file__, "-v"])
|
||||
|
||||
@@ -203,6 +203,83 @@ class TestSIGKILLEscalation:
|
||||
assert "interrupted" in result_holder["value"]["output"].lower()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Regression: _run_tool cleanup on BaseException (issue #35309)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestRunToolCleanupOnBaseException:
|
||||
"""Verify that _run_tool cleans up _interrupted_threads even when
|
||||
_invoke_tool raises a BaseException (e.g. CancelledError).
|
||||
|
||||
Regression test for #35309: without the finally block, a BaseException
|
||||
bypasses ``except Exception``, leaking the worker tid into
|
||||
_interrupted_threads. ThreadPoolExecutor recycles tids, so the next
|
||||
tool scheduled on the same thread is instantly "interrupted".
|
||||
"""
|
||||
|
||||
def test_cleanup_on_base_exception(self):
|
||||
from unittest.mock import MagicMock, patch
|
||||
import types
|
||||
from tools.interrupt import set_interrupt, is_interrupted, _interrupted_threads, _lock
|
||||
|
||||
# Clear global state
|
||||
with _lock:
|
||||
_interrupted_threads.clear()
|
||||
|
||||
# Build a minimal mock agent with the attributes _run_tool needs
|
||||
agent = MagicMock()
|
||||
agent._interrupt_requested = False
|
||||
agent._tool_worker_threads = set()
|
||||
agent._tool_worker_threads_lock = threading.Lock()
|
||||
|
||||
# _set_interrupt delegates to the real module
|
||||
def _mock_set_interrupt(active, tid=None):
|
||||
set_interrupt(active, tid)
|
||||
agent._set_interrupt = _mock_set_interrupt
|
||||
|
||||
# _invoke_tool raises BaseException (simulating CancelledError)
|
||||
agent._invoke_tool = MagicMock(side_effect=BaseException("simulated CancelledError"))
|
||||
|
||||
# Bind the real concurrent method so we get _run_tool
|
||||
from run_agent import AIAgent
|
||||
agent._execute_tool_calls_concurrent = types.MethodType(
|
||||
AIAgent._execute_tool_calls_concurrent, agent
|
||||
)
|
||||
|
||||
# Build a single tool call
|
||||
tc = MagicMock()
|
||||
tc.id = "tc_base_exc"
|
||||
tc.function.name = "dummy_tool"
|
||||
tc.function.arguments = "{}"
|
||||
|
||||
assistant_msg = MagicMock()
|
||||
assistant_msg.tool_calls = [tc]
|
||||
|
||||
# _execute_tool_calls_concurrent will submit _run_tool to a
|
||||
# ThreadPoolExecutor. The BaseException propagates out of the
|
||||
# worker, but the finally block should still clean up.
|
||||
try:
|
||||
agent._execute_tool_calls_concurrent(assistant_msg, [], "default")
|
||||
except Exception:
|
||||
pass # ThreadPoolExecutor may re-raise
|
||||
|
||||
# After the worker finishes (even with BaseException), the worker
|
||||
# tid should have been removed from _interrupted_threads and
|
||||
# _tool_worker_threads.
|
||||
assert len(agent._tool_worker_threads) == 0, (
|
||||
f"_tool_worker_threads not cleaned up: {agent._tool_worker_threads}"
|
||||
)
|
||||
|
||||
# Verify no stale tid is left in the global interrupt set. The
|
||||
# worker thread is recycled by ThreadPoolExecutor, so a leaked tid
|
||||
# would poison the next task on that thread. We cleared the set at
|
||||
# the start and never set any interrupt ourselves, so a leak from
|
||||
# _run_tool is the only way an entry could land here.
|
||||
with _lock:
|
||||
leaked = set(_interrupted_threads)
|
||||
assert leaked == set(), f"leaked tids in _interrupted_threads: {leaked}"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Manual smoke test checklist (not automated)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@@ -234,6 +234,44 @@ def test_browserbase_does_not_use_gateway_only_configuration():
|
||||
assert provider.is_available() is False
|
||||
|
||||
|
||||
def test_browser_use_availability_skips_refresh_for_expired_cached_gateway_token(tmp_path, monkeypatch):
|
||||
_install_fake_tools_package()
|
||||
monkeypatch.delenv("TOOL_GATEWAY_USER_TOKEN", raising=False)
|
||||
expired_at = "2000-01-01T00:00:00+00:00"
|
||||
(tmp_path / "auth.json").write_text(
|
||||
'{"providers":{"nous":{"access_token":"expired-token","refresh_token":"refresh-token","expires_at":"%s"}}}'
|
||||
% expired_at,
|
||||
encoding="utf-8",
|
||||
)
|
||||
refresh_calls = []
|
||||
|
||||
def _record_refresh(*, refresh_skew_seconds=120, **_kwargs):
|
||||
refresh_calls.append(refresh_skew_seconds)
|
||||
return "fresh-token"
|
||||
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.auth.resolve_nous_access_token",
|
||||
_record_refresh,
|
||||
)
|
||||
|
||||
env = os.environ.copy()
|
||||
env.pop("BROWSER_USE_API_KEY", None)
|
||||
env.update({
|
||||
"HERMES_HOME": str(tmp_path),
|
||||
"BROWSER_USE_GATEWAY_URL": "http://127.0.0.1:3009",
|
||||
})
|
||||
|
||||
with patch.dict(os.environ, env, clear=True):
|
||||
browser_use_module = _load_plugin_module(
|
||||
"plugins.browser.browser_use.provider",
|
||||
"browser/browser_use/provider.py",
|
||||
)
|
||||
provider = browser_use_module.BrowserUseBrowserProvider()
|
||||
assert provider.is_available() is True
|
||||
|
||||
assert refresh_calls == []
|
||||
|
||||
|
||||
def test_browser_use_managed_gateway_adds_idempotency_key_and_persists_external_call_id():
|
||||
_install_fake_tools_package()
|
||||
env = os.environ.copy()
|
||||
|
||||
@@ -12,6 +12,7 @@ assert MODULE_SPEC and MODULE_SPEC.loader
|
||||
managed_tool_gateway = module_from_spec(MODULE_SPEC)
|
||||
sys.modules[MODULE_SPEC.name] = managed_tool_gateway
|
||||
MODULE_SPEC.loader.exec_module(managed_tool_gateway)
|
||||
is_managed_tool_gateway_ready = managed_tool_gateway.is_managed_tool_gateway_ready
|
||||
resolve_managed_tool_gateway = managed_tool_gateway.resolve_managed_tool_gateway
|
||||
|
||||
|
||||
@@ -97,3 +98,37 @@ def test_read_nous_access_token_refreshes_expiring_cached_token(tmp_path, monkey
|
||||
)
|
||||
|
||||
assert managed_tool_gateway.read_nous_access_token() == "fresh-token"
|
||||
|
||||
|
||||
def test_is_managed_tool_gateway_ready_skips_refresh_for_expired_cached_token(tmp_path, monkeypatch):
|
||||
monkeypatch.delenv("TOOL_GATEWAY_USER_TOKEN", raising=False)
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
expired_at = (datetime.now(timezone.utc) - timedelta(seconds=30)).isoformat()
|
||||
(tmp_path / "auth.json").write_text(json.dumps({
|
||||
"providers": {
|
||||
"nous": {
|
||||
"access_token": "expired-token",
|
||||
"refresh_token": "refresh-token",
|
||||
"expires_at": expired_at,
|
||||
}
|
||||
}
|
||||
}))
|
||||
refresh_calls = []
|
||||
|
||||
def _record_refresh(*, refresh_skew_seconds=120, **_kwargs):
|
||||
refresh_calls.append(refresh_skew_seconds)
|
||||
return "fresh-token"
|
||||
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.auth.resolve_nous_access_token",
|
||||
_record_refresh,
|
||||
)
|
||||
|
||||
with patch.dict(
|
||||
os.environ,
|
||||
{"TOOL_GATEWAY_DOMAIN": "nousresearch.com"},
|
||||
clear=False,
|
||||
), patch.object(managed_tool_gateway, "managed_nous_tools_enabled", return_value=True):
|
||||
assert is_managed_tool_gateway_ready("modal") is True
|
||||
|
||||
assert refresh_calls == []
|
||||
|
||||
@@ -1,9 +1,12 @@
|
||||
"""Tests for MCP stability fixes — event loop handler, PID tracking, shutdown robustness."""
|
||||
|
||||
import asyncio
|
||||
import os
|
||||
import signal
|
||||
from unittest.mock import patch, MagicMock
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -171,6 +174,221 @@ class TestStdioPidTracking:
|
||||
assert fake_pid not in _orphan_stdio_pids
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Fix 2b: stdio descendant reaping via process group (issue #23799)
|
||||
# ---------------------------------------------------------------------------
|
||||
#
|
||||
# When a stdio MCP wrapper (e.g. ``openclaw mcp serve``) itself spawns a
|
||||
# helper subprocess (``claude mcp serve``) and then exits, the helper
|
||||
# reparents to systemd-user and is invisible to the per-pid orphan reaper.
|
||||
# The fix captures the wrapper's pgid at spawn time and reaps via killpg,
|
||||
# which reaches same-group descendants whether or not the direct pid is alive.
|
||||
|
||||
class TestStdioPgroupReaping:
|
||||
"""_kill_orphaned_mcp_children reaps via killpg when a pgid is tracked."""
|
||||
|
||||
def _reset_state(self):
|
||||
from tools.mcp_tool import _stdio_pids, _orphan_stdio_pids, _stdio_pgids, _lock
|
||||
with _lock:
|
||||
_stdio_pids.clear()
|
||||
_orphan_stdio_pids.clear()
|
||||
_stdio_pgids.clear()
|
||||
|
||||
def test_killpg_used_when_pgid_tracked(self, monkeypatch):
|
||||
"""SIGTERM and SIGKILL route through killpg when pgid is known."""
|
||||
from tools.mcp_tool import (
|
||||
_kill_orphaned_mcp_children,
|
||||
_orphan_stdio_pids,
|
||||
_stdio_pgids,
|
||||
_lock,
|
||||
)
|
||||
|
||||
self._reset_state()
|
||||
fake_pid = 525252
|
||||
fake_pgid = 525252 # session leader: pgid == pid
|
||||
with _lock:
|
||||
_orphan_stdio_pids.add(fake_pid)
|
||||
_stdio_pgids[fake_pid] = fake_pgid
|
||||
|
||||
fake_sigkill = 9
|
||||
monkeypatch.setattr(signal, "SIGKILL", fake_sigkill, raising=False)
|
||||
|
||||
# Ensure os.killpg exists on this platform for the test to make sense;
|
||||
# the production fallback path is covered by the per-pid tests above.
|
||||
if not hasattr(os, "killpg"):
|
||||
pytest.skip("os.killpg not available on this platform")
|
||||
|
||||
with patch("tools.mcp_tool.os.killpg") as mock_killpg, \
|
||||
patch("tools.mcp_tool.os.kill") as mock_kill, \
|
||||
patch("gateway.status._pid_exists", return_value=True), \
|
||||
patch("time.sleep"):
|
||||
_kill_orphaned_mcp_children()
|
||||
|
||||
# Both phases should have used killpg (pgroup reach), not per-pid kill.
|
||||
mock_killpg.assert_any_call(fake_pgid, signal.SIGTERM)
|
||||
mock_killpg.assert_any_call(fake_pgid, fake_sigkill)
|
||||
assert mock_killpg.call_count == 2
|
||||
mock_kill.assert_not_called()
|
||||
|
||||
with _lock:
|
||||
assert fake_pid not in _orphan_stdio_pids
|
||||
assert fake_pid not in _stdio_pgids
|
||||
|
||||
def test_killpg_failure_falls_back_to_kill(self, monkeypatch):
|
||||
"""If killpg raises ProcessLookupError (pgroup gone), try os.kill."""
|
||||
from tools.mcp_tool import (
|
||||
_kill_orphaned_mcp_children,
|
||||
_orphan_stdio_pids,
|
||||
_stdio_pgids,
|
||||
_lock,
|
||||
)
|
||||
|
||||
self._reset_state()
|
||||
fake_pid = 636363
|
||||
fake_pgid = 636363
|
||||
with _lock:
|
||||
_orphan_stdio_pids.add(fake_pid)
|
||||
_stdio_pgids[fake_pid] = fake_pgid
|
||||
|
||||
if not hasattr(os, "killpg"):
|
||||
pytest.skip("os.killpg not available on this platform")
|
||||
|
||||
with patch(
|
||||
"tools.mcp_tool.os.killpg",
|
||||
side_effect=ProcessLookupError("no such process group"),
|
||||
) as mock_killpg, \
|
||||
patch("tools.mcp_tool.os.kill") as mock_kill, \
|
||||
patch("gateway.status._pid_exists", return_value=False), \
|
||||
patch("time.sleep"):
|
||||
_kill_orphaned_mcp_children()
|
||||
|
||||
# killpg was attempted (phase 1 SIGTERM) and fell back to os.kill.
|
||||
# Phase 3 skips because _pid_exists returns False (direct pid gone).
|
||||
mock_killpg.assert_called()
|
||||
mock_kill.assert_any_call(fake_pid, signal.SIGTERM)
|
||||
|
||||
with _lock:
|
||||
assert fake_pid not in _orphan_stdio_pids
|
||||
assert fake_pid not in _stdio_pgids
|
||||
|
||||
def test_no_pgid_uses_per_pid_kill(self, monkeypatch):
|
||||
"""When no pgid is recorded (e.g. Windows), fall back to os.kill."""
|
||||
from tools.mcp_tool import (
|
||||
_kill_orphaned_mcp_children,
|
||||
_orphan_stdio_pids,
|
||||
_stdio_pgids,
|
||||
_lock,
|
||||
)
|
||||
|
||||
self._reset_state()
|
||||
fake_pid = 747474
|
||||
with _lock:
|
||||
_orphan_stdio_pids.add(fake_pid)
|
||||
# No entry in _stdio_pgids.
|
||||
|
||||
with patch("tools.mcp_tool.os.kill") as mock_kill, \
|
||||
patch("gateway.status._pid_exists", return_value=False), \
|
||||
patch("time.sleep"):
|
||||
# killpg may or may not exist; either way the no-pgid path skips it.
|
||||
_kill_orphaned_mcp_children()
|
||||
|
||||
mock_kill.assert_any_call(fake_pid, signal.SIGTERM)
|
||||
|
||||
with _lock:
|
||||
assert fake_pid not in _orphan_stdio_pids
|
||||
|
||||
@pytest.mark.live_system_guard_bypass
|
||||
@pytest.mark.skipif(
|
||||
not hasattr(os, "killpg") or not hasattr(os, "setsid"),
|
||||
reason="POSIX-only: requires os.killpg and os.setsid",
|
||||
)
|
||||
def test_grandchild_reaped_via_pgroup(self, tmp_path):
|
||||
"""End-to-end: parent spawns grandchild, parent exits, killpg reaps grandchild.
|
||||
|
||||
Mirrors issue #23799: a stdio MCP wrapper (parent) launches a long-lived
|
||||
helper subprocess (grandchild) in the same process group, then the
|
||||
wrapper exits while the grandchild keeps running. killpg on the pgid
|
||||
captured at spawn time must still deliver the signal to the grandchild.
|
||||
|
||||
Marked ``live_system_guard_bypass`` because this test genuinely needs
|
||||
real signal delivery to its own subprocess tree (the conftest guard
|
||||
only knows the test's *initial* children; the spawned tree here is
|
||||
outside that allowlist).
|
||||
"""
|
||||
import subprocess
|
||||
import sys
|
||||
import time as _time
|
||||
|
||||
psutil = pytest.importorskip("psutil")
|
||||
|
||||
# Grandchild: sleep forever, write its pid then wait.
|
||||
grandchild_pid_file = tmp_path / "grandchild.pid"
|
||||
grandchild_script = tmp_path / "grandchild.py"
|
||||
grandchild_script.write_text(
|
||||
"import os, sys, time\n"
|
||||
f"open({str(grandchild_pid_file)!r}, 'w').write(str(os.getpid()))\n"
|
||||
"while True:\n"
|
||||
" time.sleep(0.5)\n"
|
||||
)
|
||||
|
||||
# Parent: spawn grandchild, exit immediately (without killing it).
|
||||
parent_script = tmp_path / "parent.py"
|
||||
parent_script.write_text(
|
||||
"import subprocess, sys\n"
|
||||
f"subprocess.Popen([sys.executable, {str(grandchild_script)!r}])\n"
|
||||
# Parent exits — grandchild reparents to init.
|
||||
)
|
||||
|
||||
# Spawn parent in its own session (mirrors stdio_client behaviour).
|
||||
parent = subprocess.Popen(
|
||||
[sys.executable, str(parent_script)],
|
||||
start_new_session=True,
|
||||
)
|
||||
parent_pgid = os.getpgid(parent.pid)
|
||||
# Wait for parent to exit and grandchild to spin up.
|
||||
parent.wait(timeout=5)
|
||||
deadline = _time.time() + 5
|
||||
while _time.time() < deadline and not grandchild_pid_file.exists():
|
||||
_time.sleep(0.05)
|
||||
assert grandchild_pid_file.exists(), "grandchild did not start"
|
||||
grandchild_pid = int(grandchild_pid_file.read_text().strip())
|
||||
|
||||
# Sanity: grandchild is alive and shares the parent's pgid.
|
||||
assert psutil.pid_exists(grandchild_pid)
|
||||
assert os.getpgid(grandchild_pid) == parent_pgid
|
||||
|
||||
# Drive the reaper: register the parent pid + pgid as an orphan.
|
||||
from tools.mcp_tool import (
|
||||
_kill_orphaned_mcp_children,
|
||||
_orphan_stdio_pids,
|
||||
_stdio_pgids,
|
||||
_stdio_pids,
|
||||
_lock,
|
||||
)
|
||||
with _lock:
|
||||
_stdio_pids.clear()
|
||||
_orphan_stdio_pids.clear()
|
||||
_stdio_pgids.clear()
|
||||
_orphan_stdio_pids.add(parent.pid)
|
||||
_stdio_pgids[parent.pid] = parent_pgid
|
||||
try:
|
||||
_kill_orphaned_mcp_children()
|
||||
finally:
|
||||
# Belt-and-suspenders: ensure grandchild is dead even if test fails.
|
||||
try:
|
||||
os.kill(grandchild_pid, signal.SIGKILL)
|
||||
except ProcessLookupError:
|
||||
pass
|
||||
|
||||
# Grandchild should be gone — SIGTERM via killpg in phase 1 reached it.
|
||||
deadline = _time.time() + 3
|
||||
while _time.time() < deadline and psutil.pid_exists(grandchild_pid):
|
||||
_time.sleep(0.05)
|
||||
assert not psutil.pid_exists(grandchild_pid), (
|
||||
"grandchild survived killpg-based reaping (issue #23799 regression)"
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Fix 3: MCP reload timeout (cli.py)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@@ -1220,6 +1220,90 @@ class TestParseTargetRefSlack:
|
||||
assert _parse_target_ref("telegram", "C0B0QV5434G")[2] is False
|
||||
|
||||
|
||||
class TestParseTargetRefEmail:
|
||||
"""_parse_target_ref recognizes email addresses as explicit for the email platform."""
|
||||
|
||||
def test_standard_email_is_explicit(self):
|
||||
chat_id, thread_id, is_explicit = _parse_target_ref("email", "user@example.com")
|
||||
assert chat_id == "user@example.com"
|
||||
assert thread_id is None
|
||||
assert is_explicit is True
|
||||
|
||||
def test_email_with_dots_in_local_part(self):
|
||||
chat_id, _, is_explicit = _parse_target_ref("email", "first.last@example.co.uk")
|
||||
assert chat_id == "first.last@example.co.uk"
|
||||
assert is_explicit is True
|
||||
|
||||
def test_email_with_plus_tag(self):
|
||||
chat_id, _, is_explicit = _parse_target_ref("email", "user+tag@gmail.com")
|
||||
assert chat_id == "user+tag@gmail.com"
|
||||
assert is_explicit is True
|
||||
|
||||
def test_email_strips_whitespace(self):
|
||||
chat_id, _, is_explicit = _parse_target_ref("email", " user@example.com ")
|
||||
assert chat_id == "user@example.com"
|
||||
assert is_explicit is True
|
||||
|
||||
def test_invalid_email_not_explicit(self):
|
||||
assert _parse_target_ref("email", "not-an-email")[2] is False
|
||||
assert _parse_target_ref("email", "@example.com")[2] is False
|
||||
assert _parse_target_ref("email", "user@")[2] is False
|
||||
assert _parse_target_ref("email", "user@.com")[2] is False
|
||||
|
||||
def test_email_not_explicit_for_other_platforms(self):
|
||||
assert _parse_target_ref("telegram", "user@example.com")[2] is False
|
||||
assert _parse_target_ref("discord", "user@example.com")[2] is False
|
||||
assert _parse_target_ref("slack", "user@example.com")[2] is False
|
||||
|
||||
|
||||
class TestEmailHomeChannelErrorHint:
|
||||
"""The no-home-channel error for email points at the real env var.
|
||||
|
||||
Email reads its home channel from EMAIL_HOME_ADDRESS (gateway/config.py),
|
||||
not the generic EMAIL_HOME_CHANNEL. The error guidance must name the
|
||||
variable that is actually consulted so users who follow it succeed.
|
||||
"""
|
||||
|
||||
def test_email_error_names_email_home_address(self):
|
||||
email_cfg = SimpleNamespace(enabled=True, token="", extra={})
|
||||
config = SimpleNamespace(
|
||||
platforms={Platform.EMAIL: email_cfg},
|
||||
get_home_channel=lambda _platform: None,
|
||||
)
|
||||
with patch("gateway.config.load_gateway_config", return_value=config), \
|
||||
patch("tools.interrupt.is_interrupted", return_value=False):
|
||||
result = json.loads(
|
||||
send_message_tool(
|
||||
{
|
||||
"action": "send",
|
||||
"target": "email",
|
||||
"message": "hi",
|
||||
}
|
||||
)
|
||||
)
|
||||
assert "EMAIL_HOME_ADDRESS" in result["error"]
|
||||
assert "EMAIL_HOME_CHANNEL" not in result["error"]
|
||||
|
||||
def test_non_email_platform_keeps_generic_home_channel_hint(self):
|
||||
telegram_cfg = SimpleNamespace(enabled=True, token="***", extra={})
|
||||
config = SimpleNamespace(
|
||||
platforms={Platform.TELEGRAM: telegram_cfg},
|
||||
get_home_channel=lambda _platform: None,
|
||||
)
|
||||
with patch("gateway.config.load_gateway_config", return_value=config), \
|
||||
patch("tools.interrupt.is_interrupted", return_value=False):
|
||||
result = json.loads(
|
||||
send_message_tool(
|
||||
{
|
||||
"action": "send",
|
||||
"target": "telegram",
|
||||
"message": "hi",
|
||||
}
|
||||
)
|
||||
)
|
||||
assert "TELEGRAM_HOME_CHANNEL" in result["error"]
|
||||
|
||||
|
||||
class TestSendDiscordThreadId:
|
||||
"""_send_discord uses thread_id when provided."""
|
||||
|
||||
|
||||
@@ -845,3 +845,85 @@ class TestResetBundledSkill:
|
||||
post_manifest = _read_manifest()
|
||||
assert "google-workspace" in post_manifest
|
||||
assert (skills_dir / "productivity" / "google-workspace" / "SKILL.md").exists()
|
||||
|
||||
def test_reset_restore_succeeds_on_readonly_nix_tree(self, tmp_path):
|
||||
"""#34972: --restore must succeed even when the user copy is a fully
|
||||
read-only tree (r-xr-xr-x dirs + files), as produced by copying a
|
||||
Nix-store source. The manifest is re-baselined and bundled re-copied."""
|
||||
import os
|
||||
import stat
|
||||
|
||||
bundled = self._setup_bundled(tmp_path)
|
||||
skills_dir = tmp_path / "user_skills"
|
||||
manifest_file = skills_dir / ".bundled_manifest"
|
||||
|
||||
dest = skills_dir / "productivity" / "google-workspace"
|
||||
sub = dest / "references"
|
||||
sub.mkdir(parents=True)
|
||||
(dest / "SKILL.md").write_text("# user version\n")
|
||||
(sub / "ref.md").write_text("# nested ref\n")
|
||||
manifest_file.write_text(
|
||||
"google-workspace:STALEHASH000000000000000000000000\n"
|
||||
)
|
||||
|
||||
# Read-only files AND directories — the real Nix-store case.
|
||||
ro_dir = (
|
||||
stat.S_IRUSR | stat.S_IXUSR | stat.S_IRGRP | stat.S_IXGRP
|
||||
| stat.S_IROTH | stat.S_IXOTH
|
||||
)
|
||||
os.chmod(sub / "ref.md", stat.S_IREAD)
|
||||
os.chmod(dest / "SKILL.md", stat.S_IREAD)
|
||||
os.chmod(sub, ro_dir)
|
||||
os.chmod(dest, ro_dir)
|
||||
|
||||
try:
|
||||
with self._patches(bundled, skills_dir, manifest_file):
|
||||
result = reset_bundled_skill("google-workspace", restore=True)
|
||||
|
||||
assert result["ok"] is True
|
||||
assert result["action"] == "restored"
|
||||
# Bundled version was re-copied over the (deleted) user copy.
|
||||
assert "upstream" in (dest / "SKILL.md").read_text()
|
||||
# The read-only nested user dir/file was fully removed, not left behind.
|
||||
assert not (sub / "ref.md").exists()
|
||||
# sync ran and re-copied the skill (not stuck in limbo).
|
||||
assert "google-workspace" in result["synced"]["copied"]
|
||||
finally:
|
||||
# Restore perms so tmp_path teardown can remove anything left.
|
||||
for p in (sub, dest):
|
||||
if p.exists():
|
||||
os.chmod(p, stat.S_IRWXU)
|
||||
|
||||
def test_reset_restore_preserves_manifest_on_rmtree_failure(self, tmp_path):
|
||||
"""#34972: when the user copy genuinely cannot be removed, the manifest
|
||||
entry must NOT be deleted — otherwise the skill enters a limbo state
|
||||
where future syncs silently skip it forever."""
|
||||
bundled = self._setup_bundled(tmp_path)
|
||||
skills_dir = tmp_path / "user_skills"
|
||||
manifest_file = skills_dir / ".bundled_manifest"
|
||||
|
||||
dest = skills_dir / "productivity" / "google-workspace"
|
||||
dest.mkdir(parents=True)
|
||||
(dest / "SKILL.md").write_text("# user version\n")
|
||||
manifest_file.write_text(
|
||||
"google-workspace:STALEHASH000000000000000000000000\n"
|
||||
)
|
||||
|
||||
# Simulate an unremovable tree (e.g. a busy mountpoint or a path even
|
||||
# chmod can't rescue) by making the removal helper raise.
|
||||
def _boom(_path):
|
||||
raise PermissionError(13, "Permission denied")
|
||||
|
||||
with self._patches(bundled, skills_dir, manifest_file), patch(
|
||||
"tools.skills_sync._rmtree_writable", side_effect=_boom
|
||||
):
|
||||
result = reset_bundled_skill("google-workspace", restore=True)
|
||||
|
||||
# Restore failed, and the manifest must be left untouched.
|
||||
assert result["ok"] is False
|
||||
assert result["action"] == "not_reset"
|
||||
assert "Manifest entry preserved" in result["message"]
|
||||
manifest_after = manifest_file.read_text()
|
||||
assert "google-workspace" in manifest_after
|
||||
# User copy is still on disk (we changed nothing).
|
||||
assert (dest / "SKILL.md").exists()
|
||||
|
||||
@@ -917,3 +917,84 @@ class TestIsImageSizeError:
|
||||
|
||||
def test_empty_message(self):
|
||||
assert not _is_image_size_error(Exception(""))
|
||||
|
||||
|
||||
class TestDownloadRetryClassification:
|
||||
"""Error-class-aware retry: 4xx fail-fast, 429/5xx/transient retried (issue #32296)."""
|
||||
|
||||
@staticmethod
|
||||
def _status_error(status_code):
|
||||
import httpx
|
||||
|
||||
request = httpx.Request("GET", "https://example.com/img.jpg")
|
||||
response = httpx.Response(status_code, request=request)
|
||||
return httpx.HTTPStatusError(
|
||||
f"{status_code}", request=request, response=response
|
||||
)
|
||||
|
||||
def _make_client_raising_status(self, status_code):
|
||||
"""AsyncClient whose response.raise_for_status() raises HTTPStatusError."""
|
||||
mock_response = MagicMock()
|
||||
mock_response.raise_for_status = MagicMock(
|
||||
side_effect=self._status_error(status_code)
|
||||
)
|
||||
mock_client = AsyncMock()
|
||||
mock_client.__aenter__ = AsyncMock(return_value=mock_client)
|
||||
mock_client.__aexit__ = AsyncMock(return_value=False)
|
||||
mock_client.get = AsyncMock(return_value=mock_response)
|
||||
return mock_client
|
||||
|
||||
def test_is_retryable_classification(self):
|
||||
from tools.vision_tools import _is_retryable_download_error
|
||||
|
||||
# Non-retryable client errors
|
||||
for code in (400, 403, 404, 410):
|
||||
assert _is_retryable_download_error(self._status_error(code)) is False
|
||||
# Retryable: rate limit + server errors
|
||||
for code in (429, 500, 502, 503):
|
||||
assert _is_retryable_download_error(self._status_error(code)) is True
|
||||
# Policy/SSRF/size errors are terminal
|
||||
assert _is_retryable_download_error(PermissionError("blocked")) is False
|
||||
assert _is_retryable_download_error(ValueError("too large")) is False
|
||||
# Unclassified (network blip) is retryable
|
||||
assert _is_retryable_download_error(ConnectionError("reset")) is True
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_404_fails_fast_without_retry(self, tmp_path):
|
||||
"""A 404 must raise on the first attempt — no backoff sleep, no extra GETs."""
|
||||
import httpx
|
||||
from tools.vision_tools import _download_image
|
||||
|
||||
mock_client = self._make_client_raising_status(404)
|
||||
with (
|
||||
patch("tools.vision_tools.httpx.AsyncClient", return_value=mock_client),
|
||||
patch("tools.vision_tools.check_website_access", return_value=None),
|
||||
patch("asyncio.sleep", new_callable=AsyncMock) as mock_sleep,
|
||||
pytest.raises(httpx.HTTPStatusError),
|
||||
):
|
||||
await _download_image(
|
||||
"https://example.com/missing.jpg", tmp_path / "x.jpg", max_retries=3
|
||||
)
|
||||
# Exactly one attempt, zero backoff sleeps.
|
||||
assert mock_client.get.await_count == 1
|
||||
mock_sleep.assert_not_called()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_503_retries_then_raises(self, tmp_path):
|
||||
"""A 5xx is retried up to max_retries, sleeping between attempts."""
|
||||
import httpx
|
||||
from tools.vision_tools import _download_image
|
||||
|
||||
mock_client = self._make_client_raising_status(503)
|
||||
with (
|
||||
patch("tools.vision_tools.httpx.AsyncClient", return_value=mock_client),
|
||||
patch("tools.vision_tools.check_website_access", return_value=None),
|
||||
patch("asyncio.sleep", new_callable=AsyncMock) as mock_sleep,
|
||||
pytest.raises(httpx.HTTPStatusError),
|
||||
):
|
||||
await _download_image(
|
||||
"https://example.com/flaky.jpg", tmp_path / "y.jpg", max_retries=3
|
||||
)
|
||||
# All three attempts used, two backoff sleeps between them.
|
||||
assert mock_client.get.await_count == 3
|
||||
assert mock_sleep.await_count == 2
|
||||
|
||||
@@ -623,10 +623,49 @@ class TestCheckWebApiKey:
|
||||
assert check_web_api_key() is True
|
||||
|
||||
def test_tool_gateway_returns_true(self):
|
||||
with patch("tools.web_tools._read_nous_access_token", return_value="nous-token"):
|
||||
with patch("tools.web_tools._peek_nous_access_token", return_value="nous-token"):
|
||||
from tools.web_tools import check_web_api_key
|
||||
assert check_web_api_key() is True
|
||||
|
||||
def test_tool_gateway_availability_skips_refresh_for_expired_cached_token(
|
||||
self,
|
||||
tmp_path,
|
||||
monkeypatch,
|
||||
):
|
||||
monkeypatch.delenv("TOOL_GATEWAY_USER_TOKEN", raising=False)
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
expired_at = "2000-01-01T00:00:00+00:00"
|
||||
(tmp_path / "auth.json").write_text(json.dumps({
|
||||
"providers": {
|
||||
"nous": {
|
||||
"access_token": "expired-token",
|
||||
"refresh_token": "refresh-token",
|
||||
"expires_at": expired_at,
|
||||
}
|
||||
}
|
||||
}))
|
||||
refresh_calls = []
|
||||
|
||||
def _record_refresh(*, refresh_skew_seconds=120, **_kwargs):
|
||||
refresh_calls.append(refresh_skew_seconds)
|
||||
return "fresh-token"
|
||||
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.auth.resolve_nous_access_token",
|
||||
_record_refresh,
|
||||
)
|
||||
|
||||
with patch.dict(
|
||||
os.environ,
|
||||
{"FIRECRAWL_GATEWAY_URL": "http://127.0.0.1:3002"},
|
||||
clear=False,
|
||||
):
|
||||
from tools.web_tools import check_web_api_key
|
||||
|
||||
assert check_web_api_key() is True
|
||||
|
||||
assert refresh_calls == []
|
||||
|
||||
def test_configured_backend_must_match_available_provider(self):
|
||||
with patch("tools.web_tools._load_web_config", return_value={"backend": "parallel"}):
|
||||
with patch("tools.web_tools._read_nous_access_token", return_value="nous-token"):
|
||||
@@ -636,7 +675,7 @@ class TestCheckWebApiKey:
|
||||
|
||||
def test_configured_firecrawl_backend_accepts_managed_gateway(self):
|
||||
with patch("tools.web_tools._load_web_config", return_value={"backend": "firecrawl"}):
|
||||
with patch("tools.web_tools._read_nous_access_token", return_value="nous-token"):
|
||||
with patch("tools.web_tools._peek_nous_access_token", return_value="nous-token"):
|
||||
with patch.dict(os.environ, {"FIRECRAWL_GATEWAY_URL": "http://127.0.0.1:3002"}, clear=False):
|
||||
from tools.web_tools import check_web_api_key
|
||||
assert check_web_api_key() is True
|
||||
|
||||
Reference in New Issue
Block a user