Add time-based commit filtering for suspect commit detection

Author: nora-shap

src/sentry/integrations/utils/commit_context.py

@@ -3,7 +3,7 @@
 import logging
 from collections.abc import Mapping, Sequence
 from dataclasses import asdict
-from datetime import datetime, timedelta, timezone
+from datetime import datetime, timedelta
 from typing import Any
 
 from django.utils.datastructures import OrderedSet
@@ -36,13 +36,17 @@
 logger = logging.getLogger("sentry.tasks.process_commit_context")
 
 
+MAX_COMMIT_AGE_DAYS = 60
+
+
 def find_commit_context_for_event_all_frames(
     code_mappings: Sequence[RepositoryProjectPathConfig],
     frames: Sequence[Mapping[str, Any]],
     organization_id: int,
     project_id: int,
     platform: str,
     sdk_name: str | None,
+    group_first_seen: datetime,
     extra: dict[str, Any],
 ) -> tuple[FileBlameInfo | None, IntegrationInstallation | None]:
     """
@@ -78,14 +82,23 @@ def find_commit_context_for_event_all_frames(
         extra=extra,
     )
 
-    most_recent_blame = max(file_blames, key=lambda blame: blame.commit.committedDate, default=None)
-    # Only return suspect commits that are less than 2 months old
-    selected_blame = (
-        most_recent_blame
-        if most_recent_blame
-        and is_date_less_than_two_months(most_recent_blame.commit.committedDate)
-        else None
-    )
+    # Sort blames by commit date (most recent first)
+    sorted_blames = sorted(file_blames, key=lambda blame: blame.commit.committedDate, reverse=True)
+
+    selected_blame = None
+    for blame in sorted_blames:
+        commit_date = blame.commit.committedDate
+
+        if is_too_young(group_first_seen, commit_date):
+            # Skip commits that happened after the issue was first seen
+            continue
+        elif is_too_old(group_first_seen, commit_date):
+            # Stop checking - all remaining commits will be even older
+            break
+        else:
+            # Found a valid commit within the time window
+            selected_blame = blame
+            break
 
     selected_install, selected_provider = (
         integration_to_install_mapping[selected_blame.code_mapping.organization_integration_id]
@@ -95,7 +108,7 @@ def find_commit_context_for_event_all_frames(
 
     _record_commit_context_all_frames_analytics(
         selected_blame=selected_blame,
-        most_recent_blame=most_recent_blame,
+        most_recent_blame=sorted_blames[0] if sorted_blames else None,
         organization_id=organization_id,
         project_id=project_id,
         extra=extra,
@@ -110,8 +123,23 @@ def find_commit_context_for_event_all_frames(
     return (selected_blame, selected_install)
 
 
-def is_date_less_than_two_months(date: datetime) -> bool:
-    return date > datetime.now(tz=timezone.utc) - timedelta(days=60)
+def is_too_young(group_first_seen: datetime, commit_date: datetime) -> bool:
+    """
+    Returns True if the commit happened after the issue was first seen.
+    These commits cannot have caused the issue.
+    """
+    return commit_date > group_first_seen
+
+
+def is_too_old(
+    group_first_seen: datetime, commit_date: datetime, max_age_days: int = MAX_COMMIT_AGE_DAYS
+) -> bool:
+    """
+    Returns True if the commit is too old to be considered relevant.
+    Commits older than max_age_days before the issue was first seen are ignored.
+    """
+    time_threshold = group_first_seen - timedelta(days=max_age_days)
+    return commit_date < time_threshold
 
 
 def get_or_create_commit_from_blame(

src/sentry/tasks/commit_context.py

@@ -25,6 +25,7 @@
 from sentry.locks import locks
 from sentry.models.commit import Commit
 from sentry.models.commitauthor import CommitAuthor
+from sentry.models.group import Group
 from sentry.models.groupowner import GroupOwner, GroupOwnerType, SuspectCommitStrategy
 from sentry.models.project import Project
 from sentry.models.projectownership import ProjectOwnership
@@ -75,7 +76,7 @@ def process_commit_context(
     sdk_name: str | None = None,
 ) -> None:
     """
-    For a given event, look at the first in_app frame, and if we can find who modified the line, we can then update who is assigned to the issue.
+    For a given event, look at ALL in_app frames, and if we can find who modified the line, we can then update who is assigned to the issue.
     """
     lock = locks.get(
         f"process-commit-context:{group_id}", duration=10, name="process_commit_context"
@@ -87,6 +88,7 @@ def process_commit_context(
             set_current_event_project(project_id)
 
             project = Project.objects.get_from_cache(id=project_id)
+            group = Group.objects.get_from_cache(id=group_id)
             set_tag("organization.slug", project.organization.slug)
 
             basic_logging_details = {
@@ -150,6 +152,7 @@ def process_commit_context(
                     project_id=project_id,
                     platform=event_platform,
                     sdk_name=sdk_name,
+                    group_first_seen=group.first_seen,
                     extra=basic_logging_details,
                 )
             except ApiError:

tests/sentry/tasks/test_commit_context.py

@@ -132,7 +132,7 @@ def setUp(self) -> None:
             lineno=30,
             commit=CommitInfo(
                 commitId="commit-id-old",
-                committedDate=datetime.now(tz=datetime_timezone.utc) - timedelta(days=62),
+                committedDate=datetime.now(tz=datetime_timezone.utc) - timedelta(days=70),
                 commitMessage="old commit message",
                 commitAuthorName=None,
                 commitAuthorEmail="old@localhost",
@@ -658,66 +658,122 @@ def test_failure_no_blames(
             },
         )
 
-    @patch("sentry.integrations.utils.commit_context.logger.info")
     @patch("sentry.tasks.groupowner.process_suspect_commits.delay")
     @patch("sentry.analytics.record")
     @patch(
         "sentry.integrations.github.integration.GitHubIntegration.get_commit_context_all_frames",
     )
-    def test_failure_old_blame(
-        self, mock_get_commit_context, mock_record, mock_process_suspect_commits, mock_logger_info
+    def test_time_threshold_filtering(
+        self, mock_get_commit_context, mock_record, mock_process_suspect_commits
     ):
         """
-        A failure case where the only blame returned is past the time threshold. We bail out and fall back
-        to the release-based suspect commits.
+        A hacky way to run a parameterized test in TestCase.
+        Tests the logic that filters commits by age relative to issue first_seen.
         """
-        mock_get_commit_context.return_value = [self.blame_too_old]
-        with self.tasks():
-            assert not GroupOwner.objects.filter(group=self.event.group).exists()
-            event_frames = get_frame_paths(self.event)
-            process_commit_context(
-                event_id=self.event.event_id,
-                event_platform=self.event.platform,
-                event_frames=event_frames,
-                group_id=self.event.group_id,
-                project_id=self.event.project_id,
-                sdk_name="sentry.python",
-            )
+        # Test cases: each tuple contains (test_case_name, blames_setup, group_first_seen_days_ago, expected_group_owner_exists, expected_commit_id)
+        test_cases = [
+            # All commits too young
+            ("all_commits_too_young", ["blame_recent"], 3, False, None),
+            # Early exit on too old
+            ("early_exit_too_old", ["blame_too_old", "blame_existing_commit"], 10, False, None),
+            # All outside threshold
+            ("mixed_young_then_old", ["blame_recent", "blame_too_old"], 10, False, None),
+            # Skip young, find valid
+            (
+                "skip_young_find_valid",
+                ["blame_recent", "blame_existing_commit"],
+                5,
+                True,
+                "existing-commit",
+            ),
+            # All valid, picks most recent
+            (
+                "all_valid_picks_most_recent",
+                ["blame_existing_commit", "blame_no_existing_commit"],
+                5,
+                True,
+                "existing-commit",
+            ),
+            # All commits too old
+            ("only_old_commits", ["blame_too_old"], 10, False, None),
+            # Empty blames
+            ("empty_blames", [], 10, False, None),
+        ]
 
-        assert not GroupOwner.objects.filter(group=self.event.group).exists()
-        mock_process_suspect_commits.assert_called_once_with(
-            event_id=self.event.event_id,
-            event_platform=self.event.platform,
-            event_frames=event_frames,
-            group_id=self.event.group_id,
-            project_id=self.event.project_id,
-            sdk_name="sentry.python",
+        existing_commit = self.create_commit(
+            project=self.project,
+            repo=self.repo,
+            author=self.commit_author,
+            key="existing-commit",
         )
+        existing_commit.update(message="")
+
+        # Map blame names to actual blame objects
+        blame_mapping = {
+            "blame_recent": self.blame_recent,
+            "blame_too_old": self.blame_too_old,
+            "blame_existing_commit": self.blame_existing_commit,
+            "blame_no_existing_commit": self.blame_no_existing_commit,
+        }
 
-        assert_any_analytics_event(
-            mock_record,
-            IntegrationsFailedToFetchCommitContextAllFrames(
-                organization_id=self.organization.id,
-                project_id=self.project.id,
-                group_id=self.event.group_id,
-                event_id=self.event.event_id,
-                num_frames=1,
-                num_successfully_mapped_frames=1,
-                reason="commit_too_old",
-            ),
-        )
+        for (
+            case_name,
+            blames_setup,
+            group_first_seen_days_ago,
+            expected_group_owner_exists,
+            expected_commit_id,
+        ) in test_cases:
+            with self.subTest(case=case_name):
+                # Reset mocks for each test case
+                mock_get_commit_context.reset_mock()
+                mock_record.reset_mock()
+
+                # Clean up any existing GroupOwners from previous test cases
+                GroupOwner.objects.filter(group=self.event.group).delete()
+
+                # Setup group first_seen
+                group_first_seen = datetime.now(tz=datetime_timezone.utc) - timedelta(
+                    days=group_first_seen_days_ago
+                )
+                self.event.group.first_seen = group_first_seen
+                self.event.group.save()
+
+                # Build the mock return value
+                mock_blames = [blame_mapping[blame_name] for blame_name in blames_setup]
+                mock_get_commit_context.return_value = mock_blames
+
+                with self.tasks():
+                    assert not GroupOwner.objects.filter(group=self.event.group).exists()
+                    event_frames = get_frame_paths(self.event)
+
+                    with self.options({"issues.suspect-commit-strategy": True}):
+                        process_commit_context(
+                            event_id=self.event.event_id,
+                            event_platform=self.event.platform,
+                            event_frames=event_frames,
+                            group_id=self.event.group_id,
+                            project_id=self.event.project_id,
+                            sdk_name="sentry.python",
+                        )
+
+                # Assert GroupOwner existence
+                group_owners = GroupOwner.objects.filter(group=self.event.group)
+                actual_exists = group_owners.exists()
+                self.assertEqual(
+                    actual_exists,
+                    expected_group_owner_exists,
+                    f"GroupOwner existence assertion failed for case: {case_name}",
+                )
 
-        mock_logger_info.assert_any_call(
-            "process_commit_context_all_frames.find_commit_context_failed",
-            extra={
-                "organization": self.organization.id,
-                "group": self.event.group_id,
-                "event": self.event.event_id,
-                "project_id": self.project.id,
-                "reason": "commit_too_old",
-                "num_frames": 1,
-            },
-        )
+                # Assert correct commit selected
+                if expected_group_owner_exists and expected_commit_id:
+                    created_commit = Commit.objects.get(key=expected_commit_id)
+                    group_owner = group_owners.first()
+                    self.assertEqual(
+                        group_owner.context["commitId"],
+                        created_commit.id,
+                        f"Wrong commit selected for case: {case_name}",
+                    )
 
     @patch("sentry.tasks.groupowner.process_suspect_commits.delay")
     @patch(