From 4280a1fbdec8893dd0a4f042584ee289f845db9b Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Tue, 7 Apr 2026 14:44:15 -0700 Subject: [PATCH 1/9] feat(preprod): Add _build_comparison_fingerprints helper for snapshot auto-approval --- src/sentry/preprod/snapshots/tasks.py | 116 +++- .../preprod/snapshots/test_auto_approve.py | 645 ++++++++++++++++++ 2 files changed, 760 insertions(+), 1 deletion(-) create mode 100644 tests/sentry/preprod/snapshots/test_auto_approve.py diff --git a/src/sentry/preprod/snapshots/tasks.py b/src/sentry/preprod/snapshots/tasks.py index 5ce11f0f8b4ab6..c4a8128a2610cf 100644 --- a/src/sentry/preprod/snapshots/tasks.py +++ b/src/sentry/preprod/snapshots/tasks.py @@ -12,7 +12,7 @@ from taskbroker_client.retry import Retry from sentry.objectstore import get_preprod_session -from sentry.preprod.models import PreprodArtifact +from sentry.preprod.models import PreprodArtifact, PreprodComparisonApproval from sentry.preprod.snapshots.image_diff.compare import compare_images_batch from sentry.preprod.snapshots.image_diff.odiff import OdiffServer from sentry.preprod.snapshots.manifest import ( @@ -113,6 +113,118 @@ def _create_pixel_batches( return batches +def _build_comparison_fingerprints(manifest: ComparisonManifest) -> set[tuple[str, ...]]: + fingerprints: set[tuple[str, ...]] = set() + for name, image in manifest.images.items(): + if image.status == "unchanged": + continue + if image.status in ("changed", "added"): + fingerprints.add((name, image.status, image.head_hash or "")) + elif image.status == "renamed": + fingerprints.add( + (name, "renamed", image.head_hash or "", image.previous_image_file_name or "") + ) + else: + fingerprints.add((name, image.status)) + return fingerprints + + +def _try_auto_approve_snapshot( + head_artifact: PreprodArtifact, + comparison_manifest: ComparisonManifest, + session: object, +) -> bool: + cc = head_artifact.commit_comparison + if not cc or not cc.pr_number or not cc.head_repo_name: + return False + + head_fingerprints = _build_comparison_fingerprints(comparison_manifest) + if not head_fingerprints: + return False + + approved_sibling = ( + PreprodArtifact.objects.filter( + project_id=head_artifact.project_id, + app_id=head_artifact.app_id, + build_configuration=head_artifact.build_configuration, + commit_comparison__pr_number=cc.pr_number, + commit_comparison__head_repo_name=cc.head_repo_name, + preprodcomparisonapproval__preprod_feature_type=PreprodComparisonApproval.FeatureType.SNAPSHOTS, + preprodcomparisonapproval__approval_status=PreprodComparisonApproval.ApprovalStatus.APPROVED, + preprodsnapshotmetrics__snapshot_comparisons_head_metrics__state=PreprodSnapshotComparison.State.SUCCESS, + ) + .exclude(id=head_artifact.id) + .order_by("-date_added") + .first() + ) + + if not approved_sibling: + return False + + sibling_comparison = ( + PreprodSnapshotComparison.objects.filter( + head_snapshot_metrics__preprod_artifact=approved_sibling, + state=PreprodSnapshotComparison.State.SUCCESS, + ) + .order_by("-date_updated") + .first() + ) + + if not sibling_comparison: + return False + + sibling_comparison_key = (sibling_comparison.extras or {}).get("comparison_key") + if not sibling_comparison_key: + return False + + try: + sibling_manifest = ComparisonManifest( + **orjson.loads(session.get(sibling_comparison_key).payload.read()) + ) + except Exception: + logger.exception( + "auto_approve: failed to load sibling comparison manifest", + extra={ + "head_artifact_id": head_artifact.id, + "sibling_artifact_id": approved_sibling.id, + "comparison_key": sibling_comparison_key, + }, + ) + return False + + sibling_fingerprints = _build_comparison_fingerprints(sibling_manifest) + + if head_fingerprints != sibling_fingerprints: + logger.info( + "auto_approve: fingerprints do not match", + extra={ + "head_artifact_id": head_artifact.id, + "sibling_artifact_id": approved_sibling.id, + }, + ) + return False + + PreprodComparisonApproval.objects.create( + preprod_artifact=head_artifact, + preprod_feature_type=PreprodComparisonApproval.FeatureType.SNAPSHOTS, + approval_status=PreprodComparisonApproval.ApprovalStatus.APPROVED, + approved_at=timezone.now(), + extras={ + "auto_approval": True, + "prev_approved_artifact_id": approved_sibling.id, + }, + ) + + logger.info( + "auto_approve: snapshot auto-approved", + extra={ + "head_artifact_id": head_artifact.id, + "prev_approved_artifact_id": approved_sibling.id, + }, + ) + return True + + @instrumented_task( name="sentry.preprod.tasks.compare_snapshots", namespace=preprod_tasks, @@ -581,6 +693,8 @@ def _fetch_hash(h: str) -> None: ): metrics.incr("preprod.snapshots.diff.zero_changes", sample_rate=1.0, tags=metric_tags) + _try_auto_approve_snapshot(head_artifact, comparison_manifest, session) + create_preprod_snapshot_status_check_task.apply_async( kwargs={ "preprod_artifact_id": head_artifact_id, diff --git a/tests/sentry/preprod/snapshots/test_auto_approve.py b/tests/sentry/preprod/snapshots/test_auto_approve.py new file mode 100644 index 00000000000000..a3a149058ef64b --- /dev/null +++ b/tests/sentry/preprod/snapshots/test_auto_approve.py @@ -0,0 +1,645 @@ +from __future__ import annotations + +from unittest.mock import MagicMock + +import orjson + +from sentry.preprod.models import PreprodArtifact, PreprodComparisonApproval +from sentry.preprod.snapshots.manifest import ( + ComparisonImageResult, + ComparisonManifest, + ComparisonSummary, +) +from sentry.preprod.snapshots.models import PreprodSnapshotComparison, PreprodSnapshotMetrics +from sentry.preprod.snapshots.tasks import ( + _build_comparison_fingerprints, + _try_auto_approve_snapshot, +) +from sentry.testutils.cases import TestCase +from sentry.testutils.silo import cell_silo_test + + +class BuildComparisonFingerprintsTest(TestCase): + def _make_manifest(self, images: dict[str, ComparisonImageResult]) -> ComparisonManifest: + changed = sum(1 for i in images.values() if i.status == "changed") + added = sum(1 for i in images.values() if i.status == "added") + removed = sum(1 for i in images.values() if i.status == "removed") + errored = sum(1 for i in images.values() if i.status == "errored") + renamed = sum(1 for i in images.values() if i.status == "renamed") + unchanged = sum(1 for i in images.values() if i.status == "unchanged") + return ComparisonManifest( + head_artifact_id=1, + base_artifact_id=2, + summary=ComparisonSummary( + total=len(images), + changed=changed, + added=added, + removed=removed, + errored=errored, + renamed=renamed, + unchanged=unchanged, + ), + images=images, + ) + + def test_extracts_changed_with_head_hash(self): + manifest = self._make_manifest( + { + "screen1.png": ComparisonImageResult( + status="changed", head_hash="abc", base_hash="def" + ), + } + ) + fps = _build_comparison_fingerprints(manifest) + assert fps == {("screen1.png", "changed", "abc")} + + def test_extracts_added_with_head_hash(self): + manifest = self._make_manifest( + { + "new_screen.png": ComparisonImageResult(status="added", head_hash="abc"), + } + ) + fps = _build_comparison_fingerprints(manifest) + assert fps == {("new_screen.png", "added", "abc")} + + def test_extracts_removed_without_hash(self): + manifest = self._make_manifest( + { + "old_screen.png": ComparisonImageResult(status="removed", base_hash="xyz"), + } + ) + fps = _build_comparison_fingerprints(manifest) + assert fps == {("old_screen.png", "removed")} + + def test_extracts_errored_without_hash(self): + manifest = self._make_manifest( + { + "broken.png": ComparisonImageResult(status="errored", reason="exceeds_pixel_limit"), + } + ) + fps = _build_comparison_fingerprints(manifest) + assert fps == {("broken.png", "errored")} + + def test_extracts_renamed_with_hash_and_previous_name(self): + manifest = self._make_manifest( + { + "new_name.png": ComparisonImageResult( + status="renamed", head_hash="abc", previous_image_file_name="old_name.png" + ), + } + ) + fps = _build_comparison_fingerprints(manifest) + assert fps == {("new_name.png", "renamed", "abc", "old_name.png")} + + def test_skips_unchanged(self): + manifest = self._make_manifest( + { + "stable.png": ComparisonImageResult( + status="unchanged", head_hash="aaa", base_hash="aaa" + ), + "changed.png": ComparisonImageResult( + status="changed", head_hash="bbb", base_hash="ccc" + ), + } + ) + fps = _build_comparison_fingerprints(manifest) + assert fps == {("changed.png", "changed", "bbb")} + + def test_mixed_statuses(self): + manifest = self._make_manifest( + { + "unchanged.png": ComparisonImageResult( + status="unchanged", head_hash="a", base_hash="a" + ), + "changed.png": ComparisonImageResult( + status="changed", head_hash="b", base_hash="c" + ), + "added.png": ComparisonImageResult(status="added", head_hash="d"), + "removed.png": ComparisonImageResult(status="removed", base_hash="e"), + "errored.png": ComparisonImageResult(status="errored"), + "renamed.png": ComparisonImageResult( + status="renamed", head_hash="f", previous_image_file_name="old.png" + ), + } + ) + fps = _build_comparison_fingerprints(manifest) + assert fps == { + ("changed.png", "changed", "b"), + ("added.png", "added", "d"), + ("removed.png", "removed"), + ("errored.png", "errored"), + ("renamed.png", "renamed", "f", "old.png"), + } + + def test_empty_manifest_returns_empty_set(self): + manifest = self._make_manifest({}) + fps = _build_comparison_fingerprints(manifest) + assert fps == set() + + +def _mock_session_with_manifests(manifests_by_key: dict[str, bytes]) -> MagicMock: + session = MagicMock() + + def _get(key): + result = MagicMock() + if key in manifests_by_key: + result.payload.read.return_value = manifests_by_key[key] + else: + raise Exception(f"Key not found: {key}") + return result + + session.get.side_effect = _get + return session + + +@cell_silo_test +class TryAutoApproveSnapshotTest(TestCase): + def setUp(self): + super().setUp() + self.organization = self.create_organization(owner=self.user) + self.project = self.create_project(organization=self.organization) + + def _create_approved_sibling( + self, + pr_number: int, + comparison_images: dict, + app_id: str = "com.example.app", + build_configuration=None, + ) -> tuple[PreprodArtifact, str, bytes]: + cc = self.create_commit_comparison( + organization=self.organization, + pr_number=pr_number, + head_repo_name="owner/repo", + ) + artifact = self.create_preprod_artifact( + project=self.project, + commit_comparison=cc, + app_id=app_id, + build_configuration=build_configuration, + ) + head_metrics = PreprodSnapshotMetrics.objects.create( + preprod_artifact=artifact, + image_count=10, + ) + base_artifact = self.create_preprod_artifact( + project=self.project, + commit_comparison=self.create_commit_comparison(organization=self.organization), + ) + base_metrics = PreprodSnapshotMetrics.objects.create( + preprod_artifact=base_artifact, + image_count=10, + ) + comparison_key = f"{self.organization.id}/{self.project.id}/{artifact.id}/{base_artifact.id}/comparison.json" + PreprodSnapshotComparison.objects.create( + head_snapshot_metrics=head_metrics, + base_snapshot_metrics=base_metrics, + state=PreprodSnapshotComparison.State.SUCCESS, + images_changed=1, + extras={"comparison_key": comparison_key}, + ) + PreprodComparisonApproval.objects.create( + preprod_artifact=artifact, + preprod_feature_type=PreprodComparisonApproval.FeatureType.SNAPSHOTS, + approval_status=PreprodComparisonApproval.ApprovalStatus.APPROVED, + ) + + manifest = ComparisonManifest( + head_artifact_id=artifact.id, + base_artifact_id=base_artifact.id, + summary=ComparisonSummary( + total=len(comparison_images), + changed=sum(1 for i in comparison_images.values() if i.status == "changed"), + added=sum(1 for i in comparison_images.values() if i.status == "added"), + removed=sum(1 for i in comparison_images.values() if i.status == "removed"), + errored=sum(1 for i in comparison_images.values() if i.status == "errored"), + renamed=sum(1 for i in comparison_images.values() if i.status == "renamed"), + unchanged=sum(1 for i in comparison_images.values() if i.status == "unchanged"), + ), + images=comparison_images, + ) + return artifact, comparison_key, orjson.dumps(manifest.dict()) + + def _create_head_manifest(self, images: dict) -> ComparisonManifest: + return ComparisonManifest( + head_artifact_id=999, + base_artifact_id=998, + summary=ComparisonSummary( + total=len(images), + changed=sum(1 for i in images.values() if i.status == "changed"), + added=sum(1 for i in images.values() if i.status == "added"), + removed=sum(1 for i in images.values() if i.status == "removed"), + errored=sum(1 for i in images.values() if i.status == "errored"), + renamed=sum(1 for i in images.values() if i.status == "renamed"), + unchanged=sum(1 for i in images.values() if i.status == "unchanged"), + ), + images=images, + ) + + def test_auto_approves_when_fingerprints_match(self): + shared_images = { + "screen1.png": ComparisonImageResult( + status="changed", head_hash="abc", base_hash="old1" + ), + "screen2.png": ComparisonImageResult( + status="unchanged", head_hash="same", base_hash="same" + ), + } + sibling, comp_key, comp_json = self._create_approved_sibling( + pr_number=42, + comparison_images=shared_images, + ) + + cc = self.create_commit_comparison( + organization=self.organization, + pr_number=42, + head_repo_name="owner/repo", + ) + head_artifact = self.create_preprod_artifact( + project=self.project, + commit_comparison=cc, + app_id="com.example.app", + ) + + head_manifest = self._create_head_manifest( + { + "screen1.png": ComparisonImageResult( + status="changed", head_hash="abc", base_hash="new_base1" + ), + "screen2.png": ComparisonImageResult( + status="unchanged", head_hash="same", base_hash="same" + ), + "screen3.png": ComparisonImageResult( + status="unchanged", head_hash="extra", base_hash="extra" + ), + } + ) + + session = _mock_session_with_manifests({comp_key: comp_json}) + result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) + + assert result is True + approval = PreprodComparisonApproval.objects.get( + preprod_artifact=head_artifact, + preprod_feature_type=PreprodComparisonApproval.FeatureType.SNAPSHOTS, + approval_status=PreprodComparisonApproval.ApprovalStatus.APPROVED, + ) + assert approval.approved_by_id is None + assert approval.extras["auto_approval"] is True + assert approval.extras["prev_approved_artifact_id"] == sibling.id + + def test_no_auto_approve_when_fingerprints_differ(self): + sibling_images = { + "screen1.png": ComparisonImageResult( + status="changed", head_hash="abc", base_hash="old1" + ), + } + _, comp_key, comp_json = self._create_approved_sibling( + pr_number=42, + comparison_images=sibling_images, + ) + + cc = self.create_commit_comparison( + organization=self.organization, + pr_number=42, + head_repo_name="owner/repo", + ) + head_artifact = self.create_preprod_artifact( + project=self.project, + commit_comparison=cc, + app_id="com.example.app", + ) + + head_manifest = self._create_head_manifest( + { + "screen1.png": ComparisonImageResult( + status="changed", head_hash="DIFFERENT", base_hash="old1" + ), + } + ) + + session = _mock_session_with_manifests({comp_key: comp_json}) + result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) + + assert result is False + assert not PreprodComparisonApproval.objects.filter( + preprod_artifact=head_artifact, + approval_status=PreprodComparisonApproval.ApprovalStatus.APPROVED, + ).exists() + + def test_no_auto_approve_when_no_pr_number(self): + cc = self.create_commit_comparison( + organization=self.organization, + pr_number=None, + ) + head_artifact = self.create_preprod_artifact( + project=self.project, + commit_comparison=cc, + ) + head_manifest = self._create_head_manifest( + { + "screen1.png": ComparisonImageResult(status="changed", head_hash="abc"), + } + ) + session = MagicMock() + result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) + + assert result is False + + def test_no_auto_approve_when_no_approved_sibling(self): + cc = self.create_commit_comparison( + organization=self.organization, + pr_number=42, + head_repo_name="owner/repo", + ) + head_artifact = self.create_preprod_artifact( + project=self.project, + commit_comparison=cc, + app_id="com.example.app", + ) + head_manifest = self._create_head_manifest( + { + "screen1.png": ComparisonImageResult(status="changed", head_hash="abc"), + } + ) + session = MagicMock() + result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) + + assert result is False + + def test_no_auto_approve_when_no_changes(self): + cc = self.create_commit_comparison( + organization=self.organization, + pr_number=42, + head_repo_name="owner/repo", + ) + head_artifact = self.create_preprod_artifact( + project=self.project, + commit_comparison=cc, + ) + head_manifest = self._create_head_manifest( + { + "screen1.png": ComparisonImageResult( + status="unchanged", head_hash="a", base_hash="a" + ), + } + ) + session = MagicMock() + result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) + + assert result is False + + def test_handles_missing_comparison_manifest(self): + sibling_images = { + "screen1.png": ComparisonImageResult( + status="changed", head_hash="abc", base_hash="old1" + ), + } + _, comp_key, _ = self._create_approved_sibling( + pr_number=42, + comparison_images=sibling_images, + ) + + cc = self.create_commit_comparison( + organization=self.organization, + pr_number=42, + head_repo_name="owner/repo", + ) + head_artifact = self.create_preprod_artifact( + project=self.project, + commit_comparison=cc, + app_id="com.example.app", + ) + head_manifest = self._create_head_manifest( + { + "screen1.png": ComparisonImageResult(status="changed", head_hash="abc"), + } + ) + + session = MagicMock() + session.get.side_effect = Exception("Not found") + result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) + + assert result is False + + def test_matches_on_app_id_and_build_config(self): + shared_images = { + "screen1.png": ComparisonImageResult( + status="changed", head_hash="abc", base_hash="old1" + ), + } + self._create_approved_sibling( + pr_number=42, + comparison_images=shared_images, + app_id="com.other.app", + ) + + cc = self.create_commit_comparison( + organization=self.organization, + pr_number=42, + head_repo_name="owner/repo", + ) + head_artifact = self.create_preprod_artifact( + project=self.project, + commit_comparison=cc, + app_id="com.example.app", + ) + head_manifest = self._create_head_manifest( + { + "screen1.png": ComparisonImageResult(status="changed", head_hash="abc"), + } + ) + session = MagicMock() + result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) + + assert result is False + + def test_auto_approve_sets_approved_at(self): + shared_images = { + "screen1.png": ComparisonImageResult( + status="changed", head_hash="abc", base_hash="old1" + ), + } + _, comp_key, comp_json = self._create_approved_sibling( + pr_number=42, + comparison_images=shared_images, + ) + + cc = self.create_commit_comparison( + organization=self.organization, + pr_number=42, + head_repo_name="owner/repo", + ) + head_artifact = self.create_preprod_artifact( + project=self.project, + commit_comparison=cc, + app_id="com.example.app", + ) + + head_manifest = self._create_head_manifest( + { + "screen1.png": ComparisonImageResult( + status="changed", head_hash="abc", base_hash="new_base1" + ), + } + ) + + session = _mock_session_with_manifests({comp_key: comp_json}) + _try_auto_approve_snapshot(head_artifact, head_manifest, session) + + approval = PreprodComparisonApproval.objects.get( + preprod_artifact=head_artifact, + approval_status=PreprodComparisonApproval.ApprovalStatus.APPROVED, + ) + assert approval.approved_at is not None + + def test_no_auto_approve_when_no_head_repo_name(self): + cc = self.create_commit_comparison( + organization=self.organization, + pr_number=42, + head_repo_name="", + ) + head_artifact = self.create_preprod_artifact( + project=self.project, + commit_comparison=cc, + ) + head_manifest = self._create_head_manifest( + { + "screen1.png": ComparisonImageResult(status="changed", head_hash="abc"), + } + ) + session = MagicMock() + result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) + + assert result is False + + def test_no_auto_approve_when_extra_changed_image_in_head(self): + sibling_images = { + "screen1.png": ComparisonImageResult( + status="changed", head_hash="abc", base_hash="old1" + ), + } + _, comp_key, comp_json = self._create_approved_sibling( + pr_number=42, + comparison_images=sibling_images, + ) + + cc = self.create_commit_comparison( + organization=self.organization, + pr_number=42, + head_repo_name="owner/repo", + ) + head_artifact = self.create_preprod_artifact( + project=self.project, + commit_comparison=cc, + app_id="com.example.app", + ) + + head_manifest = self._create_head_manifest( + { + "screen1.png": ComparisonImageResult( + status="changed", head_hash="abc", base_hash="old1" + ), + "screen2.png": ComparisonImageResult( + status="changed", head_hash="new_change", base_hash="old2" + ), + } + ) + + session = _mock_session_with_manifests({comp_key: comp_json}) + result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) + + assert result is False + + def test_no_auto_approve_when_sibling_not_approved_for_snapshots(self): + cc = self.create_commit_comparison( + organization=self.organization, + pr_number=42, + head_repo_name="owner/repo", + ) + sibling = self.create_preprod_artifact( + project=self.project, + commit_comparison=cc, + app_id="com.example.app", + ) + head_metrics = PreprodSnapshotMetrics.objects.create( + preprod_artifact=sibling, + image_count=10, + ) + base_artifact = self.create_preprod_artifact( + project=self.project, + commit_comparison=self.create_commit_comparison(organization=self.organization), + ) + base_metrics = PreprodSnapshotMetrics.objects.create( + preprod_artifact=base_artifact, + image_count=10, + ) + PreprodSnapshotComparison.objects.create( + head_snapshot_metrics=head_metrics, + base_snapshot_metrics=base_metrics, + state=PreprodSnapshotComparison.State.SUCCESS, + images_changed=1, + ) + PreprodComparisonApproval.objects.create( + preprod_artifact=sibling, + preprod_feature_type=PreprodComparisonApproval.FeatureType.SIZE, + approval_status=PreprodComparisonApproval.ApprovalStatus.APPROVED, + ) + + cc2 = self.create_commit_comparison( + organization=self.organization, + pr_number=42, + head_repo_name="owner/repo", + ) + head_artifact = self.create_preprod_artifact( + project=self.project, + commit_comparison=cc2, + app_id="com.example.app", + ) + head_manifest = self._create_head_manifest( + { + "screen1.png": ComparisonImageResult(status="changed", head_hash="abc"), + } + ) + session = MagicMock() + result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) + + assert result is False + + def test_auto_approves_with_renamed_images(self): + shared_images = { + "new_name.png": ComparisonImageResult( + status="renamed", head_hash="abc", previous_image_file_name="old_name.png" + ), + } + sibling, comp_key, comp_json = self._create_approved_sibling( + pr_number=42, + comparison_images=shared_images, + ) + + cc = self.create_commit_comparison( + organization=self.organization, + pr_number=42, + head_repo_name="owner/repo", + ) + head_artifact = self.create_preprod_artifact( + project=self.project, + commit_comparison=cc, + app_id="com.example.app", + ) + + head_manifest = self._create_head_manifest( + { + "new_name.png": ComparisonImageResult( + status="renamed", head_hash="abc", previous_image_file_name="old_name.png" + ), + } + ) + + session = _mock_session_with_manifests({comp_key: comp_json}) + result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) + + assert result is True + assert PreprodComparisonApproval.objects.filter( + preprod_artifact=head_artifact, + approval_status=PreprodComparisonApproval.ApprovalStatus.APPROVED, + ).exists() From 213987e1c645b5b0e1e6dbfde7b0b9a468b0b547 Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Tue, 7 Apr 2026 15:08:58 -0700 Subject: [PATCH 2/9] fix(preprod): Skip images with missing hashes in fingerprint instead of using empty strings --- src/sentry/preprod/snapshots/tasks.py | 10 ++++--- .../preprod/snapshots/test_auto_approve.py | 27 +++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/src/sentry/preprod/snapshots/tasks.py b/src/sentry/preprod/snapshots/tasks.py index c4a8128a2610cf..9e6afe4ec3cbc7 100644 --- a/src/sentry/preprod/snapshots/tasks.py +++ b/src/sentry/preprod/snapshots/tasks.py @@ -119,11 +119,13 @@ def _build_comparison_fingerprints(manifest: ComparisonManifest) -> set[tuple[st if image.status == "unchanged": continue if image.status in ("changed", "added"): - fingerprints.add((name, image.status, image.head_hash or "")) + if not image.head_hash: + continue + fingerprints.add((name, image.status, image.head_hash)) elif image.status == "renamed": - fingerprints.add( - (name, "renamed", image.head_hash or "", image.previous_image_file_name or "") - ) + if not image.head_hash or not image.previous_image_file_name: + continue + fingerprints.add((name, "renamed", image.head_hash, image.previous_image_file_name)) else: fingerprints.add((name, image.status)) return fingerprints diff --git a/tests/sentry/preprod/snapshots/test_auto_approve.py b/tests/sentry/preprod/snapshots/test_auto_approve.py index a3a149058ef64b..38115dc262b43e 100644 --- a/tests/sentry/preprod/snapshots/test_auto_approve.py +++ b/tests/sentry/preprod/snapshots/test_auto_approve.py @@ -136,6 +136,33 @@ def test_empty_manifest_returns_empty_set(self): fps = _build_comparison_fingerprints(manifest) assert fps == set() + def test_skips_changed_with_missing_head_hash(self): + manifest = self._make_manifest( + { + "no_hash.png": ComparisonImageResult(status="changed", base_hash="abc"), + "has_hash.png": ComparisonImageResult( + status="changed", head_hash="def", base_hash="ghi" + ), + } + ) + fps = _build_comparison_fingerprints(manifest) + assert fps == {("has_hash.png", "changed", "def")} + + def test_skips_renamed_with_missing_hash_or_previous_name(self): + manifest = self._make_manifest( + { + "no_hash.png": ComparisonImageResult( + status="renamed", previous_image_file_name="old.png" + ), + "no_prev.png": ComparisonImageResult(status="renamed", head_hash="abc"), + "valid.png": ComparisonImageResult( + status="renamed", head_hash="def", previous_image_file_name="old_valid.png" + ), + } + ) + fps = _build_comparison_fingerprints(manifest) + assert fps == {("valid.png", "renamed", "def", "old_valid.png")} + def _mock_session_with_manifests(manifests_by_key: dict[str, bytes]) -> MagicMock: session = MagicMock() From 4023b4bfcd2f20c9e1be1de5039250078cfef04b Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Tue, 7 Apr 2026 15:42:05 -0700 Subject: [PATCH 3/9] feat(preprod): Show 'Auto-approved' label with tooltip for auto-approved snapshots --- .../endpoints/preprod_artifact_snapshot.py | 2 ++ .../project_preprod_snapshot_models.py | 1 + .../header/snapshotHeaderActions.tsx | 20 ++++++++++++++++--- .../app/views/preprod/types/snapshotTypes.ts | 1 + 4 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/sentry/preprod/api/endpoints/preprod_artifact_snapshot.py b/src/sentry/preprod/api/endpoints/preprod_artifact_snapshot.py index b52166275c2806..c5703b8cc59641 100644 --- a/src/sentry/preprod/api/endpoints/preprod_artifact_snapshot.py +++ b/src/sentry/preprod/api/endpoints/preprod_artifact_snapshot.py @@ -394,9 +394,11 @@ def get(self, request: Request, organization: Organization, snapshot_id: str) -> source="github", ) ) + is_auto_approved = any((a.extras or {}).get("auto_approval") is True for a in approved) approval_info = SnapshotApprovalInfo( status="approved", approvers=approver_list, + is_auto_approved=is_auto_approved, ) elif all_approvals: # If records exist but none are APPROVED, they must be NEEDS_APPROVAL diff --git a/src/sentry/preprod/api/models/snapshots/project_preprod_snapshot_models.py b/src/sentry/preprod/api/models/snapshots/project_preprod_snapshot_models.py index a9c3c8efc98cea..054b91e893471a 100644 --- a/src/sentry/preprod/api/models/snapshots/project_preprod_snapshot_models.py +++ b/src/sentry/preprod/api/models/snapshots/project_preprod_snapshot_models.py @@ -59,6 +59,7 @@ class SnapshotApprover(BaseModel): class SnapshotApprovalInfo(BaseModel): status: Literal["approved", "requires_approval"] approvers: list[SnapshotApprover] = [] + is_auto_approved: bool = False class SnapshotDetailsApiResponse(BaseModel): diff --git a/static/app/views/preprod/snapshots/header/snapshotHeaderActions.tsx b/static/app/views/preprod/snapshots/header/snapshotHeaderActions.tsx index ded2ee207b8817..417b3f7bd4ff7b 100644 --- a/static/app/views/preprod/snapshots/header/snapshotHeaderActions.tsx +++ b/static/app/views/preprod/snapshots/header/snapshotHeaderActions.tsx @@ -11,10 +11,12 @@ import {Client} from 'sentry/api'; import {ConfirmDelete} from 'sentry/components/confirmDelete'; import {DropdownMenu} from 'sentry/components/dropdownMenu'; import type {MenuItemProps} from 'sentry/components/dropdownMenu'; +import {Tooltip} from 'sentry/components/tooltip'; import { IconCheckmark, IconDelete, IconEllipsis, + IconInfo, IconRefresh, IconThumb, IconTimer, @@ -47,6 +49,7 @@ export function SnapshotHeaderActions({ const [isDeleting, setIsDeleting] = useState(false); const isApproved = data.approval_info?.status === 'approved'; + const isAutoApproved = data.approval_info?.is_auto_approved ?? false; const approvers: AvatarUser[] = (data.approval_info?.approvers ?? []).map((a, i) => ({ id: a.id ?? `approver-${i}`, name: a.name ?? '', @@ -148,9 +151,20 @@ export function SnapshotHeaderActions({ {data.approval_info && (isApproved ? ( - }> - {t('Approved')} - + + }> + {isAutoApproved ? t('Auto-approved') : t('Approved')} + + {isAutoApproved && ( + + + + )} + {approvers.length > 0 && ( )} diff --git a/static/app/views/preprod/types/snapshotTypes.ts b/static/app/views/preprod/types/snapshotTypes.ts index fa9dbfb9480e3c..255b4e2fd3f100 100644 --- a/static/app/views/preprod/types/snapshotTypes.ts +++ b/static/app/views/preprod/types/snapshotTypes.ts @@ -36,6 +36,7 @@ export interface SnapshotApprover { export interface SnapshotApprovalInfo { approvers: SnapshotApprover[]; status: 'approved' | 'requires_approval'; + is_auto_approved?: boolean; } export interface SnapshotDetailsApiResponse { From 924609eaf07b2988ab2df0fb544b18d606724c99 Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Tue, 7 Apr 2026 15:43:26 -0700 Subject: [PATCH 4/9] fix(preprod): Fix Tooltip import path for auto-approved badge --- .../views/preprod/snapshots/header/snapshotHeaderActions.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/static/app/views/preprod/snapshots/header/snapshotHeaderActions.tsx b/static/app/views/preprod/snapshots/header/snapshotHeaderActions.tsx index 417b3f7bd4ff7b..0d4138963e7c3b 100644 --- a/static/app/views/preprod/snapshots/header/snapshotHeaderActions.tsx +++ b/static/app/views/preprod/snapshots/header/snapshotHeaderActions.tsx @@ -5,13 +5,13 @@ import {Tag} from '@sentry/scraps/badge'; import {Button} from '@sentry/scraps/button'; import {Flex} from '@sentry/scraps/layout'; import {Text} from '@sentry/scraps/text'; +import {Tooltip} from '@sentry/scraps/tooltip'; import {addErrorMessage, addSuccessMessage} from 'sentry/actionCreators/indicator'; import {Client} from 'sentry/api'; import {ConfirmDelete} from 'sentry/components/confirmDelete'; import {DropdownMenu} from 'sentry/components/dropdownMenu'; import type {MenuItemProps} from 'sentry/components/dropdownMenu'; -import {Tooltip} from 'sentry/components/tooltip'; import { IconCheckmark, IconDelete, From 094fea0b7ae3018dbd14f9f2323197ee6b876623 Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Tue, 7 Apr 2026 15:44:28 -0700 Subject: [PATCH 5/9] fix(preprod): Fix vertical alignment of auto-approved info icon --- .../views/preprod/snapshots/header/snapshotHeaderActions.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/static/app/views/preprod/snapshots/header/snapshotHeaderActions.tsx b/static/app/views/preprod/snapshots/header/snapshotHeaderActions.tsx index 0d4138963e7c3b..44761cf64f2e87 100644 --- a/static/app/views/preprod/snapshots/header/snapshotHeaderActions.tsx +++ b/static/app/views/preprod/snapshots/header/snapshotHeaderActions.tsx @@ -161,7 +161,9 @@ export function SnapshotHeaderActions({ 'Automatically approved because the changes match a previously approved build on this PR.' )} > - + + + )} From 83efc2e923aaef759212d13f08ea2e44ace9405e Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Tue, 7 Apr 2026 16:04:45 -0700 Subject: [PATCH 6/9] fix(preprod): Fix mypy errors in snapshot auto-approval --- src/sentry/preprod/snapshots/tasks.py | 3 ++- tests/sentry/preprod/snapshots/test_auto_approve.py | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/sentry/preprod/snapshots/tasks.py b/src/sentry/preprod/snapshots/tasks.py index 9e6afe4ec3cbc7..c06285e611678b 100644 --- a/src/sentry/preprod/snapshots/tasks.py +++ b/src/sentry/preprod/snapshots/tasks.py @@ -7,6 +7,7 @@ import orjson from django.db import IntegrityError from django.utils import timezone +from objectstore_client import Session from objectstore_client.client import RequestError from pydantic import ValidationError from taskbroker_client.retry import Retry @@ -134,7 +135,7 @@ def _build_comparison_fingerprints(manifest: ComparisonManifest) -> set[tuple[st def _try_auto_approve_snapshot( head_artifact: PreprodArtifact, comparison_manifest: ComparisonManifest, - session: object, + session: Session, ) -> bool: cc = head_artifact.commit_comparison if not cc or not cc.pr_number or not cc.head_repo_name: diff --git a/tests/sentry/preprod/snapshots/test_auto_approve.py b/tests/sentry/preprod/snapshots/test_auto_approve.py index 38115dc262b43e..683e3c78e7d49c 100644 --- a/tests/sentry/preprod/snapshots/test_auto_approve.py +++ b/tests/sentry/preprod/snapshots/test_auto_approve.py @@ -311,6 +311,7 @@ def test_auto_approves_when_fingerprints_match(self): approval_status=PreprodComparisonApproval.ApprovalStatus.APPROVED, ) assert approval.approved_by_id is None + assert approval.extras is not None assert approval.extras["auto_approval"] is True assert approval.extras["prev_approved_artifact_id"] == sibling.id From 8a77ec7843f50aca51370a48e24452f47c92ef2f Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Tue, 7 Apr 2026 16:30:48 -0700 Subject: [PATCH 7/9] ref(preprod): Remove unused return value and redundant tests from auto-approval --- src/sentry/preprod/snapshots/tasks.py | 17 +- .../preprod/snapshots/test_auto_approve.py | 215 +++--------------- 2 files changed, 40 insertions(+), 192 deletions(-) diff --git a/src/sentry/preprod/snapshots/tasks.py b/src/sentry/preprod/snapshots/tasks.py index c06285e611678b..cc284d7eccaef2 100644 --- a/src/sentry/preprod/snapshots/tasks.py +++ b/src/sentry/preprod/snapshots/tasks.py @@ -136,14 +136,14 @@ def _try_auto_approve_snapshot( head_artifact: PreprodArtifact, comparison_manifest: ComparisonManifest, session: Session, -) -> bool: +) -> None: cc = head_artifact.commit_comparison if not cc or not cc.pr_number or not cc.head_repo_name: - return False + return head_fingerprints = _build_comparison_fingerprints(comparison_manifest) if not head_fingerprints: - return False + return approved_sibling = ( PreprodArtifact.objects.filter( @@ -162,7 +162,7 @@ def _try_auto_approve_snapshot( ) if not approved_sibling: - return False + return sibling_comparison = ( PreprodSnapshotComparison.objects.filter( @@ -174,11 +174,11 @@ def _try_auto_approve_snapshot( ) if not sibling_comparison: - return False + return sibling_comparison_key = (sibling_comparison.extras or {}).get("comparison_key") if not sibling_comparison_key: - return False + return try: sibling_manifest = ComparisonManifest( @@ -193,7 +193,7 @@ def _try_auto_approve_snapshot( "comparison_key": sibling_comparison_key, }, ) - return False + return sibling_fingerprints = _build_comparison_fingerprints(sibling_manifest) @@ -205,7 +205,7 @@ def _try_auto_approve_snapshot( "sibling_artifact_id": approved_sibling.id, }, ) - return False + return PreprodComparisonApproval.objects.create( preprod_artifact=head_artifact, @@ -225,7 +225,6 @@ def _try_auto_approve_snapshot( "prev_approved_artifact_id": approved_sibling.id, }, ) - return True @instrumented_task( diff --git a/tests/sentry/preprod/snapshots/test_auto_approve.py b/tests/sentry/preprod/snapshots/test_auto_approve.py index 683e3c78e7d49c..6587d98caac7d9 100644 --- a/tests/sentry/preprod/snapshots/test_auto_approve.py +++ b/tests/sentry/preprod/snapshots/test_auto_approve.py @@ -42,69 +42,6 @@ def _make_manifest(self, images: dict[str, ComparisonImageResult]) -> Comparison images=images, ) - def test_extracts_changed_with_head_hash(self): - manifest = self._make_manifest( - { - "screen1.png": ComparisonImageResult( - status="changed", head_hash="abc", base_hash="def" - ), - } - ) - fps = _build_comparison_fingerprints(manifest) - assert fps == {("screen1.png", "changed", "abc")} - - def test_extracts_added_with_head_hash(self): - manifest = self._make_manifest( - { - "new_screen.png": ComparisonImageResult(status="added", head_hash="abc"), - } - ) - fps = _build_comparison_fingerprints(manifest) - assert fps == {("new_screen.png", "added", "abc")} - - def test_extracts_removed_without_hash(self): - manifest = self._make_manifest( - { - "old_screen.png": ComparisonImageResult(status="removed", base_hash="xyz"), - } - ) - fps = _build_comparison_fingerprints(manifest) - assert fps == {("old_screen.png", "removed")} - - def test_extracts_errored_without_hash(self): - manifest = self._make_manifest( - { - "broken.png": ComparisonImageResult(status="errored", reason="exceeds_pixel_limit"), - } - ) - fps = _build_comparison_fingerprints(manifest) - assert fps == {("broken.png", "errored")} - - def test_extracts_renamed_with_hash_and_previous_name(self): - manifest = self._make_manifest( - { - "new_name.png": ComparisonImageResult( - status="renamed", head_hash="abc", previous_image_file_name="old_name.png" - ), - } - ) - fps = _build_comparison_fingerprints(manifest) - assert fps == {("new_name.png", "renamed", "abc", "old_name.png")} - - def test_skips_unchanged(self): - manifest = self._make_manifest( - { - "stable.png": ComparisonImageResult( - status="unchanged", head_hash="aaa", base_hash="aaa" - ), - "changed.png": ComparisonImageResult( - status="changed", head_hash="bbb", base_hash="ccc" - ), - } - ) - fps = _build_comparison_fingerprints(manifest) - assert fps == {("changed.png", "changed", "bbb")} - def test_mixed_statuses(self): manifest = self._make_manifest( { @@ -302,15 +239,15 @@ def test_auto_approves_when_fingerprints_match(self): ) session = _mock_session_with_manifests({comp_key: comp_json}) - result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) + _try_auto_approve_snapshot(head_artifact, head_manifest, session) - assert result is True approval = PreprodComparisonApproval.objects.get( preprod_artifact=head_artifact, preprod_feature_type=PreprodComparisonApproval.FeatureType.SNAPSHOTS, approval_status=PreprodComparisonApproval.ApprovalStatus.APPROVED, ) assert approval.approved_by_id is None + assert approval.approved_at is not None assert approval.extras is not None assert approval.extras["auto_approval"] is True assert approval.extras["prev_approved_artifact_id"] == sibling.id @@ -346,9 +283,8 @@ def test_no_auto_approve_when_fingerprints_differ(self): ) session = _mock_session_with_manifests({comp_key: comp_json}) - result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) + _try_auto_approve_snapshot(head_artifact, head_manifest, session) - assert result is False assert not PreprodComparisonApproval.objects.filter( preprod_artifact=head_artifact, approval_status=PreprodComparisonApproval.ApprovalStatus.APPROVED, @@ -369,9 +305,11 @@ def test_no_auto_approve_when_no_pr_number(self): } ) session = MagicMock() - result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) - - assert result is False + _try_auto_approve_snapshot(head_artifact, head_manifest, session) + assert not PreprodComparisonApproval.objects.filter( + preprod_artifact=head_artifact, + approval_status=PreprodComparisonApproval.ApprovalStatus.APPROVED, + ).exists() def test_no_auto_approve_when_no_approved_sibling(self): cc = self.create_commit_comparison( @@ -390,9 +328,11 @@ def test_no_auto_approve_when_no_approved_sibling(self): } ) session = MagicMock() - result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) - - assert result is False + _try_auto_approve_snapshot(head_artifact, head_manifest, session) + assert not PreprodComparisonApproval.objects.filter( + preprod_artifact=head_artifact, + approval_status=PreprodComparisonApproval.ApprovalStatus.APPROVED, + ).exists() def test_no_auto_approve_when_no_changes(self): cc = self.create_commit_comparison( @@ -412,9 +352,11 @@ def test_no_auto_approve_when_no_changes(self): } ) session = MagicMock() - result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) - - assert result is False + _try_auto_approve_snapshot(head_artifact, head_manifest, session) + assert not PreprodComparisonApproval.objects.filter( + preprod_artifact=head_artifact, + approval_status=PreprodComparisonApproval.ApprovalStatus.APPROVED, + ).exists() def test_handles_missing_comparison_manifest(self): sibling_images = { @@ -445,9 +387,11 @@ def test_handles_missing_comparison_manifest(self): session = MagicMock() session.get.side_effect = Exception("Not found") - result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) - - assert result is False + _try_auto_approve_snapshot(head_artifact, head_manifest, session) + assert not PreprodComparisonApproval.objects.filter( + preprod_artifact=head_artifact, + approval_status=PreprodComparisonApproval.ApprovalStatus.APPROVED, + ).exists() def test_matches_on_app_id_and_build_config(self): shared_images = { @@ -477,106 +421,11 @@ def test_matches_on_app_id_and_build_config(self): } ) session = MagicMock() - result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) - - assert result is False - - def test_auto_approve_sets_approved_at(self): - shared_images = { - "screen1.png": ComparisonImageResult( - status="changed", head_hash="abc", base_hash="old1" - ), - } - _, comp_key, comp_json = self._create_approved_sibling( - pr_number=42, - comparison_images=shared_images, - ) - - cc = self.create_commit_comparison( - organization=self.organization, - pr_number=42, - head_repo_name="owner/repo", - ) - head_artifact = self.create_preprod_artifact( - project=self.project, - commit_comparison=cc, - app_id="com.example.app", - ) - - head_manifest = self._create_head_manifest( - { - "screen1.png": ComparisonImageResult( - status="changed", head_hash="abc", base_hash="new_base1" - ), - } - ) - - session = _mock_session_with_manifests({comp_key: comp_json}) _try_auto_approve_snapshot(head_artifact, head_manifest, session) - - approval = PreprodComparisonApproval.objects.get( + assert not PreprodComparisonApproval.objects.filter( preprod_artifact=head_artifact, approval_status=PreprodComparisonApproval.ApprovalStatus.APPROVED, - ) - assert approval.approved_at is not None - - def test_no_auto_approve_when_no_head_repo_name(self): - cc = self.create_commit_comparison( - organization=self.organization, - pr_number=42, - head_repo_name="", - ) - head_artifact = self.create_preprod_artifact( - project=self.project, - commit_comparison=cc, - ) - head_manifest = self._create_head_manifest( - { - "screen1.png": ComparisonImageResult(status="changed", head_hash="abc"), - } - ) - session = MagicMock() - result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) - - assert result is False - - def test_no_auto_approve_when_extra_changed_image_in_head(self): - sibling_images = { - "screen1.png": ComparisonImageResult( - status="changed", head_hash="abc", base_hash="old1" - ), - } - _, comp_key, comp_json = self._create_approved_sibling( - pr_number=42, - comparison_images=sibling_images, - ) - - cc = self.create_commit_comparison( - organization=self.organization, - pr_number=42, - head_repo_name="owner/repo", - ) - head_artifact = self.create_preprod_artifact( - project=self.project, - commit_comparison=cc, - app_id="com.example.app", - ) - - head_manifest = self._create_head_manifest( - { - "screen1.png": ComparisonImageResult( - status="changed", head_hash="abc", base_hash="old1" - ), - "screen2.png": ComparisonImageResult( - status="changed", head_hash="new_change", base_hash="old2" - ), - } - ) - - session = _mock_session_with_manifests({comp_key: comp_json}) - result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) - - assert result is False + ).exists() def test_no_auto_approve_when_sibling_not_approved_for_snapshots(self): cc = self.create_commit_comparison( @@ -629,9 +478,11 @@ def test_no_auto_approve_when_sibling_not_approved_for_snapshots(self): } ) session = MagicMock() - result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) - - assert result is False + _try_auto_approve_snapshot(head_artifact, head_manifest, session) + assert not PreprodComparisonApproval.objects.filter( + preprod_artifact=head_artifact, + approval_status=PreprodComparisonApproval.ApprovalStatus.APPROVED, + ).exists() def test_auto_approves_with_renamed_images(self): shared_images = { @@ -639,7 +490,7 @@ def test_auto_approves_with_renamed_images(self): status="renamed", head_hash="abc", previous_image_file_name="old_name.png" ), } - sibling, comp_key, comp_json = self._create_approved_sibling( + _, comp_key, comp_json = self._create_approved_sibling( pr_number=42, comparison_images=shared_images, ) @@ -664,9 +515,7 @@ def test_auto_approves_with_renamed_images(self): ) session = _mock_session_with_manifests({comp_key: comp_json}) - result = _try_auto_approve_snapshot(head_artifact, head_manifest, session) - - assert result is True + _try_auto_approve_snapshot(head_artifact, head_manifest, session) assert PreprodComparisonApproval.objects.filter( preprod_artifact=head_artifact, approval_status=PreprodComparisonApproval.ApprovalStatus.APPROVED, From f795348d72f132df315fa715ee2876092e20f6cc Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Tue, 7 Apr 2026 16:46:24 -0700 Subject: [PATCH 8/9] fix(preprod): Prevent auto-approve failure from reverting successful comparison Wrap _try_auto_approve_snapshot in try/except so an unhandled exception doesn't propagate to the outer BaseException handler, which would incorrectly mark an already-successful comparison as FAILED. --- src/sentry/preprod/snapshots/tasks.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/sentry/preprod/snapshots/tasks.py b/src/sentry/preprod/snapshots/tasks.py index cc284d7eccaef2..f3148f1985f2e9 100644 --- a/src/sentry/preprod/snapshots/tasks.py +++ b/src/sentry/preprod/snapshots/tasks.py @@ -695,7 +695,13 @@ def _fetch_hash(h: str) -> None: ): metrics.incr("preprod.snapshots.diff.zero_changes", sample_rate=1.0, tags=metric_tags) - _try_auto_approve_snapshot(head_artifact, comparison_manifest, session) + try: + _try_auto_approve_snapshot(head_artifact, comparison_manifest, session) + except Exception: + logger.exception( + "Auto-approve failed after successful comparison", + extra={"head_artifact_id": head_artifact_id}, + ) create_preprod_snapshot_status_check_task.apply_async( kwargs={ From 2086ee0128c61a961c051826c846ba19b4262f82 Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Wed, 8 Apr 2026 08:36:59 -0700 Subject: [PATCH 9/9] ref(preprod): Type fingerprint tuples with ImageFingerprint NamedTuple Replace raw tuple[str, ...] with a typed ImageFingerprint NamedTuple for better readability of the name, status, hash, and previous_image_file_name fields in snapshot auto-approval fingerprints. Co-Authored-By: Claude Opus 4.6 --- src/sentry/preprod/snapshots/tasks.py | 19 ++++++++++++++----- .../preprod/snapshots/test_auto_approve.py | 15 ++++++++------- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/sentry/preprod/snapshots/tasks.py b/src/sentry/preprod/snapshots/tasks.py index f3148f1985f2e9..d93b513e53817a 100644 --- a/src/sentry/preprod/snapshots/tasks.py +++ b/src/sentry/preprod/snapshots/tasks.py @@ -114,21 +114,30 @@ def _create_pixel_batches( return batches -def _build_comparison_fingerprints(manifest: ComparisonManifest) -> set[tuple[str, ...]]: - fingerprints: set[tuple[str, ...]] = set() +class ImageFingerprint(NamedTuple): + name: str + status: str + head_hash: str | None = None + previous_image_file_name: str | None = None + + +def _build_comparison_fingerprints(manifest: ComparisonManifest) -> set[ImageFingerprint]: + fingerprints: set[ImageFingerprint] = set() for name, image in manifest.images.items(): if image.status == "unchanged": continue if image.status in ("changed", "added"): if not image.head_hash: continue - fingerprints.add((name, image.status, image.head_hash)) + fingerprints.add(ImageFingerprint(name, image.status, image.head_hash)) elif image.status == "renamed": if not image.head_hash or not image.previous_image_file_name: continue - fingerprints.add((name, "renamed", image.head_hash, image.previous_image_file_name)) + fingerprints.add( + ImageFingerprint(name, "renamed", image.head_hash, image.previous_image_file_name) + ) else: - fingerprints.add((name, image.status)) + fingerprints.add(ImageFingerprint(name, image.status)) return fingerprints diff --git a/tests/sentry/preprod/snapshots/test_auto_approve.py b/tests/sentry/preprod/snapshots/test_auto_approve.py index 6587d98caac7d9..310c3617dc9d3a 100644 --- a/tests/sentry/preprod/snapshots/test_auto_approve.py +++ b/tests/sentry/preprod/snapshots/test_auto_approve.py @@ -12,6 +12,7 @@ ) from sentry.preprod.snapshots.models import PreprodSnapshotComparison, PreprodSnapshotMetrics from sentry.preprod.snapshots.tasks import ( + ImageFingerprint, _build_comparison_fingerprints, _try_auto_approve_snapshot, ) @@ -61,11 +62,11 @@ def test_mixed_statuses(self): ) fps = _build_comparison_fingerprints(manifest) assert fps == { - ("changed.png", "changed", "b"), - ("added.png", "added", "d"), - ("removed.png", "removed"), - ("errored.png", "errored"), - ("renamed.png", "renamed", "f", "old.png"), + ImageFingerprint("changed.png", "changed", "b"), + ImageFingerprint("added.png", "added", "d"), + ImageFingerprint("removed.png", "removed"), + ImageFingerprint("errored.png", "errored"), + ImageFingerprint("renamed.png", "renamed", "f", "old.png"), } def test_empty_manifest_returns_empty_set(self): @@ -83,7 +84,7 @@ def test_skips_changed_with_missing_head_hash(self): } ) fps = _build_comparison_fingerprints(manifest) - assert fps == {("has_hash.png", "changed", "def")} + assert fps == {ImageFingerprint("has_hash.png", "changed", "def")} def test_skips_renamed_with_missing_hash_or_previous_name(self): manifest = self._make_manifest( @@ -98,7 +99,7 @@ def test_skips_renamed_with_missing_hash_or_previous_name(self): } ) fps = _build_comparison_fingerprints(manifest) - assert fps == {("valid.png", "renamed", "def", "old_valid.png")} + assert fps == {ImageFingerprint("valid.png", "renamed", "def", "old_valid.png")} def _mock_session_with_manifests(manifests_by_key: dict[str, bytes]) -> MagicMock: