diff --git a/tests/tools/test_browser_content_none_guard.py b/tests/tools/test_browser_content_none_guard.py new file mode 100644 index 0000000000..6952bb938c --- /dev/null +++ b/tests/tools/test_browser_content_none_guard.py @@ -0,0 +1,109 @@ +"""Tests for None guard on browser_tool LLM response content. + +browser_tool.py has two call sites that access response.choices[0].message.content +without checking for None — _extract_relevant_content (line 996) and +browser_vision (line 1626). When reasoning-only models (DeepSeek-R1, QwQ) +return content=None, these produce null snapshots or null analysis. + +These tests verify both sites are guarded. +""" + +import types +from unittest.mock import MagicMock, patch + +import pytest + + +# ── helpers ──────────────────────────────────────────────────────────────── + +def _make_response(content): + """Build a minimal OpenAI-compatible ChatCompletion response stub.""" + message = types.SimpleNamespace(content=content) + choice = types.SimpleNamespace(message=message) + return types.SimpleNamespace(choices=[choice]) + + +# ── _extract_relevant_content (line 996) ────────────────────────────────── + +class TestExtractRelevantContentNoneGuard: + """tools/browser_tool.py — _extract_relevant_content()""" + + def test_none_content_falls_back_to_truncated(self): + """When LLM returns None content, should fall back to truncated snapshot.""" + with patch("tools.browser_tool.call_llm", return_value=_make_response(None)), \ + patch("tools.browser_tool._get_extraction_model", return_value="test-model"): + from tools.browser_tool import _extract_relevant_content + result = _extract_relevant_content("This is a long snapshot text", "find the button") + + assert result is not None + assert isinstance(result, str) + assert len(result) > 0 + + def test_normal_content_returned(self): + """Normal string content should pass through.""" + with patch("tools.browser_tool.call_llm", return_value=_make_response("Extracted content here")), \ + patch("tools.browser_tool._get_extraction_model", return_value="test-model"): + from tools.browser_tool import _extract_relevant_content + result = _extract_relevant_content("snapshot text", "task") + + assert result == "Extracted content here" + + def test_empty_string_content_falls_back(self): + """Empty string content should also fall back to truncated.""" + with patch("tools.browser_tool.call_llm", return_value=_make_response(" ")), \ + patch("tools.browser_tool._get_extraction_model", return_value="test-model"): + from tools.browser_tool import _extract_relevant_content + result = _extract_relevant_content("This is a long snapshot text", "task") + + assert result is not None + assert len(result) > 0 + + +# ── browser_vision (line 1626) ──────────────────────────────────────────── + +class TestBrowserVisionNoneGuard: + """tools/browser_tool.py — browser_vision() analysis extraction""" + + def test_none_content_produces_fallback_message(self): + """When LLM returns None content, analysis should have a fallback message.""" + response = _make_response(None) + analysis = (response.choices[0].message.content or "").strip() + fallback = analysis or "Vision analysis returned no content." + + assert fallback == "Vision analysis returned no content." + + def test_normal_content_passes_through(self): + """Normal analysis content should pass through unchanged.""" + response = _make_response(" The page shows a login form. ") + analysis = (response.choices[0].message.content or "").strip() + fallback = analysis or "Vision analysis returned no content." + + assert fallback == "The page shows a login form." + + +# ── source line verification ────────────────────────────────────────────── + +class TestBrowserSourceLinesAreGuarded: + """Verify the actual source file has the fix applied.""" + + @staticmethod + def _read_file() -> str: + import os + base = os.path.dirname(os.path.dirname(os.path.dirname(__file__))) + with open(os.path.join(base, "tools", "browser_tool.py")) as f: + return f.read() + + def test_extract_relevant_content_guarded(self): + src = self._read_file() + # The old unguarded pattern should NOT exist + assert "return response.choices[0].message.content\n" not in src, ( + "browser_tool.py _extract_relevant_content still has unguarded " + ".content return — apply None guard" + ) + + def test_browser_vision_guarded(self): + src = self._read_file() + assert "analysis = response.choices[0].message.content\n" not in src, ( + "browser_tool.py browser_vision still has unguarded " + ".content assignment — apply None guard" + ) diff --git a/tools/browser_tool.py b/tools/browser_tool.py index f71ba78d4d..ffb772c1d6 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -993,7 +993,7 @@ def _extract_relevant_content( if model: call_kwargs["model"] = model response = call_llm(**call_kwargs) - return response.choices[0].message.content + return (response.choices[0].message.content or "").strip() or _truncate_snapshot(snapshot_text) except Exception: return _truncate_snapshot(snapshot_text) @@ -1623,10 +1623,10 @@ def browser_vision(question: str, annotate: bool = False, task_id: Optional[str] call_kwargs["model"] = vision_model response = call_llm(**call_kwargs) - analysis = response.choices[0].message.content + analysis = (response.choices[0].message.content or "").strip() response_data = { "success": True, - "analysis": analysis, + "analysis": analysis or "Vision analysis returned no content.", "screenshot_path": str(screenshot_path), } # Include annotation data if annotated screenshot was taken