diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/notify.py b/agents/frontend-triage/hackbot_agents/frontend_triage/notify.py index e1c252f3ff..1ebb222c1f 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/notify.py +++ b/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 "" + 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", diff --git a/agents/frontend-triage/tests/test_notify.py b/agents/frontend-triage/tests/test_notify.py index d62ae31634..f56081f950 100644 --- a/agents/frontend-triage/tests/test_notify.py +++ b/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 (