From 3a2a2a667d2e13878e60529322fcab5929fe9fae Mon Sep 17 00:00:00 2001 From: Ollie Hensman-Crook Date: Fri, 11 Sep 2026 17:28:53 +0100 Subject: [PATCH] Use first public parent for unresolvable Phabricator bases (#6792) --- .../hackbot_runtime/revision.py | 20 ++++++++++++++-- libs/hackbot-runtime/tests/test_revision.py | 24 +++++++++++++++++++ .../phabricator_client/models.py | 11 +++++++++ libs/phabricator-client/tests/test_client.py | 22 +++++++++++++++++ 4 files changed, 75 insertions(+), 2 deletions(-) diff --git a/libs/hackbot-runtime/hackbot_runtime/revision.py b/libs/hackbot-runtime/hackbot_runtime/revision.py index f7391ecdb1..4854ffd24a 100644 --- a/libs/hackbot-runtime/hackbot_runtime/revision.py +++ b/libs/hackbot-runtime/hackbot_runtime/revision.py @@ -7,7 +7,11 @@ from pathlib import Path from typing import TYPE_CHECKING, NamedTuple -from phabricator_client import PhabricatorClient, PhabricatorSettings +from phabricator_client import ( + PhabricatorClient, + PhabricatorSettings, + UnresolvedCommitError, +) from hackbot_runtime import changes @@ -131,7 +135,19 @@ async def _resolve_stack(client: PhabricatorClient, revision_id: int) -> Stack: ) # Expand it: moz-phab records an abbreviated hash for a repo the # size of firefox, and git can only fetch a full object id. - base = await client.resolve_commit(diff.base_commit) + try: + base = await client.resolve_commit(diff.base_commit) + except UnresolvedCommitError: + if not diff.first_public_parent: + raise + log.info( + "D%s base %s is not imported; using first public parent %s.", + revision["id"], + diff.base_commit, + diff.first_public_parent, + ) + base = await client.resolve_commit(diff.first_public_parent) + patches.append( Patch( revision_id=revision["id"], diff --git a/libs/hackbot-runtime/tests/test_revision.py b/libs/hackbot-runtime/tests/test_revision.py index ded1054e91..67bfb4d388 100644 --- a/libs/hackbot-runtime/tests/test_revision.py +++ b/libs/hackbot-runtime/tests/test_revision.py @@ -70,6 +70,7 @@ def _fake_conduit( revisions: dict[int, dict], *, base: str = BASE, + first_public_parent: str | None = None, querycommits: dict | None = None, raw_diffs: dict[int, str] | None = None, with_authors: bool = True, @@ -98,6 +99,12 @@ def _result(method: str, params: dict): if rev_id not in revisions: return {} diff = {"id": rev_id * 10, "sourceControlBaseRevision": base} + if first_public_parent: + diff["properties"] = { + "local:commits": { + "local-node": {"firstPublicParent": first_public_parent} + } + } if with_authors: diff["authorName"] = f"Author {rev_id}" diff["authorEmail"] = f"author{rev_id}@example.com" @@ -245,6 +252,23 @@ async def test_an_unresolvable_base_is_reported(monkeypatch): ) +async def test_first_public_parent_is_used_when_base_is_unlanded(monkeypatch): + short = "69706d7a081e" + public_parent = "2" * 40 + revisions = _with_stack_graph({42: _revision(42)}, {42: []}) + + stack, _ = await _stack_of( + monkeypatch, + revisions, + 42, + base=short, + first_public_parent=public_parent, + querycommits={"identifierMap": {}, "data": {}}, + ) + + assert stack.base_commit == public_parent + + async def test_a_missing_revision_is_reported(monkeypatch): with pytest.raises(RuntimeError, match="D42 not found"): await _stack_of(monkeypatch, {}, 42) diff --git a/libs/phabricator-client/phabricator_client/models.py b/libs/phabricator-client/phabricator_client/models.py index 25e1ae8795..dec98e3446 100644 --- a/libs/phabricator-client/phabricator_client/models.py +++ b/libs/phabricator-client/phabricator_client/models.py @@ -25,6 +25,7 @@ class PhabricatorDiff(BaseModel): base_commit: Annotated[str | None, Field(alias="sourceControlBaseRevision")] = None author_name: Annotated[str | None, Field(alias="authorName")] = None author_email: Annotated[str | None, Field(alias="authorEmail")] = None + properties: dict = Field(default_factory=dict) @property def author(self) -> str | None: @@ -32,3 +33,13 @@ def author(self) -> str | None: if self.author_name and self.author_email: return f"{self.author_name} <{self.author_email}>" return None + + @property + def first_public_parent(self) -> str | None: + """The first landed parent recorded by mozphab, if present.""" + local_commits = self.properties.get("local:commits") or {} + for commit in local_commits.values(): + first_public_parent = commit.get("firstPublicParent") + if first_public_parent: + return first_public_parent + return None diff --git a/libs/phabricator-client/tests/test_client.py b/libs/phabricator-client/tests/test_client.py index 666b447e06..d74d81d096 100644 --- a/libs/phabricator-client/tests/test_client.py +++ b/libs/phabricator-client/tests/test_client.py @@ -273,6 +273,28 @@ async def test_query_latest_diff_carries_the_author(monkeypatch): assert diff.author == "Ada Lovelace " +async def test_query_latest_diff_carries_first_public_parent(monkeypatch): + _capture_post( + monkeypatch, + { + "result": { + "9": { + "id": "9", + "sourceControlBaseRevision": "unlanded", + "properties": { + "local:commits": { + "empty-node": {}, + "local-node": {"firstPublicParent": "public-parent"}, + } + }, + } + } + }, + ) + diff = await _client().query_latest_diff(42) + assert diff.first_public_parent == "public-parent" + + async def test_query_latest_diff_has_no_author_when_phabricator_gives_none( monkeypatch, ):