Gate Bugzilla attachment downloads behind a mimetype allowlist

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,

libs/agent-tools/agent_tools/bugzilla.py

@@ -9,14 +9,171 @@
 from __future__ import annotations
 
 import base64
+import mimetypes
 from dataclasses import dataclass
+from pathlib import PurePosixPath
 from typing import Annotated, Any
 
 import bugsy
 from pydantic import Field
 
 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,22 +350,83 @@ 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,
 ) -> dict:
     """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)