diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/config.py b/agents/frontend-triage/hackbot_agents/frontend_triage/config.py index 9364c0a901..4781bd73a6 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/config.py +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/config.py @@ -33,6 +33,11 @@ # and plans only: it records a comment with its findings/plan and, at high # confidence, may propose field updates (e.g. keyword/severity). It never # creates bugs or attaches files. +# +# `bugzilla.update_bug` needs `editbugs` on the account the apply step uses (the +# same one `bug-fix` writes with). Note the apply step coalesces a same-bug field +# change with the nearest comment into one PUT, so if that privilege were ever +# lost, a rejected field change would take the analysis comment down with it. ENABLED_ACTION_TYPES = [ "bugzilla.add_comment", "bugzilla.update_bug", diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/frontend-triage.md b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/frontend-triage.md index 93621bd150..d78f852ec3 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/frontend-triage.md +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/frontend-triage.md @@ -34,6 +34,11 @@ restate the whole bug. Do not claim the fix is verified — you did not run it. ## Confidence and field changes +Your `confidence` decides whether your actions reach the bug: a **high**-confidence +run is applied to Bugzilla automatically, while **medium** and **low** are held for +a human to review first. Reserve `high` for when you have actually localized the +cause in specific code — not for a plausible-sounding hypothesis. + - **High** (you found the specific code and the cause is clear): record the plan comment. If a rule or convention clearly applies, you may also record a `bugzilla_update_bug` for an obviously-correct field (e.g. adding a relevant diff --git a/services/hackbot-api/alembic/versions/d2b3c4e5f6a7_run_notified_at.py b/services/hackbot-api/alembic/versions/d2b3c4e5f6a7_run_notified_at.py new file mode 100644 index 0000000000..5fbc4616df --- /dev/null +++ b/services/hackbot-api/alembic/versions/d2b3c4e5f6a7_run_notified_at.py @@ -0,0 +1,30 @@ +"""Run notified_at. + +Revision ID: d2b3c4e5f6a7 +Revises: c1a2f3e4b5d6 +Create Date: 2026-07-28 00:00:00.000000 + +""" + +from typing import Sequence, Union + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision: str = "d2b3c4e5f6a7" +down_revision: Union[str, Sequence[str], None] = "c1a2f3e4b5d6" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + """Upgrade schema.""" + op.add_column( + "runs", sa.Column("notified_at", sa.DateTime(timezone=True), nullable=True) + ) + + +def downgrade() -> None: + """Downgrade schema.""" + op.drop_column("runs", "notified_at") diff --git a/services/hackbot-api/app/actions_applier.py b/services/hackbot-api/app/actions_applier.py index 49f2f020ce..a66447ecda 100644 --- a/services/hackbot-api/app/actions_applier.py +++ b/services/hackbot-api/app/actions_applier.py @@ -28,8 +28,15 @@ from sqlalchemy.ext.asyncio import AsyncSession from app import gcs -from app.agents import AGENT_REGISTRY +from app.agents import AGENT_REGISTRY, AgentSpec from app.database.models import Run, RunAction +from app.notify import ( + FAILED, + HELD, + NO_ACTIONS, + POSTED, + notify_run_completed, +) from app.schemas import RunStatus log = logging.getLogger(__name__) @@ -86,6 +93,32 @@ def _sub(match: re.Match) -> str: return value +def _reported_confidence(run: Run) -> str | None: + """The run's self-reported confidence, normalised, or None if unusable. + + ``confidence`` reaches us from the agent's free-form JSON block, so tolerate + casing and stray whitespace rather than reading "High" as "no confidence". + """ + value = ((run.summary or {}).get("findings") or {}).get("confidence") + if not isinstance(value, str): + return None + return value.strip().lower() or None + + +def _should_auto_apply(spec: AgentSpec | None, run: Run) -> bool: + """Whether `run`'s recorded actions may be applied without a human. + + Pure, so the policy is testable on its own. Two independent gates: the + agent's `auto_apply_actions` opt-in, then — for agents that report a + confidence — whether that confidence is one the spec admits. + """ + if spec is None or not spec.auto_apply_actions: + return False + if spec.auto_apply_confidence is None: + return True + return _reported_confidence(run) in spec.auto_apply_confidence + + async def ensure_action_rows( db: AsyncSession, run: Run ) -> list[tuple[RunAction, list[dict]]]: @@ -223,12 +256,26 @@ async def _apply_pending_rows( results_by_ref[member.ref] = outcome.result +def _apply_outcome(rows: list[tuple[RunAction, list[dict]]]) -> str: + """Summarise what an apply pass actually achieved, for the notification.""" + if not rows: + return NO_ACTIONS + if all(row.status == "applied" for row, _ in rows): + return POSTED + return FAILED + + async def on_run_completed(db: AsyncSession, run: Run) -> None: - """Record a completed run's actions, and auto-apply them if the agent opts in. + """Record a completed run's actions, auto-apply them, then notify. Called from the `apply-run-actions` push route. Actions are always recorded (so the UI can show/manually apply them); they're applied automatically only - when the run's agent has `auto_apply_actions=True`. + when `_should_auto_apply` says so. + + The notification runs here rather than off its own `run.completed` + subscription because it has to report *whether* Bugzilla was written — a + second consumer would race this one. It fires either way: a result too + unconfident to post is still worth a human's glance. """ # Defense-in-depth: only a succeeded run's actions are recorded/applied. A # failed/timed-out run may have recorded actions before erroring, but acting @@ -243,16 +290,24 @@ async def on_run_completed(db: AsyncSession, run: Run) -> None: await db.commit() spec = AGENT_REGISTRY.get(run.agent) - if spec and spec.auto_apply_actions: + if _should_auto_apply(spec, run): await _apply_pending_rows(db, run, rows) + outcome = _apply_outcome(rows) else: + outcome = HELD if rows else NO_ACTIONS log.info( - "Recorded %d action(s) for run %s; auto-apply off for agent %s", + "Recorded %d action(s) for run %s; not auto-applying " + "(agent %s, confidence %s)", len(rows), run.run_id, run.agent, + _reported_confidence(run), ) + if spec and spec.notify_completion: + await notify_run_completed(run, outcome) + await db.commit() + async def apply_all_pending(db: AsyncSession, run: Run) -> None: """Apply all of a run's not-yet-`applied` actions on demand (manual). diff --git a/services/hackbot-api/app/agents.py b/services/hackbot-api/app/agents.py index 0ea62cecfc..07119d275c 100644 --- a/services/hackbot-api/app/agents.py +++ b/services/hackbot-api/app/agents.py @@ -27,6 +27,15 @@ class AgentSpec: # succeeds. Off by default: actions are still recorded and can always be # applied manually from the UI; only opted-in agents auto-apply. auto_apply_actions: bool = False + # Confidence levels (from ``summary["findings"]["confidence"]``) whose actions + # may be auto-applied, for agents that report one. ``None`` means no + # confidence restriction. Fail closed: when this is set, a run whose findings + # carry no usable confidence never qualifies. Widening the policy (e.g. to + # also accept "medium") is an edit to this set, not to the applier. + auto_apply_confidence: frozenset[str] | None = None + # Whether a succeeded run gets a completion notification (see app/notify.py). + # Off by default; the destination is deployment config, not a per-agent choice. + notify_completion: bool = False def model_to_env(inputs: BaseModel) -> dict[str, str]: @@ -79,6 +88,12 @@ def model_to_env(inputs: BaseModel) -> dict[str, str]: description="Triage a Firefox desktop frontend bug (read-only) and produce a root-cause analysis and proposed fix plan.", job_name="hackbot-agent-frontend-triage", input_schema=FrontendTriageInputs, + # Triage results go to the bug unattended, so only the ones the agent + # localized to specific code qualify. A medium/low result is still + # recorded for manual apply from the UI. + auto_apply_actions=True, + auto_apply_confidence=frozenset({"high"}), + notify_completion=True, ), "test-repair": AgentSpec( name="test-repair", diff --git a/services/hackbot-api/app/config.py b/services/hackbot-api/app/config.py index ded3f8cdad..e516272e33 100644 --- a/services/hackbot-api/app/config.py +++ b/services/hackbot-api/app/config.py @@ -70,6 +70,21 @@ class Settings(BaseSettings): push_auth_audience: str = "" push_auth_service_account: str = "" + # Run-completion notification (email; the recipients are Slack channel + # addresses via Slack's "Email to channel"). Unset => notification skipped. + sendgrid_api_key: str | None = None + notification_sender: str | None = None + # Where to report a triaged bug, keyed by its `" :: "`. + # A channel belongs to the team that owns the component, so this is a map + # rather than one address; the optional `"default"` key catches components + # with no channel of their own. JSON-encoded in the environment, e.g. + # {"Firefox :: New Tab Page": "hnt-…@mozilla.org.slack.com"}. + notification_slack_emails: dict[str, str] = {} + + # Link targets used in notifications. + bugzilla_url: str = "https://bugzilla.mozilla.org" + hackbot_ui_url: str = "" + # Server port: int = 8080 environment: str = "development" diff --git a/services/hackbot-api/app/database/models.py b/services/hackbot-api/app/database/models.py index 6dd1b32dba..7ddcf8be88 100644 --- a/services/hackbot-api/app/database/models.py +++ b/services/hackbot-api/app/database/models.py @@ -39,6 +39,12 @@ class Run(Base): finalized_at: Mapped[datetime | None] = mapped_column( DateTime(timezone=True), nullable=True ) + # When the completion notification was sent. Unlike an action row, an email + # is not idempotent, so this is what stops a redelivered `run.completed` + # event from reposting to the channel (see app/notify.py). + notified_at: Mapped[datetime | None] = mapped_column( + DateTime(timezone=True), nullable=True + ) class RunAction(Base): diff --git a/services/hackbot-api/app/notify.py b/services/hackbot-api/app/notify.py new file mode 100644 index 0000000000..37c0d6900c --- /dev/null +++ b/services/hackbot-api/app/notify.py @@ -0,0 +1,205 @@ +"""Tell a Slack channel what a finished run did. + +Delivery is email to a Slack channel address (Slack's "Email to channel"), which +is why this is an email module and not a Slack API client: Slack renders an +inbound message's *subject* as the message title, so the one-line summary lives +there and the links live in the body. + +Sending is best-effort and gated on configuration, mirroring +``services/hackbot-pulse-listener/app/notify.py``: an unconfigured deployment +logs and moves on rather than failing the run-completion path. +""" + +from __future__ import annotations + +import asyncio +import logging +from datetime import datetime, timezone + +import requests + +from app.config import settings +from app.database.models import Run + +log = logging.getLogger(__name__) + +# Slack shows an inbound email's subject as the message title and truncates long +# ones, so keep it to something that reads as one line in the channel. +MAX_SUBJECT_LENGTH = 150 + +# Key in `notification_slack_emails` for components with no channel of their own. +DEFAULT_CHANNEL_KEY = "default" + +_BUGZILLA_TIMEOUT_SECONDS = 30 + +# What became of a completed run's recorded actions. These are distinguished +# because "we decided to apply" and "Bugzilla accepted it" are not the same +# thing, and a reader in the channel needs to know which one happened. +POSTED = "posted" # every action landed on Bugzilla +HELD = "held" # not eligible for auto-apply; awaiting human review +FAILED = "failed" # auto-apply ran but at least one action did not land +NO_ACTIONS = "no_actions" # the run recorded nothing to apply + +_OUTCOME_WORDING = { + POSTED: "Posted to Bugzilla.", + HELD: "Not posted — awaiting review in the hackbot UI.", + FAILED: "Tried to post, but at least one action failed — see the run.", + NO_ACTIONS: "Nothing to post — the run recorded no actions.", +} + + +def _findings(run: Run) -> dict: + return (run.summary or {}).get("findings") or {} + + +def _bug_id(run: Run) -> object | None: + findings = _findings(run) + return findings.get("bug_id") or (run.inputs or {}).get("bug_id") + + +def _bug_product_component(bug_id: object) -> str | None: + """The bug's ``" :: "``, or None if it can't be read. + + Read from Bugzilla rather than from the run: the agent doesn't report the + component, and Bugzilla is authoritative anyway — a bug moved between + components mid-triage should reach the team that owns it now. Blocking, so + callers hand it to a thread. + """ + try: + response = requests.get( + f"{settings.bugzilla_url.rstrip('/')}/rest/bug/{bug_id}", + params={"include_fields": "product,component"}, + timeout=_BUGZILLA_TIMEOUT_SECONDS, + ) + response.raise_for_status() + bugs = response.json().get("bugs") or [] + except Exception: + log.exception("Could not read the component of bug %s", bug_id) + return None + + if not bugs: + # Inaccessible or nonexistent; Bugzilla returns an empty list either way. + log.warning("Bugzilla returned no data for bug %s", bug_id) + return None + + return f"{bugs[0]['product']} :: {bugs[0]['component']}" + + +def _recipient_for(run: Run) -> str | None: + """The Slack channel address a run's result belongs in, or None to stay quiet. + + Routes on the bug's component so each team hears about its own bugs. A + component with no channel falls back to ``default`` if one is configured; + with no fallback we say nothing, because posting one team's triage into + another team's channel is worse than silence. + """ + channels = settings.notification_slack_emails + if not channels: + return None + + bug_id = _bug_id(run) + key = _bug_product_component(bug_id) if bug_id else None + if key is not None and key in channels: + return channels[key] + + fallback = channels.get(DEFAULT_CHANNEL_KEY) + if fallback is None: + log.info( + "No channel configured for %s (run %s); not notifying", key, run.run_id + ) + return fallback + + +def _bug_url(bug_id: object) -> str: + return f"{settings.bugzilla_url.rstrip('/')}/show_bug.cgi?id={bug_id}" + + +def _run_url(run: Run) -> str | None: + if not settings.hackbot_ui_url: + return None + return f"{settings.hackbot_ui_url.rstrip('/')}/runs/{run.run_id}" + + +def _one_line(text: str) -> str: + return " ".join(text.split()) + + +def build_notification(run: Run, outcome: str) -> tuple[str, str]: + """Render the (subject, body) for a finished run. + + Pure, so the wording is testable without touching SendGrid. Every field is + optional in practice — the agent's structured plan is parsed best-effort — so + each one degrades to something still worth reading in the channel. + """ + findings = _findings(run) + bug_id = _bug_id(run) + confidence = findings.get("confidence") or "unknown" + + subject = f"[{run.agent}] " + (f"Bug {bug_id}" if bug_id else f"Run {run.run_id}") + summary = findings.get("summary") + if isinstance(summary, str) and summary.strip(): + subject += f" — {_one_line(summary)}" + if len(subject) > MAX_SUBJECT_LENGTH: + subject = subject[: MAX_SUBJECT_LENGTH - 1].rstrip() + "…" + + lines = [ + _OUTCOME_WORDING.get(outcome, f"Outcome: {outcome}"), + f"Confidence: {confidence}", + ] + if bug_id: + lines.append(f"Bug: {_bug_url(bug_id)}") + run_url = _run_url(run) + if run_url: + lines.append(f"Run: {run_url}") + + return subject, "\n".join(lines) + + +async def notify_run_completed(run: Run, outcome: str) -> None: + """Send the run's one-line result to its team's Slack channel address. + + At-most-once per run: ``Run.notified_at`` is stamped on success, so a + redelivered ``run.completed`` event doesn't repost. The caller commits. + """ + if run.notified_at is not None: + log.info("Run %s already notified at %s; skipping", run.run_id, run.notified_at) + return + + if not (settings.sendgrid_api_key and settings.notification_sender): + log.info("Notification not configured; skipping for run %s", run.run_id) + return + + # Resolving the channel reads Bugzilla; keep it off the event loop. + recipient = await asyncio.to_thread(_recipient_for, run) + if recipient is None: + return + + import sendgrid + from sendgrid.helpers.mail import Content, From, Mail, Subject, To + + subject, body = build_notification(run, outcome) + message = Mail( + From(settings.notification_sender), + To(recipient), + Subject(subject), + Content("text/plain", body), + ) + + try: + response = await asyncio.to_thread( + sendgrid.SendGridAPIClient(api_key=settings.sendgrid_api_key).send, + message=message, + ) + except Exception: + # A missed channel message must not fail the completion path — the + # actions are already applied and the run is already durable. + log.exception("Failed to notify for run %s", run.run_id) + return + + run.notified_at = datetime.now(timezone.utc) + log.info( + "Notified %s about run %s (status %s)", + recipient, + run.run_id, + response.status_code, + ) diff --git a/services/hackbot-api/pyproject.toml b/services/hackbot-api/pyproject.toml index fe79eeee31..5ba08375d9 100644 --- a/services/hackbot-api/pyproject.toml +++ b/services/hackbot-api/pyproject.toml @@ -16,6 +16,8 @@ dependencies = [ "google-cloud-run>=0.10.0", "google-cloud-pubsub>=2.21.0", "google-auth>=2.29.0", + "requests>=2.31.0", + "sendgrid>=6.11.0", "sentry-sdk>=2.51.0", "cachetools>=5.3.0", "httpx>=0.26.0", diff --git a/services/hackbot-api/tests/test_actions_applier.py b/services/hackbot-api/tests/test_actions_applier.py index 344c41fd59..3c7034e39e 100644 --- a/services/hackbot-api/tests/test_actions_applier.py +++ b/services/hackbot-api/tests/test_actions_applier.py @@ -16,6 +16,7 @@ on_run_completed, resolve_placeholders, ) +from app.agents import AGENT_REGISTRY from app.schemas import RunStatus @@ -75,6 +76,79 @@ class _FakeRun: agent: str = "bug-fix" run_id: uuid.UUID = field(default_factory=uuid.uuid4) summary: dict | None = None + inputs: dict = field(default_factory=dict) + notified_at: object | None = None + + +def _spec(*, auto=True, confidence=None, notify=False): + return SimpleNamespace( + auto_apply_actions=auto, + auto_apply_confidence=confidence, + notify_completion=notify, + ) + + +def _run_with_confidence(confidence): + return _FakeRun( + status=RunStatus.succeeded.value, + summary={"findings": {"confidence": confidence}, "actions": []}, + ) + + +def test_auto_apply_off_never_applies(): + # The master switch wins: confidence is irrelevant when the agent hasn't + # opted in at all. + spec = _spec(auto=False, confidence=frozenset({"high"})) + assert not actions_applier._should_auto_apply(spec, _run_with_confidence("high")) + + +def test_unknown_agent_never_applies(): + assert not actions_applier._should_auto_apply(None, _run_with_confidence("high")) + + +def test_no_confidence_restriction_applies_unconditionally(): + # Existing/future agents that opt in without naming confidence levels keep + # applying regardless of what findings say. + spec = _spec(confidence=None) + assert actions_applier._should_auto_apply(spec, _run_with_confidence("low")) + assert actions_applier._should_auto_apply( + spec, _FakeRun(status=RunStatus.succeeded.value) + ) + + +def test_confidence_restriction_admits_listed_level_only(): + spec = _spec(confidence=frozenset({"high"})) + assert actions_applier._should_auto_apply(spec, _run_with_confidence("high")) + assert not actions_applier._should_auto_apply(spec, _run_with_confidence("medium")) + assert not actions_applier._should_auto_apply(spec, _run_with_confidence("low")) + + +def test_confidence_restriction_can_widen_to_several_levels(): + # Widening to medium later must be a config change, not a code change. + spec = _spec(confidence=frozenset({"high", "medium"})) + assert actions_applier._should_auto_apply(spec, _run_with_confidence("medium")) + assert not actions_applier._should_auto_apply(spec, _run_with_confidence("low")) + + +def test_confidence_restriction_is_case_and_space_insensitive(): + # `confidence` is parsed out of the model's free-form JSON block, so don't + # let "High" silently mean "never apply". + spec = _spec(confidence=frozenset({"high"})) + assert actions_applier._should_auto_apply(spec, _run_with_confidence("High")) + assert actions_applier._should_auto_apply(spec, _run_with_confidence(" HIGH ")) + + +def test_missing_confidence_fails_closed(): + spec = _spec(confidence=frozenset({"high"})) + for run in ( + _FakeRun(status=RunStatus.succeeded.value), # no summary at all + _FakeRun(status=RunStatus.succeeded.value, summary={}), # no findings + _FakeRun(status=RunStatus.succeeded.value, summary={"findings": {}}), + _run_with_confidence(None), + _run_with_confidence(""), + _run_with_confidence(42), # not even a string + ): + assert not actions_applier._should_auto_apply(spec, run) class _FakeDB: @@ -90,24 +164,36 @@ async def execute(self, *a, **k): ) -def _patch_applier(monkeypatch, *, auto: bool | None): - """Stub ensure/apply and the registry; record what got called. +def _patch_applier(monkeypatch, *, auto: bool | None, confidence=None, notify=False): + """Stub ensure/apply/notify and the registry; record what got called. `auto=None` means the agent isn't in the registry at all. """ calls = {"ensured": False, "applied": False} + rows = [(_row(0, "pending"), [])] async def fake_ensure(db, run): calls["ensured"] = True - return [("row", [])] + return rows async def fake_apply(db, run, rows): calls["applied"] = True + # A successful apply pass leaves its rows `applied`; the outcome the + # notification reports is read back off them. + for row, _ in rows: + row.status = "applied" + + async def fake_notify(run, applied): + # Recorded as a list so a double-send would be visible. + calls.setdefault("notified", []).append(applied) monkeypatch.setattr(actions_applier, "ensure_action_rows", fake_ensure) monkeypatch.setattr(actions_applier, "_apply_pending_rows", fake_apply) + monkeypatch.setattr(actions_applier, "notify_run_completed", fake_notify) registry = ( - {} if auto is None else {"bug-fix": SimpleNamespace(auto_apply_actions=auto)} + {} + if auto is None + else {"bug-fix": _spec(auto=auto, confidence=confidence, notify=notify)} ) monkeypatch.setattr(actions_applier, "AGENT_REGISTRY", registry) return calls @@ -142,6 +228,162 @@ async def test_succeeded_unknown_agent_does_not_apply(monkeypatch): assert calls == {"ensured": True, "applied": False} +async def test_succeeded_high_confidence_run_applies(monkeypatch): + calls = _patch_applier(monkeypatch, auto=True, confidence=frozenset({"high"})) + await on_run_completed(_FakeDB(), _run_with_confidence("high")) + assert calls == {"ensured": True, "applied": True} + + +async def test_succeeded_low_confidence_run_records_but_does_not_apply(monkeypatch): + calls = _patch_applier(monkeypatch, auto=True, confidence=frozenset({"high"})) + for level in ("medium", "low"): + calls["ensured"] = calls["applied"] = False + await on_run_completed(_FakeDB(), _run_with_confidence(level)) + # Recorded for the UI (and manual apply), but nothing reaches Bugzilla. + assert calls == {"ensured": True, "applied": False}, level + + +async def test_frontend_triage_auto_applies_at_high_confidence_only(): + # Guards the live policy, not the mechanism: widening this set posts + # lower-confidence analyses to real bugs, so it should be a deliberate edit. + spec = AGENT_REGISTRY["frontend-triage"] + assert spec.auto_apply_actions is True + assert spec.auto_apply_confidence == frozenset({"high"}) + assert spec.notify_completion is True + + +async def test_other_agents_do_not_auto_apply_or_notify(): + # frontend-triage is the only agent opted in; the rest stay human-gated + # and silent. + for name, spec in AGENT_REGISTRY.items(): + if name == "frontend-triage": + continue + assert spec.auto_apply_actions is False, name + assert spec.notify_completion is False, name + + +# --- completion notification -------------------------------------------- # + + +async def test_notifies_that_actions_were_applied(monkeypatch): + calls = _patch_applier( + monkeypatch, auto=True, confidence=frozenset({"high"}), notify=True + ) + await on_run_completed(_FakeDB(), _run_with_confidence("high")) + assert calls["notified"] == [actions_applier.POSTED] + + +async def test_notifies_even_when_nothing_was_applied(monkeypatch): + # A medium/low result posts nothing to Bugzilla but still tells the channel, + # so a human knows a triage ran and can go look at it. + calls = _patch_applier( + monkeypatch, auto=True, confidence=frozenset({"high"}), notify=True + ) + await on_run_completed(_FakeDB(), _run_with_confidence("medium")) + assert calls["notified"] == [actions_applier.HELD] + + +async def test_notifies_a_failed_apply_as_failed(monkeypatch): + # Deciding to apply is not the same as Bugzilla accepting it. Reporting + # "posted" when the PUT was rejected would quietly mislead the channel. + calls = {} + + async def fake_ensure(db, run): + return [(_row(0, "failed", error="boom"), [])] + + async def fake_apply(db, run, rows): + pass # leaves the row `failed`, as a rejected PUT would + + async def fake_notify(run, outcome): + calls["notified"] = outcome + + monkeypatch.setattr(actions_applier, "ensure_action_rows", fake_ensure) + monkeypatch.setattr(actions_applier, "_apply_pending_rows", fake_apply) + monkeypatch.setattr(actions_applier, "notify_run_completed", fake_notify) + monkeypatch.setattr( + actions_applier, + "AGENT_REGISTRY", + {"bug-fix": _spec(auto=True, confidence=frozenset({"high"}), notify=True)}, + ) + + await on_run_completed(_FakeDB(), _run_with_confidence("high")) + assert calls["notified"] == actions_applier.FAILED + + +async def test_notifies_a_run_with_no_actions_as_such(monkeypatch): + # Neither "posted" nor "awaiting review" is true when the agent recorded + # nothing at all. + calls = {} + + async def fake_ensure(db, run): + return [] + + async def fake_apply(db, run, rows): + pass + + async def fake_notify(run, outcome): + calls["notified"] = outcome + + monkeypatch.setattr(actions_applier, "ensure_action_rows", fake_ensure) + monkeypatch.setattr(actions_applier, "_apply_pending_rows", fake_apply) + monkeypatch.setattr(actions_applier, "notify_run_completed", fake_notify) + monkeypatch.setattr( + actions_applier, + "AGENT_REGISTRY", + {"bug-fix": _spec(auto=True, confidence=frozenset({"high"}), notify=True)}, + ) + + await on_run_completed(_FakeDB(), _run_with_confidence("high")) + assert calls["notified"] == actions_applier.NO_ACTIONS + + +async def test_does_not_notify_for_agents_that_did_not_opt_in(monkeypatch): + calls = _patch_applier(monkeypatch, auto=True, notify=False) + await on_run_completed(_FakeDB(), _run_with_confidence("high")) + assert "notified" not in calls + + +async def test_does_not_notify_a_run_that_did_not_succeed(monkeypatch): + calls = _patch_applier(monkeypatch, auto=True, notify=True) + await on_run_completed(_FakeDB(), _FakeRun(status=RunStatus.failed.value)) + assert "notified" not in calls + + +async def test_notifies_after_applying(monkeypatch): + # Ordering matters: the message asserts whether Bugzilla was written, so it + # cannot be built before the apply pass has run. + order = [] + + async def fake_ensure(db, run): + return [(_row(0, "applied"), [])] + + async def fake_apply(db, run, rows): + order.append("apply") + + async def fake_notify(run, outcome): + order.append("notify") + + monkeypatch.setattr(actions_applier, "ensure_action_rows", fake_ensure) + monkeypatch.setattr(actions_applier, "_apply_pending_rows", fake_apply) + monkeypatch.setattr(actions_applier, "notify_run_completed", fake_notify) + monkeypatch.setattr( + actions_applier, + "AGENT_REGISTRY", + {"bug-fix": _spec(auto=True, confidence=frozenset({"high"}), notify=True)}, + ) + + await on_run_completed(_FakeDB(), _run_with_confidence("high")) + assert order == ["apply", "notify"] + + +async def test_manual_apply_does_not_notify(monkeypatch): + # Apply-all is a human clicking a button in the UI; they don't need Slack to + # tell them what they just did. + calls = _patch_applier(monkeypatch, auto=False, notify=True) + await apply_all_pending(_FakeDB(), _run_with_confidence("high")) + assert "notified" not in calls + + async def test_apply_all_pending_always_applies(monkeypatch): # Manual apply ignores the opt-in flag entirely. calls = _patch_applier(monkeypatch, auto=False) diff --git a/services/hackbot-api/tests/test_notify.py b/services/hackbot-api/tests/test_notify.py new file mode 100644 index 0000000000..dc25e64392 --- /dev/null +++ b/services/hackbot-api/tests/test_notify.py @@ -0,0 +1,291 @@ +"""Tests for the run-completion notification. + +The message is delivered by email to a Slack channel address ("Email to +channel"), so Slack renders the subject as the message title — that is where the +one-line summary belongs. See app/notify.py. +""" + +import uuid +from dataclasses import dataclass, field +from datetime import datetime, timezone +from unittest.mock import patch + +from app import notify + + +@dataclass +class _FakeRun: + agent: str = "frontend-triage" + run_id: uuid.UUID = field(default_factory=lambda: uuid.UUID(int=7)) + status: str = "succeeded" + inputs: dict = field(default_factory=lambda: {"bug_id": 2014702}) + summary: dict | None = None + notified_at: object | None = None + + +def _triaged(**findings): + base = { + "summary": "Weather widget disappears after switching regions", + "confidence": "high", + } + base.update(findings) + return _FakeRun(summary={"findings": base, "actions": []}) + + +def _configure(monkeypatch): + monkeypatch.setattr(notify.settings, "bugzilla_url", "https://bugzilla.mozilla.org") + monkeypatch.setattr(notify.settings, "hackbot_ui_url", "https://hackbot.example") + + +# --- subject: the one-line summary ------------------------------------- # + + +def test_subject_is_one_line_with_agent_bug_and_summary(monkeypatch): + _configure(monkeypatch) + subject, _ = notify.build_notification(_triaged(), notify.POSTED) + assert "\n" not in subject + assert "frontend-triage" in subject + assert "2014702" in subject + assert "Weather widget disappears after switching regions" in subject + + +def test_subject_truncates_a_rambling_summary(monkeypatch): + _configure(monkeypatch) + subject, _ = notify.build_notification(_triaged(summary="x" * 500), notify.POSTED) + assert len(subject) <= notify.MAX_SUBJECT_LENGTH + assert subject.endswith("…") + + +def test_subject_collapses_a_multiline_summary(monkeypatch): + _configure(monkeypatch) + subject, _ = notify.build_notification( + _triaged(summary="first line\nsecond line"), notify.POSTED + ) + assert "\n" not in subject + assert "first line second line" in subject + + +def test_subject_falls_back_without_a_summary(monkeypatch): + _configure(monkeypatch) + subject, _ = notify.build_notification( + _FakeRun(summary={"findings": {}}), notify.HELD + ) + assert "\n" not in subject + assert "2014702" in subject + + +# --- body: links and outcome ------------------------------------------- # + + +def test_body_links_the_bug(monkeypatch): + _configure(monkeypatch) + _, body = notify.build_notification(_triaged(), notify.POSTED) + assert "https://bugzilla.mozilla.org/show_bug.cgi?id=2014702" in body + + +def test_body_links_the_run(monkeypatch): + _configure(monkeypatch) + _, body = notify.build_notification(_triaged(), notify.POSTED) + assert f"https://hackbot.example/runs/{uuid.UUID(int=7)}" in body + + +def test_body_omits_run_link_when_ui_url_unset(monkeypatch): + _configure(monkeypatch) + monkeypatch.setattr(notify.settings, "hackbot_ui_url", "") + _, body = notify.build_notification(_triaged(), notify.POSTED) + assert "/runs/" not in body + + +def test_body_reports_confidence(monkeypatch): + _configure(monkeypatch) + _, body = notify.build_notification(_triaged(confidence="medium"), notify.HELD) + assert "medium" in body + + +def test_body_distinguishes_applied_from_awaiting_review(monkeypatch): + _configure(monkeypatch) + _, posted = notify.build_notification(_triaged(), notify.POSTED) + _, held = notify.build_notification(_triaged(confidence="low"), notify.HELD) + assert "Posted to Bugzilla" in posted + assert "Posted to Bugzilla" not in held + assert "awaiting review" in held + + +def test_body_reports_a_failed_apply_as_neither_posted_nor_held(monkeypatch): + _configure(monkeypatch) + _, body = notify.build_notification(_triaged(), notify.FAILED) + assert "Posted to Bugzilla" not in body + assert "awaiting review" not in body + assert "failed" in body + + +def test_body_reports_a_run_that_recorded_nothing(monkeypatch): + _configure(monkeypatch) + _, body = notify.build_notification(_triaged(), notify.NO_ACTIONS) + assert "Posted to Bugzilla" not in body + assert "no actions" in body + + +def test_body_survives_a_run_with_no_summary(monkeypatch): + _configure(monkeypatch) + subject, body = notify.build_notification(_FakeRun(), notify.HELD) + assert subject and body + assert "https://bugzilla.mozilla.org/show_bug.cgi?id=2014702" in body + + +def test_body_survives_a_run_with_no_bug_id(monkeypatch): + _configure(monkeypatch) + subject, body = notify.build_notification(_FakeRun(inputs={}), notify.HELD) + assert subject and body + assert "show_bug.cgi" not in body + + +# --- sending: gated on config, at most once per run --------------------- # + + +NEWTAB = "Firefox :: New Tab Page" +NEWTAB_CHANNEL = "hnt-dev-aaa@mozilla.org.slack.com" +FALLBACK_CHANNEL = "hackbot-default@mozilla.org.slack.com" + + +def _configure_sending(monkeypatch, channels=None, component=NEWTAB): + _configure(monkeypatch) + monkeypatch.setattr(notify.settings, "sendgrid_api_key", "key") + monkeypatch.setattr(notify.settings, "notification_sender", "hackbot@mozilla.com") + monkeypatch.setattr( + notify.settings, + "notification_slack_emails", + {NEWTAB: NEWTAB_CHANNEL} if channels is None else channels, + ) + # The component comes from Bugzilla, not from the run; stub the lookup. + monkeypatch.setattr(notify, "_bug_product_component", lambda bug_id: component) + + +def _sent_to(sg): + return sg.return_value.send.call_args.kwargs["message"].get()["personalizations"][ + 0 + ]["to"] + + +# --- routing: which channel gets the message ---------------------------- # + + +async def test_routes_to_the_channel_for_the_bugs_component(monkeypatch): + # One address per team: New Tab Page bugs go to the New Tab Page channel. + _configure_sending(monkeypatch) + with patch("sendgrid.SendGridAPIClient") as sg: + await notify.notify_run_completed(_triaged(), notify.POSTED) + assert _sent_to(sg) == [{"email": NEWTAB_CHANNEL}] + + +async def test_routes_a_second_component_to_its_own_channel(monkeypatch): + other = "Firefox :: Address Bar" + _configure_sending( + monkeypatch, + channels={NEWTAB: NEWTAB_CHANNEL, other: "urlbar@mozilla.org.slack.com"}, + component=other, + ) + with patch("sendgrid.SendGridAPIClient") as sg: + await notify.notify_run_completed(_triaged(), notify.POSTED) + assert _sent_to(sg) == [{"email": "urlbar@mozilla.org.slack.com"}] + + +async def test_unmapped_component_falls_back_to_default(monkeypatch): + _configure_sending( + monkeypatch, + channels={NEWTAB: NEWTAB_CHANNEL, "default": FALLBACK_CHANNEL}, + component="Firefox :: Menus", + ) + with patch("sendgrid.SendGridAPIClient") as sg: + await notify.notify_run_completed(_triaged(), notify.POSTED) + assert _sent_to(sg) == [{"email": FALLBACK_CHANNEL}] + + +async def test_unmapped_component_with_no_default_sends_nothing(monkeypatch): + # Better silent than posting one team's triage into another team's channel. + _configure_sending(monkeypatch, component="Firefox :: Menus") + with patch("sendgrid.SendGridAPIClient") as sg: + await notify.notify_run_completed(_triaged(), notify.POSTED) + sg.assert_not_called() + + +async def test_unknown_component_still_reaches_the_default(monkeypatch): + # A Bugzilla lookup failure shouldn't lose the message when there's a + # default channel to fall back on. + _configure_sending( + monkeypatch, + channels={NEWTAB: NEWTAB_CHANNEL, "default": FALLBACK_CHANNEL}, + component=None, + ) + with patch("sendgrid.SendGridAPIClient") as sg: + await notify.notify_run_completed(_triaged(), notify.POSTED) + assert _sent_to(sg) == [{"email": FALLBACK_CHANNEL}] + + +async def test_no_channels_configured_sends_nothing(monkeypatch): + _configure_sending(monkeypatch, channels={}) + with patch("sendgrid.SendGridAPIClient") as sg: + await notify.notify_run_completed(_triaged(), notify.POSTED) + sg.assert_not_called() + + +async def test_skips_when_sendgrid_unconfigured(monkeypatch): + _configure_sending(monkeypatch) + monkeypatch.setattr(notify.settings, "sendgrid_api_key", None) + monkeypatch.setattr(notify.settings, "notification_sender", None) + with patch("sendgrid.SendGridAPIClient") as sg: + await notify.notify_run_completed(_triaged(), notify.POSTED) + sg.assert_not_called() + + +async def test_skips_when_no_channel_is_configured(monkeypatch): + # Credentials present but no destination: still nothing to do. + _configure_sending(monkeypatch, channels={}) + with patch("sendgrid.SendGridAPIClient") as sg: + await notify.notify_run_completed(_triaged(), notify.POSTED) + sg.assert_not_called() + + +async def test_sends_once_to_the_channel_address(monkeypatch): + _configure_sending(monkeypatch) + run = _triaged() + with patch("sendgrid.SendGridAPIClient") as sg: + await notify.notify_run_completed(run, notify.POSTED) + + sg.assert_called_once_with(api_key="key") + client = sg.return_value + assert client.send.call_count == 1 + sent = client.send.call_args.kwargs["message"].get() + assert sent["personalizations"][0]["to"] == [{"email": NEWTAB_CHANNEL}] + assert sent["from"] == {"email": "hackbot@mozilla.com"} + assert "2014702" in sent["subject"] + assert "Posted to Bugzilla" in sent["content"][0]["value"] + + +async def test_stamps_notified_at_on_success(monkeypatch): + _configure_sending(monkeypatch) + run = _triaged() + with patch("sendgrid.SendGridAPIClient"): + await notify.notify_run_completed(run, notify.POSTED) + assert isinstance(run.notified_at, datetime) + + +async def test_does_not_resend_an_already_notified_run(monkeypatch): + # Pub/Sub is at-least-once and an email is not idempotent, unlike an action row. + _configure_sending(monkeypatch) + run = _triaged() + run.notified_at = datetime.now(timezone.utc) + with patch("sendgrid.SendGridAPIClient") as sg: + await notify.notify_run_completed(run, notify.POSTED) + sg.assert_not_called() + + +async def test_send_failure_is_swallowed_and_leaves_run_unnotified(monkeypatch): + # A missed channel message must not fail the completion path, and must stay + # eligible for a later retry. + _configure_sending(monkeypatch) + run = _triaged() + with patch("sendgrid.SendGridAPIClient") as sg: + sg.return_value.send.side_effect = RuntimeError("sendgrid down") + await notify.notify_run_completed(run, notify.POSTED) + assert run.notified_at is None diff --git a/uv.lock b/uv.lock index 8e61cf4f57..c417b3023a 100644 --- a/uv.lock +++ b/uv.lock @@ -2665,6 +2665,8 @@ dependencies = [ { name = "phabricator-client" }, { name = "pydantic" }, { name = "pydantic-settings" }, + { name = "requests" }, + { name = "sendgrid" }, { name = "sentry-sdk" }, { name = "sqlalchemy", extra = ["asyncio"] }, { name = "uvicorn", extra = ["standard"] }, @@ -2696,6 +2698,8 @@ requires-dist = [ { name = "pydantic-settings", specifier = ">=2.1.0" }, { name = "pytest", marker = "extra == 'dev'", specifier = ">=8.0.0" }, { name = "pytest-asyncio", marker = "extra == 'dev'", specifier = ">=0.23.0" }, + { name = "requests", specifier = ">=2.31.0" }, + { name = "sendgrid", specifier = ">=6.11.0" }, { name = "sentry-sdk", specifier = ">=2.51.0" }, { name = "sqlalchemy", extras = ["asyncio"], specifier = ">=2.0.25" }, { name = "uvicorn", extras = ["standard"], specifier = ">=0.27.0" },