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 740a66151f..cf36a69957 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py @@ -14,6 +14,7 @@ import json import logging import os +import re from functools import lru_cache from typing import Any @@ -100,8 +101,19 @@ def _repository_phid() -> str: """.strip() +# Mirrors moz-phab's WIP_RE / revision_title (mozphab.commits): strip any +# existing WIP prefix, then prepend "WIP: " for a work-in-progress revision. +_WIP_PREFIX_RE = re.compile(r"^(?:WIP[: ]|WIP$)", re.IGNORECASE) + + +def _revision_title(title: str, wip: bool) -> str: + title = _WIP_PREFIX_RE.sub("", title) or "WIP" + title = title.strip() + return f"WIP: {title}" if wip else title + + def _revision_fields(revision_id: int) -> dict: - """The current title/summary of an existing revision (for update-with-no-title).""" + """The current fields (title/summary/status) of an existing revision.""" result = _conduit_request( "differential.revision.search", constraints={"ids": [int(revision_id)]} ) @@ -109,13 +121,12 @@ def _revision_fields(revision_id: int) -> dict: return data[0].get("fields", {}) if data else {} -def _arc_commit_message( - title: str, summary: str | None, reviewers: list | None, bug_id: Any, url: str -) -> str: +def _arc_commit_message(title: str, summary: str | None, bug_id: Any, url: str) -> str: """Build moz-phab's arc commit message, with the Differential Revision URL. Mirrors ``Commit.build_arc_commit_message`` + ``amend_revision_url`` so the - reconstructed commit reads identically to a moz-phab submission. + reconstructed commit reads identically to a moz-phab submission. Reviewers + are always empty: hackbot never assigns them (WIP submissions omit them). """ body = summary or "" if body: @@ -125,7 +136,7 @@ def _arc_commit_message( title=title, body=body, test_plan="", - reviewers=", ".join(reviewers or []), + reviewers="", bug_id=bug_id if bug_id is not None else "", ) @@ -133,27 +144,19 @@ def _arc_commit_message( def _set_local_commits( diff_id: Any, local_commits: dict, - params: dict[str, Any], + title: str, + summary: str | None, bug_id: Any, revision_id: int, ) -> None: """Complete and store moz-phab's ``local:commits`` diff property. The git-derived fields (author/time/tree/parents/node) come from the - agent-built artifact; ``summary`` (the revision title) and the arc-formatted - ``message`` are filled in here, since they need the revision URL. + agent-built artifact; ``summary`` (the resolved, possibly ``WIP:``-prefixed + revision title) and the arc-formatted ``message`` are filled in here, since + they need the revision URL. """ - title = params.get("title") - summary = params.get("summary") - if not title: - fields = _revision_fields(revision_id) - title = fields.get("title") or f"Bug {bug_id}" - if summary is None: - summary = fields.get("summary") - - message = _arc_commit_message( - title, summary, params.get("reviewers"), bug_id, _revision_url(revision_id) - ) + message = _arc_commit_message(title, summary, bug_id, _revision_url(revision_id)) for commit_info in local_commits.values(): commit_info["summary"] = title commit_info["message"] = message @@ -188,6 +191,8 @@ async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult diff_payload = submission.get("diff", submission) local_commits = submission.get("local_commits") + wip = params.get("wip", True) + try: diff_result = _conduit_request( "differential.creatediff", @@ -196,23 +201,46 @@ async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult ) 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 = _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. transactions: list[dict[str, Any]] = [ - {"type": "update", "value": diff_phid} + {"type": "update", "value": diff_phid}, + {"type": "title", "value": title}, ] - if params.get("title"): - transactions.append({"type": "title", "value": params["title"]}) - if params.get("summary"): - transactions.append({"type": "summary", "value": params["summary"]}) - if params.get("reviewers"): - # Assumes Phabricator resolves these identifiers directly; - # if Mozilla's instance requires PHIDs instead of usernames - # for this transaction, a user.search-based resolution step - # needs adding here — not verified against a live instance. - transactions.append( - {"type": "reviewers.add", "value": params["reviewers"]} - ) + 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 @@ -223,6 +251,13 @@ async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult object_data = revision_result.get("object") or {} new_revision_id = object_data.get("id") or revision_id + if post_transactions and new_revision_id: + _conduit_request( + "differential.revision.edit", + objectIdentifier=new_revision_id, + transactions=post_transactions, + ) + # 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 @@ -231,7 +266,8 @@ async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult _set_local_commits( diff_result["diffid"], local_commits, - params, + title, + raw_summary, bug_id, new_revision_id, ) diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py b/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py index f096ee23c2..e467908ed3 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py @@ -39,10 +39,6 @@ async def submit_patch( ), ), ] = None, - reviewers: Annotated[ - list[str] | None, - Field(default=None, description="Reviewers to request on the revision."), - ] = None, title: Annotated[ str | None, Field( @@ -91,7 +87,6 @@ async def submit_patch( params: dict[str, Any] = { "bug_id": bug_id, "revision_id": revision_id, - "reviewers": reviewers or [], "title": title, "summary": summary, } diff --git a/libs/hackbot-runtime/tests/test_phabricator_actions.py b/libs/hackbot-runtime/tests/test_phabricator_actions.py index 172bbc7e46..43c59c756a 100644 --- a/libs/hackbot-runtime/tests/test_phabricator_actions.py +++ b/libs/hackbot-runtime/tests/test_phabricator_actions.py @@ -21,7 +21,6 @@ async def test_create_records_without_revision_id(): assert action["params"] == { "bug_id": 1, "revision_id": None, - "reviewers": [], "title": "Fix the thing", "summary": "Details", } @@ -40,9 +39,3 @@ async def test_ref_is_recorded(): rec, bug_id=1, reasoning="r", title="Fix", ref="patch" ) assert rec.actions[0]["ref"] == "patch" - - -async def test_reviewers_default_to_empty_list(): - rec = ActionsRecorder() - await phabricator.submit_patch(rec, bug_id=1, reasoning="r", title="Fix") - assert rec.actions[0]["params"]["reviewers"] == [] diff --git a/libs/hackbot-runtime/tests/test_phabricator_handler.py b/libs/hackbot-runtime/tests/test_phabricator_handler.py index 8411da6cfc..dfc679fc88 100644 --- a/libs/hackbot-runtime/tests/test_phabricator_handler.py +++ b/libs/hackbot-runtime/tests/test_phabricator_handler.py @@ -42,7 +42,22 @@ def fake(method, **payload): return fake, calls -async def test_submit_patch_create_success(monkeypatch): +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 + + +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" + + +async def test_submit_patch_create_wip_by_default(monkeypatch): fake, calls = _fake_conduit( { "differential.creatediff": {"phid": "PHID-DIFF-1", "diffid": 1}, @@ -53,13 +68,7 @@ async def test_submit_patch_create_success(monkeypatch): 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", - "reviewers": ["alice"], - }, + {"bug_id": 1, "revision_id": None, "title": "Fix", "summary": "s"}, _ctx(), ) @@ -75,13 +84,41 @@ async def test_submit_patch_create_success(monkeypatch): edit_call = next(c for c in calls if c[0] == "differential.revision.edit") assert "objectIdentifier" not in edit_call[1] - transactions = {t["type"]: t["value"] for t in edit_call[1]["transactions"]} + transactions = {t["type"]: t.get("value") for t in edit_call[1]["transactions"]} assert transactions["update"] == "PHID-DIFF-1" - assert transactions["title"] == "Fix" - assert transactions["reviewers.add"] == ["alice"] + # WIP by default: title is prefixed, the revision is 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", lambda: "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( { @@ -104,12 +141,7 @@ async def test_submit_patch_sets_local_commits_property(monkeypatch): "tree": "tree1", } result = await phabricator_handler.SubmitPatchHandler().apply( - { - "bug_id": 5, - "title": "Fix the thing", - "summary": "does it", - "reviewers": ["alice"], - }, + {"bug_id": 5, "title": "Fix the thing", "summary": "does it"}, _ctx(local_commits={"node1": dict(git_fields)}), ) assert result.status == "applied" @@ -129,14 +161,14 @@ async def test_submit_patch_sets_local_commits_property(monkeypatch): assert stored["author"] == "Hackbot Agent" assert stored["tree"] == "tree1" assert stored["parents"] == ["base1"] - # summary is the revision title; message is arc-formatted with the URL - assert stored["summary"] == "Fix the thing" - assert stored["message"].startswith("Fix the thing\n\nSummary:\ndoes it") + # WIP by default: summary/title carry 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 ( "Differential Revision: https://phabricator.services.mozilla.com/D77" in stored["message"] ) - assert "Reviewers: alice" in stored["message"] + assert "Reviewers: \n" in stored["message"] assert "Bug #: 5" in stored["message"] # creatediff gets the raw diff payload; local_commits never leaks into it. @@ -165,11 +197,12 @@ async def test_submit_patch_local_commits_fetches_title_on_update(monkeypatch): ) assert result.status == "applied" - # No title on the action -> fall back to the existing revision's title. + # 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"] == "Existing title" + assert stored["summary"] == "WIP: Existing title" assert ( "Differential Revision: https://phabricator.services.mozilla.com/D42" in stored["message"] @@ -181,6 +214,11 @@ async def test_submit_patch_update_sets_object_identifier(monkeypatch): { "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) @@ -196,6 +234,63 @@ 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( + 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", lambda: "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. + 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}] + + +async def test_submit_patch_non_wip_update_requests_review(monkeypatch): + fake, calls = _fake_conduit( + { + "differential.creatediff": {"phid": "PHID-DIFF-6", "diffid": 6}, + "differential.revision.edit": {"object": {"id": 60}}, + "differential.revision.search": { + "data": [ + {"fields": {"title": "T", "status": {"value": "changes-planned"}}} + ] + }, + } + ) + monkeypatch.setattr(phabricator_handler, "_conduit_request", fake) + monkeypatch.setattr(phabricator_handler, "_repository_phid", lambda: "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, ): @@ -203,6 +298,7 @@ async def test_submit_patch_falls_back_to_given_revision_id_when_edit_omits_obje { "differential.creatediff": {"phid": "PHID-DIFF-3"}, "differential.revision.edit": {"object": {}}, + "differential.revision.search": {"data": [{"fields": {}}]}, } ) monkeypatch.setattr(phabricator_handler, "_conduit_request", fake) diff --git a/services/hackbot-api/app/actions_applier.py b/services/hackbot-api/app/actions_applier.py index 10d668a139..4a75c79c2a 100644 --- a/services/hackbot-api/app/actions_applier.py +++ b/services/hackbot-api/app/actions_applier.py @@ -36,6 +36,11 @@ _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. @@ -108,6 +113,10 @@ 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 37e10eef5c..4c668346f2 100644 --- a/services/hackbot-api/tests/test_actions_applier.py +++ b/services/hackbot-api/tests/test_actions_applier.py @@ -399,11 +399,52 @@ 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]