Add test plans to Phabricator patch submissions (#6737)

Author: ayoubdiourin7

docs/hackbot/actions.md

@@ -34,17 +34,17 @@ record in its `config.py` and passes them to `actions_server_for`, which builds
 carrying only those. `bug-fix`, for instance, allows `phabricator.submit_patch` on a fresh
 triage run but swaps it for `phabricator.update_patch` on a follow-up.
 
-| Action type                 | Records the intent to…                     | Params                             |
-| --------------------------- | ------------------------------------------ | ---------------------------------- |
-| `bugzilla.update_bug`       | Change a bug's fields                      | `bug_id`, `changes`                |
-| `bugzilla.add_comment`      | Comment on a bug                           | `bug_id`, `text`, `is_private`     |
-| `bugzilla.add_attachment`   | Attach a file to a bug                     | `bug_id`, + a `file` attachment    |
-| `bugzilla.create_bug`       | File a new bug                             | the new bug's fields               |
-| `phabricator.submit_patch`  | Deliver a fix as a **new** revision        | `bug_id`, `title`, `summary`       |
-| `phabricator.update_patch`  | Add a new diff to an **existing** revision | `revision_id`                      |
-| `phabricator.add_comment`   | Reply on a revision without changing code  | `revision_id`, `text`              |
-| `testrail.submit_test_plan` | Submit a generated test plan to TestRail   | the validated feature + test cases |
-| `slack.post_message`        | Post a message to Slack                    | `channel`, `text`                  |
+| Action type                 | Records the intent to…                     | Params                                    |
+| --------------------------- | ------------------------------------------ | ----------------------------------------- |
+| `bugzilla.update_bug`       | Change a bug's fields                      | `bug_id`, `changes`                       |
+| `bugzilla.add_comment`      | Comment on a bug                           | `bug_id`, `text`, `is_private`            |
+| `bugzilla.add_attachment`   | Attach a file to a bug                     | `bug_id`, + a `file` attachment           |
+| `bugzilla.create_bug`       | File a new bug                             | the new bug's fields                      |
+| `phabricator.submit_patch`  | Deliver a fix as a **new** revision        | `bug_id`, `title`, `summary`, `test_plan` |
+| `phabricator.update_patch`  | Add a new diff to an **existing** revision | `revision_id`                             |
+| `phabricator.add_comment`   | Reply on a revision without changing code  | `revision_id`, `text`                     |
+| `testrail.submit_test_plan` | Submit a generated test plan to TestRail   | the validated feature + test cases        |
+| `slack.post_message`        | Post a message to Slack                    | `channel`, `text`                         |
 
 All but `testrail.submit_test_plan` take a **`reasoning`** argument — a free-text audit trail
 stored on the action and shown in the UI beside the proposed change. `phabricator.submit_patch`

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

@@ -145,7 +145,13 @@ def _local_commit_author_fields(previous_commits: list[dict]) -> dict[str, str]:
     }
 
 
-def _arc_commit_message(title: str, summary: str | None, bug_id: Any, url: str) -> str:
+def _arc_commit_message(
+    title: str,
+    summary: str | None,
+    test_plan: 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
@@ -159,7 +165,7 @@ def _arc_commit_message(title: str, summary: str | None, bug_id: Any, url: str)
     return _ARC_COMMIT_MESSAGE_TEMPLATE.format(
         title=title,
         body=body,
-        test_plan="",
+        test_plan=test_plan or "",
         reviewers="",
         bug_id=bug_id if bug_id is not None else "",
     )
@@ -170,6 +176,7 @@ async def _set_local_commits(
     local_commits: dict,
     title: str,
     summary: str | None,
+    test_plan: str | None,
     bug_id: Any,
     revision_id: int,
 ) -> None:
@@ -180,7 +187,9 @@ async def _set_local_commits(
     arc-formatted ``message`` are filled in here, since they need the revision
     URL.
     """
-    message = _arc_commit_message(title, summary, bug_id, _revision_url(revision_id))
+    message = _arc_commit_message(
+        title, summary, test_plan, bug_id, _revision_url(revision_id)
+    )
     for commit_info in local_commits.values():
         commit_info["summary"] = title
         commit_info["message"] = message
@@ -220,6 +229,7 @@ class SubmitPatchHandler:
     async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult:
         bug_id = params["bug_id"]
         summary = params.get("summary")
+        test_plan = params.get("test_plan")
 
         try:
             raw = await ctx.download_artifact(_DIFF_ARTIFACT_KEY)
@@ -251,6 +261,8 @@ async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult
             ]
             if summary:
                 transactions.append({"type": "summary", "value": summary})
+            if test_plan:
+                transactions.append({"type": "testPlan", "value": test_plan})
 
             revision_result = await _conduit_request(
                 "differential.revision.edit", transactions=transactions
@@ -266,6 +278,7 @@ async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult
                 submission["local_commits"],
                 title,
                 summary,
+                test_plan,
                 bug_id,
                 revision_id,
             )
@@ -329,6 +342,7 @@ async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult
                 submission["local_commits"],
                 fields.get("title") or f"D{revision_id}",
                 fields.get("summary"),
+                fields.get("testPlan"),
                 fields.get("bugzilla.bug-id"),
                 revision_id,
             )

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

@@ -44,8 +44,8 @@ def _validate_summary(summary: str | None) -> None:
     if match:
         raise ToolError(
             f'Invalid Phabricator summary: "{match.group()}" at the beginning of '
-            "a line is interpreted as a Test Plan field. Call submit_patch again "
-            "with that fixed."
+            "a line belongs in the Test Plan field. Move the verification details "
+            "to the test_plan argument and call submit_patch again."
         )
 
 
@@ -65,9 +65,19 @@ async def submit_patch(
     reasoning: Annotated[
         str, Field(description="Why you are submitting this patch (for audit log).")
     ],
+    test_plan: Annotated[
+        str | None,
+        Field(default=None, description="Revision test plan."),
+    ] = None,
     summary: Annotated[
         str | None,
-        Field(default=None, description="Revision summary/description."),
+        Field(
+            default=None,
+            description=(
+                "Revision summary/description. Keep test and verification details "
+                "in test_plan instead."
+            ),
+        ),
     ] = None,
     ref: Annotated[
         str | None,
@@ -102,7 +112,12 @@ async def submit_patch(
     _validate_summary(summary)
     recorder.record(
         "phabricator.submit_patch",
-        {"bug_id": bug_id, "title": title, "summary": summary},
+        {
+            "bug_id": bug_id,
+            "title": title,
+            "summary": summary,
+            "test_plan": test_plan,
+        },
         reasoning=reasoning,
         ref=ref,
     )

libs/hackbot-runtime/tests/test_phabricator_actions.py

@@ -5,17 +5,23 @@
 from hackbot_runtime.actions import ActionsRecorder, phabricator
 
 
-async def test_submit_records_create_params_only():
+async def test_submit_records_test_plan():
     rec = ActionsRecorder()
     await phabricator.submit_patch(
-        rec, bug_id=1, title="Fix the thing", reasoning="r", summary="Details"
+        rec,
+        bug_id=1,
+        title="Fix the thing",
+        test_plan="uv run pytest tests/test_thing.py (passed)",
+        reasoning="r",
+        summary="Details",
     )
     action = rec.actions[0]
     assert action["type"] == "phabricator.submit_patch"
     assert action["params"] == {
         "bug_id": 1,
         "title": "Fix the thing",
         "summary": "Details",
+        "test_plan": "uv run pytest tests/test_thing.py (passed)",
     }
     assert "ref" not in action
 
@@ -37,7 +43,7 @@ async def test_submit_rejects_test_plan_headers(header):
         )
 
     assert header in str(exc.value)
-    assert "Call submit_patch again with that fixed" in str(exc.value)
+    assert "test_plan argument" in str(exc.value)
     assert rec.actions == []
 
 
@@ -55,7 +61,11 @@ async def test_submit_accepts_safe_summary(summary):
     rec = ActionsRecorder()
 
     await phabricator.submit_patch(
-        rec, bug_id=1, title="Fix", reasoning="r", summary=summary
+        rec,
+        bug_id=1,
+        title="Fix",
+        reasoning="r",
+        summary=summary,
     )
 
     assert rec.actions[0]["params"]["summary"] == summary
@@ -67,10 +77,20 @@ async def test_submit_requires_title():
         await phabricator.submit_patch(rec, bug_id=1, reasoning="r")
 
 
+async def test_submit_accepts_missing_test_plan():
+    rec = ActionsRecorder()
+    await phabricator.submit_patch(rec, bug_id=1, title="Fix", reasoning="r")
+    assert rec.actions[0]["params"]["test_plan"] is None
+
+
 async def test_submit_ref_is_recorded():
     rec = ActionsRecorder()
     await phabricator.submit_patch(
-        rec, bug_id=1, title="Fix", reasoning="r", ref="patch"
+        rec,
+        bug_id=1,
+        title="Fix",
+        reasoning="r",
+        ref="patch",
     )
     assert rec.actions[0]["ref"] == "patch"
 
@@ -96,7 +116,12 @@ def test_agent_facing_schemas_are_case_specific():
     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(submit.input_schema["required"]) == {
+        "bug_id",
+        "title",
+        "reasoning",
+    }
+    assert "test_plan" in submit.input_schema["properties"]
 
     # An update carries the revision id and nothing else: the revision's title,
     # summary and bug id stay as they are, and there is no new URL to reference.

libs/hackbot-runtime/tests/test_phabricator_handler.py

@@ -99,7 +99,12 @@ async def test_submit_patch_creates_planned_changes_revision(monkeypatch):
     )
 
     result = await phabricator_handler.SubmitPatchHandler().apply(
-        {"bug_id": 1, "title": "Fix", "summary": "s"},
+        {
+            "bug_id": 1,
+            "title": "Fix",
+            "summary": "s",
+            "test_plan": "uv run pytest (passed)",
+        },
         _ctx(),
     )
 
@@ -124,6 +129,31 @@ async def test_submit_patch_creates_planned_changes_revision(monkeypatch):
     assert "reviewers.add" not in transactions
     assert transactions["bugzilla.bug-id"] == "1"
     assert transactions["summary"] == "s"
+    assert transactions["testPlan"] == "uv run pytest (passed)"
+
+
+async def test_submit_patch_accepts_legacy_action_without_test_plan(monkeypatch):
+    fake, calls = _fake_conduit(
+        {
+            "differential.creatediff": {"phid": "PHID-DIFF-1", "diffid": 1},
+            "differential.revision.edit": {"object": {"id": 555}},
+            "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": 1, "title": "Fix", "summary": "s"},
+        _ctx(),
+    )
+
+    assert result.status == "applied"
+    edit_call = next(c for c in calls if c[0] == "differential.revision.edit")
+    transaction_types = {t["type"] for t in edit_call[1]["transactions"]}
+    assert "testPlan" not in transaction_types
 
 
 async def test_submit_patch_sets_local_commits_property(monkeypatch):
@@ -150,7 +180,12 @@ 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"},
+        {
+            "bug_id": 5,
+            "title": "Fix the thing",
+            "summary": "does it",
+            "test_plan": "uv run pytest tests/test_thing.py (passed)",
+        },
         _ctx(local_commits={"node1": dict(git_fields)}),
     )
     assert result.status == "applied"
@@ -173,6 +208,7 @@ async def test_submit_patch_sets_local_commits_property(monkeypatch):
     # The stored title matches the visible revision title and reviewers are empty.
     assert stored["summary"] == "Fix the thing"
     assert stored["message"].startswith("Fix the thing\n\nSummary:\ndoes it")
+    assert "Test Plan:\nuv run pytest tests/test_thing.py (passed)" in stored["message"]
     assert (
         "Differential Revision: https://phabricator.services.mozilla.com/D77"
         in stored["message"]
@@ -228,6 +264,7 @@ async def test_update_patch_local_commits_use_the_revisions_own_fields(monkeypat
                         "fields": {
                             "title": "WIP: Existing title",
                             "summary": "old sum",
+                            "testPlan": "existing test plan",
                             "bugzilla.bug-id": "9",
                         }
                     }
@@ -254,6 +291,7 @@ async def test_update_patch_local_commits_use_the_revisions_own_fields(monkeypat
     )["n"]
     assert stored["summary"] == "WIP: Existing title"
     assert stored["message"].startswith("WIP: Existing title\n\nSummary:\nold sum")
+    assert "Test Plan:\nexisting test plan" in stored["message"]
     assert "Bug #: 9" in stored["message"]
     assert (
         "Differential Revision: https://phabricator.services.mozilla.com/D42"