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(