fix(visual-review): skip baseline healing on merge-queue branches

[gantoine]

Problem

Any PR that deletes a snapshot identifier currently cannot get through the merge queue. It fails Visual regression tests pass, gets bisected out, re-enters, and fails again forever.

That covers renaming a story, dropping one, or making a story single-theme. Concretely, #74295 renames CollapsibleFrame's InitiallyExpanded dark story, and has been stuck in the queue for hours. #74746's batch hit the same thing yesterday.

The failing job is Complete Visual Review run, which the gate collates:

Done: 4110 snapshots — 4109 unchanged, 0 changed, 0 new, 1 removed
Visual changes detected — review at: .../visual_review/runs/4dc40c1a-...
##[error]Process completed with exit code 1

Nothing looks different. 0 changed, 0 new. The gate fails purely on a phantom removed.

Where the phantom comes from

_resolve_baselines_with_merge_base merges the branch baseline with the merge-base baseline, to repair entries a rebase replayed destructively. _tombstoned_identifiers is what stops that healing from resurrecting a removal a human already approved, and it filters on run__branch=branch. Tombstones are strictly branch-scoped.

That pairing holds on a normal PR branch. It falls apart in the queue:

	PR branch	Merge-queue batch branch
Branch	hp/error-tracking-code-contrast	trunk-merge/pr-68901/<uuid>, fresh per attempt
Run history	yes	none
Tombstones	removal approved, bot commits baseline, check green	none exist
Result	healing suppressed	healing resurrects the deletion, flags it REMOVED, gate fails

Nobody can approve it away either. The batch branch is gone before a human could review the run, and the merge base keeps the deleted entry until the PR lands. Deadlock.

I confirmed the merge commit's own baseline file is correct: 275fcac2999:frontend/snapshots.yml has 4109 entries with the deleted identifier absent. The 4110th is added server-side by healing. Master pushes are all clean at 0 removed, so the baseline itself is fine.

ci-storybook.yml already special-cases trunk-merge/ branches twice (the no-ci bypass and the narrowed story selection). The Visual Review backend had no notion of them.

Changes

Skip merge-base healing on trunk-merge/ branches, next to the existing default-branch early return.

-    if branch == default_branch:
+    if branch == default_branch or _is_merge_queue_branch(branch):
         return branch_baseline, 0

This is semantically right, not just a workaround. Healing exists to repair rebase-rewritten history, and an ephemeral batch branch has none. A batch branch is master with the batched PR heads merged in, so its baseline file is already exactly what is about to land.

Note

The tradeoff: a rebase-mangled baseline would now pass silently in the queue instead of being caught. That is fine, because the queue branch's file is the one that lands on master, and the next master run validates it.

A broader alternative is making tombstone lookup ancestry-aware so it inherits from the batched PRs' branches. That is a bigger change and it does not unblock the queue today.

How did you test this code?

Added test_skips_merge_base_for_merge_queue_branch to TestMergeBaseBaselineHealing. It catches the exact regression: if the exemption is dropped, an identifier deleted on the branch but still present at the merge base gets healed back in. No existing test in that class uses a merge-queue branch name, so nothing covered this.

I verified it actually fails without the fix, rather than assuming:

Test run against the unfixed logic

>       assert merged == branch_baseline
E       AssertionError: assert equals failed
E          -{
E          ^  'A': 'h1' ^,              ^{'A': 'h1' ^}
E          -  'DeletedStory': 'h2',
E          -}
FAILED products/visual_review/backend/tests/test_logic.py::TestMergeBaseBaselineHealing::test_skips_merge_base_for_merge_queue_branch

_mock_github's hardcoded "my-branch" became a branch_name parameter so the helper can serve a differently-named branch. Default unchanged, so every existing case is untouched.

Ran locally: the full products/visual_review/backend/tests/ suite (411 passed in 16m15s), ruff check, ruff format, and repo-wide uv run mypy --cache-fine-grained . (clean, 17184 files).

No manual testing. The failure only reproduces inside a real Trunk batch, which I cannot stand up locally.

Automatic notifications

• Publish to changelog?
• Alert Sales and Marketing teams?

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

I asked Claude Code (Opus 5) to work out why visual regression checks kept failing in the merge queue, starting from a Trunk queue screenshot and nothing else. It traced the gate to vr-complete, pulled the job logs, and found 0 changed, 0 new, 1 removed, which ruled out an actual visual diff. From there it fetched the batch merge commit to prove the baseline file on disk was correct, then read logic.py to find healing as the source of the extra entry and the branch-scoped tombstone query as the reason it only bites in the queue.

Two fixes were on the table. Ancestry-aware tombstones is the more general one, but it is a real change to how approvals resolve and would not land in time to unblock anything. The prefix exemption is narrow and matches what the healing code is actually for, so we went with it and noted the alternative above.

Skills invoked: /writing-tests (gated whether the new test earns its place, which is why there is one test and not a matrix), /writing-code-comments (the _is_merge_queue_branch docstring, and rewriting an em-dash into a connective).

[trunk-io]

Merging to master in this repository is managed by Trunk.

• To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[greptile-apps]

_{Reviews (1): Last reviewed commit: "fix(visual-review): skip baseline healin..." | Re-trigger Greptile}

[veria-ai]

Medium: Visual review gate bypass through branch naming

A contributor can create a same-repository PR from a trunk-merge/... branch and delete baseline entries. Because CI passes the PR's branch name here, the prefix alone skips merge-base healing, so omitted snapshots are not classified as REMOVED and the required visual-review status can succeed without approval. Verify through GitHub that the run belongs to an actual Trunk-created merge-queue PR, or pass a server-derived trusted queue indicator instead of trusting the branch name.

[veria-ai]

PR overview

This PR updates the visual-review backend to skip baseline healing for merge-queue branches.

One security issue remains open: the merge-queue determination relies only on a branch-name prefix. A same-repository contributor can use that prefix to prevent omitted snapshots from being marked as removed, allowing the required visual-review status to pass without approval. No issues have yet been addressed.

Open issues (1)

• Medium: Visual review gate bypass through branch naming — products/visual_review/backend/logic.py:565

Fixed/addressed: 0 · PR risk: 7/10

[hex-security-app]

Do not trust merge-queue branch names

branch is the PR head ref supplied by the CI workflow, and any same-repository contributor can name a branch trunk-merge/.... That makes a normal review run skip removal detection, allowing deleted snapshot identifiers to receive the required non-partial Visual Review success status without the approval that a normal branch requires. Verify that the run belongs to a Trunk-created queue PR/server-side, rather than using a forgeable ref prefix.

Prompt To Fix With AI

Replace the branch-prefix trust decision with a server-side verification that the run is associated with an actual Trunk-created merge-queue PR (using trusted GitHub PR metadata or a separately authenticated queue indicator). Do not let a caller-controlled PR head branch name alone bypass merge-base healing/removal detection. Add a regression test for a normal same-repository `trunk-merge/...` head ref proving it remains gated.

---

_{Severity: medium | Confidence: 93% | React with 👍 if useful or 👎 if not}

[gantoine]

Superseded by #75420, which fixes the same merge-queue healing failure by inheriting tombstones from a server-verified source PR instead of skipping healing entirely on trunk-merge/ branches. That approach keeps healing working on queue branches and is already on master, so this PR's diff is now empty.