diff --git a/src/sentry/seer/autofix/pr_iteration/missing_permissions.py b/src/sentry/seer/autofix/pr_iteration/missing_permissions.py index 8f1675ac99ec..cf9ba89c5ad4 100644 --- a/src/sentry/seer/autofix/pr_iteration/missing_permissions.py +++ b/src/sentry/seer/autofix/pr_iteration/missing_permissions.py @@ -35,6 +35,8 @@ from typing import Any from django.utils import timezone +from scm import actions as scm_actions +from scm.types import CreatePullRequestCommentProtocol from sentry import analytics from sentry.analytics.events.pr_iteration_events import ( @@ -42,6 +44,7 @@ ) from sentry.locks import locks from sentry.models.organization import Organization +from sentry.scm.factory import new as make_scm from sentry.seer.agent.client_models import SeerRunState from sentry.seer.autofix.github_perms import ( MissingGithubPermissions, @@ -105,9 +108,29 @@ def repos_missing_permissions( return get_missing_permissions_by_repo(organization, repo_names) +def _comment_failed( + reason: str, + scopes_tag: str, + log_ctx: PrIterationLogContext, + log_fields: dict[str, Any], + *, + exc_info: bool = True, +) -> bool: + metrics.incr( + "autofix.pr_iteration.missing_permissions.comment_failed", + tags={"missing_scopes": scopes_tag, "reason": reason}, + ) + log_ctx.error( + "autofix.pr_iteration.missing_permissions.comment_failed", + exc_info=exc_info, + reason=reason, + **log_fields, + ) + return False + + def _post_comment( organization: Organization, - repo_name: str, pr_number: int, info: MissingGithubPermissions, log_ctx: PrIterationLogContext, @@ -126,16 +149,21 @@ def _post_comment( **log_fields, ) return False + try: - client = info.integration.get_installation(organization_id=organization.id).get_client() - client.create_comment(repo_name, str(pr_number), {"body": _comment_body(url)}) + scm = make_scm(organization.id, info.repository_id, referrer="seer") except Exception: - metrics.incr( - "autofix.pr_iteration.missing_permissions.comment_failed", - tags={"missing_scopes": scopes_tag}, + return _comment_failed("scm_init_failed", scopes_tag, log_ctx, log_fields) + + if not isinstance(scm, CreatePullRequestCommentProtocol): + return _comment_failed( + "unsupported_provider", scopes_tag, log_ctx, log_fields, exc_info=False ) - log_ctx.error("autofix.pr_iteration.missing_permissions.comment_failed", **log_fields) - return False + + try: + scm_actions.create_pull_request_comment(scm, str(pr_number), _comment_body(url)) + except Exception: + return _comment_failed("post_failed", scopes_tag, log_ctx, log_fields) return True @@ -333,7 +361,7 @@ def post_missing_permissions_comment( _skip(log_ctx, "raced", **log_fields) return - if not _post_comment(organization, repo_name, pr_number, info, log_ctx, log_fields): + if not _post_comment(organization, pr_number, info, log_ctx, log_fields): return record_missing_permissions_marker( diff --git a/tests/sentry/seer/autofix/pr_iteration/test_missing_permissions.py b/tests/sentry/seer/autofix/pr_iteration/test_missing_permissions.py index bedbe4b50b2a..923728f91767 100644 --- a/tests/sentry/seer/autofix/pr_iteration/test_missing_permissions.py +++ b/tests/sentry/seer/autofix/pr_iteration/test_missing_permissions.py @@ -2,6 +2,7 @@ from unittest.mock import MagicMock, patch import pytest +from scm.types import CreatePullRequestCommentProtocol from sentry.analytics.events.pr_iteration_events import ( AiAutofixPrIterationMissingPermissionsEvent, @@ -48,6 +49,19 @@ def _perms( ) +def _patch_scm(case: TestCase) -> tuple[MagicMock, MagicMock]: + scm_patcher = patch( + f"{MODULE}.make_scm", + return_value=MagicMock(spec=CreatePullRequestCommentProtocol), + ) + make_scm = scm_patcher.start() + case.addCleanup(scm_patcher.stop) + actions_patcher = patch(f"{MODULE}.scm_actions") + actions = actions_patcher.start() + case.addCleanup(actions_patcher.stop) + return make_scm, actions + + def _log_ctx(state: SeerRunState) -> PrIterationLogContext: return PrIterationLogContext.for_run( logging.getLogger(MODULE), state, organization_id=1, group_id=None @@ -158,16 +172,9 @@ def setUp(self) -> None: organization=self.organization, seer_run_state_id=RUN_ID, user_id=self.user.id ) - def _stub_client(self) -> MagicMock: - client = MagicMock() - patcher = patch.object( - RpcIntegration, - "get_installation", - return_value=MagicMock(get_client=MagicMock(return_value=client)), - ) - patcher.start() - self.addCleanup(patcher.stop) - return client + def _stub_scm(self) -> MagicMock: + self.mock_make_scm, actions = _patch_scm(self) + return actions def _post( self, *, integration_id: int = INTEGRATION_ID, queued_repository_id: int | None = None @@ -185,7 +192,7 @@ def _post( def test_comments_and_marks(self, mock_get_perms) -> None: mock_get_perms.return_value = {REPO_NAME: _perms(repository_id=123)} - client = self._stub_client() + actions = self._stub_scm() with assert_analytics_events( [ @@ -199,12 +206,12 @@ def test_comments_and_marks(self, mock_get_perms) -> None: ): self._post() - client.create_comment.assert_called_once() - repo_name, pr_number, payload = client.create_comment.call_args[0] - assert repo_name == REPO_NAME + actions.create_pull_request_comment.assert_called_once() + _, pr_number, body = actions.create_pull_request_comment.call_args[0] assert pr_number == "7" - assert "additional GitHub permissions" in payload["body"] - assert f"/settings/installations/{INTEGRATION_ID}/permissions/update" in payload["body"] + assert "additional GitHub permissions" in body + assert f"/settings/installations/{INTEGRATION_ID}/permissions/update" in body + assert self.mock_make_scm.call_args[0] == (self.organization.id, 123) self.seer_run.refresh_from_db() marker = get_missing_permissions_marker(self.seer_run, REPO_NAME) @@ -214,7 +221,7 @@ def test_comments_and_marks(self, mock_get_perms) -> None: def test_stays_silent_once_marked(self, mock_get_perms) -> None: mock_get_perms.return_value = {REPO_NAME: _perms()} - client = self._stub_client() + actions = self._stub_scm() self.seer_run.update( extras={MISSING_PERMISSIONS_EXTRA: {REPO_NAME: {"missing_scopes": ["contents"]}}} ) @@ -223,13 +230,13 @@ def test_stays_silent_once_marked(self, mock_get_perms) -> None: self._post() assert_not_analytics_event(mock_record, AiAutofixPrIterationMissingPermissionsEvent) - client.create_comment.assert_not_called() + actions.create_pull_request_comment.assert_not_called() def test_records_the_ids_resolved_here_not_the_ones_queued(self, mock_get_perms) -> None: mock_get_perms.return_value = { REPO_NAME: _perms(integration_id=INTEGRATION_ID, repository_id=123) } - self._stub_client() + self._stub_scm() with assert_analytics_events( [ @@ -247,12 +254,12 @@ def test_comments_and_logs_when_the_repo_was_re_pointed_while_queued( self, mock_get_perms ) -> None: mock_get_perms.return_value = {REPO_NAME: _perms(repository_id=123)} - client = self._stub_client() + actions = self._stub_scm() with self.assertLogs(MODULE, level="ERROR") as logs: self._post(queued_repository_id=456) - client.create_comment.assert_called_once() + actions.create_pull_request_comment.assert_called_once() assert len(logs.records) == 1 assert logs.records[0].msg == "autofix.pr_iteration.missing_permissions.repository_changed" assert logs.records[0].__dict__["queued_repository_id"] == 456 @@ -260,17 +267,17 @@ def test_comments_and_logs_when_the_repo_was_re_pointed_while_queued( def test_stays_silent_when_nothing_is_missing(self, mock_get_perms) -> None: mock_get_perms.return_value = {} - client = self._stub_client() + actions = self._stub_scm() self._post() - client.create_comment.assert_not_called() + actions.create_pull_request_comment.assert_not_called() self.seer_run.refresh_from_db() assert get_missing_permissions_marker(self.seer_run, REPO_NAME) is None def test_no_second_comment_when_a_racing_task_marks_first(self, mock_get_perms) -> None: mock_get_perms.return_value = {REPO_NAME: _perms()} - client = self._stub_client() + actions = self._stub_scm() def _mark(run: SeerRun) -> None: run.extras = {MISSING_PERMISSIONS_EXTRA: {REPO_NAME: {"missing_scopes": ["contents"]}}} @@ -278,30 +285,41 @@ def _mark(run: SeerRun) -> None: with patch.object(SeerRun, "refresh_from_db", autospec=True, side_effect=_mark): self._post() - client.create_comment.assert_not_called() + actions.create_pull_request_comment.assert_not_called() def test_stays_silent_when_run_deleted_before_marker_write(self, mock_get_perms) -> None: mock_get_perms.return_value = {REPO_NAME: _perms()} - client = self._stub_client() + actions = self._stub_scm() with patch.object(SeerRun, "refresh_from_db", side_effect=SeerRun.DoesNotExist): self._post() - client.create_comment.assert_not_called() + actions.create_pull_request_comment.assert_not_called() def test_no_marker_when_the_comment_fails(self, mock_get_perms) -> None: mock_get_perms.return_value = {REPO_NAME: _perms()} - client = self._stub_client() - client.create_comment.side_effect = Exception("nope") + actions = self._stub_scm() + actions.create_pull_request_comment.side_effect = Exception("nope") + + self._post() + + self.seer_run.refresh_from_db() + assert get_missing_permissions_marker(self.seer_run, REPO_NAME) is None + + def test_no_marker_when_the_provider_cannot_comment(self, mock_get_perms) -> None: + mock_get_perms.return_value = {REPO_NAME: _perms()} + actions = self._stub_scm() + self.mock_make_scm.return_value = object() self._post() + actions.create_pull_request_comment.assert_not_called() self.seer_run.refresh_from_db() assert get_missing_permissions_marker(self.seer_run, REPO_NAME) is None def test_raises_for_the_task_to_retry_when_the_lock_is_held(self, mock_get_perms) -> None: mock_get_perms.return_value = {REPO_NAME: _perms()} - client = self._stub_client() + actions = self._stub_scm() lock = MagicMock() lock.acquire.side_effect = UnableToAcquireLock() @@ -309,16 +327,16 @@ def test_raises_for_the_task_to_retry_when_the_lock_is_held(self, mock_get_perms with pytest.raises(UnableToAcquireLock): self._post() - client.create_comment.assert_not_called() + actions.create_pull_request_comment.assert_not_called() def test_stays_silent_when_no_seer_run(self, mock_get_perms) -> None: self.seer_run.delete() mock_get_perms.return_value = {REPO_NAME: _perms()} - client = self._stub_client() + actions = self._stub_scm() self._post() - client.create_comment.assert_not_called() + actions.create_pull_request_comment.assert_not_called() class ScopesTagTest(TestCase): @@ -380,9 +398,7 @@ def test_commented_tag_is_per_repo(self, mock_get_perms, mock_incr) -> None: mock_get_perms.return_value = { REPO_NAME: _perms(missing_scopes=["pull_requests", "contents"]) } - patcher = patch.object(RpcIntegration, "get_installation") - patcher.start() - self.addCleanup(patcher.stop) + _patch_scm(self) post_missing_permissions_comment( organization=self.organization,