diff --git a/agents/bug-fix/hackbot_agents/bug_fix/agent.py b/agents/bug-fix/hackbot_agents/bug_fix/agent.py index 1bdf8bf3c4..8f8a9f746d 100644 --- a/agents/bug-fix/hackbot_agents/bug_fix/agent.py +++ b/agents/bug-fix/hackbot_agents/bug_fix/agent.py @@ -28,9 +28,10 @@ from .config import ( BUGZILLA_READ_TOOLS, - ENABLED_ACTION_TYPES, FIREFOX_TOOLS, + PHABRICATOR_FOLLOW_UP_ACTIONS, SOURCE_WRITE_TOOLS, + TRIAGE_AND_FIX_ACTIONS, ) HERE = Path(__file__).resolve().parent @@ -111,13 +112,24 @@ async def run_bug_fix( # hackbot.toml; here we only wrap its tools as an MCP server. firefox_server = build_sdk_server("firefox", fx_ctx, firefox.TOOLS) + if revision_id: + action_types = PHABRICATOR_FOLLOW_UP_ACTIONS + user_prompt = render_prompt( + "follow-up.md", revision_id=revision_id, bug_id=bug, comment=comment + ) + else: + action_types = TRIAGE_AND_FIX_ACTIONS + user_prompt = render_prompt( + "triage-and-fix.md", bug_id=bug, rules_path=str(rules_dir.resolve()) + ) + # Action-recording MCP server (in-process). Standalone/script runs pass # actions_recorder=None and get a local recorder that copies attachments # under ./artifacts (no uploader). actions_recorder, actions_server = actions_server_for( - actions_recorder, types=ENABLED_ACTION_TYPES + actions_recorder, types=action_types ) - enabled_action_tools = actions_to_tool_names(ENABLED_ACTION_TYPES) + enabled_action_tools = actions_to_tool_names(action_types) system_prompt = render_prompt("system.md", rules_dir=str(rules_dir.resolve())) @@ -149,15 +161,6 @@ async def run_bug_fix( setting_sources=[], ) - if revision_id and comment: - user_prompt = render_prompt( - "follow-up.md", revision_id=revision_id, bug_id=bug, comment=comment - ) - else: - user_prompt = render_prompt( - "triage-and-fix.md", bug_id=bug, rules_path=str(rules_dir.resolve()) - ) - result_msg: ResultMessage | None = None with Reporter(verbose=verbose, log_path=log) as reporter: reporter.header(f"bug {bug}") diff --git a/agents/bug-fix/hackbot_agents/bug_fix/config.py b/agents/bug-fix/hackbot_agents/bug_fix/config.py index ba8f5ac8a0..67cad90153 100644 --- a/agents/bug-fix/hackbot_agents/bug_fix/config.py +++ b/agents/bug-fix/hackbot_agents/bug_fix/config.py @@ -10,14 +10,22 @@ "mcp__bugzilla__download_attachment", ] - -# Recordable action types the agent may take, by dotted id. -ENABLED_ACTION_TYPES = [ +# Action types that the agent may record during triage/fix runs. +TRIAGE_AND_FIX_ACTIONS = [ "bugzilla.update_bug", "bugzilla.add_comment", "bugzilla.add_attachment", "bugzilla.create_bug", "phabricator.submit_patch", +] + +# Action types that the agent may record during follow-up runs on a revision. +PHABRICATOR_FOLLOW_UP_ACTIONS = [ + "bugzilla.update_bug", + "bugzilla.add_comment", + "bugzilla.add_attachment", + "bugzilla.create_bug", + "phabricator.update_patch", "phabricator.add_comment", ] 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 5eaf38aa09..374eff8b35 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 @@ -8,7 +8,7 @@ Respond only to the comments quoted below. Ignore any earlier mentions of you el First investigate to understand what they are asking for, then address each one 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_submit_patch with revision_id={revision_id} so the existing revision D{revision_id} is updated. Do not create a new revision. +- 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 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. 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 da12e42a0b..1ee5be7ac7 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 whichever patch action this run gives you: + `phabricator_submit_patch` creates a new revision, `phabricator_update_patch` adds a new diff to the + revision you were called on. Only the one that fits this run is available, so use the one you have. - 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,7 @@ 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`, `phabricator_add_comment`) do **not** mutate Bugzilla or Phabricator directly. Use `phabricator_add_comment` 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. 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`, 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. Before calling any action tool, state in your response: diff --git a/agents/bug-fix/tests/test_action_types.py b/agents/bug-fix/tests/test_action_types.py new file mode 100644 index 0000000000..3b66716cec --- /dev/null +++ b/agents/bug-fix/tests/test_action_types.py @@ -0,0 +1,33 @@ +"""Tests for the per-flow enabled action lists (see bug_fix/config.py).""" + +from hackbot_agents.bug_fix.config import ( + PHABRICATOR_FOLLOW_UP_ACTIONS, + TRIAGE_AND_FIX_ACTIONS, +) +from hackbot_runtime.actions.phabricator import PATCH_ACTION_TYPES + + +def test_triage_flow_can_only_create_a_revision(): + assert "phabricator.submit_patch" in TRIAGE_AND_FIX_ACTIONS + assert "phabricator.update_patch" not in TRIAGE_AND_FIX_ACTIONS + # Nothing to comment on yet: there is no revision in this flow. + assert "phabricator.add_comment" not in TRIAGE_AND_FIX_ACTIONS + + +def test_follow_up_flow_can_only_update_the_revision(): + assert "phabricator.update_patch" in PHABRICATOR_FOLLOW_UP_ACTIONS + assert "phabricator.submit_patch" not in PHABRICATOR_FOLLOW_UP_ACTIONS + assert "phabricator.add_comment" in PHABRICATOR_FOLLOW_UP_ACTIONS + + +def test_flows_never_offer_both_patch_actions(): + for types in (TRIAGE_AND_FIX_ACTIONS, PHABRICATOR_FOLLOW_UP_ACTIONS): + assert len(PATCH_ACTION_TYPES.intersection(types)) == 1 + + +def test_bugzilla_actions_are_available_in_both_flows(): + bugzilla_types = {t for t in TRIAGE_AND_FIX_ACTIONS if t.startswith("bugzilla.")} + assert bugzilla_types + assert bugzilla_types == { + t for t in PHABRICATOR_FOLLOW_UP_ACTIONS if t.startswith("bugzilla.") + } 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 04cf2aa14c..6f99334e6f 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py @@ -1,4 +1,10 @@ -"""Apply-side Phabricator action: submits an already-built diff payload. +"""Apply-side Phabricator actions: submit an already-built diff payload. + +``SubmitPatchHandler`` creates a revision and ``UpdatePatchHandler`` adds a diff +to an existing one — one handler per recorded action type, so neither carries +the other's conditionals. What they share (loading the diff artifact, the +``creatediff`` call, the transactions common to both edits) lives in the +module-level helpers above them. Pairs with the recording side in ``actions/phabricator.py`` and the payload built agent-side in ``hackbot_runtime.changes.build_phabricator_diff`` (while @@ -83,14 +89,14 @@ async def _repository_phid() -> str: # Mirrors moz-phab's WIP_RE / revision_title (mozphab.commits): strip any -# existing WIP prefix, then prepend "WIP: " for a work-in-progress revision. +# existing WIP prefix, then prepend "WIP: ". Everything hackbot creates is a +# work-in-progress draft; a human promotes it out of WIP when they take it over. _WIP_PREFIX_RE = re.compile(r"^(?:WIP[: ]|WIP$)", re.IGNORECASE) -def _revision_title(title: str, wip: bool) -> str: +def _revision_title(title: str) -> str: title = _WIP_PREFIX_RE.sub("", title) or "WIP" - title = title.strip() - return f"WIP: {title}" if wip else title + return f"WIP: {title.strip()}" async def _revision_fields(revision_id: int) -> dict: @@ -168,9 +174,15 @@ async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult class SubmitPatchHandler: + """Applies ``phabricator.submit_patch``: the diff becomes a new revision. + + Nothing exists on the Phabricator side yet, so the agent's title, summary, + and bug id are what the revision gets, created in a single edit. + """ + async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult: bug_id = params["bug_id"] - revision_id = params.get("revision_id") + summary = params.get("summary") try: raw = await ctx.download_artifact(_DIFF_ARTIFACT_KEY) @@ -183,101 +195,99 @@ async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult f"No Phabricator submission artifact for this run: {exc}" ) - # The creatediff payload plus the git side of the local:commits property - # (completed and stored after the revision edit). Tolerate a bare diff - # payload without the wrapper too. - diff_payload = submission.get("diff", submission) - local_commits = submission.get("local_commits") - - wip = params.get("wip", True) - try: diff_result = await _conduit_request( "differential.creatediff", repositoryPHID=await _repository_phid(), - **diff_payload, + **submission["diff"], ) - diff_phid = diff_result["phid"] - - # Resolve title/summary (and, for updates, the current status) once; - # reused for the transactions and the local:commits property. - raw_title = params.get("title") - raw_summary = params.get("summary") - existing_status = None - if revision_id: - fields = await _revision_fields(revision_id) - existing_status = (fields.get("status") or {}).get("value") - if not raw_title: - raw_title = fields.get("title") - if raw_summary is None: - raw_summary = fields.get("summary") - title = _revision_title(raw_title or f"Bug {bug_id}", wip) # Reviewers are never assigned by hackbot: a WIP draft gets them at - # promotion time, and the agent doesn't choose them. + # promotion time, and the agent doesn't choose them. A new revision + # has no status yet, so plan-changes rides this same edit. + title = _revision_title(params.get("title") or f"Bug {bug_id}") transactions: list[dict[str, Any]] = [ - {"type": "update", "value": diff_phid}, + {"type": "update", "value": diff_result["phid"]}, {"type": "title", "value": title}, + {"type": "bugzilla.bug-id", "value": str(bug_id)}, + {"type": "plan-changes", "value": True}, ] - if raw_summary: - transactions.append({"type": "summary", "value": raw_summary}) - transactions.append({"type": "bugzilla.bug-id", "value": str(bug_id)}) - - # Mark WIP via a `plan-changes` transaction, mirroring moz-phab. If - # the revision is already `changes-planned`, Phabricator errors on a - # no-op status change, so send it in a separate follow-up edit. - post_transactions: list[dict[str, Any]] = [] - if wip: - plan_changes = {"type": "plan-changes", "value": True} - if existing_status == "changes-planned": - post_transactions.append(plan_changes) - else: - transactions.append(plan_changes) - elif existing_status and existing_status not in ( - "needs-review", - "accepted", - ): - transactions.append({"type": "request-review", "value": True}) - - edit_args: dict[str, Any] = {"transactions": transactions} - if revision_id: - edit_args["objectIdentifier"] = revision_id + if summary: + transactions.append({"type": "summary", "value": summary}) + revision_result = await _conduit_request( - "differential.revision.edit", **edit_args + "differential.revision.edit", transactions=transactions ) - - object_data = revision_result.get("object") or {} - new_revision_id = object_data.get("id") or revision_id - - if post_transactions and new_revision_id: - await _conduit_request( - "differential.revision.edit", - objectIdentifier=new_revision_id, - transactions=post_transactions, - ) + revision_id = revision_result["object"]["id"] # Store commit info on the diff, exactly as moz-phab does *after* # creating the revision (so the message can embed the Differential # Revision URL). Without this, `moz-phab patch` on the revision # fails with "a diff without commit information detected". - if local_commits and new_revision_id: - await _set_local_commits( - diff_result["diffid"], - local_commits, - title, - raw_summary, - bug_id, - new_revision_id, - ) + await _set_local_commits( + diff_result["diffid"], + submission["local_commits"], + title, + summary, + bug_id, + revision_id, + ) except Exception as exc: log.exception("Failed to submit Phabricator diff for bug %s", bug_id) return ActionResult.failed(str(exc)) - revision_url = _revision_url(new_revision_id) if new_revision_id else None + url = _revision_url(revision_id) return ActionResult.ok( - { - "revision_id": new_revision_id, - "revision_url": revision_url, - "url": revision_url, - } + {"revision_id": revision_id, "revision_url": url, "url": url} ) + + +class UpdatePatchHandler: + """Applies ``phabricator.update_patch``: a new diff on an existing revision. + + Only the diff changes: title, summary, bug id, and review status are left + exactly as they are, so an update never overwrites something a reviewer or + the patch author has since edited. The revision's fields are read only to + rebuild the ``local:commits`` message, which has to match the revision. + """ + + async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult: + revision_id = params["revision_id"] + + try: + raw = await ctx.download_artifact(_DIFF_ARTIFACT_KEY) + submission = json.loads(raw) + except Exception as exc: + log.exception( + "Failed to load Phabricator submission artifact for run %s", ctx.run_id + ) + return ActionResult.failed( + f"No Phabricator submission artifact for this run: {exc}" + ) + + try: + diff_result = await _conduit_request( + "differential.creatediff", + repositoryPHID=await _repository_phid(), + **submission["diff"], + ) + await _conduit_request( + "differential.revision.edit", + objectIdentifier=revision_id, + transactions=[{"type": "update", "value": diff_result["phid"]}], + ) + + fields = await _revision_fields(revision_id) + await _set_local_commits( + diff_result["diffid"], + submission["local_commits"], + fields.get("title") or f"D{revision_id}", + fields.get("summary"), + fields.get("bugzilla.bug-id"), + revision_id, + ) + except Exception as exc: + log.exception("Failed to update Phabricator revision D%s", revision_id) + return ActionResult.failed(str(exc)) + + return ActionResult.ok() diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/registry.py b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/registry.py index e4e7449d69..6eee8c5e4f 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/registry.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/registry.py @@ -8,7 +8,10 @@ from hackbot_runtime.actions.handlers.phabricator_handler import ( AddCommentHandler as PhabricatorAddCommentHandler, ) -from hackbot_runtime.actions.handlers.phabricator_handler import SubmitPatchHandler +from hackbot_runtime.actions.handlers.phabricator_handler import ( + SubmitPatchHandler, + UpdatePatchHandler, +) # 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 @@ -19,6 +22,7 @@ "bugzilla.add_attachment": AddAttachmentHandler(), "bugzilla.create_bug": CreateBugHandler(), "phabricator.submit_patch": SubmitPatchHandler(), + "phabricator.update_patch": UpdatePatchHandler(), "phabricator.add_comment": PhabricatorAddCommentHandler(), } diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py b/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py index 67b83fb3eb..f50334b9b3 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py @@ -4,13 +4,18 @@ 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 typing import Annotated -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 @@ -20,6 +25,12 @@ "revision to correct it.*" ) +# 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: return f"Recorded {action_type} (#{len(recorder.actions) - 1})." @@ -29,28 +40,18 @@ def _confirm(recorder: ActionsRecorder, action_type: str) -> str: async def submit_patch( recorder: ActionsRecorder, bug_id: Annotated[int, Field(description="Bug this patch fixes.")], - reasoning: Annotated[ - str, Field(description="Why you are submitting this patch (for audit log).") - ], - revision_id: Annotated[ - int | 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." - ), - ), - ] = None, title: Annotated[ - str | None, + str, Field( - default=None, - description="Revision title. Required when creating a new revision.", + description=( + "Title for the new revision: a single line describing the fix, " + "as you would write a commit message subject." + ) ), - ] = None, + ], + reasoning: Annotated[ + str, Field(description="Why you are submitting this patch (for audit log).") + ], summary: Annotated[ str | None, Field(default=None, description="Revision summary/description."), @@ -68,11 +69,12 @@ async def submit_patch( ), ] = None, ) -> str: - """Submit your fix for review as a Phabricator revision. + """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. + attachment. If the fix belongs on a revision that already exists (for + example, you were asked for changes on it), use ``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 @@ -80,30 +82,61 @@ async def submit_patch( 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). """ - 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, + {"bug_id": bug_id, "title": title, "summary": summary}, reasoning=reasoning, ref=ref, ) return _confirm(recorder, "phabricator.submit_patch") +@tool +async def update_patch( + recorder: ActionsRecorder, + 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 updating this patch (for audit log).") + ], +) -> str: + """Submit your fix as a new diff on an existing Phabricator revision. + + 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 ``submit_patch`` instead. If the review only + asks a question and no code change is warranted, use ``add_comment``. + + You do not supply a patch file. Your final code changes in the working + 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. + + Only the diff changes: the revision keeps its title, summary, and bug + association exactly as they are. Set `ref` if you want to reference the + revision's URL from another action in the same run, written as + `{{actions..url}}` (for example, inside a bug comment). + """ + recorder.record( + "phabricator.update_patch", + {"revision_id": revision_id}, + reasoning=reasoning, + ) + return _confirm(recorder, "phabricator.update_patch") + + @tool async def add_comment( recorder: ActionsRecorder, 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 8f096618fa..044f79e503 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 @@ -232,7 +233,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 @@ -255,8 +256,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 454fc2a726..ecab4cd8df 100644 --- a/libs/hackbot-runtime/tests/test_context.py +++ b/libs/hackbot-runtime/tests/test_context.py @@ -158,8 +158,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( @@ -169,7 +172,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 e7c40e453c..4823bf0339 100644 --- a/libs/hackbot-runtime/tests/test_phabricator_actions.py +++ b/libs/hackbot-runtime/tests/test_phabricator_actions.py @@ -1,46 +1,77 @@ -"""Tests for the phabricator.submit_patch recording tool.""" +"""Tests for the phabricator recording tools (submit/update patch, comment).""" 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_only_the_revision_id(): + rec = ActionsRecorder() + await phabricator.update_patch(rec, revision_id=12345, reasoning="r") + action = rec.actions[0] + assert action["type"] == "phabricator.update_patch" + # Nothing about the revision itself is the agent's to set on an update. + assert action["params"] == {"revision_id": 12345} + + +async def test_update_requires_revision_id(): + rec = ActionsRecorder() + with pytest.raises(TypeError): + await phabricator.update_patch(rec, reasoning="r") + + +async def test_update_ref_is_recorded(): + rec = ActionsRecorder() + await phabricator.update_patch(rec, revision_id=7, reasoning="r", ref="patch") + assert rec.actions[0]["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"} + + # An update carries the revision id and nothing else about the revision: + # its title, summary and bug id stay as they are. + assert set(update.input_schema["required"]) == {"revision_id", "reasoning"} + assert set(update.input_schema["properties"]) == { + "revision_id", + "reasoning", + "ref", + } + + async def test_add_comment_records_revision_and_text(): rec = ActionsRecorder() await phabricator.add_comment( diff --git a/libs/hackbot-runtime/tests/test_phabricator_handler.py b/libs/hackbot-runtime/tests/test_phabricator_handler.py index bd53cf2f10..4228d29b2f 100644 --- a/libs/hackbot-runtime/tests/test_phabricator_handler.py +++ b/libs/hackbot-runtime/tests/test_phabricator_handler.py @@ -12,6 +12,8 @@ import pytest 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 @pytest.fixture(autouse=True) @@ -39,10 +41,16 @@ def _phabricator_env(monkeypatch): } +# The agent-built artifact always carries both keys (see +# changes.build_phabricator_diff), so handlers may rely on them. +_LOCAL_COMMITS = {"node1": {"author": "Hackbot Agent", "commit": "node1"}} + + def _ctx(diff=_DIFF_PAYLOAD, local_commits=None): - submission = {"diff": diff} - if local_commits is not None: - submission["local_commits"] = local_commits + submission = { + "diff": diff, + "local_commits": local_commits or dict(_LOCAL_COMMITS), + } async def download(key): assert key == "changes/phabricator_diff.json" @@ -56,27 +64,35 @@ def _fake_conduit(responses): async def fake(method, **payload): calls.append((method, payload)) - return responses[method] + return responses.get(method, {}) return fake, calls +def test_each_patch_action_type_has_its_own_handler(): + assert isinstance( + get_handler("phabricator.submit_patch"), phabricator_handler.SubmitPatchHandler + ) + assert isinstance( + get_handler("phabricator.update_patch"), phabricator_handler.UpdatePatchHandler + ) + # Every type the recording side can emit is registered. + assert all(get_handler(t) is not None for t in PATCH_ACTION_TYPES) + + def test_revision_title_strips_and_reprefixes(): rt = phabricator_handler._revision_title - assert rt("Fix bug", wip=True) == "WIP: Fix bug" - assert rt("WIP: Fix bug", wip=True) == "WIP: Fix bug" # not doubled - assert rt("WIP: Fix bug", wip=False) == "Fix bug" # prefix stripped + assert rt("Fix bug") == "WIP: Fix bug" + assert rt("WIP: Fix bug") == "WIP: Fix bug" # not doubled def test_revision_title_never_blank_for_bare_wip_marker(): # A title that is only a WIP marker must fall back to the original, not go # blank (which would be an invalid Phabricator title). - rt = phabricator_handler._revision_title - assert rt("WIP:", wip=True) == "WIP: WIP" - assert rt("WIP", wip=False) == "WIP" + assert phabricator_handler._revision_title("WIP:") == "WIP: WIP" -async def test_submit_patch_create_wip_by_default(monkeypatch): +async def test_submit_patch_creates_wip_revision(monkeypatch): fake, calls = _fake_conduit( { "differential.creatediff": {"phid": "PHID-DIFF-1", "diffid": 1}, @@ -89,7 +105,7 @@ async def test_submit_patch_create_wip_by_default(monkeypatch): ) result = await phabricator_handler.SubmitPatchHandler().apply( - {"bug_id": 1, "revision_id": None, "title": "Fix", "summary": "s"}, + {"bug_id": 1, "title": "Fix", "summary": "s"}, _ctx(), ) @@ -108,41 +124,14 @@ async def test_submit_patch_create_wip_by_default(monkeypatch): assert "objectIdentifier" not in edit_call[1] transactions = {t["type"]: t.get("value") for t in edit_call[1]["transactions"]} assert transactions["update"] == "PHID-DIFF-1" - # WIP by default: title is prefixed, the revision is marked changes-planned, - # and reviewers are NOT requested. + # Everything hackbot creates is a WIP draft: title prefixed, revision + # marked changes-planned, and reviewers are NOT requested. assert transactions["title"] == "WIP: Fix" assert transactions["plan-changes"] is True assert "reviewers.add" not in transactions assert transactions["bugzilla.bug-id"] == "1" -async def test_submit_patch_create_non_wip(monkeypatch): - fake, calls = _fake_conduit( - { - "differential.creatediff": {"phid": "PHID-DIFF-1", "diffid": 1}, - "differential.revision.edit": {"object": {"id": 555}}, - } - ) - monkeypatch.setattr(phabricator_handler, "_conduit_request", fake) - monkeypatch.setattr( - phabricator_handler, "_repository_phid", AsyncMock(return_value="PHID-REPO-1") - ) - - await phabricator_handler.SubmitPatchHandler().apply( - {"bug_id": 1, "title": "Fix", "summary": "s", "wip": False}, - _ctx(), - ) - - edit_call = next(c for c in calls if c[0] == "differential.revision.edit") - transactions = {t["type"]: t.get("value") for t in edit_call[1]["transactions"]} - # Not WIP: no WIP prefix and no plan-changes; a brand-new revision needs no - # request-review (Phabricator auto needs-review). Reviewers are never set. - assert transactions["title"] == "Fix" - assert "reviewers.add" not in transactions - assert "plan-changes" not in transactions - assert "request-review" not in transactions - - async def test_submit_patch_sets_local_commits_property(monkeypatch): fake, calls = _fake_conduit( { @@ -187,7 +176,7 @@ async def test_submit_patch_sets_local_commits_property(monkeypatch): assert stored["author"] == "Hackbot Agent" assert stored["tree"] == "tree1" assert stored["parents"] == ["base1"] - # WIP by default: summary/title carry the WIP prefix and reviewers are empty. + # The stored title carries the WIP prefix and reviewers are empty. assert stored["summary"] == "WIP: Fix the thing" assert stored["message"].startswith("WIP: Fix the thing\n\nSummary:\ndoes it") assert ( @@ -203,50 +192,11 @@ 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): - fake, calls = _fake_conduit( - { - "differential.creatediff": {"phid": "PHID-DIFF-1", "diffid": 3}, - "differential.revision.edit": {"object": {"id": 42}}, - "differential.revision.search": { - "data": [{"fields": {"title": "Existing title", "summary": "old sum"}}] - }, - "differential.setdiffproperty": {}, - } - ) - monkeypatch.setattr(phabricator_handler, "_conduit_request", fake) - monkeypatch.setattr( - phabricator_handler, "_repository_phid", AsyncMock(return_value="PHID-REPO-1") - ) - - result = await phabricator_handler.SubmitPatchHandler().apply( - {"bug_id": 9, "revision_id": 42}, - _ctx(local_commits={"n": {"author": "A"}}), - ) - assert result.status == "applied" - - # No title on the action -> fall back to the existing revision's title, with - # the WIP prefix applied. - stored = json.loads( - next(c for c in calls if c[0] == "differential.setdiffproperty")[1]["data"] - )["n"] - assert stored["summary"] == "WIP: Existing title" - assert ( - "Differential Revision: https://phabricator.services.mozilla.com/D42" - in stored["message"] - ) - - -async def test_submit_patch_update_sets_object_identifier(monkeypatch): +async def test_update_patch_only_updates_the_diff(monkeypatch): fake, calls = _fake_conduit( { "differential.creatediff": {"phid": "PHID-DIFF-2", "diffid": 2}, "differential.revision.edit": {"object": {"id": 12345}}, - "differential.revision.search": { - "data": [ - {"fields": {"title": "T", "status": {"value": "needs-review"}}} - ] - }, } ) monkeypatch.setattr(phabricator_handler, "_conduit_request", fake) @@ -254,59 +204,40 @@ async def test_submit_patch_update_sets_object_identifier(monkeypatch): phabricator_handler, "_repository_phid", AsyncMock(return_value="PHID-REPO-1") ) - result = await phabricator_handler.SubmitPatchHandler().apply( - {"bug_id": 7, "revision_id": 12345}, _ctx() + result = await phabricator_handler.UpdatePatchHandler().apply( + {"revision_id": 12345}, _ctx() ) assert result.status == "applied" assert result.result["revision_id"] == 12345 - edit_call = next(c for c in calls if c[0] == "differential.revision.edit") - assert edit_call[1]["objectIdentifier"] == 12345 - - -async def test_submit_patch_wip_update_already_changes_planned_uses_second_edit( - monkeypatch, -): - fake, calls = _fake_conduit( - { - "differential.creatediff": {"phid": "PHID-DIFF-5", "diffid": 5}, - "differential.revision.edit": {"object": {"id": 50}}, - "differential.revision.search": { - "data": [ - {"fields": {"title": "T", "status": {"value": "changes-planned"}}} - ] - }, - } - ) - monkeypatch.setattr(phabricator_handler, "_conduit_request", fake) - monkeypatch.setattr( - phabricator_handler, "_repository_phid", AsyncMock(return_value="PHID-REPO-1") - ) - - result = await phabricator_handler.SubmitPatchHandler().apply( - {"bug_id": 3, "revision_id": 50}, _ctx() - ) - assert result.status == "applied" - # Revision is already changes-planned, so plan-changes can't ride the main - # edit (no-op status change errors) — it goes in a second, separate edit. + # Exactly one edit, carrying nothing but the new diff: the revision's title, + # summary, bug id and review status are all left alone. edits = [c for c in calls if c[0] == "differential.revision.edit"] - assert len(edits) == 2 - assert all(t["type"] != "plan-changes" for t in edits[0][1]["transactions"]) - assert edits[1][1]["objectIdentifier"] == 50 - assert edits[1][1]["transactions"] == [{"type": "plan-changes", "value": True}] + assert len(edits) == 1 + assert edits[0][1]["objectIdentifier"] == 12345 + assert edits[0][1]["transactions"] == [{"type": "update", "value": "PHID-DIFF-2"}] + # The revision is read, but only to rebuild the local:commits message. + assert "differential.revision.search" in [c[0] for c in calls] -async def test_submit_patch_non_wip_update_requests_review(monkeypatch): +async def test_update_patch_local_commits_use_the_revisions_own_fields(monkeypatch): fake, calls = _fake_conduit( { - "differential.creatediff": {"phid": "PHID-DIFF-6", "diffid": 6}, - "differential.revision.edit": {"object": {"id": 60}}, + "differential.creatediff": {"phid": "PHID-DIFF-1", "diffid": 3}, + "differential.revision.edit": {"object": {"id": 42}}, "differential.revision.search": { "data": [ - {"fields": {"title": "T", "status": {"value": "changes-planned"}}} + { + "fields": { + "title": "WIP: Existing title", + "summary": "old sum", + "bugzilla.bug-id": "9", + } + } ] }, + "differential.setdiffproperty": {}, } ) monkeypatch.setattr(phabricator_handler, "_conduit_request", fake) @@ -314,46 +245,41 @@ async def test_submit_patch_non_wip_update_requests_review(monkeypatch): phabricator_handler, "_repository_phid", AsyncMock(return_value="PHID-REPO-1") ) - await phabricator_handler.SubmitPatchHandler().apply( - {"bug_id": 4, "revision_id": 60, "wip": False}, _ctx() - ) - - # Re-activating an existing non-review revision requests review; not WIP. - edit_call = next(c for c in calls if c[0] == "differential.revision.edit") - types = {t["type"] for t in edit_call[1]["transactions"]} - assert "request-review" in types - assert "plan-changes" not in types - - -async def test_submit_patch_falls_back_to_given_revision_id_when_edit_omits_object( - monkeypatch, -): - fake, _ = _fake_conduit( - { - "differential.creatediff": {"phid": "PHID-DIFF-3"}, - "differential.revision.edit": {"object": {}}, - "differential.revision.search": {"data": [{"fields": {}}]}, - } - ) - monkeypatch.setattr(phabricator_handler, "_conduit_request", fake) - monkeypatch.setattr( - phabricator_handler, "_repository_phid", AsyncMock(return_value="PHID-REPO-1") + result = await phabricator_handler.UpdatePatchHandler().apply( + {"revision_id": 42}, + _ctx(local_commits={"n": {"author": "A"}}), ) + assert result.status == "applied" - result = await phabricator_handler.SubmitPatchHandler().apply( - {"bug_id": 7, "revision_id": 999}, _ctx() + # The commit message mirrors the revision as it stands: its own title, + # summary and bug id, verbatim. + stored = json.loads( + next(c for c in calls if c[0] == "differential.setdiffproperty")[1]["data"] + )["n"] + assert stored["summary"] == "WIP: Existing title" + assert stored["message"].startswith("WIP: Existing title\n\nSummary:\nold sum") + assert "Bug #: 9" in stored["message"] + assert ( + "Differential Revision: https://phabricator.services.mozilla.com/D42" + in stored["message"] ) - assert result.status == "applied" - assert result.result["revision_id"] == 999 -async def test_submit_patch_missing_artifact_fails(): +@pytest.mark.parametrize( + ("handler", "params"), + [ + ("SubmitPatchHandler", {"bug_id": 1, "title": "x"}), + ("UpdatePatchHandler", {"revision_id": 7}), + ], +) +async def test_missing_artifact_fails(handler, params): 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 getattr(phabricator_handler, handler)().apply(params, ctx) assert result.status == "failed" + assert "No Phabricator submission artifact" in result.error async def test_submit_patch_conduit_error_fails(monkeypatch): diff --git a/services/hackbot-api/app/actions_applier.py b/services/hackbot-api/app/actions_applier.py index 4a75c79c2a..10d668a139 100644 --- a/services/hackbot-api/app/actions_applier.py +++ b/services/hackbot-api/app/actions_applier.py @@ -36,11 +36,6 @@ _PLACEHOLDER_RE = re.compile(r"\{\{actions\.([^.}]+)\.([^}]+)\}\}") -# Whether Phabricator patches are submitted as work-in-progress is hardcoded -# here for now; later this should come from per-agent config (hackbot.toml), -# which isn't wired into hackbot-api yet. -SUBMIT_PATCHES_AS_WIP = True - def resolve_placeholders(value: Any, results_by_ref: dict[str, dict]) -> Any: """Substitute `{{actions..}}` in `value` using prior results. @@ -113,10 +108,6 @@ async def _dispatch( f"No handler registered for action type '{action_type}'" ) - if action_type == "phabricator.submit_patch": - # WIP is a hackbot-api policy, not an agent choice; inject it here. - params = {**params, "wip": SUBMIT_PATCHES_AS_WIP} - ctx = ApplyContext( run_id=str(run.run_id), download_artifact=lambda key, run_id=str(run.run_id): ( diff --git a/services/hackbot-api/tests/test_actions_applier.py b/services/hackbot-api/tests/test_actions_applier.py index 4c668346f2..37e10eef5c 100644 --- a/services/hackbot-api/tests/test_actions_applier.py +++ b/services/hackbot-api/tests/test_actions_applier.py @@ -399,52 +399,11 @@ async def test_backward_placeholder_resolves_in_coalesced_comment(monkeypatch): # 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 call carries the hackbot-api-injected `wip` flag. assert handler.calls == [ - {"wip": True}, + {}, { "bug_id": 5, "changes": {"a": 1}, "comment": {"body": "see http://x/D1", "is_private": False}, }, ] - - -async def test_phabricator_submit_patch_gets_wip_injected(monkeypatch): - # WIP is a hackbot-api policy injected at dispatch, not part of the recorded - # action — the agent never sets it. - handler = _RecordingHandler( - SimpleNamespace(status="applied", result={}, error=None) - ) - monkeypatch.setattr(actions_applier, "get_handler", lambda t: handler) - - row = _row( - 0, - "pending", - action_type="phabricator.submit_patch", - params={"bug_id": 1, "title": "x"}, - ) - await actions_applier._apply_pending_rows( - _FakeDB(), _FakeRun(status=RunStatus.succeeded.value), [(row, [])] - ) - - assert handler.calls[0]["wip"] is actions_applier.SUBMIT_PATCHES_AS_WIP - - -async def test_non_phabricator_action_gets_no_wip(monkeypatch): - handler = _RecordingHandler( - SimpleNamespace(status="applied", result={}, error=None) - ) - monkeypatch.setattr(actions_applier, "get_handler", lambda t: handler) - - row = _row( - 0, - "pending", - action_type="bugzilla.add_comment", - params={"bug_id": 1, "text": "hi"}, - ) - await actions_applier._apply_pending_rows( - _FakeDB(), _FakeRun(status=RunStatus.succeeded.value), [(row, [])] - ) - - assert "wip" not in handler.calls[0]