diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/frontend-triage.md b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/frontend-triage.md index dcee83c455..92999eed86 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/frontend-triage.md +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/frontend-triage.md @@ -19,6 +19,12 @@ Typical components: Desktop and Android bugs here are usually UI/UX papercuts, documented with a **video or screenshot** and steps to reproduce. +Screenshots you can look at; screen recordings you cannot. `download_attachment` +refuses video, audio, and archive attachments, because Claude cannot interpret them +and fetching one costs a large download for nothing. When the only evidence is a +recording, triage from the description, the steps to reproduce, and the code, and say +plainly in your comment that you did not view the recording — do not imply you did. + **Install and update bugs look different, and that is not a reason to skip them.** An installer or updater bug is normally a _failure_ rather than a papercut: an update that did not apply, an install that rolled back, a version that stayed where it was, diff --git a/libs/agent-tools/agent_tools/bugzilla.py b/libs/agent-tools/agent_tools/bugzilla.py index aa256750d3..6437af42a9 100644 --- a/libs/agent-tools/agent_tools/bugzilla.py +++ b/libs/agent-tools/agent_tools/bugzilla.py @@ -9,7 +9,9 @@ from __future__ import annotations import base64 +import mimetypes from dataclasses import dataclass +from pathlib import PurePosixPath from typing import Annotated, Any import bugsy @@ -17,6 +19,161 @@ from agent_tools.registry import ToolError, tool, tools_in +# What Claude can actually interpret once an attachment reaches it — as an image +# block via the built-in Read tool, or as text. Deliberately narrower than what +# Bugzilla accepts: a screencast costs a full download and tens of thousands of +# tokens and still comes back undecodable, which is what +# https://github.com/mozilla/bugbug/issues/6701 was filed about. Anything absent +# is refused, so a new video or archive format needs no matching reject list. +ALLOWED_ATTACHMENT_TYPES = frozenset( + { + # The four image formats the Anthropic API accepts as image blocks. + "image/png", + "image/jpeg", + "image/gif", + "image/webp", + # Not rendered as an image, but readable as markup. + "image/svg+xml", + "application/pdf", + # Textual application/* types Bugzilla serves for logs, testcases, configs. + "application/json", + "application/xml", + "application/javascript", + "application/x-javascript", + } +) + +# Every text/* subtype is allowed on top of the set above — text/plain, text/html, +# text/css, text/csv, text/x-log and text/x-phabricator-request are all just text, +# and enumerating them would only mean missing one. +_ALLOWED_TYPE_PREFIX = "text/" + +# Bugzilla records the type the uploader chose, so a plain log routinely arrives +# as application/octet-stream, and so does a screencast saved without a type. +# Resolve those from the file name instead. +_OCTET_STREAM = "application/octet-stream" + +# Python's built-in table, with the system mime database deliberately left out +# (`filenames=()`). Reading /etc/mime.types would make the verdict depend on the +# host: .log resolves to text/plain on a macOS dev box via /etc/apache2/mime.types +# and to nothing in the CI and agent containers, so an update.log would be +# readable on a laptop and refused in production. The built-in table already +# covers .md, .json, .csv, .html, .png and, on the reject side, .mp4, .webm, +# .mov, .zip and .tar. +_MIMETYPES = mimetypes.MimeTypes(filenames=()) + +# What the built-in table does not know, and Bugzilla reporters attach anyway. +_EXTENSION_TYPES = { + ".log": "text/plain", + ".diff": "text/plain", + ".patch": "text/plain", +} + +# `claude_sdk._make_tool` can only emit {"type": "text"}, so a base64 image in a +# tool result is never an image block, just a very long string. +INLINEABLE_ATTACHMENT_TYPES = frozenset( + { + "application/json", + "application/xml", + "application/javascript", + "application/x-javascript", + # Markup, so the text is the content. + "image/svg+xml", + } +) + +# Bugzilla accepts attachments up to 10MB, and base64 inflates by 4/3, so an +# unbounded inline is ~13MB of text for one file. 256KiB decoded is ~350KB of +# base64, on the order of 85k tokens: enough for an update.log or a testcase, +# and past that download_attachment plus Grep beats reading the whole thing. +MAX_INLINE_BYTES = 256 * 1024 + +MAX_INLINE_ATTACHMENTS = 10 + +# The allowlist in the form the agent reads back in an error payload. +_ALLOWED_SUMMARY = ( + "text/*, images (png/jpeg/gif/webp/svg), PDF, and JSON/XML/JavaScript" +) + + +def _normalize_type(content_type: str | None) -> str: + """Lowercase a content type and drop its parameters. + + Bugzilla passes through whatever the uploader sent, so ``TEXT/PLAIN`` and + ``text/plain; charset=UTF-8`` both turn up and both mean text/plain. + """ + return (content_type or "").split(";", 1)[0].strip().lower() + + +def _effective_content_type(att: dict[str, Any]) -> str: + """Resolve what an attachment is, rather than only what it claims to be. + + ``is_patch`` wins outright because a Bugzilla patch is text by definition — + ``hackbot_runtime.actions.bugzilla.add_attachment`` makes the same assumption + in the write direction. Returns "" when nothing identifies the attachment, + which the caller treats as not readable. + """ + if att.get("is_patch"): + return "text/plain" + content_type = _normalize_type(att.get("content_type")) + if content_type and content_type != _OCTET_STREAM: + return content_type + file_name = att.get("file_name") or "" + suffix = PurePosixPath(file_name).suffix.lower() + guessed = _EXTENSION_TYPES.get(suffix) or _MIMETYPES.guess_type(file_name)[0] + return _normalize_type(guessed) + + +def attachment_type_allowed(att: dict[str, Any]) -> tuple[bool, str]: + """Return whether an attachment is readable, and the type that decided it. + + The type comes back too so a caller can say *what* it refused instead of only + that it refused something. + """ + effective = _effective_content_type(att) + allowed = ( + effective.startswith(_ALLOWED_TYPE_PREFIX) + or effective in ALLOWED_ATTACHMENT_TYPES + ) + return allowed, effective + + +def _inlineable(effective_content_type: str) -> bool: + """Whether this type is worth base64-ing into a tool result rather than a file.""" + return ( + effective_content_type.startswith(_ALLOWED_TYPE_PREFIX) + or effective_content_type in INLINEABLE_ATTACHMENT_TYPES + ) + + +def _type_not_allowed_error( + attachment_id: int, att: dict[str, Any], effective: str +) -> ToolError: + """Build the refusal an agent sees for an attachment it cannot read. + + ``claude_sdk`` renders a ToolError payload as the tool result itself, so the + hint is the only place to keep the agent from burning turns retrying, and to + tell it to disclose that it never saw the attachment. + """ + return ToolError( + f"attachment {attachment_id} is " + f"{effective or 'of an unrecognized type'}, which Claude cannot read", + payload={ + "error": "attachment_type_not_allowed", + "attachment_id": attachment_id, + "file_name": att.get("file_name"), + "content_type": att.get("content_type"), + "effective_content_type": effective or None, + "allowed": _ALLOWED_SUMMARY, + "hint": ( + "Video, audio, and archive attachments are refused on purpose — " + "Claude cannot interpret them and downloading one is expensive. " + "Do not retry. Work from the bug's text and say in your comment " + "that you did not view this attachment." + ), + }, + ) + @dataclass class BugzillaContext: @@ -193,8 +350,9 @@ async def get_bug_attachments( bool, Field( description=( - "If true, include base64-encoded attachment content. Default " - "false. Use sparingly — attachments can be large." + "If true, inline base64 content for the textual attachments " + "only (text/*, JSON, XML, JavaScript, SVG) under 256KiB. Images " + "and PDFs are never inlined; use download_attachment for those." ) ), ] = False, @@ -202,13 +360,73 @@ async def get_bug_attachments( """Fetch attachments for a bug. By default returns metadata only (cheap, safe for large binaries). Set - include_data=true to also download the content — Bugzilla returns it - base64-encoded in the 'data' field of each attachment. + include_data=true to also inline the content, base64-encoded in each + attachment's 'data' field, for the textual types only: text/*, JSON, XML, + JavaScript, and SVG, under 256KiB, for the first 10 that qualify. + + Anything else keeps its metadata and gains a 'data_omitted' note saying why. + Images and PDFs are among them on purpose: a tool result is text, so a + base64 image is a long string rather than something Claude can see. Use + download_attachment and read the file for those. An attachment the proxy + refuses gets a 'data_error' and does not take the rest of the response with + it. """ - params = {} if include_data else {"exclude_fields": "data"} - result = _request(ctx, f"bug/{bug_id}/attachment", params) + result = _request(ctx, f"bug/{bug_id}/attachment", {"exclude_fields": "data"}) atts = result.get("bugs", {}).get(str(bug_id), []) - return {"bug_id": bug_id, "count": len(atts), "attachments": atts} + payload = {"bug_id": bug_id, "count": len(atts), "attachments": atts} + if not include_data: + return payload + + # The list endpoint is all-or-nothing on `data`, so asking it for content + # would pull a screencast over the wire before we could drop it. Fetching the + # inlineable attachments one at a time never moves the bytes this is here to + # avoid, at the cost of one request each. + inlined = 0 + omitted = 0 + errors = 0 + for att in atts: + allowed, effective = attachment_type_allowed(att) + size = att.get("size") + if not allowed: + note = f"{effective or 'unrecognized type'} is not readable by Claude" + elif not _inlineable(effective): + note = ( + f"{effective} is binary, and a tool result is text, so inlining it " + "would cost the whole file in tokens without producing an image. " + "Use download_attachment and read the file." + ) + elif isinstance(size, int) and size > MAX_INLINE_BYTES: + note = ( + f"{size} bytes is over the {MAX_INLINE_BYTES}-byte inline limit. " + "Use download_attachment, then Grep the file for what you need." + ) + elif inlined >= MAX_INLINE_ATTACHMENTS: + note = ( + f"only the first {MAX_INLINE_ATTACHMENTS} attachments are inlined " + "per call. Use download_attachment for this one." + ) + else: + try: + one = _request(ctx, f"bug/attachment/{att['id']}") + except ToolError as e: + att["data_error"] = e.payload or {"message": str(e)} + errors += 1 + continue + fetched = one.get("attachments", {}).get(str(att["id"])) + if fetched is None: + att["data_error"] = {"error": "attachment_not_returned"} + errors += 1 + continue + att["data"] = fetched.get("data") + inlined += 1 + continue + att["data_omitted"] = note + omitted += 1 + + payload["inlined_count"] = inlined + payload["omitted_count"] = omitted + payload["error_count"] = errors + return payload @tool @@ -233,17 +451,35 @@ async def download_attachment( the agent never has to round-trip the blob through its own context. Use get_bug_attachments first to discover attachment IDs. Returns the written path, size, and content_type. + + Only types Claude can read are downloaded: text/*, png/jpeg/gif/webp/svg + images, PDF, and JSON/XML/JavaScript. Video, audio, and archives fail with + 'attachment_type_not_allowed' — a permanent answer, not a retryable one. """ - result = _request(ctx, f"bug/attachment/{attachment_id}") + # Metadata first, so a refused attachment's bytes never leave Bugzilla. One + # extra round trip on the happy path buys that. + meta = _request(ctx, f"bug/attachment/{attachment_id}", {"exclude_fields": "data"}) - att = result.get("attachments", {}).get(str(attachment_id)) + att = meta.get("attachments", {}).get(str(attachment_id)) if att is None: raise ToolError( f"attachment {attachment_id} not found", payload={"error": "attachment_not_found", "attachment_id": attachment_id}, ) - raw = base64.b64decode(att["data"]) + allowed, effective = attachment_type_allowed(att) + if not allowed: + raise _type_not_allowed_error(attachment_id, att, effective) + + full = _request(ctx, f"bug/attachment/{attachment_id}") + data = full.get("attachments", {}).get(str(attachment_id), {}).get("data") + if data is None: + raise ToolError( + f"attachment {attachment_id} came back without data", + payload={"error": "attachment_no_data", "attachment_id": attachment_id}, + ) + + raw = base64.b64decode(data) with open(dest_path, "wb") as fp: fp.write(raw) diff --git a/libs/agent-tools/tests/test_bugzilla.py b/libs/agent-tools/tests/test_bugzilla.py index 587202c2d2..d33d3b9ca6 100644 --- a/libs/agent-tools/tests/test_bugzilla.py +++ b/libs/agent-tools/tests/test_bugzilla.py @@ -2,6 +2,7 @@ from unittest.mock import MagicMock +import bugsy import pytest from agent_tools import bugzilla from agent_tools.bugzilla import BugzillaContext @@ -52,3 +53,329 @@ async def test_search_bugs_raises_tool_error_on_bugsy_failure(): with pytest.raises(ToolError) as ei: await bugzilla.search_bugs(BugzillaContext(client=client), params={}) assert ei.value.payload["error"] == "access_denied" + + +def _attachment_client(att, *, data="aGVsbG8="): + """A bugsy client that serves one attachment, with and without its data. + + Mirrors Bugzilla: `exclude_fields=data` returns the metadata only, and the + unqualified request returns the same record plus base64 `data`. Records every + (path, params) pair so a test can assert what was and was not fetched. + """ + client = MagicMock() + calls = [] + + def request(path, params=None): + params = params or {} + calls.append((path, params)) + if path.endswith("/attachment"): + body = ( + dict(att) + if params.get("exclude_fields") == "data" + else {**att, "data": data} + ) + return {"bugs": {"1": [body]}} + if params.get("exclude_fields") == "data": + return {"attachments": {str(att["id"]): dict(att)}} + return {"attachments": {str(att["id"]): {**att, "data": data}}} + + client.request.side_effect = request + client.calls = calls + return client + + +def _att(**overrides): + base = {"id": 7, "file_name": "shot.png", "content_type": "image/png"} + return {**base, **overrides} + + +async def test_download_attachment_writes_allowed_image(tmp_path): + client = _attachment_client(_att()) + dest = tmp_path / "shot.png" + result = await bugzilla.download_attachment( + BugzillaContext(client=client), attachment_id=7, dest_path=str(dest) + ) + assert dest.read_bytes() == b"hello" + assert result["content_type"] == "image/png" + assert result["size_bytes"] == 5 + # Metadata is probed before the bytes are asked for. + assert client.calls[0] == ("bug/attachment/7", {"exclude_fields": "data"}) + assert client.calls[1] == ("bug/attachment/7", {}) + + +async def test_download_attachment_writes_allowed_text(tmp_path): + client = _attachment_client(_att(file_name="update.log", content_type="text/x-log")) + dest = tmp_path / "update.log" + await bugzilla.download_attachment( + BugzillaContext(client=client), attachment_id=7, dest_path=str(dest) + ) + assert dest.read_bytes() == b"hello" + + +@pytest.mark.parametrize( + "att", + [ + _att(file_name="screencast.mp4", content_type="video/mp4"), + _att(file_name="logs.zip", content_type="application/zip"), + _att(file_name="clip.webm", content_type="video/webm"), + _att(file_name="mystery", content_type=""), + _att(file_name="mystery", content_type=None), + ], + ids=["mp4", "zip", "webm", "empty-type", "no-type"], +) +async def test_download_attachment_refuses_unreadable_types(tmp_path, att): + client = _attachment_client(att) + dest = tmp_path / "out.bin" + with pytest.raises(ToolError) as ei: + await bugzilla.download_attachment( + BugzillaContext(client=client), attachment_id=7, dest_path=str(dest) + ) + assert ei.value.payload["error"] == "attachment_type_not_allowed" + assert not dest.exists() + # The refusal costs one metadata request; the bytes are never fetched. + assert client.calls == [("bug/attachment/7", {"exclude_fields": "data"})] + + +@pytest.mark.parametrize( + ("file_name", "allowed"), + [ + ("update.log", True), + ("patch.diff", True), + ("bug.patch", True), + ("shot.png", True), + ("screencast.mp4", False), + ("trace", False), + ], +) +async def test_octet_stream_resolves_by_file_name(tmp_path, file_name, allowed): + """Bugzilla's fallback type covers both plain logs and untyped screencasts.""" + client = _attachment_client( + _att(file_name=file_name, content_type="application/octet-stream") + ) + dest = tmp_path / "out.bin" + ctx = BugzillaContext(client=client) + if allowed: + await bugzilla.download_attachment(ctx, attachment_id=7, dest_path=str(dest)) + assert dest.read_bytes() == b"hello" + else: + with pytest.raises(ToolError) as ei: + await bugzilla.download_attachment( + ctx, attachment_id=7, dest_path=str(dest) + ) + assert ei.value.payload["error"] == "attachment_type_not_allowed" + + +async def test_is_patch_overrides_an_odd_content_type(tmp_path): + """A patch is text even when the uploader typed it as something else.""" + client = _attachment_client( + _att( + file_name="fix.patch", + content_type="application/octet-stream", + is_patch=True, + ) + ) + dest = tmp_path / "fix.patch" + await bugzilla.download_attachment( + BugzillaContext(client=client), attachment_id=7, dest_path=str(dest) + ) + assert dest.read_bytes() == b"hello" + + +async def test_content_type_parameters_and_case_are_ignored(tmp_path): + client = _attachment_client( + _att(file_name="steps.txt", content_type="TEXT/PLAIN; charset=UTF-8") + ) + dest = tmp_path / "steps.txt" + await bugzilla.download_attachment( + BugzillaContext(client=client), attachment_id=7, dest_path=str(dest) + ) + assert dest.read_bytes() == b"hello" + + +async def test_download_attachment_not_found(): + client = MagicMock() + client.request.return_value = {"attachments": {}} + with pytest.raises(ToolError) as ei: + await bugzilla.download_attachment( + BugzillaContext(client=client), attachment_id=7, dest_path="/tmp/x" + ) + assert ei.value.payload["error"] == "attachment_not_found" + + +async def test_get_bug_attachments_metadata_only_never_asks_for_data(): + client = _attachment_client(_att()) + result = await bugzilla.get_bug_attachments( + BugzillaContext(client=client), bug_id=1 + ) + assert result["count"] == 1 + assert "data" not in result["attachments"][0] + assert client.calls == [("bug/1/attachment", {"exclude_fields": "data"})] + + +def _list_client(atts, *, data="aGVsbG8=", fail_ids=()): + """A client serving an attachment list, then per-attachment data on demand. + + `fail_ids` makes those attachment IDs raise the way the proxy does for a bug + the API key cannot reach. + """ + client = MagicMock() + calls = [] + + def request(path, params=None): + calls.append((path, params or {})) + if path == "bug/1/attachment": + return {"bugs": {"1": [dict(a) for a in atts]}} + att_id = path.rsplit("/", 1)[1] + if int(att_id) in fail_ids: + err = bugsy.BugsyException("nope") + err.code = 102 + raise err + source = next(a for a in atts if str(a["id"]) == att_id) + return {"attachments": {att_id: {**source, "data": data}}} + + client.request.side_effect = request + client.calls = calls + return client + + +async def test_get_bug_attachments_inlines_text_and_refuses_video(): + log = {"id": 7, "file_name": "update.log", "content_type": "text/plain"} + mp4 = {"id": 8, "file_name": "screencast.mp4", "content_type": "video/mp4"} + client = _list_client([log, mp4]) + + result = await bugzilla.get_bug_attachments( + BugzillaContext(client=client), bug_id=1, include_data=True + ) + + by_id = {a["id"]: a for a in result["attachments"]} + assert by_id[7]["data"] == "aGVsbG8=" + assert "data" not in by_id[8] + assert "video/mp4" in by_id[8]["data_omitted"] + assert (result["inlined_count"], result["omitted_count"]) == (1, 1) + # The video is never fetched: one list call plus one fetch for the log. + assert client.calls == [ + ("bug/1/attachment", {"exclude_fields": "data"}), + ("bug/attachment/7", {}), + ] + + +async def test_get_bug_attachments_does_not_inline_binary_it_would_still_download(): + """A tool result is text, so a base64 png is a long string, not an image block. + + download_attachment plus Read is the only path that reaches the model as an + image, so inlining one would cost the whole file in tokens for nothing. + """ + png = {"id": 7, "file_name": "shot.png", "content_type": "image/png"} + pdf = {"id": 8, "file_name": "report.pdf", "content_type": "application/pdf"} + svg = {"id": 9, "file_name": "case.svg", "content_type": "image/svg+xml"} + client = _list_client([png, pdf, svg]) + + result = await bugzilla.get_bug_attachments( + BugzillaContext(client=client), bug_id=1, include_data=True + ) + + by_id = {a["id"]: a for a in result["attachments"]} + for att_id in (7, 8): + assert "data" not in by_id[att_id] + assert "download_attachment" in by_id[att_id]["data_omitted"] + # SVG is markup, so the text is the content. + assert by_id[9]["data"] == "aGVsbG8=" + assert (result["inlined_count"], result["omitted_count"]) == (1, 2) + assert ("bug/attachment/7", {}) not in client.calls + assert ("bug/attachment/8", {}) not in client.calls + + # Still downloadable to disk, where size does not cost context. + assert bugzilla.attachment_type_allowed(png) == (True, "image/png") + assert bugzilla.attachment_type_allowed(pdf) == (True, "application/pdf") + + +async def test_get_bug_attachments_skips_text_over_the_inline_size_limit(): + big = { + "id": 7, + "file_name": "huge.log", + "content_type": "text/plain", + "size": bugzilla.MAX_INLINE_BYTES + 1, + } + small = { + "id": 8, + "file_name": "small.log", + "content_type": "text/plain", + "size": bugzilla.MAX_INLINE_BYTES, + } + client = _list_client([big, small]) + + result = await bugzilla.get_bug_attachments( + BugzillaContext(client=client), bug_id=1, include_data=True + ) + + by_id = {a["id"]: a for a in result["attachments"]} + assert "data" not in by_id[7] + assert "inline limit" in by_id[7]["data_omitted"] + assert by_id[8]["data"] == "aGVsbG8=" + assert ("bug/attachment/7", {}) not in client.calls + + +async def test_get_bug_attachments_caps_the_number_it_inlines(): + atts = [ + {"id": i, "file_name": f"{i}.log", "content_type": "text/plain"} + for i in range(1, bugzilla.MAX_INLINE_ATTACHMENTS + 4) + ] + client = _list_client(atts) + + result = await bugzilla.get_bug_attachments( + BugzillaContext(client=client), bug_id=1, include_data=True + ) + + assert result["inlined_count"] == bugzilla.MAX_INLINE_ATTACHMENTS + assert result["omitted_count"] == 3 + # Nothing is dropped silently: the ones past the cap say so. + over = [a for a in result["attachments"] if "data" not in a] + assert len(over) == 3 + assert all("only the first" in a["data_omitted"] for a in over) + + +async def test_get_bug_attachments_one_failure_does_not_discard_the_rest(): + """Before the per-attachment loop this was one request that could not fail partway. + + A raise from the loop would throw away everything already fetched, so an + inaccessible attachment has to degrade to a note on itself. + """ + a = {"id": 7, "file_name": "a.log", "content_type": "text/plain"} + b = {"id": 8, "file_name": "b.log", "content_type": "text/plain"} + c = {"id": 9, "file_name": "c.log", "content_type": "text/plain"} + client = _list_client([a, b, c], fail_ids=(8,)) + + result = await bugzilla.get_bug_attachments( + BugzillaContext(client=client), bug_id=1, include_data=True + ) + + by_id = {x["id"]: x for x in result["attachments"]} + assert by_id[7]["data"] == "aGVsbG8=" + assert by_id[9]["data"] == "aGVsbG8=" + assert by_id[8]["data_error"]["error"] == "access_denied" + assert (result["inlined_count"], result["error_count"]) == (2, 1) + + +def test_type_resolution_does_not_read_the_system_mime_database(monkeypatch): + """The verdict must not depend on whether /etc/mime.types knows an extension. + + `.log` resolves to text/plain from /etc/apache2/mime.types on macOS and to + nothing in the CI and agent containers, so leaning on `mimetypes.guess_type` + made an update.log readable on a laptop and refused in production. Poison the + module-level function to prove nothing calls it. + """ + + def boom(*args, **kwargs): + raise AssertionError("system mime database consulted") + + monkeypatch.setattr(bugzilla.mimetypes, "guess_type", boom) + + for file_name in ("update.log", "patch.diff", "bug.patch"): + att = { + "file_name": file_name, + "content_type": "application/octet-stream", + } + assert bugzilla.attachment_type_allowed(att) == (True, "text/plain") + + mp4 = {"file_name": "screencast.mp4", "content_type": "application/octet-stream"} + assert bugzilla.attachment_type_allowed(mp4) == (False, "video/mp4")