Skip to content

ref(pr-iter): post the missing-permissions comment through SCM #123918

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
joseph-sentry wants to merge 1 commit into
base: master
Choose a base branch
from
+89 −45
Open

ref(pr-iter): post the missing-permissions comment through SCM #123918

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 37 additions & 9 deletions src/sentry/seer/autofix/pr_iteration/missing_permissions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,16 @@
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 (
AiAutofixPrIterationMissingPermissionsEvent,
)
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,
Expand Down Expand Up @@ -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,
Expand All @@ -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


Expand Down Expand Up @@ -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(
Expand Down
88 changes: 52 additions & 36 deletions tests/sentry/seer/autofix/pr_iteration/test_missing_permissions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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),
)

@cursor cursor Bot Sep 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test SCM stub fails protocol check

Medium Severity

_patch_scm returns a MagicMock that Python 3.13 isinstance does not treat as CreatePullRequestCommentProtocol, so _post_comment always takes the unsupported_provider path. Happy-path tests never post a comment and fail or pass for the wrong reason.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 0beed77. Configure here.

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
Expand Down Expand Up @@ -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
Expand All @@ -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(
[
Expand All @@ -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)
Expand All @@ -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"]}}}
)
Expand All @@ -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(
[
Expand All @@ -247,78 +254,89 @@ 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
assert logs.records[0].__dict__["repository_id"] == 123

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"]}}}

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()

with patch(f"{MODULE}.locks.get", return_value=lock):
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):
Expand Down Expand Up @@ -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,
Expand Down
Loading