-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
feat(pr-iteration): react with 🎉 on comment after iteration completes #119441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
dd123c6
53739fb
53eabad
e87da3a
d48ef55
7ce2525
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,15 +5,19 @@ | |
|
|
||
| from django.db import router, transaction | ||
| from django.utils import timezone | ||
| from scm.manager import SourceCodeManager | ||
|
|
||
| from sentry import analytics, features | ||
| from sentry.analytics.events.autofix_events import ( | ||
| AiAutofixIntrospectionEvent, | ||
| AiAutofixPrCreatedCompletedEvent, | ||
| ) | ||
| from sentry.integrations.github.utils import is_github_rate_limit_sensitive | ||
| from sentry.models.group import Group | ||
| from sentry.models.organization import Organization | ||
| from sentry.models.project import Project | ||
| from sentry.models.repository import Repository | ||
| from sentry.scm.factory import new as make_scm | ||
| from sentry.seer.agent.client_models import Artifact | ||
| from sentry.seer.agent.client_utils import fetch_run_status | ||
| from sentry.seer.agent.on_completion_hook import AgentOnCompletionHook | ||
|
|
@@ -41,6 +45,12 @@ | |
| introspect_root_cause, | ||
| introspect_solution, | ||
| ) | ||
| from sentry.seer.autofix.pr_iteration.feedback import parse_feedback | ||
| from sentry.seer.autofix.pr_iteration.feedback_sources.github_comment import ( | ||
| GithubPrCommentFeedbackSource, | ||
| GithubPrCommentFeedbackType, | ||
| GithubPrReviewCommentFeedbackSource, | ||
| ) | ||
| from sentry.seer.autofix.utils import ( | ||
| AutofixStoppingPoint, | ||
| clear_preference_automation_handoff, | ||
|
|
@@ -54,7 +64,10 @@ | |
| from sentry.sentry_apps.event_types import SentryAppEventType | ||
| from sentry.sentry_apps.tasks.sentry_apps import broadcast_webhooks_for_organization | ||
| from sentry.sentry_apps.utils.webhooks import SeerActionType | ||
| from sentry.tasks.seer.pr_iteration import consume_queued_autofix_feedback | ||
| from sentry.tasks.seer.pr_iteration import ( | ||
| _swap_comment_reaction_to_done, | ||
| consume_queued_autofix_feedback, | ||
| ) | ||
| from sentry.utils import metrics | ||
|
|
||
| if TYPE_CHECKING: | ||
|
|
@@ -125,6 +138,10 @@ def execute(cls, organization: Organization, run_id: int) -> None: | |
| # so the user knows to update them (at most once per repo per run). | ||
| cls._maybe_comment_on_missing_permissions(organization, run_id, state) | ||
|
|
||
| # When a PR-comment-triggered iteration finishes, acknowledge the | ||
| # originating comment(s) by swapping our :eye: for a :tada:. | ||
| cls._maybe_react_to_completed_iteration(organization, run_id, state) | ||
|
|
||
| # Continue the automated pipeline if stopping_point hasn't been reached | ||
| cls._maybe_continue_pipeline(organization, run_id, state, group) | ||
|
|
||
|
|
@@ -186,6 +203,119 @@ def _maybe_comment_on_missing_permissions( | |
|
|
||
| comment_on_out_of_date_github_permissions(organization, state, missing_by_repo) | ||
|
|
||
| @classmethod | ||
| def _repo_name_for_feedback( | ||
| cls, | ||
| state: SeerRunState, | ||
| source: GithubPrCommentFeedbackSource | GithubPrReviewCommentFeedbackSource, | ||
| ) -> str | None: | ||
| """The repo (a ``repo_pr_states`` key, i.e. ``Repository.name`` = "owner/repo") | ||
| a feedback comment lives on. | ||
|
|
||
| Uses the ``repo_name`` captured on the source at trigger time. For feedback | ||
| serialized before that field existed it's absent, so we fall back to the | ||
| sole repo; a multi-repo run with no ``repo_name`` is skipped rather than | ||
| reacted on the wrong repo. | ||
| """ | ||
| if source.repo_name is not None: | ||
| return source.repo_name | ||
| if len(state.repo_pr_states) == 1: | ||
| return next(iter(state.repo_pr_states)) | ||
| return None | ||
|
|
||
| @classmethod | ||
| def _maybe_react_to_completed_iteration( | ||
| cls, | ||
| organization: Organization, | ||
| run_id: int, | ||
| state: SeerRunState, | ||
| ) -> None: | ||
| """Swap :eye: to :tada: on the comment(s) that triggered a completed PR iteration.""" | ||
| if not features.has("organizations:autofix-pr-iteration", organization=organization): | ||
| return | ||
|
|
||
| current_step, _ = cls._get_current_step(state) | ||
| if current_step != AutofixStep.PR_ITERATION or state.status != "completed": | ||
| return | ||
|
|
||
| # we only want to react when the code change was PUSHED | ||
| # if we don't check this, we would react tada before the commit lands | ||
| _, is_synced = state.has_code_changes() | ||
| if not is_synced: | ||
| return | ||
|
|
||
| # The consumed feedback is serialized once onto the iteration's opening | ||
| # PR_ITERATION block, so find the most recent one and read it off there. | ||
| raw = None | ||
| for block in reversed(state.blocks): | ||
| metadata = block.message.metadata or {} | ||
| if metadata.get("step") == AutofixStep.PR_ITERATION.value: | ||
| raw = metadata.get("feedback") | ||
| break | ||
|
alexsohn1126 marked this conversation as resolved.
|
||
| if not raw: | ||
| return | ||
|
|
||
| sources: list[GithubPrCommentFeedbackSource | GithubPrReviewCommentFeedbackSource] = [] | ||
| for feedback in parse_feedback(raw): | ||
| if isinstance( | ||
| feedback.source, | ||
| (GithubPrCommentFeedbackSource, GithubPrReviewCommentFeedbackSource), | ||
| ): | ||
| sources.append(feedback.source) | ||
| if not sources: | ||
| return | ||
|
|
||
| delete_eyes = not is_github_rate_limit_sensitive(organization.slug) | ||
| scm_by_repo: dict[str, SourceCodeManager] = {} | ||
| for source in sources: | ||
| comment_id = source.comment.id | ||
| if comment_id is None: | ||
| continue | ||
|
|
||
| repo_name = cls._repo_name_for_feedback(state, source) | ||
| if repo_name is None: | ||
| continue | ||
|
|
||
| scm = scm_by_repo.get(repo_name) | ||
| if scm is None: | ||
| # Resolve the repo by its DB id so we don't assume a provider: | ||
| # a fixed ("github", ...) tuple can't resolve GitHub Enterprise | ||
| # repos (provider "integrations:github_enterprise"). | ||
| repo = Repository.objects.filter( | ||
| organization_id=organization.id, name=repo_name | ||
| ).first() | ||
| if repo is None: | ||
| logger.warning( | ||
| "autofix.on_completion_hook.completion_reaction.repo_not_found", | ||
| extra={"run_id": run_id, "organization_id": organization.id}, | ||
| ) | ||
| continue | ||
| try: | ||
| scm = make_scm(organization.id, repo.id, referrer="seer") | ||
| except Exception: | ||
| logger.warning( | ||
| "autofix.on_completion_hook.completion_reaction.scm_init_failed", | ||
| extra={"run_id": run_id, "organization_id": organization.id}, | ||
| exc_info=True, | ||
| ) | ||
| continue | ||
| scm_by_repo[repo_name] = scm | ||
|
|
||
| source_type: GithubPrCommentFeedbackType = ( | ||
| "github-pr-review-comment" | ||
| if isinstance(source, GithubPrReviewCommentFeedbackSource) | ||
| else "github-pr-comment" | ||
| ) | ||
| pr_state = state.repo_pr_states.get(repo_name) | ||
| pr_number = pr_state.pr_number if pr_state and pr_state.pr_number else 0 | ||
|
sentry[bot] marked this conversation as resolved.
Comment on lines
+309
to
+310
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: The code uses the Autofix PR number instead of the original user PR number when swapping comment reactions, causing API calls to fail silently. Suggested FixStore the original PR number from the user's pull request when the iteration is triggered. This stored original PR number should then be passed to and used by the Prompt for AI Agent |
||
| _swap_comment_reaction_to_done( | ||
| scm, | ||
| source_type=source_type, | ||
| pr_number=pr_number, | ||
| comment_id=comment_id, | ||
| delete_eyes=delete_eyes, | ||
| ) | ||
|
|
||
| @classmethod | ||
| def find_latest_artifact_for_step(cls, state: SeerRunState, key: str) -> Artifact | None: | ||
| for block in reversed(state.blocks): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,12 @@ | |
| from scm.types import ( | ||
| CreatePullRequestCommentReactionProtocol, | ||
| CreateReviewCommentReactionProtocol, | ||
| DeletePullRequestCommentReactionProtocol, | ||
| DeleteReviewCommentReactionProtocol, | ||
| GetAuthenticatedActorProtocol, | ||
| GetPullRequestCommentReactionsProtocol, | ||
| GetRepositoryUserPermissionProtocol, | ||
| GetReviewCommentReactionsProtocol, | ||
| ) | ||
| from taskbroker_client.retry import Retry | ||
|
|
||
|
|
@@ -243,6 +248,95 @@ def _add_comment_eyes_reaction( | |
| sentry_sdk.capture_exception(e) | ||
|
|
||
|
|
||
| def _swap_comment_reaction_to_done( | ||
| scm: SourceCodeManager, | ||
| *, | ||
| source_type: GithubPrCommentFeedbackType, | ||
| pr_number: int, | ||
| comment_id: int, | ||
| delete_eyes: bool, | ||
| ) -> None: | ||
| """Acknowledge iteration completion on a PR comment: add :hooray:, drop our :eyes:. | ||
|
|
||
| The :hooray: add is idempotent on GitHub (a repeat is a harmless no-op), so | ||
| unlike the read-before-add merge-request path we always add directly. When | ||
| ``delete_eyes`` is False (rate-limit-sensitive orgs) we skip the reaction | ||
| reads the deletion needs and only add :hooray:. Our own :eyes: is matched via | ||
| ``get_authenticated_actor`` so we never remove another user's reaction. | ||
|
|
||
| The add and the :eyes: cleanup are separate GitHub API phases: the add is the | ||
| acknowledgement itself, so its failure aborts (leaving the :eyes: as a | ||
| visibly-in-progress marker), while the cleanup is best-effort — if it fails | ||
| the :hooray: is already posted and we only risk a stale :eyes:. A failure in | ||
| either phase is caught and logged so it can't break the completion hook. | ||
|
|
||
| Review comments and top-level PR comments sit behind separate GitHub | ||
| endpoints, so we branch on ``source_type`` and call each one's SCM actions. | ||
| """ | ||
| pr, cid = str(pr_number), str(comment_id) | ||
| log_extra = {"source_type": source_type, "pr_number": pr_number, "comment_id": comment_id} | ||
| unsupported = "autofix.pr_iteration.comment_completion.unsupported_provider" | ||
|
|
||
| if source_type == "github-pr-review-comment": | ||
| if not isinstance(scm, CreateReviewCommentReactionProtocol): | ||
| logger.warning(unsupported, extra=log_extra) | ||
| return | ||
| try: | ||
| scm_actions.create_review_comment_reaction(scm, pr, cid, "hooray") | ||
| except Exception: | ||
| logger.exception("autofix.pr_iteration.comment_completion.add_failed", extra=log_extra) | ||
| return | ||
|
|
||
| if not delete_eyes: | ||
| return | ||
| if not ( | ||
| isinstance(scm, GetAuthenticatedActorProtocol) | ||
| and isinstance(scm, GetReviewCommentReactionsProtocol) | ||
| and isinstance(scm, DeleteReviewCommentReactionProtocol) | ||
| ): | ||
| logger.warning(unsupported, extra=log_extra) | ||
| return | ||
| try: | ||
| actor_id = scm_actions.get_authenticated_actor(scm)["data"]["id"] | ||
| for reaction in scm_actions.get_review_comment_reactions(scm, pr, cid)["data"]: | ||
| author = reaction["author"] | ||
| if reaction["content"] == "eyes" and author and author["id"] == actor_id: | ||
| scm_actions.delete_review_comment_reaction(scm, pr, cid, reaction["id"]) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: The code passes a GitHub reaction ID, which is an integer, to an SCM function that may expect a string. This could cause silent failure when deleting reactions. Suggested FixTo ensure type consistency, explicitly cast the reaction ID to a string before passing it to the SCM deletion functions. For example, use Prompt for AI AgentAlso affects:
|
||
| except Exception: | ||
| logger.exception( | ||
| "autofix.pr_iteration.comment_completion.delete_eyes_failed", extra=log_extra | ||
| ) | ||
| else: | ||
| if not isinstance(scm, CreatePullRequestCommentReactionProtocol): | ||
| logger.warning(unsupported, extra=log_extra) | ||
| return | ||
| try: | ||
| scm_actions.create_pull_request_comment_reaction(scm, pr, cid, "hooray") | ||
| except Exception: | ||
| logger.exception("autofix.pr_iteration.comment_completion.add_failed", extra=log_extra) | ||
| return | ||
|
|
||
| if not delete_eyes: | ||
| return | ||
| if not ( | ||
| isinstance(scm, GetAuthenticatedActorProtocol) | ||
| and isinstance(scm, GetPullRequestCommentReactionsProtocol) | ||
| and isinstance(scm, DeletePullRequestCommentReactionProtocol) | ||
| ): | ||
| logger.warning(unsupported, extra=log_extra) | ||
| return | ||
| try: | ||
| actor_id = scm_actions.get_authenticated_actor(scm)["data"]["id"] | ||
| for reaction in scm_actions.get_pull_request_comment_reactions(scm, pr, cid)["data"]: | ||
| author = reaction["author"] | ||
| if reaction["content"] == "eyes" and author and author["id"] == actor_id: | ||
| scm_actions.delete_pull_request_comment_reaction(scm, pr, cid, reaction["id"]) | ||
| except Exception: | ||
| logger.exception( | ||
| "autofix.pr_iteration.comment_completion.delete_eyes_failed", extra=log_extra | ||
| ) | ||
|
|
||
|
|
||
| @instrumented_task( | ||
| name="sentry.tasks.autofix.trigger_pr_iteration_from_comment", | ||
| namespace=seer_tasks, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Terminal push skips comment reaction
Medium Severity
_maybe_react_to_completed_iterationreturns wheneverhas_code_changes()reports unsynced repos, but_send_step_webhookfor the same PR iteration still emitsITERATION_COMPLETEDwhen_iteration_terminal_errored_reposis non-empty. After a terminal push failure, the webhook fires while the originating GitHub comment keeps the:eyes:reaction and never gets:hooray:.Additional Locations (1)
src/sentry/seer/autofix/on_completion_hook.py#L400-L403Reviewed by Cursor Bugbot for commit 7ce2525. Configure here.