diff --git a/agents/README.md b/agents/README.md index b53491edae..48e628f63b 100644 --- a/agents/README.md +++ b/agents/README.md @@ -136,6 +136,7 @@ its own extra (`[bugzilla]`, `[firefox]`): ```python from agent_tools import bugzilla from agent_tools.claude_sdk import build_sdk_server + server = build_sdk_server("bugzilla", BugzillaContext(client=...), bugzilla.TOOLS) ``` diff --git a/agents/build-repair/README.md b/agents/build-repair/README.md index 10cbde2928..4fef364fc6 100644 --- a/agents/build-repair/README.md +++ b/agents/build-repair/README.md @@ -35,6 +35,18 @@ Second stage - fixing: The result reports `blamed_commit` so the caller can attribute the failure to a developer. +## Email notification + +A run that produced a patch records an `email.send` action carrying the analysis, the +blamed commit and the patch, addressed to that commit's author and to the developer who +pushed the failing change (the hackbot team is copied apply-side). A run that proposed no +patch is a transient or not-to-blame failure and is not emailed -- see +`NOTIFY_ONLY_WITH_PATCH` in [config.py](hackbot_agents/build_repair/config.py). + +The email is delivered by the apply step, not from the run, so it is visible in the +hackbot UI before it lands and is delivered at most once. `build-repair` opts into +auto-apply, so a succeeded run reports without waiting for a human. + ## Test the agent ```sh diff --git a/agents/build-repair/hackbot_agents/build_repair/__main__.py b/agents/build-repair/hackbot_agents/build_repair/__main__.py index ffa37b7eee..9a50232e4b 100644 --- a/agents/build-repair/hackbot_agents/build_repair/__main__.py +++ b/agents/build-repair/hackbot_agents/build_repair/__main__.py @@ -1,8 +1,15 @@ +import logging + from hackbot_runtime import HackbotContext, run_async +from hackbot_runtime.actions.email import record_email from pydantic_settings import BaseSettings, SettingsConfigDict from .agent import BuildRepairResult, run_build_repair -from .resolve import resolve_push +from .config import NOTIFY_ONLY_WITH_PATCH +from .notify import build_email, recipients, resolve_author_email +from .resolve import PushInfo, resolve_push + +logger = logging.getLogger(__name__) class AgentInputs(BaseSettings): @@ -37,9 +44,9 @@ async def main(ctx: HackbotContext) -> BuildRepairResult: # Pin the checkout to the failure commit and fetch deep enough to include the # whole push, so the agent can `git show` every commit in it. - await ctx.prepare_repo(ref=git_commits[0], depth=len(git_commits) + 1) + await ctx.prepare_repo(ref=push.git_commits[0], depth=len(push.git_commits) + 1) - return await run_build_repair( + result = await run_build_repair( bugzilla_mcp_server={ "type": "http", "url": inputs.bugzilla_mcp_url, @@ -59,6 +66,39 @@ async def main(ctx: HackbotContext) -> BuildRepairResult: publish_file=ctx.publish_file, ) + try: + _record_analysis_email(ctx, result, push, task_id) + except Exception: + # A notification is never worth losing a finished analysis over. + logger.exception("Could not record the failure-analysis email") + return result + + +def _record_analysis_email( + ctx: HackbotContext, result: BuildRepairResult, push: PushInfo, task_id: str +) -> None: + has_patch = ctx.source_changed + if NOTIFY_ONLY_WITH_PATCH and not has_patch: + logger.info("Run produced no patch; not emailing the failure analysis") + return + + blamed_author = resolve_author_email(ctx.repo_path, result.blamed_commit) + subject, body = build_email( + result, + push, + task_id=task_id, + run_id=ctx.run_id, + has_patch=has_patch, + blamed_author=blamed_author, + ) + record_email( + ctx.actions, + to=recipients(push, blamed_author), + subject=subject, + body_markdown=body, + attach_patch=has_patch, + ) + if __name__ == "__main__": run_async(main) diff --git a/agents/build-repair/hackbot_agents/build_repair/config.py b/agents/build-repair/hackbot_agents/build_repair/config.py index b52b13b1ad..e41c865bb3 100644 --- a/agents/build-repair/hackbot_agents/build_repair/config.py +++ b/agents/build-repair/hackbot_agents/build_repair/config.py @@ -8,6 +8,10 @@ ANALYSIS_MODEL = "claude-opus-4-8" FIX_MODEL = "claude-opus-4-8" +# A run that proposed no patch is a transient or not-to-blame failure; emailing the +# developer about it is noise. +NOTIFY_ONLY_WITH_PATCH = True + # Bugzilla MCP tool names as exposed to the agent (mcp____). BUGZILLA_READ_TOOLS = [ "mcp__bugzilla__search_bugs", diff --git a/agents/build-repair/hackbot_agents/build_repair/notify.py b/agents/build-repair/hackbot_agents/build_repair/notify.py new file mode 100644 index 0000000000..5fd63c0446 --- /dev/null +++ b/agents/build-repair/hackbot_agents/build_repair/notify.py @@ -0,0 +1,166 @@ +# -*- coding: utf-8 -*- +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this file, +# You can obtain one at http://mozilla.org/MPL/2.0/. + +"""The email a finished run sends about the build failure. + +Recorded as an ``email.send`` action rather than sent from the run, so it is +visible in the hackbot UI before it lands and is delivered at most once (see +``hackbot_runtime.actions.email``). + +It reaches the developer who pushed the failing change and the author the agent +blamed; the team address is added apply-side. Every identifier a recipient would +otherwise have to look up -- revisions, task, bug, run -- is a link. +""" + +from __future__ import annotations + +import subprocess +from pathlib import Path + +from hackbot_runtime.actions.email import PATCH_PLACEHOLDER, demote_headings +from hackbot_runtime.actions.slack import HACKBOT_UI_URL + +from .agent import BuildRepairResult +from .resolve import PushInfo + +GIT_COMMIT_URL = "https://github.com/mozilla-firefox/firefox/commit/{sha}" +HG_REV_URL = "https://hg.mozilla.org/mozilla-unified/rev/{rev}" +TASK_URL = "https://firefox-ci-tc.services.mozilla.com/tasks/{task_id}" +TREEHERDER_JOB_URL = ( + "https://treeherder.mozilla.org/#/jobs" + "?repo={project}&revision={revision}&selectedTaskRun={task_id}" +) +BUG_URL = "https://bugzilla.mozilla.org/show_bug.cgi?id={bug_id}" +RUN_URL = HACKBOT_UI_URL.rstrip("/") + "/runs/{run_id}" + + +def resolve_author_email(source_repo: Path, sha: str | None) -> str | None: + """The blamed commit's author email, so the notification reaches them.""" + if not sha: + return None + try: + proc = subprocess.run( + ["git", "-C", str(source_repo), "show", "-s", "--format=%ae", sha], + capture_output=True, + text=True, + ) + except OSError: + return None + return proc.stdout.strip() or None + + +def _link(url: str, label: str) -> str: + return f"[{label}]({url})" + + +def recipients(push: PushInfo, blamed_author: str | None) -> list[str]: + """Who the failure concerns: the blamed author first, then the pusher.""" + return [address for address in (blamed_author, push.developer_email) if address] + + +def _why_section( + push: PushInfo, blamed_commit: str | None, author: str | None +) -> list[str]: + """Explain why each recipient is on the email.""" + notes = [] + if push.developer_email: + notes.append( + f"- **{push.developer_email}** pushed the change whose build failed." + ) + if blamed_commit: + who = f"**{author}** authored" if author else "The agent believes" + link = _link( + GIT_COMMIT_URL.format(sha=blamed_commit), f"`{blamed_commit[:12]}`" + ) + notes.append(f"- {who} {link}, which introduced the failure.") + return ["", "## Why you're receiving this", "", *notes] if notes else [] + + +def _analysis_sections(result: BuildRepairResult) -> list[str]: + lines: list[str] = [] + for text, title in ((result.summary, "Summary"), (result.analysis, "Analysis")): + if text: + lines += ["", f"## {title}", "", demote_headings(text)] + return lines + + +def build_email( + result: BuildRepairResult, + push: PushInfo, + *, + task_id: str, + run_id: str, + has_patch: bool = False, + blamed_author: str | None = None, +) -> tuple[str, str]: + """The subject and markdown body of the build-failure email.""" + failure_commit = push.git_commits[0] + subject = ( + f"[build-repair] Build failure analysis for " + f"{push.project}@{failure_commit[:12]}" + ) + + lines = [ + "# Build failure analysis", + "", + f"- **Repository:** {push.project}", + "- **Revision (git):** " + + _link(GIT_COMMIT_URL.format(sha=failure_commit), f"`{failure_commit[:12]}`"), + "- **Failed task:** " + _link(TASK_URL.format(task_id=task_id), f"`{task_id}`"), + ] + # Absent only when the run was pinned to a git commit by hand; both lines are + # keyed on the hg revision, so they go together. + if push.hg_revision: + lines += [ + "- **Revision (hg):** " + + _link( + HG_REV_URL.format(rev=push.hg_revision), f"`{push.hg_revision[:12]}`" + ), + "- **Treeherder:** " + + _link( + TREEHERDER_JOB_URL.format( + project=push.project, revision=push.hg_revision, task_id=task_id + ), + "jobs", + ), + ] + + if result.blamed_commit: + by = f" by {blamed_author}" if blamed_author else "" + lines.append( + "- **Likely culprit:** " + + _link( + GIT_COMMIT_URL.format(sha=result.blamed_commit), + f"`{result.blamed_commit[:12]}`", + ) + + by + ) + else: + lines.append( + "- **Not caused by this push:** the failure is pre-existing or " + "infrastructure, so no commit here is blamed." + ) + if result.bug_id: + lines.append( + "- **Bug:** " + + _link(BUG_URL.format(bug_id=result.bug_id), str(result.bug_id)) + ) + lines.append("- **Run details:** " + RUN_URL.format(run_id=run_id)) + + lines += _why_section(push, result.blamed_commit, blamed_author) + lines += _analysis_sections(result) + + if result.local_build_verified is not None: + lines += [ + "", + "## Verification", + "", + f"- Local build verified: {result.local_build_verified}", + ] + if has_patch: + # The diff itself is substituted for the placeholder when the mail is sent, + # from the same artifact it attaches. + lines += ["", "## Proposed patch", "", "```diff", PATCH_PLACEHOLDER, "```"] + return subject, "\n".join(lines) diff --git a/agents/build-repair/hackbot_agents/build_repair/resolve.py b/agents/build-repair/hackbot_agents/build_repair/resolve.py index 430fc04b6c..8e605fb3d8 100644 --- a/agents/build-repair/hackbot_agents/build_repair/resolve.py +++ b/agents/build-repair/hackbot_agents/build_repair/resolve.py @@ -3,13 +3,13 @@ # License, v. 2.0. If a copy of the MPL was not distributed with this file, # You can obtain one at http://mozilla.org/MPL/2.0/. -"""Resolve a Taskcluster build-failure task into the commits to repair. +"""Resolve a Taskcluster build-failure task into the push to repair. Given a failing build task id, look up its push: the failure (head) commit the tree is checked out at, plus the other commits that landed in the same push so the agent can blame the one that broke the build. Uses the same public Taskcluster / lando / pushlog lookups the pulse listener does, so the agent -derives the commits itself from a task id. +derives everything it reports from a task id. """ from __future__ import annotations @@ -43,6 +43,18 @@ def _get_json(url: str) -> dict: return resp.json() +def _task(task_id: str) -> dict: + return _get_json(_TC_TASK_URL.format(task_id=task_id)) + + +def _task_push(task: dict) -> tuple[str | None, str | None]: + tags = task.get("tags") or {} + return ( + tags.get("project"), + (task.get("payload") or {}).get("env", {}).get("GECKO_HEAD_REV"), + ) + + def _hg_to_git(rev: str) -> str: return _get_json(_LANDO_HG2GIT.format(rev=rev))["git_hash"] @@ -83,16 +95,13 @@ class PushInfo: project: str | None hg_revision: str | None git_commits: list[str] + # ``createdForUser``: who pushed the change that failed to build. + developer_email: str | None = None def task_push(task_id: str) -> tuple[str | None, str | None]: """The ``(project, hg_revision)`` a task ran on; what Treeherder is keyed on.""" - task = _get_json(_TC_TASK_URL.format(task_id=task_id)) - tags = task.get("tags") or {} - return ( - tags.get("project"), - (task.get("payload") or {}).get("env", {}).get("GECKO_HEAD_REV"), - ) + return _task_push(_task(task_id)) def resolve_push(task_id: str, git_commit: str | None = None) -> PushInfo: @@ -102,7 +111,8 @@ def resolve_push(task_id: str, git_commit: str | None = None) -> PushInfo: task is still fetched for its revision. Raises on network errors or when the failure commit cannot be determined. """ - project, hg_rev = task_push(task_id) + task = _task(task_id) + project, hg_rev = _task_push(task) push = _push_git_commits(project, hg_rev) if hg_rev and project else [] @@ -118,4 +128,5 @@ def resolve_push(task_id: str, git_commit: str | None = None) -> PushInfo: project=project, hg_revision=hg_rev, git_commits=[failure_commit] + [c for c in push if c != failure_commit], + developer_email=(task.get("tags") or {}).get("createdForUser"), ) diff --git a/agents/build-repair/pyproject.toml b/agents/build-repair/pyproject.toml index 669aac824b..1a5844a459 100644 --- a/agents/build-repair/pyproject.toml +++ b/agents/build-repair/pyproject.toml @@ -31,3 +31,6 @@ build-backend = "hatchling.build" [tool.hatch.build.targets.wheel] packages = ["hackbot_agents", "evals"] + +[tool.pytest.ini_options] +testpaths = ["tests"] diff --git a/agents/build-repair/tests/__init__.py b/agents/build-repair/tests/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/agents/build-repair/tests/test_notify.py b/agents/build-repair/tests/test_notify.py new file mode 100644 index 0000000000..fd18b37cb4 --- /dev/null +++ b/agents/build-repair/tests/test_notify.py @@ -0,0 +1,123 @@ +from hackbot_agents.build_repair.agent import BuildRepairResult +from hackbot_agents.build_repair.notify import build_email, recipients +from hackbot_agents.build_repair.resolve import PushInfo + +HG_REVISION = "341517e50536aabbccddeeff00112233445566" +GIT_REVISION = "7b15e34863cf6b30b613ffadf9d6431fe5a55585" +CULPRIT = "c338a2c1c8d3695b7dec835125af624282555b7e" +TASK_ID = "JfAGrrtoQPS3fXrwZmq1Pg" + + +def _push(developer_email="dev@mozilla.com"): + return PushInfo( + project="autoland", + hg_revision=HG_REVISION, + git_commits=[GIT_REVISION, CULPRIT], + developer_email=developer_email, + ) + + +def _result(**overrides): + fields = { + "git_commit": GIT_REVISION, + "blamed_commit": CULPRIT, + "summary": "The build broke on a missing include.", + "num_turns": 8, + } + return BuildRepairResult(**{**fields, **overrides}) + + +def _email(result=None, push=None, **kwargs): + return build_email( + result or _result(), + push or _push(), + task_id=TASK_ID, + run_id="1218e630-78c8", + **kwargs, + ) + + +def test_the_blamed_author_comes_before_the_pusher(): + assert recipients(_push(), "author@mozilla.com") == [ + "author@mozilla.com", + "dev@mozilla.com", + ] + + +def test_an_unknown_author_leaves_only_the_pusher(): + assert recipients(_push(), None) == ["dev@mozilla.com"] + + +def test_a_push_with_no_known_pusher_reaches_nobody_individually(): + # The handler still addresses the team. + assert recipients(_push(developer_email=None), None) == [] + + +def test_the_subject_names_the_repository_and_the_failure_commit(): + subject, _ = _email() + assert ( + subject + == f"[build-repair] Build failure analysis for autoland@{GIT_REVISION[:12]}" + ) + + +def test_the_email_links_every_identifier(): + _, body = _email() + assert ( + f"[`{GIT_REVISION[:12]}`]" + f"(https://github.com/mozilla-firefox/firefox/commit/{GIT_REVISION})" in body + ) + assert ( + f"[`{HG_REVISION[:12]}`]" + f"(https://hg.mozilla.org/mozilla-unified/rev/{HG_REVISION})" in body + ) + assert ( + f"[`{TASK_ID}`](https://firefox-ci-tc.services.mozilla.com/tasks/{TASK_ID})" + in body + ) + assert "https://hackbot.moz.tools/runs/1218e630-78c8" in body + + +def test_the_culprit_and_its_author_are_named(): + _, body = _email(blamed_author="author@mozilla.com") + assert f"**Likely culprit:** [`{CULPRIT[:12]}`]" in body + assert "by author@mozilla.com" in body + assert "**author@mozilla.com** authored" in body + + +def test_a_push_the_agent_cleared_says_so(): + _, body = _email(result=_result(blamed_commit=None)) + assert "Not caused by this push" in body + assert "Likely culprit" not in body + + +def test_the_pusher_is_told_why_they_are_on_the_email(): + _, body = _email() + assert "**dev@mozilla.com** pushed the change whose build failed." in body + + +def test_agent_prose_nests_under_the_email_headings(): + _, body = _email(result=_result(analysis="# Root cause\n\ndetail")) + assert "## Analysis" in body + assert "### Root cause" in body + + +def test_the_local_build_verification_is_reported_when_known(): + _, body = _email(result=_result(local_build_verified=True)) + assert "- Local build verified: True" in body + + +def test_no_verification_section_without_a_verdict(): + _, body = _email() + assert "## Verification" not in body + + +def test_the_patch_section_frames_a_placeholder_the_apply_step_fills(): + _, body = _email(has_patch=True) + assert body.endswith("## Proposed patch\n\n```diff\n{patch}\n```") + + +def test_no_patch_section_without_a_patch(): + _, body = _email() + assert "Proposed patch" not in body + assert "{patch}" not in body diff --git a/agents/test-repair/README.md b/agents/test-repair/README.md index 926cba1349..3ff9f4749a 100644 --- a/agents/test-repair/README.md +++ b/agents/test-repair/README.md @@ -63,7 +63,7 @@ Stage 2: - A patch in Hackbot format -## Slack notification +## Notifications A run whose verdict a sheriff has to act on records a `slack.post_message` action carrying it -- the recommendation, the classification and confidence, the failing job @@ -71,12 +71,17 @@ carrying it -- the recommendation, the classification and confidence, the failin ruled out, and whether a patch is attached. A known intermittent -- `intermittent` classified `do_not_backout` -- is not posted: it asks nothing of a sheriff and is the majority verdict, so it would be noise. An intermittent recommending `rerun` is still -posted, since the retrigger is the sheriff's to run. The hackbot team gets every -verdict either way, by email from the pulse listener. +posted, since the retrigger is the sheriff's to run. -The message is posted by the apply step, not from the run, so it is visible in the -hackbot UI before it lands and is delivered at most once. `test-repair` opts into -auto-apply, so a succeeded run posts without waiting for a human. +Every verdict also records an `email.send` action carrying the full analysis and the +proposed patch, for the hackbot team to track what the agent decided -- unfiltered, and +never addressed to the developer the agent happens to blame. Treeherder is re-read just +before it is recorded, so a failure a sheriff dealt with while the run worked says so in +the subject. + +Both are delivered by the apply step, not from the run, so they are visible in the +hackbot UI before they land and are delivered at most once. `test-repair` opts into +auto-apply, so a succeeded run reports without waiting for a human. ## Test the agent diff --git a/agents/test-repair/hackbot_agents/test_repair/__main__.py b/agents/test-repair/hackbot_agents/test_repair/__main__.py index 44bfd5bb58..bbb9525afe 100644 --- a/agents/test-repair/hackbot_agents/test_repair/__main__.py +++ b/agents/test-repair/hackbot_agents/test_repair/__main__.py @@ -3,13 +3,19 @@ from pathlib import Path from hackbot_runtime import HackbotContext, run_async +from hackbot_runtime.actions.email import record_email from hackbot_runtime.actions.slack import record_message from pydantic_settings import BaseSettings, SettingsConfigDict from .agent import TestRepairResult from .config import SKIP_FIREFOX_BUILD, SLACK_CHANNEL -from .notify import build_message, resolve_culprit_author, sheriff_action_required -from .resolve import Investigation, resolve_investigation +from .notify import ( + build_email, + build_message, + resolve_culprit_author, + sheriff_action_required, +) +from .resolve import Investigation, resolve_investigation, sheriff_classification logger = logging.getLogger(__name__) @@ -70,21 +76,56 @@ async def main(ctx: HackbotContext) -> TestRepairResult: publish_file=ctx.publish_file, ) + culprit_author = resolve_culprit_author(source_repo, result.culprit_commit) if sheriff_action_required(result): message = build_message( result, investigation, task_id=task_id, run_id=ctx.run_id, - culprit_author=resolve_culprit_author(source_repo, result.culprit_commit), + culprit_author=culprit_author, ) record_message(ctx.actions, SLACK_CHANNEL, message) else: logger.info( "Verdict is %s; not notifying %s", result.classification, SLACK_CHANNEL ) + + try: + _record_verdict_email(ctx, result, investigation, task_id, culprit_author) + except Exception: + # A notification is never worth losing a finished analysis over. + logger.exception("Could not record the verdict email") return result +def _record_verdict_email( + ctx: HackbotContext, + result: TestRepairResult, + investigation: Investigation, + task_id: str, + culprit_author: str | None, +) -> None: + """Email every verdict to the team, actionable or not. + + Unlike the Slack message this is not filtered: the team tracks what the agent + decided, including the intermittents no sheriff has to act on. + """ + subject, body = build_email( + result, + investigation, + task_id=task_id, + run_id=ctx.run_id, + culprit_author=culprit_author, + already_actioned=sheriff_classification(investigation.project, task_id), + ) + record_email( + ctx.actions, + subject=subject, + body_markdown=body, + attach_patch=ctx.source_changed, + ) + + if __name__ == "__main__": run_async(main) diff --git a/agents/test-repair/hackbot_agents/test_repair/notify.py b/agents/test-repair/hackbot_agents/test_repair/notify.py index 82e89b5ed2..0f8e11b6e9 100644 --- a/agents/test-repair/hackbot_agents/test_repair/notify.py +++ b/agents/test-repair/hackbot_agents/test_repair/notify.py @@ -3,20 +3,19 @@ # License, v. 2.0. If a copy of the MPL was not distributed with this file, # You can obtain one at http://mozilla.org/MPL/2.0/. -"""The Slack message a finished run sends to the channel. +"""What a finished run reports, in Slack and by email. -Recorded as a ``slack.post_message`` action rather than posted from the run: it is -then visible in the hackbot UI before it lands, and the apply step delivers it at -most once (see ``hackbot_runtime.actions.slack``). +Both are recorded as actions rather than sent from the run: they are then visible +in the hackbot UI before they land, and the apply step delivers each at most once +(see ``hackbot_runtime.actions.slack`` / ``.email``). -Only verdicts a sheriff acts on are posted -- see :func:`sheriff_action_required`. +Only verdicts a sheriff acts on go to the channel -- see +:func:`sheriff_action_required`. Every verdict is emailed, so the team can track +what the agent decided either way. -A few lines of context, then the verdict in full. Every identifier a sheriff would -otherwise have to look up -- revisions, task, bug, run -- is a link, the way the -pulse listener's email does it -(``services/hackbot-pulse-listener/app/notify.py``); unlike the email this stays -short enough to read in a channel, since the run holds the detail. The verdict is -what a sheriff acts on, so it is never truncated. +Every identifier a recipient would otherwise have to look up -- revisions, task, +bug, run -- is a link. The Slack message stays short enough to read in a channel, +since the run holds the detail; the email carries the full analysis. """ from __future__ import annotations @@ -24,6 +23,7 @@ import subprocess from pathlib import Path +from hackbot_runtime.actions.email import PATCH_PLACEHOLDER, demote_headings from hackbot_runtime.actions.slack import HACKBOT_UI_URL from .agent import TestRepairResult @@ -168,3 +168,121 @@ def build_message( if result.summary.strip(): lines += ["", result.summary.strip()] return "\n".join(lines) + + +def _md_link(url: str, label: str) -> str: + return f"[{label}]({url})" + + +def _groups_label(investigation: Investigation) -> str: + """A one-line name for the run's failing groups, for the email subject.""" + if not investigation.failing_groups: + return investigation.label or "unresolved tests" + first, *rest = [group.group for group in investigation.failing_groups] + return f"{first} (+{len(rest)} more)" if rest else first + + +def _already_actioned_banner(classification: str | None) -> list[str]: + """Say up front that the tree has been dealt with, when it has.""" + if not classification: + return [] + return [ + f"> **Already actioned by a sheriff.** Treeherder now classifies this job as " + f"_{classification}_, so the tree has been dealt with.", + "", + ] + + +def _analysis_sections(result: TestRepairResult) -> list[str]: + lines: list[str] = [] + for text, title in ((result.summary, "Summary"), (result.analysis, "Analysis")): + if text: + lines += ["", f"## {title}", "", demote_headings(text)] + return lines + + +def build_email( + result: TestRepairResult, + investigation: Investigation, + *, + task_id: str, + run_id: str, + culprit_author: str | None = None, + already_actioned: str | None = None, +) -> tuple[str, str]: + """The subject and markdown body of the verdict email.""" + recommendation = _RECOMMENDATIONS.get(result.recommendation, result.recommendation) + # In the subject too, so it can be skipped from the inbox. + prefix = "[already actioned] " if already_actioned else "" + subject = ( + f"[test-repair] {prefix}{recommendation} - " + f"{_groups_label(investigation)} ({investigation.project})" + ) + + groups = ( + ", ".join(f"`{group.group}`" for group in investigation.failing_groups) + or "not resolved" + ) + lines = [ + *_already_actioned_banner(already_actioned), + "# Test failure analysis", + "", + f"- **Recommendation:** {recommendation}", + f"- **Failing tests:** {groups}", + f"- **Classification:** {result.classification}", + f"- **Confidence:** {result.confidence}", + f"- **Repository:** {investigation.project}", + "- **Revision (git):** " + + _md_link( + GIT_COMMIT_URL.format(sha=investigation.failure_commit), + f"`{investigation.failure_commit[:12]}`", + ), + "- **Revision (hg):** " + + _md_link( + HG_REV_URL.format(rev=investigation.hg_revision), + f"`{investigation.hg_revision[:12]}`", + ), + "- **Failed task:** " + + _md_link(TASK_URL.format(task_id=task_id), f"`{task_id}`"), + "- **Treeherder:** " + + _md_link( + TREEHERDER_JOB_URL.format( + project=investigation.project, + revision=investigation.hg_revision, + task_id=task_id, + ), + "jobs", + ), + ] + + if result.culprit_commit: + by = f" by {culprit_author}" if culprit_author else "" + lines.append( + "- **Culprit commit:** " + + _md_link( + GIT_COMMIT_URL.format(sha=result.culprit_commit), + f"`{result.culprit_commit[:12]}`", + ) + + by + ) + bug = result.culprit_bug or result.intermittent_bug + if bug: + lines.append("- **Bug:** " + _md_link(BUG_URL.format(bug_id=bug), str(bug))) + lines.append("- **Run details:** " + RUN_URL.format(run_id=run_id)) + + lines += _analysis_sections(result) + if result.proposed_patch: + # The diff itself is substituted for the placeholder when the mail is sent, + # from the same artifact it attaches. + lines += [ + "", + "## Proposed patch", + "", + "```diff", + PATCH_PLACEHOLDER, + "```", + "", + "_For the author: squash this into your existing patches and reland. It" + " is a suggestion, not a follow-up to land on its own._", + ] + return subject, "\n".join(lines) diff --git a/agents/test-repair/hackbot_agents/test_repair/resolve.py b/agents/test-repair/hackbot_agents/test_repair/resolve.py index 8ae7effcf8..936c96d174 100644 --- a/agents/test-repair/hackbot_agents/test_repair/resolve.py +++ b/agents/test-repair/hackbot_agents/test_repair/resolve.py @@ -148,6 +148,39 @@ def _failing_groups(task_id: str) -> list[FailingGroup]: return groups +# Treeherder /api/failureclassification/, restricted to the verdicts that mean a +# sheriff has already dealt with the failure. "not classified" (1) and "new failure +# not classified" (6) are left out: they still may be a real regression. +_ACTIONED_CLASSIFICATIONS = { + 2: "fixed by commit", + 3: "expected fail", + 4: "intermittent", + 5: "infra", + 7: "autoclassified intermittent", + 8: "intermittent needs bugid", +} + + +def sheriff_classification(project: str, task_id: str) -> str | None: + """How a sheriff classified this failure while the run worked, if they did. + + A run takes long enough that the tree is often dealt with before it reports. + Best effort: None on any error, so the report goes out unmarked rather than + not at all. + """ + try: + jobs = ( + _get_json(f"{_TREEHERDER}/{project}/jobs/?task_id={task_id}").get("results") + or [] + ) + except (requests.exceptions.RequestException, ValueError): + logger.warning("Could not re-read the classification of task %s", task_id) + return None + if not jobs: + return None + return _ACTIONED_CLASSIFICATIONS.get(jobs[0].get("failure_classification_id")) + + def _open_intermittent_bugs(suggestion: dict) -> list[int]: """Ids of the unresolved intermittent-failure bugs a failure line matches.""" bugs = suggestion.get("bugs") or {} diff --git a/agents/test-repair/tests/test_notify.py b/agents/test-repair/tests/test_notify.py index cdae9a5c21..699d4cfde5 100644 --- a/agents/test-repair/tests/test_notify.py +++ b/agents/test-repair/tests/test_notify.py @@ -1,5 +1,9 @@ from hackbot_agents.test_repair.agent import TestRepairResult -from hackbot_agents.test_repair.notify import build_message, sheriff_action_required +from hackbot_agents.test_repair.notify import ( + build_email, + build_message, + sheriff_action_required, +) from hackbot_agents.test_repair.resolve import ( CommitRange, FailingGroup, @@ -174,3 +178,88 @@ def test_lists_every_failing_group(): def test_falls_back_when_groups_and_label_are_unknown(): message = _message(investigation=_investigation(groups=[], label="")) assert "Failing: tests not resolved in `xpcshell on linux1804-64/opt`" in message + + +def _email(result=None, investigation=None, **kwargs): + return build_email( + result or _result(), + investigation or _investigation(), + task_id=TASK_ID, + run_id="1218e630-78c8", + **kwargs, + ) + + +def test_the_email_subject_names_the_verdict_and_the_failing_group(): + subject, _ = _email() + assert subject == ( + "[test-repair] BACK OUT the culprit - " + "toolkit/modules/tests/xpcshell/xpcshell.toml (autoland)" + ) + + +def test_extra_failing_groups_are_counted_in_the_subject(): + subject, _ = _email( + investigation=_investigation( + groups=[FailingGroup("a.toml", ["a.js"]), FailingGroup("b.toml", ["b.js"])] + ) + ) + assert subject.startswith("[test-repair] BACK OUT the culprit - a.toml (+1 more)") + + +def test_an_already_actioned_failure_is_flagged_in_subject_and_body(): + subject, body = _email(already_actioned="fixed by commit") + assert subject.startswith("[test-repair] [already actioned] ") + assert "Already actioned by a sheriff" in body + assert "_fixed by commit_" in body + + +def test_the_email_links_every_identifier(): + _, body = _email() + assert f"[`{GIT_REVISION[:12]}`]({GIT_URL})" in body + assert f"[`{HG_REVISION[:12]}`]({HG_URL})" in body + assert ( + f"[`{TASK_ID}`](https://firefox-ci-tc.services.mozilla.com/tasks/{TASK_ID})" + in body + ) + assert "https://hackbot.moz.tools/runs/1218e630-78c8" in body + + +def test_the_culprit_author_is_named_when_known(): + _, body = _email(culprit_author="author@mozilla.com") + assert "by author@mozilla.com" in body + + +def test_agent_prose_nests_under_the_email_headings(): + _, body = _email(result=_result(analysis="# Root cause\n\ndetail")) + assert "## Analysis" in body + assert "### Root cause" in body + + +def test_the_patch_section_frames_a_placeholder_the_apply_step_fills(): + _, body = _email(result=_result(proposed_patch=True)) + assert "## Proposed patch\n\n```diff\n{patch}\n```" in body + + +def test_the_author_advice_follows_the_patch(): + _, body = _email(result=_result(proposed_patch=True)) + assert body.index("{patch}") < body.index("squash this into your existing") + + +def test_no_patch_section_without_a_patch(): + _, body = _email() + assert "Proposed patch" not in body + assert "{patch}" not in body + + +def test_an_intermittent_verdict_is_still_emailed(): + # Unlike Slack, the email is not filtered by sheriff_action_required. + subject, body = _email( + result=_result( + classification="intermittent", + recommendation="do_not_backout", + culprit_commit=None, + ) + ) + assert "DO NOT back out (intermittent)" in subject + assert "**Classification:** intermittent" in body diff --git a/agents/test-repair/tests/test_resolve.py b/agents/test-repair/tests/test_resolve.py index 204a2b3cc5..cb9280e458 100644 --- a/agents/test-repair/tests/test_resolve.py +++ b/agents/test-repair/tests/test_resolve.py @@ -207,3 +207,29 @@ def boom(url): monkeypatch.setattr(resolve, "_get_json", boom) assert resolve._known_intermittent_bugs("autoland", "TASK") == [] + + +def test_sheriff_classification_names_the_verdict(monkeypatch): + monkeypatch.setattr( + resolve, + "_get_json", + lambda url: {"results": [{"failure_classification_id": 2}]}, + ) + assert resolve.sheriff_classification("autoland", "TASK") == "fixed by commit" + + +def test_an_unclassified_job_was_not_actioned(monkeypatch): + monkeypatch.setattr( + resolve, + "_get_json", + lambda url: {"results": [{"failure_classification_id": 6}]}, + ) + assert resolve.sheriff_classification("autoland", "TASK") is None + + +def test_sheriff_classification_survives_a_treeherder_error(monkeypatch): + def boom(url): + raise resolve.requests.exceptions.RequestException("down") + + monkeypatch.setattr(resolve, "_get_json", boom) + assert resolve.sheriff_classification("autoland", "TASK") is None diff --git a/docs/hackbot/actions.md b/docs/hackbot/actions.md index 934bd1e452..694d08ec05 100644 --- a/docs/hackbot/actions.md +++ b/docs/hackbot/actions.md @@ -1,8 +1,8 @@ # Actions: record now, apply later -An agent never mutates Bugzilla, Phabricator, TestRail or Slack while it runs. It calls a -tool that **records what it intends to do**; hackbot-api performs it after the run has -finished and is known good. +An agent never mutates Bugzilla, Phabricator, TestRail or Slack, and never sends mail, while +it runs. It calls a tool that **records what it intends to do**; hackbot-api performs it +after the run has finished and is known good. Why the indirection: @@ -45,6 +45,7 @@ triage run but swaps it for `phabricator.update_patch` on a follow-up. | `phabricator.add_comment` | Reply on a revision without changing code | `revision_id`, `text` | | `testrail.submit_test_plan` | Submit a generated test plan to TestRail | the validated feature + test cases | | `slack.post_message` | Post a message to Slack | `channel`, `text` | +| `email.send` | Email a report about the run | `to`, `subject`, `body_markdown` | All but `testrail.submit_test_plan` take a **`reasoning`** argument — a free-text audit trail stored on the action and shown in the UI beside the proposed change. `phabricator.submit_patch` @@ -52,7 +53,8 @@ is the only model-facing tool that exposes **`ref`** (see cross-references below `testrail` and `slack` also provide `record_test_plan` / `record_message` helpers that agent code calls directly rather than the model choosing to — for an action the agent always takes -once it has a result, not one the model decides on. +once it has a result, not one the model decides on. `email` provides `record_email` +alongside its tool for the same reason. `bugzilla.add_comment` appends a feedback-reaction footer to every recorded comment, and `is_private=true` marks it security-group-only. diff --git a/docs/hackbot/triggers.md b/docs/hackbot/triggers.md index 543d5a2864..2349b6bd8d 100644 --- a/docs/hackbot/triggers.md +++ b/docs/hackbot/triggers.md @@ -47,8 +47,9 @@ the trigger form and the run filter. An always-on Cloud Run **worker pool** (no HTTP port) that consumes `task-failed` messages from `pulse.mozilla.org`, decides which failures are worth an agent, and dispatches -`build-repair` (failed build tasks) or `test-repair` (failed test tasks). When the run -finishes it polls the result and emails a report. +`build-repair` (failed build tasks) or `test-repair` (failed test tasks). Dispatch is where +its involvement ends: the agent reports its own result, as an `email.send` action (and, for +test-repair, a Slack message) applied once the run has succeeded. **It holds no investigation logic.** Each agent resolves the push, the commit range and the failing tests itself from the task id. The listener only decides _what to hand off_ — which diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/__init__.py b/libs/hackbot-runtime/hackbot_runtime/actions/__init__.py index 9f9a04f787..6342a0125f 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/__init__.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/__init__.py @@ -7,7 +7,14 @@ claude-sdk adapter is ``hackbot_runtime.actions.claude_sdk.actions_server_for``. """ -from hackbot_runtime.actions import bugzilla, phabricator, slack, testrail, try_server +from hackbot_runtime.actions import ( + bugzilla, + email, + phabricator, + slack, + testrail, + try_server, +) from hackbot_runtime.actions.recorder import ActionHook, ActionsRecorder ACTIONS_SERVER_NAME = "actions" @@ -17,6 +24,7 @@ "ActionHook", "ActionsRecorder", "bugzilla", + "email", "phabricator", "slack", "testrail", diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/email.py b/libs/hackbot-runtime/hackbot_runtime/actions/email.py new file mode 100644 index 0000000000..3f92ecd8b7 --- /dev/null +++ b/libs/hackbot-runtime/hackbot_runtime/actions/email.py @@ -0,0 +1,156 @@ +"""Email-domain recordable action. + +An agent -- or the deterministic code around it, via :func:`record_email` -- +records a report it wants delivered by email; the apply side sends it with +SendGrid (see ``handlers/email_handler.py``). + +Recording rather than sending gives a notification the same properties as every +other action -- visible in the UI before it lands, delivered at most once, and +never sent at all for a run that did not succeed. + +The team address, the sender and the local-testing override live apply-side, so +an agent only names the individuals its result concerns. +""" + +from __future__ import annotations + +import re +from collections.abc import Iterable +from typing import Annotated + +from agent_tools.registry import ToolError, tool, tools_in +from pydantic import Field + +from hackbot_runtime.actions.recorder import ActionsRecorder + +ACTION_TYPE = "email.send" + +# Substituted with the run's patch when the mail is sent. The agent decides +# whether the body mentions the patch at all, and how it is framed; this only +# says where the text goes. +PATCH_PLACEHOLDER = "{patch}" + + +def _params( + to: Iterable[str], + subject: str, + body_markdown: str, + attach_patch: bool = False, +) -> dict: + subject = subject.strip() + body_markdown = body_markdown.strip() + if not subject: + raise ToolError("subject must not be blank") + if not body_markdown: + raise ToolError("body_markdown must not be blank") + + recipients: list[str] = [] + for address in to: + address = address.strip() + if address and address not in recipients: + recipients.append(address) + + return { + "to": recipients, + "subject": subject, + "body_markdown": body_markdown, + "attach_patch": attach_patch, + } + + +@tool +async def send( + recorder: ActionsRecorder, + to: Annotated[ + list[str], + Field( + description=( + "Who the report concerns, as email addresses. May be empty for a " + "report that concerns no individual; the team address is added " + "when the mail is sent, so never list it here." + ) + ), + ], + subject: Annotated[ + str, + Field( + description=( + "Subject line. Lead with the verdict, so it reads in an inbox " + "listing without opening the mail." + ) + ), + ], + body_markdown: Annotated[ + str, + Field( + description=( + "Body in Markdown: headings, lists, tables and fenced code all " + "render. Link every identifier a recipient would otherwise have " + "to look up." + ) + ), + ], + reasoning: Annotated[ + str, + Field(description="Why these recipients need to hear about it (audit log)."), + ], +) -> str: + """Record an intended email. + + Recorded into the run summary for human review -- does not send any mail. + """ + recorder.record( + ACTION_TYPE, _params(to, subject, body_markdown), reasoning=reasoning + ) + return f"Recorded {ACTION_TYPE} (#{len(recorder.actions) - 1})." + + +def record_email( + recorder: ActionsRecorder, + *, + to: Iterable[str] = (), + subject: str, + body_markdown: str, + attach_patch: bool = False, + ref: str | None = None, +) -> dict: + """Record a report the agent was never asked to decide on. + + For a run whose outcome is always worth reporting: the wording is code, not a + model turn. ``to`` may be empty for a report that concerns no individual; the + handler still addresses the team. + + The run's patch can travel two ways, independently: inline, by putting + :data:`PATCH_PLACEHOLDER` in the body, and as a file, with ``attach_patch``. + Either way it is read once when the mail is sent, so the diff a recipient + reads and the file they save cannot differ. + """ + return recorder.record( + ACTION_TYPE, + _params(to, subject, body_markdown, attach_patch), + ref=ref, + ) + + +def demote_headings(md: str, by: int = 2) -> str: + """Shift ATX headings down ``by`` levels so agent prose nests under our own. + + Lines inside code fences (and ``#include`` and the like, which lack the + required space after ``#``) are left untouched. + """ + out = [] + in_fence = False + for line in md.splitlines(): + if line.lstrip().startswith(("```", "~~~")): + in_fence = not in_fence + out.append(line) + continue + match = re.match(r"(#{1,6}) ", line) if not in_fence else None + if match: + level = min(len(match.group(1)) + by, 6) + line = "#" * level + line[len(match.group(1)) :] + out.append(line) + return "\n".join(out) + + +TOOLS = tools_in(__name__) diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/email_handler.py b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/email_handler.py new file mode 100644 index 0000000000..098f7c4c00 --- /dev/null +++ b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/email_handler.py @@ -0,0 +1,138 @@ +"""Apply-side email action: delivers a recorded report through SendGrid. + +Configured entirely by environment, so an agent never carries an address it +does not choose per run: + +``SENDGRID_API_KEY``/``NOTIFICATION_SENDER`` + Required; without both, nothing is sent. +``NOTIFICATION_TEAM_EMAIL`` + Copied on every email and used as ``Reply-To``, so feedback reaches the team + and the team sees what the agents report. +``NOTIFICATION_OVERRIDE_EMAIL`` + Replaces every recipient with this one address. The single switch that keeps + a development deployment from mailing real developers. + +The recorded body is sent as it stands, except that ``{patch}`` is substituted +with the run's patch artifact. That artifact is read here rather than baked in at +record time, so an inline diff and an attachment of it cannot drift apart. How the +patch is introduced -- headings, fencing, whether it appears at all -- is the +recording agent's to decide. +""" + +from __future__ import annotations + +import base64 +import logging +import os +from typing import Any + +from hackbot_runtime.actions.email import PATCH_PLACEHOLDER +from hackbot_runtime.actions.handlers.base import ActionResult, ApplyContext +from hackbot_runtime.changes import PATCH_ARTIFACT + +log = logging.getLogger(__name__) + +# A diff long enough to bury the rest of the mail is cut off; the attachment, +# when the caller asked for one, still carries every line. +_MAX_PATCH_LINES = 400 + + +def _recipients(params: dict[str, Any]) -> list[str]: + override = os.environ.get("NOTIFICATION_OVERRIDE_EMAIL", "").strip() + if override: + return [override] + recipients = list(params.get("to") or []) + team = os.environ.get("NOTIFICATION_TEAM_EMAIL", "").strip() + if team and team not in recipients: + recipients.append(team) + return recipients + + +async def _patch(ctx: ApplyContext) -> bytes | None: + """The run's patch, or None when it published none. + + Read once however many ways the mail carries it, so the diff a recipient reads + is the file they save. A patch that never made it to storage costs the + recipient the patch, not the whole notification. + """ + try: + return await ctx.download_artifact(PATCH_ARTIFACT) + except Exception: + log.exception("Could not read the patch of run %s", ctx.run_id) + return None + + +def _truncated(patch: bytes) -> str: + lines = patch.decode(errors="replace").splitlines() + if len(lines) <= _MAX_PATCH_LINES: + return "\n".join(lines) + return "\n".join( + lines[:_MAX_PATCH_LINES] + + [f"... truncated to {_MAX_PATCH_LINES} of {len(lines)} lines"] + ) + + +class SendEmailHandler: + async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult: + api_key = os.environ.get("SENDGRID_API_KEY", "") + sender = os.environ.get("NOTIFICATION_SENDER", "") + if not (api_key and sender): + return ActionResult.failed( + "SENDGRID_API_KEY / NOTIFICATION_SENDER are not configured" + ) + + recipients = _recipients(params) + if not recipients: + return ActionResult.failed("No recipients for this email") + + import markdown2 + import sendgrid + from sendgrid.helpers.mail import ( + Attachment, + Cc, + Content, + Disposition, + FileContent, + FileName, + From, + HtmlContent, + Mail, + ReplyTo, + Subject, + To, + ) + + body_md = params["body_markdown"] + inline = PATCH_PLACEHOLDER in body_md + attach = bool(params.get("attach_patch")) + patch = await _patch(ctx) if inline or attach else None + if inline: + body_md = body_md.replace( + PATCH_PLACEHOLDER, + _truncated(patch) if patch else "(the patch could not be read)", + ) + message = Mail( + From(sender), + [To(recipients[0])] + [Cc(address) for address in recipients[1:]], + Subject(params["subject"]), + Content("text/plain", body_md), + HtmlContent( + markdown2.markdown(body_md, extras=["fenced-code-blocks", "tables"]) + ), + ) + team = os.environ.get("NOTIFICATION_TEAM_EMAIL", "").strip() + if team: + message.reply_to = ReplyTo(team) + if attach and patch is not None: + message.add_attachment( + Attachment( + FileContent(base64.b64encode(patch).decode()), + FileName(PATCH_ARTIFACT.rsplit("/", 1)[-1]), + disposition=Disposition("attachment"), + ) + ) + + response = sendgrid.SendGridAPIClient(api_key=api_key).send(message=message) + return ActionResult.ok( + {"recipients": recipients, "status_code": response.status_code} + ) diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/registry.py b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/registry.py index aa0241e44b..70e4e2c2b3 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/registry.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/registry.py @@ -5,6 +5,7 @@ CreateBugHandler, UpdateBugHandler, ) +from hackbot_runtime.actions.handlers.email_handler import SendEmailHandler from hackbot_runtime.actions.handlers.phabricator_handler import ( AddCommentHandler as PhabricatorAddCommentHandler, ) @@ -29,6 +30,7 @@ "phabricator.add_comment": PhabricatorAddCommentHandler(), "testrail.submit_test_plan": SubmitTestPlanHandler(), "slack.post_message": PostMessageHandler(), + "email.send": SendEmailHandler(), "try_server.push": PushHandler(), } diff --git a/libs/hackbot-runtime/hackbot_runtime/changes.py b/libs/hackbot-runtime/hackbot_runtime/changes.py index 67b8490af0..b00aba7eab 100644 --- a/libs/hackbot-runtime/hackbot_runtime/changes.py +++ b/libs/hackbot-runtime/hackbot_runtime/changes.py @@ -73,6 +73,22 @@ def base_commit(repo: Path) -> str: return _git(repo, "rev-parse", "HEAD").strip() +# Where publish_changes() puts the run's patch; the same key downstream reads. +PATCH_ARTIFACT = "changes/changes.patch" + + +def has_changes(repo: Path, base: str) -> bool: + """Whether the agent left anything for :func:`collect` to publish. + + Read-only, unlike ``collect``, which commits the working tree as it goes: for + code that needs the answer mid-run without disturbing what the agent is + editing. + """ + return _has_uncommitted(repo) or bool( + _git(repo, "rev-list", "-1", f"{base}..HEAD").strip() + ) + + def _has_uncommitted(repo: Path) -> bool: return bool(_git(repo, "status", "--porcelain").strip()) diff --git a/libs/hackbot-runtime/hackbot_runtime/context.py b/libs/hackbot-runtime/hackbot_runtime/context.py index e954e7daba..c1f9d71174 100644 --- a/libs/hackbot-runtime/hackbot_runtime/context.py +++ b/libs/hackbot-runtime/hackbot_runtime/context.py @@ -221,9 +221,20 @@ def publish_json(self, key: str, payload: dict) -> str: self.uploader, self.run_artifacts_dir, key, payload ) + @property + def source_changed(self) -> bool: + """Whether this run will publish a patch, for a notification to gate on. + + Read-only: the patch itself belongs to the apply step, which reads the + published artifact (see ``actions/handlers/email_handler.py``). + """ + if self._repo_path is None or self._source_base is None: + return False + return changes.has_changes(self._repo_path, self._source_base) + def publish_changes( self, - patch_key: str = "changes/changes.patch", + patch_key: str = changes.PATCH_ARTIFACT, meta_key: str = "changes/changes.json", phabricator_diff_key: str = "changes/phabricator_diff.json", try_push_key: str = "changes/try_push.json", diff --git a/libs/hackbot-runtime/pyproject.toml b/libs/hackbot-runtime/pyproject.toml index e4aa070f3e..bfd797f86a 100644 --- a/libs/hackbot-runtime/pyproject.toml +++ b/libs/hackbot-runtime/pyproject.toml @@ -14,6 +14,8 @@ dependencies = [ "phabricator-client", "testrail-client", "slack-sdk>=3.27.0", + "sendgrid>=6.12.5", + "markdown2>=2.4.0", "weave>=0.53.4" ] diff --git a/libs/hackbot-runtime/tests/test_changes.py b/libs/hackbot-runtime/tests/test_changes.py index 97994d46eb..69c0577b4d 100644 --- a/libs/hackbot-runtime/tests/test_changes.py +++ b/libs/hackbot-runtime/tests/test_changes.py @@ -15,6 +15,7 @@ _synthetic_commit, build_phabricator_diff, build_try_push, + has_changes, ) @@ -208,3 +209,34 @@ def test_build_try_push_rejects_abbreviated_base(tmp_path): # Lando needs a full published hash; a short one would fail server-side with # a far less obvious error. assert build_try_push(tmp_path, base[:12]) is None + + +# --- has_changes ------------------------------------------------------- # + + +def test_has_changes_sees_an_uncommitted_edit(tmp_path): + base = _init_repo(tmp_path) + (tmp_path / "file.txt").write_text("line1\nline2 edited\nline3\n") + assert has_changes(tmp_path, base) is True + + +def test_has_changes_sees_a_commit(tmp_path): + base = _init_repo(tmp_path) + _commit_change(tmp_path, "line1\nline2 committed\nline3\n") + assert has_changes(tmp_path, base) is True + + +def test_has_changes_is_false_for_an_untouched_tree(tmp_path): + base = _init_repo(tmp_path) + assert has_changes(tmp_path, base) is False + + +def test_has_changes_leaves_the_tree_and_index_alone(tmp_path): + base = _init_repo(tmp_path) + (tmp_path / "new.txt").write_text("brand new\n") + before = _git(tmp_path, "status", "--porcelain") + + assert has_changes(tmp_path, base) is True + + assert _git(tmp_path, "status", "--porcelain") == before + assert _git(tmp_path, "rev-parse", "HEAD").strip() == base diff --git a/libs/hackbot-runtime/tests/test_context.py b/libs/hackbot-runtime/tests/test_context.py index 6b25185952..116d412b09 100644 --- a/libs/hackbot-runtime/tests/test_context.py +++ b/libs/hackbot-runtime/tests/test_context.py @@ -243,3 +243,32 @@ def test_publish_changes_skips_phabricator_diff_without_action(tmp_path, monkeyp tmp_path / "artifacts" / "local-test" / "changes" / "phabricator_diff.json" ) assert not written.exists() + + +def test_source_changed_is_false_for_an_agent_that_touched_no_source(tmp_path): + hb = _hb(tmp_path, HackbotConfig()) + assert hb.source_changed is False + + +def test_source_changed_asks_git_without_disturbing_the_tree(tmp_path, monkeypatch): + # Read-only, unlike collect(): a notification gating on this must not commit + # the working tree out from under the agent. + hb = _hb_with_source(tmp_path, monkeypatch) + collected = [] + monkeypatch.setattr( + "hackbot_runtime.context.changes.collect", + lambda repo, base, repo_url: collected.append(base), + ) + monkeypatch.setattr( + "hackbot_runtime.context.changes.has_changes", lambda repo, base: True + ) + assert hb.source_changed is True + assert collected == [] + + +def test_source_changed_is_false_when_the_agent_changed_nothing(tmp_path, monkeypatch): + hb = _hb_with_source(tmp_path, monkeypatch) + monkeypatch.setattr( + "hackbot_runtime.context.changes.has_changes", lambda repo, base: False + ) + assert hb.source_changed is False diff --git a/libs/hackbot-runtime/tests/test_email_actions.py b/libs/hackbot-runtime/tests/test_email_actions.py new file mode 100644 index 0000000000..0ef6f9919f --- /dev/null +++ b/libs/hackbot-runtime/tests/test_email_actions.py @@ -0,0 +1,119 @@ +"""Tests for the recording side of the email action.""" + +import pytest +from agent_tools.registry import ToolError +from hackbot_runtime.actions import email +from hackbot_runtime.actions.recorder import ActionsRecorder + + +def test_record_email_records_the_action(): + rec = ActionsRecorder() + action = email.record_email( + rec, + to=["dev@mozilla.com"], + subject=" build failure ", + body_markdown=" # Analysis ", + attach_patch=True, + ) + assert rec.actions == [action] + assert action == { + "type": "email.send", + "params": { + "to": ["dev@mozilla.com"], + "subject": "build failure", + "body_markdown": "# Analysis", + "attach_patch": True, + }, + "reasoning": None, + } + + +def test_recipients_are_deduped_and_blanks_dropped(): + rec = ActionsRecorder() + action = email.record_email( + rec, + to=[" dev@mozilla.com ", "dev@mozilla.com", "", "author@mozilla.com"], + subject="s", + body_markdown="b", + ) + assert action["params"]["to"] == ["dev@mozilla.com", "author@mozilla.com"] + + +def test_a_report_concerning_no_individual_still_records(): + # The handler addresses the team; an empty recipient list is not an error. + rec = ActionsRecorder() + action = email.record_email(rec, subject="s", body_markdown="b") + assert action["params"]["to"] == [] + + +@pytest.mark.parametrize( + "subject,body", [("", "b"), (" ", "b"), ("s", ""), ("s", " ")] +) +def test_blank_subject_or_body_is_rejected(subject, body): + rec = ActionsRecorder() + with pytest.raises(ToolError): + email.record_email(rec, subject=subject, body_markdown=body) + assert rec.actions == [] + + +def test_demote_headings_nests_agent_prose(): + assert email.demote_headings("# Root\ntext\n## Sub") == "### Root\ntext\n#### Sub" + + +def test_demote_headings_leaves_fenced_code_alone(): + md = "```\n# not a heading\n```\n# heading" + assert email.demote_headings(md) == "```\n# not a heading\n```\n### heading" + + +def test_the_body_can_carry_the_patch_without_attaching_it(): + rec = ActionsRecorder() + action = email.record_email( + rec, subject="s", body_markdown=f"```diff\n{email.PATCH_PLACEHOLDER}\n```" + ) + assert action["params"]["attach_patch"] is False + assert email.PATCH_PLACEHOLDER in action["params"]["body_markdown"] + + +def test_a_patch_can_be_attached_without_appearing_in_the_body(): + rec = ActionsRecorder() + action = email.record_email(rec, subject="s", body_markdown="b", attach_patch=True) + assert action["params"]["attach_patch"] is True + assert email.PATCH_PLACEHOLDER not in action["params"]["body_markdown"] + + +async def test_send_records_the_action(): + rec = ActionsRecorder() + confirmation = await email.send( + rec, + to=[" dev@mozilla.com ", "dev@mozilla.com"], + subject=" build failure ", + body_markdown=" # Analysis ", + reasoning="the pusher has to back this out", + ) + assert "email.send (#0)" in confirmation + assert rec.actions == [ + { + "type": "email.send", + "params": { + "to": ["dev@mozilla.com"], + "subject": "build failure", + "body_markdown": "# Analysis", + "attach_patch": False, + }, + "reasoning": "the pusher has to back this out", + } + ] + + +@pytest.mark.parametrize("subject,body", [("", "b"), ("s", " ")]) +async def test_send_rejects_blank_arguments(subject, body): + rec = ActionsRecorder() + with pytest.raises(ToolError): + await email.send( + rec, to=["a@b.c"], subject=subject, body_markdown=body, reasoning="why" + ) + assert rec.actions == [] + + +def test_tools_are_exposed_under_the_email_namespace(): + assert [t.dotted for t in email.TOOLS] == ["email.send"] diff --git a/libs/hackbot-runtime/tests/test_email_handler.py b/libs/hackbot-runtime/tests/test_email_handler.py new file mode 100644 index 0000000000..459bf9d1bf --- /dev/null +++ b/libs/hackbot-runtime/tests/test_email_handler.py @@ -0,0 +1,209 @@ +"""Tests for the apply-side email handler. + +Mocks SendGrid so these exercise the handler's own logic -- recipient policy, +attachments, error handling -- without touching a network. +""" + +import base64 +import json + +import pytest +from hackbot_runtime.actions.handlers import email_handler + + +def _ctx(artifacts=None): + async def download(key): + if artifacts is None or key not in artifacts: + raise FileNotFoundError(key) + return artifacts[key] + + from hackbot_runtime.actions.handlers import ApplyContext + + return ApplyContext( + run_id="run-1", agent="build-repair", download_artifact=download + ) + + +class _FakeClient: + sent = None + + def __init__(self, api_key): + self.api_key = api_key + + def send(self, message): + _FakeClient.sent = message + return type("Response", (), {"status_code": 202})() + + +@pytest.fixture(autouse=True) +def _configured(monkeypatch): + _FakeClient.sent = None + monkeypatch.setenv("SENDGRID_API_KEY", "key") + monkeypatch.setenv("NOTIFICATION_SENDER", "hackbot@mozilla.com") + monkeypatch.setenv("NOTIFICATION_TEAM_EMAIL", "team@mozilla.com") + monkeypatch.delenv("NOTIFICATION_OVERRIDE_EMAIL", raising=False) + import sendgrid + + monkeypatch.setattr(sendgrid, "SendGridAPIClient", _FakeClient) + + +def _params(**overrides): + params = { + "to": ["dev@mozilla.com"], + "subject": "build failure", + "body_markdown": "# Analysis\n\ntext", + "attach_patch": False, + } + params.update(overrides) + return params + + +def _addresses(message): + return [ + address["email"] + for personalization in message.get()["personalizations"] + for group in ("to", "cc") + for address in personalization.get(group, []) + ] + + +async def test_sends_to_the_recorded_recipients_and_the_team(): + result = await email_handler.SendEmailHandler().apply(_params(), _ctx()) + assert result.status == "applied" + assert result.result == { + "recipients": ["dev@mozilla.com", "team@mozilla.com"], + "status_code": 202, + } + assert _addresses(_FakeClient.sent) == ["dev@mozilla.com", "team@mozilla.com"] + + +async def test_a_report_with_no_recipients_still_reaches_the_team(): + await email_handler.SendEmailHandler().apply(_params(to=[]), _ctx()) + assert _addresses(_FakeClient.sent) == ["team@mozilla.com"] + + +async def test_the_override_replaces_every_recipient(monkeypatch): + monkeypatch.setenv("NOTIFICATION_OVERRIDE_EMAIL", "me@mozilla.com") + await email_handler.SendEmailHandler().apply(_params(), _ctx()) + assert _addresses(_FakeClient.sent) == ["me@mozilla.com"] + + +async def test_the_body_is_sent_as_both_text_and_html(): + await email_handler.SendEmailHandler().apply(_params(), _ctx()) + contents = _FakeClient.sent.get()["content"] + assert contents[0]["type"] == "text/plain" + assert contents[0]["value"] == "# Analysis\n\ntext" + assert "

Analysis

" in contents[1]["value"] + + +async def test_the_inline_diff_and_the_attachment_are_the_same_bytes(): + patch = b"--- a/x\n+++ b/x\n+#include \n" + await email_handler.SendEmailHandler().apply( + _params( + attach_patch=True, + body_markdown="# Analysis\n\n```diff\n{patch}\n```", + ), + _ctx({"changes/changes.patch": patch}), + ) + sent = _FakeClient.sent.get() + (attachment,) = sent["attachments"] + assert attachment["filename"] == "changes.patch" + assert attachment["disposition"] == "attachment" + assert base64.b64decode(attachment["content"]) == patch + body = sent["content"][0]["value"] + assert body == "# Analysis\n\n```diff\n" + patch.decode().rstrip("\n") + "\n```" + + +async def test_the_patch_is_read_once_for_both_uses(): + reads = [] + + async def download(key): + reads.append(key) + return b"+fix\n" + + from hackbot_runtime.actions.handlers import ApplyContext + + ctx = ApplyContext(run_id="r", agent="a", download_artifact=download) + await email_handler.SendEmailHandler().apply( + _params( + attach_patch=True, + body_markdown="```diff\n{patch}\n```", + ), + ctx, + ) + assert reads == ["changes/changes.patch"] + + +async def test_a_long_patch_is_truncated_inline_but_attached_whole(): + patch = ("+line\n" * 500).encode() + await email_handler.SendEmailHandler().apply( + _params( + attach_patch=True, + body_markdown="```diff\n{patch}\n```", + ), + _ctx({"changes/changes.patch": patch}), + ) + sent = _FakeClient.sent.get() + assert "truncated to 400 of 500 lines" in sent["content"][0]["value"] + assert base64.b64decode(sent["attachments"][0]["content"]) == patch + + +async def test_the_built_payload_is_what_sendgrid_can_serialize(): + # `.get()` holding a helper object instead of its value only fails when the + # SDK serializes the request, which a mocked client never reaches. + await email_handler.SendEmailHandler().apply( + _params(attach_patch=True), + _ctx({"changes/changes.patch": b"diff --git a b"}), + ) + json.dumps(_FakeClient.sent.get()) + + +async def test_an_unreadable_patch_does_not_lose_the_email(): + result = await email_handler.SendEmailHandler().apply( + _params( + attach_patch=True, + body_markdown="```diff\n{patch}\n```", + ), + _ctx(), + ) + assert result.status == "applied" + sent = _FakeClient.sent.get() + assert "attachments" not in sent + assert "{patch}" not in sent["content"][0]["value"] + + +async def test_a_body_can_reference_the_patch_without_attaching_it(): + await email_handler.SendEmailHandler().apply( + _params(body_markdown="```diff\n{patch}\n```"), + _ctx({"changes/changes.patch": b"+fix\n"}), + ) + sent = _FakeClient.sent.get() + assert "attachments" not in sent + assert "+fix" in sent["content"][0]["value"] + + +async def test_the_body_is_untouched_when_the_run_recorded_no_patch(): + await email_handler.SendEmailHandler().apply(_params(), _ctx()) + sent = _FakeClient.sent.get() + assert "attachments" not in sent + assert sent["content"][0]["value"] == "# Analysis\n\ntext" + + +async def test_without_sendgrid_configured_nothing_is_sent(monkeypatch): + monkeypatch.delenv("SENDGRID_API_KEY") + result = await email_handler.SendEmailHandler().apply(_params(), _ctx()) + assert result.status == "failed" + assert "SENDGRID_API_KEY" in result.error + assert _FakeClient.sent is None + + +async def test_a_sendgrid_error_is_not_a_delivered_email(monkeypatch): + # Raised, not caught: the applier stamps the action failed with this message. + import sendgrid + + def _boom(api_key): + raise RuntimeError("sendgrid is down") + + monkeypatch.setattr(sendgrid, "SendGridAPIClient", _boom) + with pytest.raises(RuntimeError, match="sendgrid is down"): + await email_handler.SendEmailHandler().apply(_params(), _ctx()) diff --git a/services/hackbot-api/app/agents.py b/services/hackbot-api/app/agents.py index 0876868949..d1d32b2b03 100644 --- a/services/hackbot-api/app/agents.py +++ b/services/hackbot-api/app/agents.py @@ -91,6 +91,9 @@ def model_to_env(inputs: BaseModel) -> dict[str, str]: description="Analyze a Firefox build failure at a specific commit and produce a candidate fix patch.", job_name="hackbot-agent-build-repair", input_schema=BuildRepairInputs, + # Its only action is the failure-analysis email, which used to be sent + # unconditionally by the pulse listener. + auto_apply_actions=True, ), "frontend-triage": AgentSpec( name="frontend-triage", diff --git a/services/hackbot-api/tests/test_actions_applier.py b/services/hackbot-api/tests/test_actions_applier.py index 8e944df6dc..1a193eb8d2 100644 --- a/services/hackbot-api/tests/test_actions_applier.py +++ b/services/hackbot-api/tests/test_actions_applier.py @@ -168,18 +168,18 @@ def test_the_real_frontend_triage_spec_asks_for_consent(): def test_which_agents_auto_apply_without_asking_for_consent(): - # `bug-fix` and `test-repair` auto-apply whatever they record, and the apply step - # dispatches against the runtime's *global* handler registry — creating bugs, - # attaching files, submitting Phabricator patches. Both predate this change, and - # bounding them is a decision about those agents, so this records the gap rather - # than closing it. Failing here means a new agent opted in without bounding what - # it records. + # `bug-fix`, `build-repair` and `test-repair` auto-apply whatever they record, and + # the apply step dispatches against the runtime's *global* handler registry — + # creating bugs, attaching files, submitting Phabricator patches. All predate this + # change, and bounding them is a decision about those agents, so this records the + # gap rather than closing it. Failing here means a new agent opted in without + # bounding what it records. unbounded = { name for name, spec in AGENT_REGISTRY.items() if spec.auto_apply_actions and not spec.auto_apply_requires_consent } - assert unbounded == {"bug-fix", "test-repair"} + assert unbounded == {"bug-fix", "build-repair", "test-repair"} class _FakeDB: @@ -259,11 +259,16 @@ async def test_succeeded_unvouched_run_records_but_does_not_apply(monkeypatch): async def test_other_agents_do_not_auto_apply(): - # Opting an agent in is a deliberate edit, so spell out who is in today: - # bug-fix and test-repair auto-apply unconditionally, frontend-triage only when - # the run vouched for itself, and everyone else stays human-gated. + # Opting an agent in is a deliberate edit, so spell out who is in today: bug-fix, + # build-repair and test-repair auto-apply unconditionally, frontend-triage only + # when the run vouched for itself, and everyone else stays human-gated. auto_apply = {n for n, s in AGENT_REGISTRY.items() if s.auto_apply_actions} - assert auto_apply == {"bug-fix", "frontend-triage", "test-repair"} + assert auto_apply == { + "bug-fix", + "build-repair", + "frontend-triage", + "test-repair", + } async def test_apply_all_pending_always_applies(monkeypatch): diff --git a/services/hackbot-pulse-listener/README.md b/services/hackbot-pulse-listener/README.md index 01e32b5998..9ad5e241c7 100644 --- a/services/hackbot-pulse-listener/README.md +++ b/services/hackbot-pulse-listener/README.md @@ -3,8 +3,8 @@ Its job is to **subscribe** to Taskcluster failure messages, **filter** them down to failures worth acting on, **dedupe** them, and dispatch a hackbot agent through the hackbot-api. It deliberately holds no investigation logic: each agent resolves the push -and commits itself, so the listener only decides _what to hand off_. When a run finishes -(minutes later) the listener polls the result and emails a report. +and commits itself, so the listener only decides _what to hand off_. Reporting is the +agent's too -- it records the email and Slack notification as actions. Failed **build** tasks go to `build-repair`; failed **test** tasks go to `test-repair`. @@ -50,11 +50,10 @@ Failed **build** tasks go to `build-repair`; failed **test** tasks go to `test-r back if the trigger fails, so only runs that really started count. Once the budget is spent, later test failures stop before any Treeherder work. Build-repair is not capped. -7. **Dispatch & report.** `POST /agents/{agent}/runs`, poll `GET /runs/{run_id}` until - terminal, then email a hackbot UI link, the analysis summary, a Treeherder link, and the - commit the agent blamed. Build-repair looks the blamed commit up in the firefox GitHub - mirror and mails its author; test-repair mails only the team address - (`NOTIFICATION_TEAM_EMAIL`) -- sheriffs are notified by the agent in Slack instead. +7. **Dispatch.** `POST /agents/{agent}/runs`, and that is the end of the listener's + involvement. Reporting belongs to the agent: it records an `email.send` action (and, + for test-repair, a Slack message), which hackbot-api applies once the run has + succeeded. See [docs/hackbot/actions.md](../../docs/hackbot/actions.md). The dedupe caches, the daily budget and pending-run tracking are all in-memory, so a restart resets them. @@ -89,26 +88,11 @@ for an ancestor still running before failing open. What differs is the unit comp ```bash export PULSE_USER=... PULSE_PASSWORD=... # https://pulseguardian.mozilla.org export HACKBOT_API_URL=https://hackbot-api.../ HACKBOT_API_KEY=... -export HACKBOT_UI_URL=https://hackbot-ui.../ export WATCHED_REPOS=autoland export DRY_RUN=true # log intended calls, don't POST 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. Build-repair mails the blamed commit's author (looked up in the -firefox GitHub mirror), the pushing developer, and the `NOTIFICATION_TEAM_EMAIL` team -address if set; test-repair mails only the team address -- never the culprit author or -the pushing developer, though the culprit is still named in the body. Its verdicts are -tracking for the hackbot team, so every verdict is mailed, intermittents included; what -reaches sheriffs is the agent's Slack message, and only when they have to act. Set -`NOTIFICATION_OVERRIDE_EMAIL` to route every notification to a single address (useful for -local testing). By default only build-repair runs that produced a patch are emailed; set -`NOTIFY_ONLY_WITH_PATCH=false` to also notify on transient / not-to-blame runs (test-repair always -notifies). -When `NOTIFICATION_TEAM_EMAIL` is set, notifications use it as `Reply-To` so recipients can -reply with feedback on the analysis. - ## Test ```bash diff --git a/services/hackbot-pulse-listener/app/client.py b/services/hackbot-pulse-listener/app/client.py index ff6b1d1379..2048308e73 100644 --- a/services/hackbot-pulse-listener/app/client.py +++ b/services/hackbot-pulse-listener/app/client.py @@ -33,22 +33,3 @@ def trigger_run(inputs: dict, agent_name: str | None = None) -> str | None: resp = httpx.post(url, json=inputs, headers=_headers(), timeout=_TIMEOUT) resp.raise_for_status() return resp.json()["run_id"] - - -def get_run(run_id: str) -> dict: - url = f"{settings.hackbot_api_url}/runs/{run_id}" - resp = httpx.get(url, headers=_headers(), timeout=_TIMEOUT) - resp.raise_for_status() - return resp.json() - - -def get_artifact(run_id: str, name: str) -> str | None: - """Download a run artifact's text content, or None if it is missing.""" - url = f"{settings.hackbot_api_url}/runs/{run_id}/artifacts/{name}" - resp = httpx.get(url, headers=_headers(), timeout=_TIMEOUT) - if resp.status_code == 404: - return None - resp.raise_for_status() - download = httpx.get(resp.json()["url"], timeout=_TIMEOUT) - download.raise_for_status() - return download.text diff --git a/services/hackbot-pulse-listener/app/config.py b/services/hackbot-pulse-listener/app/config.py index 67234753ee..023c54c033 100644 --- a/services/hackbot-pulse-listener/app/config.py +++ b/services/hackbot-pulse-listener/app/config.py @@ -10,15 +10,10 @@ class Settings(BaseSettings): # hackbot-api hackbot_api_url: str = "" hackbot_api_key: str = "" - hackbot_ui_url: str = "https://hackbot.moz.tools" agent_name: str = "build-repair" # Agent that analyzes test failures (separate Cloud Run Job from build-repair). test_repair_agent_name: str = "test-repair" - # Source links shown in notifications. - 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. @@ -56,27 +51,12 @@ class Settings(BaseSettings): # gate failing open -- could otherwise cost far more than the failures are worth. max_test_repairs_per_day: int = 50 - # Polling the API for run completion - poll_interval_seconds: int = 60 - run_max_age_minutes: int = 12 * 60 - # Shared worker pool for message processing and run polling. A - # regression check may block for a few minutes waiting for a parent - # build to settle, so the pool is sized well above the number of - # builds/runs in flight at once. Threads are cheap and mostly idle + # Worker pool for message processing. A regression check may block for a few + # minutes waiting for a parent build to settle, so the pool is sized well above + # the number of builds in flight at once. Threads are cheap and mostly idle # while waiting. max_workers: int = 256 - # Email notifications (SendGrid) - sendgrid_api_key: str | None = None - notification_sender: str | None = None - # Team address CC'd on every build-repair notification alongside the revision - # author, and the only recipient of test-repair verdicts. - notification_team_email: str | None = None - # Send all notifications to this address instead of the developer (local testing). - notification_override_email: str | None = None - # Only notify when the run produced a patch (skip transient / not-to-blame runs). - notify_only_with_patch: bool = True - dry_run: bool = False log_level: str = "INFO" # mozci's own (loguru) logging. Its per-task "missing results" warnings are diff --git a/services/hackbot-pulse-listener/app/consumer.py b/services/hackbot-pulse-listener/app/consumer.py index 5227ca4ee0..f60e0b15c0 100644 --- a/services/hackbot-pulse-listener/app/consumer.py +++ b/services/hackbot-pulse-listener/app/consumer.py @@ -9,9 +9,8 @@ from kombu import Connection, Exchange, Queue from kombu.mixins import ConsumerMixin -from app import client, lando, regression, taskcluster, treeherder, worker +from app import client, lando, regression, taskcluster, treeherder from app.config import settings -from app.models import RunContext logger = logging.getLogger(__name__) @@ -73,7 +72,7 @@ def _is_test_task(tags: dict) -> bool: return tags.get("kind") in TEST_KINDS or bool(tags.get("test-suite")) -def process(body: dict, executor: Executor) -> str | None: +def process(body: dict) -> str | None: """Handle one Taskcluster failure message. Returns the triggered run id.""" tags = (body.get("task") or {}).get("tags") or {} @@ -84,9 +83,9 @@ def process(body: dict, executor: Executor) -> str | None: task_label = tags.get("label") or "" if "build" in task_label and "test" not in task_label: - return _process_build(body, tags, executor) + return _process_build(body, tags) if _is_test_task(tags): - return _process_test(body, tags, executor) + return _process_test(body, tags) logger.debug("Ignoring non-build, non-test task %s", task_label) return None @@ -98,13 +97,12 @@ def _release(cache: TTLCache, lock: threading.Lock, keys) -> None: cache.pop(key, None) -def _process_build(body: dict, tags: dict, executor: Executor) -> str | None: +def _process_build(body: dict, tags: dict) -> str | None: """Build-failure path: trigger the build-repair agent.""" project = tags.get("project") task_label = tags.get("label") or "" task_id = body["status"]["taskId"] task_name = tags.get("label") or task_id - developer_email = tags.get("createdForUser") task = taskcluster.get_task(task_id) @@ -221,20 +219,10 @@ def _process_build(body: dict, tags: dict, executor: Executor) -> str | None: git_commit, job_link, ) - if run_id is not None: - ctx = RunContext( - run_id=run_id, - repo=project, - git_commit=git_commit, - hg_revision=hg_revision, - task_id=task_id, - developer_email=developer_email, - ) - executor.submit(worker.poll_and_notify, ctx) return run_id -def _process_test(body: dict, tags: dict, executor: Executor) -> str | None: +def _process_test(body: dict, tags: dict) -> str | None: """Test-failure path: filter, then trigger the test-repair agent for the task. One push emits many failing test tasks. We wait for Treeherder's verdict on this @@ -246,7 +234,6 @@ def _process_test(body: dict, tags: dict, executor: Executor) -> str | None: task_id = status.get("taskId") project = tags.get("project") label = tags.get("label") or task_id - developer_email = tags.get("createdForUser") task = taskcluster.get_task(task_id) @@ -339,7 +326,7 @@ def _process_test(body: dict, tags: dict, executor: Executor) -> str | None: def claimed_elsewhere() -> bool: return _push_claimed(hg_revision) - trigger = (project, hg_revision, task_id, label, developer_email, executor) + trigger = (project, hg_revision, task_id, label) whole_task = False try: groups = treeherder.failing_groups(project, hg_revision, task_id) @@ -543,8 +530,6 @@ def _trigger_test_repair( hg_revision: str, task_id: str, label: str, - developer_email: str | None, - executor: Executor, ) -> str | None: job_link = treeherder.job_url(project, hg_revision, task_id) if not _reserve_test_run(): @@ -592,38 +577,13 @@ def _trigger_test_repair( hg_revision, job_link, ) - if run_id is not None: - git_commit = lando.hg_to_git(hg_revision) - if not git_commit: - # Not fatal, unlike on the build path: the agent works from the task id - # alone, and a revision Lando has not mirrored yet is routine for a - # just-landed push. The notification omits the git revision instead of - # linking to an empty commit. - logger.warning( - "Could not map hg revision %s to git for task %s; " - "the notification will omit the git revision -- %s", - hg_revision, - task_id, - job_link, - ) - ctx = RunContext( - run_id=run_id, - repo=project, - git_commit=git_commit or "", - hg_revision=hg_revision, - task_id=task_id, - developer_email=developer_email, - agent=settings.test_repair_agent_name, - test_groups=list(test_groups), - ) - executor.submit(worker.poll_and_notify, ctx) return run_id def make_handler(executor: Executor): def run(body: dict) -> None: try: - process(body, executor) + process(body) except Exception: logger.exception("Error handling pulse message") diff --git a/services/hackbot-pulse-listener/app/github.py b/services/hackbot-pulse-listener/app/github.py deleted file mode 100644 index 23b8efeedf..0000000000 --- a/services/hackbot-pulse-listener/app/github.py +++ /dev/null @@ -1,32 +0,0 @@ -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/models.py b/services/hackbot-pulse-listener/app/models.py deleted file mode 100644 index 3863ee2fdb..0000000000 --- a/services/hackbot-pulse-listener/app/models.py +++ /dev/null @@ -1,17 +0,0 @@ -from dataclasses import dataclass, field - - -@dataclass -class RunContext: - """What the notifier needs about a triggered agent run.""" - - run_id: str - repo: str - git_commit: str - hg_revision: str - task_id: str - developer_email: str | None - # Which agent produced the run, and (for test-repair) every failing test group - # the run covers. These drive the notifier's recipient/body routing. - agent: str = "build-repair" - test_groups: list[str] = field(default_factory=list) diff --git a/services/hackbot-pulse-listener/app/notify.py b/services/hackbot-pulse-listener/app/notify.py deleted file mode 100644 index ab41b47004..0000000000 --- a/services/hackbot-pulse-listener/app/notify.py +++ /dev/null @@ -1,437 +0,0 @@ -import base64 -import logging -import re - -from app import client, github, treeherder -from app.config import settings -from app.models import RunContext - -logger = logging.getLogger(__name__) - -PATCH_ARTIFACT = "changes/changes.patch" -MAX_PATCH_LINES = 400 - - -def send_email( - ctx: RunContext, run_doc: dict, already_actioned: str | None = None -) -> None: - """Email the failure analysis. Only succeeded runs are notified. - - Routes on the agent that produced the run: test-repair sends a verdict-led body to - the hackbot team address; build-repair keeps its existing behavior. - - ``already_actioned`` is Treeherder's classification when a sheriff has already - dealt with the failure. - """ - if run_doc.get("status") != "succeeded": - logger.info("Run %s did not succeed; skipping notification", ctx.run_id) - return - if ctx.agent == settings.test_repair_agent_name: - _send_test_repair_email(ctx, run_doc, already_actioned) - else: - _send_build_repair_email(ctx, run_doc) - - -def _send_build_repair_email(ctx: RunContext, run_doc: dict) -> None: - patch = _fetch_patch(ctx.run_id, run_doc) - if settings.notify_only_with_patch and not patch: - logger.info("Run %s produced no patch; skipping notification", ctx.run_id) - return - - 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 - if not (settings.sendgrid_api_key and settings.notification_sender): - logger.info("SendGrid not configured; skipping email for run %s", ctx.run_id) - return - - subject = ( - f"[build-repair] Build failure analysis for {ctx.repo}@{ctx.git_commit[:12]}" - ) - body_md = _build_body(ctx, run_doc, patch, blamed_author) - _deliver(subject, body_md, recipients, patch) - - -def _send_test_repair_email( - ctx: RunContext, run_doc: dict, already_actioned: str | None = None -) -> None: - findings = (run_doc.get("summary") or {}).get("findings") or {} - culprit = findings.get("culprit_commit") - culprit_author = ( - github.commit_author_email(culprit) - if culprit and findings.get("classification") == "regression" - else None - ) - # test-repair verdicts are always notified (including do-not-backout verdicts), so - # the build-repair notify_only_with_patch gate does not apply here. - # - # The hackbot team address only: sheriffs are notified in Slack, by the agent, and - # only for the verdicts they act on, while the team gets every verdict here to track - # what the agent decided. Never the developer whose commit the agent happens to - # blame -- the culprit is still named in the body. - recipients = _recipients(settings.notification_team_email) - if not recipients: - logger.info( - "No recipients for test-repair run %s; skipping notification", ctx.run_id - ) - return - if not (settings.sendgrid_api_key and settings.notification_sender): - logger.info("SendGrid not configured; skipping email for run %s", ctx.run_id) - return - - patch = _fetch_patch(ctx.run_id, run_doc) - # In the subject too, so it can be skipped from the inbox. - prefix = "[already actioned] " if already_actioned else "" - subject = ( - f"[test-repair] {prefix}{_banner(findings)} - " - f"{_test_groups_label(ctx)} ({ctx.repo})" - ) - body_md = _build_test_repair_body( - ctx, findings, patch, culprit_author, already_actioned - ) - _deliver(subject, body_md, recipients, patch) - - -def _deliver(subject: str, body_md: str, recipients: list[str], patch: str | None): - import markdown2 - import sendgrid - from sendgrid.helpers.mail import ( - Attachment, - Cc, - Content, - Disposition, - FileContent, - FileName, - FileType, - From, - HtmlContent, - Mail, - ReplyTo, - Subject, - To, - ) - - html = markdown2.markdown(body_md, extras=["fenced-code-blocks", "tables"]) - sg = sendgrid.SendGridAPIClient(api_key=settings.sendgrid_api_key) - to_emails = [To(recipients[0])] + [Cc(addr) for addr in recipients[1:]] - message = Mail( - From(settings.notification_sender), - to_emails, - Subject(subject), - Content("text/plain", body_md), - HtmlContent(html), - ) - if patch: - message.attachment = Attachment( - FileContent(base64.b64encode(patch.encode()).decode()), - FileName("changes.patch"), - 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 notification to %s (status %s)", - ", ".join(recipients), - response.status_code, - ) - - -def _recipients(primary: str | None, secondary: str | None = None) -> list[str]: - """Recipients for a run, deduped and ordered by priority, team address last. - - build-repair puts the blamed commit's author first and the pushing developer - second; test-repair passes only the team address and so reaches no individual. - ``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 (primary, secondary, settings.notification_team_email): - if addr and addr not in recipients: - recipients.append(addr) - return recipients - - -# The headline names the sheriff's action, which is always a backout. -_RECOMMENDATION_BANNER = { - "backout": "BACK OUT the culprit", - "do_not_backout": "DO NOT back out (intermittent)", - "land_fix": "BACK OUT the culprit, reland with the proposed fix squashed in", -} - - -def _banner(findings: dict) -> str: - """The recommendation as a human-readable headline, or the raw value.""" - recommendation = findings.get("recommendation") - return _RECOMMENDATION_BANNER.get(recommendation, recommendation or "analysis") - - -def _test_groups_label(ctx: RunContext) -> str: - """A one-line name for the run's failing groups, for the email subject.""" - if not ctx.test_groups: - return f"task {ctx.task_id}" - first, *rest = ctx.test_groups - return f"{first} (+{len(rest)} more)" if rest else first - - -def _already_actioned_banner(reason: str | None) -> list[str]: - """Say up front that the tree has been dealt with, when it has.""" - if not reason: - return [] - return [ - f"> **Already actioned by a sheriff.** Treeherder now classifies this job as " - f"_{reason}_, so the tree has been dealt with.", - "", - ] - - -def _build_test_repair_body( - ctx: RunContext, - findings: dict, - patch: str | None, - culprit_author: str | None, - already_actioned: str | None = None, -) -> str: - groups = ", ".join(f"`{g}`" for g in ctx.test_groups) or "not resolved" - lines = [ - *_already_actioned_banner(already_actioned), - "# Test failure analysis", - "", - f"- **Recommendation:** {_banner(findings)}", - f"- **Failing tests:** {groups}", - f"- **Classification:** {findings.get('classification')}", - f"- **Repository:** {ctx.repo}", - ] - # Omitted rather than linked as an empty commit when Lando has not mirrored the - # revision yet; the hg revision below always identifies the push. - if ctx.git_commit: - lines.append( - f"- **Revision (git):** [`{ctx.git_commit[:12]}`]({_git_url(ctx.git_commit)})" - ) - lines += [ - 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.job_url(ctx.repo, ctx.hg_revision, ctx.task_id)})", - ] - - confidence = findings.get("confidence") - if confidence is not None: - lines.append(f"- **Confidence:** {confidence}") - - culprit = findings.get("culprit_commit") - if culprit: - by = f" by {culprit_author}" if culprit_author else "" - lines.append( - f"- **Culprit commit:** [`{culprit[:12]}`]({_git_url(culprit)}){by}" - ) - - last_green = findings.get("last_green_revision") - if last_green: - lines.append(f"- **Last green revision:** `{last_green}`") - - bug = findings.get("culprit_bug") - if bug: - lines.append(f"- **Bug:** [{bug}]({_bug_url(bug)})") - - lines += _run_details(ctx) + _analysis_sections(findings) + _patch_section(patch) - # No team footer: the team is the only recipient. - lines += _patch_advice(patch) - return "\n".join(lines) - - -def _patch_advice(patch: str | None) -> list[str]: - """Say who the patch is for, next to the patch itself.""" - if not patch: - return [] - return [ - "", - "_For the author: squash this into your existing patches and reland. It is a " - "suggestion, not a follow-up to land on its own._", - ] - - -def _run_details(ctx: RunContext) -> list[str]: - if not settings.hackbot_ui_url: - return [] - return [ - f"- **Run details:** {settings.hackbot_ui_url.rstrip('/')}/runs/{ctx.run_id}" - ] - - -def _analysis_sections(findings: dict) -> list[str]: - lines: list[str] = [] - for key, title in (("summary", "Summary"), ("analysis", "Analysis")): - if findings.get(key): - lines += ["", f"## {title}", "", _demote_headings(findings[key])] - return lines - - -def _patch_section(patch: str | None) -> list[str]: - return ["", "## Proposed patch", "", _patch_block(patch)] if patch else [] - - -def _team_footer() -> list[str]: - if not settings.notification_team_email: - return [] - return [ - "", - "---", - "", - "_Reply to this email with any feedback on this analysis; it reaches " - "the hackbot team._", - ] - - -def _fetch_patch(run_id: str, run_doc: dict) -> str | None: - """Download the proposed-fix patch artifact, if the run produced one.""" - artifacts = run_doc.get("artifacts") or [] - if not any(a.get("name") == PATCH_ARTIFACT for a in artifacts): - return None - try: - return client.get_artifact(run_id, PATCH_ARTIFACT) - except Exception: - logger.exception("Failed to fetch patch for run %s", run_id) - return None - - -def _git_url(git_commit: str) -> str: - return f"{settings.firefox_git_url.rstrip('/')}/commit/{git_commit}" - - -def _hg_url(hg_revision: str) -> str: - return f"{settings.firefox_hg_url.rstrip('/')}/rev/{hg_revision}" - - -def _task_url(task_id: str) -> str: - return f"{settings.taskcluster_root_url.rstrip('/')}/tasks/{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, - blamed_author: str | None = None, -) -> str: - summary = run_doc.get("summary") or {} - findings = summary.get("findings") or {} - # A null verdict is the agent clearing the push; an absent one is no verdict. - cleared = "blamed_commit" in findings and not findings["blamed_commit"] - blamed_commit = findings.get("blamed_commit") - - lines = [ - "# Build failure analysis", - "", - f"- **Repository:** {ctx.repo}", - 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.job_url(ctx.repo, ctx.hg_revision, ctx.task_id)})", - ] - - if cleared: - lines.append( - "- **Not caused by this push:** the failure is pre-existing or " - "infrastructure, so no commit here is blamed." - ) - elif 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)})") - - lines += _run_details(ctx) - lines += _recipients_note(ctx, blamed_commit, blamed_author) - lines += _analysis_sections(findings) - - if findings.get("local_build_verified") is not None: - lines += [ - "", - "## Verification", - "", - f"- Local build verified: {findings['local_build_verified']}", - ] - - lines += _patch_section(patch) + _team_footer() - 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. - - Lines inside code fences (and ``#include`` and the like, which lack the - required space after ``#``) are left untouched. - """ - out = [] - in_fence = False - for line in md.splitlines(): - if line.lstrip().startswith(("```", "~~~")): - in_fence = not in_fence - out.append(line) - continue - match = re.match(r"(#{1,6}) ", line) if not in_fence else None - if match: - level = min(len(match.group(1)) + by, 6) - line = "#" * level + line[len(match.group(1)) :] - out.append(line) - return "\n".join(out) - - -def _patch_block(patch: str) -> str: - patch_lines = patch.splitlines() - shown = patch_lines[:MAX_PATCH_LINES] - block = ["```diff", *shown, "```"] - if len(patch_lines) > MAX_PATCH_LINES: - block.append( - f"\n_Patch truncated to {MAX_PATCH_LINES} lines; " - "see the attached changes.patch for the full diff._" - ) - return "\n".join(block) diff --git a/services/hackbot-pulse-listener/app/worker.py b/services/hackbot-pulse-listener/app/worker.py deleted file mode 100644 index d0d715aef9..0000000000 --- a/services/hackbot-pulse-listener/app/worker.py +++ /dev/null @@ -1,70 +0,0 @@ -import logging -import time - -from app import client, notify, treeherder -from app.config import settings -from app.models import RunContext - -logger = logging.getLogger(__name__) - -TERMINAL_STATUSES = {"succeeded", "failed", "timed_out"} - - -def poll_and_notify(ctx: RunContext) -> None: - """Poll the run until terminal, then notify. - - Runs on a background executor thread; never lets an exception escape. - """ - try: - run_doc = _poll_until_terminal(ctx.run_id) - except Exception: - logger.exception("Polling failed for run %s", ctx.run_id) - return - - if run_doc is None: - logger.warning( - "Run %s did not finish within %s minutes; giving up", - ctx.run_id, - settings.run_max_age_minutes, - ) - return - - try: - notify.send_email(ctx, run_doc, _already_actioned(ctx)) - except Exception: - logger.exception("Failed to send notification for run %s", ctx.run_id) - - -def _already_actioned(ctx: RunContext) -> str | None: - """Treeherder's verdict now that the run has finished, or None. - - A sheriff often acts while a run works. Never raises: the email goes out unmarked. - """ - try: - reason = treeherder.recheck_skip_reason(ctx.repo, ctx.task_id) - except Exception: - logger.exception( - "Could not re-check the classification of task %s before notifying", - ctx.task_id, - ) - return None - if reason: - logger.info( - "Task %s was classified as %s while run %s was working; " - "the notification will say so", - ctx.task_id, - reason, - ctx.run_id, - ) - return reason - - -def _poll_until_terminal(run_id: str) -> dict | None: - deadline = time.monotonic() + settings.run_max_age_minutes * 60 - while True: - run_doc = client.get_run(run_id) - if run_doc.get("status") in TERMINAL_STATUSES: - return run_doc - if time.monotonic() >= deadline: - return None - time.sleep(settings.poll_interval_seconds) diff --git a/services/hackbot-pulse-listener/deploy.sh b/services/hackbot-pulse-listener/deploy.sh index 94913f00b1..111c043b68 100755 --- a/services/hackbot-pulse-listener/deploy.sh +++ b/services/hackbot-pulse-listener/deploy.sh @@ -18,16 +18,13 @@ # created from its value; existing secrets are never overwritten (rotate with # `gcloud secrets versions add`): # PULSE_PASSWORD -> secret `pulse-password` -# SENDGRID_API_KEY -> secret `sendgrid-api-key` # HACKBOT_API_KEY -> secret `external-api-key` (shared with hackbot-api) # # Usage: -# source .env # provides PULSE_PASSWORD, HACKBOT_API_KEY, SENDGRID_API_KEY, etc. +# source .env # provides PULSE_PASSWORD, HACKBOT_API_KEY, etc. # PROJECT=my-proj REGION=us-central1 \ # HACKBOT_API_URL=https://hackbot-api-xxxx.run.app \ -# HACKBOT_UI_URL=https://hackbot-ui-xxxx.run.app \ -# PULSE_USER=my-pulse-user NOTIFICATION_SENDER= \ -# NOTIFICATION_TEAM_EMAIL=hackbot-developers@mozilla.com \ +# PULSE_USER=my-pulse-user \ # ./deploy.sh set -euo pipefail @@ -36,11 +33,8 @@ REGION="${REGION:-us-central1}" SERVICE="${SERVICE:-hackbot-pulse-listener}" REPO="${REPO:-hackbot}" HACKBOT_API_URL="${HACKBOT_API_URL:?set HACKBOT_API_URL to the hackbot-api base URL}" -HACKBOT_UI_URL="${HACKBOT_UI_URL:?set HACKBOT_UI_URL to the hackbot-ui base URL}" PULSE_USER="${PULSE_USER:?set PULSE_USER (https://pulseguardian.mozilla.org)}" WATCHED_REPOS="${WATCHED_REPOS:-autoland}" -NOTIFICATION_SENDER="${NOTIFICATION_SENDER:?set NOTIFICATION_SENDER (verified SendGrid sender)}" -NOTIFICATION_TEAM_EMAIL="${NOTIFICATION_TEAM_EMAIL:-}" SA_NAME="${SA_NAME:-hackbot-pulse-listener-run}" SA_EMAIL="${SA_EMAIL:-${SA_NAME}@${PROJECT}.iam.gserviceaccount.com}" @@ -48,13 +42,11 @@ SA_EMAIL="${SA_EMAIL:-${SA_NAME}@${PROJECT}.iam.gserviceaccount.com}" # Secret Manager secret names (where the values live). PULSE_SECRET="${PULSE_SECRET:-pulse-password}" API_KEY_SECRET="${API_KEY_SECRET:-external-api-key}" -SENDGRID_SECRET="${SENDGRID_SECRET:-sendgrid-api-key}" # Secret values, using the same names as the app's .env so `source .env` works. # Used only to seed a secret that does not exist yet (never overwrites). PULSE_PASSWORD="${PULSE_PASSWORD:-}" HACKBOT_API_KEY="${HACKBOT_API_KEY:-}" -SENDGRID_API_KEY="${SENDGRID_API_KEY:-}" IMAGE="${REGION}-docker.pkg.dev/${PROJECT}/${REPO}/${SERVICE}:latest" # Build context is the repo root (the Dockerfile needs the workspace lock files). @@ -79,10 +71,9 @@ ensure_secret() { # secret_name value } ensure_secret "${PULSE_SECRET}" "${PULSE_PASSWORD}" ensure_secret "${API_KEY_SECRET}" "${HACKBOT_API_KEY}" -ensure_secret "${SENDGRID_SECRET}" "${SENDGRID_API_KEY}" echo "==> Granting the SA read access to its secrets" -for s in "${PULSE_SECRET}" "${API_KEY_SECRET}" "${SENDGRID_SECRET}"; do +for s in "${PULSE_SECRET}" "${API_KEY_SECRET}"; do gcloud secrets add-iam-policy-binding "$s" \ --member="serviceAccount:${SA_EMAIL}" \ --role=roles/secretmanager.secretAccessor >/dev/null @@ -99,11 +90,8 @@ gcloud builds submit "${ROOT_DIR}" \ --config <(printf 'steps:\n- name: gcr.io/cloud-builders/docker\n env: ["DOCKER_BUILDKIT=1"]\n args: ["build","-t","%s","-f","services/%s/Dockerfile","."]\nimages: ["%s"]\n' "${IMAGE}" "${SERVICE}" "${IMAGE}") echo "==> Deploying worker pool" -ENV_VARS="HACKBOT_API_URL=${HACKBOT_API_URL},HACKBOT_UI_URL=${HACKBOT_UI_URL}" -ENV_VARS="${ENV_VARS},ENVIRONMENT=production" +ENV_VARS="HACKBOT_API_URL=${HACKBOT_API_URL},ENVIRONMENT=production" ENV_VARS="${ENV_VARS},PULSE_USER=${PULSE_USER},WATCHED_REPOS=${WATCHED_REPOS}" -ENV_VARS="${ENV_VARS},NOTIFICATION_SENDER=${NOTIFICATION_SENDER}" -ENV_VARS="${ENV_VARS},NOTIFICATION_TEAM_EMAIL=${NOTIFICATION_TEAM_EMAIL}" gcloud beta run worker-pools deploy "${SERVICE}" \ --image "${IMAGE}" \ @@ -112,6 +100,6 @@ gcloud beta run worker-pools deploy "${SERVICE}" \ --memory 2Gi \ --service-account "${SA_EMAIL}" \ --set-env-vars "${ENV_VARS}" \ - --set-secrets "PULSE_PASSWORD=${PULSE_SECRET}:latest,HACKBOT_API_KEY=${API_KEY_SECRET}:latest,SENDGRID_API_KEY=${SENDGRID_SECRET}:latest" + --set-secrets "PULSE_PASSWORD=${PULSE_SECRET}:latest,HACKBOT_API_KEY=${API_KEY_SECRET}:latest" echo "==> Deployed worker pool '${SERVICE}'" diff --git a/services/hackbot-pulse-listener/pyproject.toml b/services/hackbot-pulse-listener/pyproject.toml index 54fedc66cd..864975a14a 100644 --- a/services/hackbot-pulse-listener/pyproject.toml +++ b/services/hackbot-pulse-listener/pyproject.toml @@ -9,8 +9,6 @@ dependencies = [ "taskcluster>=97.1,<102.1", "httpx>=0.26.0", "pydantic-settings>=2.1.0", - "sendgrid>=6.12.5", - "markdown2>=2.4.0", "cachetools>=5.3.0", "sentry-sdk>=2.51.0", "tenacity~=9.1.4", diff --git a/services/hackbot-pulse-listener/scripts/send_test_run.py b/services/hackbot-pulse-listener/scripts/send_test_run.py deleted file mode 100644 index 741ae12537..0000000000 --- a/services/hackbot-pulse-listener/scripts/send_test_run.py +++ /dev/null @@ -1,95 +0,0 @@ -"""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, backfill and push-age gates 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." - ) - - # Any real failing task is necessarily older than the push-age limit by the - # time you find it on Treeherder, and may well be a backfill, so --force has - # to lift those gates too. - if args.force: - consumer.regression.is_new_build_failure = lambda *a, **k: True - consumer.regression.is_stale_push = lambda *a, **k: False - consumer.taskcluster.is_action_scheduled = lambda *a, **k: False - - 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 1995743685..1830ac0d3b 100644 --- a/services/hackbot-pulse-listener/tests/test_consumer.py +++ b/services/hackbot-pulse-listener/tests/test_consumer.py @@ -89,7 +89,6 @@ def test_sample_messages_route_to_test_repair_not_build(): # The captured samples are all test tasks. They now reach the test-repair path # (they were ignored outright when the listener only handled builds), and none of # them triggers the build-repair agent. - executor = MagicMock() with ( patch.object(consumer.taskcluster, "get_task", return_value=_task_def()), patch.object(consumer.treeherder, "failing_groups", return_value=[]) as groups, @@ -98,23 +97,20 @@ def test_sample_messages_route_to_test_repair_not_build(): patch.object(consumer.client, "trigger_run") as trigger, ): for body in _sample_bodies(): - assert consumer.process(body, executor) is None + assert consumer.process(body) is None # At least the autoland samples were routed to the test-repair path. assert groups.called trigger.assert_not_called() - executor.submit.assert_not_called() def test_missing_label_is_skipped_not_crashed(): - executor = MagicMock() body = {"status": {"taskId": "XYZ"}, "task": {"tags": {"project": "autoland"}}} with patch.object(consumer.client, "trigger_run") as trigger: - assert consumer.process(body, executor) is None + assert consumer.process(body) is None trigger.assert_not_called() -def test_build_failure_triggers_run_and_submits_poll(): - executor = MagicMock() +def test_build_failure_triggers_run(): with ( patch.object(consumer.taskcluster, "get_task", return_value=_task_def()), patch.object(consumer.lando, "hg_to_git", return_value="deadbeef"), @@ -122,26 +118,16 @@ def test_build_failure_triggers_run_and_submits_poll(): patch.object(consumer.regression, "is_new_build_failure", return_value=True), patch.object(consumer.client, "trigger_run", return_value="run-1") as trigger, ): - run_id = consumer.process(_build_msg(), executor) + run_id = consumer.process(_build_msg()) assert run_id == "run-1" trigger.assert_called_once() inputs = trigger.call_args.args[0] 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 - assert ctx.run_id == "run-1" - assert ctx.git_commit == "deadbeef" - assert ctx.hg_revision == "hgrev" - assert ctx.task_id == "ABC" - assert ctx.repo == "autoland" - assert ctx.developer_email == "dev@mozilla.com" def test_only_failure_tasks_sent_to_agent(): - executor = MagicMock() with ( patch.object(consumer.taskcluster, "get_task", return_value=_task_def()), patch.object(consumer.lando, "hg_to_git", return_value="deadbeef"), @@ -149,7 +135,7 @@ def test_only_failure_tasks_sent_to_agent(): 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) + consumer.process(_build_msg()) # The agent resolves the push (and its authors) itself; the listener # only hands it the failing tasks. @@ -159,7 +145,6 @@ def test_only_failure_tasks_sent_to_agent(): def test_same_revision_triggers_once(): - executor = MagicMock() with ( patch.object(consumer.taskcluster, "get_task", return_value=_task_def()), patch.object(consumer.lando, "hg_to_git", return_value="deadbeef"), @@ -167,14 +152,13 @@ def test_same_revision_triggers_once(): 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(task_id="T1"), executor) - consumer.process(_build_msg(task_id="T2"), executor) + consumer.process(_build_msg(task_id="T1")) + consumer.process(_build_msg(task_id="T2")) trigger.assert_called_once() def test_inherited_failure_is_skipped_before_mapping(): - executor = MagicMock() with ( patch.object(consumer.taskcluster, "get_task", return_value=_task_def()), patch.object(consumer.treeherder, "recheck_skip_reason", return_value=None), @@ -182,15 +166,13 @@ def test_inherited_failure_is_skipped_before_mapping(): patch.object(consumer.lando, "hg_to_git") as hg_to_git, patch.object(consumer.client, "trigger_run") as trigger, ): - assert consumer.process(_build_msg(), executor) is None + assert consumer.process(_build_msg()) is None hg_to_git.assert_not_called() trigger.assert_not_called() - executor.submit.assert_not_called() def test_multiple_builds_same_revision_trigger_once(): - executor = MagicMock() with ( patch.object(consumer.taskcluster, "get_task", return_value=_task_def()), patch.object(consumer.lando, "hg_to_git", return_value="deadbeef"), @@ -198,14 +180,13 @@ def test_multiple_builds_same_revision_trigger_once(): 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(task_id="T1", label="build-linux64/opt"), executor) - consumer.process(_build_msg(task_id="T2", label="build-macosx64/opt"), executor) + consumer.process(_build_msg(task_id="T1", label="build-linux64/opt")) + consumer.process(_build_msg(task_id="T2", label="build-macosx64/opt")) trigger.assert_called_once() def test_inherited_label_does_not_suppress_new_label_on_same_revision(): - executor = MagicMock() with ( patch.object(consumer.taskcluster, "get_task", return_value=_task_def()), patch.object(consumer.lando, "hg_to_git", return_value="deadbeef"), @@ -216,16 +197,12 @@ def test_inherited_label_does_not_suppress_new_label_on_same_revision(): ): # Inherited failure on the first label must not mark the revision seen. assert ( - consumer.process( - _build_msg(task_id="T1", label="build-linux64/opt"), executor - ) + consumer.process(_build_msg(task_id="T1", label="build-linux64/opt")) is None ) # A genuine regression on another label of the same push still runs. assert ( - consumer.process( - _build_msg(task_id="T2", label="build-macosx64/opt"), executor - ) + consumer.process(_build_msg(task_id="T2", label="build-macosx64/opt")) == "run-1" ) @@ -233,19 +210,17 @@ def test_inherited_label_does_not_suppress_new_label_on_same_revision(): def test_unwatched_project_skipped_before_api_call(): - executor = MagicMock() with ( patch.object(consumer.taskcluster, "get_task") as get_task, patch.object(consumer.client, "trigger_run") as trigger, ): - assert consumer.process(_build_msg(project="try"), executor) is None + assert consumer.process(_build_msg(project="try")) is None get_task.assert_not_called() trigger.assert_not_called() def test_backfilled_task_skipped_before_push_checks(fresh_push): - executor = MagicMock() backfill = _task_def(parent="ACTION-CALLBACK") with ( patch.object(consumer.taskcluster, "get_task", return_value=backfill), @@ -253,16 +228,14 @@ def test_backfilled_task_skipped_before_push_checks(fresh_push): patch.object(consumer.regression, "is_new_build_failure") as is_new, patch.object(consumer.client, "trigger_run") as trigger, ): - assert consumer.process(_build_msg(), executor) is None + assert consumer.process(_build_msg()) is None fresh_push.assert_not_called() is_new.assert_not_called() trigger.assert_not_called() - executor.submit.assert_not_called() def test_stale_push_skipped_before_regression_check(fresh_push): - executor = MagicMock() fresh_push.return_value = True with ( patch.object(consumer.taskcluster, "get_task", return_value=_task_def()), @@ -270,7 +243,7 @@ def test_stale_push_skipped_before_regression_check(fresh_push): patch.object(consumer.regression, "is_new_build_failure") as is_new, patch.object(consumer.client, "trigger_run") as trigger, ): - assert consumer.process(_build_msg(), executor) is None + assert consumer.process(_build_msg()) is None fresh_push.assert_called_once_with( "autoland", "hgrev", consumer.settings.max_push_age_hours * 3600 @@ -278,11 +251,9 @@ def test_stale_push_skipped_before_regression_check(fresh_push): # The regression check can block for an hour, so it must come after. is_new.assert_not_called() trigger.assert_not_called() - executor.submit.assert_not_called() def test_stale_push_is_not_marked_seen(): - executor = MagicMock() with ( patch.object(consumer.taskcluster, "get_task", return_value=_task_def()), patch.object(consumer.regression, "is_stale_push", side_effect=[True, False]), @@ -291,14 +262,13 @@ def test_stale_push_is_not_marked_seen(): patch.object(consumer.regression, "is_new_build_failure", return_value=True), patch.object(consumer.client, "trigger_run", return_value="run-1"), ): - assert consumer.process(_build_msg(task_id="T1"), executor) is None + assert consumer.process(_build_msg(task_id="T1")) is None # A stale verdict is not a claim on the revision, so a later message for # it (e.g. once the push date becomes readable) is still handled. - assert consumer.process(_build_msg(task_id="T2"), executor) == "run-1" + assert consumer.process(_build_msg(task_id="T2")) == "run-1" def test_unmappable_revision_skipped(): - executor = MagicMock() with ( patch.object(consumer.taskcluster, "get_task", return_value=_task_def()), patch.object(consumer.treeherder, "recheck_skip_reason", return_value=None), @@ -306,14 +276,12 @@ def test_unmappable_revision_skipped(): patch.object(consumer.lando, "hg_to_git", return_value=None), patch.object(consumer.client, "trigger_run") as trigger, ): - assert consumer.process(_build_msg(), executor) is None + assert consumer.process(_build_msg()) is None trigger.assert_not_called() - executor.submit.assert_not_called() def test_trigger_failure_releases_revision_for_retry(): - executor = MagicMock() with ( patch.object(consumer.taskcluster, "get_task", return_value=_task_def()), patch.object(consumer.lando, "hg_to_git", return_value="deadbeef"), @@ -323,9 +291,9 @@ def test_trigger_failure_releases_revision_for_retry(): consumer.client, "trigger_run", side_effect=[RuntimeError("boom"), "run-2"] ) as trigger, ): - assert consumer.process(_build_msg(task_id="T1"), executor) is None + assert consumer.process(_build_msg(task_id="T1")) is None # Same revision can be retried because the failed claim was released. - assert consumer.process(_build_msg(task_id="T2"), executor) == "run-2" + assert consumer.process(_build_msg(task_id="T2")) == "run-2" assert trigger.call_count == 2 @@ -361,7 +329,6 @@ def env(monkeypatch): is_new_task_failure=MagicMock(return_value=True), hg_to_git=MagicMock(return_value="gitH"), trigger_run=MagicMock(return_value="tr-1"), - executor=MagicMock(), ) monkeypatch.setattr(consumer.taskcluster, "get_task", mocks.get_task) monkeypatch.setattr(consumer.treeherder, "failing_groups", mocks.failing_groups) @@ -387,7 +354,7 @@ def env(monkeypatch): def test_test_failure_triggers_rca_run(env): - run_id = consumer.process(_test_msg(), env.executor) + run_id = consumer.process(_test_msg()) assert run_id == "tr-1" env.trigger_run.assert_called_once() @@ -400,16 +367,12 @@ def test_test_failure_triggers_rca_run(env): } assert "test_id" not in inputs assert "candidate_commits" not in inputs - fn, ctx = env.executor.submit.call_args.args - assert fn is consumer.worker.poll_and_notify - assert ctx.agent == "test-repair" - assert ctx.test_groups == [_GROUP] def test_treeherder_intermittent_skipped_before_any_walk(env): # Treeherder's own verdict rules the failure out before any mozci work. env.await_skip_reason.return_value = "intermittent" - assert consumer.process(_test_msg(), env.executor) is None + assert consumer.process(_test_msg()) is None env.failing_groups.assert_not_called() env.new_test_failures.assert_not_called() env.trigger_run.assert_not_called() @@ -418,36 +381,36 @@ def test_treeherder_intermittent_skipped_before_any_walk(env): def test_unclassified_failure_is_investigated(env): # "not classified" / "new failure" leave the decision to the mozci walk. env.await_skip_reason.return_value = None - assert consumer.process(_test_msg(), env.executor) == "tr-1" + assert consumer.process(_test_msg()) == "tr-1" env.new_test_failures.assert_called_once() def test_inherited_test_group_skipped(env): env.new_test_failures.side_effect = lambda *_: set() - assert consumer.process(_test_msg(), env.executor) is None + assert consumer.process(_test_msg()) is None env.trigger_run.assert_not_called() def test_one_run_per_push(env): # The agent reads the push's other failures itself, so the first task worth # investigating is enough; later failing tasks of the same push are skipped. - assert consumer.process(_test_msg(task_id="A"), env.executor) == "tr-1" - assert consumer.process(_test_msg(task_id="B"), env.executor) is None + assert consumer.process(_test_msg(task_id="A")) == "tr-1" + assert consumer.process(_test_msg(task_id="B")) is None env.trigger_run.assert_called_once() def test_later_task_of_a_claimed_push_stops_before_treeherder(env): - assert consumer.process(_test_msg(task_id="A"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="A")) == "tr-1" env.job_for_task.reset_mock() env.failing_groups.reset_mock() - assert consumer.process(_test_msg(task_id="B"), env.executor) is None + assert consumer.process(_test_msg(task_id="B")) is None env.job_for_task.assert_not_called() env.failing_groups.assert_not_called() def test_no_failing_groups_skips(env): env.failing_groups.return_value = [] - assert consumer.process(_test_msg(), env.executor) is None + assert consumer.process(_test_msg()) is None env.trigger_run.assert_not_called() @@ -457,18 +420,16 @@ def test_task_without_group_results_still_triggers_run(env): env.failing_groups.side_effect = consumer.treeherder.GroupResultsUnavailable( "no group results for task ERR" ) - assert consumer.process(_test_msg(task_id="ERR"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="ERR")) == "tr-1" env.trigger_run.assert_called_once() - # With no groups resolved there is nothing to filter or to name. + # With no groups resolved there is nothing to filter. env.new_test_failures.assert_not_called() - _, ctx = env.executor.submit.call_args.args - assert ctx.test_groups == [] def test_unreadable_group_results_triggers_once_per_push(env): env.failing_groups.side_effect = RuntimeError("treeherder down") - consumer.process(_test_msg(task_id="A"), env.executor) - consumer.process(_test_msg(task_id="B"), env.executor) + consumer.process(_test_msg(task_id="A")) + consumer.process(_test_msg(task_id="B")) env.trigger_run.assert_called_once() @@ -477,8 +438,8 @@ def test_rejected_task_does_not_suppress_a_real_regression_on_the_push(env): # inherited leaves the push open for the next failing task -- which may be the # genuine regression. env.new_test_failures.side_effect = [set(), {_GROUP}] - assert consumer.process(_test_msg(task_id="A"), env.executor) is None - assert consumer.process(_test_msg(task_id="B"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="A")) is None + assert consumer.process(_test_msg(task_id="B")) == "tr-1" env.trigger_run.assert_called_once() @@ -486,31 +447,21 @@ def test_intermittent_task_does_not_suppress_the_next_task(env): # Same for a task Treeherder has already classified: it must not claim a push # it will not investigate. env.await_skip_reason.side_effect = ["intermittent", None] - assert consumer.process(_test_msg(task_id="A"), env.executor) is None - assert consumer.process(_test_msg(task_id="B"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="A")) is None + assert consumer.process(_test_msg(task_id="B")) == "tr-1" env.trigger_run.assert_called_once() -def test_missing_git_mapping_still_triggers_run(env): - # Unlike a build failure, the run is still useful (the agent works from the - # task id), so a revision Lando has not mirrored yet must not drop it. - env.hg_to_git.return_value = None - assert consumer.process(_test_msg(), env.executor) == "tr-1" - env.trigger_run.assert_called_once() - _, ctx = env.executor.submit.call_args.args - assert ctx.git_commit == "" - - def test_unwatched_project_test_skipped(env): - assert consumer.process(_test_msg(project="try"), env.executor) is None + assert consumer.process(_test_msg(project="try")) is None env.get_task.assert_not_called() env.failing_groups.assert_not_called() def test_test_repair_trigger_failure_releases_group_for_retry(env): env.trigger_run.side_effect = [RuntimeError("boom"), "tr-2"] - assert consumer.process(_test_msg(task_id="A"), env.executor) is None - assert consumer.process(_test_msg(task_id="B"), env.executor) == "tr-2" + assert consumer.process(_test_msg(task_id="A")) is None + assert consumer.process(_test_msg(task_id="B")) == "tr-2" assert env.trigger_run.call_count == 2 @@ -518,21 +469,17 @@ def test_multiple_failing_groups_trigger_one_run_per_task(env): groups = ["dom/base/test/mochitest.ini", "layout/test/mochitest.ini"] env.failing_groups.return_value = groups - assert consumer.process(_test_msg(), env.executor) == "tr-1" + assert consumer.process(_test_msg()) == "tr-1" # The whole task gets a single run; the agent investigates every failing group. env.trigger_run.assert_called_once() assert env.trigger_run.call_args.args[0]["failure_tasks"] == { "test-linux1804-64/opt-mochitest-browser-chrome-1": "TT" } - assert env.executor.submit.call_count == 1 - # Every failing group is named, not an arbitrary one of them. - _, ctx = env.executor.submit.call_args.args - assert ctx.test_groups == groups def test_missing_hg_revision_skips_test_task(env): env.get_task.return_value = _task_def(revision=None) - assert consumer.process(_test_msg(), env.executor) is None + assert consumer.process(_test_msg()) is None # Bail before doing the (network-heavy) group resolution. env.failing_groups.assert_not_called() env.trigger_run.assert_not_called() @@ -545,10 +492,8 @@ def test_every_failing_group_reaches_the_mozci_walk(env): "b/mochitest.ini" } - assert consumer.process(_test_msg(), env.executor) == "tr-1" + assert consumer.process(_test_msg()) == "tr-1" assert env.new_test_failures.call_args.args[3] == groups - _, ctx = env.executor.submit.call_args.args - assert ctx.test_groups == ["b/mochitest.ini"] def test_queue_name_includes_non_production_environment(): @@ -567,7 +512,7 @@ def test_classification_landing_during_the_check_cancels_the_run(env): # The regression check takes minutes, which is about how long Treeherder needs # to classify an intermittent; a verdict that arrives meanwhile must win. env.recheck_skip_reason.return_value = "intermittent" - assert consumer.process(_test_msg(), env.executor) is None + assert consumer.process(_test_msg()) is None env.trigger_run.assert_not_called() @@ -579,18 +524,15 @@ def test_recheck_happens_after_the_regression_check(env): )[1] env.recheck_skip_reason.side_effect = lambda p, t: order.append("recheck") - assert consumer.process(_test_msg(), env.executor) == "tr-1" + assert consumer.process(_test_msg()) == "tr-1" assert order == ["walk", "recheck"] def test_backfill_in_a_new_task_group_is_deduped(env): # Backfills and retriggers are dispatched by action tasks, which start their own # Taskcluster task group. The same push+group must still be investigated once. - assert consumer.process(_test_msg(task_id="A", group_id="G1"), env.executor) - assert ( - consumer.process(_test_msg(task_id="B", group_id="ACTION-GROUP"), env.executor) - is None - ) + assert consumer.process(_test_msg(task_id="A", group_id="G1")) + assert consumer.process(_test_msg(task_id="B", group_id="ACTION-GROUP")) is None env.trigger_run.assert_called_once() @@ -598,9 +540,9 @@ def test_different_pushes_are_not_deduped(env): # Dedupe is per push: a different manifest newly failing on a later push is a # separate regression and must be investigated again. _consecutive_pushes(env, "rev-one", "rev-two") - consumer.process(_test_msg(task_id="A"), env.executor) + consumer.process(_test_msg(task_id="A")) env.failing_groups.return_value = ["other/test/mochitest.ini"] - consumer.process(_test_msg(task_id="B"), env.executor) + consumer.process(_test_msg(task_id="B")) assert env.trigger_run.call_count == 2 @@ -611,7 +553,7 @@ def test_verdict_is_awaited_before_resolving_groups(env): env.await_skip_reason.side_effect = lambda p, t, j: order.append("await") env.failing_groups.side_effect = lambda *_: (order.append("groups"), [_GROUP])[1] - assert consumer.process(_test_msg(), env.executor) == "tr-1" + assert consumer.process(_test_msg()) == "tr-1" assert order == ["await", "groups"] @@ -619,7 +561,7 @@ def test_the_verdict_is_awaited_once_on_every_path(env): # A task with no group results used to run its own second wait; the up-front one # covers it, and waiting twice would double the delay before a real repair. env.failing_groups.side_effect = consumer.treeherder.GroupResultsUnavailable("none") - assert consumer.process(_test_msg(), env.executor) == "tr-1" + assert consumer.process(_test_msg()) == "tr-1" env.await_skip_reason.assert_called_once() @@ -627,14 +569,14 @@ def test_group_less_intermittent_is_dropped_by_the_up_front_gate(env): # The only filter such a failure gets, since it has no manifest to compare. env.await_skip_reason.return_value = "intermittent" env.failing_groups.side_effect = consumer.treeherder.GroupResultsUnavailable("none") - assert consumer.process(_test_msg(), env.executor) is None + assert consumer.process(_test_msg()) is None env.trigger_run.assert_not_called() def test_the_job_is_passed_to_the_verdict_wait(env): # The wait needs the ingested job: it is the verdict as of ingestion, and without # it the wait cannot tell "not classified yet" from "never ingested". - assert consumer.process(_test_msg(), env.executor) == "tr-1" + assert consumer.process(_test_msg()) == "tr-1" assert env.await_skip_reason.call_args.args[2] is env.job_for_task.return_value @@ -642,7 +584,7 @@ def test_backfilled_test_task_is_skipped(env): # Same rule as the build path: a backfill or retrigger re-runs work the push # already scheduled, so it is not a new failure to investigate. env.get_task.return_value = _task_def(parent="ACTION-CALLBACK") - assert consumer.process(_test_msg(), env.executor) is None + assert consumer.process(_test_msg()) is None env.failing_groups.assert_not_called() env.trigger_run.assert_not_called() @@ -651,7 +593,7 @@ def test_stale_push_skips_a_test_failure(env, fresh_push): # A test failure surfacing days after its push is not worth repairing either, # and the check must precede the ancestor walk, which can block for an hour. fresh_push.return_value = True - assert consumer.process(_test_msg(), env.executor) is None + assert consumer.process(_test_msg()) is None fresh_push.assert_called_once_with( "autoland", "hgrev", consumer.settings.max_push_age_hours * 3600 ) @@ -666,8 +608,8 @@ def test_group_less_task_claims_the_push_for_every_path(env): consumer.treeherder.GroupResultsUnavailable("none"), [_GROUP], ] - assert consumer.process(_test_msg(task_id="A"), env.executor) == "tr-1" - assert consumer.process(_test_msg(task_id="B"), env.executor) is None + assert consumer.process(_test_msg(task_id="A")) == "tr-1" + assert consumer.process(_test_msg(task_id="B")) is None env.trigger_run.assert_called_once() @@ -677,8 +619,8 @@ def test_manifest_failure_claims_the_push_against_a_group_less_task(env): [_GROUP], consumer.treeherder.GroupResultsUnavailable("none"), ] - assert consumer.process(_test_msg(task_id="A"), env.executor) == "tr-1" - assert consumer.process(_test_msg(task_id="B"), env.executor) is None + assert consumer.process(_test_msg(task_id="A")) == "tr-1" + assert consumer.process(_test_msg(task_id="B")) is None env.trigger_run.assert_called_once() @@ -692,13 +634,13 @@ def test_a_rejected_task_is_logged_with_a_treeherder_link(env, caplog): # The reason to log links at all: every verdict must be checkable in the UI. env.await_skip_reason.return_value = "intermittent" with caplog.at_level(logging.INFO, logger="app.consumer"): - assert consumer.process(_test_msg(), env.executor) is None + assert consumer.process(_test_msg()) is None assert _LINK in caplog.text def test_a_triggered_run_is_logged_with_a_treeherder_link(env, caplog): with caplog.at_level(logging.INFO, logger="app.consumer"): - assert consumer.process(_test_msg(), env.executor) == "tr-1" + assert consumer.process(_test_msg()) == "tr-1" assert _LINK in caplog.text @@ -707,7 +649,7 @@ def test_an_action_scheduled_task_is_logged_with_a_treeherder_link(env, caplog): # the ordering change exists to cover. env.get_task.return_value = _task_def(parent="ACTION-CALLBACK") with caplog.at_level(logging.INFO, logger="app.consumer"): - assert consumer.process(_test_msg(), env.executor) is None + assert consumer.process(_test_msg()) is None assert _LINK in caplog.text @@ -717,9 +659,9 @@ def test_runs_stop_at_the_daily_limit(env, monkeypatch): env.get_task.side_effect = lambda task_id: _task_def(next(revisions)) _distinct_groups(env) - assert consumer.process(_test_msg(task_id="A"), env.executor) == "tr-1" - assert consumer.process(_test_msg(task_id="B"), env.executor) == "tr-1" - assert consumer.process(_test_msg(task_id="C"), env.executor) is None + assert consumer.process(_test_msg(task_id="A")) == "tr-1" + assert consumer.process(_test_msg(task_id="B")) == "tr-1" + assert consumer.process(_test_msg(task_id="C")) is None assert env.trigger_run.call_count == 2 @@ -730,9 +672,9 @@ def test_the_limit_is_a_rolling_window(env, monkeypatch): env.get_task.side_effect = lambda task_id: _task_def(next(revisions)) _distinct_groups(env) - assert consumer.process(_test_msg(task_id="A"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="A")) == "tr-1" consumer._test_run_times[0] -= consumer._RATE_WINDOW_SECONDS + 1 - assert consumer.process(_test_msg(task_id="B"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="B")) == "tr-1" def test_a_spent_budget_stops_before_any_treeherder_work(env, monkeypatch): @@ -740,10 +682,10 @@ def test_a_spent_budget_stops_before_any_treeherder_work(env, monkeypatch): revisions = iter(["rev-1", "rev-2"]) env.get_task.side_effect = lambda task_id: _task_def(next(revisions)) - assert consumer.process(_test_msg(task_id="A"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="A")) == "tr-1" env.job_for_task.reset_mock() env.failing_groups.reset_mock() - assert consumer.process(_test_msg(task_id="B"), env.executor) is None + assert consumer.process(_test_msg(task_id="B")) is None env.job_for_task.assert_not_called() env.failing_groups.assert_not_called() @@ -755,8 +697,8 @@ def test_a_failed_trigger_gives_its_slot_back(env, monkeypatch): revisions = iter(["rev-1", "rev-2"]) env.get_task.side_effect = lambda task_id: _task_def(next(revisions)) - assert consumer.process(_test_msg(task_id="A"), env.executor) is None - assert consumer.process(_test_msg(task_id="B"), env.executor) == "tr-2" + assert consumer.process(_test_msg(task_id="A")) is None + assert consumer.process(_test_msg(task_id="B")) == "tr-2" def test_a_budget_blocked_task_does_not_claim_its_push(env, monkeypatch): @@ -767,10 +709,10 @@ def test_a_budget_blocked_task_does_not_claim_its_push(env, monkeypatch): env.get_task.side_effect = lambda task_id: _task_def(next(revisions)) env.failing_groups.side_effect = [[_GROUP], ["other/mochitest.ini"]] * 2 - assert consumer.process(_test_msg(task_id="A"), env.executor) == "tr-1" - assert consumer.process(_test_msg(task_id="B"), env.executor) is None + assert consumer.process(_test_msg(task_id="A")) == "tr-1" + assert consumer.process(_test_msg(task_id="B")) is None monkeypatch.setattr(consumer.settings, "max_test_repairs_per_day", 2) - assert consumer.process(_test_msg(task_id="B2"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="B2")) == "tr-1" def test_the_limit_does_not_apply_to_build_repair(env, monkeypatch): @@ -782,14 +724,14 @@ def test_the_limit_does_not_apply_to_build_repair(env, monkeypatch): patch.object(consumer.lando, "hg_to_git", return_value="gitH"), patch.object(consumer.client, "trigger_run", return_value="br-1") as trigger, ): - assert consumer.process(_build_msg(), env.executor) == "br-1" + assert consumer.process(_build_msg()) == "br-1" trigger.assert_called_once() def test_exhausting_the_budget_is_logged_once(env, monkeypatch, caplog): monkeypatch.setattr(consumer.settings, "max_test_repairs_per_day", 1) with caplog.at_level(logging.WARNING, logger="app.consumer"): - assert consumer.process(_test_msg(task_id="A"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="A")) == "tr-1" assert ( sum( "budget of 1 runs per 24h is now spent" in r.message for r in caplog.records @@ -817,7 +759,7 @@ def claim_meanwhile(project, rev, config, groups, abort=None): return set(groups) env.new_test_failures.side_effect = claim_meanwhile - assert consumer.process(_test_msg(task_id="A"), env.executor) is None + assert consumer.process(_test_msg(task_id="A")) is None env.trigger_run.assert_not_called() assert len(consumer._test_run_times) == 0 @@ -830,7 +772,7 @@ def test_source_test_tasks_are_not_routed_to_test_repair(env): body["task"]["tags"]["label"] = "source-test-node-newtab-unit-tests" body["task"]["tags"].pop("test-suite", None) - assert consumer.process(body, env.executor) is None + assert consumer.process(body) is None env.get_task.assert_not_called() env.trigger_run.assert_not_called() @@ -839,7 +781,7 @@ def test_group_less_task_inherited_from_an_ancestor_is_skipped(env): env.failing_groups.side_effect = consumer.treeherder.GroupResultsUnavailable("none") env.is_new_task_failure.return_value = False - assert consumer.process(_test_msg(), env.executor) is None + assert consumer.process(_test_msg()) is None env.trigger_run.assert_not_called() assert env.is_new_task_failure.call_args.args[:3] == ( "autoland", @@ -852,7 +794,7 @@ def test_group_less_task_new_at_this_push_still_triggers(env): env.failing_groups.side_effect = consumer.treeherder.GroupResultsUnavailable("none") env.is_new_task_failure.return_value = True - assert consumer.process(_test_msg(), env.executor) == "tr-1" + assert consumer.process(_test_msg()) == "tr-1" env.trigger_run.assert_called_once() @@ -861,7 +803,7 @@ def test_an_unreadable_group_lookup_does_not_wait_on_an_ancestor(env): # the same broken API, so that failure still runs the agent outright. env.failing_groups.side_effect = RuntimeError("treeherder down") - assert consumer.process(_test_msg(), env.executor) == "tr-1" + assert consumer.process(_test_msg()) == "tr-1" env.is_new_task_failure.assert_not_called() @@ -874,7 +816,7 @@ def test_group_less_suites_are_investigated_as_a_whole_task(env): body = _test_msg() body["task"]["tags"]["label"] = "test-macosx1500-aarch64/debug-gtest-1proc" - assert consumer.process(body, env.executor) == "tr-1" + assert consumer.process(body) == "tr-1" env.is_new_task_failure.assert_called_once() @@ -882,7 +824,7 @@ def test_manifest_suites_are_still_investigated(env): # The guard must not swallow a suite that does report manifests. body = _test_msg() body["task"]["tags"]["label"] = "test-linux2404-64/debug-mochitest-browser-chrome-7" - assert consumer.process(body, env.executor) == "tr-1" + assert consumer.process(body) == "tr-1" def test_a_walk_is_abandoned_once_the_push_is_claimed(env): @@ -896,7 +838,7 @@ def walk(project, rev, config, groups, should_abort=None): raise consumer.regression.WalkAborted("group at rev") env.new_test_failures.side_effect = walk - assert consumer.process(_test_msg(), env.executor) is None + assert consumer.process(_test_msg()) is None assert aborted["stopped"] is True env.trigger_run.assert_not_called() @@ -906,7 +848,7 @@ def test_an_abandoned_walk_does_not_look_inherited(env, caplog): # about the failure rather than "we stopped asking". env.new_test_failures.side_effect = consumer.regression.WalkAborted("group at rev") with caplog.at_level(logging.INFO, logger="app.consumer"): - assert consumer.process(_test_msg(), env.executor) is None + assert consumer.process(_test_msg()) is None assert "abandoning the check" in caplog.text assert "inherited" not in caplog.text assert "No new, non-intermittent groups" not in caplog.text @@ -915,7 +857,7 @@ def test_an_abandoned_walk_does_not_look_inherited(env, caplog): def test_the_group_less_walk_is_also_abandoned(env): env.failing_groups.side_effect = consumer.treeherder.GroupResultsUnavailable("none") env.is_new_task_failure.side_effect = consumer.regression.WalkAborted("task at rev") - assert consumer.process(_test_msg(), env.executor) is None + assert consumer.process(_test_msg()) is None env.trigger_run.assert_not_called() @@ -925,7 +867,7 @@ def _known_intermittent(*bug_ids): def test_a_known_intermittent_bug_skips_before_waiting_for_a_verdict(env): env.intermittent_match.return_value = _known_intermittent(2016093) - assert consumer.process(_test_msg(), env.executor) is None + assert consumer.process(_test_msg()) is None env.await_skip_reason.assert_not_called() env.failing_groups.assert_not_called() env.trigger_run.assert_not_called() @@ -934,13 +876,13 @@ def test_a_known_intermittent_bug_skips_before_waiting_for_a_verdict(env): def test_the_skipped_bug_is_logged(env, caplog): env.intermittent_match.return_value = _known_intermittent(2016093) with caplog.at_level(logging.INFO, logger="app.consumer"): - assert consumer.process(_test_msg(), env.executor) is None + assert consumer.process(_test_msg()) is None assert "2016093" in caplog.text assert _LINK in caplog.text def test_the_ingested_job_is_what_the_gate_reads(env): - assert consumer.process(_test_msg(), env.executor) == "tr-1" + assert consumer.process(_test_msg()) == "tr-1" assert env.intermittent_match.call_args.args == ( "autoland", env.job_for_task.return_value, @@ -952,13 +894,13 @@ def test_a_known_intermittent_does_not_claim_its_push(env): _known_intermittent(2016093), consumer.treeherder.IntermittentMatch(), ] - assert consumer.process(_test_msg(task_id="A"), env.executor) is None - assert consumer.process(_test_msg(task_id="B"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="A")) is None + assert consumer.process(_test_msg(task_id="B")) == "tr-1" def test_no_intermittent_match_still_runs(env): env.intermittent_match.return_value = consumer.treeherder.IntermittentMatch() - assert consumer.process(_test_msg(), env.executor) == "tr-1" + assert consumer.process(_test_msg()) == "tr-1" def _consecutive_pushes(env, *revisions): @@ -969,25 +911,25 @@ def _consecutive_pushes(env, *revisions): def test_the_same_manifest_on_later_pushes_is_deduped(env): _consecutive_pushes(env, "rev-one", "rev-two", "rev-three") - assert consumer.process(_test_msg(task_id="A"), env.executor) == "tr-1" - assert consumer.process(_test_msg(task_id="B"), env.executor) is None - assert consumer.process(_test_msg(task_id="C"), env.executor) is None + assert consumer.process(_test_msg(task_id="A")) == "tr-1" + assert consumer.process(_test_msg(task_id="B")) is None + assert consumer.process(_test_msg(task_id="C")) is None env.trigger_run.assert_called_once() def test_a_new_manifest_on_a_later_push_still_runs(env): _consecutive_pushes(env, "rev-one", "rev-two") - assert consumer.process(_test_msg(task_id="A"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="A")) == "tr-1" env.failing_groups.return_value = ["layout/style/test/mochitest.toml"] - assert consumer.process(_test_msg(task_id="B"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="B")) == "tr-1" assert env.trigger_run.call_count == 2 def test_one_unseen_manifest_is_enough_to_run(env): _consecutive_pushes(env, "rev-one", "rev-two") - assert consumer.process(_test_msg(task_id="A"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="A")) == "tr-1" env.failing_groups.return_value = [_GROUP, "layout/style/test/mochitest.toml"] - assert consumer.process(_test_msg(task_id="B"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="B")) == "tr-1" def test_manifest_dedupe_is_per_project(): @@ -999,38 +941,38 @@ def test_manifest_dedupe_is_per_project(): def test_a_skipped_task_does_not_claim_its_manifests(env): _consecutive_pushes(env, "rev-one", "rev-two") env.await_skip_reason.side_effect = ["intermittent", None] - assert consumer.process(_test_msg(task_id="A"), env.executor) is None - assert consumer.process(_test_msg(task_id="B"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="A")) is None + assert consumer.process(_test_msg(task_id="B")) == "tr-1" def test_a_failed_trigger_releases_the_manifests(env): _consecutive_pushes(env, "rev-one", "rev-two") env.trigger_run.side_effect = [RuntimeError("boom"), "tr-2"] - assert consumer.process(_test_msg(task_id="A"), env.executor) is None - assert consumer.process(_test_msg(task_id="B"), env.executor) == "tr-2" + assert consumer.process(_test_msg(task_id="A")) is None + assert consumer.process(_test_msg(task_id="B")) == "tr-2" def test_a_group_less_task_is_not_suppressed_by_the_manifest_cache(env): _consecutive_pushes(env, "rev-one", "rev-two") - assert consumer.process(_test_msg(task_id="A"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="A")) == "tr-1" env.failing_groups.side_effect = consumer.treeherder.GroupResultsUnavailable("none") - assert consumer.process(_test_msg(task_id="B"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="B")) == "tr-1" def test_the_deduped_task_is_logged_with_a_treeherder_link(env, caplog): _consecutive_pushes(env, "rev-one", "hgrev") - assert consumer.process(_test_msg(task_id="A"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="A")) == "tr-1" with caplog.at_level(logging.INFO, logger="app.consumer"): - assert consumer.process(_test_msg(task_id="TT"), env.executor) is None + assert consumer.process(_test_msg(task_id="TT")) is None assert "already investigated on a recent push" in caplog.text assert _LINK in caplog.text def test_a_manifest_dedupe_costs_no_recheck(env): _consecutive_pushes(env, "rev-one", "rev-two") - assert consumer.process(_test_msg(task_id="A"), env.executor) == "tr-1" + assert consumer.process(_test_msg(task_id="A")) == "tr-1" env.recheck_skip_reason.reset_mock() - assert consumer.process(_test_msg(task_id="B"), env.executor) is None + assert consumer.process(_test_msg(task_id="B")) is None env.recheck_skip_reason.assert_not_called() @@ -1043,19 +985,19 @@ def _distinct_groups(env): def test_test_verify_tasks_are_skipped(env): label = "test-linux2404-64/opt-test-verify" - assert consumer.process(_test_msg(label=label), env.executor) is None + assert consumer.process(_test_msg(label=label)) is None env.job_for_task.assert_not_called() env.trigger_run.assert_not_called() def test_a_chunked_test_verify_task_is_skipped(env): label = "test-linux64/opt-test-verify-wpt-1" - assert consumer.process(_test_msg(label=label), env.executor) is None + assert consumer.process(_test_msg(label=label)) is None env.trigger_run.assert_not_called() def test_an_ordinary_task_is_not_mistaken_for_test_verify(env): - assert consumer.process(_test_msg(), env.executor) == "tr-1" + assert consumer.process(_test_msg()) == "tr-1" def _build_env(monkeypatch, reason=None, introduced=True): @@ -1068,12 +1010,12 @@ def _build_env(monkeypatch, reason=None, introduced=True): consumer.treeherder, "recheck_skip_reason", MagicMock(return_value=reason) ) monkeypatch.setattr(consumer.client, "trigger_run", trigger) - return SimpleNamespace(trigger=trigger, walk=walk, executor=MagicMock()) + return SimpleNamespace(trigger=trigger, walk=walk) def test_an_infra_build_failure_is_skipped(monkeypatch): env = _build_env(monkeypatch, reason="infra") - assert consumer.process(_build_msg(), env.executor) is None + assert consumer.process(_build_msg()) is None env.trigger.assert_not_called() @@ -1081,10 +1023,10 @@ def test_a_classified_build_failure_is_read_after_the_walk(monkeypatch): # Before the walk it would cost the ingest and classification waits the test # path pays; after it, Treeherder has had minutes and it is one request. env = _build_env(monkeypatch, reason="intermittent") - assert consumer.process(_build_msg(), env.executor) is None + assert consumer.process(_build_msg()) is None env.walk.assert_called_once() def test_an_unclassified_build_failure_still_runs(monkeypatch): - env = _build_env(monkeypatch) - assert consumer.process(_build_msg(), env.executor) == "run-1" + _build_env(monkeypatch) + assert consumer.process(_build_msg()) == "run-1" diff --git a/services/hackbot-pulse-listener/tests/test_github.py b/services/hackbot-pulse-listener/tests/test_github.py deleted file mode 100644 index 8b592119da..0000000000 --- a/services/hackbot-pulse-listener/tests/test_github.py +++ /dev/null @@ -1,32 +0,0 @@ -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 deleted file mode 100644 index f7ff0b9d99..0000000000 --- a/services/hackbot-pulse-listener/tests/test_notify.py +++ /dev/null @@ -1,629 +0,0 @@ -import base64 -from unittest.mock import MagicMock, patch - -from app import notify -from app.models import RunContext - - -def _ctx(**over): - base = dict( - run_id="run-1", - repo="autoland", - git_commit="deadbeefcafe", - hg_revision="0123456789ab", - task_id="TASK123", - developer_email="dev@mozilla.com", - ) - base.update(over) - return RunContext(**base) - - -def _test_repair_ctx(**over): - over.setdefault("test_groups", ["dom/base/test/mochitest.ini"]) - return _ctx(agent="test-repair", **over) - - -def test_skips_without_recipient(): - # No developer, no team, no override -> nothing to send, must not raise. - notify.send_email(_ctx(developer_email=None), {"status": "succeeded"}) - - -def test_skips_without_sendgrid_config(monkeypatch): - monkeypatch.setattr(notify.settings, "sendgrid_api_key", None) - monkeypatch.setattr(notify.settings, "notification_sender", None) - notify.send_email(_ctx(), {"status": "succeeded"}) - - -def test_skips_when_not_succeeded(monkeypatch): - monkeypatch.setattr(notify.settings, "sendgrid_api_key", "key") - monkeypatch.setattr(notify.settings, "notification_sender", "from@mozilla.com") - with patch("sendgrid.SendGridAPIClient") as sg: - notify.send_email(_ctx(), {"status": "failed"}) - sg.assert_not_called() - - -def test_body_contains_source_links(): - body = notify._build_body(_ctx(), {"status": "succeeded", "summary": {}}) - assert "https://github.com/mozilla-firefox/firefox/commit/deadbeefcafe" in body - assert "https://hg.mozilla.org/mozilla-unified/rev/0123456789ab" in body - 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) - assert "https://bugzilla.mozilla.org/show_bug.cgi?id=12345" in body - - no_bug = notify._build_body(_ctx(), {"status": "succeeded", "summary": {}}) - assert "show_bug.cgi" not in no_bug - - -def test_body_contains_ui_link_and_summary(monkeypatch): - monkeypatch.setattr(notify.settings, "hackbot_ui_url", "https://ui.example/") - body = notify._build_body( - _ctx(), - { - "status": "succeeded", - "summary": { - "findings": { - "summary": "Fixed a missing include", - "analysis": "The commit removed a needed header", - "local_build_verified": True, - } - }, - }, - ) - assert "https://ui.example/runs/run-1" in body - assert "Fixed a missing include" in body - assert "The commit removed a needed header" in body - assert "Local build verified: True" in body - - -def test_body_includes_patch(): - body = notify._build_body( - _ctx(), - {"status": "succeeded", "summary": {}}, - patch="--- a/f\n+++ b/f\n@@ -1 +1 @@\n-old\n+new\n", - ) - assert "## Proposed patch" in body - assert "```diff" in body - assert "+new" in body - - -def test_analysis_headings_demoted_under_section(): - run_doc = { - "status": "succeeded", - "summary": {"findings": {"analysis": "# Root cause\n\n## Details\ntext"}}, - } - body = notify._build_body(_ctx(), run_doc) - assert "## Analysis" in body - assert "### Root cause" in body - assert "#### Details" in body - - -def test_demote_headings_leaves_code_fences_and_includes_alone(): - md = "```cpp\n#include \n```" - assert notify._demote_headings(md) == md - - -def test_sends_email_when_configured(monkeypatch): - monkeypatch.setattr(notify.settings, "sendgrid_api_key", "key") - monkeypatch.setattr(notify.settings, "notification_sender", "from@mozilla.com") - monkeypatch.setattr(notify.settings, "notify_only_with_patch", False) - - fake_client = MagicMock() - fake_client.send.return_value = MagicMock(status_code=202) - with patch("sendgrid.SendGridAPIClient", return_value=fake_client): - notify.send_email(_ctx(), {"status": "succeeded", "summary": {}}) - - fake_client.send.assert_called_once() - - -def test_override_sends_even_without_developer_email(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", "me@mozilla.com" - ) - monkeypatch.setattr(notify.settings, "notify_only_with_patch", False) - - fake_client = MagicMock() - fake_client.send.return_value = MagicMock(status_code=202) - with patch("sendgrid.SendGridAPIClient", return_value=fake_client): - notify.send_email( - _ctx(developer_email=None), {"status": "succeeded", "summary": {}} - ) - - fake_client.send.assert_called_once() - - -def test_skips_when_no_patch_and_notify_only_with_patch(monkeypatch): - monkeypatch.setattr(notify.settings, "sendgrid_api_key", "key") - monkeypatch.setattr(notify.settings, "notification_sender", "from@mozilla.com") - monkeypatch.setattr(notify.settings, "notify_only_with_patch", True) - - with patch("sendgrid.SendGridAPIClient") as sg: - notify.send_email(_ctx(), {"status": "succeeded", "summary": {}}) - sg.assert_not_called() - - -def test_sends_without_patch_when_gate_disabled(monkeypatch): - monkeypatch.setattr(notify.settings, "sendgrid_api_key", "key") - monkeypatch.setattr(notify.settings, "notification_sender", "from@mozilla.com") - monkeypatch.setattr(notify.settings, "notify_only_with_patch", False) - - fake_client = MagicMock() - fake_client.send.return_value = MagicMock(status_code=202) - with patch("sendgrid.SendGridAPIClient", return_value=fake_client): - notify.send_email(_ctx(), {"status": "succeeded", "summary": {}}) - - fake_client.send.assert_called_once() - - -def test_fetch_patch_returns_none_without_artifact(): - assert notify._fetch_patch("run-1", {"artifacts": []}) is None - - -def test_fetch_patch_downloads_listed_artifact(): - run_doc = {"artifacts": [{"name": notify.PATCH_ARTIFACT}]} - with patch.object(notify.client, "get_artifact", return_value="THE PATCH") as ga: - assert notify._fetch_patch("run-1", run_doc) == "THE PATCH" - ga.assert_called_once_with("run-1", notify.PATCH_ARTIFACT) - - -def test_recipients_author_and_team(monkeypatch): - monkeypatch.setattr(notify.settings, "notification_override_email", None) - monkeypatch.setattr(notify.settings, "notification_team_email", "team@mozilla.com") - assert notify._recipients("dev@mozilla.com") == [ - "dev@mozilla.com", - "team@mozilla.com", - ] - - -def test_recipients_override_wins(monkeypatch): - monkeypatch.setattr( - notify.settings, "notification_override_email", "me@mozilla.com" - ) - monkeypatch.setattr(notify.settings, "notification_team_email", "team@mozilla.com") - assert notify._recipients("dev@mozilla.com") == ["me@mozilla.com"] - - -def test_recipients_dedupes_and_skips_empty(monkeypatch): - monkeypatch.setattr(notify.settings, "notification_override_email", None) - monkeypatch.setattr(notify.settings, "notification_team_email", "dev@mozilla.com") - assert notify._recipients("dev@mozilla.com") == ["dev@mozilla.com"] - monkeypatch.setattr(notify.settings, "notification_team_email", None) - assert notify._recipients(None) == [] - - -def _test_repair_findings(**over): - base = { - "classification": "regression", - "recommendation": "backout", - "culprit_commit": "abc123def456", - "confidence": 0.8, - "last_green_revision": "green99", - "summary": "A landed commit removed a null check.", - "analysis": "# Root cause\nThe diff dropped validation.", - } - base.update(over) - return base - - -def test_test_repair_body_leads_with_recommendation(): - body = notify._build_test_repair_body( - _test_repair_ctx(), _test_repair_findings(), None, "culprit@mozilla.com" - ) - assert "Test failure analysis" in body - assert "BACK OUT the culprit" in body - assert "dom/base/test/mochitest.ini" in body - assert "abc123def456"[:12] in body - assert "by culprit@mozilla.com" in body - assert "green99" in body - assert "## Analysis" in body - - -def test_test_repair_body_names_every_failing_group(): - ctx = _test_repair_ctx( - test_groups=["dom/base/test/mochitest.ini", "layout/test/mochitest.ini"] - ) - body = notify._build_test_repair_body(ctx, _test_repair_findings(), None, None) - assert "dom/base/test/mochitest.ini" in body - assert "layout/test/mochitest.ini" in body - - -def test_test_repair_subject_summarizes_multiple_groups(): - ctx = _test_repair_ctx(test_groups=["a/mochitest.ini", "b/mochitest.ini"]) - assert notify._test_groups_label(ctx) == "a/mochitest.ini (+1 more)" - assert ( - notify._test_groups_label(_test_repair_ctx()) == "dom/base/test/mochitest.ini" - ) - assert notify._test_groups_label(_test_repair_ctx(test_groups=[])) == "task TASK123" - - -def test_test_repair_body_omits_unmapped_git_revision(): - # An unmapped revision must not render an empty commit link. - body = notify._build_test_repair_body( - _test_repair_ctx(git_commit=""), _test_repair_findings(), None, None - ) - assert "Revision (git)" not in body - assert "firefox/commit/)" not in body - assert "Revision (hg)" in body - - -def test_test_repair_intermittent_body_says_do_not_backout(): - findings = _test_repair_findings( - classification="intermittent", - recommendation="do_not_backout", - culprit_commit=None, - ) - body = notify._build_test_repair_body(_test_repair_ctx(), findings, None, None) - assert "DO NOT back out" in body - assert "Culprit commit" not in body - - -def test_test_repair_recipients_are_the_team_alone(monkeypatch): - monkeypatch.setattr(notify.settings, "notification_override_email", None) - monkeypatch.setattr(notify.settings, "notification_team_email", "team@mozilla.com") - assert notify._recipients(notify.settings.notification_team_email) == [ - "team@mozilla.com" - ] - - -def test_test_repair_recipients_override_wins(monkeypatch): - monkeypatch.setattr( - notify.settings, "notification_override_email", "me@mozilla.com" - ) - monkeypatch.setattr(notify.settings, "notification_team_email", "team@mozilla.com") - assert notify._recipients(notify.settings.notification_team_email) == [ - "me@mozilla.com" - ] - - -def test_test_repair_intermittent_sends_without_patch(monkeypatch): - # No patch, notify_only_with_patch True -> test-repair still sends (unlike - # build-repair), and an intermittent verdict is mailed like any other: the team - # tracks every verdict, only the sheriff-facing Slack post is filtered. - monkeypatch.setattr(notify.settings, "sendgrid_api_key", "key") - monkeypatch.setattr(notify.settings, "notification_sender", "from@mozilla.com") - monkeypatch.setattr(notify.settings, "notify_only_with_patch", True) - monkeypatch.setattr(notify.settings, "notification_override_email", None) - monkeypatch.setattr(notify.settings, "notification_team_email", "team@mozilla.com") - - run_doc = { - "status": "succeeded", - "summary": { - "findings": _test_repair_findings( - classification="intermittent", - recommendation="do_not_backout", - culprit_commit=None, - ) - }, - } - fake_client = MagicMock() - fake_client.send.return_value = MagicMock(status_code=202) - with patch("sendgrid.SendGridAPIClient", return_value=fake_client): - notify.send_email(_test_repair_ctx(), run_doc) - - fake_client.send.assert_called_once() - personalizations = fake_client.send.call_args.kwargs["message"].get()[ - "personalizations" - ][0] - assert personalizations["to"] == [{"email": "team@mozilla.com"}] - - -def test_test_repair_never_mails_the_culprit_author(monkeypatch): - # A verdict goes to the team, never to the developer the agent blamed, even when - # it is confident enough to name one. - 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", "team@mozilla.com") - - run_doc = {"status": "succeeded", "summary": {"findings": _test_repair_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="culprit@mozilla.com" - ), - ): - notify.send_email(_test_repair_ctx(), run_doc) - - message = fake_client.send.call_args.kwargs["message"].get() - personalizations = message["personalizations"][0] - assert personalizations["to"] == [{"email": "team@mozilla.com"}] - assert "cc" not in personalizations - # Still named in the body, which is the point of resolving it at all. - assert "culprit@mozilla.com" in message["content"][0]["value"] - - -def test_test_repair_skips_without_a_team_address(monkeypatch): - # The team address is the only recipient, so without it there is nobody to mail. - 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) - - run_doc = {"status": "succeeded", "summary": {"findings": _test_repair_findings()}} - with patch("sendgrid.SendGridAPIClient") as sg: - notify.send_email(_test_repair_ctx(), run_doc) - sg.assert_not_called() - - -def test_test_repair_ignores_the_pushing_developer(monkeypatch): - # ctx.developer_email is the push author; only build-repair mails them. - 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", "team@mozilla.com") - - ctx = _test_repair_ctx(developer_email="pusher@mozilla.com") - run_doc = {"status": "succeeded", "summary": {"findings": _test_repair_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) - - personalizations = fake_client.send.call_args.kwargs["message"].get()[ - "personalizations" - ][0] - assert personalizations["to"] == [{"email": "team@mozilla.com"}] - assert "cc" not in personalizations - - -def test_attaches_patch_file(monkeypatch): - monkeypatch.setattr(notify.settings, "sendgrid_api_key", "key") - monkeypatch.setattr(notify.settings, "notification_sender", "from@mozilla.com") - run_doc = { - "status": "succeeded", - "artifacts": [{"name": notify.PATCH_ARTIFACT}], - "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.client, "get_artifact", return_value="DIFF-CONTENT"), - ): - notify.send_email(_ctx(), run_doc) - - attachments = fake_client.send.call_args.kwargs["message"].get()["attachments"] - assert len(attachments) == 1 - assert attachments[0]["filename"] == "changes.patch" - assert base64.b64decode(attachments[0]["content"]).decode() == "DIFF-CONTENT" - - -def test_the_headline_names_the_sheriffs_action_not_a_landing(): - findings = _test_repair_findings(recommendation="land_fix") - body = notify._build_test_repair_body(_test_repair_ctx(), findings, None, None) - assert "LAND the proposed fix" not in body - assert "BACK OUT the culprit, reland with the proposed fix squashed in" in body - - -def test_the_patch_is_presented_as_advice_for_a_squashed_reland(): - findings = _test_repair_findings(recommendation="land_fix") - body = notify._build_test_repair_body( - _test_repair_ctx(), findings, "--- a/f\n+++ b/f\n", None - ) - assert "squash this into your existing patches and reland" in body - assert "not a follow-up to land on its own" in body - - -def test_no_patch_advice_without_a_patch(): - body = notify._build_test_repair_body( - _test_repair_ctx(), _test_repair_findings(), None, None - ) - assert "squash this into your existing patches" not in body - - -def test_no_team_footer_when_the_team_is_the_recipient(monkeypatch): - monkeypatch.setattr(notify.settings, "notification_team_email", "team@mozilla.com") - body = notify._build_test_repair_body( - _test_repair_ctx(), _test_repair_findings(), None, None - ) - assert "reaches the hackbot team" not in body - - -def test_an_unknown_recommendation_is_shown_verbatim(): - assert notify._banner({"recommendation": "backout_and_reland"}) == ( - "backout_and_reland" - ) - assert notify._banner({}) == "analysis" - - -def test_already_actioned_body_leads_with_the_banner(): - body = notify._build_test_repair_body( - _test_repair_ctx(), - _test_repair_findings(), - None, - None, - already_actioned="fixed by commit", - ) - banner, _ = body.split("# Test failure analysis", 1) - assert "Already actioned by a sheriff" in banner - assert "fixed by commit" in banner - assert "BACK OUT the culprit" in body - assert "## Analysis" in body - - -def test_an_unactioned_body_has_no_banner(): - body = notify._build_test_repair_body( - _test_repair_ctx(), _test_repair_findings(), None, None - ) - assert "Already actioned" not in body - assert body.startswith("# Test failure analysis") - - -def test_already_actioned_is_marked_in_the_subject(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", "me@mozilla.com" - ) - - run_doc = {"status": "succeeded", "summary": {"findings": _test_repair_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(_test_repair_ctx(), run_doc, "fixed by commit") - - message = fake_client.send.call_args.kwargs["message"].get() - assert message["subject"].startswith("[test-repair] [already actioned] ") - assert "Already actioned by a sheriff" in message["content"][0]["value"] - - -def test_build_repair_ignores_the_actioned_flag(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", "me@mozilla.com" - ) - monkeypatch.setattr(notify.settings, "notify_only_with_patch", False) - - 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): - notify.send_email(_ctx(), run_doc, "fixed by commit") - - message = fake_client.send.call_args.kwargs["message"].get() - assert "already actioned" not in message["subject"] - - -def _pre_existing_doc(): - return { - "status": "succeeded", - "summary": {"findings": {"blamed_commit": None}}, - } - - -def test_a_cleared_push_blames_nobody(): - body = notify._build_body(_ctx(), _pre_existing_doc()) - assert "Likely culprit" not in body - assert "Not caused by this push" in body - - -def test_a_cleared_push_does_not_claim_an_author_introduced_it(): - body = notify._build_body( - _ctx(), _pre_existing_doc(), blamed_author="innocent@mozilla.com" - ) - assert "introduced the failure" not in body - - -def test_no_verdict_is_not_reported_as_cleared(): - # An absent blamed_commit is missing data, not the agent clearing the push. - body = notify._build_body(_ctx(), {"status": "succeeded", "summary": {}}) - assert "Not caused by this push" not in body - assert "Likely culprit" not in body - - -def test_a_cleared_push_does_not_mail_an_author(monkeypatch): - monkeypatch.setattr(notify.settings, "sendgrid_api_key", "k") - 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) - author = MagicMock(return_value="innocent@mozilla.com") - monkeypatch.setattr(notify.github, "commit_author_email", author) - with patch.object(notify, "_deliver") as deliver: - notify.send_email(_ctx(), _pre_existing_doc()) - author.assert_not_called() - assert "innocent@mozilla.com" not in deliver.call_args.args[2] diff --git a/services/hackbot-pulse-listener/tests/test_worker.py b/services/hackbot-pulse-listener/tests/test_worker.py deleted file mode 100644 index 4c50306715..0000000000 --- a/services/hackbot-pulse-listener/tests/test_worker.py +++ /dev/null @@ -1,105 +0,0 @@ -from unittest.mock import patch - -import pytest -from app import worker -from app.models import RunContext - -CTX = RunContext( - run_id="run-1", - repo="autoland", - git_commit="deadbeef", - hg_revision="hg123", - task_id="T1", - developer_email="dev@mozilla.com", -) - - -@pytest.fixture(autouse=True) -def unactioned(): - """Keep the pre-notification re-check off the network; no sheriff acted.""" - with patch.object( - worker.treeherder, "recheck_skip_reason", return_value=None - ) as recheck: - yield recheck - - -def test_terminal_run_notifies_once(): - run_doc = {"status": "succeeded", "summary": {}} - with ( - patch.object(worker.client, "get_run", return_value=run_doc) as get_run, - patch.object(worker, "notify") as notify, - ): - worker.poll_and_notify(CTX) - - get_run.assert_called_once() - notify.send_email.assert_called_once_with(CTX, run_doc, None) - - -def test_gives_up_after_max_age(monkeypatch): - monkeypatch.setattr(worker.settings, "run_max_age_minutes", 0) - with ( - patch.object( - worker.client, "get_run", return_value={"status": "running"} - ) as get_run, - patch.object(worker, "notify") as notify, - ): - worker.poll_and_notify(CTX) - - get_run.assert_called_once() - notify.send_email.assert_not_called() - - -def test_a_late_sheriff_action_marks_the_notification(): - run_doc = {"status": "succeeded", "summary": {}} - with ( - patch.object(worker.client, "get_run", return_value=run_doc), - patch.object( - worker.treeherder, "recheck_skip_reason", return_value="fixed by commit" - ), - patch.object(worker, "notify") as notify, - ): - worker.poll_and_notify(CTX) - - notify.send_email.assert_called_once_with(CTX, run_doc, "fixed by commit") - - -def test_an_unactioned_failure_notifies_unmarked(): - run_doc = {"status": "succeeded", "summary": {}} - with ( - patch.object(worker.client, "get_run", return_value=run_doc), - patch.object(worker.treeherder, "recheck_skip_reason", return_value=None), - patch.object(worker, "notify") as notify, - ): - worker.poll_and_notify(CTX) - - notify.send_email.assert_called_once_with(CTX, run_doc, None) - - -def test_the_analysis_is_still_sent_after_a_sheriff_acted(): - run_doc = {"status": "succeeded", "summary": {}} - with ( - patch.object(worker.client, "get_run", return_value=run_doc), - patch.object( - worker.treeherder, "recheck_skip_reason", return_value="fixed by commit" - ), - patch.object(worker, "notify") as notify, - ): - worker.poll_and_notify(CTX) - - notify.send_email.assert_called_once() - - -def test_a_failed_recheck_does_not_block_the_notification(): - run_doc = {"status": "succeeded", "summary": {}} - with ( - patch.object(worker.client, "get_run", return_value=run_doc), - patch.object( - worker.treeherder, - "recheck_skip_reason", - side_effect=RuntimeError("treeherder down"), - ), - patch.object(worker, "notify") as notify, - ): - worker.poll_and_notify(CTX) - - notify.send_email.assert_called_once_with(CTX, run_doc, None) diff --git a/uv.lock b/uv.lock index 02f899bf0f..2c8435e0f3 100644 --- a/uv.lock +++ b/uv.lock @@ -2767,10 +2767,8 @@ dependencies = [ { name = "cachetools" }, { name = "httpx" }, { name = "kombu" }, - { name = "markdown2" }, { name = "mozci" }, { name = "pydantic-settings" }, - { name = "sendgrid" }, { name = "sentry-sdk" }, { name = "taskcluster" }, { name = "tenacity" }, @@ -2787,11 +2785,9 @@ requires-dist = [ { name = "cachetools", specifier = ">=5.3.0" }, { name = "httpx", specifier = ">=0.26.0" }, { name = "kombu", specifier = ">=5.6,<6" }, - { name = "markdown2", specifier = ">=2.4.0" }, { name = "mozci", specifier = "~=2.4.8" }, { name = "pydantic-settings", specifier = ">=2.1.0" }, { name = "pytest", marker = "extra == 'dev'", specifier = ">=8.0.0" }, - { name = "sendgrid", specifier = ">=6.12.5" }, { name = "sentry-sdk", specifier = ">=2.51.0" }, { name = "taskcluster", specifier = ">=97.1,<102.1" }, { name = "tenacity", specifier = "~=9.1.4" }, @@ -2809,9 +2805,11 @@ dependencies = [ { name = "google-auth" }, { name = "httpx" }, { name = "lando-client" }, + { name = "markdown2" }, { name = "phabricator-client" }, { name = "pydantic-settings" }, { name = "requests" }, + { name = "sendgrid" }, { name = "slack-sdk" }, { name = "testrail-client" }, { name = "weave" }, @@ -2835,10 +2833,12 @@ requires-dist = [ { name = "google-auth", specifier = ">=2.0.0" }, { name = "httpx", specifier = ">=0.26.0" }, { name = "lando-client", editable = "libs/lando-client" }, + { name = "markdown2", specifier = ">=2.4.0" }, { name = "mozphab", marker = "extra == 'phabricator'", specifier = "==2.15.3" }, { name = "phabricator-client", editable = "libs/phabricator-client" }, { name = "pydantic-settings", specifier = ">=2.1.0" }, { name = "requests", specifier = ">=2.32.0" }, + { name = "sendgrid", specifier = ">=6.12.5" }, { name = "slack-sdk", specifier = ">=3.27.0" }, { name = "testrail-client", editable = "libs/testrail-client" }, { name = "weave", specifier = ">=0.53.4" },