Add Block Kit layout to frontend-triage notifications

agents/frontend-triage/hackbot_agents/frontend_triage/notify.py

@@ -54,29 +54,39 @@ def channel_for(product: str | None, component: str | None) -> str | None:
     return SLACK_CHANNELS.get(f"{product.strip()} :: {component.strip()}")
 
 
-def build_message(result: FrontendTriageResult, *, run_id: str) -> str:
-    """Render the notification for an auto-applied run."""
-    assessment = result.severity_assessment
+def _bug_link(result: FrontendTriageResult) -> str:
+    return _link(BUG_URL.format(bug_id=result.bug_id), f"Bug {result.bug_id}")
+
+
+def _summary(result: FrontendTriageResult) -> str:
+    return result.summary.strip() if result.summary else ""
+
+
+def _is_urgent(result: FrontendTriageResult) -> bool:
     # Gated on the severity's own confidence, not the run's -- the two are independent,
     # and a run that localized the cause precisely can still be unsure how bad the bug
     # is. The comment drops its severity block on the same threshold, so without this
     # Slack could shout S1 while the bug says nothing about severity at all.
     #
     # Both values arrive normalized from `parse_plan`.
-    urgent = bool(
+    assessment = result.severity_assessment
+    return bool(
         assessment
         and assessment.suggested == URGENT_SEVERITY
         and assessment.confidence in REPORTABLE_SEVERITY_CONFIDENCES
     )
 
-    headline = _link(BUG_URL.format(bug_id=result.bug_id), f"Bug {result.bug_id}")
-    if result.summary and result.summary.strip():
-        headline += f" — {result.summary.strip()}"
+
+def build_message(result: FrontendTriageResult, *, run_id: str) -> str:
+    headline = _bug_link(result)
+    summary = _summary(result)
+    if summary:
+        headline += f" — {summary}"
     # The level is spelled out next to the emoji, so it still reads as an S1 for anyone
     # whose client does not render one. "suggested", because a bare "(S1)" would read as
     # the bug having been marked S1, and nothing was written to the field.
     headline = f"*{headline}*"
-    if urgent:
+    if _is_urgent(result):
         headline = f":red_circle: {headline} (suggested {URGENT_SEVERITY})"
 
     return "\n".join(
@@ -87,6 +97,72 @@ def build_message(result: FrontendTriageResult, *, run_id: str) -> str:
     )
 
 
+def _severity_field(result: FrontendTriageResult) -> str | None:
+    """The level this run suggests, when it is sure enough to suggest one.
+
+    Never a level the bug received: this agent only comments (`ENABLED_ACTION_TYPES`),
+    and `rules/severity-assessment.md` tells it as much, so the label says suggested
+    rather than leaving a reader to assume the field was set. Below the reportable
+    threshold there is no field at all, on the same gate as the comment's severity
+    block and the headline's marker.
+    """
+    assessment = result.severity_assessment
+    if not assessment or assessment.confidence not in REPORTABLE_SEVERITY_CONFIDENCES:
+        return None
+    if not assessment.suggested:
+        return None
+    return f"*Suggested severity*\n{assessment.suggested}"
+
+
+def _component_field(result: FrontendTriageResult) -> str | None:
+    """Where the bug lives, for the channels that own more than one component."""
+    if not result.product or not result.component:
+        return None
+    return f"*Component*\n{result.product.strip()} :: {result.component.strip()}"
+
+
+def build_blocks(result: FrontendTriageResult, *, run_id: str) -> list[dict]:
+    headline = f"*{_bug_link(result)}*"
+    summary = _summary(result)
+    if summary:
+        headline += f"\n{summary}"
+    if _is_urgent(result):
+        # Worded as the text version words it: nothing was written to the severity
+        # field, so a bare "S1" would read as the bug having been marked one.
+        headline = f":red_circle: *suggested {URGENT_SEVERITY}* {headline}"
+
+    blocks: list[dict] = [
+        {"type": "section", "text": {"type": "mrkdwn", "text": headline}}
+    ]
+
+    fields = [
+        field
+        for field in (_severity_field(result), _component_field(result))
+        if field is not None
+    ]
+    if fields:
+        blocks.append(
+            {
+                "type": "section",
+                "fields": [{"type": "mrkdwn", "text": field} for field in fields],
+            }
+        )
+
+    blocks.append(
+        {
+            "type": "context",
+            "elements": [
+                {
+                    "type": "mrkdwn",
+                    "text": "Triaged by frontend-triage · "
+                    + _link(RUN_URL.format(run_id=run_id), "run details"),
+                }
+            ],
+        }
+    )
+    return blocks
+
+
 def record_notification(
     recorder: ActionsRecorder, result: FrontendTriageResult, *, run_id: str
 ) -> dict | None:
@@ -114,4 +190,9 @@ def record_notification(
         return None
 
     logger.info("Bug %s: reporting triage to %s", result.bug_id, channel)
-    return record_message(recorder, channel, build_message(result, run_id=run_id))
+    return record_message(
+        recorder,
+        channel,
+        build_message(result, run_id=run_id),
+        blocks=build_blocks(result, run_id=run_id),
+    )

agents/frontend-triage/tests/test_notify.py

@@ -6,12 +6,16 @@
 ask for it.
 """
 
+import re
+
+import pytest
 from hackbot_agents.frontend_triage.agent import (
     FrontendTriageResult,
     SeverityAssessment,
 )
 from hackbot_agents.frontend_triage.config import TRIAGE_SCOPE
 from hackbot_agents.frontend_triage.notify import (
+    build_blocks,
     build_message,
     channel_for,
     record_notification,
@@ -166,6 +170,184 @@ def test_an_auto_applied_run_records_one_slack_action():
     assert [a["type"] for a in recorder.actions] == ["slack.post_message"]
     assert action["params"]["channel"] == "#hnt-dev-triage"
     assert action["params"]["text"] == build_message(_result(), run_id=RUN_ID)
+    # The layout travels with the text, which stays the fallback.
+    assert action["params"]["blocks"] == build_blocks(_result(), run_id=RUN_ID)
+
+
+# --- the Block Kit layout ---
+
+
+def _blocks(**overrides) -> list[dict]:
+    return build_blocks(_result(**overrides), run_id=RUN_ID)
+
+
+def _block_of(kind: str, **overrides) -> dict | None:
+    return next((b for b in _blocks(**overrides) if b["type"] == kind), None)
+
+
+def test_the_layout_reads_bug_then_facts_then_run():
+    assert [b["type"] for b in _blocks()] == ["section", "section", "context"]
+
+
+def test_the_headline_links_the_bug_and_puts_the_summary_under_it():
+    assert _blocks()[0] == {
+        "type": "section",
+        "text": {"type": "mrkdwn", "text": f"*{BUG_LINK}*\n{SUMMARY}"},
+    }
+
+
+def test_a_bug_with_no_summary_is_just_the_link():
+    for summary in (None, "", "   "):
+        assert _blocks(summary=summary)[0]["text"]["text"] == f"*{BUG_LINK}*"
+
+
+def test_an_s1_leads_the_headline():
+    # Marked and named, so it still reads as an S1 where the emoji does not render,
+    # and named "suggested" because nothing was written to the severity field.
+    headline = _blocks(
+        severity_assessment=SeverityAssessment(suggested="S1", confidence="high")
+    )[0]["text"]["text"]
+    assert headline == f":red_circle: *suggested S1* *{BUG_LINK}*\n{SUMMARY}"
+
+
+@pytest.mark.parametrize("confidence", ["high", "medium"])
+def test_an_s1_the_run_is_unsure_of_still_leads(confidence):
+    # The same gate the comment's severity block uses: reportable means marked.
+    headline = _blocks(
+        severity_assessment=SeverityAssessment(suggested="S1", confidence=confidence)
+    )[0]["text"]["text"]
+    assert headline.startswith(":red_circle: *suggested S1*")
+
+
+def test_an_s1_below_the_threshold_is_not_marked():
+    # At `low` the agent says nothing about severity anywhere, so Slack must not
+    # shout a level the bug itself never mentions.
+    headline = _blocks(
+        severity_assessment=SeverityAssessment(suggested="S1", confidence="low")
+    )[0]["text"]["text"]
+    assert headline == f"*{BUG_LINK}*\n{SUMMARY}"
+
+
+def test_the_fields_grid_carries_the_severity_and_the_component():
+    assert _blocks()[1]["fields"] == [
+        {"type": "mrkdwn", "text": "*Suggested severity*\nS3"},
+        {"type": "mrkdwn", "text": "*Component*\nFirefox :: New Tab Page"},
+    ]
+
+
+def test_the_severity_is_always_labelled_a_suggestion():
+    # This agent only comments; it cannot set the field, so no confidence makes the
+    # level something the bug received.
+    for confidence in ("high", "medium"):
+        fields = _blocks(
+            severity_assessment=SeverityAssessment(
+                suggested="S2", confidence=confidence
+            )
+        )[1]["fields"]
+        assert fields[0]["text"] == "*Suggested severity*\nS2"
+
+
+def test_a_severity_below_the_threshold_is_not_reported():
+    # Same gate as the marker and as the comment's severity block.
+    fields = _blocks(
+        severity_assessment=SeverityAssessment(suggested="S2", confidence="low")
+    )[1]["fields"]
+    assert [f["text"] for f in fields] == ["*Component*\nFirefox :: New Tab Page"]
+
+
+def test_a_field_with_nothing_to_say_is_dropped():
+    fields = _blocks(severity_assessment=None)[1]["fields"]
+    assert [f["text"] for f in fields] == ["*Component*\nFirefox :: New Tab Page"]
+    # And with neither, the grid itself goes rather than rendering empty.
+    assert [b["type"] for b in _blocks(severity_assessment=None, product=None)] == [
+        "section",
+        "context",
+    ]
+
+
+def test_the_run_sits_in_the_context_line():
+    element = _block_of("context")["elements"][0]
+    assert element["text"] == (
+        "Triaged by frontend-triage · "
+        f"<https://hackbot.moz.tools/runs/{RUN_ID}|run details>"
+    )
+
+
+def test_the_notification_asks_for_nothing_yet():
+    # The layout is the whole change: no interactive element is posted until the
+    # buttons land, so nothing here can be clicked.
+    assert _block_of("actions") is None
+
+
+# --- the two renderings say the same things ---
+
+_LINK = re.compile(r"<([^|>]+)\|([^>]+)>")
+# Punctuation that only ever joins words: dropped so that "(S1)" and "*S1*" are the
+# same fact said twice, rather than two tokens that happen to look alike.
+_PUNCTUATION = "*()·,—"
+
+
+def _visible_text(blocks: list[dict]) -> str:
+    """Every string a reader sees in `blocks`, whatever block it sits in."""
+    parts: list[str] = []
+    for block in blocks:
+        if isinstance(block.get("text"), dict):
+            parts.append(block["text"]["text"])
+        for field in block.get("fields", []):
+            parts.append(field["text"])
+        for element in block.get("elements", []):
+            if isinstance(element.get("text"), dict):
+                parts.append(element["text"]["text"])
+            elif isinstance(element.get("text"), str):
+                parts.append(element["text"])
+    return "\n".join(parts)
+
+
+def _facts(text: str) -> set[str]:
+    """What `text` tells a reader, as words, with link markup reduced to its label."""
+    labelled = _LINK.sub(lambda match: match.group(2), text)
+    return {word.strip(_PUNCTUATION) for word in labelled.split()} - {""}
+
+
+def _urls(text: str) -> set[str]:
+    return {match.group(1) for match in _LINK.finditer(text)}
+
+
+@pytest.mark.parametrize(
+    "overrides",
+    [
+        {},
+        {"summary": None},
+        {"severity_assessment": None},
+        {"severity_assessment": SeverityAssessment(suggested="S1", confidence="high")},
+        {
+            "severity_assessment": SeverityAssessment(
+                suggested="S1", confidence="medium"
+            )
+        },
+        {"product": None, "component": None},
+    ],
+    ids=[
+        "ordinary",
+        "no-summary",
+        "no-severity",
+        "urgent",
+        "urgent-suggested",
+        "no-component",
+    ],
+)
+def test_the_blocks_say_everything_the_fallback_text_says(overrides):
+    # The blocks are what almost everyone reads, and the text is what Slack falls back
+    # to. They may lay the same facts out differently, but nothing may be in the
+    # fallback and missing from the blocks: that would be a fact only the people
+    # reading a push notification ever see.
+    result = _result(**overrides)
+    text = build_message(result, run_id=RUN_ID)
+    blocks = _visible_text(build_blocks(result, run_id=RUN_ID))
+
+    assert _facts(text) <= _facts(blocks)
+    # Labels may be worded differently; the things they link to may not.
+    assert _urls(text) <= _urls(blocks)
 
 
 def test_a_run_that_was_not_auto_applied_reports_nothing():