diff --git a/agents/test-repair/README.md b/agents/test-repair/README.md index 36ecbc2746..926cba1349 100644 --- a/agents/test-repair/README.md +++ b/agents/test-repair/README.md @@ -65,10 +65,14 @@ Stage 2: ## Slack notification -Every run records a `slack.post_message` action carrying its verdict -- the -recommendation, the classification and confidence, the failing job (linked to the -push in Treeherder), the culprit or the candidates that could not be ruled out, -and whether a patch is attached. +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 +(linked to the push in Treeherder), the culprit or the candidates that could not be +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. 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 diff --git a/agents/test-repair/hackbot_agents/test_repair/__main__.py b/agents/test-repair/hackbot_agents/test_repair/__main__.py index 90e5ac8af7..347c36f94c 100644 --- a/agents/test-repair/hackbot_agents/test_repair/__main__.py +++ b/agents/test-repair/hackbot_agents/test_repair/__main__.py @@ -9,7 +9,7 @@ from .agent import TestRepairResult from .config import SKIP_FIREFOX_BUILD, SLACK_CHANNEL from .logs import download_failure_logs -from .notify import build_message, resolve_culprit_author +from .notify import build_message, resolve_culprit_author, sheriff_action_required from .resolve import Investigation, resolve_investigation logger = logging.getLogger(__name__) @@ -77,15 +77,19 @@ async def main(ctx: HackbotContext) -> TestRepairResult: publish_file=ctx.publish_file, ) - # Notifications are active for all runs - message = build_message( - result, - investigation, - task_id=task_id, - run_id=ctx.run_id, - culprit_author=resolve_culprit_author(source_repo, result.culprit_commit), - ) - record_message(ctx.actions, SLACK_CHANNEL, message) + 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), + ) + record_message(ctx.actions, SLACK_CHANNEL, message) + else: + logger.info( + "Verdict is %s; not notifying %s", result.classification, SLACK_CHANNEL + ) return result diff --git a/agents/test-repair/hackbot_agents/test_repair/notify.py b/agents/test-repair/hackbot_agents/test_repair/notify.py index 0d9a70451d..bba498a058 100644 --- a/agents/test-repair/hackbot_agents/test_repair/notify.py +++ b/agents/test-repair/hackbot_agents/test_repair/notify.py @@ -9,6 +9,8 @@ then visible in the hackbot UI before it lands, and the apply step delivers it at most once (see ``hackbot_runtime.actions.slack``). +Only verdicts a sheriff acts on are posted -- see :func:`sheriff_action_required`. + 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 @@ -44,6 +46,19 @@ } +def sheriff_action_required(result: TestRepairResult) -> bool: + """Whether the verdict is one a sheriff has to act on. + + A known intermittent asks for nothing -- no backout, no retrigger -- and it is the + majority verdict, so posting those is pure noise. ``rerun`` is not one of them: an + intermittent the agent could not confirm still asks for a retrigger. + """ + return not ( + result.classification == "intermittent" + and result.recommendation == "do_not_backout" + ) + + def _link(url: str, label: str) -> str: return f"<{url}|{label}>" diff --git a/agents/test-repair/tests/test_notify.py b/agents/test-repair/tests/test_notify.py index 9aa8fd528e..fbf9257cb1 100644 --- a/agents/test-repair/tests/test_notify.py +++ b/agents/test-repair/tests/test_notify.py @@ -1,5 +1,5 @@ from hackbot_agents.test_repair.agent import TestRepairResult -from hackbot_agents.test_repair.notify import build_message +from hackbot_agents.test_repair.notify import build_message, sheriff_action_required from hackbot_agents.test_repair.resolve import ( CommitRange, FailingGroup, @@ -55,6 +55,36 @@ def _message(result=None, investigation=None, **kwargs): ) +def test_a_known_intermittent_is_not_worth_a_notification(): + assert not sheriff_action_required( + _result( + classification="intermittent", + recommendation="do_not_backout", + culprit_commit=None, + ) + ) + + +def test_an_unconfirmed_intermittent_still_asks_for_a_retrigger(): + assert sheriff_action_required( + _result( + classification="intermittent", + recommendation="rerun", + culprit_commit=None, + ) + ) + + +def test_every_regression_verdict_is_notified(): + assert sheriff_action_required(_result()) + assert sheriff_action_required(_result(recommendation="rerun")) + # No culprit survived, so there is nothing to back out -- but a regression the + # agent could not pin down is still a sheriff's problem. + assert sheriff_action_required( + _result(recommendation="do_not_backout", culprit_commit=None) + ) + + def test_reports_the_verdict_and_its_context_in_five_lines(): assert _message(culprit_author="standard8@mozilla.com").splitlines() == [ "*test-repair: BACK OUT the culprit* (regression, confidence 0.7)", diff --git a/services/hackbot-pulse-listener/README.md b/services/hackbot-pulse-listener/README.md index 99482a2d92..01e32b5998 100644 --- a/services/hackbot-pulse-listener/README.md +++ b/services/hackbot-pulse-listener/README.md @@ -53,8 +53,8 @@ Failed **build** tasks go to `build-repair`; failed **test** tasks go to `test-r 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 the notification address - (`TEST_REPAIR_NOTIFICATION_EMAIL`). + mirror and mails its author; test-repair mails only the team address + (`NOTIFICATION_TEAM_EMAIL`) -- sheriffs are notified by the agent in Slack instead. The dedupe caches, the daily budget and pending-run tracking are all in-memory, so a restart resets them. @@ -98,9 +98,10 @@ 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 `TEST_REPAIR_NOTIFICATION_EMAIL` and the team address -- -never the culprit author or the pushing developer, though the culprit is still named -in the body. Set +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 diff --git a/services/hackbot-pulse-listener/app/config.py b/services/hackbot-pulse-listener/app/config.py index 01f5a59808..67234753ee 100644 --- a/services/hackbot-pulse-listener/app/config.py +++ b/services/hackbot-pulse-listener/app/config.py @@ -69,10 +69,9 @@ class Settings(BaseSettings): # Email notifications (SendGrid) sendgrid_api_key: str | None = None notification_sender: str | None = None - # Team address CC'd on every notification alongside the revision author. + # 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 - # Distribution address; primary recipient of test-repair verdicts. - test_repair_notification_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). diff --git a/services/hackbot-pulse-listener/app/notify.py b/services/hackbot-pulse-listener/app/notify.py index 78973f0242..ab41b47004 100644 --- a/services/hackbot-pulse-listener/app/notify.py +++ b/services/hackbot-pulse-listener/app/notify.py @@ -17,9 +17,8 @@ def send_email( ) -> 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 test-repair notification address; build-repair keeps its - existing behavior. + 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. @@ -70,10 +69,11 @@ def _send_test_repair_email( # 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 distribution list and the team address only. A verdict is a triage signal - # for the team, not something to mail at the developer whose commit the agent - # happens to blame -- the culprit is still named in the body. - recipients = _recipients(settings.test_repair_notification_email) + # 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 @@ -84,7 +84,7 @@ def _send_test_repair_email( return patch = _fetch_patch(ctx.run_id, run_doc) - # In the subject too, so a sheriff can skip it from the inbox. + # 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)} - " @@ -146,9 +146,9 @@ 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 its distribution 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. + 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] @@ -187,8 +187,7 @@ def _already_actioned_banner(reason: str | None) -> list[str]: return [] return [ f"> **Already actioned by a sheriff.** Treeherder now classifies this job as " - f"_{reason}_, so the tree has been dealt with and this analysis needs no " - f"action from a sheriff. It is sent for the developer's reland.", + f"_{reason}_, so the tree has been dealt with.", "", ] @@ -243,7 +242,8 @@ def _build_test_repair_body( lines.append(f"- **Bug:** [{bug}]({_bug_url(bug)})") lines += _run_details(ctx) + _analysis_sections(findings) + _patch_section(patch) - lines += _patch_advice(patch) + _team_footer() + # No team footer: the team is the only recipient. + lines += _patch_advice(patch) return "\n".join(lines) diff --git a/services/hackbot-pulse-listener/deploy.sh b/services/hackbot-pulse-listener/deploy.sh index 8aed83e552..94913f00b1 100755 --- a/services/hackbot-pulse-listener/deploy.sh +++ b/services/hackbot-pulse-listener/deploy.sh @@ -41,8 +41,6 @@ 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:-}" -# Distribution address; primary recipient of test-repair verdicts. -TEST_REPAIR_NOTIFICATION_EMAIL="${TEST_REPAIR_NOTIFICATION_EMAIL:-}" SA_NAME="${SA_NAME:-hackbot-pulse-listener-run}" SA_EMAIL="${SA_EMAIL:-${SA_NAME}@${PROJECT}.iam.gserviceaccount.com}" @@ -106,7 +104,6 @@ ENV_VARS="${ENV_VARS},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}" -ENV_VARS="${ENV_VARS},TEST_REPAIR_NOTIFICATION_EMAIL=${TEST_REPAIR_NOTIFICATION_EMAIL}" gcloud beta run worker-pools deploy "${SERVICE}" \ --image "${IMAGE}" \ diff --git a/services/hackbot-pulse-listener/tests/test_notify.py b/services/hackbot-pulse-listener/tests/test_notify.py index dd3f3e75b4..f7ff0b9d99 100644 --- a/services/hackbot-pulse-listener/tests/test_notify.py +++ b/services/hackbot-pulse-listener/tests/test_notify.py @@ -18,10 +18,6 @@ def _ctx(**over): return RunContext(**base) -def settings_test_repair_address() -> str | None: - return notify.settings.test_repair_notification_email - - def _test_repair_ctx(**over): over.setdefault("test_groups", ["dom/base/test/mochitest.ini"]) return _ctx(agent="test-repair", **over) @@ -357,18 +353,11 @@ def test_test_repair_intermittent_body_says_do_not_backout(): assert "Culprit commit" not in body -def test_test_repair_recipients_address_then_culprit(monkeypatch): +def test_test_repair_recipients_are_the_team_alone(monkeypatch): monkeypatch.setattr(notify.settings, "notification_override_email", None) - monkeypatch.setattr( - notify.settings, "test_repair_notification_email", "test-repair@mozilla.com" - ) monkeypatch.setattr(notify.settings, "notification_team_email", "team@mozilla.com") - assert notify._recipients( - settings_test_repair_address(), "culprit@mozilla.com" - ) == [ - "test-repair@mozilla.com", - "culprit@mozilla.com", - "team@mozilla.com", + assert notify._recipients(notify.settings.notification_team_email) == [ + "team@mozilla.com" ] @@ -376,24 +365,21 @@ def test_test_repair_recipients_override_wins(monkeypatch): monkeypatch.setattr( notify.settings, "notification_override_email", "me@mozilla.com" ) - monkeypatch.setattr( - notify.settings, "test_repair_notification_email", "test-repair@mozilla.com" - ) - assert notify._recipients( - settings_test_repair_address(), "culprit@mozilla.com" - ) == ["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). + # 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, "test_repair_notification_email", "test-repair@mozilla.com" - ) - monkeypatch.setattr(notify.settings, "notification_team_email", None) + monkeypatch.setattr(notify.settings, "notification_team_email", "team@mozilla.com") run_doc = { "status": "succeeded", @@ -414,7 +400,7 @@ def test_test_repair_intermittent_sends_without_patch(monkeypatch): personalizations = fake_client.send.call_args.kwargs["message"].get()[ "personalizations" ][0] - assert personalizations["to"] == [{"email": "test-repair@mozilla.com"}] + assert personalizations["to"] == [{"email": "team@mozilla.com"}] def test_test_repair_never_mails_the_culprit_author(monkeypatch): @@ -423,10 +409,7 @@ def test_test_repair_never_mails_the_culprit_author(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, "test_repair_notification_email", "test-repair@mozilla.com" - ) - monkeypatch.setattr(notify.settings, "notification_team_email", None) + monkeypatch.setattr(notify.settings, "notification_team_email", "team@mozilla.com") run_doc = {"status": "succeeded", "summary": {"findings": _test_repair_findings()}} fake_client = MagicMock() @@ -441,37 +424,23 @@ def test_test_repair_never_mails_the_culprit_author(monkeypatch): message = fake_client.send.call_args.kwargs["message"].get() personalizations = message["personalizations"][0] - assert personalizations["to"] == [{"email": "test-repair@mozilla.com"}] + 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_mails_the_team_address_too(monkeypatch): +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, "test_repair_notification_email", "test-repair@mozilla.com" - ) - monkeypatch.setattr(notify.settings, "notification_team_email", "team@mozilla.com") + monkeypatch.setattr(notify.settings, "notification_team_email", None) 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" - ), - ): + with patch("sendgrid.SendGridAPIClient") as sg: notify.send_email(_test_repair_ctx(), run_doc) - - personalizations = fake_client.send.call_args.kwargs["message"].get()[ - "personalizations" - ][0] - assert personalizations["to"] == [{"email": "test-repair@mozilla.com"}] - assert personalizations["cc"] == [{"email": "team@mozilla.com"}] + sg.assert_not_called() def test_test_repair_ignores_the_pushing_developer(monkeypatch): @@ -479,10 +448,7 @@ def test_test_repair_ignores_the_pushing_developer(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, "test_repair_notification_email", "test-repair@mozilla.com" - ) - monkeypatch.setattr(notify.settings, "notification_team_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()}} @@ -497,7 +463,7 @@ def test_test_repair_ignores_the_pushing_developer(monkeypatch): personalizations = fake_client.send.call_args.kwargs["message"].get()[ "personalizations" ][0] - assert personalizations["to"] == [{"email": "test-repair@mozilla.com"}] + assert personalizations["to"] == [{"email": "team@mozilla.com"}] assert "cc" not in personalizations @@ -546,6 +512,14 @@ def test_no_patch_advice_without_a_patch(): 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"