diff --git a/agents/raven-research/plugins/research-flow/research_flow/tools/web.py b/agents/raven-research/plugins/research-flow/research_flow/tools/web.py index 57e1b56ae..b82ecc4a1 100644 --- a/agents/raven-research/plugins/research-flow/research_flow/tools/web.py +++ b/agents/raven-research/plugins/research-flow/research_flow/tools/web.py @@ -1823,7 +1823,9 @@ async def _fetch( # seconds on a slow resolver, and one process serves every session's turn. is_valid, error_msg = await asyncio.to_thread(_judge_fetch_target, url) if not is_valid: - return json.dumps({"error": f"URL validation failed: {error_msg}", "url": url}, ensure_ascii=False) + # The same rule as the handlers below: most of these reasons name the + # hostname, and a reader whose every target is refused is one cause. + return json.dumps({"error": "URL validation failed", "detail": error_msg, "url": url}, ensure_ascii=False) # "no explicit proxy" is not "direct": with trust_env on, httpx # still honours HTTP(S)_PROXY from the environment. diff --git a/raven/agent/tools/media_gen.py b/raven/agent/tools/media_gen.py index 23ebc0e1c..64e450539 100644 --- a/raven/agent/tools/media_gen.py +++ b/raven/agent/tools/media_gen.py @@ -240,15 +240,21 @@ async def _chat(self, payload: dict[str, Any]) -> dict[str, Any]: return message def _format_http_error(self, e: httpx.HTTPStatusError) -> str: + # The status, not the body: ``failure_class`` keys the loop's tool-failure + # streak on this envelope's ``error`` alone, and a vendor body carries a + # request id, so interpolating it splits one dead endpoint into a class per + # call and the stop-repeating nudge never fires. The body moves to ``detail``, + # which keeps it in front of the model and inside ``is_hard_tool_failure``'s + # transient-marker scan. The same rule governs every envelope below. body = e.response.text[:400] - hint = "" + answer: dict[str, Any] = {"error": f"HTTP {e.response.status_code}", "detail": body} if e.response.status_code == 403: - hint = ( - " | Request denied (HTTP 403). You can route media calls through a " + answer["hint"] = ( + "Request denied (HTTP 403). You can route media calls through a " "proxy via tools.media.proxy (or HTTPS_PROXY) and retry." ) logger.error("{} HTTP {}: {}", self.name, e.response.status_code, body) - return json.dumps({"error": f"HTTP {e.response.status_code}: {body}{hint}"}, ensure_ascii=False) + return json.dumps(answer, ensure_ascii=False) _IMAGE_API_MODELS = ( @@ -558,7 +564,7 @@ async def execute( ) blank = next((i for i, spec in enumerate(wanted) if not str(spec.get("prompt") or "").strip()), None) if blank is not None: - return json.dumps({"error": f"the picture at index {blank} has no prompt"}, ensure_ascii=False) + return json.dumps({"error": "a picture has no prompt", "index": blank}, ensure_ascii=False) if len(wanted) == 1: return await self._generate_one(wanted[0], _safe_stem(wanted[0].get("filename")), model, output_dir) stems = _distinct_stems(wanted) @@ -598,7 +604,12 @@ def _batch_answer(self, wanted: list[dict[str, Any]], answers: list[str | BaseEx paths.extend(reply["paths"]) else: item["error"] = reply.get("error") or "no image returned" - for key in ("note", "finish_reason", "retryable"): + # Every key ``error`` was split into travels with it: the reason a + # picture failed, the proxy a 403 can be routed through, the + # reference that was refused. ``error`` carries only the class now, + # so a key left behind here is a batched picture told strictly less + # than the single call is. + for key in ("detail", "hint", "url", "note", "finish_reason", "retryable"): if reply.get(key) is not None: item[key] = reply[key] if reply.get("model"): @@ -640,7 +651,7 @@ async def _generate_one(self, spec: dict[str, Any], stem: str, model: str | None for ref in images[:6]: content.append(self._image_part(ref)) except OSError as e: - return json.dumps({"error": f"could not read input image: {e}"}, ensure_ascii=False) + return json.dumps({"error": "could not read input image", "detail": str(e)}, ensure_ascii=False) else: content = prompt @@ -664,7 +675,7 @@ async def _generate_one(self, spec: dict[str, Any], stem: str, model: str | None return self._format_http_error(e) except Exception as e: logger.error("image_generate error: {}", e) - return json.dumps({"error": str(e)}, ensure_ascii=False) + return json.dumps({"error": type(e).__name__, "detail": str(e)}, ensure_ascii=False) out_images = msg.get("images") or [] if not out_images: @@ -720,7 +731,7 @@ async def _via_images_api( # edits form takes the bytes behind it. refs = [self._image_part(ref) for ref in (images or [])[:16]] except OSError as e: - return json.dumps({"error": f"could not read input image: {e}"}, ensure_ascii=False) + return json.dumps({"error": "could not read input image", "detail": str(e)}, ensure_ascii=False) try: async with httpx.AsyncClient(proxy=self._proxy, timeout=300.0) as client: if openrouter: @@ -745,7 +756,7 @@ async def _via_images_api( fetched = await guarded_fetch(client, url, what="image reference") if fetched is None: return json.dumps( - {"error": f"input image {url} refused: not a fetchable public target"}, + {"error": "input image refused: not a fetchable public target", "url": url}, ensure_ascii=False, ) fetched.raise_for_status() @@ -772,7 +783,7 @@ async def _via_images_api( return self._format_http_error(e) except Exception as e: logger.error("image_generate error: {}", e) - return json.dumps({"error": str(e)}, ensure_ascii=False) + return json.dumps({"error": type(e).__name__, "detail": str(e)}, ensure_ascii=False) paths: list[str] = [] for item in items: encoded = item.get("b64_json") if isinstance(item, dict) else None @@ -866,7 +877,7 @@ async def execute( return self._format_http_error(e) except Exception as e: logger.error("text_to_speech error: {}", e) - return json.dumps({"error": str(e)}, ensure_ascii=False) + return json.dumps({"error": type(e).__name__, "detail": str(e)}, ensure_ascii=False) if not pcm: return json.dumps( @@ -1107,7 +1118,7 @@ async def execute( return self._format_http_error(e) except Exception as e: logger.error("video_generate error: {}", e) - return json.dumps({"error": str(e)}, ensure_ascii=False) + return json.dumps({"error": type(e).__name__, "detail": str(e)}, ensure_ascii=False) path = self._output_path("mp4") path.write_bytes(data) diff --git a/raven/agent/tools/web.py b/raven/agent/tools/web.py index b14dd0c92..7deb412bb 100644 --- a/raven/agent/tools/web.py +++ b/raven/agent/tools/web.py @@ -796,7 +796,9 @@ async def execute(self, url: str, extractMode: str = "markdown", maxChars: int | max_chars = maxChars or self.max_chars is_valid, error_msg = validate_url_target(url) if not is_valid: - return json.dumps({"error": f"URL validation failed: {error_msg}", "url": url}, ensure_ascii=False) + # The same rule as the handlers below: most of these reasons name the + # hostname, and a reader whose every target is refused is one cause. + return json.dumps({"error": "URL validation failed", "detail": error_msg, "url": url}, ensure_ascii=False) try: logger.debug("WebFetch[{}]: {}", self.provider, "proxy enabled" if self.proxy else "direct connection") diff --git a/tests/test_agent_loop_tool_loop_break.py b/tests/test_agent_loop_tool_loop_break.py index 4a233e195..bcff0dfcb 100644 --- a/tests/test_agent_loop_tool_loop_break.py +++ b/tests/test_agent_loop_tool_loop_break.py @@ -55,7 +55,7 @@ def workspace(): # by the textual tests: the payload starts with "{" and spells the key # '"error":' rather than "error:". ('{"error": "Jina API key not configured", "url": "https://a.example"}', True), - ('{"error": "URL validation failed: blocked host", "url": "https://a.example"}', True), + ('{"error": "URL validation failed", "detail": "blocked host", "url": "https://a.example"}', True), ('{"url": "https://a.example", "text": "the page", "length": 9}', False), # Transient markers are tested before the envelope, so a retryable failure # stays retryable however it is wrapped. @@ -76,7 +76,7 @@ def test_different_envelope_failures_are_different_classes(): every envelope classified alike, a blocked URL followed by a reader's HTTP refusal would fire the stop-repeating nudge at a model that had changed both its cause and its approach -- the case this whole function exists to keep apart.""" - blocked = '{"error": "URL validation failed: blocked host", "url": "https://a.example"}' + blocked = '{"error": "URL validation failed", "detail": "blocked host", "url": "https://a.example"}' refused = '{"error": "Jina answered HTTP 403", "url": "https://b.example"}' keyless = '{"error": "Jina API key not configured. Set it in the config", "url": "https://c.example"}' diff --git a/tests/test_agent_tools_web_providers.py b/tests/test_agent_tools_web_providers.py index 7ea0596fb..1d55cc5af 100644 --- a/tests/test_agent_tools_web_providers.py +++ b/tests/test_agent_tools_web_providers.py @@ -466,6 +466,26 @@ async def test_two_vendor_refusals_stay_two_streak_classes(monkeypatch: pytest.M assert failure_class(spent) != failure_class(blocked) +async def test_one_blocked_reason_on_two_hosts_is_one_streak_class() -> None: + """The gate in front of the reader, which the two tests above never reach. + + Every reason this gate composes names the address it refused, so the reason + in the classification key gave a model walking an internal range one class + per address: the streak never reached the break threshold and the nudge that + would have sent it somewhere else never fired. The gate runs before any + transport, so nothing here needs a reader. + """ + envelopes = [ + await WebFetchTool(api_key="k", provider="jina").execute(u) + for u in ("http://10.0.0.1/p", "http://192.168.1.1/p") + ] + + assert all(is_hard_tool_failure(raw) for raw in envelopes) + assert len({failure_class(raw) for raw in envelopes}) == 1 + # The address the model has to stop reaching for is still in front of it. + assert "10.0.0.1" in json.loads(envelopes[0])["detail"] + + def test_every_reader_is_covered_by_the_fetch_table() -> None: assert set(_FETCH_CASES) == set(FETCH_PROVIDERS) diff --git a/tests/test_agents_research_tools.py b/tests/test_agents_research_tools.py index d384db60b..4dd5c7c8d 100644 --- a/tests/test_agents_research_tools.py +++ b/tests/test_agents_research_tools.py @@ -473,6 +473,23 @@ async def test_a_private_address_is_still_refused(monkeypatch): assert "URL validation failed" in answer +@pytest.mark.asyncio +async def test_one_blocked_reason_on_two_hosts_is_one_streak_class(monkeypatch): + """This copy of the reader carries the same gate, and drifting from the host's + is what let the envelope fix land on one of them last time. Every reason the + gate composes names the address, so the reason in the classification key gave + a model walking an internal range one class per address and the stop-repeating + nudge never came.""" + _patch_client(monkeypatch, _PageTransport()) + set_current_session("t") + + envelopes = [await WebFetchTool().execute(url=u) for u in ("http://10.0.0.1/p", "http://192.168.1.1/p")] + + assert all(is_hard_tool_failure(raw) for raw in envelopes) + assert len({failure_class(raw) for raw in envelopes}) == 1 + assert "10.0.0.1" in json.loads(envelopes[0])["detail"] + + @pytest.mark.asyncio async def test_a_non_http_scheme_is_still_refused(monkeypatch): _patch_client(monkeypatch, _PageTransport()) diff --git a/tests/test_media_gen_tool.py b/tests/test_media_gen_tool.py index 366236828..02ecb89f4 100644 --- a/tests/test_media_gen_tool.py +++ b/tests/test_media_gen_tool.py @@ -21,6 +21,7 @@ import httpx import pytest +from raven.agent.loop.failure_streak import failure_class, is_hard_tool_failure from raven.agent.tools import media_gen from raven.agent.tools.media_gen import ImageGenerateTool, SpeechGenerateTool, VideoGenerateTool @@ -261,6 +262,12 @@ def _chat_reply(*, count: int = 1) -> dict: return {"choices": [{"message": {"images": [{"image_url": {"url": "data:image/png;base64," + _B64}}] * count}}]} +async def _refuse_every_fetch(_client, _url, *, what="") -> None: + """Stand in for ``guarded_fetch`` judging a target private: it answers None, + which is the branch that names the refused address.""" + return None + + def _prompt_of(request: httpx.Request) -> str: content = json.loads(request.content)["messages"][0]["content"] return content if isinstance(content, str) else content[0]["text"] @@ -341,8 +348,10 @@ def handler(request: httpx.Request) -> httpx.Response: async def test_a_picture_that_raises_is_that_picture_s_answer(monkeypatch, tmp_path) -> None: - """A reference outside a confined workspace raises rather than returning; the - batch gathers the exception as one result instead of losing every sibling.""" + """A reference outside a confined workspace raises; that picture wears the + refusal as its own result instead of taking every sibling down with it. The + reason rides in ``detail``, because ``error`` is the streak's classification + key and a path interpolated into it would split one bad reference per file.""" outside = tmp_path / "out.png" outside.write_bytes(_PNG) workspace = tmp_path / "ws" @@ -358,7 +367,8 @@ async def test_a_picture_that_raises_is_that_picture_s_answer(monkeypatch, tmp_p out = json.loads(await tool.execute(prompts=[{"prompt": "cover"}, {"prompt": "logo", "images": [str(outside)]}])) assert out["success"] and out["failed"] == 1 and len(out["paths"]) == 1 - assert "outside the workspace" in out["results"][1]["error"] + assert out["results"][1]["error"] == "could not read input image" + assert "outside the workspace" in out["results"][1]["detail"] async def test_the_pictures_of_a_batch_are_in_flight_at_once(monkeypatch, tmp_path) -> None: @@ -529,7 +539,8 @@ async def test_a_call_with_no_prompt_at_all_says_what_to_send(monkeypatch, tmp_p ) assert "nothing to generate" in json.loads(await tool.execute())["error"] - assert "index 1" in json.loads(await tool.execute(prompts=[{"prompt": "a"}, {"prompt": " "}]))["error"] + blank = json.loads(await tool.execute(prompts=[{"prompt": "a"}, {"prompt": " "}])) + assert blank["error"] == "a picture has no prompt" and blank["index"] == 1 # ── the speech path: what the stream carries, and what the file becomes ── @@ -933,3 +944,225 @@ def test_images_endpoint_accepts_openrouter_chat_style_usage(prompt, completion) assert usage["cost_usd"] == 0.04 assert usage["cache_read_tokens"] is None assert usage["cache_write_tokens"] is None + + +# ── what the loop reads off these envelopes ── + + +def _class_of(answer: str) -> str: + return failure_class(answer) + + +async def test_one_dead_endpoint_is_one_streak_class(monkeypatch, tmp_path) -> None: + """The loop breaks a tool-failure loop at two, keyed on ``(tool, failure_class)``, + and an envelope is classified by its ``error`` string alone. A vendor body carries + a request id, so a body interpolated into that key gave every call its own class + and the stop-repeating nudge was unreachable for this whole tool.""" + bodies = iter( + [ + '{"error":{"message":"upstream unavailable","request_id":"req_aaa111"}}', + '{"error":{"message":"upstream unavailable","request_id":"req_bbb222"}}', + ] + ) + tool = _image_tool( + monkeypatch, + lambda _r: httpx.Response(500, text=next(bodies)), + model="google/gemini-2.5-flash-image", + workspace=tmp_path / "ws", + ) + + first = await tool.execute("a circle") + second = await tool.execute("a square") + + assert _class_of(first) == _class_of(second) + # Not vacuous: both have to COUNT as failures for the streak to reach two at all. + assert is_hard_tool_failure(first) and is_hard_tool_failure(second) + # And the body the model needs is still in front of it, one key over. + assert "req_aaa111" in json.loads(first)["detail"] + + +async def test_two_different_statuses_stay_two_classes(monkeypatch, tmp_path) -> None: + """The control on the test above, and the failure mode it must not cause: a model + that hit a missing model and then a server fault has changed what it is doing, and + folding those together would fire the nudge at a model still adapting.""" + statuses = iter([404, 500]) + tool = _image_tool( + monkeypatch, + lambda _r: httpx.Response(next(statuses), text='{"error":"no"}'), + model="google/gemini-2.5-flash-image", + workspace=tmp_path / "ws", + ) + + assert _class_of(await tool.execute("a circle")) != _class_of(await tool.execute("a square")) + + +async def test_a_reference_it_cannot_read_is_one_class_across_files(monkeypatch, tmp_path) -> None: + """The same rule where the variable part is a path rather than a request id.""" + tool = _image_tool( + monkeypatch, + lambda _r: httpx.Response(200, json=_chat_reply()), + model="google/gemini-2.5-flash-image", + workspace=tmp_path / "ws", + ) + + first = await tool.execute("a circle", images=[str(tmp_path / "gone-a.png")]) + second = await tool.execute("a square", images=[str(tmp_path / "gone-b.png")]) + + assert _class_of(first) == _class_of(second) + assert "gone-a.png" in json.loads(first)["detail"] + + +async def test_two_transports_dying_the_same_way_are_one_class(tmp_path) -> None: + """The catch-all, where the exception's own text spells the host it failed to + reach. Two hosts behind one dead network are one cause; a different exception + type is not, which is the other half the type keeps apart.""" + + async def dies(exc: Exception, prompt: str) -> str: + # Each transport gets its own patch window: patching over a live patch + # would leave every tool here sharing the first one's transport. + with pytest.MonkeyPatch.context() as mp: + + def handler(_r: httpx.Request) -> httpx.Response: + raise exc + + tool = _image_tool(mp, handler, model="google/gemini-2.5-flash-image", workspace=tmp_path / "ws") + return await tool.execute(prompt) + + first = await dies(httpx.ConnectError("[Errno 61] Connection refused to a.example"), "a circle") + second = await dies(httpx.ConnectError("[Errno 61] Connection refused to b.example"), "a square") + other = await dies(httpx.ReadError("the reader gave up"), "a star") + + assert _class_of(first) == _class_of(second) + assert _class_of(first) != _class_of(other) + assert "a.example" in json.loads(first)["detail"] + + +async def test_a_denied_call_still_names_the_proxy_it_needs(monkeypatch, tmp_path) -> None: + """Splitting the envelope must not cost the operator the one thing a 403 is + worth saying: the media calls can be routed through a configured proxy.""" + tool = _image_tool( + monkeypatch, + lambda _r: httpx.Response(403, text="denied"), + model="google/gemini-2.5-flash-image", + workspace=tmp_path / "ws", + ) + + answer = json.loads(await tool.execute("a circle")) + + assert answer["error"] == "HTTP 403" + assert "tools.media.proxy" in answer["hint"] + + +def _raising_client(mp: pytest.MonkeyPatch, exc: Exception) -> None: + """Every client this module builds dies the same way, whatever the route.""" + + def _boom(*_a, **_kw): + raise exc + + mp.setattr(media_gen.httpx, "AsyncClient", _boom) + + +async def test_the_images_route_classes_its_failures_like_the_chat_route(tmp_path) -> None: + """A dedicated image model never touches chat/completions, so its own copies of + both handlers need the same rule -- one that drifted would put the streak back + where it was for every ``gpt-image`` call.""" + with pytest.MonkeyPatch.context() as mp: + tool = _image_tool( + mp, + lambda _r: httpx.Response(200, json={"data": []}), + model="openai/gpt-image-2", + workspace=tmp_path / "ws", + ) + unreadable = [await tool.execute("a poster", images=[str(tmp_path / f"gone-{n}.png")]) for n in ("a", "b")] + + assert len({failure_class(raw) for raw in unreadable}) == 1 + assert "gone-a.png" in json.loads(unreadable[0])["detail"] + + dead = [] + for host in ("alpha.example", "beta.example"): + with pytest.MonkeyPatch.context() as mp: + tool = _image_tool( + mp, lambda _r: httpx.Response(200), model="openai/gpt-image-2", workspace=tmp_path / "ws" + ) + _raising_client(mp, httpx.ConnectError(f"cannot reach {host}")) + dead.append(await tool.execute("a poster")) + + assert len({failure_class(raw) for raw in dead}) == 1 + assert "alpha.example" in json.loads(dead[0])["detail"] + + +@pytest.mark.parametrize( + "build,call", + [ + (lambda: SpeechGenerateTool(SimpleNamespace(api_base="https://api.test", model="", api_key="k")), "read me"), + (lambda: VideoGenerateTool(SimpleNamespace(api_base="https://api.test", model="", api_key="k")), "a river"), + ], + ids=["speech", "video"], +) +async def test_every_media_tool_reads_one_dead_network_as_one_class(build, call) -> None: + """The streak key is ``(tool, failure_class)``, so each tool accumulates its own. + Speech and video each carry their own catch-all, and an exception's text names the + host it could not reach: left there, neither tool could ever reach the nudge.""" + answers = [] + for host in ("alpha.example", "beta.example"): + with pytest.MonkeyPatch.context() as mp: + _raising_client(mp, httpx.ConnectError(f"[Errno 61] cannot reach {host}")) + answers.append(await build().execute(call)) + + assert all(is_hard_tool_failure(raw) for raw in answers) + assert len({failure_class(raw) for raw in answers}) == 1 + assert "alpha.example" in json.loads(answers[0])["detail"] + + +async def test_a_batched_403_names_the_proxy_the_single_call_names(monkeypatch, tmp_path) -> None: + """``error`` carrying only the class is what the streak needs; it is not a + reason to tell a batched picture less than a single one. The way out of a 403 + lives in ``hint`` now, so the reducer that folds eight answers into one has to + carry it, or asking for eight backdrops hides the proxy setting that asking for + one would have named.""" + + def handler(request: httpx.Request) -> httpx.Response: + if _prompt_of(request) == "chapter one": + return httpx.Response(403, text="denied") + return httpx.Response(200, json=_chat_reply()) + + tool = _image_tool(monkeypatch, handler, model="google/gemini-2.5-flash-image", workspace=tmp_path / "ws") + + out = json.loads(await tool.execute(prompts=[{"prompt": "cover"}, {"prompt": "chapter one"}])) + + failed = out["results"][1] + assert out["failed"] == 1 and failed["error"] == "HTTP 403" + assert "tools.media.proxy" in failed["hint"] + assert failed["detail"] == "denied" + + +async def test_a_batched_picture_is_told_which_reference_was_refused(monkeypatch, tmp_path) -> None: + """The other key ``error`` was split into: the address a guarded fetch refused + moved to ``url``, and a picture is one of several that could have carried a bad + reference, so dropping it in the fold leaves the model with a refusal it cannot + act on.""" + monkeypatch.setattr(media_gen, "guarded_fetch", _refuse_every_fetch) + + def handler(_r: httpx.Request) -> httpx.Response: + return httpx.Response(200, json={"data": [{"b64_json": _B64, "media_type": "image/png"}]}) + + tool = _image_tool( + monkeypatch, + handler, + model="openai/gpt-image-2", + workspace=tmp_path / "ws", + api_base="https://api.mycorp.example/v1", + ) + + out = json.loads( + await tool.execute( + prompts=[ + {"prompt": "cover"}, + {"prompt": "chapter one", "images": ["https://10.0.0.1/logo.png"]}, + ] + ) + ) + + failed = out["results"][1] + assert out["failed"] == 1 and failed["error"] == "input image refused: not a fetchable public target" + assert failed["url"] == "https://10.0.0.1/logo.png" diff --git a/tests/test_security_web_ssrf.py b/tests/test_security_web_ssrf.py index f92df9644..17e5440bb 100644 --- a/tests/test_security_web_ssrf.py +++ b/tests/test_security_web_ssrf.py @@ -33,7 +33,9 @@ def _boom(*_a, **_k): out = await WebFetchTool().execute(url="http://totally-public.example.com/x") parsed = json.loads(out) assert "validation failed" in parsed["error"] - assert "private/internal" in parsed["error"] + # The reason sits in ``detail``: ``error`` is the tool-failure streak's + # classification key, so it carries the class and never the address. + assert "private/internal" in parsed["detail"] async def test_rejects_loopback(monkeypatch):