diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/notify.py b/agents/frontend-triage/hackbot_agents/frontend_triage/notify.py index 0f865979d1..1a43c09160 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/notify.py +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/notify.py @@ -53,22 +53,29 @@ 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.""" +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: assessment = result.severity_assessment suggested = (assessment.suggested or "") if assessment else "" - # The severity the run *judged*, which is not always the one it wrote to the bug: - # `rules/severity-assessment.md` holds back the field change when the severity - # confidence is not high. An S1 it only suspects is still worth flagging. - urgent = suggested.strip().upper() == URGENT_SEVERITY - - 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()}" + return suggested.strip().upper() == URGENT_SEVERITY + + +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. headline = f"*{headline}*" - if urgent: + if _is_urgent(result): headline = f":red_circle: {headline} ({URGENT_SEVERITY})" return "\n".join( @@ -79,6 +86,68 @@ def build_message(result: FrontendTriageResult, *, run_id: str) -> str: ) +def _severity_field(result: FrontendTriageResult) -> str | None: + assessment = result.severity_assessment + suggested = (assessment.suggested or "").strip() if assessment else "" + if not suggested: + return None + value = suggested.upper() + # The assessment's own confidence, not the run's: `rules/severity-assessment.md` + # holds the field change back below `high`, so anything less is a suggestion the + # bug did not receive, and reading it as applied would be wrong. + confidence = (assessment.confidence or "").strip() if assessment else "" + if confidence and confidence.lower() != "high": + value += f" (suggested, {confidence.lower()} confidence)" + return f"*Severity*\n{value}" + + +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): + headline = f":red_circle: *{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: @@ -106,4 +175,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), + ) diff --git a/agents/frontend-triage/tests/test_notify.py b/agents/frontend-triage/tests/test_notify.py index 002601337d..552224adf6 100644 --- a/agents/frontend-triage/tests/test_notify.py +++ b/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, @@ -153,6 +157,154 @@ 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. + headline = _blocks( + severity_assessment=SeverityAssessment(suggested="S1", confidence="high") + )[0]["text"]["text"] + assert headline == f":red_circle: *S1* *{BUG_LINK}*\n{SUMMARY}" + + +def test_the_fields_grid_carries_the_severity_and_the_component(): + assert _blocks()[1]["fields"] == [ + {"type": "mrkdwn", "text": "*Severity*\nS3"}, + {"type": "mrkdwn", "text": "*Component*\nFirefox :: New Tab Page"}, + ] + + +def test_a_severity_the_bug_did_not_receive_says_so(): + # Below `high` the assessment is held back from the bug, so reporting it plainly + # would read as a field that was written when it was not. + fields = _blocks( + severity_assessment=SeverityAssessment(suggested="S2", confidence="medium") + )[1]["fields"] + assert fields[0]["text"] == "*Severity*\nS2 (suggested, medium confidence)" + + +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"" + ) + + +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():