Split phabricator.submit_patch into submit_patch and update_patch

agents/bug-fix/hackbot_agents/bug_fix/config.py

@@ -18,6 +18,7 @@
     "bugzilla.add_attachment",
     "bugzilla.create_bug",
     "phabricator.submit_patch",
+    "phabricator.update_patch",
 ]
 
 # Firefox build/test tools.

agents/bug-fix/hackbot_agents/bug_fix/prompts/system.md

@@ -42,7 +42,9 @@ Follow these rules:
   to avoid spot fixes where a more general fix on a higher level or earlier is more appropriate.
 - Avoid adding too much defense in depth, especially in performance critical paths. While defense in depth is
   generally not a bad thing, unnecessary/redundant checks cost valuable performance.
-- When you have a fix you are confident in, submit it with the `phabricator_submit_patch` action.
+- When you have a fix you are confident in, submit it with the `phabricator_submit_patch` action. Only if the
+  fix belongs on a Phabricator revision that already exists (and you know its id), use
+  `phabricator_update_patch` with that id instead.
 - If a bug has been closed or a developer has already added a fix patch (even if you cannot download it),
   then don't create a fix and move on.
 - If you detect that a bug has already been fixed by another bug, don't create another fix patch
@@ -65,7 +67,8 @@ When you spawn an investigator via the Task tool, write a complete, self-contain
 
 # Recording actions
 
-The `actions` MCP tools (`bugzilla_update_bug`, `bugzilla_add_comment`, `phabricator_submit_patch`) do **not** mutate Bugzilla or Phabricator directly. 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.
+The `actions` MCP tools (`bugzilla_update_bug`, `bugzilla_add_comment`, `phabricator_submit_patch`,
+`phabricator_update_patch`) do **not** mutate Bugzilla or Phabricator directly. 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.
 
 Before calling any action tool, state in your response:
 

libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py

@@ -1,5 +1,10 @@
 """Apply-side Phabricator action: submits an already-built diff payload.
 
+One handler serves both ``phabricator.submit_patch`` and
+``phabricator.update_patch``: the Conduit calls are the same either way, and a
+``revision_id`` in the params (only ``update_patch`` records one) is what turns
+the revision edit into an update of that revision.
+
 Pairs with the recording side in ``actions/phabricator.py`` and the payload
 built agent-side in ``hackbot_runtime.changes.build_phabricator_diff`` (while
 the agent still has its own checkout — nothing here ever touches git, a
@@ -169,7 +174,7 @@ def _set_local_commits(
     )
 
 
-class SubmitPatchHandler:
+class PatchSubmissionHandler:
     async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult:
         bug_id = params["bug_id"]
         revision_id = params.get("revision_id")

libs/hackbot-runtime/hackbot_runtime/actions/handlers/registry.py

@@ -5,7 +5,11 @@
     CreateBugHandler,
     UpdateBugHandler,
 )
-from hackbot_runtime.actions.handlers.phabricator_handler import SubmitPatchHandler
+from hackbot_runtime.actions.handlers.phabricator_handler import PatchSubmissionHandler
+
+# Creating and updating a revision are separate agent-facing actions but one
+# apply-side handler (see phabricator_handler's module docstring).
+_patch_submission = PatchSubmissionHandler()
 
 # Maps a recorded action's dotted `type` to the handler that applies it.
 # Adding a new action type later is a one-line addition here — the dispatch
@@ -15,7 +19,8 @@
     "bugzilla.add_comment": AddCommentHandler(),
     "bugzilla.add_attachment": AddAttachmentHandler(),
     "bugzilla.create_bug": CreateBugHandler(),
-    "phabricator.submit_patch": SubmitPatchHandler(),
+    "phabricator.submit_patch": _patch_submission,
+    "phabricator.update_patch": _patch_submission,
 }
 
 

libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py

@@ -4,51 +4,129 @@
 change (nothing is submitted to Phabricator here) and returns a short
 confirmation string. See ``actions/handlers/phabricator_handler.py`` for the
 apply side.
+
+Creating a revision and updating one are deliberately two tools: each takes
+only the parameters its own case needs, so the agent cannot create a revision
+when it meant to update one (or invent a revision id) by getting one optional
+argument wrong.
 """
 
 from __future__ import annotations
 
 from typing import Annotated, Any
 
-from agent_tools.registry import ToolError, tool, tools_in
+from agent_tools.registry import tool, tools_in
 from pydantic import Field
 
 from hackbot_runtime.actions.recorder import ActionsRecorder
 
+# Both patch actions submit the working directory's changes as a diff, so
+# anything gated on "this run submits a patch" (the diff artifact built in
+# ``context.publish_changes``, the WIP policy applied in hackbot-api) has to
+# cover both types.
+PATCH_ACTION_TYPES = frozenset({"phabricator.submit_patch", "phabricator.update_patch"})
+
 
-def _confirm(recorder: ActionsRecorder, action_type: str) -> str:
+def _record(
+    recorder: ActionsRecorder,
+    action_type: str,
+    params: dict[str, Any],
+    reasoning: str,
+    ref: str | None,
+) -> str:
+    recorder.record(action_type, params, reasoning=reasoning, ref=ref)
     return f"Recorded {action_type} (#{len(recorder.actions) - 1})."
 
 
 @tool
 async def submit_patch(
     recorder: ActionsRecorder,
     bug_id: Annotated[int, Field(description="Bug this patch fixes.")],
+    title: Annotated[
+        str,
+        Field(
+            description=(
+                "Title for the new revision: a single line describing the fix, "
+                "as you would write a commit message subject."
+            )
+        ),
+    ],
     reasoning: Annotated[
         str, Field(description="Why you are submitting this patch (for audit log).")
     ],
-    revision_id: Annotated[
-        int | None,
+    summary: Annotated[
+        str | None,
+        Field(default=None, description="Revision summary/description."),
+    ] = None,
+    ref: Annotated[
+        str | None,
         Field(
             default=None,
             description=(
-                "An existing Phabricator revision to attach a new diff to. "
-                "Omit to create a brand-new revision instead — never inferred "
-                "automatically, so pass this explicitly whenever you intend "
-                "to update rather than create."
+                "Optional label for this action so a later action (e.g. a "
+                "bugzilla.add_comment in the same run) can reference its "
+                "result once applied, via {{actions.<ref>.revision_url}} in that "
+                "action's text."
             ),
         ),
     ] = None,
+) -> str:
+    """Submit your fix for review as a new Phabricator revision.
+
+    This is how you deliver a code fix. Do not attach the patch to a bug: a
+    Phabricator revision is the correct destination for a fix, not a bug
+    attachment. If the fix belongs on a revision that already exists, use
+    `phabricator_update_patch` instead.
+
+    You do not supply a patch file. Your final code changes in the working
+    directory are submitted as the revision's diff, so make and verify all your
+    edits first, then call this once you are done. Calling it records the
+    submission as a proposed action for review; it is not sent to Phabricator
+    during the run.
+
+    Set `ref` if you want to reference the new revision's URL from another
+    action in the same run, written as `{{actions.<ref>.revision_url}}` (for
+    example, inside a bug comment).
+    """
+    return _record(
+        recorder,
+        "phabricator.submit_patch",
+        {"bug_id": bug_id, "title": title, "summary": summary},
+        reasoning,
+        ref,
+    )
+
+
+@tool
+async def update_patch(
+    recorder: ActionsRecorder,
+    bug_id: Annotated[int, Field(description="Bug this patch fixes.")],
+    revision_id: Annotated[
+        int,
+        Field(
+            description=(
+                "The existing Phabricator revision to attach the new diff to, as "
+                "a number (12345 for D12345). Only pass a revision id you were "
+                "given or read from the bug: never guess one."
+            )
+        ),
+    ],
+    reasoning: Annotated[
+        str, Field(description="Why you are submitting this patch (for audit log).")
+    ],
     title: Annotated[
         str | None,
         Field(
             default=None,
-            description="Revision title. Required when creating a new revision.",
+            description="New revision title. Omit to keep the current title.",
         ),
     ] = None,
     summary: Annotated[
         str | None,
-        Field(default=None, description="Revision summary/description."),
+        Field(
+            default=None,
+            description="New revision summary. Omit to keep the current summary.",
+        ),
     ] = None,
     ref: Annotated[
         str | None,
@@ -57,46 +135,40 @@ async def submit_patch(
             description=(
                 "Optional label for this action so a later action (e.g. a "
                 "bugzilla.add_comment in the same run) can reference its "
-                "result once applied, via {{actions.<ref>.url}} in that "
+                "result once applied, via {{actions.<ref>.revision_url}} in that "
                 "action's text."
             ),
         ),
     ] = None,
 ) -> str:
-    """Submit your fix for review as a Phabricator revision.
+    """Submit your fix as a new diff on an existing Phabricator revision.
 
-    This is how you deliver a code fix. Do not attach the patch to a bug: a
-    Phabricator revision is the correct destination for a fix, not a bug
-    attachment.
+    Use this only when a revision for this work already exists (for example, you
+    are addressing review comments on it) and you know its id. To deliver a fix
+    that has no revision yet, use `phabricator_submit_patch` instead.
 
     You do not supply a patch file. Your final code changes in the working
-    directory are submitted as the revision's diff, so make and verify all your
-    edits first, then call this once you are done. Calling it records the
-    submission as a proposed action for review; it is not sent to Phabricator
-    during the run.
+    directory are submitted as the new diff, so make and verify all your edits
+    first, then call this once you are done. Calling it records the submission as
+    a proposed action for review; it is not sent to Phabricator during the run.
 
-    To create a new revision, pass a `title` (and ideally a `summary`). To add a
-    new diff to an existing revision instead, pass that revision's `revision_id`.
-    Set `ref` if you want to reference the new revision's URL from another action
-    in the same run, written as `{{actions.<ref>.url}}` (for example, inside a
-    bug comment).
+    The revision keeps its current title and summary unless you pass new ones.
+    Set `ref` if you want to reference the revision's URL from another action in
+    the same run, written as `{{actions.<ref>.revision_url}}` (for example,
+    inside a bug comment).
     """
-    if revision_id is None and not title:
-        raise ToolError("title is required when creating a new revision")
-
-    params: dict[str, Any] = {
-        "bug_id": bug_id,
-        "revision_id": revision_id,
-        "title": title,
-        "summary": summary,
-    }
-    recorder.record(
-        "phabricator.submit_patch",
-        params,
-        reasoning=reasoning,
-        ref=ref,
+    return _record(
+        recorder,
+        "phabricator.update_patch",
+        {
+            "bug_id": bug_id,
+            "revision_id": revision_id,
+            "title": title,
+            "summary": summary,
+        },
+        reasoning,
+        ref,
     )
-    return _confirm(recorder, "phabricator.submit_patch")
 
 
 TOOLS = tools_in(__name__)

libs/hackbot-runtime/hackbot_runtime/changes.py

@@ -222,8 +222,8 @@ def build_phabricator_diff(repo: Path, base: str, repo_url: str) -> dict | None:
     for its own work — no separate clone or checkout happens here. Returns
     ``None`` if building the diff fails for any reason (e.g. the checkout
     lacks an ``.arcconfig``, or nothing actually changed) — this is
-    best-effort, gated by the caller on whether a `phabricator.submit_patch`
-    action was even recorded, so a failure here shouldn't break an otherwise
+    best-effort, gated by the caller on whether a Phabricator patch action
+    was even recorded, so a failure here shouldn't break an otherwise
     successful run.
 
     ``repositoryPHID`` is deliberately not included here — it's resolved by

libs/hackbot-runtime/hackbot_runtime/context.py

@@ -25,6 +25,7 @@
 from pydantic_settings import BaseSettings, SettingsConfigDict
 
 from hackbot_runtime import artifacts, changes
+from hackbot_runtime.actions.phabricator import PATCH_ACTION_TYPES
 from hackbot_runtime.actions.recorder import ActionsRecorder
 from hackbot_runtime.config import HackbotConfig, load_config
 from hackbot_runtime.providers import AnthropicAuth
@@ -206,7 +207,7 @@ def publish_changes(
         Returns the patch key, or ``None`` when the agent never prepared a source
         checkout or made no changes at all.
 
-        If the agent recorded a ``phabricator.submit_patch`` action, also builds
+        If the agent recorded a Phabricator patch action, also builds
         and publishes the Phabricator submission payload here — while the
         checkout the agent already has is still around — so the downstream
         apply step never needs its own checkout (see
@@ -229,8 +230,7 @@ def publish_changes(
         self.publish_json(meta_key, change_set.metadata)
 
         wants_phabricator = any(
-            action["type"] == "phabricator.submit_patch"
-            for action in self.actions.actions
+            action["type"] in PATCH_ACTION_TYPES for action in self.actions.actions
         )
         if wants_phabricator:
             diff_payload = changes.build_phabricator_diff(

libs/hackbot-runtime/tests/test_context.py

@@ -130,8 +130,11 @@ def _hb_with_source(tmp_path, monkeypatch):
     return hb
 
 
+@pytest.mark.parametrize(
+    "action_type", ["phabricator.submit_patch", "phabricator.update_patch"]
+)
 def test_publish_changes_builds_phabricator_diff_when_action_recorded(
-    tmp_path, monkeypatch
+    tmp_path, monkeypatch, action_type
 ):
     hb = _hb_with_source(tmp_path, monkeypatch)
     monkeypatch.setattr(
@@ -141,7 +144,7 @@ def test_publish_changes_builds_phabricator_diff_when_action_recorded(
             "local_commits": {"node": {"author": "A"}},
         },
     )
-    hb.actions.record("phabricator.submit_patch", {"bug_id": 1}, reasoning="r")
+    hb.actions.record(action_type, {"bug_id": 1}, reasoning="r")
 
     hb.publish_changes()