From 487d11c699ee3c105b71d9682f1b1f01b475fd5c Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Tue, 4 Aug 2026 15:20:33 +0200 Subject: [PATCH 1/2] Suppress redundant comment actions when a bug-fix run submits or updates a patch. --- .../bug_fix/prompts/follow-up.md | 4 +- .../hackbot_agents/bug_fix/prompts/system.md | 2 + services/hackbot-api/app/actions_applier.py | 38 +++++- services/hackbot-api/app/database/models.py | 3 +- services/hackbot-api/app/routers/runs.py | 4 +- .../hackbot-api/tests/test_actions_applier.py | 111 +++++++++++++++++- services/hackbot-ui/app/globals.css | 4 + services/hackbot-ui/components/RunDetail.tsx | 8 +- services/hackbot-ui/lib/types.ts | 2 +- 9 files changed, 157 insertions(+), 19 deletions(-) diff --git a/agents/bug-fix/hackbot_agents/bug_fix/prompts/follow-up.md b/agents/bug-fix/hackbot_agents/bug_fix/prompts/follow-up.md index 978b82f7de..f772941f18 100644 --- a/agents/bug-fix/hackbot_agents/bug_fix/prompts/follow-up.md +++ b/agents/bug-fix/hackbot_agents/bug_fix/prompts/follow-up.md @@ -10,9 +10,7 @@ A quoted comment rarely stands alone: an inline one only makes sense next to the Then address each quoted comment by taking the matching path: -- If it requests a code change (a fix, tweak, or follow-up to the patch): make the necessary source changes, verify them, and call phabricator_update_patch with revision_id={revision_id} so the existing revision D{revision_id} is updated. +- If any quoted comment requests a code change (a fix, tweak, or follow-up to the patch): make the necessary source changes, verify them, and call phabricator_update_patch with revision_id={revision_id} so the existing revision D{revision_id} is updated. Do not record a comment action in that run. - If it is only a question or a request for clarification (no code change is warranted): do not edit the source or submit a patch. Investigate, then reply on the revision by calling phabricator_add_comment with revision_id={revision_id}. This posts on D{revision_id} itself; do not answer via a Bugzilla comment. -A single review can mix both: make the code changes it asks for and answer the questions it raises in the same run. - If you are unsure, prefer answering with a comment over making speculative code changes. diff --git a/agents/bug-fix/hackbot_agents/bug_fix/prompts/system.md b/agents/bug-fix/hackbot_agents/bug_fix/prompts/system.md index 1ee5be7ac7..080ba7d308 100644 --- a/agents/bug-fix/hackbot_agents/bug_fix/prompts/system.md +++ b/agents/bug-fix/hackbot_agents/bug_fix/prompts/system.md @@ -69,6 +69,8 @@ When you spawn an investigator via the Task tool, write a complete, self-contain The `actions` MCP tools (`bugzilla_update_bug`, `bugzilla_add_comment`, and the Phabricator actions this run enables) do **not** mutate Bugzilla or Phabricator directly. Use `phabricator_add_comment`, when available, to reply on a Differential revision (for example, to answer a question when no code change is needed); use `phabricator_submit_patch` to deliver a code fix as a new revision, or `phabricator_update_patch` to deliver it as a new diff on the existing revision. Only the actions listed in your toolset are available: if one is missing, it does not apply to this run. They record an intended action into the run's `summary.json` for a human reviewer (or a downstream apply step) to enact. Treat each recorded action as a final, irrevocable proposal — once recorded it appears in the run output verbatim. +When you record `phabricator_submit_patch` or `phabricator_update_patch`, do not record `bugzilla_add_comment` or `phabricator_add_comment` in the same run. The patch action is the complete response for that run. + Before calling any action tool, state in your response: - **What** action you are recording and **why** (cite the specific rule) diff --git a/services/hackbot-api/app/actions_applier.py b/services/hackbot-api/app/actions_applier.py index 49f2f020ce..77576fb38a 100644 --- a/services/hackbot-api/app/actions_applier.py +++ b/services/hackbot-api/app/actions_applier.py @@ -5,9 +5,10 @@ manageable in the UI. Whether they're then applied *automatically* depends on the agent's `auto_apply_actions` opt-in (see `app/agents.py`); either way they can be applied on demand (manual apply-all from the UI). Application runs each -pending row through the handler registry in `hackbot_runtime.actions.handlers` -and is idempotent per action — an already-`applied` row is never re-applied, so -Pub/Sub retries and repeated manual applies are safe. +pending or failed row through the handler registry in +`hackbot_runtime.actions.handlers` and is idempotent per action — terminal +`applied` and `suppressed` rows are never re-applied, so Pub/Sub retries and +repeated manual applies are safe. """ from __future__ import annotations @@ -24,6 +25,7 @@ merge_resolved, plan_coalesced_groups, ) +from hackbot_runtime.actions.phabricator import PATCH_ACTION_TYPES from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession @@ -35,6 +37,8 @@ log = logging.getLogger(__name__) _PLACEHOLDER_RE = re.compile(r"\{\{actions\.([^.}]+)\.([^}]+)\}\}") +_COMMENT_ACTION_TYPES = frozenset({"bugzilla.add_comment", "phabricator.add_comment"}) +_APPLICABLE_STATUSES = frozenset({"pending", "failed"}) def resolve_placeholders(value: Any, results_by_ref: dict[str, dict]) -> Any: @@ -151,7 +155,7 @@ async def _dispatch( async def _apply_pending_rows( db: AsyncSession, run: Run, rows: list[tuple[RunAction, list[dict]]] ) -> None: - """Apply every not-yet-`applied` row in `rows`, committing per action. + """Apply every pending or failed row in `rows`, committing per action. Same-bug Bugzilla field changes are coalesced with the closest comment into a single `PUT /bug/{id}` so Bugzilla applies them as one transaction (one @@ -162,14 +166,36 @@ async def _apply_pending_rows( that are already `applied` (seeded from prior applies) plus ones applied earlier in this pass, so a later (even manual) apply can still reference an earlier action's result. + + A run that submits or updates a Phabricator patch suppresses any separate + Bugzilla or Phabricator comment actions. The patch is the complete response + for that run; suppressed actions stay visible but are never dispatched. """ + if any(row.type in PATCH_ACTION_TYPES for row, _ in rows): + suppressed = False + for row, _ in rows: + if row.status in _APPLICABLE_STATUSES and row.type in _COMMENT_ACTION_TYPES: + log.info( + "Suppressing %s action %s for patch-submitting run %s", + row.type, + row.idx, + run.run_id, + ) + row.status = "suppressed" + row.result = None + row.error = None + row.applied_at = None + suppressed = True + if suppressed: + await db.commit() + results_by_ref: dict[str, dict] = { row.ref: row.result for row, _ in rows if row.ref and row.status == "applied" and row.result is not None } - pending = [(row, att) for row, att in rows if row.status != "applied"] + pending = [(row, att) for row, att in rows if row.status in _APPLICABLE_STATUSES] # Plan which pending rows coalesce into one bug PUT (indices into `pending`). # Drop any group whose rows carry a `ref`: nothing should reference a @@ -255,7 +281,7 @@ async def on_run_completed(db: AsyncSession, run: Run) -> None: async def apply_all_pending(db: AsyncSession, run: Run) -> None: - """Apply all of a run's not-yet-`applied` actions on demand (manual). + """Apply all of a run's pending or failed actions on demand (manual). Ensures the rows exist first, so this works whether or not they were recorded automatically on completion. diff --git a/services/hackbot-api/app/database/models.py b/services/hackbot-api/app/database/models.py index 6dd1b32dba..bcfcb89a44 100644 --- a/services/hackbot-api/app/database/models.py +++ b/services/hackbot-api/app/database/models.py @@ -46,7 +46,8 @@ class RunAction(Base): One row per entry in `summary.json["actions"]`, upserted by the action-applier the first time it sees a run so replays (Pub/Sub at-least-once delivery) can - skip actions already `applied` and only retry `pending`/`failed` ones. + skip terminal `applied`/`suppressed` actions and only retry `pending`/`failed` + ones. """ __tablename__ = "run_actions" diff --git a/services/hackbot-api/app/routers/runs.py b/services/hackbot-api/app/routers/runs.py index 661b2d859f..5dce4212d1 100644 --- a/services/hackbot-api/app/routers/runs.py +++ b/services/hackbot-api/app/routers/runs.py @@ -219,8 +219,8 @@ async def apply_run_actions( ) -> list[RunActionDoc]: """Manually apply all of a run's pending actions (apply-all). - Idempotent — already-applied actions are skipped — so this is safe to - click again after a partial failure. Returns the actions' updated state. + Idempotent — applied and suppressed actions are skipped — so this is safe + to click again after a partial failure. Returns the actions' updated state. """ run = await db.get(Run, run_id) if run is None: diff --git a/services/hackbot-api/tests/test_actions_applier.py b/services/hackbot-api/tests/test_actions_applier.py index 344c41fd59..ae10344ee8 100644 --- a/services/hackbot-api/tests/test_actions_applier.py +++ b/services/hackbot-api/tests/test_actions_applier.py @@ -10,6 +10,7 @@ from dataclasses import dataclass, field from types import SimpleNamespace +import pytest from app import actions_applier from app.actions_applier import ( apply_all_pending, @@ -210,6 +211,110 @@ async def test_apply_pending_rows_retries_failed_and_skips_applied(monkeypatch): assert pending.status == "applied" +# --- patch runs suppress separate comments -------------------------------- # + + +@pytest.mark.parametrize( + "patch_type", ["phabricator.submit_patch", "phabricator.update_patch"] +) +@pytest.mark.parametrize("patch_status", ["pending", "failed"]) +@pytest.mark.parametrize( + "comment_type", ["bugzilla.add_comment", "phabricator.add_comment"] +) +async def test_patch_run_suppresses_comments_regardless_of_order( + monkeypatch, patch_type, patch_status, comment_type +): + calls = [] + + async def fake_dispatch(run, action_type, params, attachments): + calls.append(action_type) + return SimpleNamespace(status="applied", result={}, error=None) + + monkeypatch.setattr(actions_applier, "_dispatch", fake_dispatch) + comment = _row( + 0, + "failed", + action_type=comment_type, + params={"text": "redundant"}, + result={"old": "result"}, + error="old error", + applied_at="old timestamp", + ) + patch = _row(1, patch_status, action_type=patch_type) + rows = [(comment, []), (patch, [])] + db = _FakeDB() + run = _FakeRun(status=RunStatus.succeeded.value) + + await actions_applier._apply_pending_rows(db, run, rows) + + assert calls == [patch_type] + assert comment.status == "suppressed" + assert comment.result is None + assert comment.error is None + assert comment.applied_at is None + assert patch.status == "applied" + + # Replays and manual apply-all leave suppressed comments untouched. + await actions_applier._apply_pending_rows(db, run, rows) + assert calls == [patch_type] + + +@pytest.mark.parametrize( + "comment_type", ["bugzilla.add_comment", "phabricator.add_comment"] +) +async def test_non_patch_run_applies_comments(monkeypatch, comment_type): + calls = [] + + async def fake_dispatch(run, action_type, params, attachments): + calls.append(action_type) + return SimpleNamespace(status="applied", result={}, error=None) + + monkeypatch.setattr(actions_applier, "_dispatch", fake_dispatch) + comment = _row(0, "pending", action_type=comment_type, params={"text": "hi"}) + + await actions_applier._apply_pending_rows( + _FakeDB(), _FakeRun(status=RunStatus.succeeded.value), [(comment, [])] + ) + + assert calls == [comment_type] + assert comment.status == "applied" + + +async def test_patch_run_does_not_coalesce_suppressed_bugzilla_comment(monkeypatch): + calls = [] + + async def fake_dispatch(run, action_type, params, attachments): + calls.append((action_type, params)) + return SimpleNamespace(status="applied", result={"bug_id": 5}, error=None) + + monkeypatch.setattr(actions_applier, "_dispatch", fake_dispatch) + update = _row( + 0, + "pending", + action_type="bugzilla.update_bug", + params={"bug_id": 5, "changes": {"status": "RESOLVED"}}, + ) + comment = _row( + 1, + "pending", + action_type="bugzilla.add_comment", + params={"bug_id": 5, "text": "redundant"}, + ) + patch = _row(2, "pending", action_type="phabricator.submit_patch") + + await actions_applier._apply_pending_rows( + _FakeDB(), + _FakeRun(status=RunStatus.succeeded.value), + [(update, []), (comment, []), (patch, [])], + ) + + assert calls == [ + ("bugzilla.update_bug", {"bug_id": 5, "changes": {"status": "RESOLVED"}}), + ("phabricator.submit_patch", {}), + ] + assert comment.status == "suppressed" + + # --- coalescing same-bug Bugzilla mutations into one PUT ---------------- # @@ -400,13 +505,13 @@ async def test_backward_placeholder_resolves_in_coalesced_comment(monkeypatch): _FakeDB(), _FakeRun(status=RunStatus.succeeded.value), rows ) - # The patch applies first (its own idx), seeding results_by_ref; the - # coalesced comment then resolves {{actions.patch.url}} at the group anchor. + # The patch applies first, but the comment that would reference its result + # is suppressed rather than coalesced into the Bugzilla update. assert handler.calls == [ {}, { "bug_id": 5, "changes": {"a": 1}, - "comment": {"body": "see http://x/D1", "is_private": False}, }, ] + assert comment.status == "suppressed" diff --git a/services/hackbot-ui/app/globals.css b/services/hackbot-ui/app/globals.css index 7d869dd4c3..2351ee5061 100644 --- a/services/hackbot-ui/app/globals.css +++ b/services/hackbot-ui/app/globals.css @@ -197,6 +197,10 @@ button.secondary { background: rgba(46, 160, 67, 0.18); color: var(--green); } +.badge.suppressed { + background: rgba(154, 163, 178, 0.18); + color: var(--muted); +} .badge.failed, .badge.timed_out { background: rgba(229, 83, 75, 0.18); diff --git a/services/hackbot-ui/components/RunDetail.tsx b/services/hackbot-ui/components/RunDetail.tsx index 3c846f6407..2f00238e1b 100644 --- a/services/hackbot-ui/components/RunDetail.tsx +++ b/services/hackbot-ui/components/RunDetail.tsx @@ -135,8 +135,9 @@ export function RunDetail({ runId }: { runId: string }) { const findings = run.summary?.findings ?? {}; const hasFindings = Object.keys(findings).length > 0; - // Both pending and failed actions are (re)applied by the apply endpoint — it - // skips only already-applied ones — so one button covers applying and retry. + // Both pending and failed actions are (re)applied by the apply endpoint; + // applied and suppressed actions are terminal, so one button covers applying + // and retry. const pendingActions = actions?.filter((a) => a.status === "pending").length ?? 0; const failedActions = @@ -210,7 +211,8 @@ export function RunDetail({ runId }: { runId: string }) { {applyError &&
{applyError}
}