transplants: warn on zero-byte binary file additions (bug 1709608)

Surface zero-byte binary file additions to the landing user as a
warning that must be acknowledged before landing.

Bug 1709608 has recurred multiple times since 2021: when Lando lands
using a Phabricator diff whose `creationMethod` is `commit` (the
auto-generated diff Phabricator creates from a landed commit), binary
file payloads have been stripped, so the file lands as 0 bytes. Most
recently this hit bug 2034984 (where both the test and reference image
in a reftest landed as 0 bytes, causing the comparison to silently pass
and removing test coverage with no signal) and bug 2005089 (Android
sample-app launcher icons landed as 0 bytes).

The 2023-12 fix in #364 only covered the uplift creation path; non-uplift
relands remain vulnerable. As an interim safety net, add a warning that
inspects the rendered patch and surfaces any zero-byte binary additions.
A follow-up will add a targeted blocker that uses `differential.querydiffs`
to detect the regression case (binary file went from non-zero to zero
between diff versions on the same revision) and block automatically,
without bothering authors of legitimate empty-binary fixtures.

The new `PreventEmptyBinaryCheck` is written as a neutral detection
primitive (no warn/block opinion in its message) so the follow-up
blocker can reuse it unchanged.

Author: tnikkel

src/lando/api/legacy/transplants.py

@@ -48,6 +48,7 @@
 from lando.main.support import LegacyAPIException
 from lando.utils.landing_checks import (
     DiffAssessor,
+    PreventEmptyBinaryCheck,
     PreventNSPRNSSCheck,
     PreventSubmodulesCheck,
     PreventSymlinksCheck,
@@ -536,6 +537,28 @@ def warning_multiple_authors(
         return f"Revision has multiple authors: {', '.join(author_usernames)}."
 
 
+@RevisionWarningCheck("Adds zero-byte binary files (bug 1709608).")
+def warning_empty_binary_files(
+    revision: dict, diff: dict, stack_state: StackAssessmentState
+) -> str | None:
+    """Warn when the active diff adds binary files with zero bytes.
+
+    Symptom of bug 1709608: diffs sourced from a landed commit have their
+    binary payloads stripped by Phabricator, so a re-land using such a diff
+    silently produces empty binary files in the tree. This warning surfaces
+    the situation; the targeted blocker (later) catches the regression case
+    automatically. Implemented as a warning so legitimately-empty binary
+    fixtures (rare but real -- e.g. `empty.xpi`, `0_sized_file`) can still
+    be landed by acknowledging the warning.
+    """
+    diff_id = PhabricatorClient.expect(diff, "id")
+    parsed_diff = stack_state.parsed_diffs[diff_id]
+
+    diff_assessor = DiffAssessor(parsed_diff=parsed_diff)
+    if issues := diff_assessor.run_diff_checks([PreventEmptyBinaryCheck]):
+        return issues[0]
+
+
 def blocker_user_no_auth0_email(
     stack_state: StackAssessmentState,
 ) -> str | None:
@@ -899,6 +922,7 @@ def blocker_try_task_config(
     warning_wip_commit_message,
     warning_unresolved_comments,
     warning_multiple_authors,
+    warning_empty_binary_files,
 ]
 
 

src/lando/main/models/repo.py

@@ -111,6 +111,10 @@ class HooksChoices(models.TextChoices):
             "BugReferencesCheck",
             "Prevent commit messages referencing non-public bugs from try.",
         )
+        PreventEmptyBinaryCheck = (
+            "PreventEmptyBinaryCheck",
+            "Detect diffs that add a binary file with zero bytes.",
+        )
 
     @property
     def path(self) -> str:

src/lando/utils/landing_checks.py

@@ -336,6 +336,58 @@ def result(self) -> str | None:
             )
 
 
+@dataclass
+class PreventEmptyBinaryCheck(PatchCheck):
+    """Detect diffs that add a binary file with zero bytes.
+
+    Symptom of bug 1709608: when Lando lands using a Phabricator diff whose
+    `creationMethod` is `commit`, binary file payloads have been stripped
+    and the file would land as 0 bytes. The fix that landed for bug 1709608
+    only covers the uplift creation path; this check defends every landing
+    path by inspecting the rendered patch directly, so it is root-cause-
+    agnostic.
+
+    The check produces a list of offending filenames; callers decide whether
+    to surface this as a warning or a blocker.
+    """
+
+    @override
+    @classmethod
+    def name(cls) -> str:
+        return "PreventEmptyBinaryCheck"
+
+    @override
+    @classmethod
+    def description(cls) -> str:
+        return "Detect diffs that add a binary file with zero bytes."
+
+    empty_binary_files: list[str] = field(default_factory=list)
+
+    def next_diff(self, diff: dict):
+        # Only flag *additions* of binary files. Modifications and deletions
+        # legitimately have zero or partial-binary payloads.
+        if not diff.get("binary") or diff.get("deleted") or not diff.get("new"):
+            return
+
+        # `binary_hunk_size` from `rs_parsepatch` is e.g. [("literal", 87)] or
+        # [("literal", N), ("delta", M)]. A pure-add of an empty file shows up
+        # as a single ("literal", 0) hunk; sum across hunks defensively.
+        total = sum(size for _kind, size in diff.get("binary_hunk_size") or [])
+        if total == 0:
+            self.empty_binary_files.append(diff["filename"])
+
+    def result(self) -> str | None:
+        if self.empty_binary_files:
+            return (
+                "Revision adds zero-byte binary files "
+                f"({wrap_filenames(self.empty_binary_files)}). This usually "
+                "means the active diff was sourced from a landed commit and "
+                "its binary payloads were stripped by Phabricator. Re-submit "
+                "the revision with `moz-phab` to restore the binary content. "
+                "See bug 1709608."
+            )
+
+
 @dataclass
 class PreventSymlinksCheck(PatchCheck):
     """Check for symlinks introduced in the diff."""

src/lando/utils/tests/test_landing_checks.py

@@ -16,6 +16,7 @@
     LandingChecks,
     PatchCollectionAssessor,
     PreventDotGithubCheck,
+    PreventEmptyBinaryCheck,
     PreventHgDirectoryCheck,
     PreventNSPRNSSCheck,
     PreventSignedCommitsCheck,
@@ -646,6 +647,99 @@ def test_check_prevent_submodules():
     ), "Check should prevent revisions from introducing submodules."
 
 
+# Patches exercising the binary file paths in `rs_parsepatch`. These reflect
+# the GIT binary patch format Phabricator emits via `differential.getrawdiff`.
+GIT_DIFF_EMPTY_BINARY_ADD = """\
+diff --git a/empty.png b/empty.png
+new file mode 100644
+GIT binary patch
+literal 0
+HcmV?d00001
+
+"""
+
+GIT_DIFF_NONEMPTY_BINARY_ADD = """\
+diff --git a/real.png b/real.png
+new file mode 100644
+GIT binary patch
+literal 87
+zcmZ?wbhEHbWMp7uXkdSr1_lE>3=9k!85kJ?7+e9j7+M3l85kIz?VMfFP&3pMz`(EJ
+o&%nUd&(O%n&CtNI%g_nQz5(_qzaP=zsBtDUMI;Cj1_p+T0VKi-RR910
+
+"""
+
+GIT_DIFF_BINARY_DELETE = """\
+diff --git a/old.png b/old.png
+deleted file mode 100644
+GIT binary patch
+literal 0
+HcmV?d00001
+
+"""
+
+
+def test_check_prevent_empty_binary_text_only():
+    """Text-only diffs are unaffected."""
+    parsed_diff = rs_parsepatch.get_diffs(
+        GIT_DIFF_FILENAME_TEMPLATE.format(filename="src/foo.cpp")
+    )
+    check = PreventEmptyBinaryCheck()
+    for diff in parsed_diff:
+        check.next_diff(diff)
+    assert check.result() is None, "Text-only diffs must not trigger the check."
+
+
+def test_check_prevent_empty_binary_nonempty_addition():
+    """Adding a non-empty binary file is allowed."""
+    parsed_diff = rs_parsepatch.get_diffs(GIT_DIFF_NONEMPTY_BINARY_ADD)
+    check = PreventEmptyBinaryCheck()
+    for diff in parsed_diff:
+        check.next_diff(diff)
+    assert check.result() is None, (
+        "Adding a binary file with content must not trigger the check."
+    )
+
+
+def test_check_prevent_empty_binary_deletion():
+    """Deleting a binary file is allowed even though its hunk is zero-sized."""
+    parsed_diff = rs_parsepatch.get_diffs(GIT_DIFF_BINARY_DELETE)
+    check = PreventEmptyBinaryCheck()
+    for diff in parsed_diff:
+        check.next_diff(diff)
+    assert check.result() is None, "Deleting a binary file must not trigger the check."
+
+
+def test_check_prevent_empty_binary_addition():
+    """Adding a zero-byte binary file is the bug 1709608 signature."""
+    parsed_diff = rs_parsepatch.get_diffs(GIT_DIFF_EMPTY_BINARY_ADD)
+    check = PreventEmptyBinaryCheck()
+    for diff in parsed_diff:
+        check.next_diff(diff)
+    result = check.result()
+    assert result is not None, "Adding a zero-byte binary file must trigger the check."
+    assert "empty.png" in result
+    assert "1709608" in result
+
+
+def test_check_prevent_empty_binary_mixed():
+    """In a mixed diff, only the empty addition is reported."""
+    parsed_diff = rs_parsepatch.get_diffs(
+        GIT_DIFF_EMPTY_BINARY_ADD
+        + GIT_DIFF_NONEMPTY_BINARY_ADD
+        + GIT_DIFF_FILENAME_TEMPLATE.format(filename="src/bar.cpp")
+    )
+    check = PreventEmptyBinaryCheck()
+    for diff in parsed_diff:
+        check.next_diff(diff)
+    result = check.result()
+    assert result is not None
+    assert "empty.png" in result
+    assert "real.png" not in result, (
+        "Non-empty binary additions must not appear in the report."
+    )
+    assert "src/bar.cpp" not in result, "Text additions must not appear in the report."
+
+
 def test_check_bug_references_public_bugs():
     patch_helper = HgPatchHelper.from_string_io(
         io.StringIO(