Suppress comments on patch runs

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.

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)

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.

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"

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:

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 ---------------- #
 
 
@@ -250,7 +355,7 @@ async def test_coalesces_update_and_comment_into_one_put(monkeypatch):
         {
             "bug_id": 5,
             "changes": {"status": "RESOLVED"},
-            "comment": {"body": "done", "is_private": False},
+            "comment": {"body": "done", "is_private": False, "is_markdown": True},
         },
     ]
     assert update.status == "applied" and comment.status == "applied"
@@ -292,7 +397,7 @@ async def test_extra_comments_applied_separately(monkeypatch):
         {
             "bug_id": 5,
             "changes": {"status": "RESOLVED"},
-            "comment": {"body": "near", "is_private": False},
+            "comment": {"body": "near", "is_private": False, "is_markdown": True},
         },
         {"bug_id": 5, "text": "far"},
     ]
@@ -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"

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);

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 && <div className="error-banner">{applyError}</div>}
           <ul className="action-list">
             {actions.map((a) => {
-              const preview = commentPreview(a);
+              const preview =
+                a.status === "suppressed" ? null : commentPreview(a);
               return (
                 <li key={a.idx}>
                   <div className="action-row">

services/hackbot-ui/lib/types.ts

@@ -36,7 +36,7 @@ export interface RunSummary {
   findings: Record<string, unknown>;
 }
 
-export type RunActionStatus = "pending" | "applied" | "failed";
+export type RunActionStatus = "pending" | "applied" | "failed" | "suppressed";
 
 // Mirror of RunActionDoc (services/hackbot-api/app/schemas.py): a recorded
 // agent action and its apply state.