Add a button to the frontend-triage Slack notification to start a bug-fix run

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

@@ -23,6 +23,8 @@
 
 from hackbot_runtime.actions.recorder import ActionsRecorder
 from hackbot_runtime.actions.slack import HACKBOT_UI_URL, record_message
+from hackbot_runtime.slack_kit import create_start_agent_run_button
+from slack_sdk.models.blocks import ConfirmObject
 
 from .agent import FrontendTriageResult
 from .config import REPORTABLE_SEVERITY_CONFIDENCES, SLACK_CHANNELS
@@ -37,6 +39,12 @@
 URGENT_SEVERITY = "S1"
 
 
+HELD_NOTE = (
+    ":hourglass: *Held for review.* This analysis is recorded but not on the bug. "
+    "Starting the fix posts it first."
+)
+
+
 def _link(url: str, label: str) -> str:
     return f"<{url}|{label}>"
 
@@ -89,12 +97,11 @@ def build_message(result: FrontendTriageResult, *, run_id: str) -> str:
     if _is_urgent(result):
         headline = f":red_circle: {headline} (suggested {URGENT_SEVERITY})"
 
-    return "\n".join(
-        [
-            headline,
-            _link(RUN_URL.format(run_id=run_id), "frontend-triage run details"),
-        ]
-    )
+    lines = [headline]
+    if not result.auto_apply:
+        lines.append(HELD_NOTE)
+    lines.append(_link(RUN_URL.format(run_id=run_id), "frontend-triage run details"))
+    return "\n".join(lines)
 
 
 def _severity_field(result: FrontendTriageResult) -> str | None:
@@ -121,6 +128,41 @@ def _component_field(result: FrontendTriageResult) -> str | None:
     return f"*Component*\n{result.product.strip()} :: {result.component.strip()}"
 
 
+def _bug_fix_button(result: FrontendTriageResult, *, run_id: str) -> dict:
+    """The one-press offer to go fix the bug this run just analyzed.
+
+    A dict, not the SDK model: `summary.json` is written with `default=str`, so a
+    model recorded unconverted is stored as the string "<slack_sdk.ButtonElement>"
+    rather than failing, and only shows up when Slack rejects the message.
+    """
+    label = "Fix this bug" if result.auto_apply else "Post analysis & fix"
+    confirm_text = (
+        "This "
+        if result.auto_apply
+        else f"This posts the triage analysis to bug {result.bug_id}, then "
+    ) + (
+        "starts the `bug-fix` agent, which will comment on Bugzilla or submit "
+        "a revision to Phabricator on its own."
+    )
+
+    return create_start_agent_run_button(
+        label,
+        agent_name="bug-fix",
+        params={"bug_id": result.bug_id},
+        dedupe_key=f"frontend-triage-run:{run_id}",
+        apply_run_id=run_id,
+        confirm=ConfirmObject(
+            title=f"Start a fix for bug {result.bug_id}?",
+            text=confirm_text,
+            # Slack's defaults are a bare Yes/No, which reads as generic next to
+            # a button that writes to Bugzilla.
+            confirm="Yes, go ahead",
+            deny="Cancel",
+        ),
+        style="primary",
+    ).to_dict()
+
+
 def build_blocks(result: FrontendTriageResult, *, run_id: str) -> list[dict]:
     headline = f"*{_bug_link(result)}*"
     summary = _summary(result)
@@ -148,6 +190,17 @@ def build_blocks(result: FrontendTriageResult, *, run_id: str) -> list[dict]:
             }
         )
 
+    if not result.auto_apply:
+        blocks.append(
+            {"type": "section", "text": {"type": "mrkdwn", "text": HELD_NOTE}}
+        )
+
+    blocks.append(
+        {
+            "type": "actions",
+            "elements": [_bug_fix_button(result, run_id=run_id)],
+        }
+    )
     blocks.append(
         {
             "type": "context",

agents/frontend-triage/tests/test_notify.py

@@ -6,6 +6,7 @@
 ask for it.
 """
 
+import json
 import re
 
 import pytest
@@ -15,6 +16,7 @@
 )
 from hackbot_agents.frontend_triage.config import TRIAGE_SCOPE
 from hackbot_agents.frontend_triage.notify import (
+    HELD_NOTE,
     build_blocks,
     build_message,
     channel_for,
@@ -185,8 +187,13 @@ 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_layout_reads_bug_then_facts_then_button_then_run():
+    assert [b["type"] for b in _blocks()] == [
+        "section",
+        "section",
+        "actions",
+        "context",
+    ]
 
 
 def test_the_headline_links_the_bug_and_puts_the_summary_under_it():
@@ -258,9 +265,10 @@ def test_a_severity_below_the_threshold_is_not_reported():
 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.
+    # And with none of them, the grid itself goes rather than rendering empty.
     assert [b["type"] for b in _blocks(severity_assessment=None, product=None)] == [
         "section",
+        "actions",
         "context",
     ]
 
@@ -273,10 +281,25 @@ def test_the_run_sits_in_the_context_line():
     )
 
 
-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 button (#6739) ---
+
+
+def _button_value(**overrides) -> dict:
+    return json.loads(_block_of("actions", **overrides)["elements"][0]["value"])
+
+
+def test_the_button_carries_what_the_receiver_needs_to_start_the_run():
+    # The one thing here worth pinning: this value crosses a process boundary and
+    # is parsed by hackbot-api, so a wrong `dedupe_key` silently buys a second run
+    # per click and a wrong `apply_run_id` starts the agent on an unanalysed bug.
+    # Neither shows up anywhere else, and neither moves when this file is
+    # rearranged.
+    assert _button_value() == {
+        "agent_name": "bug-fix",
+        "params": {"bug_id": BUG_ID},
+        "dedupe_key": f"frontend-triage-run:{RUN_ID}",
+        "apply_run_id": RUN_ID,
+    }
 
 
 # --- the two renderings say the same things ---
@@ -350,18 +373,50 @@ def test_the_blocks_say_everything_the_fallback_text_says(overrides):
     assert _urls(text) <= _urls(blocks)
 
 
-def test_a_run_that_was_not_auto_applied_reports_nothing():
-    # Medium and low results wrote nothing to the bug, so there is nothing to report.
+def test_a_held_run_reports_too_because_its_reader_needs_the_button():
+    # It used to report nothing. The button is why that changed: a result the agent
+    # would not let apply itself is exactly the one a human has to decide on, and
+    # nobody would find it without being told it exists.
+    recorder = ActionsRecorder()
+    action = record_notification(
+        recorder, _result(auto_apply=False, confidence="medium"), run_id=RUN_ID
+    )
+
+    assert action is not None
+    blocks = action["params"]["blocks"]
+    assert any(b["type"] == "actions" for b in blocks)
+    # And it says the analysis is not on the bug, in both renderings.
+    assert HELD_NOTE in action["params"]["text"]
+    assert any(HELD_NOTE == b.get("text", {}).get("text") for b in blocks)
+
+
+def test_a_run_with_nothing_to_fix_reports_nothing():
+    # No fix to offer, so a button would be an offer to act on a bug this agent
+    # just called out of scope, and the channel would have nothing to do with it.
     recorder = ActionsRecorder()
     assert (
         record_notification(
-            recorder, _result(auto_apply=False, confidence="medium"), run_id=RUN_ID
+            recorder,
+            _result(actionable=False, auto_apply=False, confidence="low"),
+            run_id=RUN_ID,
         )
         is None
     )
     assert recorder.actions == []
 
 
+def test_a_run_that_reported_no_verdict_still_reports():
+    # `is not False`, matching `may_apply_unattended`: a plan that did not parse is
+    # treated as having something to fix rather than as out of scope.
+    recorder = ActionsRecorder()
+    assert (
+        record_notification(
+            recorder, _result(actionable=None, auto_apply=False), run_id=RUN_ID
+        )
+        is not None
+    )
+
+
 def test_a_run_in_an_unowned_component_reports_nothing():
     recorder = ActionsRecorder()
     assert (