diff --git a/src/sentry/seer/autofix/on_completion_hook.py b/src/sentry/seer/autofix/on_completion_hook.py index a7d8842908ad..86cec0e52031 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,15 +247,7 @@ 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.""" if not features.has("organizations:autofix-pr-iteration", organization=organization): return @@ -290,10 +284,13 @@ 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). + 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 +341,7 @@ 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 comments resolve below (CW-1688). if source_type == "github-pr-comment": _add_comment_reaction( scm, @@ -355,6 +351,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 +365,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: + outcomes = {"resolve_unsupported_provider": 1} + elif result.failed: + outcomes = {"resolve_failed": 1} + else: + 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: 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..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,6 +34,8 @@ class GithubPullRequestReviewComment(GithubIssueComment): line: int | None = None start_line: int | None = None diff_hunk: str | None = None + # GraphQL node id used to resolve the review thread at completion (CW-1688). + 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..db5ad3664a5c 100644 --- a/src/sentry/tasks/seer/pr_iteration.py +++ b/src/sentry/tasks/seer/pr_iteration.py @@ -1,12 +1,15 @@ from __future__ import annotations import logging +from collections.abc import Collection +from dataclasses import dataclass from datetime import timedelta from typing import Any 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, @@ -18,15 +21,18 @@ GetAuthenticatedActorProtocol, GetPullRequestCommentReactionsProtocol, GetPullRequestReviewProtocol, + GetPullRequestReviewThreadsProtocol, GetRepositoryUserPermissionProtocol, GetReviewCommentReactionsProtocol, GetReviewCommentsProtocol, PaginationParams, Reaction, ReactionResult, + ResolveReviewThreadProtocol, ResourceId, Review, ReviewComment, + ReviewThread, ) from taskbroker_client.retry import Retry @@ -367,6 +373,71 @@ 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).""" + 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] = [] + # 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: + 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 +777,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..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 @@ -27,6 +28,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 +899,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,19 +929,33 @@ 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") @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"): - 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 @@ -944,24 +964,23 @@ 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): - 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") @@ -975,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") @@ -1007,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. @@ -1035,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( @@ -1053,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") @@ -1074,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 @@ -1087,11 +1095,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). @@ -1099,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 @@ -1129,11 +1135,99 @@ 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_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"), + ] + ) + + 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"] + + @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" + ), + } + + self._run(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)]) + + self._run(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")]) + + self._run(state) + + 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..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, @@ -22,8 +24,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 +1086,195 @@ 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, + "comments": [{"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() + 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 + 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) -> ReviewComment: + 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