diff --git a/src/sentry/seer/autofix/on_completion_hook.py b/src/sentry/seer/autofix/on_completion_hook.py index e86fa91c3585..28a4706b64f0 100644 --- a/src/sentry/seer/autofix/on_completion_hook.py +++ b/src/sentry/seer/autofix/on_completion_hook.py @@ -53,6 +53,10 @@ ) from sentry.seer.autofix.pr_iteration.logs import PrIterationLogContext from sentry.seer.autofix.pr_iteration.pause import PauseReason, pause_pr_iteration +from sentry.seer.autofix.pr_iteration.pr_state import ( + iteration_prs_any_closed, + record_pr_closed, +) from sentry.seer.autofix.pr_ready_for_review import ( emit_pr_ready_for_review, format_pull_requests_payload, @@ -1165,6 +1169,16 @@ def _pr_iteration_push_outcome( ) return PrIterationOutcome.PR_CREATION_ERRORED + if iteration_prs_any_closed(group.organization, state): + record_pr_closed("push") + pause_pr_iteration( + run_id=run_id, + organization_id=group.organization.id, + reason=PauseReason.PR_CLOSED, + ) + log_ctx.info("autofix.pr_iteration.push", outcome="not_pushed", reason="pr_closed") + return PrIterationOutcome.PR_CLOSED + pushed = cls._push_iteration_changes( log_ctx, group, diff --git a/src/sentry/seer/autofix/pr_iteration/emit.py b/src/sentry/seer/autofix/pr_iteration/emit.py index a6193afe0c97..ad919129d1d2 100644 --- a/src/sentry/seer/autofix/pr_iteration/emit.py +++ b/src/sentry/seer/autofix/pr_iteration/emit.py @@ -55,6 +55,7 @@ class PrIterationOutcome(StrEnum): ALREADY_PUSHED = "already_pushed" NO_CODE_CHANGES = "no_code_changes" NO_PULL_REQUEST = "no_pull_request" + PR_CLOSED = "pr_closed" PR_CREATION_ERRORED = "pr_creation_errored" PUSH_FAILED = "push_failed" TIMEOUT = "timeout" diff --git a/src/sentry/seer/autofix/pr_iteration/pause.py b/src/sentry/seer/autofix/pr_iteration/pause.py index c06404436587..60ef1f18ed8c 100644 --- a/src/sentry/seer/autofix/pr_iteration/pause.py +++ b/src/sentry/seer/autofix/pr_iteration/pause.py @@ -27,6 +27,7 @@ class PauseReason(StrEnum): USER_STOP = "user_stop" RUN_ERRORED = "run_errored" + PR_CLOSED = "pr_closed" def _get_seer_run(run_id: int, organization_id: int) -> SeerRun | None: diff --git a/src/sentry/seer/autofix/pr_iteration/pr_state.py b/src/sentry/seer/autofix/pr_iteration/pr_state.py new file mode 100644 index 000000000000..505387d3fd27 --- /dev/null +++ b/src/sentry/seer/autofix/pr_iteration/pr_state.py @@ -0,0 +1,73 @@ +"""Read the live state of the pull requests an Autofix run opened. + +Closing a PR is how someone tells Seer to stop working on it. Both ends of an +iteration ask here: the consume task before it spends an agent run, and the +completion hook before it pushes what that run produced. +""" + +from __future__ import annotations + +from typing import Literal + +from scm import actions as scm_actions +from scm.types import GetPullRequestProtocol + +from sentry.models.organization import Organization +from sentry.models.repository import Repository +from sentry.scm.factory import new as make_scm +from sentry.seer.agent.client_models import SeerRunState +from sentry.seer.autofix.pr_iteration.constants import PR_ITERATION_PROVIDER_SLUG +from sentry.utils import metrics + +PR_CLOSED_METRIC = "autofix.pr_iteration.pr_closed" + +# Where we caught it: before the agent run, or before the push. The only tag on +# the metric, and closed so it stays two time series. +PrClosedGate = Literal["consume", "push"] + + +def record_pr_closed(gate: PrClosedGate) -> None: + metrics.incr(PR_CLOSED_METRIC, tags={"gate": gate}) + + +def iteration_prs_any_closed(organization: Organization, state: SeerRunState) -> bool: + """True when any PR on the run reads back as closed. + + One closed PR stops the whole run: an iteration pushes to every repo at + once, so there is no way to serve the open PRs while leaving the closed one + alone. + + A PR we cannot read (no number, repo gone, unsupported provider, API error) + is passed over: a transient read failure should not silently drop an + iteration's work. + """ + for repo_name, pr_state in state.repo_pr_states.items(): + pr_number = pr_state.pr_number + if pr_number is None: + continue + + repo, _resolution = Repository.objects.resolve_active( + organization_id=organization.id, + name=repo_name, + normalized_provider=PR_ITERATION_PROVIDER_SLUG, + ) + if repo is None: + continue + + try: + scm = make_scm(organization.id, repo.id, referrer="seer") + except Exception: + continue + + if not isinstance(scm, GetPullRequestProtocol): + continue + + try: + pull_request = scm_actions.get_pull_request(scm, str(pr_number)) + except Exception: + continue + + if pull_request["data"]["state"] == "closed": + return True + + return False diff --git a/src/sentry/seer/endpoints/group_ai_autofix.py b/src/sentry/seer/endpoints/group_ai_autofix.py index 83c5bfc51de1..f81ccd511594 100644 --- a/src/sentry/seer/endpoints/group_ai_autofix.py +++ b/src/sentry/seer/endpoints/group_ai_autofix.py @@ -98,6 +98,7 @@ PAUSED_PR_ITERATION_DETAIL = { PauseReason.USER_STOP: "Iteration was stopped for this pull request", PauseReason.RUN_ERRORED: "Seer can no longer iterate on this pull request", + PauseReason.PR_CLOSED: "This pull request is closed, so Seer stopped iterating on it", } diff --git a/src/sentry/tasks/seer/pr_iteration.py b/src/sentry/tasks/seer/pr_iteration.py index 9a7b50d28110..a87fe191c978 100644 --- a/src/sentry/tasks/seer/pr_iteration.py +++ b/src/sentry/tasks/seer/pr_iteration.py @@ -95,6 +95,10 @@ pause_pr_iteration, record_pause_blocked, ) +from sentry.seer.autofix.pr_iteration.pr_state import ( + iteration_prs_any_closed, + record_pr_closed, +) from sentry.seer.autofix.pr_iteration.queue import ( QueuedAutofixFeedback, clear_queued_autofix_feedback, @@ -418,6 +422,20 @@ def consume_queued_autofix_feedback( activation_id=task_state.id if task_state else None, ) + if iteration_prs_any_closed(organization, state): + record_pr_closed("consume") + pause_pr_iteration( + run_id=run_id, + organization_id=organization_id, + reason=PauseReason.PR_CLOSED, + ) + log_ctx.info( + "autofix.pr_iteration.consume_feedback.skipped", + trigger_id=trigger_id, + reason="pr_closed", + ) + return + try: _drain_queued_autofix_feedback( log_ctx=log_ctx, 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 ae66053fc389..7a5f772c949b 100644 --- a/tests/sentry/seer/autofix/test_autofix_on_completion_hook.py +++ b/tests/sentry/seer/autofix/test_autofix_on_completion_hook.py @@ -2,6 +2,7 @@ from unittest.mock import MagicMock, patch from sentry.models.activity import Activity +from sentry.models.repository import Repository from sentry.seer.agent.client_models import ( AgentFilePatch, Artifact, @@ -629,6 +630,7 @@ def test_push_changes_pushes_when_any_unsynced_repo_not_errored(self, mock_push_ HOOK_PATH = "sentry.seer.autofix.on_completion_hook" +PR_STATE_PATH = "sentry.seer.autofix.pr_iteration.pr_state" class TestPrIterationCompletionHook(TestCase): @@ -801,6 +803,92 @@ def test_a_repo_whose_pr_creation_errored_stops_the_push(self, mock_push): assert pushed is False mock_push.assert_not_called() + def _github_repo(self, name: str = "test-repo", external_id: str = "1") -> None: + Repository.objects.create( + organization_id=self.organization.id, + name=name, + provider="integrations:github", + external_id=external_id, + ) + + @patch(f"{PR_STATE_PATH}.metrics.incr") + @patch(f"{HOOK_PATH}.pause_pr_iteration") + @patch(f"{PR_STATE_PATH}.GetPullRequestProtocol", object) + @patch(f"{PR_STATE_PATH}.scm_actions.get_pull_request") + @patch(f"{PR_STATE_PATH}.make_scm") + @patch(f"{HOOK_PATH}.trigger_push_changes") + def test_a_closed_pr_stops_the_push( + self, mock_push, mock_make_scm, mock_get_pull_request, mock_pause, mock_incr + ): + """Closing the PR is the stop signal; pushing into it would talk past it.""" + self._github_repo() + mock_get_pull_request.return_value = {"data": {"state": "closed"}} + + pushed = self._push(self._unsynced()) + + assert pushed is False + mock_push.assert_not_called() + assert mock_pause.call_args.kwargs["reason"] == PauseReason.PR_CLOSED + mock_incr.assert_any_call("autofix.pr_iteration.pr_closed", tags={"gate": "push"}) + + @patch(f"{PR_STATE_PATH}.GetPullRequestProtocol", object) + @patch(f"{PR_STATE_PATH}.scm_actions.get_pull_request") + @patch(f"{PR_STATE_PATH}.make_scm") + @patch(f"{HOOK_PATH}.trigger_push_changes") + def test_an_open_pr_still_pushes(self, mock_push, mock_make_scm, mock_get_pull_request): + self._github_repo() + mock_get_pull_request.return_value = {"data": {"state": "open"}} + + pushed = self._push(self._unsynced()) + + assert pushed is True + mock_push.assert_called_once() + + @patch(f"{PR_STATE_PATH}.GetPullRequestProtocol", object) + @patch(f"{PR_STATE_PATH}.scm_actions.get_pull_request") + @patch(f"{PR_STATE_PATH}.make_scm") + @patch(f"{HOOK_PATH}.trigger_push_changes") + def test_one_closed_pr_stops_a_multi_repo_push( + self, mock_push, mock_make_scm, mock_get_pull_request + ): + """A push serves every repo at once, so it cannot skip just the closed one.""" + self._github_repo() + self._github_repo("other-repo", external_id="2") + state = self._unsynced() + state.repo_pr_states["other-repo"] = RepoPRState( + repo_name="other-repo", + provider="github", + pr_id=88, + pr_number=8, + pr_url="https://example.com/pull/8", + pr_creation_status="completed", + commit_sha="stale-sha", + ) + mock_get_pull_request.side_effect = [ + {"data": {"state": "open"}}, + {"data": {"state": "closed"}}, + ] + + pushed = self._push(state) + + assert pushed is False + mock_push.assert_not_called() + + @patch(f"{PR_STATE_PATH}.GetPullRequestProtocol", object) + @patch(f"{PR_STATE_PATH}.scm_actions.get_pull_request", side_effect=ValueError("boom")) + @patch(f"{PR_STATE_PATH}.make_scm") + @patch(f"{HOOK_PATH}.trigger_push_changes") + def test_a_pr_we_cannot_read_still_pushes( + self, mock_push, mock_make_scm, mock_get_pull_request + ): + """A transient read failure must not silently drop the iteration's changes.""" + self._github_repo() + + pushed = self._push(self._unsynced()) + + assert pushed is True + mock_push.assert_called_once() + @patch(f"{HOOK_PATH}.trigger_push_changes", side_effect=ValueError("boom")) def test_a_failed_push_is_swallowed(self, mock_push): state = self._unsynced() diff --git a/tests/sentry/tasks/seer/test_pr_iteration.py b/tests/sentry/tasks/seer/test_pr_iteration.py index 0681318c6a5d..b954c3e4dc37 100644 --- a/tests/sentry/tasks/seer/test_pr_iteration.py +++ b/tests/sentry/tasks/seer/test_pr_iteration.py @@ -8,6 +8,7 @@ from scm.types import ReviewComment from sentry.models.pullrequest import PullRequest +from sentry.models.repository import Repository from sentry.seer.agent.client_models import ( AgentFilePatch, FilePatch, @@ -72,6 +73,7 @@ TASK_PATH = "sentry.tasks.seer.pr_iteration" CHECK_SUITE_SOURCE_PATH = "sentry.seer.autofix.pr_iteration.feedback_sources.check_suite" PAUSE_PATH = "sentry.seer.autofix.pr_iteration.pause" +PR_STATE_PATH = "sentry.seer.autofix.pr_iteration.pr_state" class _CommentScmStub: @@ -917,6 +919,70 @@ def _state_on_head(self, **kwargs: Any) -> SeerRunState: def _call(self) -> None: consume_queued_autofix_feedback(run_id=67890, organization_id=self.organization.id) + def _state_with_open_pr(self) -> SeerRunState: + state = self._state() + state.repo_pr_states = { + "owner/repo": RepoPRState(repo_name="owner/repo", pr_number=7, commit_sha="abc") + } + Repository.objects.create( + organization_id=self.organization.id, + name="owner/repo", + provider="integrations:github", + external_id="1", + ) + return state + + @patch(f"{PR_STATE_PATH}.metrics.incr") + @patch(f"{TASK_PATH}.pause_pr_iteration") + @patch(f"{PR_STATE_PATH}.GetPullRequestProtocol", object) + @patch(f"{PR_STATE_PATH}.scm_actions.get_pull_request") + @patch(f"{PR_STATE_PATH}.make_scm") + @patch(f"{TASK_PATH}.trigger_autofix_agent") + @patch(f"{TASK_PATH}.pop_queued_autofix_feedback") + @patch(f"{TASK_PATH}.fetch_run_status") + def test_a_closed_pr_stops_the_iteration_before_it_starts( + self, + mock_fetch: MagicMock, + mock_pop: MagicMock, + mock_trigger: MagicMock, + mock_make_scm: MagicMock, + mock_get_pull_request: MagicMock, + mock_pause: MagicMock, + mock_incr: MagicMock, + ) -> None: + """The run is paused rather than left to re-check on every later trigger.""" + mock_fetch.return_value = self._state_with_open_pr() + mock_get_pull_request.return_value = {"data": {"state": "closed"}} + + self._call() + + mock_trigger.assert_not_called() + mock_pop.assert_not_called() + assert mock_pause.call_args.kwargs["reason"] == PauseReason.PR_CLOSED + mock_incr.assert_any_call("autofix.pr_iteration.pr_closed", tags={"gate": "consume"}) + + @patch(f"{PR_STATE_PATH}.GetPullRequestProtocol", object) + @patch(f"{PR_STATE_PATH}.scm_actions.get_pull_request") + @patch(f"{PR_STATE_PATH}.make_scm") + @patch(f"{TASK_PATH}.trigger_autofix_agent") + @patch(f"{TASK_PATH}.pop_queued_autofix_feedback") + @patch(f"{TASK_PATH}.fetch_run_status") + def test_an_open_pr_still_iterates( + self, + mock_fetch: MagicMock, + mock_pop: MagicMock, + mock_trigger: MagicMock, + mock_make_scm: MagicMock, + mock_get_pull_request: MagicMock, + ) -> None: + mock_fetch.return_value = self._state_with_open_pr() + mock_pop.return_value = [self._ui_queued()] + mock_get_pull_request.return_value = {"data": {"state": "open"}} + + self._call() + + mock_trigger.assert_called_once() + @patch(f"{TASK_PATH}.trigger_autofix_agent") @patch(f"{TASK_PATH}.pop_queued_autofix_feedback") @patch(f"{TASK_PATH}.fetch_run_status")