From 6656b3477318964d0d68ef085e6fd1e1b3ab324a Mon Sep 17 00:00:00 2001 From: Alex Sohn Date: Fri, 24 Jul 2026 16:16:24 -0400 Subject: [PATCH 1/4] feat(pr-iteration): resolve all review comment threads after iteration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Succeeds the 🎉 react step (CW-1398). After an Autofix PR iteration completes, resolve all inline review comment threads that were part of that iteration's feedback, alongside the existing 👀 reaction removal. CW-1688 --- src/sentry/seer/autofix/on_completion_hook.py | 62 ++++-- .../feedback_sources/github_comment.py | 4 + src/sentry/tasks/seer/pr_iteration.py | 77 +++++++ .../test_autofix_on_completion_hook.py | 195 +++++++++++++++++- tests/sentry/tasks/seer/test_pr_iteration.py | 185 +++++++++++++++++ 5 files changed, 505 insertions(+), 18 deletions(-) diff --git a/src/sentry/seer/autofix/on_completion_hook.py b/src/sentry/seer/autofix/on_completion_hook.py index a7d8842908ad..df0884d40662 100644 --- a/src/sentry/seer/autofix/on_completion_hook.py +++ b/src/sentry/seer/autofix/on_completion_hook.py @@ -66,6 +66,7 @@ from sentry.tasks.seer.pr_iteration import ( _add_comment_reaction, _delete_own_comment_eyes_reaction, + _resolve_review_comment_threads, consume_queued_autofix_feedback, ) from sentry.utils import metrics @@ -90,11 +91,12 @@ } -def _record_completion_reaction(outcome: str) -> None: +def _record_completion_reaction(outcome: str, amount: int = 1) -> None: """Record where a completion-reaction attempt exited so silent drop-offs of the :tada: ack are visible in aggregate rather than invisible.""" metrics.incr( "autofix.on_completion_hook.completion_reaction", + amount=amount, tags={"outcome": outcome}, ) @@ -245,14 +247,13 @@ def _maybe_react_to_completed_iteration( run_id: int, state: SeerRunState, ) -> None: - """React :tada: on the comment(s) that triggered a completed iteration and - remove the trigger-time :eyes:. - - Only top-level ``@sentry`` PR comments are acked with :tada: — inline review - comments are resolvable threads acked separately (CW-1688). The trigger-time - :eyes: is removed from both, since both received it, completing the - :eyes:->:tada: swap on top-level comments and clearing the lingering :eyes: - on inline ones. + """Acknowledge the comment(s) that triggered a completed iteration. + + Top-level ``@sentry`` PR comments are acked with :tada:. Inline review + comments are acked by resolving their review thread (CW-1688), in addition + to the trigger-time :eyes: being removed. The :eyes: is removed from both + comment types, since both received it, completing the :eyes:->:tada: swap on + top-level comments and clearing the lingering :eyes: on inline ones. """ if not features.has("organizations:autofix-pr-iteration", organization=organization): return @@ -290,10 +291,14 @@ def _maybe_react_to_completed_iteration( _record_completion_reaction("no_pr_comment_sources") return - # Rate-limit-sensitive orgs skip the extra reaction-delete API calls. - delete_eyes = not is_github_rate_limit_sensitive(organization.slug) + # Rate-limit-sensitive orgs skip the extra reaction-delete / resolve API calls. + rate_limit_sensitive = is_github_rate_limit_sensitive(organization.slug) + delete_eyes = not rate_limit_sensitive scm_by_repo: dict[str, SourceCodeManager] = {} + # Inline review-comment node ids to resolve, grouped by (repo, PR) so we + # fetch each PR's threads once and resolve each shared thread once. + resolve_by_repo_pr: dict[tuple[str, int], list[str]] = {} for source in sources: comment_id = source.comment.id if comment_id is None: @@ -344,8 +349,8 @@ def _maybe_react_to_completed_iteration( pr_number = pr_state.pr_number source_type = source.type - # Inline review comments are acked by resolving the thread (CW-1688), - # not with :tada:; only top-level PR comments get the :tada:. + # Only top-level PR comments get the :tada:; inline review comments are + # acked by resolving their thread after this loop (CW-1688). if source_type == "github-pr-comment": _add_comment_reaction( scm, @@ -355,6 +360,12 @@ def _maybe_react_to_completed_iteration( reaction="hooray", ) _record_completion_reaction("reacted") + elif source_type == "github-pr-review-comment" and not rate_limit_sensitive: + unique_id = getattr(source.comment, "unique_id", None) + if unique_id is None: + _record_completion_reaction("resolve_no_unique_id") + else: + resolve_by_repo_pr.setdefault((repo_name, pr_number), []).append(unique_id) if delete_eyes: _delete_own_comment_eyes_reaction( scm, @@ -363,6 +374,31 @@ def _maybe_react_to_completed_iteration( comment_id=comment_id, ) + if rate_limit_sensitive and any( + source.type == "github-pr-review-comment" for source in sources + ): + _record_completion_reaction("resolve_rate_limited") + + for (repo_name, pr_number), unique_ids in resolve_by_repo_pr.items(): + result = _resolve_review_comment_threads( + scm_by_repo[repo_name], + pr_number=pr_number, + comment_unique_ids=unique_ids, + ) + if result.unsupported_provider: + _record_completion_reaction("resolve_unsupported_provider") + elif result.failed: + _record_completion_reaction("resolve_failed") + else: + if result.resolved: + _record_completion_reaction("resolved", result.resolved) + if result.already_resolved: + _record_completion_reaction( + "resolve_skipped_already_resolved", result.already_resolved + ) + if result.not_found: + _record_completion_reaction("resolve_thread_not_found", result.not_found) + @classmethod def find_latest_artifact_for_step(cls, state: SeerRunState, key: str) -> Artifact | None: for block in reversed(state.blocks): diff --git a/src/sentry/seer/autofix/pr_iteration/feedback_sources/github_comment.py b/src/sentry/seer/autofix/pr_iteration/feedback_sources/github_comment.py index 443667b0c432..fca01eff5b35 100644 --- a/src/sentry/seer/autofix/pr_iteration/feedback_sources/github_comment.py +++ b/src/sentry/seer/autofix/pr_iteration/feedback_sources/github_comment.py @@ -34,6 +34,10 @@ class GithubPullRequestReviewComment(GithubIssueComment): line: int | None = None start_line: int | None = None diff_hunk: str | None = None + # GitHub GraphQL node id (``PRRC_…``) of the review comment, used at completion + # time to translate the comment to its review thread and resolve it (CW-1688). + # Optional so feedback serialized before this field predates it. + unique_id: str | None = None def _blocks_feedback(blocks: Sequence[Any]) -> list[Any]: diff --git a/src/sentry/tasks/seer/pr_iteration.py b/src/sentry/tasks/seer/pr_iteration.py index 7b58e0c6e384..749ae265003a 100644 --- a/src/sentry/tasks/seer/pr_iteration.py +++ b/src/sentry/tasks/seer/pr_iteration.py @@ -1,6 +1,8 @@ from __future__ import annotations import logging +from collections.abc import Collection +from dataclasses import dataclass from datetime import timedelta from typing import Any @@ -18,15 +20,18 @@ GetAuthenticatedActorProtocol, GetPullRequestCommentReactionsProtocol, GetPullRequestReviewProtocol, + GetPullRequestReviewThreadsProtocol, GetRepositoryUserPermissionProtocol, GetReviewCommentReactionsProtocol, GetReviewCommentsProtocol, PaginationParams, Reaction, ReactionResult, + ResolveReviewThreadProtocol, ResourceId, Review, ReviewComment, + ReviewThread, ) from taskbroker_client.retry import Retry @@ -367,6 +372,77 @@ def _own_eyes_reaction_ids(reactions: list[ReactionResult], actor_id: ResourceId logger.exception("autofix.pr_iteration.completion_reaction.delete_eyes_failed") +@dataclass +class ResolveReviewThreadsResult: + resolved: int = 0 + already_resolved: int = 0 + not_found: int = 0 + unsupported_provider: bool = False + failed: bool = False + + +def _resolve_review_comment_threads( + scm: SourceCodeManager, + *, + pr_number: int, + comment_unique_ids: Collection[str], +) -> ResolveReviewThreadsResult: + """Resolve the review threads of this iteration's inline comments (CW-1688). + + Fetches every review thread on the PR once, maps each requested comment's + GraphQL node id to its owning thread, then resolves each unique unresolved + thread. Shared threads collapse to one resolve; already-resolved threads are + skipped. A GitHub failure is logged and swallowed so the completion hook keeps + running. + """ + if not ( + isinstance(scm, ResolveReviewThreadProtocol) + and isinstance(scm, GetPullRequestReviewThreadsProtocol) + ): + logger.warning("autofix.pr_iteration.completion_reaction.unsupported_provider") + return ResolveReviewThreadsResult(unsupported_provider=True) + + try: + threads: list[ReviewThread] = [] + cursor: str | None = None + while True: + pagination: PaginationParams | None = {"cursor": cursor} if cursor else None + result = scm_actions.get_pull_request_review_threads(scm, str(pr_number), pagination) + threads.extend(result["data"]) + cursor = result["meta"].get("next_cursor") + if not cursor: + break + + thread_by_comment: dict[str, ReviewThread] = {} + for thread in threads: + for comment in thread["comments"]: + unique_id = comment.get("unique_id") + if unique_id is not None: + thread_by_comment[unique_id] = thread + + outcome = ResolveReviewThreadsResult() + thread_ids_to_resolve: set[ResourceId] = set() + already_resolved_ids: set[ResourceId] = set() + for comment_unique_id in comment_unique_ids: + owning_thread = thread_by_comment.get(comment_unique_id) + if owning_thread is None: + outcome.not_found += 1 + continue + if owning_thread["is_resolved"]: + already_resolved_ids.add(owning_thread["id"]) + else: + thread_ids_to_resolve.add(owning_thread["id"]) + + outcome.already_resolved = len(already_resolved_ids) + for thread_id in thread_ids_to_resolve: + scm_actions.resolve_review_thread(scm, str(pr_number), str(thread_id)) + outcome.resolved += 1 + return outcome + except Exception: + logger.exception("autofix.pr_iteration.completion_reaction.resolve_failed") + return ResolveReviewThreadsResult(failed=True) + + def _comment_pr_iteration_ineligible( client: Any, *, @@ -706,6 +782,7 @@ def _build_review_feedback( start_line=_diff_line_number(comment.get("start_line")), diff_hunk=comment.get("diff_hunk"), user=GithubPrCommentUser(login=author["username"] if author else None), + unique_id=comment.get("unique_id"), ) source = GithubPrReviewCommentFeedbackSource( comment=review_comment, diff --git a/tests/sentry/seer/autofix/test_autofix_on_completion_hook.py b/tests/sentry/seer/autofix/test_autofix_on_completion_hook.py index 3c6de55e07aa..0ad9c065ab60 100644 --- a/tests/sentry/seer/autofix/test_autofix_on_completion_hook.py +++ b/tests/sentry/seer/autofix/test_autofix_on_completion_hook.py @@ -27,6 +27,7 @@ from sentry.seer.autofix.utils import AutofixStoppingPoint from sentry.seer.models import AutofixHandoffPoint, SeerAutomationHandoffConfiguration from sentry.sentry_apps.utils.webhooks import SeerActionType +from sentry.tasks.seer.pr_iteration import ResolveReviewThreadsResult from sentry.testutils.cases import TestCase from sentry.testutils.helpers.datetime import before_now @@ -897,9 +898,13 @@ def _top_level_source(self, comment_id: int = 111) -> GithubPrCommentFeedbackSou repo_name="owner/repo", ) - def _review_source(self, comment_id: int = 222) -> GithubPrReviewCommentFeedbackSource: + def _review_source( + self, comment_id: int = 222, unique_id: str | None = "PRRC_222" + ) -> GithubPrReviewCommentFeedbackSource: return GithubPrReviewCommentFeedbackSource( - comment=GithubPullRequestReviewComment(id=comment_id, body="inline feedback"), + comment=GithubPullRequestReviewComment( + id=comment_id, body="inline feedback", unique_id=unique_id + ), ) def _state_with( @@ -923,13 +928,19 @@ def _state_with( ) return state + @patch(f"{REACT_PATH}.is_github_rate_limit_sensitive", return_value=False) + @patch(f"{REACT_PATH}._resolve_review_comment_threads") @patch(f"{REACT_PATH}.make_scm") @patch(f"{REACT_PATH}._add_comment_reaction") - def test_reacts_hooray_on_top_level_comment_only(self, mock_react, mock_make_scm): + def test_reacts_hooray_on_top_level_comment_only( + self, mock_react, mock_make_scm, mock_resolve, mock_sensitive + ): # A review comment is present alongside the top-level comment; only the - # top-level one is acked (review comments are handled by CW-1688). + # top-level one is acked with :tada: while the review comment's thread is + # resolved (CW-1688). scm = MagicMock() mock_make_scm.return_value = scm + mock_resolve.return_value = ResolveReviewThreadsResult(resolved=1) state = self._state_with([self._top_level_source(111), self._review_source(222)]) with self.feature("organizations:autofix-pr-iteration"): @@ -944,6 +955,9 @@ def test_reacts_hooray_on_top_level_comment_only(self, mock_react, mock_make_scm assert mock_react.call_args.kwargs["reaction"] == "hooray" assert mock_react.call_args.kwargs["pr_number"] == 7 + # The review comment's thread is resolved alongside the top-level :tada:. + mock_resolve.assert_called_once() + @patch(f"{REACT_PATH}.make_scm") @patch(f"{REACT_PATH}._add_comment_reaction") def test_noop_on_error_status(self, mock_react, mock_make_scm): @@ -1087,11 +1101,12 @@ def test_deletes_own_eyes_on_top_level_comment( assert mock_delete_eyes.call_args.kwargs["comment_id"] == 111 @patch(f"{REACT_PATH}.is_github_rate_limit_sensitive", return_value=False) + @patch(f"{REACT_PATH}._resolve_review_comment_threads") @patch(f"{REACT_PATH}._delete_own_comment_eyes_reaction") @patch(f"{REACT_PATH}.make_scm") @patch(f"{REACT_PATH}._add_comment_reaction") def test_deletes_own_eyes_on_review_comment_without_hooray( - self, mock_react, mock_make_scm, mock_delete_eyes, mock_sensitive + self, mock_react, mock_make_scm, mock_delete_eyes, mock_resolve, mock_sensitive ): # An inline review comment gets its trigger-time :eyes: removed, but no # :tada: (its thread is resolved separately, CW-1688). @@ -1137,3 +1152,173 @@ def test_skips_eyes_delete_for_rate_limit_sensitive_org( # :tada: is still added, but the eyes-delete is skipped entirely. assert mock_react.call_args.kwargs["reaction"] == "hooray" mock_delete_eyes.assert_not_called() + + @patch(f"{REACT_PATH}.is_github_rate_limit_sensitive", return_value=False) + @patch(f"{REACT_PATH}._resolve_review_comment_threads") + @patch(f"{REACT_PATH}._delete_own_comment_eyes_reaction") + @patch(f"{REACT_PATH}.make_scm") + @patch(f"{REACT_PATH}._add_comment_reaction") + def test_resolves_review_comment_thread( + self, mock_react, mock_make_scm, mock_delete_eyes, mock_resolve, mock_sensitive + ): + scm = MagicMock() + mock_make_scm.return_value = scm + mock_resolve.return_value = ResolveReviewThreadsResult(resolved=1) + state = self._state_with( + [self._top_level_source(111), self._review_source(222, unique_id="PRRC_222")] + ) + + with self.feature("organizations:autofix-pr-iteration"): + AutofixOnCompletionHook._maybe_react_to_completed_iteration( + self.organization, 123, state + ) + + # Top-level :tada: is unaffected. + assert mock_react.call_count == 1 + assert mock_react.call_args.kwargs["comment_id"] == 111 + + mock_resolve.assert_called_once() + assert mock_resolve.call_args.args[0] is scm + assert mock_resolve.call_args.kwargs["pr_number"] == 7 + assert mock_resolve.call_args.kwargs["comment_unique_ids"] == ["PRRC_222"] + + @patch(f"{REACT_PATH}.is_github_rate_limit_sensitive", return_value=False) + @patch(f"{REACT_PATH}._resolve_review_comment_threads") + @patch(f"{REACT_PATH}._delete_own_comment_eyes_reaction") + @patch(f"{REACT_PATH}.make_scm") + @patch(f"{REACT_PATH}._add_comment_reaction") + def test_batches_multiple_review_comments_per_pr( + self, mock_react, mock_make_scm, mock_delete_eyes, mock_resolve, mock_sensitive + ): + scm = MagicMock() + mock_make_scm.return_value = scm + mock_resolve.return_value = ResolveReviewThreadsResult(resolved=2) + state = self._state_with( + [ + self._review_source(222, unique_id="PRRC_222"), + self._review_source(333, unique_id="PRRC_333"), + ] + ) + + with self.feature("organizations:autofix-pr-iteration"): + AutofixOnCompletionHook._maybe_react_to_completed_iteration( + self.organization, 123, state + ) + + # One call per PR carrying every unique_id, not one call per comment. + mock_resolve.assert_called_once() + assert mock_resolve.call_args.kwargs["pr_number"] == 7 + assert mock_resolve.call_args.kwargs["comment_unique_ids"] == ["PRRC_222", "PRRC_333"] + + @patch(f"{REACT_PATH}.is_github_rate_limit_sensitive", return_value=False) + @patch(f"{REACT_PATH}._resolve_review_comment_threads") + @patch(f"{REACT_PATH}._delete_own_comment_eyes_reaction") + @patch(f"{REACT_PATH}.make_scm") + @patch(f"{REACT_PATH}._add_comment_reaction") + def test_skips_review_resolve_when_repo_ambiguous_multi_repo( + self, mock_react, mock_make_scm, mock_delete_eyes, mock_resolve, mock_sensitive + ): + # Review-comment sources don't carry ``repo_name``; with more than one repo + # in the run their repo can't be inferred, so resolution is skipped. + scm = MagicMock() + mock_make_scm.return_value = scm + state = run_state( + blocks=[ + self._synced_pr_iteration_block([self._review_source(222, unique_id="PRRC_222")]) + ] + ) + state.repo_pr_states = { + "owner/repo": RepoPRState(repo_name="owner/repo", pr_number=7, commit_sha="synced-sha"), + "owner/other": RepoPRState( + repo_name="owner/other", pr_number=9, commit_sha="other-sha" + ), + } + + with self.feature("organizations:autofix-pr-iteration"): + AutofixOnCompletionHook._maybe_react_to_completed_iteration( + self.organization, 123, state + ) + + mock_resolve.assert_not_called() + + @patch(f"{REACT_PATH}.is_github_rate_limit_sensitive", return_value=False) + @patch(f"{REACT_PATH}._resolve_review_comment_threads") + @patch(f"{REACT_PATH}._delete_own_comment_eyes_reaction") + @patch(f"{REACT_PATH}.make_scm") + @patch(f"{REACT_PATH}._add_comment_reaction") + def test_skips_resolve_for_legacy_source_without_unique_id( + self, mock_react, mock_make_scm, mock_delete_eyes, mock_resolve, mock_sensitive + ): + # A source serialized before unique_id was stored still gets :eyes: removed + # but is not resolvable. + scm = MagicMock() + mock_make_scm.return_value = scm + state = self._state_with([self._review_source(222, unique_id=None)]) + + with self.feature("organizations:autofix-pr-iteration"): + AutofixOnCompletionHook._maybe_react_to_completed_iteration( + self.organization, 123, state + ) + + mock_resolve.assert_not_called() + # :eyes: removal still happens for the inline comment. + assert mock_delete_eyes.call_count == 1 + assert mock_delete_eyes.call_args.kwargs["comment_id"] == 222 + + @patch(f"{REACT_PATH}.is_github_rate_limit_sensitive", return_value=True) + @patch(f"{REACT_PATH}._resolve_review_comment_threads") + @patch(f"{REACT_PATH}._delete_own_comment_eyes_reaction") + @patch(f"{REACT_PATH}.make_scm") + @patch(f"{REACT_PATH}._add_comment_reaction") + def test_skips_resolve_for_rate_limit_sensitive_org( + self, mock_react, mock_make_scm, mock_delete_eyes, mock_resolve, mock_sensitive + ): + scm = MagicMock() + mock_make_scm.return_value = scm + state = self._state_with([self._review_source(222, unique_id="PRRC_222")]) + + with self.feature("organizations:autofix-pr-iteration"): + AutofixOnCompletionHook._maybe_react_to_completed_iteration( + self.organization, 123, state + ) + + mock_resolve.assert_not_called() + + @patch(f"{REACT_PATH}._resolve_review_comment_threads") + @patch(f"{REACT_PATH}.make_scm") + @patch(f"{REACT_PATH}._add_comment_reaction") + def test_noop_resolve_when_feature_disabled(self, mock_react, mock_make_scm, mock_resolve): + state = self._state_with([self._review_source(222, unique_id="PRRC_222")]) + AutofixOnCompletionHook._maybe_react_to_completed_iteration(self.organization, 123, state) + mock_resolve.assert_not_called() + + @patch(f"{REACT_PATH}._resolve_review_comment_threads") + @patch(f"{REACT_PATH}.make_scm") + @patch(f"{REACT_PATH}._add_comment_reaction") + def test_noop_resolve_on_error_status(self, mock_react, mock_make_scm, mock_resolve): + state = self._state_with([self._review_source(222, unique_id="PRRC_222")], status="error") + with self.feature("organizations:autofix-pr-iteration"): + AutofixOnCompletionHook._maybe_react_to_completed_iteration( + self.organization, 123, state + ) + mock_resolve.assert_not_called() + + @patch(f"{REACT_PATH}._resolve_review_comment_threads") + @patch(f"{REACT_PATH}.make_scm") + @patch(f"{REACT_PATH}._add_comment_reaction") + def test_skips_resolve_when_repo_name_ambiguous(self, mock_react, mock_make_scm, mock_resolve): + self.create_repo( + project=self.project, + provider="integrations:gitlab", + external_id="456", + name="owner/repo", + ) + state = self._state_with([self._review_source(222, unique_id="PRRC_222")]) + + with self.feature("organizations:autofix-pr-iteration"): + AutofixOnCompletionHook._maybe_react_to_completed_iteration( + self.organization, 123, state + ) + + mock_make_scm.assert_not_called() + mock_resolve.assert_not_called() diff --git a/tests/sentry/tasks/seer/test_pr_iteration.py b/tests/sentry/tasks/seer/test_pr_iteration.py index b8ccb1fae685..1d010a0d8c1d 100644 --- a/tests/sentry/tasks/seer/test_pr_iteration.py +++ b/tests/sentry/tasks/seer/test_pr_iteration.py @@ -22,8 +22,10 @@ from sentry.seer.autofix.pr_iteration.queue import QueuedAutofixFeedback from sentry.seer.models import SeerApiError from sentry.tasks.seer.pr_iteration import ( + _build_review_feedback, _delete_own_comment_eyes_reaction, _ineligible_pr_iteration_comment_body, + _resolve_review_comment_threads, consume_queued_autofix_feedback, trigger_consume_pr_iteration_feedback, trigger_pr_iteration_from_comment, @@ -1082,3 +1084,186 @@ def test_swallows_exceptions(self, mock_scm_actions: MagicMock) -> None: ) mock_scm_actions.delete_pull_request_comment_reaction.assert_not_called() + + +class _ResolveThreadScmProtocols: + """Method surface matching the resolve protocols so ``spec`` MagicMocks + satisfy the ``@runtime_checkable`` ``isinstance`` guards.""" + + def get_thread_id_from_review_comment_unique_id(self, *args: Any, **kwargs: Any) -> Any: ... + + def resolve_review_thread(self, *args: Any, **kwargs: Any) -> Any: ... + + def get_pull_request_review_threads(self, *args: Any, **kwargs: Any) -> Any: ... + + +class ResolveReviewCommentThreadsTest(TestCase): + def _scm(self) -> MagicMock: + return MagicMock(spec=_ResolveThreadScmProtocols) + + def _thread( + self, + thread_id: str, + comment_unique_ids: list[str], + *, + is_resolved: bool = False, + ) -> dict[str, Any]: + return { + "id": thread_id, + "is_resolved": is_resolved, + "is_outdated": False, + "file_path": "test.py", + "line": 1, + "start_line": None, + "comments": [{"id": uid, "unique_id": uid} for uid in comment_unique_ids], + } + + def _page( + self, threads: list[dict[str, Any]], next_cursor: str | None = None + ) -> dict[str, Any]: + return {"data": threads, "meta": {"next_cursor": next_cursor}} + + @patch(f"{TASK_PATH}.scm_actions") + def test_resolves_matching_threads(self, mock_scm_actions: MagicMock) -> None: + scm = self._scm() + mock_scm_actions.get_pull_request_review_threads.return_value = self._page( + [self._thread("PRRT_1", ["PRRC_a"]), self._thread("PRRT_2", ["PRRC_b"])] + ) + + result = _resolve_review_comment_threads(scm, pr_number=7, comment_unique_ids=["PRRC_a"]) + + assert result.resolved == 1 + mock_scm_actions.resolve_review_thread.assert_called_once_with(scm, "7", "PRRT_1") + + @patch(f"{TASK_PATH}.scm_actions") + def test_dedupes_shared_thread(self, mock_scm_actions: MagicMock) -> None: + scm = self._scm() + mock_scm_actions.get_pull_request_review_threads.return_value = self._page( + [self._thread("PRRT_1", ["PRRC_a", "PRRC_b"])] + ) + + result = _resolve_review_comment_threads( + scm, pr_number=7, comment_unique_ids=["PRRC_a", "PRRC_b"] + ) + + assert result.resolved == 1 + mock_scm_actions.resolve_review_thread.assert_called_once_with(scm, "7", "PRRT_1") + + @patch(f"{TASK_PATH}.scm_actions") + def test_skips_already_resolved(self, mock_scm_actions: MagicMock) -> None: + scm = self._scm() + mock_scm_actions.get_pull_request_review_threads.return_value = self._page( + [self._thread("PRRT_1", ["PRRC_a"], is_resolved=True)] + ) + + result = _resolve_review_comment_threads(scm, pr_number=7, comment_unique_ids=["PRRC_a"]) + + assert result.resolved == 0 + assert result.already_resolved == 1 + mock_scm_actions.resolve_review_thread.assert_not_called() + + @patch(f"{TASK_PATH}.scm_actions") + def test_unknown_unique_id_not_found(self, mock_scm_actions: MagicMock) -> None: + scm = self._scm() + mock_scm_actions.get_pull_request_review_threads.return_value = self._page( + [self._thread("PRRT_1", ["PRRC_a"])] + ) + + result = _resolve_review_comment_threads( + scm, pr_number=7, comment_unique_ids=["PRRC_missing"] + ) + + assert result.resolved == 0 + assert result.not_found == 1 + mock_scm_actions.resolve_review_thread.assert_not_called() + + @patch(f"{TASK_PATH}.scm_actions") + def test_pages_until_exhausted(self, mock_scm_actions: MagicMock) -> None: + scm = self._scm() + mock_scm_actions.get_pull_request_review_threads.side_effect = [ + self._page([self._thread("PRRT_1", ["PRRC_a"])], next_cursor="page-2"), + self._page([self._thread("PRRT_2", ["PRRC_b"])]), + ] + + result = _resolve_review_comment_threads(scm, pr_number=7, comment_unique_ids=["PRRC_b"]) + + assert result.resolved == 1 + assert mock_scm_actions.get_pull_request_review_threads.call_count == 2 + mock_scm_actions.resolve_review_thread.assert_called_once_with(scm, "7", "PRRT_2") + + @patch(f"{TASK_PATH}.scm_actions") + def test_noop_for_unsupported_provider(self, mock_scm_actions: MagicMock) -> None: + # A mock missing one protocol method fails the isinstance guard. + scm = MagicMock( + spec=["resolve_review_thread", "get_thread_id_from_review_comment_unique_id"] + ) + + result = _resolve_review_comment_threads(scm, pr_number=7, comment_unique_ids=["PRRC_a"]) + + assert result.unsupported_provider is True + mock_scm_actions.get_pull_request_review_threads.assert_not_called() + mock_scm_actions.resolve_review_thread.assert_not_called() + + @patch(f"{TASK_PATH}.scm_actions") + def test_swallows_exceptions(self, mock_scm_actions: MagicMock) -> None: + scm = self._scm() + mock_scm_actions.get_pull_request_review_threads.return_value = self._page( + [self._thread("PRRT_1", ["PRRC_a"])] + ) + mock_scm_actions.resolve_review_thread.side_effect = RuntimeError("boom") + + result = _resolve_review_comment_threads(scm, pr_number=7, comment_unique_ids=["PRRC_a"]) + + assert result.failed is True + + +class BuildReviewFeedbackTest(TestCase): + def _review_comment(self, unique_id: str | None) -> dict[str, Any]: + return { + "id": 222, + "unique_id": unique_id, + "url": "https://example.com/c/222", + "file_path": "test.py", + "body": "inline feedback", + "author": {"id": "1", "username": "octocat"}, + "created_at": None, + "diff_hunk": None, + "line": None, + "start_line": None, + "review_id": 55, + "author_association": None, + "commit_sha": None, + "head": None, + "thread_id": None, + } + + def test_carries_unique_id_onto_source(self) -> None: + feedback = _build_review_feedback( + [self._review_comment("PRRC_a")], + None, + review_id=55, + review_html_url=None, + review_state=None, + review_author=None, + author_is_bot=False, + ) + + assert len(feedback) == 1 + source = feedback[0].source + assert isinstance(source, GithubPrReviewCommentFeedbackSource) + assert source.comment.unique_id == "PRRC_a" + + def test_missing_unique_id_is_none(self) -> None: + feedback = _build_review_feedback( + [self._review_comment(None)], + None, + review_id=55, + review_html_url=None, + review_state=None, + review_author=None, + author_is_bot=False, + ) + + source = feedback[0].source + assert isinstance(source, GithubPrReviewCommentFeedbackSource) + assert source.comment.unique_id is None From c8fd936acfc51db5ad3d5a97ba335b2bcc82b061 Mon Sep 17 00:00:00 2001 From: Alex Sohn Date: Fri, 24 Jul 2026 16:25:36 -0400 Subject: [PATCH 2/4] ref(pr-iteration): condense comments and docstrings --- src/sentry/seer/autofix/on_completion_hook.py | 15 +++------------ .../feedback_sources/github_comment.py | 4 +--- src/sentry/tasks/seer/pr_iteration.py | 9 +-------- 3 files changed, 5 insertions(+), 23 deletions(-) diff --git a/src/sentry/seer/autofix/on_completion_hook.py b/src/sentry/seer/autofix/on_completion_hook.py index df0884d40662..eb7bfc98af6f 100644 --- a/src/sentry/seer/autofix/on_completion_hook.py +++ b/src/sentry/seer/autofix/on_completion_hook.py @@ -247,14 +247,7 @@ def _maybe_react_to_completed_iteration( run_id: int, state: SeerRunState, ) -> None: - """Acknowledge the comment(s) that triggered a completed iteration. - - Top-level ``@sentry`` PR comments are acked with :tada:. Inline review - comments are acked by resolving their review thread (CW-1688), in addition - to the trigger-time :eyes: being removed. The :eyes: is removed from both - comment types, since both received it, completing the :eyes:->:tada: swap on - top-level comments and clearing the lingering :eyes: on inline ones. - """ + """Acknowledge the comment(s) that triggered a completed iteration.""" if not features.has("organizations:autofix-pr-iteration", organization=organization): return @@ -296,8 +289,7 @@ def _maybe_react_to_completed_iteration( delete_eyes = not rate_limit_sensitive scm_by_repo: dict[str, SourceCodeManager] = {} - # Inline review-comment node ids to resolve, grouped by (repo, PR) so we - # fetch each PR's threads once and resolve each shared thread once. + # Inline review-comment node ids to resolve, grouped by (repo, PR). resolve_by_repo_pr: dict[tuple[str, int], list[str]] = {} for source in sources: comment_id = source.comment.id @@ -349,8 +341,7 @@ def _maybe_react_to_completed_iteration( pr_number = pr_state.pr_number source_type = source.type - # Only top-level PR comments get the :tada:; inline review comments are - # acked by resolving their thread after this loop (CW-1688). + # Only top-level PR comments get the :tada:; inline comments resolve below (CW-1688). if source_type == "github-pr-comment": _add_comment_reaction( scm, diff --git a/src/sentry/seer/autofix/pr_iteration/feedback_sources/github_comment.py b/src/sentry/seer/autofix/pr_iteration/feedback_sources/github_comment.py index fca01eff5b35..3c763039adff 100644 --- a/src/sentry/seer/autofix/pr_iteration/feedback_sources/github_comment.py +++ b/src/sentry/seer/autofix/pr_iteration/feedback_sources/github_comment.py @@ -34,9 +34,7 @@ class GithubPullRequestReviewComment(GithubIssueComment): line: int | None = None start_line: int | None = None diff_hunk: str | None = None - # GitHub GraphQL node id (``PRRC_…``) of the review comment, used at completion - # time to translate the comment to its review thread and resolve it (CW-1688). - # Optional so feedback serialized before this field predates it. + # GraphQL node id used to resolve the review thread at completion (CW-1688). unique_id: str | None = None diff --git a/src/sentry/tasks/seer/pr_iteration.py b/src/sentry/tasks/seer/pr_iteration.py index 749ae265003a..883f65cb561e 100644 --- a/src/sentry/tasks/seer/pr_iteration.py +++ b/src/sentry/tasks/seer/pr_iteration.py @@ -387,14 +387,7 @@ def _resolve_review_comment_threads( pr_number: int, comment_unique_ids: Collection[str], ) -> ResolveReviewThreadsResult: - """Resolve the review threads of this iteration's inline comments (CW-1688). - - Fetches every review thread on the PR once, maps each requested comment's - GraphQL node id to its owning thread, then resolves each unique unresolved - thread. Shared threads collapse to one resolve; already-resolved threads are - skipped. A GitHub failure is logged and swallowed so the completion hook keeps - running. - """ + """Resolve the review threads of this iteration's inline comments (CW-1688).""" if not ( isinstance(scm, ResolveReviewThreadProtocol) and isinstance(scm, GetPullRequestReviewThreadsProtocol) From 4a6c91c0e9e46c4041f2949199ee9a99017581e6 Mon Sep 17 00:00:00 2001 From: Alex Sohn Date: Fri, 24 Jul 2026 16:53:36 -0400 Subject: [PATCH 3/4] ref(pr-iteration): use iter_all_pages helper and simplify metrics --- src/sentry/seer/autofix/on_completion_hook.py | 20 +++++++++---------- src/sentry/tasks/seer/pr_iteration.py | 18 +++++++++-------- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/sentry/seer/autofix/on_completion_hook.py b/src/sentry/seer/autofix/on_completion_hook.py index eb7bfc98af6f..86cec0e52031 100644 --- a/src/sentry/seer/autofix/on_completion_hook.py +++ b/src/sentry/seer/autofix/on_completion_hook.py @@ -377,18 +377,18 @@ def _maybe_react_to_completed_iteration( comment_unique_ids=unique_ids, ) if result.unsupported_provider: - _record_completion_reaction("resolve_unsupported_provider") + outcomes = {"resolve_unsupported_provider": 1} elif result.failed: - _record_completion_reaction("resolve_failed") + outcomes = {"resolve_failed": 1} else: - if result.resolved: - _record_completion_reaction("resolved", result.resolved) - if result.already_resolved: - _record_completion_reaction( - "resolve_skipped_already_resolved", result.already_resolved - ) - if result.not_found: - _record_completion_reaction("resolve_thread_not_found", result.not_found) + outcomes = { + "resolved": result.resolved, + "resolve_skipped_already_resolved": result.already_resolved, + "resolve_thread_not_found": result.not_found, + } + for outcome, amount in outcomes.items(): + if amount: + _record_completion_reaction(outcome, amount) @classmethod def find_latest_artifact_for_step(cls, state: SeerRunState, key: str) -> Artifact | None: diff --git a/src/sentry/tasks/seer/pr_iteration.py b/src/sentry/tasks/seer/pr_iteration.py index 883f65cb561e..db5ad3664a5c 100644 --- a/src/sentry/tasks/seer/pr_iteration.py +++ b/src/sentry/tasks/seer/pr_iteration.py @@ -9,6 +9,7 @@ import sentry_sdk from scm import actions as scm_actions from scm.errors import ResourceNotFound +from scm.helpers import iter_all_pages from scm.manager import SourceCodeManager from scm.types import ( Author, @@ -397,14 +398,15 @@ def _resolve_review_comment_threads( try: threads: list[ReviewThread] = [] - cursor: str | None = None - while True: - pagination: PaginationParams | None = {"cursor": cursor} if cursor else None - result = scm_actions.get_pull_request_review_threads(scm, str(pr_number), pagination) - threads.extend(result["data"]) - cursor = result["meta"].get("next_cursor") - if not cursor: - break + # Empty starting cursor so GitHub's GraphQL first page is `after: null`. + for page in iter_all_pages( + lambda pagination: scm_actions.get_pull_request_review_threads( + scm, str(pr_number), pagination + ), + per_page=100, + cursor="", + ): + threads.extend(page["data"]) thread_by_comment: dict[str, ReviewThread] = {} for thread in threads: From b58a4cf76c2aeb3cf19e4efd8164773deb60d0a5 Mon Sep 17 00:00:00 2001 From: Alex Sohn Date: Fri, 24 Jul 2026 17:46:41 -0400 Subject: [PATCH 4/4] test(pr-iteration): trim resolve-thread test boilerplate Fold redundant early-return tests into their twins, add a _run helper, merge overlapping resolve assertions, trim unread fixture fields, and strengthen the pagination test to verify cursor threading. Also type the review-comment fixture as ReviewComment to fix latent mypy errors. --- .../test_autofix_on_completion_hook.py | 171 ++++-------------- tests/sentry/tasks/seer/test_pr_iteration.py | 29 ++- 2 files changed, 60 insertions(+), 140 deletions(-) diff --git a/tests/sentry/seer/autofix/test_autofix_on_completion_hook.py b/tests/sentry/seer/autofix/test_autofix_on_completion_hook.py index 0ad9c065ab60..41c00262eefa 100644 --- a/tests/sentry/seer/autofix/test_autofix_on_completion_hook.py +++ b/tests/sentry/seer/autofix/test_autofix_on_completion_hook.py @@ -1,3 +1,4 @@ +import contextlib from typing import TypedDict from unittest.mock import MagicMock, patch @@ -928,6 +929,17 @@ def _state_with( ) return state + def _run(self, state: SeerRunState, *, feature: bool = True) -> None: + ctx = ( + self.feature("organizations:autofix-pr-iteration") + if feature + else contextlib.nullcontext() + ) + with ctx: + AutofixOnCompletionHook._maybe_react_to_completed_iteration( + self.organization, 123, state + ) + @patch(f"{REACT_PATH}.is_github_rate_limit_sensitive", return_value=False) @patch(f"{REACT_PATH}._resolve_review_comment_threads") @patch(f"{REACT_PATH}.make_scm") @@ -943,10 +955,7 @@ def test_reacts_hooray_on_top_level_comment_only( mock_resolve.return_value = ResolveReviewThreadsResult(resolved=1) state = self._state_with([self._top_level_source(111), self._review_source(222)]) - with self.feature("organizations:autofix-pr-iteration"): - AutofixOnCompletionHook._maybe_react_to_completed_iteration( - self.organization, 123, state - ) + self._run(state) assert mock_react.call_count == 1 assert mock_react.call_args.args[0] is scm @@ -960,22 +969,18 @@ def test_reacts_hooray_on_top_level_comment_only( @patch(f"{REACT_PATH}.make_scm") @patch(f"{REACT_PATH}._add_comment_reaction") - def test_noop_on_error_status(self, mock_react, mock_make_scm): - state = self._state_with([self._top_level_source()], status="error") - with self.feature("organizations:autofix-pr-iteration"): - AutofixOnCompletionHook._maybe_react_to_completed_iteration( - self.organization, 123, state - ) + @patch(f"{REACT_PATH}._resolve_review_comment_threads") + def test_noop_on_error_status(self, mock_resolve, mock_react, mock_make_scm): + state = self._state_with([self._top_level_source(), self._review_source()], status="error") + self._run(state) mock_react.assert_not_called() + mock_resolve.assert_not_called() @patch(f"{REACT_PATH}.make_scm") @patch(f"{REACT_PATH}._add_comment_reaction") def test_noop_when_step_not_pr_iteration(self, mock_react, mock_make_scm): state = run_state(blocks=[solution_memory_block()]) - with self.feature("organizations:autofix-pr-iteration"): - AutofixOnCompletionHook._maybe_react_to_completed_iteration( - self.organization, 123, state - ) + self._run(state) mock_react.assert_not_called() @patch(f"{REACT_PATH}.make_scm") @@ -989,18 +994,17 @@ def test_noop_when_not_synced(self, mock_react, mock_make_scm): "owner/repo": RepoPRState(repo_name="owner/repo", pr_number=7, commit_sha="old-sha") }, ) - with self.feature("organizations:autofix-pr-iteration"): - AutofixOnCompletionHook._maybe_react_to_completed_iteration( - self.organization, 123, state - ) + self._run(state) mock_react.assert_not_called() @patch(f"{REACT_PATH}.make_scm") @patch(f"{REACT_PATH}._add_comment_reaction") - def test_noop_when_feature_disabled(self, mock_react, mock_make_scm): - state = self._state_with([self._top_level_source()]) - AutofixOnCompletionHook._maybe_react_to_completed_iteration(self.organization, 123, state) + @patch(f"{REACT_PATH}._resolve_review_comment_threads") + def test_noop_when_feature_disabled(self, mock_resolve, mock_react, mock_make_scm): + state = self._state_with([self._top_level_source(), self._review_source()]) + self._run(state, feature=False) mock_react.assert_not_called() + mock_resolve.assert_not_called() @patch(f"{REACT_PATH}.make_scm") @patch(f"{REACT_PATH}._add_comment_reaction") @@ -1021,10 +1025,7 @@ def test_multi_repo_skips_source_without_repo_name(self, mock_react, mock_make_s ), } - with self.feature("organizations:autofix-pr-iteration"): - AutofixOnCompletionHook._maybe_react_to_completed_iteration( - self.organization, 123, state - ) + self._run(state) # Only the source that carries repo_name is reacted on; the legacy one is # skipped rather than reacted on the wrong repo. @@ -1049,16 +1050,14 @@ def test_skips_reaction_when_pr_number_missing(self, mock_react, mock_make_scm): }, ) - with self.feature("organizations:autofix-pr-iteration"): - AutofixOnCompletionHook._maybe_react_to_completed_iteration( - self.organization, 123, state - ) + self._run(state) mock_react.assert_not_called() @patch(f"{REACT_PATH}.make_scm") @patch(f"{REACT_PATH}._add_comment_reaction") - def test_skips_reaction_when_repo_name_ambiguous(self, mock_react, mock_make_scm): + @patch(f"{REACT_PATH}._resolve_review_comment_threads") + def test_skips_reaction_when_repo_name_ambiguous(self, mock_resolve, mock_react, mock_make_scm): # The same slug can exist under multiple providers in one org; rather than # guess and react on the wrong repo, the source is skipped. self.create_repo( @@ -1067,15 +1066,13 @@ def test_skips_reaction_when_repo_name_ambiguous(self, mock_react, mock_make_scm external_id="456", name="owner/repo", ) - state = self._state_with([self._top_level_source()]) + state = self._state_with([self._top_level_source(), self._review_source()]) - with self.feature("organizations:autofix-pr-iteration"): - AutofixOnCompletionHook._maybe_react_to_completed_iteration( - self.organization, 123, state - ) + self._run(state) mock_make_scm.assert_not_called() mock_react.assert_not_called() + mock_resolve.assert_not_called() @patch(f"{REACT_PATH}.is_github_rate_limit_sensitive", return_value=False) @patch(f"{REACT_PATH}._delete_own_comment_eyes_reaction") @@ -1088,10 +1085,7 @@ def test_deletes_own_eyes_on_top_level_comment( mock_make_scm.return_value = scm state = self._state_with([self._top_level_source(111)]) - with self.feature("organizations:autofix-pr-iteration"): - AutofixOnCompletionHook._maybe_react_to_completed_iteration( - self.organization, 123, state - ) + self._run(state) assert mock_react.call_args.kwargs["reaction"] == "hooray" assert mock_delete_eyes.call_count == 1 @@ -1114,10 +1108,7 @@ def test_deletes_own_eyes_on_review_comment_without_hooray( mock_make_scm.return_value = scm state = self._state_with([self._top_level_source(111), self._review_source(222)]) - with self.feature("organizations:autofix-pr-iteration"): - AutofixOnCompletionHook._maybe_react_to_completed_iteration( - self.organization, 123, state - ) + self._run(state) # :tada: only on the top-level comment. assert mock_react.call_count == 1 @@ -1144,44 +1135,12 @@ def test_skips_eyes_delete_for_rate_limit_sensitive_org( mock_make_scm.return_value = scm state = self._state_with([self._top_level_source(111)]) - with self.feature("organizations:autofix-pr-iteration"): - AutofixOnCompletionHook._maybe_react_to_completed_iteration( - self.organization, 123, state - ) + self._run(state) # :tada: is still added, but the eyes-delete is skipped entirely. assert mock_react.call_args.kwargs["reaction"] == "hooray" mock_delete_eyes.assert_not_called() - @patch(f"{REACT_PATH}.is_github_rate_limit_sensitive", return_value=False) - @patch(f"{REACT_PATH}._resolve_review_comment_threads") - @patch(f"{REACT_PATH}._delete_own_comment_eyes_reaction") - @patch(f"{REACT_PATH}.make_scm") - @patch(f"{REACT_PATH}._add_comment_reaction") - def test_resolves_review_comment_thread( - self, mock_react, mock_make_scm, mock_delete_eyes, mock_resolve, mock_sensitive - ): - scm = MagicMock() - mock_make_scm.return_value = scm - mock_resolve.return_value = ResolveReviewThreadsResult(resolved=1) - state = self._state_with( - [self._top_level_source(111), self._review_source(222, unique_id="PRRC_222")] - ) - - with self.feature("organizations:autofix-pr-iteration"): - AutofixOnCompletionHook._maybe_react_to_completed_iteration( - self.organization, 123, state - ) - - # Top-level :tada: is unaffected. - assert mock_react.call_count == 1 - assert mock_react.call_args.kwargs["comment_id"] == 111 - - mock_resolve.assert_called_once() - assert mock_resolve.call_args.args[0] is scm - assert mock_resolve.call_args.kwargs["pr_number"] == 7 - assert mock_resolve.call_args.kwargs["comment_unique_ids"] == ["PRRC_222"] - @patch(f"{REACT_PATH}.is_github_rate_limit_sensitive", return_value=False) @patch(f"{REACT_PATH}._resolve_review_comment_threads") @patch(f"{REACT_PATH}._delete_own_comment_eyes_reaction") @@ -1200,13 +1159,11 @@ def test_batches_multiple_review_comments_per_pr( ] ) - with self.feature("organizations:autofix-pr-iteration"): - AutofixOnCompletionHook._maybe_react_to_completed_iteration( - self.organization, 123, state - ) + self._run(state) # One call per PR carrying every unique_id, not one call per comment. mock_resolve.assert_called_once() + assert mock_resolve.call_args.args[0] is scm assert mock_resolve.call_args.kwargs["pr_number"] == 7 assert mock_resolve.call_args.kwargs["comment_unique_ids"] == ["PRRC_222", "PRRC_333"] @@ -1234,10 +1191,7 @@ def test_skips_review_resolve_when_repo_ambiguous_multi_repo( ), } - with self.feature("organizations:autofix-pr-iteration"): - AutofixOnCompletionHook._maybe_react_to_completed_iteration( - self.organization, 123, state - ) + self._run(state) mock_resolve.assert_not_called() @@ -1255,10 +1209,7 @@ def test_skips_resolve_for_legacy_source_without_unique_id( mock_make_scm.return_value = scm state = self._state_with([self._review_source(222, unique_id=None)]) - with self.feature("organizations:autofix-pr-iteration"): - AutofixOnCompletionHook._maybe_react_to_completed_iteration( - self.organization, 123, state - ) + self._run(state) mock_resolve.assert_not_called() # :eyes: removal still happens for the inline comment. @@ -1277,48 +1228,6 @@ def test_skips_resolve_for_rate_limit_sensitive_org( mock_make_scm.return_value = scm state = self._state_with([self._review_source(222, unique_id="PRRC_222")]) - with self.feature("organizations:autofix-pr-iteration"): - AutofixOnCompletionHook._maybe_react_to_completed_iteration( - self.organization, 123, state - ) - - mock_resolve.assert_not_called() + self._run(state) - @patch(f"{REACT_PATH}._resolve_review_comment_threads") - @patch(f"{REACT_PATH}.make_scm") - @patch(f"{REACT_PATH}._add_comment_reaction") - def test_noop_resolve_when_feature_disabled(self, mock_react, mock_make_scm, mock_resolve): - state = self._state_with([self._review_source(222, unique_id="PRRC_222")]) - AutofixOnCompletionHook._maybe_react_to_completed_iteration(self.organization, 123, state) - mock_resolve.assert_not_called() - - @patch(f"{REACT_PATH}._resolve_review_comment_threads") - @patch(f"{REACT_PATH}.make_scm") - @patch(f"{REACT_PATH}._add_comment_reaction") - def test_noop_resolve_on_error_status(self, mock_react, mock_make_scm, mock_resolve): - state = self._state_with([self._review_source(222, unique_id="PRRC_222")], status="error") - with self.feature("organizations:autofix-pr-iteration"): - AutofixOnCompletionHook._maybe_react_to_completed_iteration( - self.organization, 123, state - ) - mock_resolve.assert_not_called() - - @patch(f"{REACT_PATH}._resolve_review_comment_threads") - @patch(f"{REACT_PATH}.make_scm") - @patch(f"{REACT_PATH}._add_comment_reaction") - def test_skips_resolve_when_repo_name_ambiguous(self, mock_react, mock_make_scm, mock_resolve): - self.create_repo( - project=self.project, - provider="integrations:gitlab", - external_id="456", - name="owner/repo", - ) - state = self._state_with([self._review_source(222, unique_id="PRRC_222")]) - - with self.feature("organizations:autofix-pr-iteration"): - AutofixOnCompletionHook._maybe_react_to_completed_iteration( - self.organization, 123, state - ) - - mock_make_scm.assert_not_called() mock_resolve.assert_not_called() diff --git a/tests/sentry/tasks/seer/test_pr_iteration.py b/tests/sentry/tasks/seer/test_pr_iteration.py index 1d010a0d8c1d..3e301395a052 100644 --- a/tests/sentry/tasks/seer/test_pr_iteration.py +++ b/tests/sentry/tasks/seer/test_pr_iteration.py @@ -2,6 +2,8 @@ from typing import Any, Literal from unittest.mock import MagicMock, patch +from scm.types import ReviewComment + from sentry.seer.agent.client_models import MemoryBlock, Message, RepoPRState, SeerRunState from sentry.seer.autofix.autofix_agent import ( PrIterationNoPullRequestException, @@ -1111,11 +1113,7 @@ def _thread( return { "id": thread_id, "is_resolved": is_resolved, - "is_outdated": False, - "file_path": "test.py", - "line": 1, - "start_line": None, - "comments": [{"id": uid, "unique_id": uid} for uid in comment_unique_ids], + "comments": [{"unique_id": uid} for uid in comment_unique_ids], } def _page( @@ -1180,11 +1178,24 @@ def test_unknown_unique_id_not_found(self, mock_scm_actions: MagicMock) -> None: @patch(f"{TASK_PATH}.scm_actions") def test_pages_until_exhausted(self, mock_scm_actions: MagicMock) -> None: scm = self._scm() - mock_scm_actions.get_pull_request_review_threads.side_effect = [ + pages = [ self._page([self._thread("PRRT_1", ["PRRC_a"])], next_cursor="page-2"), self._page([self._thread("PRRT_2", ["PRRC_b"])]), ] + # Assert the cursor is threaded across pages, not ignored: the first page + # must start at ``after: null`` (empty cursor) and the second at page-1's + # next_cursor. + def get_threads(scm_arg: Any, pr_number_str: str, pagination: Any) -> dict[str, Any]: + if mock_scm_actions.get_pull_request_review_threads.call_count == 1: + assert pagination["cursor"] == "" + assert pagination["per_page"] == 100 + else: + assert pagination["cursor"] == "page-2" + return pages[mock_scm_actions.get_pull_request_review_threads.call_count - 1] + + mock_scm_actions.get_pull_request_review_threads.side_effect = get_threads + result = _resolve_review_comment_threads(scm, pr_number=7, comment_unique_ids=["PRRC_b"]) assert result.resolved == 1 @@ -1218,9 +1229,9 @@ def test_swallows_exceptions(self, mock_scm_actions: MagicMock) -> None: class BuildReviewFeedbackTest(TestCase): - def _review_comment(self, unique_id: str | None) -> dict[str, Any]: + def _review_comment(self, unique_id: str | None) -> ReviewComment: return { - "id": 222, + "id": "222", "unique_id": unique_id, "url": "https://example.com/c/222", "file_path": "test.py", @@ -1230,7 +1241,7 @@ def _review_comment(self, unique_id: str | None) -> dict[str, Any]: "diff_hunk": None, "line": None, "start_line": None, - "review_id": 55, + "review_id": "55", "author_association": None, "commit_sha": None, "head": None,