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

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/migrations/0053_alter_repo_hooks.py

@@ -0,0 +1,74 @@
+# Generated by Django 6.0.4 on 2026-05-13 12:00
+
+import django.contrib.postgres.fields
+from django.db import migrations, models
+
+import lando.main.models.repo
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("main", "0052_alter_profile_options"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="repo",
+            name="hooks",
+            field=django.contrib.postgres.fields.ArrayField(
+                base_field=models.CharField(
+                    choices=[
+                        (
+                            "PreventSymlinksCheck",
+                            "Check for symlinks introduced in the diff.",
+                        ),
+                        (
+                            "TryTaskConfigCheck",
+                            "Check for `try_task_config.json` introduced in the diff.",
+                        ),
+                        (
+                            "PreventDotGithubCheck",
+                            "Prevent changes to GitHub workflows directory.",
+                        ),
+                        (
+                            "PreventHgDirectoryCheck",
+                            "Prevent patches from modifying .hg/ directory.",
+                        ),
+                        (
+                            "PreventNSPRNSSCheck",
+                            "Prevent changes to vendored NSPR directories.",
+                        ),
+                        (
+                            "PreventSignedCommitsCheck",
+                            "Prevent patches from introducing signed commits.",
+                        ),
+                        (
+                            "PreventSubmodulesCheck",
+                            "Prevent introduction of Git submodules into the repository.",
+                        ),
+                        (
+                            "CommitMessagesCheck",
+                            "Check the format of the passed commit message for issues.",
+                        ),
+                        (
+                            "WPTSyncCheck",
+                            "Check the WPTSync bot is only pushing changes to relevant subset of the tree.",
+                        ),
+                        (
+                            "BugReferencesCheck",
+                            "Prevent commit messages referencing non-public bugs from try.",
+                        ),
+                        (
+                            "PreventEmptyBinaryCheck",
+                            "Detect diffs that add a binary file with zero bytes.",
+                        ),
+                    ],
+                    max_length=255,
+                ),
+                blank=True,
+                default=lando.main.models.repo.get_default_hooks,
+                null=True,
+            ),
+        ),
+    ]

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/management/commands/migrate_data.py

@@ -97,3 +97,31 @@ def migrate_1_bug1971103_add_firefox_required_automation_permissions(
                 self.stdout.write(f"{repo.name} ", ending="")
 
         self.stdout.write("done.")
+
+    def migrate_2_bug1709608_enable_prevent_empty_binary_hook(
+        self, ask_confirm: bool = True
+    ):
+        """
+        Enable the `PreventEmptyBinaryCheck` hook for all Phabricator repos.
+
+        See bug 1709608: a Phabricator diff whose `creationMethod` is `commit`
+        can land binary files as zero bytes. The hook defends every landing
+        path, but it only fires when enabled on a repo.
+        """
+        hook = Repo.HooksChoices.PreventEmptyBinaryCheck.value
+        phab_repos = Repo.objects.filter(is_phabricator_repo=True).exclude(
+            hooks__contains=[hook]
+        )
+        if not phab_repos:
+            self.stdout.write(f"No Phabricator repo found without the {hook} hook.")
+            raise SystemExit()
+
+        repo_names = ", ".join(repo.name for repo in phab_repos)
+        self._get_confirmation(ask_confirm, f"Enabling {hook} hook for: ", repo_names)
+
+        for repo in phab_repos:
+            repo.hooks = (repo.hooks or []) + [hook]
+            repo.save()
+            self.stdout.write(f"{repo.name} ", ending="")
+
+        self.stdout.write("done.")

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(