diff --git a/services/hackbot-pulse-listener/README.md b/services/hackbot-pulse-listener/README.md index d098b4fd77..0935faf895 100644 --- a/services/hackbot-pulse-listener/README.md +++ b/services/hackbot-pulse-listener/README.md @@ -2,8 +2,9 @@ Listens to Taskcluster build-failure pulse messages, and for failed **Firefox build tasks** triggers the `build-repair` hackbot agent through the hackbot-api. When a run -finishes (minutes later) it emails the developer who pushed the change a link to the -hackbot UI and a summary of the analysis and fix. +finishes (minutes later) it emails a link to the hackbot UI, a summary of the analysis +and fix, a Treeherder link to the failing job, and the commit the agent blamed for the +failure. The email's primary recipient is the author of that blamed commit. ## How it works @@ -14,8 +15,11 @@ hackbot UI and a summary of the analysis and fix. 3. Fetch the task definition to read `GECKO_HEAD_REV` (the revision is not in the message). 4. Dedupe by revision with an in-memory TTL cache, so only one agent run is triggered per revision even when many build tasks fail for the same push. -5. `POST /agents/build-repair/runs`, then poll `GET /runs/{run_id}` until terminal and send - the report email. +5. Trigger the agent with the failing tasks -- it resolves the push commits itself and + returns the commit it blamed for the failure, so the listener does no pushlog lookups. +6. `POST /agents/build-repair/runs`, then poll `GET /runs/{run_id}` until terminal. Look the + blamed commit up in the firefox GitHub mirror to get its author's email and send the + report email to that developer. The dedupe cache and pending-run tracking are in-memory (reset on restart). @@ -31,9 +35,14 @@ uv run --package hackbot-pulse-listener python -m app ``` Email is sent only when `SENDGRID_API_KEY` and `NOTIFICATION_SENDER` are set; otherwise it -is logged and skipped. Notifications go to the revision author and, if set, the `NOTIFICATION_TEAM_EMAIL` team address. Set `NOTIFICATION_OVERRIDE_EMAIL` to route every notification to a +is logged and skipped. The blamed commit's author (looked up in the firefox GitHub mirror), +the pushing developer, and, if set, the +`NOTIFICATION_TEAM_EMAIL` team address all get the email, which spells out why each +recipient is on it. Set `NOTIFICATION_OVERRIDE_EMAIL` to route every notification to a single address (useful for local testing). By default only runs that produced a patch are emailed; set `NOTIFY_ONLY_WITH_PATCH=false` to also notify on transient / not-to-blame runs. +When `NOTIFICATION_TEAM_EMAIL` is set, notifications use it as `Reply-To` so recipients can +reply with feedback on the analysis. ## Test diff --git a/services/hackbot-pulse-listener/app/config.py b/services/hackbot-pulse-listener/app/config.py index 2525501525..60ea95ac65 100644 --- a/services/hackbot-pulse-listener/app/config.py +++ b/services/hackbot-pulse-listener/app/config.py @@ -17,6 +17,7 @@ class Settings(BaseSettings): firefox_git_url: str = "https://github.com/mozilla-firefox/firefox" firefox_hg_url: str = "https://hg.mozilla.org/mozilla-unified" bugzilla_url: str = "https://bugzilla.mozilla.org" + treeherder_url: str = "https://treeherder.mozilla.org" # Failure filtering and agent inputs. # ``watched_repos`` is a comma-separated list of Taskcluster ``project`` tags. diff --git a/services/hackbot-pulse-listener/app/consumer.py b/services/hackbot-pulse-listener/app/consumer.py index 7a53471438..a37865b86f 100644 --- a/services/hackbot-pulse-listener/app/consumer.py +++ b/services/hackbot-pulse-listener/app/consumer.py @@ -82,7 +82,6 @@ def process(body: dict, executor: Executor) -> str | None: return None inputs: dict = { - "git_commit": git_commit, "failure_tasks": {task_name: task_id}, "run_try_push": settings.run_try_push, } diff --git a/services/hackbot-pulse-listener/app/github.py b/services/hackbot-pulse-listener/app/github.py new file mode 100644 index 0000000000..23b8efeedf --- /dev/null +++ b/services/hackbot-pulse-listener/app/github.py @@ -0,0 +1,32 @@ +import logging + +import httpx + +from app.config import settings + +logger = logging.getLogger(__name__) + +_TIMEOUT = httpx.Timeout(30.0) + + +def _repo_slug() -> str: + """``owner/repo`` parsed from the configured firefox git url.""" + return settings.firefox_git_url.rstrip("/").removeprefix("https://github.com/") + + +def commit_author_email(git_commit: str) -> str | None: + """Author email of a firefox git commit, or None. + + The build-repair agent returns the commit it blamed for the failure; we look + that commit up in the firefox GitHub mirror to notify its author directly. + """ + url = f"https://api.github.com/repos/{_repo_slug()}/commits/{git_commit}" + headers = {"Accept": "application/vnd.github+json"} + try: + resp = httpx.get(url, headers=headers, timeout=_TIMEOUT) + resp.raise_for_status() + author = (resp.json().get("commit") or {}).get("author") or {} + except (httpx.HTTPError, ValueError) as exc: + logger.warning("Failed to fetch author for commit %s: %s", git_commit, exc) + return None + return author.get("email") or None diff --git a/services/hackbot-pulse-listener/app/notify.py b/services/hackbot-pulse-listener/app/notify.py index 9769878616..2e2f8aab6f 100644 --- a/services/hackbot-pulse-listener/app/notify.py +++ b/services/hackbot-pulse-listener/app/notify.py @@ -2,7 +2,7 @@ import logging import re -from app import client +from app import client, github from app.config import settings from app.models import RunContext @@ -26,7 +26,10 @@ def send_email(ctx: RunContext, run_doc: dict) -> None: logger.info("Run %s produced no patch; skipping notification", ctx.run_id) return - recipients = _recipients(ctx.developer_email) + findings = (run_doc.get("summary") or {}).get("findings") or {} + blamed_commit = findings.get("blamed_commit") + blamed_author = github.commit_author_email(blamed_commit) if blamed_commit else None + recipients = _recipients(blamed_author, ctx.developer_email) if not recipients: logger.info("No recipients for run %s; skipping notification", ctx.run_id) return @@ -47,6 +50,7 @@ def send_email(ctx: RunContext, run_doc: dict) -> None: From, HtmlContent, Mail, + ReplyTo, Subject, To, ) @@ -55,7 +59,7 @@ def send_email(ctx: RunContext, run_doc: dict) -> None: f"[build-repair] Build failure analysis for {ctx.repo}@{ctx.git_commit[:12]}" ) - body_md = _build_body(ctx, run_doc, patch) + body_md = _build_body(ctx, run_doc, patch, blamed_author) html = markdown2.markdown(body_md, extras=["fenced-code-blocks", "tables"]) sg = sendgrid.SendGridAPIClient(api_key=settings.sendgrid_api_key) @@ -74,6 +78,8 @@ def send_email(ctx: RunContext, run_doc: dict) -> None: FileType("text/x-patch"), Disposition("attachment"), ) + if settings.notification_team_email: + message.reply_to = ReplyTo(settings.notification_team_email) response = sg.send(message=message) logger.info( "Sent build-repair notification to %s (status %s)", @@ -82,16 +88,20 @@ def send_email(ctx: RunContext, run_doc: dict) -> None: ) -def _recipients(developer_email: str | None) -> list[str]: - """Recipients for a run: the revision author plus the team address, deduped. +def _recipients( + author_email: str | None, developer_email: str | None = None +) -> list[str]: + """Recipients for a run, deduped and ordered by priority. + The blamed commit's author is the primary To (the person who introduced the + failure), then the pushing developer, then the team address. ``notification_override_email`` short-circuits to a single address so local testing never mails real developers or the team. """ if settings.notification_override_email: return [settings.notification_override_email] recipients: list[str] = [] - for addr in (developer_email, settings.notification_team_email): + for addr in (author_email, developer_email, settings.notification_team_email): if addr and addr not in recipients: recipients.append(addr) return recipients @@ -121,13 +131,26 @@ def _task_url(task_id: str) -> str: return f"{settings.taskcluster_root_url.rstrip('/')}/tasks/{task_id}" +def _treeherder_url(repo: str, hg_revision: str, task_id: str) -> str: + return ( + f"{settings.treeherder_url.rstrip('/')}/#/jobs" + f"?repo={repo}&revision={hg_revision}&selectedTaskRun={task_id}" + ) + + def _bug_url(bug_id: object) -> str: return f"{settings.bugzilla_url.rstrip('/')}/show_bug.cgi?id={bug_id}" -def _build_body(ctx: RunContext, run_doc: dict, patch: str | None = None) -> str: +def _build_body( + ctx: RunContext, + run_doc: dict, + patch: str | None = None, + blamed_author: str | None = None, +) -> str: summary = run_doc.get("summary") or {} findings = summary.get("findings") or {} + blamed_commit = findings.get("blamed_commit") lines = [ "# Build failure analysis", @@ -136,8 +159,17 @@ def _build_body(ctx: RunContext, run_doc: dict, patch: str | None = None) -> str f"- **Revision (git):** [`{ctx.git_commit[:12]}`]({_git_url(ctx.git_commit)})", f"- **Revision (hg):** [`{ctx.hg_revision[:12]}`]({_hg_url(ctx.hg_revision)})", f"- **Failed task:** [`{ctx.task_id}`]({_task_url(ctx.task_id)})", + f"- **Treeherder:** " + f"[jobs]({_treeherder_url(ctx.repo, ctx.hg_revision, ctx.task_id)})", ] + if blamed_commit: + by = f" by {blamed_author}" if blamed_author else "" + lines.append( + f"- **Likely culprit:** " + f"[`{blamed_commit[:12]}`]({_git_url(blamed_commit)}){by}" + ) + bug_id = findings.get("bug_id") or (run_doc.get("inputs") or {}).get("bug_id") if bug_id: lines.append(f"- **Bug:** [{bug_id}]({_bug_url(bug_id)})") @@ -146,6 +178,8 @@ def _build_body(ctx: RunContext, run_doc: dict, patch: str | None = None) -> str run_url = f"{settings.hackbot_ui_url.rstrip('/')}/runs/{ctx.run_id}" lines.append(f"- **Run details:** {run_url}") + lines += _recipients_note(ctx, blamed_commit, blamed_author) + if findings.get("summary"): lines += ["", "## Summary", "", _demote_headings(findings["summary"])] if findings.get("analysis"): @@ -162,9 +196,49 @@ def _build_body(ctx: RunContext, run_doc: dict, patch: str | None = None) -> str if patch: lines += ["", "## Proposed patch", "", _patch_block(patch)] + if settings.notification_team_email: + lines += [ + "", + "---", + "", + "_Reply to this email with any feedback on this analysis; it reaches " + "the hackbot team._", + ] + return "\n".join(lines) +def _recipients_note( + ctx: RunContext, blamed_commit: str | None, blamed_author: str | None +) -> list[str]: + """Explain why each recipient is on the email. + + The notification goes to the developer who pushed the failing change, the + author the agent blamed for the failure, and the team; spell out both roles + so the recipient list is self-explanatory. + """ + notes: list[str] = [] + if ctx.developer_email: + notes.append( + f"- **{ctx.developer_email}** pushed the change whose build failed." + ) + if blamed_commit and blamed_author: + notes.append( + f"- **{blamed_author}** authored " + f"[`{blamed_commit[:12]}`]({_git_url(blamed_commit)}), which the " + "build-repair agent believes introduced the failure." + ) + elif blamed_commit: + notes.append( + f"- The build-repair agent believes " + f"[`{blamed_commit[:12]}`]({_git_url(blamed_commit)}) introduced the " + "failure." + ) + if not notes: + return [] + return ["", "## Why you're receiving this", "", *notes] + + def _demote_headings(md: str, by: int = 2) -> str: """Shift ATX headings down ``by`` levels so agent docs nest under our own. diff --git a/services/hackbot-pulse-listener/scripts/send_test_run.py b/services/hackbot-pulse-listener/scripts/send_test_run.py new file mode 100644 index 0000000000..56cc14e836 --- /dev/null +++ b/services/hackbot-pulse-listener/scripts/send_test_run.py @@ -0,0 +1,90 @@ +"""Trigger a build-repair run for a real failing build task and email the result. + +Drives the listener's normal path (trigger the agent via hackbot-api, poll the +run to completion, send the notification) from a synthetic pulse message, so you +can test on a real failure without waiting for a live one. + +Credentials and settings are read from the environment / ``.env`` (see the +service README): HACKBOT_API_URL, HACKBOT_API_KEY, SENDGRID_API_KEY, +NOTIFICATION_SENDER, and NOTIFICATION_OVERRIDE_EMAIL. Always set +NOTIFICATION_OVERRIDE_EMAIL to your own address so the run emails you and not the +real developer; the script refuses to run otherwise. + +Usage (from the service directory, with the env exported): + + uv run --package hackbot-pulse-listener python scripts/send_test_run.py \ + --label build-linux64/opt [--project autoland] [--force] + +Find a task id on Treeherder: a red build ("B") job -> Task inspector -> taskId. +""" + +import argparse +import sys +from concurrent.futures import ThreadPoolExecutor + +from app import consumer +from app.config import settings + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument("task_id", help="Taskcluster task id of a failed build job") + parser.add_argument( + "--label", + default="build-linux64/opt", + help="Build task label; must contain 'build' and not 'test'", + ) + parser.add_argument("--project", default="autoland", help="Taskcluster project tag") + parser.add_argument( + "--created-for", default="", help="createdForUser: the pushing developer email" + ) + parser.add_argument( + "--force", + action="store_true", + help="Skip the regression gate so a run always triggers", + ) + args = parser.parse_args() + + if not settings.notification_override_email: + parser.error( + "Set NOTIFICATION_OVERRIDE_EMAIL to your address so the test emails you, " + "not the real developer." + ) + + if args.force: + consumer.regression.is_new_build_failure = lambda *a, **k: True + + if args.project not in settings.watched_repos_set: + settings.watched_repos = f"{settings.watched_repos},{args.project}" + + msg = { + "status": {"taskId": args.task_id}, + "task": { + "tags": { + "kind": "build", + "project": args.project, + "label": args.label, + "createdForUser": args.created_for, + } + }, + } + + with ThreadPoolExecutor(max_workers=4) as executor: + run_id = consumer.process(msg, executor) + if run_id is None: + print( + "No run triggered (filtered out, deduped, or DRY_RUN). Check " + "WATCHED_REPOS/DRY_RUN, or pass --force to skip the regression gate.", + file=sys.stderr, + ) + return 1 + print( + f"Triggered run {run_id}; polling until it finishes and emailing " + f"{settings.notification_override_email} (this can take several minutes)..." + ) + print("Done.") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/services/hackbot-pulse-listener/tests/test_consumer.py b/services/hackbot-pulse-listener/tests/test_consumer.py index df9662b2a9..cf3a7c647b 100644 --- a/services/hackbot-pulse-listener/tests/test_consumer.py +++ b/services/hackbot-pulse-listener/tests/test_consumer.py @@ -66,8 +66,8 @@ def test_build_failure_triggers_run_and_submits_poll(): assert run_id == "run-1" trigger.assert_called_once() inputs = trigger.call_args.args[0] - assert inputs["git_commit"] == "deadbeef" assert inputs["failure_tasks"] == {"build-linux64/opt": "ABC"} + assert "git_commits" not in inputs executor.submit.assert_called_once() fn, ctx = executor.submit.call_args.args assert fn is consumer.worker.poll_and_notify @@ -79,6 +79,23 @@ def test_build_failure_triggers_run_and_submits_poll(): assert ctx.developer_email == "dev@mozilla.com" +def test_only_failure_tasks_sent_to_agent(): + executor = MagicMock() + with ( + patch.object(consumer.taskcluster, "get_hg_revision", return_value="hgrev"), + patch.object(consumer.lando, "hg_to_git", return_value="deadbeef"), + patch.object(consumer.regression, "is_new_build_failure", return_value=True), + patch.object(consumer.client, "trigger_run", return_value="run-1") as trigger, + ): + consumer.process(_build_msg(), executor) + + # The agent resolves the push (and its authors) itself; the listener + # only hands it the failing tasks. + inputs = trigger.call_args.args[0] + assert inputs["failure_tasks"] == {"build-linux64/opt": "ABC"} + assert "git_commit" not in inputs + + def test_same_revision_triggers_once(): executor = MagicMock() with ( diff --git a/services/hackbot-pulse-listener/tests/test_github.py b/services/hackbot-pulse-listener/tests/test_github.py new file mode 100644 index 0000000000..8b592119da --- /dev/null +++ b/services/hackbot-pulse-listener/tests/test_github.py @@ -0,0 +1,32 @@ +from unittest.mock import MagicMock, patch + +import httpx +from app import github + + +def _resp(payload): + resp = MagicMock() + resp.raise_for_status.return_value = None + resp.json.return_value = payload + return resp + + +def test_repo_slug_from_firefox_git_url(): + assert github._repo_slug() == "mozilla-firefox/firefox" + + +def test_commit_author_email_returns_author(): + payload = {"commit": {"author": {"email": "dev@mozilla.com"}}} + with patch.object(github.httpx, "get", return_value=_resp(payload)) as get: + assert github.commit_author_email("abc123") == "dev@mozilla.com" + assert "mozilla-firefox/firefox/commits/abc123" in get.call_args.args[0] + + +def test_commit_author_email_none_on_http_error(): + with patch.object(github.httpx, "get", side_effect=httpx.HTTPError("boom")): + assert github.commit_author_email("abc123") is None + + +def test_commit_author_email_none_when_missing(): + with patch.object(github.httpx, "get", return_value=_resp({"commit": {}})): + assert github.commit_author_email("abc123") is None diff --git a/services/hackbot-pulse-listener/tests/test_notify.py b/services/hackbot-pulse-listener/tests/test_notify.py index 42f83e6331..db7b1c50ed 100644 --- a/services/hackbot-pulse-listener/tests/test_notify.py +++ b/services/hackbot-pulse-listener/tests/test_notify.py @@ -44,6 +44,97 @@ def test_body_contains_source_links(): assert "https://firefox-ci-tc.services.mozilla.com/tasks/TASK123" in body +def test_body_contains_treeherder_link(): + body = notify._build_body(_ctx(), {"status": "succeeded", "summary": {}}) + assert ( + "https://treeherder.mozilla.org/#/jobs?repo=autoland" + "&revision=0123456789ab&selectedTaskRun=TASK123" in body + ) + + +def test_body_contains_culprit_when_blamed(): + run_doc = { + "status": "succeeded", + "summary": {"findings": {"blamed_commit": "abcdef123456789"}}, + } + body = notify._build_body(_ctx(), run_doc, blamed_author="culprit@mozilla.com") + assert "Likely culprit" in body + assert "https://github.com/mozilla-firefox/firefox/commit/abcdef123456789" in body + assert "by culprit@mozilla.com" in body + + +def test_body_omits_culprit_when_absent(): + body = notify._build_body(_ctx(), {"status": "succeeded", "summary": {}}) + assert "Likely culprit" not in body + + +def test_recipients_blamed_author_is_primary(monkeypatch): + monkeypatch.setattr(notify.settings, "notification_override_email", None) + monkeypatch.setattr(notify.settings, "notification_team_email", "team@mozilla.com") + assert notify._recipients("culprit@mozilla.com", "pusher@mozilla.com") == [ + "culprit@mozilla.com", + "pusher@mozilla.com", + "team@mozilla.com", + ] + + +def test_email_goes_to_blamed_author_first(monkeypatch): + monkeypatch.setattr(notify.settings, "sendgrid_api_key", "key") + monkeypatch.setattr(notify.settings, "notification_sender", "from@mozilla.com") + monkeypatch.setattr(notify.settings, "notification_override_email", None) + monkeypatch.setattr(notify.settings, "notification_team_email", None) + monkeypatch.setattr(notify.settings, "notify_only_with_patch", False) + + run_doc = { + "status": "succeeded", + "summary": {"findings": {"blamed_commit": "cafe1234"}}, + } + fake_client = MagicMock() + fake_client.send.return_value = MagicMock(status_code=202) + with ( + patch("sendgrid.SendGridAPIClient", return_value=fake_client), + patch.object( + notify.github, "commit_author_email", return_value="culprit@mozilla.com" + ) as author, + ): + notify.send_email( + _ctx(developer_email="pusher@mozilla.com"), + run_doc, + ) + + author.assert_called_once_with("cafe1234") + + personalizations = fake_client.send.call_args.kwargs["message"].get()[ + "personalizations" + ][0] + assert personalizations["to"] == [{"email": "culprit@mozilla.com"}] + assert personalizations["cc"] == [{"email": "pusher@mozilla.com"}] + + +def test_email_sets_team_reply_to(monkeypatch): + monkeypatch.setattr(notify.settings, "sendgrid_api_key", "key") + monkeypatch.setattr(notify.settings, "notification_sender", "from@mozilla.com") + monkeypatch.setattr(notify.settings, "notification_override_email", None) + monkeypatch.setattr(notify.settings, "notify_only_with_patch", False) + monkeypatch.setattr( + notify.settings, "notification_team_email", "hackbot-developers@mozilla.com" + ) + + run_doc = {"status": "succeeded", "summary": {"findings": {}}} + fake_client = MagicMock() + fake_client.send.return_value = MagicMock(status_code=202) + with ( + patch("sendgrid.SendGridAPIClient", return_value=fake_client), + patch.object(notify.github, "commit_author_email", return_value=None), + ): + notify.send_email(_ctx(), run_doc) + + message = fake_client.send.call_args.kwargs["message"].get() + assert message["reply_to"] == {"email": "hackbot-developers@mozilla.com"} + body = message["content"][0]["value"] + assert "reaches the hackbot team" in body + + def test_body_contains_bug_link_when_present(): run_doc = {"status": "succeeded", "summary": {"findings": {"bug_id": 12345}}} body = notify._build_body(_ctx(), run_doc)