From c4618c4ffc33e158a54331a9ace95684f4ac48e8 Mon Sep 17 00:00:00 2001 From: Kohei Yoshino Date: Mon, 9 Nov 2026 01:01:10 -0500 Subject: [PATCH] Link Hackbot's Bugzilla comments to their run with front matter hackbot-api now puts a front matter block at the top of every Bugzilla comment it posts: the run, the action, the agent and a link to the run in the Hackbot UI. BMO's Hackbot extension hides the block and shows the agent and the link in the comment header instead (bug 2086124). The block is added at apply time, so a model can't write or change it, and the Hackbot UI and Slack never show it. ApplyContext carries the action ID; a field update merged with a comment carries the comment's ID. The block is left out when the action has no ID (older summaries) or a value isn't an identifier, since BMO would show it as text. Agent text posted without our block (those comments, a comment inside update_bug's changes, attachment comments and new-bug descriptions) starts on a new line when it begins with a rule, so BMO can't read a block the model wrote as metadata. --- docs/hackbot/actions.md | 22 +++ .../hackbot-api/app/action_handlers/base.py | 10 +- .../app/action_handlers/bugzilla_handler.py | 63 +++++++- services/hackbot-api/app/actions_applier.py | 10 +- .../hackbot-api/tests/test_actions_applier.py | 44 ++++++ .../tests/test_bugzilla_handler.py | 139 +++++++++++++++++- 6 files changed, 280 insertions(+), 8 deletions(-) diff --git a/docs/hackbot/actions.md b/docs/hackbot/actions.md index c324b461c6a..83b71fb84d9 100644 --- a/docs/hackbot/actions.md +++ b/docs/hackbot/actions.md @@ -65,6 +65,28 @@ be turned back on for a recorded message by passing `unfurl=True` to `record_mes `bugzilla.add_comment` appends a feedback-reaction footer to every recorded comment, and `is_private=true` marks it security-group-only. +When hackbot-api posts a Bugzilla comment, it puts a front matter block at the top that +links the comment to its run: + +```text +--- +hackbot: + run: + action: + agent: + url: https://hackbot.moz.tools/runs/ +--- +``` + +BMO's Hackbot extension hides the block from readers, bugmail, webhooks and Pulse, and shows +the agent and a link to the run in the comment header instead. REST `raw_text` keeps it. +The block is added at apply time, not recorded, so a model can't write or change it and the +Hackbot UI and Slack never show it. It is left out when the action has no ID (summaries +recorded before action IDs existed) or when a value isn't an identifier, because BMO would +then show it as text. Agent text posted without the block (those comments, attachment +comments and new-bug descriptions) is moved down a line when it starts with a rule, so BMO +doesn't read a block the model wrote as the comment's metadata. + Adding a type is a declaration in the runtime's domain module plus one line in hackbot-api's handler registry. The dispatch loop never changes. diff --git a/services/hackbot-api/app/action_handlers/base.py b/services/hackbot-api/app/action_handlers/base.py index b407925d994..6fb89d7b2e9 100644 --- a/services/hackbot-api/app/action_handlers/base.py +++ b/services/hackbot-api/app/action_handlers/base.py @@ -34,9 +34,9 @@ def __call__(self, key: str, max_bytes: int | None = None) -> Awaitable[bytes]: class ApplyContext: """Everything an :class:`ActionHandler` needs from the run. - Scoped to a single action application (not the whole run), since only - ``attachments`` varies per action. Handlers never talk to GCS directly — - ``download_artifact`` is provided by the caller (hackbot-api's + Scoped to a single action application (not the whole run), since + ``attachments`` and ``action_id`` vary per action. Handlers never talk to GCS + directly — ``download_artifact`` is provided by the caller (hackbot-api's ``/internal/events/apply-run-actions`` route) — so this package stays free of a dependency on any particular storage backend. Async, matching hackbot-api's own GCS wrappers and ``ActionHandler.apply`` itself. @@ -47,6 +47,10 @@ class ApplyContext: download_artifact: ArtifactDownloader hackbot_client: HackbotClient attachments: list[dict[str, str]] = field(default_factory=list) + # The `action_id` the runtime recorded for the action (`RunAction.ref`). None + # for summaries recorded before action IDs existed. When several actions are + # applied as one call, it is the one whose comment is posted. + action_id: str | None = None def artifact_key(self, name: str) -> str | None: """The uploaded key for an attachment recorded under ``name``, if any.""" diff --git a/services/hackbot-api/app/action_handlers/bugzilla_handler.py b/services/hackbot-api/app/action_handlers/bugzilla_handler.py index ff4d78d0fe0..4ae0ab03613 100644 --- a/services/hackbot-api/app/action_handlers/bugzilla_handler.py +++ b/services/hackbot-api/app/action_handlers/bugzilla_handler.py @@ -10,6 +10,7 @@ import base64 import logging import os +import re from functools import lru_cache from typing import Any @@ -57,6 +58,52 @@ def _request(method: str, path: str, json_body: dict[str, Any]) -> dict[str, Any return response.json() +# BMO's Hackbot extension reads this block off the top of comments from the +# Hackbot account, hides it, and shows the agent and a link to the run in the +# comment header instead. It reads only this shape: `run`, `action` and `agent` +# must be identifiers and `url` an https URL. Anything else shows as plain text, +# so a block that would not parse is left out rather than posted. +_IDENTIFIER = re.compile(r"[A-Za-z0-9._-]+") + + +def _front_matter(ctx: ApplyContext) -> str: + """The block that links a comment to the run and action that wrote it. + + Added here rather than by the agent so a model can't write or change it. + Empty when the action has no ID (a summary recorded before action IDs). + """ + if ctx.action_id is None: + return "" + fields = {"run": ctx.run_id, "action": ctx.action_id, "agent": ctx.agent} + invalid = [key for key, value in fields.items() if not _IDENTIFIER.fullmatch(value)] + if invalid: + log.warning( + "Posting the comment without front matter: %s not an identifier", + ", ".join(invalid), + ) + return "" + url = f"{settings.hackbot_ui_url.rstrip('/')}/runs/{ctx.run_id}" + lines = [f" {key}: {value}" for key, value in fields.items()] + return "\n".join(["---", "hackbot:", *lines, f" url: {url}", "---", "", ""]) + + +def _unparsed(text: str) -> str: + """Agent text that BMO can't take for front matter. + + BMO reads a block only at the very start of a comment, so text that starts + with a rule is moved down a line: a block the model wrote is then shown as + text rather than taken as the comment's metadata. + """ + return "\n" + text if text.startswith("---") else text + + +def _with_front_matter(comment: dict[str, Any], ctx: ApplyContext) -> dict[str, Any]: + """The comment with its front matter, or with none that BMO would read.""" + front_matter = _front_matter(ctx) + body = comment["body"] if front_matter else _unparsed(comment["body"]) + return {**comment, "body": front_matter + body} + + def _comment_body(params: dict[str, Any]) -> dict[str, Any]: """Build the ``comment`` object for a PUT /bug/{id}. @@ -81,8 +128,14 @@ async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult # comment (the coalescer hands us params built from other rows). body = dict(params.get("changes", {})) if params.get("comment"): - body["comment"] = params["comment"] + body["comment"] = _with_front_matter(params["comment"], ctx) body["comment_tags"] = [ctx.agent] + elif isinstance(body.get("comment"), dict) and isinstance( + body["comment"].get("body"), str + ): + # A comment the agent put in `changes` itself, which is also posted + # from the Hackbot account. + body["comment"] = _with_front_matter(body["comment"], ctx) try: _request("PUT", f"bug/{bug_id}", body) except Exception as exc: @@ -94,7 +147,8 @@ async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult class AddCommentHandler: async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult: bug_id = params["bug_id"] - body: dict[str, Any] = {"comment": _comment_body(params)} + comment = _with_front_matter(_comment_body(params), ctx) + body: dict[str, Any] = {"comment": comment} body["comment_tags"] = [ctx.agent] try: _request("PUT", f"bug/{bug_id}", body) @@ -126,7 +180,7 @@ async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult "is_patch": params.get("is_patch", False), } if params.get("comment"): - body["comment"] = params["comment"] + body["comment"] = _unparsed(params["comment"]) try: data = _request("POST", f"bug/{bug_id}/attachment", body) @@ -146,6 +200,9 @@ async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult class CreateBugHandler: async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult: + # The description is comment 0, which gets no front matter. + if isinstance(params.get("description"), str): + params = {**params, "description": _unparsed(params["description"])} try: data = _request("POST", "bug", params) except Exception as exc: diff --git a/services/hackbot-api/app/actions_applier.py b/services/hackbot-api/app/actions_applier.py index f016f510863..6ec3a2169fd 100644 --- a/services/hackbot-api/app/actions_applier.py +++ b/services/hackbot-api/app/actions_applier.py @@ -164,6 +164,7 @@ async def _dispatch( params: dict, attachments: list[dict], hackbot_client: HackbotClient, + action_id: str | None, ) -> ActionResult: """Run one handler call, converting failures into a failed `ActionResult`. @@ -184,6 +185,7 @@ async def _dispatch( attachments=attachments, agent=run.agent, hackbot_client=hackbot_client, + action_id=action_id, ) try: return await handler.apply(params, ctx) @@ -303,18 +305,24 @@ async def _apply_pending_rows( (member.type, resolve_placeholders(member.params, results_by_ref)) for member in member_rows ] + # The group carries at most one comment, which keeps its own ID. + comment_id = next( + (m.ref for m in member_rows if m.type == "bugzilla.add_comment"), + None, + ) outcome = await _dispatch( run, "bugzilla.update_bug", merge_resolved(entries), [], hackbot_client, + comment_id, ) else: member_rows = [row] params = resolve_placeholders(row.params, results_by_ref) outcome = await _dispatch( - run, row.type, params, attachments, hackbot_client + run, row.type, params, attachments, hackbot_client, row.ref ) # Only stamp applied_at on a real success, so a failed row isn't diff --git a/services/hackbot-api/tests/test_actions_applier.py b/services/hackbot-api/tests/test_actions_applier.py index c3fd1976af2..32bb647d176 100644 --- a/services/hackbot-api/tests/test_actions_applier.py +++ b/services/hackbot-api/tests/test_actions_applier.py @@ -670,6 +670,50 @@ async def test_extra_comments_applied_separately(monkeypatch): ] +class _ContextRecordingHandler: + def __init__(self): + self.action_ids = [] + + async def apply(self, params, ctx): + self.action_ids.append(ctx.action_id) + return SimpleNamespace(status="applied", result={}, error=None) + + +async def test_handlers_get_the_action_id_of_the_comment_they_post(monkeypatch): + """A coalesced PUT carries its comment's ID, which goes in the front matter.""" + handler = _ContextRecordingHandler() + monkeypatch.setattr(actions_applier, "get_handler", lambda t: handler) + + update = _row( + 0, + "pending", + action_type="bugzilla.update_bug", + params={"bug_id": 5, "changes": {"status": "RESOLVED"}}, + ref="update-id", + ) + comment = _row( + 1, + "pending", + action_type="bugzilla.add_comment", + params={"bug_id": 5, "text": "done"}, + ref="comment-id", + ) + other = _row( + 2, + "pending", + action_type="bugzilla.add_comment", + params={"bug_id": 99, "text": "different bug"}, + ref="other-id", + ) + rows = [(update, []), (comment, []), (other, [])] + + await actions_applier._apply_pending_rows( + _FakeDB(), _FakeRun(status=RunStatus.succeeded.value), rows, _CLIENT + ) + + assert handler.action_ids == ["comment-id", "other-id"] + + async def test_lone_same_type_actions_on_different_bugs_not_merged(monkeypatch): handler = _RecordingHandler( SimpleNamespace(status="applied", result={}, error=None) diff --git a/services/hackbot-api/tests/test_bugzilla_handler.py b/services/hackbot-api/tests/test_bugzilla_handler.py index 1b7d855d5af..f30068449d0 100644 --- a/services/hackbot-api/tests/test_bugzilla_handler.py +++ b/services/hackbot-api/tests/test_bugzilla_handler.py @@ -6,11 +6,13 @@ """ import base64 +import logging from app.action_handlers import ApplyContext, bugzilla_handler +from app.config import settings -def _ctx(attachments=None, artifacts=None, agent="test-agent"): +def _ctx(attachments=None, artifacts=None, agent="test-agent", action_id=None): artifacts = artifacts or {} async def download(key): @@ -22,6 +24,7 @@ async def download(key): attachments=attachments or [], agent=agent, hackbot_client=object(), + action_id=action_id, ) @@ -99,6 +102,140 @@ async def test_comment_handlers_add_acting_agent_tag(monkeypatch): ] +# What BMO's Hackbot extension expects at the top of the comment: identifiers +# for run, action and agent, an https url, then a blank line before the text. +_FRONT_MATTER = ( + "---\n" + "hackbot:\n" + " run: run-1\n" + " action: 3f2a9c1e\n" + " agent: frontend-triage\n" + " url: https://hackbot.example/runs/run-1\n" + "---\n" + "\n" +) + + +async def test_comment_handlers_prepend_front_matter(monkeypatch): + monkeypatch.setattr(settings, "hackbot_ui_url", "https://hackbot.example/") + bodies = [] + monkeypatch.setattr( + bugzilla_handler, "_request", lambda _m, _p, body: bodies.append(body) + ) + ctx = _ctx(agent="frontend-triage", action_id="3f2a9c1e") + + await bugzilla_handler.AddCommentHandler().apply( + {"bug_id": 5, "text": "standalone"}, ctx + ) + await bugzilla_handler.UpdateBugHandler().apply( + {"bug_id": 5, "changes": {}, "comment": {"body": "coalesced"}}, ctx + ) + + assert [body["comment"]["body"] for body in bodies] == [ + _FRONT_MATTER + "standalone", + _FRONT_MATTER + "coalesced", + ] + + +async def test_update_bug_handler_without_comment_has_no_front_matter(monkeypatch): + bodies = [] + monkeypatch.setattr( + bugzilla_handler, "_request", lambda _m, _p, body: bodies.append(body) + ) + await bugzilla_handler.UpdateBugHandler().apply( + {"bug_id": 5, "changes": {"status": "RESOLVED"}}, + _ctx(action_id="3f2a9c1e"), + ) + assert bodies == [{"status": "RESOLVED"}] + + +async def test_comment_without_action_id_has_no_front_matter(monkeypatch): + """Summaries recorded before action IDs existed post the text as it is.""" + bodies = [] + monkeypatch.setattr( + bugzilla_handler, "_request", lambda _m, _p, body: bodies.append(body) + ) + await bugzilla_handler.AddCommentHandler().apply( + {"bug_id": 5, "text": "hi"}, _ctx(action_id=None) + ) + assert bodies[0]["comment"]["body"] == "hi" + + +async def test_agent_block_is_not_read_as_front_matter(monkeypatch): + """Without our block, a block the model wrote must not reach the top.""" + bodies = [] + monkeypatch.setattr( + bugzilla_handler, "_request", lambda _m, _p, body: bodies.append(body) + ) + forged = "---\nhackbot:\n run: x\n action: y\n agent: z\n---\n\nhi" + await bugzilla_handler.AddCommentHandler().apply( + {"bug_id": 5, "text": forged}, _ctx(action_id=None) + ) + await bugzilla_handler.AddCommentHandler().apply( + {"bug_id": 5, "text": forged}, _ctx(action_id="3f2a9c1e") + ) + assert bodies[0]["comment"]["body"] == "\n" + forged + # With our block first, BMO reads ours and shows the model's as text. + assert bodies[1]["comment"]["body"].endswith("---\n\n" + forged) + assert bodies[1]["comment"]["body"].startswith("---\nhackbot:\n run: run-1\n") + + +async def test_other_agent_text_is_not_read_as_front_matter(monkeypatch): + """A block the model wrote can't reach the top of any comment we post.""" + calls = [] + monkeypatch.setattr( + bugzilla_handler, + "_request", + lambda _m, path, body: calls.append((path, body)) or {"ids": [1], "id": 1}, + ) + forged = "---\nhackbot:\n run: x\n action: y\n agent: z\n---\n\nhi" + ctx = _ctx( + attachments=[{"name": "file", "uploaded_key": "k"}], + artifacts={"k": b"data"}, + action_id="3f2a9c1e", + ) + + await bugzilla_handler.UpdateBugHandler().apply( + {"bug_id": 5, "changes": {"comment": {"body": forged}}}, ctx + ) + await bugzilla_handler.AddAttachmentHandler().apply( + { + "bug_id": 5, + "file_name": "a.txt", + "summary": "a", + "content_type": "text/plain", + "comment": forged, + }, + ctx, + ) + await bugzilla_handler.CreateBugHandler().apply( + {"product": "P", "summary": "S", "description": forged}, ctx + ) + + update, attachment, new_bug = (body for _path, body in calls) + # A comment in `changes` gets our block first, like any other comment. + assert update["comment"]["body"].startswith("---\nhackbot:\n run: run-1\n") + assert update["comment"]["body"].endswith("\n\n" + forged) + # Attachment comments and descriptions get no block, so they are moved down. + assert attachment["comment"] == "\n" + forged + assert new_bug["description"] == "\n" + forged + + +async def test_front_matter_left_out_when_bmo_would_not_parse_it(monkeypatch, caplog): + """BMO shows a block it can't parse as text, so none is posted.""" + bodies = [] + monkeypatch.setattr( + bugzilla_handler, "_request", lambda _m, _p, body: bodies.append(body) + ) + with caplog.at_level(logging.WARNING): + await bugzilla_handler.AddCommentHandler().apply( + {"bug_id": 5, "text": "hi"}, + _ctx(agent="frontend triage", action_id="3f2a9c1e"), + ) + assert bodies[0]["comment"]["body"] == "hi" + assert "agent not an identifier" in caplog.text + + async def test_add_attachment_handler_downloads_and_base64_encodes(monkeypatch): seen = {} monkeypatch.setattr(