From 278f200dbdfbc9c21cdb12df89760e6c977e5462 Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Thu, 6 Aug 2026 23:13:33 -0400 Subject: [PATCH 1/2] Handle stacked Phabricator revision patches --- .taskcluster.yml | 4 + .../bug-fix/hackbot_agents/bug_fix/broker.py | 36 +-- agents/bug-fix/tests/test_broker.py | 73 +++--- .../hackbot_runtime/changes.py | 24 +- .../hackbot_runtime/context.py | 13 ++ .../hackbot_runtime/revision.py | 69 ++++-- libs/hackbot-runtime/tests/test_revision.py | 164 ++++++++++++- .../phabricator_client/__init__.py | 11 +- .../phabricator_client/client.py | 116 +++++++++- .../phabricator_client/models.py | 20 ++ libs/phabricator-client/tests/test_client.py | 218 ++++++++++++++++++ 11 files changed, 668 insertions(+), 80 deletions(-) diff --git a/.taskcluster.yml b/.taskcluster.yml index 24e57c8434..7b030f07c6 100644 --- a/.taskcluster.yml +++ b/.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: diff --git a/agents/bug-fix/hackbot_agents/bug_fix/broker.py b/agents/bug-fix/hackbot_agents/bug_fix/broker.py index d56bf1653c..a57a50c361 100644 --- a/agents/bug-fix/hackbot_agents/bug_fix/broker.py +++ b/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 diff --git a/agents/bug-fix/tests/test_broker.py b/agents/bug-fix/tests/test_broker.py index 2d596d0c16..8499ef3ce4 100644 --- a/agents/bug-fix/tests/test_broker.py +++ b/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,56 @@ 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, 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, "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, raw_diff="parent\n"), + RevisionPatch(revision_id=42, diff_id=9, 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 +91,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: diff --git a/libs/hackbot-runtime/hackbot_runtime/changes.py b/libs/hackbot-runtime/hackbot_runtime/changes.py index 70f2a7953f..e6b16a3771 100644 --- a/libs/hackbot-runtime/hackbot_runtime/changes.py +++ b/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 diff --git a/libs/hackbot-runtime/hackbot_runtime/context.py b/libs/hackbot-runtime/hackbot_runtime/context.py index 044f79e503..28c89bcb63 100644 --- a/libs/hackbot-runtime/hackbot_runtime/context.py +++ b/libs/hackbot-runtime/hackbot_runtime/context.py @@ -143,6 +143,19 @@ async def prepare_repo( self._prepared_ref = resolved_ref return path + def reset_source_base(self) -> 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. + """ + self._source_base = changes.base_commit(self.repo_path) + @property def repo_path(self) -> Path: """The prepared source checkout path. Call :meth:`prepare_repo` first.""" diff --git a/libs/hackbot-runtime/hackbot_runtime/revision.py b/libs/hackbot-runtime/hackbot_runtime/revision.py index 1714e9b865..563bbe942d 100644 --- a/libs/hackbot-runtime/hackbot_runtime/revision.py +++ b/libs/hackbot-runtime/hackbot_runtime/revision.py @@ -6,19 +6,28 @@ The agent holds no credentials, so it does not talk to Conduit itself: it asks a broker sidecar (which holds the Phabricator key) for the revision's base commit + -raw diff over a keyless loopback URL, then checks out that base and applies the -diff locally (``git apply`` needs no key). The broker endpoint contract is -``GET {broker_url}/phabricator/revision/{id}/patch`` -> ``{base_commit, raw_diff}``. +the patches to replay onto it over a keyless loopback URL, then checks out that +base and applies them locally (``git apply`` needs no key). The broker endpoint +contract is ``GET {broker_url}/phabricator/revision/{id}/patch`` -> +``{base_commit, patches: [{revision_id, diff_id, raw_diff}]}``, bottom-first. + +There is more than one patch when the revision is stacked on parent revisions +that have not landed: the commit it was built on then exists only in the +author's repository, so no remote can fetch it and the tree is rebuilt by +replaying the parents' diffs onto the closest base that is fetchable. """ from __future__ import annotations import logging import subprocess +from pathlib import Path from typing import TYPE_CHECKING import httpx +from hackbot_runtime import changes + if TYPE_CHECKING: from hackbot_runtime.context import HackbotContext @@ -34,14 +43,17 @@ async def checkout_revision( ) -> None: """Prepare the source at the revision's base commit and apply its diff. - Fetches the base commit + raw diff from the broker (``broker_url``, a keyless + Fetches the base commit + patches from the broker (``broker_url``, a keyless loopback URL). Raises :class:`RuntimeError` if the broker can't provide the - patch or the diff does not apply cleanly — so the run fails visibly rather - than editing the wrong tree. + patches or one does not apply cleanly, so the run fails visibly rather than + editing the wrong tree. - The diff is left uncommitted, so the run's recorded change base stays at the - revision's base commit and the final submission is the complete, updated - revision (base -> revision + the agent's follow-up edits). + The revision's own diff is left uncommitted, so the run's recorded change + base stays at the revision's base and the final submission is the complete, + updated revision (base -> revision + the agent's follow-up edits). The + diffs of any unlanded parent revisions are committed first and the change + base is moved on top of them: they set the scene for the run, but they + belong to their own revisions and must not reappear in this one's diff. """ url = f"{broker_url.rstrip('/')}/phabricator/revision/{revision_id}/patch" async with httpx.AsyncClient(timeout=_TIMEOUT) as client: @@ -53,21 +65,48 @@ async def checkout_revision( ) payload = response.json() base = payload["base_commit"] - raw_diff = payload["raw_diff"] + patches = payload["patches"] + if not patches: + raise RuntimeError(f"Broker returned no patches for D{revision_id}") - # Prepare the checkout explicitly at the revision's base commit, then apply - # the diff onto the working tree so the tree matches the revision. Must run - # before anything else touches the source (prepare_repo raises otherwise). + # Prepare the checkout explicitly at the base commit, then apply the patches + # onto the working tree so the tree matches the revision. Must run before + # anything else touches the source (prepare_repo raises otherwise). repo = await ctx.prepare_repo(ref=base) log.info("Checking out D%s (base %s) before running the agent", revision_id, base) + *ancestors, revision_patch = patches + for patch in ancestors: + log.info( + "Restoring unlanded parent D%s (diff %s) of D%s", + patch["revision_id"], + patch["diff_id"], + revision_id, + ) + _apply(repo, patch, base) + # Committed with the runtime's own identity: this stands in for a commit + # nobody but the author has, and only its tree matters. + changes.commit_all( + repo, + f"D{patch['revision_id']} diff {patch['diff_id']} " + f"(unlanded parent of D{revision_id})", + ) + if ancestors: + # The parents are now history, not this run's work. + ctx.reset_source_base() + + _apply(repo, revision_patch, base) + + +def _apply(repo: Path, patch: dict, base: str) -> None: + """Apply one revision's raw diff onto the working tree, or raise.""" result = subprocess.run( ["git", "-C", str(repo), "apply"], - input=raw_diff.encode(), + input=patch["raw_diff"].encode(), capture_output=True, ) if result.returncode != 0: raise RuntimeError( - f"Could not apply diff for D{revision_id} onto {base}: " + f"Could not apply diff for D{patch['revision_id']} onto {base}: " f"{result.stderr.decode().strip()}" ) diff --git a/libs/hackbot-runtime/tests/test_revision.py b/libs/hackbot-runtime/tests/test_revision.py index 733b43ab80..0565309948 100644 --- a/libs/hackbot-runtime/tests/test_revision.py +++ b/libs/hackbot-runtime/tests/test_revision.py @@ -1,27 +1,59 @@ """Tests for checking the source tree out at a Phabricator revision.""" +import subprocess from pathlib import Path import httpx import pytest -from hackbot_runtime import revision +from hackbot_runtime import changes, revision BROKER = "http://127.0.0.1:8765" +PARENT_DIFF = """diff --git a/f.txt b/f.txt +--- a/f.txt ++++ b/f.txt +@@ -1 +1,2 @@ + one ++two +""" + +CHILD_DIFF = """diff --git a/f.txt b/f.txt +--- a/f.txt ++++ b/f.txt +@@ -1,2 +1,3 @@ + one + two ++three +""" + + +def _patch(revision_id: int, diff_id: int, raw_diff: str) -> dict: + return {"revision_id": revision_id, "diff_id": diff_id, "raw_diff": raw_diff} + class _FakeCtx: """Stand-in for HackbotContext: records the ref passed to prepare_repo.""" - def __init__(self, repo: Path): + def __init__(self, repo: Path, track_head: bool = False): self._repo = repo + self._track_head = track_head self.prepared_ref = None + self.source_base = None + self.source_base_resets = 0 async def prepare_repo( self, ref: str | None = None, depth: int | None = None ) -> Path: self.prepared_ref = ref + if self._track_head: + self.source_base = changes.base_commit(self._repo) return self._repo + def reset_source_base(self) -> None: + self.source_base_resets += 1 + if self._track_head: + self.source_base = changes.base_commit(self._repo) + def _patch_broker(monkeypatch, *, status=200, payload=None, text=""): """Stub httpx.AsyncClient.get to return a canned broker response.""" @@ -54,21 +86,59 @@ async def get(self, url): def _patch_git(monkeypatch, *, returncode=0, stderr=b""): - calls = {} + calls = [] def _fake_run(cmd, input=None, capture_output=False): - calls["cmd"] = cmd - calls["input"] = input + calls.append({"cmd": cmd, "input": input}) return type("R", (), {"returncode": returncode, "stderr": stderr})() monkeypatch.setattr(revision.subprocess, "run", _fake_run) return calls +@pytest.fixture +def repo(tmp_path: Path) -> Path: + """A one-commit git repo standing in for the prepared checkout.""" + repo = tmp_path / "src" + repo.mkdir() + subprocess.run(["git", "init", "-q", str(repo)], check=True) + (repo / "f.txt").write_text("one\n") + subprocess.run( + ["git", "-C", str(repo), "add", "-A"], check=True, capture_output=True + ) + subprocess.run( + [ + "git", + "-C", + str(repo), + "-c", + "user.name=Test", + "-c", + "user.email=test@example.com", + "commit", + "-q", + "-m", + "base", + ], + check=True, + capture_output=True, + ) + return repo + + +def _git_out(repo: Path, *args: str) -> str: + return subprocess.run( + ["git", "-C", str(repo), *args], check=True, capture_output=True, text=True + ).stdout.strip() + + async def test_checkout_applies_diff_at_base(monkeypatch, tmp_path): http = _patch_broker( monkeypatch, - payload={"base_commit": "base9", "raw_diff": "diff --git a/f b/f\n"}, + payload={ + "base_commit": "base9", + "patches": [_patch(42, 9, "diff --git a/f b/f\n")], + }, ) git = _patch_git(monkeypatch) ctx = _FakeCtx(tmp_path) @@ -77,8 +147,11 @@ async def test_checkout_applies_diff_at_base(monkeypatch, tmp_path): assert http["url"] == f"{BROKER}/phabricator/revision/42/patch" assert ctx.prepared_ref == "base9" - assert git["cmd"][:4] == ["git", "-C", str(tmp_path), "apply"] - assert git["input"] == b"diff --git a/f b/f\n" + assert len(git) == 1 + assert git[0]["cmd"][:4] == ["git", "-C", str(tmp_path), "apply"] + assert git[0]["input"] == b"diff --git a/f b/f\n" + # Nothing was committed, so the change base stays at the revision's base. + assert ctx.source_base_resets == 0 async def test_checkout_raises_on_broker_error(monkeypatch, tmp_path): @@ -88,10 +161,22 @@ async def test_checkout_raises_on_broker_error(monkeypatch, tmp_path): await revision.checkout_revision(ctx, 42, BROKER) +async def test_checkout_raises_when_the_broker_returns_no_patches( + monkeypatch, tmp_path +): + _patch_broker(monkeypatch, payload={"base_commit": "base9", "patches": []}) + ctx = _FakeCtx(tmp_path) + with pytest.raises(RuntimeError, match="no patches for D42"): + await revision.checkout_revision(ctx, 42, BROKER) + + async def test_checkout_raises_when_apply_fails(monkeypatch, tmp_path): _patch_broker( monkeypatch, - payload={"base_commit": "base9", "raw_diff": "diff --git a/f b/f\n"}, + payload={ + "base_commit": "base9", + "patches": [_patch(42, 9, "diff --git a/f b/f\n")], + }, ) _patch_git(monkeypatch, returncode=1, stderr=b"patch does not apply") ctx = _FakeCtx(tmp_path) @@ -99,6 +184,67 @@ async def test_checkout_raises_when_apply_fails(monkeypatch, tmp_path): await revision.checkout_revision(ctx, 42, BROKER) +async def test_checkout_names_the_stacked_revision_that_fails_to_apply( + monkeypatch, tmp_path +): + # The failing patch is the parent's, so the error must not blame D42. + _patch_broker( + monkeypatch, + payload={ + "base_commit": "base9", + "patches": [_patch(41, 8, "parent\n"), _patch(42, 9, "child\n")], + }, + ) + _patch_git(monkeypatch, returncode=1, stderr=b"patch does not apply") + ctx = _FakeCtx(tmp_path) + with pytest.raises(RuntimeError, match="Could not apply diff for D41"): + await revision.checkout_revision(ctx, 42, BROKER) + + +async def test_checkout_rebuilds_a_stack_onto_the_fetchable_base(monkeypatch, repo): + # D42 is stacked on the unlanded D41: both diffs are replayed, the parent's + # as a commit and D42's own left in the working tree. + _patch_broker( + monkeypatch, + payload={ + "base_commit": "base9", + "patches": [_patch(41, 8, PARENT_DIFF), _patch(42, 9, CHILD_DIFF)], + }, + ) + ctx = _FakeCtx(repo, track_head=True) + base = _git_out(repo, "rev-parse", "HEAD") + + await revision.checkout_revision(ctx, 42, BROKER) + + # The tree is the revision's: base + parent + child. + assert (repo / "f.txt").read_text() == "one\ntwo\nthree\n" + # The parent is history the run inherits, so the change base moved onto it + # and only D42's own diff is left uncommitted for the agent to build on. + assert ctx.source_base_resets == 1 + assert ctx.source_base == _git_out(repo, "rev-parse", "HEAD") != base + assert _git_out(repo, "show", "HEAD:f.txt") == "one\ntwo" + assert _git_out(repo, "log", "-1", "--format=%s") == ( + "D41 diff 8 (unlanded parent of D42)" + ) + # Unstaged, i.e. D42's diff is the run's starting point, not yet a commit. + assert _git_out(repo, "status", "--porcelain") == "M f.txt" + + +async def test_checkout_leaves_an_unstacked_revision_uncommitted(monkeypatch, repo): + _patch_broker( + monkeypatch, + payload={"base_commit": "base9", "patches": [_patch(42, 9, PARENT_DIFF)]}, + ) + ctx = _FakeCtx(repo, track_head=True) + base = _git_out(repo, "rev-parse", "HEAD") + + await revision.checkout_revision(ctx, 42, BROKER) + + assert (repo / "f.txt").read_text() == "one\ntwo\n" + assert _git_out(repo, "rev-parse", "HEAD") == base + assert ctx.source_base_resets == 0 + + def test_revision_uses_httpx(): # Guard against reintroducing an in-agent Conduit client (which needs a key). assert revision.httpx is httpx diff --git a/libs/phabricator-client/phabricator_client/__init__.py b/libs/phabricator-client/phabricator_client/__init__.py index ffacddc9e6..a11470471d 100644 --- a/libs/phabricator-client/phabricator_client/__init__.py +++ b/libs/phabricator-client/phabricator_client/__init__.py @@ -1,10 +1,17 @@ -from phabricator_client.client import PhabricatorClient, UnresolvedCommitError +from phabricator_client.client import ( + MissingPatchError, + PhabricatorClient, + UnresolvedCommitError, +) from phabricator_client.config import PhabricatorSettings -from phabricator_client.models import PhabricatorDiff +from phabricator_client.models import PatchStack, PhabricatorDiff, RevisionPatch __all__ = [ + "MissingPatchError", + "PatchStack", "PhabricatorClient", "PhabricatorDiff", "PhabricatorSettings", + "RevisionPatch", "UnresolvedCommitError", ] diff --git a/libs/phabricator-client/phabricator_client/client.py b/libs/phabricator-client/phabricator_client/client.py index 6ccd9537fe..83dedf4507 100644 --- a/libs/phabricator-client/phabricator_client/client.py +++ b/libs/phabricator-client/phabricator_client/client.py @@ -20,15 +20,24 @@ import httpx from phabricator_client.config import PhabricatorSettings -from phabricator_client.models import PhabricatorDiff +from phabricator_client.models import PatchStack, PhabricatorDiff, RevisionPatch _FULL_COMMIT_LEN = 40 +# How far down a stack :meth:`PhabricatorClient.get_patch_stack` walks looking +# for a fetchable base. Deep stacks exist, but a walk that long more likely +# means every base is unresolvable, and failing beats collecting diffs forever. +_MAX_STACK_DEPTH = 20 + class UnresolvedCommitError(Exception): """A commit identifier could not be expanded to a full, fetchable hash.""" +class MissingPatchError(Exception): + """A revision has no diff to check out.""" + + def _is_full_commit(ref: str) -> bool: """True if ``ref`` is a full 40-char lowercase-hex git commit hash.""" ref = ref.lower() @@ -193,3 +202,108 @@ async def resolve_commit(self, ref: str) -> str: ) return identifiers.pop() + + async def get_parent_revision_ids(self, revision_id: int) -> list[int]: + """The ids of the revisions ``D`` is stacked on top of. + + Phabricator records a stack as ``revision.parent`` edges between + revisions, so ``edge.search`` names the parents by PHID and a revision + search turns those back into ids. Empty for a standalone revision or + the bottom of a stack. + """ + revision = await self.search_revision_by_id(revision_id) + if revision is None: + return [] + result = await self.conduit_request( + "edge.search", + sourcePHIDs=[revision["phid"]], + types=["revision.parent"], + ) + parent_phids = [ + edge["destinationPHID"] + for edge in result.get("data") or [] + if edge.get("destinationPHID") + ] + if not parent_phids: + return [] + result = await self.conduit_request( + "differential.revision.search", constraints={"phids": parent_phids} + ) + return [parent["id"] for parent in result.get("data") or []] + + async def get_patch_stack(self, revision_id: int) -> PatchStack: + """A fetchable base commit plus the patches that rebuild a revision. + + Usually that is the revision's own diff on the commit it was built on. + A stacked revision is built on its parent revision's commit, which only + exists in the author's local repository: no remote can fetch it, so the + tree cannot be checked out there. In that case, walk down the stack + collecting each ancestor's latest diff until a revision whose base does + resolve, and let the caller replay the collected diffs onto it. + + Raises :class:`MissingPatchError` when a revision on the way down has + nothing to apply, and :class:`UnresolvedCommitError` when the walk runs + out of stack (or of parents to choose between) before finding a base. + """ + patches: list[RevisionPatch] = [] + visited: set[int] = set() + current = revision_id + while True: + visited.add(current) + diff = await self.query_latest_diff(current) + if diff is None: + raise MissingPatchError(f"D{current} has no diffs") + if not diff.base_commit: + raise MissingPatchError(f"D{current} diff {diff.id} has no base commit") + patches.insert( + 0, + RevisionPatch( + revision_id=current, + diff_id=diff.id, + raw_diff=await self.get_raw_diff(diff.id), + ), + ) + try: + base_commit = await self.resolve_commit(diff.base_commit) + except UnresolvedCommitError as error: + current = self._next_in_stack( + current, + await self.get_parent_revision_ids(current), + visited, + error, + ) + continue + return PatchStack(base_commit=base_commit, patches=patches) + + @staticmethod + def _next_in_stack( + revision_id: int, + parent_ids: list[int], + visited: set[int], + error: UnresolvedCommitError, + ) -> int: + """The single unvisited parent to continue the walk down a stack with. + + Anything else (no parent, a fork with several parents, a stack deeper + than :data:`_MAX_STACK_DEPTH`) leaves no one series of patches to + rebuild, so re-raise the unresolved base that started the walk with the + reason the walk stopped. + """ + candidates = [parent_id for parent_id in parent_ids if parent_id not in visited] + if len(candidates) == 1 and len(visited) < _MAX_STACK_DEPTH: + return candidates[0] + + if not parent_ids: + reason = f"D{revision_id} has no parent revision to fall back on" + elif not candidates: + reason = f"D{revision_id}'s parent revisions are already in the stack" + elif len(candidates) > 1: + reason = ( + f"D{revision_id} has {len(candidates)} parent revisions, so the " + "patches to apply are ambiguous" + ) + else: + reason = ( + f"gave up after walking {_MAX_STACK_DEPTH} revisions down the stack" + ) + raise UnresolvedCommitError(f"{error} Cannot rebuild the tree: {reason}.") diff --git a/libs/phabricator-client/phabricator_client/models.py b/libs/phabricator-client/phabricator_client/models.py index 59f03fb739..e18b52768c 100644 --- a/libs/phabricator-client/phabricator_client/models.py +++ b/libs/phabricator-client/phabricator_client/models.py @@ -16,3 +16,23 @@ class PhabricatorDiff(BaseModel): id: int base_commit: str | None = Field(default=None, alias="sourceControlBaseRevision") + + +class RevisionPatch(BaseModel): + """One revision's diff, as raw unified-diff text.""" + + revision_id: int + diff_id: int + raw_diff: str + + +class PatchStack(BaseModel): + """A fetchable base commit plus the patches that rebuild a revision's tree. + + ``patches`` is ordered bottom-first: apply them in order onto + ``base_commit`` and the last one is the requested revision. A revision that + is not stacked (or whose ancestors have all landed) yields a single patch. + """ + + base_commit: str + patches: list[RevisionPatch] diff --git a/libs/phabricator-client/tests/test_client.py b/libs/phabricator-client/tests/test_client.py index 1731ae7408..f5e9a6f42c 100644 --- a/libs/phabricator-client/tests/test_client.py +++ b/libs/phabricator-client/tests/test_client.py @@ -5,6 +5,7 @@ import httpx import pytest from phabricator_client import ( + MissingPatchError, PhabricatorClient, PhabricatorSettings, UnresolvedCommitError, @@ -54,6 +55,40 @@ async def post(self, url, data=None): return captured +def _route_posts(monkeypatch, handlers: dict) -> list[tuple[str, dict]]: + """Stub httpx so each Conduit method is answered from ``handlers``. + + A handler is the method's ``result`` payload, a callable taking the request + params, or a list serving one payload per call, enough to script the + several calls a walk down a stack makes. Returns the (method, params) log. + """ + calls: list[tuple[str, dict]] = [] + + class _FakeAsyncClient: + def __init__(self, timeout=None): + pass + + async def __aenter__(self): + return self + + async def __aexit__(self, *exc): + return False + + async def post(self, url, data=None): + method = url.rsplit("/api/", 1)[1] + params = json.loads(data["params"]) + calls.append((method, params)) + handler = handlers[method] + if isinstance(handler, list): + handler = handler.pop(0) + if callable(handler): + handler = handler(params) + return _FakeResponse({"result": handler}) + + monkeypatch.setattr(client_module.httpx, "AsyncClient", _FakeAsyncClient) + return calls + + async def test_conduit_request_returns_result(monkeypatch): captured = _capture_post(monkeypatch, {"result": {"data": [1, 2]}}) result = await _client().conduit_request("some.method", foo="bar") @@ -380,6 +415,189 @@ async def test_resolve_commit_raises_when_unresolved(monkeypatch): assert "may not be imported" in message +LANDED = "0397cc0f5dcabc6f44e0f742107ff3695882d5e4" +# What a stacked revision records as its base: the author's local commit for the +# parent revision, which no repository has. +UNLANDED = "69706d7a081e" + + +def _diffs(base_commit: str, diff_id: int) -> dict: + """A ``differential.querydiffs`` result holding one diff.""" + return { + str(diff_id): {"id": str(diff_id), "sourceControlBaseRevision": base_commit} + } + + +def _revisions(*ids: int) -> dict: + """A ``differential.revision.search`` result naming revisions by id.""" + return {"data": [{"id": id_, "phid": f"PHID-DREV-{id_}"} for id_ in ids]} + + +def _parents(*ids: int) -> dict: + """An ``edge.search`` result pointing at parent revisions.""" + return {"data": [{"destinationPHID": f"PHID-DREV-{id_}"} for id_ in ids]} + + +async def test_get_parent_revision_ids(monkeypatch): + calls = _route_posts( + monkeypatch, + { + "differential.revision.search": [_revisions(42), _revisions(40, 41)], + "edge.search": _parents(40, 41), + }, + ) + assert await _client().get_parent_revision_ids(42) == [40, 41] + edge_params = dict(calls)["edge.search"] + assert edge_params["sourcePHIDs"] == ["PHID-DREV-42"] + assert edge_params["types"] == ["revision.parent"] + + +async def test_get_parent_revision_ids_empty_at_the_bottom_of_a_stack(monkeypatch): + calls = _route_posts( + monkeypatch, + {"differential.revision.search": _revisions(42), "edge.search": _parents()}, + ) + assert await _client().get_parent_revision_ids(42) == [] + # No parent PHIDs to look up, so no second revision search. + assert [method for method, _ in calls].count("differential.revision.search") == 1 + + +async def test_get_patch_stack_returns_one_patch_on_a_fetchable_base(monkeypatch): + _route_posts( + monkeypatch, + { + "differential.querydiffs": _diffs(LANDED, 9), + "differential.getrawdiff": "child\n", + }, + ) + stack = await _client().get_patch_stack(42) + assert stack.base_commit == LANDED + assert [(p.revision_id, p.diff_id, p.raw_diff) for p in stack.patches] == [ + (42, 9, "child\n") + ] + + +async def test_get_patch_stack_walks_down_to_a_fetchable_base(monkeypatch): + # D42 sits on D41, which has not landed: D42's base commit is unknown to + # Diffusion, so the tree is rebuilt from D41's base by applying both diffs. + _route_posts( + monkeypatch, + { + "differential.querydiffs": [_diffs(UNLANDED, 9), _diffs(LANDED, 8)], + "differential.getrawdiff": lambda params: ( + "child\n" if params["diffID"] == 9 else "parent\n" + ), + "diffusion.querycommits": {"identifierMap": {}, "data": {}}, + "differential.revision.search": [_revisions(42), _revisions(41)], + "edge.search": _parents(41), + }, + ) + stack = await _client().get_patch_stack(42) + assert stack.base_commit == LANDED + # Bottom-first: the parent's diff has to be applied before the child's. + assert [(p.revision_id, p.diff_id, p.raw_diff) for p in stack.patches] == [ + (41, 8, "parent\n"), + (42, 9, "child\n"), + ] + + +async def test_get_patch_stack_raises_when_the_bottom_base_is_unknown(monkeypatch): + _route_posts( + monkeypatch, + { + "differential.querydiffs": _diffs(UNLANDED, 9), + "differential.getrawdiff": "child\n", + "diffusion.querycommits": {"identifierMap": {}, "data": {}}, + "differential.revision.search": _revisions(42), + "edge.search": _parents(), + }, + ) + with pytest.raises(UnresolvedCommitError) as excinfo: + await _client().get_patch_stack(42) + # Both halves of the story: the base could not be expanded, and there was + # no parent revision to rebuild it from. + assert UNLANDED in str(excinfo.value) + assert "D42 has no parent revision" in str(excinfo.value) + + +async def test_get_patch_stack_raises_when_a_revision_has_several_parents(monkeypatch): + _route_posts( + monkeypatch, + { + "differential.querydiffs": _diffs(UNLANDED, 9), + "differential.getrawdiff": "child\n", + "diffusion.querycommits": {"identifierMap": {}, "data": {}}, + "differential.revision.search": [_revisions(42), _revisions(40, 41)], + "edge.search": _parents(40, 41), + }, + ) + with pytest.raises(UnresolvedCommitError, match="2 parent revisions"): + await _client().get_patch_stack(42) + + +async def test_get_patch_stack_gives_up_on_an_endless_stack(monkeypatch): + # Every revision claims an unlanded base and one more parent below it. + _route_posts( + monkeypatch, + { + "differential.querydiffs": lambda params: _diffs( + UNLANDED, params["revisionIDs"][0] + ), + "differential.getrawdiff": lambda params: f"diff {params['diffID']}\n", + "diffusion.querycommits": {"identifierMap": {}, "data": {}}, + "differential.revision.search": lambda params: _revisions( + *( + params["constraints"]["ids"] + if "ids" in params["constraints"] + else [ + int(phid.rsplit("-", 1)[1]) + for phid in params["constraints"]["phids"] + ] + ) + ), + "edge.search": lambda params: _parents( + int(params["sourcePHIDs"][0].rsplit("-", 1)[1]) - 1 + ), + }, + ) + with pytest.raises(UnresolvedCommitError, match="gave up after walking"): + await _client().get_patch_stack(1000) + + +async def test_get_patch_stack_stops_on_a_parent_cycle(monkeypatch): + _route_posts( + monkeypatch, + { + "differential.querydiffs": lambda params: _diffs( + UNLANDED, params["revisionIDs"][0] + ), + "differential.getrawdiff": "diff\n", + "diffusion.querycommits": {"identifierMap": {}, "data": {}}, + "differential.revision.search": [ + _revisions(42), + _revisions(41), + _revisions(41), + _revisions(42), + ], + "edge.search": [_parents(41), _parents(42)], + }, + ) + with pytest.raises(UnresolvedCommitError, match="already in the stack"): + await _client().get_patch_stack(42) + + +async def test_get_patch_stack_raises_when_a_revision_has_no_diff(monkeypatch): + _route_posts(monkeypatch, {"differential.querydiffs": {}}) + with pytest.raises(MissingPatchError, match="D42 has no diffs"): + await _client().get_patch_stack(42) + + +async def test_get_patch_stack_raises_when_a_diff_has_no_base_commit(monkeypatch): + _route_posts(monkeypatch, {"differential.querydiffs": {"9": {"id": "9"}}}) + with pytest.raises(MissingPatchError, match="D42 diff 9 has no base commit"): + await _client().get_patch_stack(42) + + def test_revision_url_default_base(): assert _client().revision_url(42) == "https://phabricator.services.mozilla.com/D42" From ea9ba319dce2f7440294418d3dd789e33da93b05 Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Fri, 7 Aug 2026 11:30:41 -0400 Subject: [PATCH 2/2] Preserve recorded base for stacked revisions --- agents/bug-fix/tests/test_broker.py | 26 ++++++++++++--- .../hackbot_runtime/changes.py | 25 ++++++++++---- .../hackbot_runtime/context.py | 15 +++++++-- .../hackbot_runtime/revision.py | 11 +++++-- libs/hackbot-runtime/tests/test_changes.py | 22 +++++++++++++ libs/hackbot-runtime/tests/test_context.py | 33 +++++++++++++++++-- libs/hackbot-runtime/tests/test_revision.py | 23 ++++++++++--- .../phabricator_client/client.py | 1 + .../phabricator_client/models.py | 9 ++++- libs/phabricator-client/tests/test_client.py | 9 +++-- 10 files changed, 149 insertions(+), 25 deletions(-) diff --git a/agents/bug-fix/tests/test_broker.py b/agents/bug-fix/tests/test_broker.py index 8499ef3ce4..30caca090e 100644 --- a/agents/bug-fix/tests/test_broker.py +++ b/agents/bug-fix/tests/test_broker.py @@ -33,7 +33,10 @@ def test_patch_route_returns_base_and_patches(): base_commit="base9full", patches=[ RevisionPatch( - revision_id=42, diff_id=9, raw_diff="diff --git a/f b/f\n" + revision_id=42, + diff_id=9, + base_commit="base9", + raw_diff="diff --git a/f b/f\n", ) ], ) @@ -45,7 +48,12 @@ def test_patch_route_returns_base_and_patches(): assert resp.json() == { "base_commit": "base9full", "patches": [ - {"revision_id": 42, "diff_id": 9, "raw_diff": "diff --git a/f b/f\n"} + { + "revision_id": 42, + "diff_id": 9, + "base_commit": "base9", + "raw_diff": "diff --git a/f b/f\n", + } ], } fake.get_patch_stack.assert_awaited_once_with(42) @@ -59,8 +67,18 @@ def test_patch_route_serves_a_stack_bottom_first(): return_value=PatchStack( base_commit="landed", patches=[ - RevisionPatch(revision_id=41, diff_id=8, raw_diff="parent\n"), - RevisionPatch(revision_id=42, diff_id=9, raw_diff="child\n"), + 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", + ), ], ) ) diff --git a/libs/hackbot-runtime/hackbot_runtime/changes.py b/libs/hackbot-runtime/hackbot_runtime/changes.py index e6b16a3771..baf3f90e7d 100644 --- a/libs/hackbot-runtime/hackbot_runtime/changes.py +++ b/libs/hackbot-runtime/hackbot_runtime/changes.py @@ -163,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) @@ -183,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, } } @@ -217,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": , "local_commits": @@ -242,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 @@ -273,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", @@ -287,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), } diff --git a/libs/hackbot-runtime/hackbot_runtime/context.py b/libs/hackbot-runtime/hackbot_runtime/context.py index 28c89bcb63..023f0c369d 100644 --- a/libs/hackbot-runtime/hackbot_runtime/context.py +++ b/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,7 +146,7 @@ async def prepare_repo( self._prepared_ref = resolved_ref return path - def reset_source_base(self) -> None: + 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 @@ -153,8 +156,13 @@ def reset_source_base(self) -> None: 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: @@ -273,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) diff --git a/libs/hackbot-runtime/hackbot_runtime/revision.py b/libs/hackbot-runtime/hackbot_runtime/revision.py index 563bbe942d..f3c7589d6c 100644 --- a/libs/hackbot-runtime/hackbot_runtime/revision.py +++ b/libs/hackbot-runtime/hackbot_runtime/revision.py @@ -9,7 +9,8 @@ the patches to replay onto it over a keyless loopback URL, then checks out that base and applies them locally (``git apply`` needs no key). The broker endpoint contract is ``GET {broker_url}/phabricator/revision/{id}/patch`` -> -``{base_commit, patches: [{revision_id, diff_id, raw_diff}]}``, bottom-first. +``{base_commit, patches: [{revision_id, diff_id, base_commit, raw_diff}]}``, +bottom-first. There is more than one patch when the revision is stacked on parent revisions that have not landed: the commit it was built on then exists only in the @@ -92,8 +93,12 @@ async def checkout_revision( f"(unlanded parent of D{revision_id})", ) if ancestors: - # The parents are now history, not this run's work. - ctx.reset_source_base() + # The parents are now history, not this run's work. The commit they end + # at is local to this container, so an updated diff keeps declaring the + # base D already recorded: nothing was rebased, and a + # made-up hash would strand the next run (and `moz-phab patch`) on a + # commit no repository has. + ctx.reset_source_base(reported_base=revision_patch["base_commit"]) _apply(repo, revision_patch, base) diff --git a/libs/hackbot-runtime/tests/test_changes.py b/libs/hackbot-runtime/tests/test_changes.py index b9624be372..a9a23b5f75 100644 --- a/libs/hackbot-runtime/tests/test_changes.py +++ b/libs/hackbot-runtime/tests/test_changes.py @@ -119,6 +119,28 @@ def test_build_phabricator_diff_with_real_change(tmp_path): assert "message" not in entry +def test_build_phabricator_diff_declares_the_reported_base(tmp_path): + # A stacked run diffs against a commit it recreated locally for an unlanded + # parent revision. That hash means nothing to Phabricator, so the diff is + # declared to sit on the base the revision itself recorded. + base = _init_repo(tmp_path) + _commit_change(tmp_path, "line1\nline2 modified\nline3\n") + + result = build_phabricator_diff( + tmp_path, + base, + "https://example.com/repo.git", + reported_base="69706d7a081e", + ) + + assert result is not None + assert result["diff"]["sourceControlBaseRevision"] == "69706d7a081e" + # The diff content is still what changed since the local base. + assert len(result["diff"]["changes"]) == 1 + entry = next(iter(result["local_commits"].values())) + assert entry["parents"] == ["69706d7a081e"] + + def test_build_phabricator_diff_without_arcconfig_returns_none(tmp_path): base = _init_repo(tmp_path, with_arcconfig=False) _commit_change(tmp_path, "line1\nline2 modified\nline3\n") diff --git a/libs/hackbot-runtime/tests/test_context.py b/libs/hackbot-runtime/tests/test_context.py index a6b87ace4a..207c084ba6 100644 --- a/libs/hackbot-runtime/tests/test_context.py +++ b/libs/hackbot-runtime/tests/test_context.py @@ -171,8 +171,8 @@ def test_publish_changes_builds_phabricator_diff_when_action_recorded( hb = _hb_with_source(tmp_path, monkeypatch) monkeypatch.setattr( "hackbot_runtime.context.changes.build_phabricator_diff", - lambda repo, base, repo_url: { - "diff": {"changes": [], "sourceControlBaseRevision": base}, + lambda repo, base, repo_url, reported_base=None: { + "diff": {"changes": [], "sourceControlBaseRevision": reported_base or base}, "local_commits": {"node": {"author": "A"}}, }, ) @@ -190,6 +190,35 @@ def test_publish_changes_builds_phabricator_diff_when_action_recorded( assert submission["local_commits"]["node"]["author"] == "A" +def test_publish_changes_declares_the_reported_base(tmp_path, monkeypatch): + # After a stacked checkout the change base is a commit made up in this + # container, so the submitted diff declares the revision's own base instead. + hb = _hb_with_source(tmp_path, monkeypatch) + monkeypatch.setattr( + "hackbot_runtime.context.changes.base_commit", lambda repo: "localparent" + ) + monkeypatch.setattr( + "hackbot_runtime.context.changes.build_phabricator_diff", + lambda repo, base, repo_url, reported_base=None: { + "diff": {"base": base, "sourceControlBaseRevision": reported_base or base}, + "local_commits": {}, + }, + ) + hb.reset_source_base(reported_base="69706d7a081e") + hb.actions.record("phabricator.update_patch", {"revision_id": 42}, reasoning="r") + + hb.publish_changes() + + submission = json.loads( + ( + tmp_path / "artifacts" / "local-test" / "changes" / "phabricator_diff.json" + ).read_text() + ) + # Diffed against the local commit, declared on the recorded one. + assert submission["diff"]["base"] == "localparent" + assert submission["diff"]["sourceControlBaseRevision"] == "69706d7a081e" + + def test_publish_changes_skips_phabricator_diff_without_action(tmp_path, monkeypatch): hb = _hb_with_source(tmp_path, monkeypatch) called = [] diff --git a/libs/hackbot-runtime/tests/test_revision.py b/libs/hackbot-runtime/tests/test_revision.py index 0565309948..742e44eec0 100644 --- a/libs/hackbot-runtime/tests/test_revision.py +++ b/libs/hackbot-runtime/tests/test_revision.py @@ -27,8 +27,15 @@ """ -def _patch(revision_id: int, diff_id: int, raw_diff: str) -> dict: - return {"revision_id": revision_id, "diff_id": diff_id, "raw_diff": raw_diff} +def _patch( + revision_id: int, diff_id: int, raw_diff: str, base_commit: str = "recorded-base" +) -> dict: + return { + "revision_id": revision_id, + "diff_id": diff_id, + "base_commit": base_commit, + "raw_diff": raw_diff, + } class _FakeCtx: @@ -40,6 +47,7 @@ def __init__(self, repo: Path, track_head: bool = False): self.prepared_ref = None self.source_base = None self.source_base_resets = 0 + self.reported_base = None async def prepare_repo( self, ref: str | None = None, depth: int | None = None @@ -49,8 +57,9 @@ async def prepare_repo( self.source_base = changes.base_commit(self._repo) return self._repo - def reset_source_base(self) -> None: + def reset_source_base(self, reported_base: str | None = None) -> None: self.source_base_resets += 1 + self.reported_base = reported_base if self._track_head: self.source_base = changes.base_commit(self._repo) @@ -208,7 +217,10 @@ async def test_checkout_rebuilds_a_stack_onto_the_fetchable_base(monkeypatch, re monkeypatch, payload={ "base_commit": "base9", - "patches": [_patch(41, 8, PARENT_DIFF), _patch(42, 9, CHILD_DIFF)], + "patches": [ + _patch(41, 8, PARENT_DIFF, base_commit="landed-base"), + _patch(42, 9, CHILD_DIFF, base_commit="69706d7a081e"), + ], }, ) ctx = _FakeCtx(repo, track_head=True) @@ -222,6 +234,9 @@ async def test_checkout_rebuilds_a_stack_onto_the_fetchable_base(monkeypatch, re # and only D42's own diff is left uncommitted for the agent to build on. assert ctx.source_base_resets == 1 assert ctx.source_base == _git_out(repo, "rev-parse", "HEAD") != base + # An updated diff still declares the base D42 recorded: the commit the tree + # was rebuilt at is local to this container, and nothing was rebased. + assert ctx.reported_base == "69706d7a081e" assert _git_out(repo, "show", "HEAD:f.txt") == "one\ntwo" assert _git_out(repo, "log", "-1", "--format=%s") == ( "D41 diff 8 (unlanded parent of D42)" diff --git a/libs/phabricator-client/phabricator_client/client.py b/libs/phabricator-client/phabricator_client/client.py index 83dedf4507..1013e85e70 100644 --- a/libs/phabricator-client/phabricator_client/client.py +++ b/libs/phabricator-client/phabricator_client/client.py @@ -260,6 +260,7 @@ async def get_patch_stack(self, revision_id: int) -> PatchStack: RevisionPatch( revision_id=current, diff_id=diff.id, + base_commit=diff.base_commit, raw_diff=await self.get_raw_diff(diff.id), ), ) diff --git a/libs/phabricator-client/phabricator_client/models.py b/libs/phabricator-client/phabricator_client/models.py index e18b52768c..24bc28e102 100644 --- a/libs/phabricator-client/phabricator_client/models.py +++ b/libs/phabricator-client/phabricator_client/models.py @@ -19,10 +19,17 @@ class PhabricatorDiff(BaseModel): class RevisionPatch(BaseModel): - """One revision's diff, as raw unified-diff text.""" + """One revision's diff, as raw unified-diff text. + + ``base_commit`` is what the revision itself recorded as the commit it was + built on, unexpanded and possibly unfetchable (see :class:`PhabricatorDiff`). + Keep it so an updated diff can declare the base the revision already had, + rather than whatever local commit the tree was rebuilt from. + """ revision_id: int diff_id: int + base_commit: str raw_diff: str diff --git a/libs/phabricator-client/tests/test_client.py b/libs/phabricator-client/tests/test_client.py index f5e9a6f42c..9afd3a1fdd 100644 --- a/libs/phabricator-client/tests/test_client.py +++ b/libs/phabricator-client/tests/test_client.py @@ -472,9 +472,9 @@ async def test_get_patch_stack_returns_one_patch_on_a_fetchable_base(monkeypatch ) stack = await _client().get_patch_stack(42) assert stack.base_commit == LANDED - assert [(p.revision_id, p.diff_id, p.raw_diff) for p in stack.patches] == [ - (42, 9, "child\n") - ] + assert [ + (p.revision_id, p.diff_id, p.base_commit, p.raw_diff) for p in stack.patches + ] == [(42, 9, LANDED, "child\n")] async def test_get_patch_stack_walks_down_to_a_fetchable_base(monkeypatch): @@ -499,6 +499,9 @@ async def test_get_patch_stack_walks_down_to_a_fetchable_base(monkeypatch): (41, 8, "parent\n"), (42, 9, "child\n"), ] + # Each patch keeps the base its own revision recorded, so an updated diff + # can be declared to sit where the revision already said it did. + assert [p.base_commit for p in stack.patches] == [LANDED, UNLANDED] async def test_get_patch_stack_raises_when_the_bottom_base_is_unknown(monkeypatch):