transplants: warn on zero-byte binary file additions (bug 1709608) by tnikkel · Pull Request #1157 · mozilla-conduit/lando

tnikkel · 2026-05-13T05:01:03Z

Summary

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 (test + reference
images in a reftest both 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, this PR adds a warning that inspects
the rendered patch and surfaces any zero-byte binary additions. A follow-up can
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 legitimately-empty
binary fixtures.

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

Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1709608

Test plan

New unit tests in src/lando/utils/tests/test_landing_checks.py cover:
text-only diff, non-empty binary addition, binary deletion, empty binary
addition (the bug 1709608 signature), and a mixed diff (only the empty
addition is reported).
Manual smoke: trigger a landing of a revision whose active diff was sourced
from a landed commit and confirm the warning appears.

github-actions · 2026-05-13T05:01:17Z

View this pull request in Lando to land it once approved.

shtrom · 2026-05-13T05:14:21Z

Looks good. There's a bit more housekeeping needed:

The hook needs to be added to this list

lando/src/lando/main/models/repo.py

Lines 71 to 72 in 5e2572b

class HooksChoices(models.TextChoices):

"""List of landing hooks that can be enabled for a repo."""

so it's available for selection (that causes the first test failure)
We should add a data-migration method in the ilk of https://github.com/mozilla-conduit/lando/blob/main/src/lando/utils/management/commands/migrate_data.py#L71-L99 which enable this hook by default for any Phabricator repo in the DB (see Repo.is_phabricator_repo https://github.com/mozilla-conduit/lando/blob/main/src/lando/main/models/repo.py#L175)
Could you run make format to fix the linting errors?

Thanks!

shtrom · 2026-05-13T05:26:20Z

Due to 1, this will need a run of make migrations, too (and a git add of the newly created file).

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 mozilla-conduit#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.

zzzeid · 2026-05-13T12:35:40Z

I don't think we need this addition in the migration file. (1) we should probably only add this to repos one at a time via the admin, and (2) this should be applied to repos that explicitly need it.

I suggest removing this change.

zzzeid · 2026-05-13T12:36:41Z

+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


What is this content?

zzzeid · 2026-05-13T12:41:40Z

+    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."


Can be more explicit here.

zzzeid · 2026-05-13T12:42:16Z

+    assert "empty.png" in result
+    assert "1709608" in result


Can be more explicit here with the warning message, and more human readable.

zzzeid · 2026-05-13T12:49:33Z

+    Symptom of bug 1709608: when Lando lands using a Phabricator diff whose
+    `creationMethod` is `commit`, binary file payloads have been stripped


Would be better to discuss the actual causes (e.g., backouts, reopened revisions, automatically updated revisions, etc.) vs. mentioning creationMethod here, since that is quite low level.

zzzeid · 2026-05-13T12:54:27Z

+
+    def next_diff(self, diff: dict):
+        # Only flag *additions* of binary files. Modifications and deletions
+        # legitimately have zero or partial-binary payloads.


Can you clarify what partial-binary payloads means here?

zzzeid · 2026-05-13T12:54:56Z

+    def result(self) -> str | None:
+        if self.empty_binary_files:
+            return (
+                "Revision adds zero-byte binary files "


We're mixing diff and revision throughout this check. I think we can stick with diff since it is the source of the issue.

zzzeid · 2026-05-13T12:55:53Z

 from lando.main.support import LegacyAPIException
 from lando.utils.landing_checks import (
    DiffAssessor,
+    PreventEmptyBinaryCheck,


Suggested change

PreventEmptyBinaryCheck,

PreventEmptyBinaryFileAdditionCheck,

zzzeid · 2026-05-13T12:59:29Z

+    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


Reducing verbosity here would be good.

Suggested change

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

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; at time of landing the check catches this case and warns about it.

zzzeid · 2026-05-13T12:59:58Z

+    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.


Suggested change

be landed by acknowledging the warning.

tnikkel requested a review from a team as a code owner May 13, 2026 05:01

tnikkel force-pushed the bug1709608/warn-zero-byte-binary branch from 24245fe to 791a5dc Compare May 13, 2026 05:23

tnikkel force-pushed the bug1709608/warn-zero-byte-binary branch from 791a5dc to bba0f76 Compare May 13, 2026 05:28

tnikkel force-pushed the bug1709608/warn-zero-byte-binary branch from bba0f76 to bfa5fd9 Compare May 13, 2026 05:47

zzzeid requested changes May 13, 2026

View reviewed changes

		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

		Symptom of bug 1709608: when Lando lands using a Phabricator diff whose
		`creationMethod` is `commit`, binary file payloads have been stripped

	PreventEmptyBinaryCheck,
	PreventEmptyBinaryFileAdditionCheck,

Conversation

tnikkel commented May 13, 2026

Summary

Test plan

Uh oh!

github-actions Bot commented May 13, 2026

Uh oh!

shtrom commented May 13, 2026

Uh oh!

shtrom commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shtrom commented May 13, 2026 •

edited

Loading