diff --git a/backend/apps/agents/browser/browser_agent.py b/backend/apps/agents/browser/browser_agent.py index a769dbd6..20df1926 100644 --- a/backend/apps/agents/browser/browser_agent.py +++ b/backend/apps/agents/browser/browser_agent.py @@ -1029,9 +1029,15 @@ async def run_browser_agent( # result) for the third time in a row? If so, attach a loud # warning to this tool_result so the model is forced to # acknowledge it on its next turn. - call_key = _hash_tool_call(tu.name, tu.input, result) - is_loop = _detect_loop(recent_tool_calls, call_key) - if call_key[0] not in _LOOP_DETECTION_EXCLUDED_TOOLS: + # Loop detection only covers the non-excluded tools, so skip the + # hash entirely for the excluded ones; otherwise a screenshot/read + # serializes its full ~1MB result here just for _detect_loop to + # discard it (it short-circuits excluded tools to False anyway). + if tu.name in _LOOP_DETECTION_EXCLUDED_TOOLS: + is_loop = False + else: + call_key = _hash_tool_call(tu.name, tu.input, result) + is_loop = _detect_loop(recent_tool_calls, call_key) recent_tool_calls.append(call_key) if len(recent_tool_calls) > _LOOP_WINDOW_SIZE * 2: recent_tool_calls = recent_tool_calls[-_LOOP_WINDOW_SIZE * 2:] diff --git a/backend/apps/agents/browser/browser_metrics.py b/backend/apps/agents/browser/browser_metrics.py index 5c0ab342..836dc03d 100644 --- a/backend/apps/agents/browser/browser_metrics.py +++ b/backend/apps/agents/browser/browser_metrics.py @@ -54,7 +54,14 @@ def tier_for(tool_name: str) -> str: return _TIER.get(tool_name, "other") +_metrics_dir_cache: str | None = None + + def _metrics_dir() -> str: + # Resolved + mkdir'd once, not on every tool call (this runs in the hot path). + global _metrics_dir_cache + if _metrics_dir_cache is not None: + return _metrics_dir_cache override = os.environ.get("OPENSWARM_BROWSER_METRICS_DIR") if override: base = override @@ -69,6 +76,7 @@ def _metrics_dir() -> str: os.makedirs(base, exist_ok=True) except Exception: pass + _metrics_dir_cache = base return base diff --git a/backend/tests/test_browser_hotpath_waste.py b/backend/tests/test_browser_hotpath_waste.py new file mode 100644 index 00000000..3e2e28fd --- /dev/null +++ b/backend/tests/test_browser_hotpath_waste.py @@ -0,0 +1,51 @@ +"""Hot-path waste removals in the browser sub-agent loop. + +Two per-action costs that were pure waste: + 1. browser_metrics._metrics_dir() ran os.makedirs() on EVERY tool call. + 2. The loop-detection hash serialized a tool's full result (a ~1MB screenshot + or 15KB read) even for tools that are excluded from loop detection, where + _detect_loop ignores the hash entirely. These pin both fixes. +""" + +import os + +import backend.apps.agents.browser.browser_metrics as M +from backend.apps.agents.browser.browser_loop import ( + _detect_loop, + _LOOP_DETECTION_EXCLUDED_TOOLS, +) + + +def test_metrics_dir_is_cached_makedirs_runs_once(monkeypatch): + M._metrics_dir_cache = None + calls = {"n": 0} + real = os.makedirs + + def counting(*a, **k): + calls["n"] += 1 + return real(*a, **k) + + monkeypatch.setattr(os, "makedirs", counting) + d1 = M._metrics_dir() + d2 = M._metrics_dir() + d3 = M._metrics_dir() + assert d1 == d2 == d3 + assert calls["n"] == 1, f"makedirs must run once, ran {calls['n']}x" + + +def test_excluded_tools_never_register_a_loop(): + # The invariant the hash-skip relies on: for every excluded tool, even ten + # identical calls in a row are NOT a loop, so computing/storing the hash for + # them was dead work. Setting is_loop=False directly is therefore equivalent. + for tool in _LOOP_DETECTION_EXCLUDED_TOOLS: + key = (tool, "in", "out") + assert _detect_loop([key] * 10, key) is False, f"{tool} wrongly looped" + + +def test_non_excluded_tool_still_loops_after_threshold(): + # Guard the other side: the fix must NOT disable loop detection for the tools + # that need it (clicks/types/etc.). + key = ("BrowserClick", '{"selector":"#x"}', "clicked") + # below threshold -> not a loop; at/over threshold within the window -> loop + assert _detect_loop([key], key) is False + assert _detect_loop([key] * 5, key) is True