From 596fd69bcc266ffc9e8e62319e6074a8e44c9f80 Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Sat, 25 Jul 2026 09:59:39 -0400 Subject: [PATCH] Split phabricator.submit_patch into submit_patch and update_patch One tool that both created and updated a revision meant the agent had to get an optional `revision_id` exactly right: omitting it silently created a new revision, and passing a guessed one silently updated a stranger's. Now `phabricator.submit_patch` creates (bug_id + required title + summary) and `phabricator.update_patch` updates (bug_id + required revision_id, with title and summary optional and defaulting to the revision's current values). Each tool only accepts the parameters its own case needs, so the schema itself rules out the wrong call. Apply-side stays one handler (renamed PatchSubmissionHandler) registered for both types, since the Conduit calls are identical apart from the revision id. Places gated on "this run submits a patch" now check the shared PATCH_ACTION_TYPES set: the diff artifact built in publish_changes and the WIP policy injected in hackbot-api. Also corrects the ref-placeholder example in both tool docstrings: the applied result exposes revision_url, not url. --- .../bug-fix/hackbot_agents/bug_fix/config.py | 1 + .../hackbot_agents/bug_fix/prompts/system.md | 7 +- .../actions/handlers/phabricator_handler.py | 7 +- .../actions/handlers/registry.py | 9 +- .../hackbot_runtime/actions/phabricator.py | 150 +++++++++++++----- .../hackbot_runtime/changes.py | 4 +- .../hackbot_runtime/context.py | 6 +- libs/hackbot-runtime/tests/test_context.py | 7 +- .../tests/test_phabricator_actions.py | 74 +++++++-- .../tests/test_phabricator_handler.py | 44 +++-- services/hackbot-api/app/actions_applier.py | 3 +- .../hackbot-api/tests/test_actions_applier.py | 10 +- 12 files changed, 235 insertions(+), 87 deletions(-) diff --git a/agents/bug-fix/hackbot_agents/bug_fix/config.py b/agents/bug-fix/hackbot_agents/bug_fix/config.py index 8509d3ee9a..ead888a276 100644 --- a/agents/bug-fix/hackbot_agents/bug_fix/config.py +++ b/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. 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 bb93876015..00d9ba1014 100644 --- a/agents/bug-fix/hackbot_agents/bug_fix/prompts/system.md +++ b/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: diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py index cf36a69957..eb85490a5b 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py +++ b/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") diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/registry.py b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/registry.py index d9835e9cf3..74b2a304e1 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/registry.py +++ b/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, } diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py b/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py index e467908ed3..34c97c463b 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py @@ -4,19 +4,37 @@ 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})." @@ -24,31 +42,91 @@ def _confirm(recorder: ActionsRecorder, action_type: str) -> str: 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..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..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..url}} in that " + "result once applied, via {{actions..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..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..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__) diff --git a/libs/hackbot-runtime/hackbot_runtime/changes.py b/libs/hackbot-runtime/hackbot_runtime/changes.py index 53ebfe5f72..70f2a7953f 100644 --- a/libs/hackbot-runtime/hackbot_runtime/changes.py +++ b/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 diff --git a/libs/hackbot-runtime/hackbot_runtime/context.py b/libs/hackbot-runtime/hackbot_runtime/context.py index 07e17e2aa4..8efd8d6c1a 100644 --- a/libs/hackbot-runtime/hackbot_runtime/context.py +++ b/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( diff --git a/libs/hackbot-runtime/tests/test_context.py b/libs/hackbot-runtime/tests/test_context.py index 270dbb988e..835c979ca6 100644 --- a/libs/hackbot-runtime/tests/test_context.py +++ b/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() diff --git a/libs/hackbot-runtime/tests/test_phabricator_actions.py b/libs/hackbot-runtime/tests/test_phabricator_actions.py index 43c59c756a..539d406e9a 100644 --- a/libs/hackbot-runtime/tests/test_phabricator_actions.py +++ b/libs/hackbot-runtime/tests/test_phabricator_actions.py @@ -1,41 +1,83 @@ -"""Tests for the phabricator.submit_patch recording tool.""" +"""Tests for the phabricator.submit_patch / update_patch recording tools.""" import pytest -from agent_tools.registry import ToolError from hackbot_runtime.actions import ActionsRecorder, phabricator -async def test_create_requires_title(): - rec = ActionsRecorder() - with pytest.raises(ToolError): - await phabricator.submit_patch(rec, bug_id=1, reasoning="r") - - -async def test_create_records_without_revision_id(): +async def test_submit_records_create_params_only(): rec = ActionsRecorder() await phabricator.submit_patch( - rec, bug_id=1, reasoning="r", title="Fix the thing", summary="Details" + rec, bug_id=1, title="Fix the thing", reasoning="r", summary="Details" ) action = rec.actions[0] assert action["type"] == "phabricator.submit_patch" assert action["params"] == { "bug_id": 1, - "revision_id": None, "title": "Fix the thing", "summary": "Details", } assert "ref" not in action -async def test_update_does_not_require_title(): +async def test_submit_requires_title(): rec = ActionsRecorder() - await phabricator.submit_patch(rec, bug_id=1, reasoning="r", revision_id=12345) - assert rec.actions[0]["params"]["revision_id"] == 12345 + with pytest.raises(TypeError): + await phabricator.submit_patch(rec, bug_id=1, reasoning="r") -async def test_ref_is_recorded(): +async def test_submit_ref_is_recorded(): rec = ActionsRecorder() await phabricator.submit_patch( - rec, bug_id=1, reasoning="r", title="Fix", ref="patch" + rec, bug_id=1, title="Fix", reasoning="r", ref="patch" ) assert rec.actions[0]["ref"] == "patch" + + +async def test_update_records_revision_id_and_keeps_fields_optional(): + rec = ActionsRecorder() + await phabricator.update_patch(rec, bug_id=1, revision_id=12345, reasoning="r") + action = rec.actions[0] + assert action["type"] == "phabricator.update_patch" + assert action["params"] == { + "bug_id": 1, + "revision_id": 12345, + "title": None, + "summary": None, + } + + +async def test_update_requires_revision_id(): + rec = ActionsRecorder() + with pytest.raises(TypeError): + await phabricator.update_patch(rec, bug_id=1, reasoning="r") + + +async def test_update_can_change_title_and_summary(): + rec = ActionsRecorder() + await phabricator.update_patch( + rec, + bug_id=1, + revision_id=7, + reasoning="r", + title="New title", + summary="New summary", + ref="patch", + ) + action = rec.actions[0] + assert action["params"]["title"] == "New title" + assert action["params"]["summary"] == "New summary" + assert action["ref"] == "patch" + + +def test_agent_facing_schemas_are_case_specific(): + """The point of the split: create takes no revision id, update demands one.""" + submit = next(t for t in phabricator.TOOLS if t.name == "submit_patch") + update = next(t for t in phabricator.TOOLS if t.name == "update_patch") + + assert "revision_id" not in submit.input_schema["properties"] + assert set(submit.input_schema["required"]) == {"bug_id", "title", "reasoning"} + assert set(update.input_schema["required"]) == { + "bug_id", + "revision_id", + "reasoning", + } diff --git a/libs/hackbot-runtime/tests/test_phabricator_handler.py b/libs/hackbot-runtime/tests/test_phabricator_handler.py index dfc679fc88..4d18aa6e75 100644 --- a/libs/hackbot-runtime/tests/test_phabricator_handler.py +++ b/libs/hackbot-runtime/tests/test_phabricator_handler.py @@ -10,6 +10,8 @@ import json from hackbot_runtime.actions.handlers import ApplyContext, phabricator_handler +from hackbot_runtime.actions.handlers.registry import get_handler +from hackbot_runtime.actions.phabricator import PATCH_ACTION_TYPES _DIFF_PAYLOAD = { "changes": [{"currentPath": "file.txt"}], @@ -42,6 +44,14 @@ def fake(method, **payload): return fake, calls +def test_both_patch_action_types_are_handled(): + handlers = {t: get_handler(t) for t in PATCH_ACTION_TYPES} + assert all( + isinstance(h, phabricator_handler.PatchSubmissionHandler) + for h in handlers.values() + ) + + def test_revision_title_strips_and_reprefixes(): rt = phabricator_handler._revision_title assert rt("Fix bug", wip=True) == "WIP: Fix bug" @@ -67,8 +77,8 @@ async def test_submit_patch_create_wip_by_default(monkeypatch): monkeypatch.setattr(phabricator_handler, "_conduit_request", fake) monkeypatch.setattr(phabricator_handler, "_repository_phid", lambda: "PHID-REPO-1") - result = await phabricator_handler.SubmitPatchHandler().apply( - {"bug_id": 1, "revision_id": None, "title": "Fix", "summary": "s"}, + result = await phabricator_handler.PatchSubmissionHandler().apply( + {"bug_id": 1, "title": "Fix", "summary": "s"}, _ctx(), ) @@ -104,7 +114,7 @@ async def test_submit_patch_create_non_wip(monkeypatch): monkeypatch.setattr(phabricator_handler, "_conduit_request", fake) monkeypatch.setattr(phabricator_handler, "_repository_phid", lambda: "PHID-REPO-1") - await phabricator_handler.SubmitPatchHandler().apply( + await phabricator_handler.PatchSubmissionHandler().apply( {"bug_id": 1, "title": "Fix", "summary": "s", "wip": False}, _ctx(), ) @@ -140,7 +150,7 @@ async def test_submit_patch_sets_local_commits_property(monkeypatch): "parents": ["base1"], "tree": "tree1", } - result = await phabricator_handler.SubmitPatchHandler().apply( + result = await phabricator_handler.PatchSubmissionHandler().apply( {"bug_id": 5, "title": "Fix the thing", "summary": "does it"}, _ctx(local_commits={"node1": dict(git_fields)}), ) @@ -177,7 +187,7 @@ async def test_submit_patch_sets_local_commits_property(monkeypatch): assert "local_commits" not in creatediff_call[1] -async def test_submit_patch_local_commits_fetches_title_on_update(monkeypatch): +async def test_update_patch_local_commits_fetches_title(monkeypatch): fake, calls = _fake_conduit( { "differential.creatediff": {"phid": "PHID-DIFF-1", "diffid": 3}, @@ -191,7 +201,7 @@ async def test_submit_patch_local_commits_fetches_title_on_update(monkeypatch): monkeypatch.setattr(phabricator_handler, "_conduit_request", fake) monkeypatch.setattr(phabricator_handler, "_repository_phid", lambda: "PHID-REPO-1") - result = await phabricator_handler.SubmitPatchHandler().apply( + result = await phabricator_handler.PatchSubmissionHandler().apply( {"bug_id": 9, "revision_id": 42}, _ctx(local_commits={"n": {"author": "A"}}), ) @@ -209,7 +219,7 @@ async def test_submit_patch_local_commits_fetches_title_on_update(monkeypatch): ) -async def test_submit_patch_update_sets_object_identifier(monkeypatch): +async def test_update_patch_sets_object_identifier(monkeypatch): fake, calls = _fake_conduit( { "differential.creatediff": {"phid": "PHID-DIFF-2", "diffid": 2}, @@ -224,7 +234,7 @@ async def test_submit_patch_update_sets_object_identifier(monkeypatch): monkeypatch.setattr(phabricator_handler, "_conduit_request", fake) monkeypatch.setattr(phabricator_handler, "_repository_phid", lambda: "PHID-REPO-1") - result = await phabricator_handler.SubmitPatchHandler().apply( + result = await phabricator_handler.PatchSubmissionHandler().apply( {"bug_id": 7, "revision_id": 12345}, _ctx() ) @@ -234,7 +244,7 @@ async def test_submit_patch_update_sets_object_identifier(monkeypatch): assert edit_call[1]["objectIdentifier"] == 12345 -async def test_submit_patch_wip_update_already_changes_planned_uses_second_edit( +async def test_update_patch_wip_already_changes_planned_uses_second_edit( monkeypatch, ): fake, calls = _fake_conduit( @@ -251,7 +261,7 @@ async def test_submit_patch_wip_update_already_changes_planned_uses_second_edit( monkeypatch.setattr(phabricator_handler, "_conduit_request", fake) monkeypatch.setattr(phabricator_handler, "_repository_phid", lambda: "PHID-REPO-1") - result = await phabricator_handler.SubmitPatchHandler().apply( + result = await phabricator_handler.PatchSubmissionHandler().apply( {"bug_id": 3, "revision_id": 50}, _ctx() ) assert result.status == "applied" @@ -265,7 +275,7 @@ async def test_submit_patch_wip_update_already_changes_planned_uses_second_edit( assert edits[1][1]["transactions"] == [{"type": "plan-changes", "value": True}] -async def test_submit_patch_non_wip_update_requests_review(monkeypatch): +async def test_update_patch_non_wip_requests_review(monkeypatch): fake, calls = _fake_conduit( { "differential.creatediff": {"phid": "PHID-DIFF-6", "diffid": 6}, @@ -280,7 +290,7 @@ async def test_submit_patch_non_wip_update_requests_review(monkeypatch): monkeypatch.setattr(phabricator_handler, "_conduit_request", fake) monkeypatch.setattr(phabricator_handler, "_repository_phid", lambda: "PHID-REPO-1") - await phabricator_handler.SubmitPatchHandler().apply( + await phabricator_handler.PatchSubmissionHandler().apply( {"bug_id": 4, "revision_id": 60, "wip": False}, _ctx() ) @@ -291,7 +301,7 @@ async def test_submit_patch_non_wip_update_requests_review(monkeypatch): assert "plan-changes" not in types -async def test_submit_patch_falls_back_to_given_revision_id_when_edit_omits_object( +async def test_update_patch_falls_back_to_given_revision_id_when_edit_omits_object( monkeypatch, ): fake, _ = _fake_conduit( @@ -304,7 +314,7 @@ async def test_submit_patch_falls_back_to_given_revision_id_when_edit_omits_obje monkeypatch.setattr(phabricator_handler, "_conduit_request", fake) monkeypatch.setattr(phabricator_handler, "_repository_phid", lambda: "PHID-REPO-1") - result = await phabricator_handler.SubmitPatchHandler().apply( + result = await phabricator_handler.PatchSubmissionHandler().apply( {"bug_id": 7, "revision_id": 999}, _ctx() ) assert result.status == "applied" @@ -316,7 +326,9 @@ async def download(key): raise KeyError(key) ctx = ApplyContext(run_id="run-1", download_artifact=download) - result = await phabricator_handler.SubmitPatchHandler().apply({"bug_id": 1}, ctx) + result = await phabricator_handler.PatchSubmissionHandler().apply( + {"bug_id": 1}, ctx + ) assert result.status == "failed" @@ -327,7 +339,7 @@ def fake(method, **payload): monkeypatch.setattr(phabricator_handler, "_conduit_request", fake) monkeypatch.setattr(phabricator_handler, "_repository_phid", lambda: "PHID-REPO-1") - result = await phabricator_handler.SubmitPatchHandler().apply( + result = await phabricator_handler.PatchSubmissionHandler().apply( {"bug_id": 1, "title": "x"}, _ctx() ) assert result.status == "failed" diff --git a/services/hackbot-api/app/actions_applier.py b/services/hackbot-api/app/actions_applier.py index 4a75c79c2a..bbccc3dc90 100644 --- a/services/hackbot-api/app/actions_applier.py +++ b/services/hackbot-api/app/actions_applier.py @@ -24,6 +24,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 @@ -113,7 +114,7 @@ async def _dispatch( f"No handler registered for action type '{action_type}'" ) - if action_type == "phabricator.submit_patch": + if action_type in PATCH_ACTION_TYPES: # WIP is a hackbot-api policy, not an agent choice; inject it here. params = {**params, "wip": SUBMIT_PATCHES_AS_WIP} diff --git a/services/hackbot-api/tests/test_actions_applier.py b/services/hackbot-api/tests/test_actions_applier.py index 4c668346f2..0243cb9e12 100644 --- a/services/hackbot-api/tests/test_actions_applier.py +++ b/services/hackbot-api/tests/test_actions_applier.py @@ -9,6 +9,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, @@ -410,9 +411,12 @@ async def test_backward_placeholder_resolves_in_coalesced_comment(monkeypatch): ] -async def test_phabricator_submit_patch_gets_wip_injected(monkeypatch): +@pytest.mark.parametrize( + "action_type", ["phabricator.submit_patch", "phabricator.update_patch"] +) +async def test_phabricator_patch_actions_get_wip_injected(monkeypatch, action_type): # WIP is a hackbot-api policy injected at dispatch, not part of the recorded - # action — the agent never sets it. + # action — the agent never sets it. Both patch actions get it. handler = _RecordingHandler( SimpleNamespace(status="applied", result={}, error=None) ) @@ -421,7 +425,7 @@ async def test_phabricator_submit_patch_gets_wip_injected(monkeypatch): row = _row( 0, "pending", - action_type="phabricator.submit_patch", + action_type=action_type, params={"bug_id": 1, "title": "x"}, ) await actions_applier._apply_pending_rows(