Handle stacked Phabricator revision patches

.taskcluster.yml

@@ -149,6 +149,10 @@ tasks:
                     uv run --locked --with pytest==9.1.0 --with pytest-cov==7.1.0 --with pytest-asyncio==1.4.0 --extra bugzilla --extra firefox --extra claude-sdk pytest --cov=agent_tools --cov-append tests/ &&
                     cd ../hackbot-runtime &&
                     uv run --locked --with pytest==9.1.0 --with pytest-cov==7.1.0 --with pytest-asyncio==1.4.0 --extra claude-sdk --extra phabricator pytest --cov=hackbot_runtime --cov-append tests/ &&
+                    cd ../phabricator-client &&
+                    uv run --locked --with pytest==9.1.0 --with pytest-cov==7.1.0 --with pytest-asyncio==1.4.0 pytest --cov=phabricator_client --cov-append tests/ &&
+                    cd ../../agents/bug-fix &&
+                    uv run --locked --with pytest==9.1.0 --with pytest-cov==7.1.0 --with pytest-asyncio==1.4.0 pytest --cov=hackbot_agents --cov-append tests/ &&
                     cd ../.. &&
                     bash <(curl -s https://codecov.io/bash)"
               metadata:

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

@@ -11,8 +11,8 @@
   a follow-up run can read the revision it was called on: its metadata, the
   full comment thread, and where each inline comment sits.
 - Phabricator: `GET /phabricator/revision/{id}/patch` returns a revision's base
-  commit + raw diff, so the agent can check its source tree out at the revision
-  before running (see ``revision.checkout_revision``).
+  commit + the patches to replay onto it, so the agent can check its source tree
+  out at the revision before running (see ``revision.checkout_revision``).
 """
 
 import logging
@@ -27,6 +27,7 @@
 from agent_tools.phabricator import PhabricatorContext
 from mcp.server.streamable_http_manager import StreamableHTTPSessionManager
 from phabricator_client import (
+    MissingPatchError,
     PhabricatorClient,
     PhabricatorSettings,
     UnresolvedCommitError,
@@ -54,33 +55,32 @@ class BrokerInputs(BaseSettings):
 
 
 def _patch_endpoint(client: PhabricatorClient):
-    """A read-only endpoint returning a revision's base commit + raw diff.
+    """A read-only endpoint returning a revision's base commit + its patches.
 
     The broker holds the Conduit key; the agent only ever sees this loopback URL,
     so it can reproduce the revision's tree without any credentials.
+
+    ``patches`` is bottom-first and usually holds just the revision's own diff;
+    a revision stacked on unlanded parents is preceded by their diffs, since the
+    commit it was built on exists only in the author's repository (see
+    ``PhabricatorClient.get_patch_stack``).
     """
 
     async def get_patch(request):
         revision_id = int(request.path_params["revision_id"])
-        diff = await client.query_latest_diff(revision_id)
-        if diff is None:
-            return JSONResponse(
-                {"error": f"D{revision_id} has no diffs"}, status_code=404
-            )
-        if not diff.base_commit:
-            return JSONResponse(
-                {"error": f"D{revision_id} diff {diff.id} has no base commit"},
-                status_code=404,
-            )
-        raw_diff = await client.get_raw_diff(diff.id)
-        # The recorded base is often an abbreviated hash; git can only fetch a
-        # full object id, so expand it here.
         try:
-            base_commit = await client.resolve_commit(diff.base_commit)
+            stack = await client.get_patch_stack(revision_id)
+        except MissingPatchError as exc:
+            log.warning("No patch for D%s: %s", revision_id, exc)
+            return JSONResponse({"error": str(exc)}, status_code=404)
         except UnresolvedCommitError as exc:
+            # 422, not 404: the revision and its base exist, they are just
+            # unusable. Say why here; `git fetch` would only report an exit
+            # status later on.
+            log.warning("Cannot serve a patch for D%s: %s", revision_id, exc)
             return JSONResponse({"error": str(exc)}, status_code=422)
 
-        return JSONResponse({"base_commit": base_commit, "raw_diff": raw_diff})
+        return JSONResponse(stack.model_dump())
 
     return get_patch
 

agents/bug-fix/tests/test_broker.py

@@ -5,8 +5,10 @@
 import pytest
 from hackbot_agents.bug_fix import broker
 from phabricator_client import (
-    PhabricatorDiff,
+    MissingPatchError,
+    PatchStack,
     PhabricatorSettings,
+    RevisionPatch,
     UnresolvedCommitError,
 )
 from pydantic import ValidationError
@@ -24,35 +26,74 @@ def _client(fake) -> TestClient:
     return TestClient(Starlette(routes=[route]))
 
 
-def test_patch_route_returns_base_and_diff():
+def test_patch_route_returns_base_and_patches():
     fake = AsyncMock()
-    fake.query_latest_diff = AsyncMock(
-        return_value=PhabricatorDiff(id=9, base_commit="base9")
+    fake.get_patch_stack = AsyncMock(
+        return_value=PatchStack(
+            base_commit="base9full",
+            patches=[
+                RevisionPatch(
+                    revision_id=42,
+                    diff_id=9,
+                    base_commit="base9",
+                    raw_diff="diff --git a/f b/f\n",
+                )
+            ],
+        )
     )
-    fake.get_raw_diff = AsyncMock(return_value="diff --git a/f b/f\n")
-    # The abbreviated base is expanded to a full, fetchable hash.
-    fake.resolve_commit = AsyncMock(return_value="base9full")
 
     resp = _client(fake).get("/phabricator/revision/42/patch")
 
     assert resp.status_code == 200
     assert resp.json() == {
         "base_commit": "base9full",
-        "raw_diff": "diff --git a/f b/f\n",
+        "patches": [
+            {
+                "revision_id": 42,
+                "diff_id": 9,
+                "base_commit": "base9",
+                "raw_diff": "diff --git a/f b/f\n",
+            }
+        ],
     }
-    fake.get_raw_diff.assert_awaited_once_with(9)
-    fake.resolve_commit.assert_awaited_once_with("base9")
+    fake.get_patch_stack.assert_awaited_once_with(42)
+
+
+def test_patch_route_serves_a_stack_bottom_first():
+    # A revision stacked on an unlanded parent: the agent gets both diffs, in
+    # the order they have to be applied.
+    fake = AsyncMock()
+    fake.get_patch_stack = AsyncMock(
+        return_value=PatchStack(
+            base_commit="landed",
+            patches=[
+                RevisionPatch(
+                    revision_id=41,
+                    diff_id=8,
+                    base_commit="landed",
+                    raw_diff="parent\n",
+                ),
+                RevisionPatch(
+                    revision_id=42,
+                    diff_id=9,
+                    base_commit="unlanded",
+                    raw_diff="child\n",
+                ),
+            ],
+        )
+    )
+
+    resp = _client(fake).get("/phabricator/revision/42/patch")
+
+    assert resp.status_code == 200
+    assert [patch["revision_id"] for patch in resp.json()["patches"]] == [41, 42]
 
 
 def test_patch_route_422_when_base_cannot_be_expanded(caplog):
     # Serving the abbreviation would only fail later in `git fetch`, which
     # reports an exit status and not a reason, so fail here and say why.
     fake = AsyncMock()
-    fake.query_latest_diff = AsyncMock(
-        return_value=PhabricatorDiff(id=9, base_commit="base9")
-    )
-    fake.get_raw_diff = AsyncMock(return_value="diff --git a/f b/f\n")
-    fake.resolve_commit = AsyncMock(
+    fake.get_patch_stack = AsyncMock(
         side_effect=UnresolvedCommitError("Cannot expand base9: not imported")
     )
 
@@ -68,24 +109,16 @@ def test_patch_route_422_when_base_cannot_be_expanded(caplog):
     assert "Cannot expand base9: not imported" in caplog.text
 
 
-def test_patch_route_404_when_no_diff():
+def test_patch_route_404_when_there_is_no_patch(caplog):
     fake = AsyncMock()
-    fake.query_latest_diff = AsyncMock(return_value=None)
+    fake.get_patch_stack = AsyncMock(side_effect=MissingPatchError("D42 has no diffs"))
 
-    resp = _client(fake).get("/phabricator/revision/42/patch")
-
-    assert resp.status_code == 404
-
-
-def test_patch_route_404_when_no_base_commit():
-    fake = AsyncMock()
-    fake.query_latest_diff = AsyncMock(
-        return_value=PhabricatorDiff(id=9, base_commit=None)
-    )
-
-    resp = _client(fake).get("/phabricator/revision/42/patch")
+    with caplog.at_level("WARNING", logger=broker.log.name):
+        resp = _client(fake).get("/phabricator/revision/42/patch")
 
     assert resp.status_code == 404
+    assert resp.json()["error"] == "D42 has no diffs"
+    assert "D42 has no diffs" in caplog.text
 
 
 def _app() -> Starlette:

libs/hackbot-runtime/hackbot_runtime/changes.py

@@ -73,11 +73,14 @@ def _has_uncommitted(repo: Path) -> bool:
     return bool(_git(repo, "status", "--porcelain").strip())
 
 
-def _wrap_uncommitted(repo: Path) -> bool:
-    """Commit any staged/unstaged/untracked changes into one synthetic commit.
-
-    Returns ``True`` if such a commit was created, ``False`` if the tree was
-    already clean.
+def commit_all(repo: Path, message: str) -> bool:
+    """Commit everything in ``repo``'s tree, untracked files included.
+
+    Returns ``False`` without committing when the tree is already clean (``git
+    commit`` would fail there). Stamped with a fixed identity (as the synthetic
+    commits below are): the checkout is ephemeral and has no git identity
+    configured, and the commit's authorship is throwaway: only its tree is ever
+    used.
     """
     if not _has_uncommitted(repo):
         return False
@@ -91,11 +94,20 @@ def _wrap_uncommitted(repo: Path) -> bool:
         "commit",
         "--no-verify",
         "-m",
-        _WIP_MESSAGE,
+        message,
     )
     return True
 
 
+def _wrap_uncommitted(repo: Path) -> bool:
+    """Commit any staged/unstaged/untracked changes into one synthetic commit.
+
+    Returns ``True`` if such a commit was created, ``False`` if the tree was
+    already clean.
+    """
+    return commit_all(repo, _WIP_MESSAGE)
+
+
 def _commit_metadata(repo: Path, base: str) -> list[dict]:
     """Structured info for each commit in ``base..HEAD`` (oldest first)."""
     fmt = _FIELD_SEP.join(["%H", "%an", "%ae", "%aI", "%s", "%b"]) + _RECORD_SEP
@@ -151,16 +163,19 @@ def _synthetic_commit(repo: Path, base: str) -> str:
     ).strip()
 
 
-def _local_commits_property(repo: Path, node: str, base: str) -> dict:
+def _local_commits_property(repo: Path, node: str, parent: str) -> dict:
     """The git side of moz-phab's ``local:commits`` diff property for ``node``.
 
     Phabricator stores this alongside the diff so ``moz-phab patch`` can
     reconstruct a real local commit from the revision; without it, patching a
     hackbot-created revision fails with "a diff without commit information
     detected". Only the fields knowable from git are set here (author, time,
-    tree, node, parents); the apply-side handler fills in ``summary`` and the
+    tree, parents); the apply-side handler fills in ``summary`` and the
     arc-formatted ``message`` once it has the revision URL, matching moz-phab's
     ``conduit.set_diff_property``.
+
+    ``parent`` is the commit the diff is declared to sit on, which is not always
+    the local commit it was diffed against (see :func:`build_phabricator_diff`).
     """
     fmt = _FIELD_SEP.join(["%an", "%ae", "%at", "%T"])
     out = _git(repo, "show", "-s", f"--format={fmt}", node)
@@ -171,7 +186,7 @@ def _local_commits_property(repo: Path, node: str, base: str) -> dict:
             "authorEmail": author_email,
             "time": int(epoch),
             "commit": node,
-            "parents": [base],
+            "parents": [parent],
             "tree": tree,
         }
     }
@@ -205,7 +220,9 @@ def _ambient_git_identity() -> Iterator[None]:
                     os.environ[key] = prev
 
 
-def build_phabricator_diff(repo: Path, base: str, repo_url: str) -> dict | None:
+def build_phabricator_diff(
+    repo: Path, base: str, repo_url: str, reported_base: str | None = None
+) -> dict | None:
     """Build the artifact for submitting a Phabricator revision.
 
     Returns ``{"diff": <differential.creatediff payload>, "local_commits":
@@ -230,6 +247,13 @@ def build_phabricator_diff(repo: Path, base: str, repo_url: str) -> dict | None:
     the apply-side handler instead, since it's specific to which Phabricator
     instance/environment (staging vs. prod) the diff actually gets submitted
     to, and that shouldn't be baked into an artifact built at agent-run time.
+
+    The diff is always computed against ``base``, the local commit the agent
+    started from, but it is *declared* to sit on ``reported_base`` when given.
+    A run on a stacked revision starts from a commit it recreated locally for
+    an unlanded parent revision: that hash means nothing outside this
+    container, so the revision keeps declaring the base it already recorded
+    (which is what every stacked patch does, hackbot's or moz-phab's).
     """
     try:
         from mozphab.args import parse_args
@@ -261,11 +285,12 @@ def build_phabricator_diff(repo: Path, base: str, repo_url: str) -> dict | None:
     if not changes_payload:
         return None
 
+    declared_base = reported_base or base
     diff_payload = {
         "changes": changes_payload,
         "sourceMachine": repo_url,
         "sourcePath": str(repo),
-        "sourceControlBaseRevision": base,
+        "sourceControlBaseRevision": declared_base,
         "sourceControlPath": "/",
         "sourceControlSystem": "git",
         "branch": "HEAD",
@@ -275,7 +300,7 @@ def build_phabricator_diff(repo: Path, base: str, repo_url: str) -> dict | None:
     }
     return {
         "diff": diff_payload,
-        "local_commits": _local_commits_property(repo, node, base),
+        "local_commits": _local_commits_property(repo, node, declared_base),
     }
 
 

libs/hackbot-runtime/hackbot_runtime/context.py

@@ -79,6 +79,9 @@ class HackbotContext(BaseSettings):
     # prepared. Stays None for agents that never touch source, which is how
     # publish_changes() knows there are no changes to collect.
     _source_base: str | None = PrivateAttr(default=None)
+    # What a submitted Phabricator diff declares as its base, when that differs
+    # from _source_base (see reset_source_base).
+    _reported_base: str | None = PrivateAttr(default=None)
     # The prepared checkout path + the ref it was prepared at, so the source is
     # prepared exactly once and a conflicting re-prepare is caught.
     _repo_path: Path | None = PrivateAttr(default=None)
@@ -143,6 +146,24 @@ async def prepare_repo(
         self._prepared_ref = resolved_ref
         return path
 
+    def reset_source_base(self, reported_base: str | None = None) -> None:
+        """Re-record the change base at the checkout's current HEAD.
+
+        :meth:`prepare_repo` records the commit the agent's changes are later
+        collected against. Call this after committing groundwork the run must
+        not claim as its own (e.g. the unlanded parents of a stacked
+        Phabricator revision), so :meth:`publish_changes` diffs against that
+        commit instead. Unlike the initial recording this is not best-effort:
+        a stale base would silently publish someone else's changes as the
+        run's.
+
+        That new base is a commit this container made up, so a Phabricator diff
+        must not declare it: pass ``reported_base`` to declare a meaningful one
+        (for a stacked revision, the base it already recorded) instead.
+        """
+        self._source_base = changes.base_commit(self.repo_path)
+        self._reported_base = reported_base
+
     @property
     def repo_path(self) -> Path:
         """The prepared source checkout path. Call :meth:`prepare_repo` first."""
@@ -260,7 +281,10 @@ def publish_changes(
         )
         if wants_phabricator:
             diff_payload = changes.build_phabricator_diff(
-                self.repo_path, self._source_base, self._config.source.repo_url
+                self.repo_path,
+                self._source_base,
+                self._config.source.repo_url,
+                reported_base=self._reported_base,
             )
             if diff_payload is not None:
                 self.publish_json(phabricator_diff_key, diff_payload)