From ef24b8f88f8e2aa34e8cdbef9b694ec5cbb6005e Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 23 Oct 2026 16:27:47 -0700 Subject: [PATCH 1/4] feat(investigations): Add the action request apply endpoint Part of a series that adds follow-up actions to Seer investigations: when a run completes, Seer suggests one next step, and the user can apply a typed action, such as a monitor change, with one click. This adds an endpoint that applies a suggested action request as the current user, or with dryRun only checks whether it would apply. Each action kind does its own checks and change, so the endpoint works the same for every kind. Depends on https://github.com/getsentry/sentry/pull/128680 --- src/sentry/api/urls.py | 8 + src/sentry/apidocs/parameters.py | 7 + ...tion_investigation_action_request_apply.py | 95 ++++++++++++ .../endpoints/serializers/action_request.py | 16 +- .../endpoints/serializers/responses.py | 7 + .../endpoints/validators/__init__.py | 2 + .../endpoints/validators/action_request.py | 12 ++ .../services/action_kinds/__init__.py | 21 ++- .../services/action_kinds/base.py | 63 +++++++- .../services/action_requests.py | 129 +++++++++++++++- .../utils/api/knownSentryApiUrls.generated.ts | 1 + ...tion_investigation_action_request_apply.py | 143 ++++++++++++++++++ 12 files changed, 492 insertions(+), 12 deletions(-) create mode 100644 src/sentry/investigations/endpoints/organization_investigation_action_request_apply.py create mode 100644 src/sentry/investigations/endpoints/validators/action_request.py diff --git a/src/sentry/api/urls.py b/src/sentry/api/urls.py index e15d16be4dfc..ac83761c2d05 100644 --- a/src/sentry/api/urls.py +++ b/src/sentry/api/urls.py @@ -296,6 +296,9 @@ from sentry.integrations.api.endpoints.organization_repository_settings import ( OrganizationRepositorySettingsEndpoint, ) +from sentry.investigations.endpoints.organization_investigation_action_request_apply import ( + OrganizationInvestigationActionRequestApplyEndpoint, +) from sentry.investigations.endpoints.organization_investigation_block_details import ( OrganizationInvestigationBlockDetailsEndpoint, ) @@ -2452,6 +2455,11 @@ def create_group_urls( OrganizationInvestigationsDuplicateEndpoint.as_view(), name="sentry-api-0-organization-investigation-duplicate", ), + re_path( + r"^(?P[^/]+)/investigations/(?P[^/]+)/action-requests/(?P[^/]+)/apply/$", + OrganizationInvestigationActionRequestApplyEndpoint.as_view(), + name="sentry-api-0-organization-investigation-action-request-apply", + ), re_path( r"^(?P[^/]+)/investigations/(?P[^/]+)/presence/$", OrganizationInvestigationPresenceEndpoint.as_view(), diff --git a/src/sentry/apidocs/parameters.py b/src/sentry/apidocs/parameters.py index 5be67561fdd7..62d972213b39 100644 --- a/src/sentry/apidocs/parameters.py +++ b/src/sentry/apidocs/parameters.py @@ -1536,6 +1536,13 @@ class InvestigationParams: type=str, description="The ID of an investigation in this organization.", ) + ACTION_REQUEST_ID = OpenApiParameter( + name="action_request_id", + location="path", + required=True, + type=str, + description="The ID of an action request in this investigation.", + ) CURSOR = OpenApiParameter( name="cursor", location="query", diff --git a/src/sentry/investigations/endpoints/organization_investigation_action_request_apply.py b/src/sentry/investigations/endpoints/organization_investigation_action_request_apply.py new file mode 100644 index 000000000000..edeca4153d5c --- /dev/null +++ b/src/sentry/investigations/endpoints/organization_investigation_action_request_apply.py @@ -0,0 +1,95 @@ +from __future__ import annotations + +from drf_spectacular.utils import extend_schema +from rest_framework import status +from rest_framework.request import Request +from rest_framework.response import Response + +from sentry.api.api_owners import ApiOwner +from sentry.api.api_publish_status import ApiPublishStatus +from sentry.api.base import cell_silo_endpoint +from sentry.api.serializers import serialize +from sentry.apidocs import constants +from sentry.apidocs.parameters import GlobalParams, InvestigationParams +from sentry.apidocs.utils import inline_sentry_response_serializer +from sentry.investigations.endpoints.base import ( + OrganizationInvestigationEndpoint, + require_authenticated_user, + service_error, +) +from sentry.investigations.endpoints.serializers import InvestigationActionRequestSerializer +from sentry.investigations.endpoints.serializers.responses import ( + InvestigationActionRequestApplyResponse, +) +from sentry.investigations.endpoints.validators import ActionRequestApplyValidator +from sentry.investigations.models import Investigation +from sentry.investigations.services.action_requests import apply_action_request +from sentry.models.organization import Organization + + +@extend_schema(tags=["Investigations"]) +@cell_silo_endpoint +class OrganizationInvestigationActionRequestApplyEndpoint(OrganizationInvestigationEndpoint): + owner = ApiOwner.ML_AI + publish_status = {"POST": ApiPublishStatus.PRIVATE} + + @extend_schema( + operation_id="Apply an Investigation Action Request", + parameters=[ + GlobalParams.ORG_ID_OR_SLUG, + InvestigationParams.INVESTIGATION_ID, + InvestigationParams.ACTION_REQUEST_ID, + ], + request=ActionRequestApplyValidator, + responses={ + 200: inline_sentry_response_serializer( + "InvestigationActionRequestApplyResponse", + InvestigationActionRequestApplyResponse, + ), + 400: constants.RESPONSE_BAD_REQUEST, + 401: constants.RESPONSE_UNAUTHORIZED, + 403: constants.RESPONSE_FORBIDDEN, + 404: constants.RESPONSE_NOT_FOUND, + 409: constants.RESPONSE_CONFLICT, + }, + ) + def post( + self, + request: Request, + organization: Organization, + investigation: Investigation, + action_request_id: str, + ) -> Response: + """ + Apply a suggested action request as the current user. Errors have a `code`. With + `dryRun`, it runs the same checks and changes nothing. + + Responds with a 409 if Seer withdrew the request. + """ + actor_id = require_authenticated_user(request) + validator = ActionRequestApplyValidator(data=request.data) + if not validator.is_valid(): + return Response(validator.errors, status=status.HTTP_400_BAD_REQUEST) + try: + result = apply_action_request( + investigation=investigation, + action_request_id=action_request_id, + request=request, + actor_id=actor_id, + dry_run=validator.validated_data["dry_run"], + ) + except Exception as error: + response = service_error(error) + if response is not None: + return response + raise + if result.error is not None: + return Response(result.error, status=result.status) + body: InvestigationActionRequestApplyResponse = { + "actionRequest": serialize( + result.action_request, + request.user, + InvestigationActionRequestSerializer(access=request.access), + ), + } + return Response(body) diff --git a/src/sentry/investigations/endpoints/serializers/action_request.py b/src/sentry/investigations/endpoints/serializers/action_request.py index 112039ba5f7b..9faba3e16ed8 100644 --- a/src/sentry/investigations/endpoints/serializers/action_request.py +++ b/src/sentry/investigations/endpoints/serializers/action_request.py @@ -13,11 +13,12 @@ Investigation, InvestigationActionRequest, InvestigationActionRequestAnchorType, - InvestigationActionRequestState, InvestigationBlock, - InvestigationStatus, ) -from sentry.investigations.services.action_kinds import get_action_kind +from sentry.investigations.services.action_kinds import ( + get_action_kind, + shared_cannot_apply_reason, +) from sentry.investigations.services.action_kinds.base import CannotApplyReason from sentry.users.api.serializers.user import UserSerializerResponse from sentry.users.models.user import User @@ -99,12 +100,9 @@ def _cannot_apply_reasons( reasons: dict[int, CannotApplyReason | None] = {} by_kind: dict[str, list[InvestigationActionRequest]] = defaultdict(list) for item in item_list: - if investigations[item.investigation_id].status == InvestigationStatus.ARCHIVED: - reasons[item.id] = CannotApplyReason.INVESTIGATION_ARCHIVED - elif item.state != InvestigationActionRequestState.SUGGESTED: - reasons[item.id] = CannotApplyReason.NOT_SUGGESTED - elif get_action_kind(item.action_kind) is None: - reasons[item.id] = CannotApplyReason.UNSUPPORTED_ACTION + reason = shared_cannot_apply_reason(investigations[item.investigation_id], item) + if reason is not None: + reasons[item.id] = reason elif access is None: reasons[item.id] = CannotApplyReason.NO_ACCESS else: diff --git a/src/sentry/investigations/endpoints/serializers/responses.py b/src/sentry/investigations/endpoints/serializers/responses.py index 8f12171bc023..bb5095ff926c 100644 --- a/src/sentry/investigations/endpoints/serializers/responses.py +++ b/src/sentry/investigations/endpoints/serializers/responses.py @@ -1,6 +1,9 @@ from datetime import datetime from typing import Any, NotRequired, TypedDict +from sentry.investigations.endpoints.serializers.action_request import ( + InvestigationActionRequestSerializerResponse, +) from sentry.investigations.endpoints.serializers.investigation import ( InvestigationOrchestrationSerializerResponse, ) @@ -258,3 +261,7 @@ class InvestigationPresenceResponse(TypedDict): class InvestigationTitlePreviewResponse(TypedDict): status: str | None preview: str | None + + +class InvestigationActionRequestApplyResponse(TypedDict): + actionRequest: InvestigationActionRequestSerializerResponse diff --git a/src/sentry/investigations/endpoints/validators/__init__.py b/src/sentry/investigations/endpoints/validators/__init__.py index c69812b7a196..cdaafe722f38 100644 --- a/src/sentry/investigations/endpoints/validators/__init__.py +++ b/src/sentry/investigations/endpoints/validators/__init__.py @@ -1,4 +1,5 @@ __all__ = ( + "ActionRequestApplyValidator", "BlockCreateValidator", "BlockDeleteValidator", "BlockExecutionResumeValidator", @@ -20,6 +21,7 @@ ) +from .action_request import ActionRequestApplyValidator from .base import StrictCamelSnakeValidator from .block import ( BlockCreateValidator, diff --git a/src/sentry/investigations/endpoints/validators/action_request.py b/src/sentry/investigations/endpoints/validators/action_request.py new file mode 100644 index 000000000000..138c3f481689 --- /dev/null +++ b/src/sentry/investigations/endpoints/validators/action_request.py @@ -0,0 +1,12 @@ +from __future__ import annotations + +from rest_framework import serializers + +from sentry.investigations.endpoints.validators.base import StrictCamelSnakeValidator + + +class ActionRequestApplyValidator(StrictCamelSnakeValidator): + dry_run = serializers.BooleanField( + default=False, + help_text="Run every check without applying, to see whether the apply would work.", + ) diff --git a/src/sentry/investigations/services/action_kinds/__init__.py b/src/sentry/investigations/services/action_kinds/__init__.py index 7fa4136191ba..c081123fa7e5 100644 --- a/src/sentry/investigations/services/action_kinds/__init__.py +++ b/src/sentry/investigations/services/action_kinds/__init__.py @@ -1,6 +1,12 @@ from __future__ import annotations -from sentry.investigations.services.action_kinds.base import ActionKind +from sentry.investigations.models import ( + Investigation, + InvestigationActionRequest, + InvestigationActionRequestState, + InvestigationStatus, +) +from sentry.investigations.services.action_kinds.base import ActionKind, CannotApplyReason from sentry.investigations.services.action_kinds.connect_telemetry import ConnectTelemetryKind from sentry.investigations.services.action_kinds.update_detector import UpdateDetectorKind @@ -11,3 +17,16 @@ def get_action_kind(kind: str | None) -> ActionKind | None: return ACTION_KINDS.get(kind) if kind is not None else None + + +def shared_cannot_apply_reason( + investigation: Investigation, action_request: InvestigationActionRequest +) -> CannotApplyReason | None: + """Why no one can apply this request, whatever its kind, or None if its kind decides.""" + if investigation.status == InvestigationStatus.ARCHIVED: + return CannotApplyReason.INVESTIGATION_ARCHIVED + if action_request.state != InvestigationActionRequestState.SUGGESTED: + return CannotApplyReason.NOT_SUGGESTED + if get_action_kind(action_request.action_kind) is None: + return CannotApplyReason.UNSUPPORTED_ACTION + return None diff --git a/src/sentry/investigations/services/action_kinds/base.py b/src/sentry/investigations/services/action_kinds/base.py index 55efc2c27e5a..76552309ae71 100644 --- a/src/sentry/investigations/services/action_kinds/base.py +++ b/src/sentry/investigations/services/action_kinds/base.py @@ -2,13 +2,20 @@ import abc from collections.abc import Sequence +from dataclasses import dataclass from enum import StrEnum from django.contrib.auth.models import AnonymousUser +from rest_framework import status +from rest_framework.request import Request from sentry.auth.access import Access from sentry.investigations.contracts import ActionPayloadSerializer -from sentry.investigations.models import Investigation, InvestigationActionRequest +from sentry.investigations.models import ( + Investigation, + InvestigationActionRequest, + InvestigationOrchestrationRun, +) from sentry.users.models.user import User from sentry.users.services.user.model import RpcUser @@ -26,6 +33,47 @@ class CannotApplyReason(StrEnum): DYNAMIC_DETECTOR = "dynamic_detector" +@dataclass(frozen=True) +class ApplyResult: + action_request: InvestigationActionRequest + status: int = status.HTTP_200_OK + error: dict[str, object] | None = None + + +@dataclass(frozen=True) +class ApplyContext: + """When applying, the caller holds the investigation, run, and request row locks.""" + + investigation: Investigation + run: InvestigationOrchestrationRun + action_request: InvestigationActionRequest + request: Request + actor_id: int + + +def apply_error( + action_request: InvestigationActionRequest, + status_code: int, + detail: str, + code: str, + **extra: object, +) -> ApplyResult: + return ApplyResult( + action_request=action_request, + status=status_code, + error={"detail": detail, "code": code, **extra}, + ) + + +def unsupported_action(action_request: InvestigationActionRequest) -> ApplyResult: + return apply_error( + action_request, + status.HTTP_400_BAD_REQUEST, + "This request has no action to apply.", + "unsupported_action", + ) + + class ActionKind(abc.ABC): """Everything Sentry needs to check, apply, and record one kind of action.""" @@ -43,6 +91,19 @@ def cannot_apply_reasons( """For each suggested request, why the viewer can't apply it, or None if they can.""" return {item.id: CannotApplyReason.UNSUPPORTED_ACTION for item in action_requests} + def check(self, context: ApplyContext) -> ApplyResult | None: + """Run the checks apply would, without changing anything. Returns the error apply would + return, or None if it would apply. cannot_apply_reasons has already passed. + """ + return unsupported_action(context.action_request) + + def apply(self, context: ApplyContext) -> ApplyResult: + """Make the change. cannot_apply_reasons has already passed. If an error updates the row, + return it rather than raise, so the update still commits. + """ + + return unsupported_action(context.action_request) + def after_upsert(self, action_request: InvestigationActionRequest) -> None: """Runs when Seer creates or changes a request, in the same transaction. Only requests that are still suggested once the whole event is handled get it. diff --git a/src/sentry/investigations/services/action_requests.py b/src/sentry/investigations/services/action_requests.py index 3f747c2ffe4d..bd56186285c8 100644 --- a/src/sentry/investigations/services/action_requests.py +++ b/src/sentry/investigations/services/action_requests.py @@ -3,9 +3,13 @@ import logging from collections.abc import Sequence +from django.db import router, transaction from django.db.models import Q, QuerySet from django.utils import timezone +from rest_framework import status +from rest_framework.request import Request +from sentry.api.exceptions import ResourceDoesNotExist from sentry.investigations.contracts import ActionRequestItem from sentry.investigations.models import ( Investigation, @@ -14,7 +18,14 @@ InvestigationActionRequestState, InvestigationOrchestrationRun, ) -from sentry.investigations.services.action_kinds import get_action_kind +from sentry.investigations.services.action_kinds import get_action_kind, shared_cannot_apply_reason +from sentry.investigations.services.action_kinds.base import ( + ApplyContext, + ApplyResult, + CannotApplyReason, + apply_error, + unsupported_action, +) logger = logging.getLogger(__name__) @@ -214,3 +225,119 @@ def supersede_block_action_requests(investigation: Investigation, block_keys: Se anchor_id__in=block_keys, ) ) + + +def _not_suggested_error(action_request: InvestigationActionRequest) -> ApplyResult: + if action_request.state == InvestigationActionRequestState.APPLIED: + return apply_error( + action_request, + status.HTTP_409_CONFLICT, + "This action was already applied.", + "already_applied", + ) + if action_request.state == InvestigationActionRequestState.WITHDRAWN: + return apply_error( + action_request, + status.HTTP_409_CONFLICT, + "Seer no longer suggests this.", + "withdrawn", + reason=action_request.reason, + ) + return apply_error( + action_request, + status.HTTP_409_CONFLICT, + "This action can no longer be applied.", + "not_suggested", + ) + + +def _apply( + *, + investigation: Investigation, + action_request_id: str, + request: Request, + actor_id: int, + dry_run: bool, +) -> ApplyResult: + investigations = Investigation.objects.select_related("organization") + runs = InvestigationOrchestrationRun.objects.all() + action_requests = InvestigationActionRequest.objects.all() + if not dry_run: + investigations = investigations.select_for_update(of=("self",)) + runs = runs.select_for_update() + action_requests = action_requests.select_for_update() + + locked = investigations.get(id=investigation.id) + try: + run = runs.get(investigation=locked) + except InvestigationOrchestrationRun.DoesNotExist: + raise ResourceDoesNotExist + try: + action_request = action_requests.get(id=action_request_id, investigation=locked) + except (InvestigationActionRequest.DoesNotExist, ValueError): + raise ResourceDoesNotExist + + match shared_cannot_apply_reason(locked, action_request): + case CannotApplyReason.INVESTIGATION_ARCHIVED: + return apply_error( + action_request, + status.HTTP_400_BAD_REQUEST, + "Archived investigations are read-only.", + CannotApplyReason.INVESTIGATION_ARCHIVED, + ) + case CannotApplyReason.NOT_SUGGESTED: + return _not_suggested_error(action_request) + case CannotApplyReason.UNSUPPORTED_ACTION: + return unsupported_action(action_request) + kind = get_action_kind(action_request.action_kind) + assert kind is not None + # The same check that decides canApply guards the endpoint, so a kind can't forget it. + reason = kind.cannot_apply_reasons( + [action_request], + investigations={locked.id: locked}, + access=request.access, + user=request.user, + ).get(action_request.id, CannotApplyReason.UNSUPPORTED_ACTION) + if reason is not None: + return apply_error( + action_request, + status.HTTP_403_FORBIDDEN + if reason == CannotApplyReason.PERMISSION_DENIED + else status.HTTP_409_CONFLICT, + "This action can't be applied.", + reason, + ) + context = ApplyContext( + investigation=locked, + run=run, + action_request=action_request, + request=request, + actor_id=actor_id, + ) + if dry_run: + return kind.check(context) or ApplyResult(action_request=action_request) + return kind.apply(context) + + +def apply_action_request( + *, + investigation: Investigation, + action_request_id: str, + request: Request, + actor_id: int, + dry_run: bool = False, +) -> ApplyResult: + """Apply a suggested action as the requesting user. A dry run only says whether it would + work, and doesn't take locks or change anything. + + If an error updates the row, it's returned instead of raised, so the update still commits. + """ + + with transaction.atomic(using=router.db_for_write(InvestigationActionRequest)): + return _apply( + investigation=investigation, + action_request_id=action_request_id, + request=request, + actor_id=actor_id, + dry_run=dry_run, + ) diff --git a/static/app/utils/api/knownSentryApiUrls.generated.ts b/static/app/utils/api/knownSentryApiUrls.generated.ts index 93165daf8764..cdcd9e3b7c81 100644 --- a/static/app/utils/api/knownSentryApiUrls.generated.ts +++ b/static/app/utils/api/knownSentryApiUrls.generated.ts @@ -194,6 +194,7 @@ export type KnownSentryApiUrls = | '/organizations/$organizationIdOrSlug/intercom-jwt/' | '/organizations/$organizationIdOrSlug/investigations/' | '/organizations/$organizationIdOrSlug/investigations/$investigationId/' + | '/organizations/$organizationIdOrSlug/investigations/$investigationId/action-requests/$actionRequestId/apply/' | '/organizations/$organizationIdOrSlug/investigations/$investigationId/blocks/' | '/organizations/$organizationIdOrSlug/investigations/$investigationId/blocks/$blockId/' | '/organizations/$organizationIdOrSlug/investigations/$investigationId/blocks/$blockId/executions/' diff --git a/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py b/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py index ce3ca7d501cb..d2d95d6212c2 100644 --- a/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py +++ b/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py @@ -3,6 +3,7 @@ from collections.abc import Sequence from datetime import timedelta from typing import Any +from unittest import mock from django.urls import reverse @@ -11,6 +12,8 @@ from sentry.incidents.utils.subscription_limits import METRIC_SUBSCRIPTION_FEATURE_FLAGS from sentry.investigations.models import ( InvestigationActionRequest, + InvestigationActionRequestState, + InvestigationStatus, ) from sentry.seer.models.run import SeerRunMirrorStatus, SeerRunType from sentry.snuba.dataset import Dataset @@ -131,6 +134,29 @@ def suggest( investigation=self.investigation, key="widen-window", **values ) + def apply_url(self, action_request_id: int | str, investigation_id: int | None = None) -> str: + return reverse( + "sentry-api-0-organization-investigation-action-request-apply", + kwargs={ + "organization_id_or_slug": self.organization.slug, + "investigation_id": investigation_id or self.investigation.id, + "action_request_id": action_request_id, + }, + ) + + def apply(self, action_request: InvestigationActionRequest, **data: Any) -> Any: + with ( + self.tasks(), + mock.patch( + "sentry.tasks.seer.investigation.dispatch_investigation_orchestration_commands.delay" + ) as self.dispatch, + ): + return self.client.post(self.apply_url(action_request.id), data=data, format="json") + + def snuba_query(self, detector: Detector | None = None) -> SnubaQuery: + data_source = (detector or self.detector).data_sources.get() + return SnubaQuery.objects.get(subscriptions__id=int(data_source.source_id)) + def details_request(self) -> dict[str, Any]: response = self.client.get( reverse( @@ -168,3 +194,120 @@ def test_a_dataset_the_plan_no_longer_allows_cannot_apply(self) -> None: False, "detector_not_found", ) + + def test_requests_that_are_no_longer_suggested_conflict(self) -> None: + action_request = self.suggest(state=InvestigationActionRequestState.SUPERSEDED) + + response = self.apply(action_request) + + assert response.status_code == 409 + assert response.data["code"] == "not_suggested" + + def test_a_dry_run_runs_the_same_checks(self) -> None: + action_request = self.suggest(state=InvestigationActionRequestState.SUPERSEDED) + + response = self.apply(action_request, dryRun=True) + + assert response.status_code == 409 + assert response.data["code"] == "not_suggested" + + def test_a_dry_run_changes_nothing(self) -> None: + action_request = self.create_investigation_action_request( + investigation=self.investigation, + key="task", + action_kind="seer_task", + action_payload={"kind": "seer_task", "prompt": "Go"}, + ) + version = self.investigation.version + + response = self.apply(action_request, dryRun=True) + + assert response.status_code == 400 + assert response.data["code"] == "unsupported_action" + action_request.refresh_from_db() + self.investigation.refresh_from_db() + assert action_request.state == InvestigationActionRequestState.SUGGESTED + assert self.investigation.version == version + + def test_rejects_an_unknown_field(self) -> None: + response = self.apply(self.suggest(), dryRn=True) + + assert response.status_code == 400 + assert response.data == {"dryRn": "Unknown field."} + + def test_archived_investigations_are_read_only(self) -> None: + action_request = self.suggest() + self.investigation.update(status=InvestigationStatus.ARCHIVED) + + response = self.apply(action_request) + + assert response.status_code == 400 + assert response.data == { + "detail": "Archived investigations are read-only.", + "code": "investigation_archived", + } + + def test_the_kind_check_that_hides_the_button_also_guards_apply(self) -> None: + self.organization.update_option("sentry:alerts_member_write", False) + member = self.create_user() + self.create_member(user=member, organization=self.organization, role="member") + self.login_as(member) + action_request = self.suggest() + + response = self.apply(action_request) + + assert response.status_code == 403 + assert response.data["code"] == "permission_denied" + action_request.refresh_from_db() + assert action_request.state == InvestigationActionRequestState.SUGGESTED + + def test_requests_without_a_supported_action_cannot_apply(self) -> None: + for key, action in [("no-action", None), ("task", {"kind": "seer_task", "prompt": "Go"})]: + action_request = self.create_investigation_action_request( + investigation=self.investigation, + key=key, + action_kind=action["kind"] if action else None, + action_payload=action, + ) + + response = self.apply(action_request) + + assert response.status_code == 400 + assert response.data == { + "detail": "This request has no action to apply.", + "code": "unsupported_action", + } + + def test_a_withdrawn_request_is_no_longer_suggested(self) -> None: + action_request = self.suggest( + state=InvestigationActionRequestState.WITHDRAWN, reason="The spikes are real." + ) + + response = self.apply(action_request) + + assert response.status_code == 409 + assert response.data == { + "detail": "Seer no longer suggests this.", + "code": "withdrawn", + "reason": "The spikes are real.", + } + assert self.snuba_query().time_window == 300 + + def test_requests_from_other_investigations_are_not_found(self) -> None: + other = self.create_investigation(organization=self.organization, created_by=self.user) + self.create_investigation_orchestration_run(investigation=other) + action_request = self.suggest() + + response = self.client.post( + self.apply_url(action_request.id, investigation_id=other.id), data={}, format="json" + ) + assert response.status_code == 404 + assert self.client.post(self.apply_url("abc"), data={}, format="json").status_code == 404 + + def test_requires_the_feature(self) -> None: + action_request = self.suggest() + + with self.feature({FEATURE: False}): + response = self.client.post(self.apply_url(action_request.id), data={}, format="json") + + assert response.status_code == 404 From 085404927ada8d4597592a968422a75ca592ef7d Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 23 Oct 2026 17:50:45 -0700 Subject: [PATCH 2/4] ref(investigations): Record every apply outcome in the shared path Kinds now only make the change: they return what the activity feed shows, or raise when the action is stale or invalid, and the shared apply path updates the request. Error codes are typed, the endpoint picks the status, and a dry run rolls back whatever it touched. --- ...tion_investigation_action_request_apply.py | 39 ++--- .../services/action_kinds/base.py | 73 +++++---- .../services/action_requests.py | 152 +++++++++++------- ...tion_investigation_action_request_apply.py | 103 +++++++++++- tests/sentry/investigations/test_contracts.py | 23 ++- 5 files changed, 274 insertions(+), 116 deletions(-) diff --git a/src/sentry/investigations/endpoints/organization_investigation_action_request_apply.py b/src/sentry/investigations/endpoints/organization_investigation_action_request_apply.py index edeca4153d5c..9127482c2320 100644 --- a/src/sentry/investigations/endpoints/organization_investigation_action_request_apply.py +++ b/src/sentry/investigations/endpoints/organization_investigation_action_request_apply.py @@ -15,7 +15,6 @@ from sentry.investigations.endpoints.base import ( OrganizationInvestigationEndpoint, require_authenticated_user, - service_error, ) from sentry.investigations.endpoints.serializers import InvestigationActionRequestSerializer from sentry.investigations.endpoints.serializers.responses import ( @@ -23,9 +22,18 @@ ) from sentry.investigations.endpoints.validators import ActionRequestApplyValidator from sentry.investigations.models import Investigation +from sentry.investigations.services.action_kinds.base import ApplyErrorCode, CannotApplyReason from sentry.investigations.services.action_requests import apply_action_request from sentry.models.organization import Organization +_STATUS = { + CannotApplyReason.INVESTIGATION_ARCHIVED: status.HTTP_400_BAD_REQUEST, + CannotApplyReason.UNSUPPORTED_ACTION: status.HTTP_400_BAD_REQUEST, + ApplyErrorCode.VALIDATION_FAILED: status.HTTP_400_BAD_REQUEST, + CannotApplyReason.NO_ACCESS: status.HTTP_403_FORBIDDEN, + CannotApplyReason.PERMISSION_DENIED: status.HTTP_403_FORBIDDEN, +} + @extend_schema(tags=["Investigations"]) @cell_silo_endpoint @@ -61,30 +69,25 @@ def post( action_request_id: str, ) -> Response: """ - Apply a suggested action request as the current user. Errors have a `code`. With - `dryRun`, it runs the same checks and changes nothing. + Apply a suggested action request as the current user. With `dryRun`, it runs the same + checks and changes nothing. - Responds with a 409 if Seer withdrew the request. + Errors about the request have a `code`. It's a 409 when the request was applied, + withdrawn or replaced, or its kind refuses it, and a 403 when the viewer can't apply it. """ actor_id = require_authenticated_user(request) validator = ActionRequestApplyValidator(data=request.data) if not validator.is_valid(): return Response(validator.errors, status=status.HTTP_400_BAD_REQUEST) - try: - result = apply_action_request( - investigation=investigation, - action_request_id=action_request_id, - request=request, - actor_id=actor_id, - dry_run=validator.validated_data["dry_run"], - ) - except Exception as error: - response = service_error(error) - if response is not None: - return response - raise + result = apply_action_request( + investigation=investigation, + action_request_id=action_request_id, + request=request, + actor_id=actor_id, + dry_run=validator.validated_data["dry_run"], + ) if result.error is not None: - return Response(result.error, status=result.status) + return Response(result.error, status=_STATUS.get(result.error["code"], 409)) body: InvestigationActionRequestApplyResponse = { "actionRequest": serialize( result.action_request, diff --git a/src/sentry/investigations/services/action_kinds/base.py b/src/sentry/investigations/services/action_kinds/base.py index 76552309ae71..b13f86c573dd 100644 --- a/src/sentry/investigations/services/action_kinds/base.py +++ b/src/sentry/investigations/services/action_kinds/base.py @@ -4,9 +4,9 @@ from collections.abc import Sequence from dataclasses import dataclass from enum import StrEnum +from typing import Any, NotRequired, TypedDict from django.contrib.auth.models import AnonymousUser -from rest_framework import status from rest_framework.request import Request from sentry.auth.access import Access @@ -33,11 +33,30 @@ class CannotApplyReason(StrEnum): DYNAMIC_DETECTOR = "dynamic_detector" +class ApplyErrorCode(StrEnum): + """What else the apply endpoint can say went wrong. The UI switches on these.""" + + ALREADY_APPLIED = "already_applied" + WITHDRAWN = "withdrawn" + STALE = "stale" + VALIDATION_FAILED = "validation_failed" + + +class ApplyError(TypedDict): + detail: str + code: CannotApplyReason | ApplyErrorCode + # Seer's reason for withdrawing it. + reason: NotRequired[str | None] + # The settings that moved since Seer suggested the change. + current: NotRequired[dict[str, object]] + # Why the change wouldn't save. + messages: NotRequired[list[str]] + + @dataclass(frozen=True) class ApplyResult: action_request: InvestigationActionRequest - status: int = status.HTTP_200_OK - error: dict[str, object] | None = None + error: ApplyError | None = None @dataclass(frozen=True) @@ -51,27 +70,20 @@ class ApplyContext: actor_id: int -def apply_error( - action_request: InvestigationActionRequest, - status_code: int, - detail: str, - code: str, - **extra: object, -) -> ApplyResult: - return ApplyResult( - action_request=action_request, - status=status_code, - error={"detail": detail, "code": code, **extra}, - ) +class ActionStale(Exception): + """What the action changes moved since Seer suggested it.""" + + def __init__(self, current: dict[str, object]) -> None: + super().__init__("The action is stale.") + self.current = current -def unsupported_action(action_request: InvestigationActionRequest) -> ApplyResult: - return apply_error( - action_request, - status.HTTP_400_BAD_REQUEST, - "This request has no action to apply.", - "unsupported_action", - ) +class ActionInvalid(Exception): + """The change wouldn't save. `errors` are the validator's errors.""" + + def __init__(self, errors: dict[str, Any] | list[Any]) -> None: + super().__init__("The action is invalid.") + self.errors = errors class ActionKind(abc.ABC): @@ -91,18 +103,17 @@ def cannot_apply_reasons( """For each suggested request, why the viewer can't apply it, or None if they can.""" return {item.id: CannotApplyReason.UNSUPPORTED_ACTION for item in action_requests} - def check(self, context: ApplyContext) -> ApplyResult | None: - """Run the checks apply would, without changing anything. Returns the error apply would - return, or None if it would apply. cannot_apply_reasons has already passed. + def check(self, context: ApplyContext) -> None: + """Run the checks apply would, without changing anything. Raise ActionStale or + ActionInvalid where apply would. Kinds with nothing to check beyond + cannot_apply_reasons can leave this as is. """ - return unsupported_action(context.action_request) - def apply(self, context: ApplyContext) -> ApplyResult: - """Make the change. cannot_apply_reasons has already passed. If an error updates the row, - return it rather than raise, so the update still commits. + def apply(self, context: ApplyContext) -> dict[str, object]: + """Make the change, and return what the activity feed should show about it. Raise + ActionStale or ActionInvalid instead of updating the request; the caller records it. """ - - return unsupported_action(context.action_request) + raise NotImplementedError def after_upsert(self, action_request: InvestigationActionRequest) -> None: """Runs when Seer creates or changes a request, in the same transaction. Only requests diff --git a/src/sentry/investigations/services/action_requests.py b/src/sentry/investigations/services/action_requests.py index bd56186285c8..bf53ed18bb9c 100644 --- a/src/sentry/investigations/services/action_requests.py +++ b/src/sentry/investigations/services/action_requests.py @@ -6,7 +6,6 @@ from django.db import router, transaction from django.db.models import Q, QuerySet from django.utils import timezone -from rest_framework import status from rest_framework.request import Request from sentry.api.exceptions import ResourceDoesNotExist @@ -20,11 +19,13 @@ ) from sentry.investigations.services.action_kinds import get_action_kind, shared_cannot_apply_reason from sentry.investigations.services.action_kinds.base import ( + ActionInvalid, + ActionStale, ApplyContext, + ApplyError, + ApplyErrorCode, ApplyResult, CannotApplyReason, - apply_error, - unsupported_action, ) logger = logging.getLogger(__name__) @@ -227,28 +228,37 @@ def supersede_block_action_requests(investigation: Investigation, block_keys: Se ) -def _not_suggested_error(action_request: InvestigationActionRequest) -> ApplyResult: +# The usual detail for a reason; anything a kind refuses gets the last one. +_REASON_DETAILS = { + CannotApplyReason.INVESTIGATION_ARCHIVED: "Archived investigations are read-only.", + CannotApplyReason.UNSUPPORTED_ACTION: "This request has no action to apply.", +} + + +def _not_suggested_error(action_request: InvestigationActionRequest) -> ApplyError: if action_request.state == InvestigationActionRequestState.APPLIED: - return apply_error( - action_request, - status.HTTP_409_CONFLICT, - "This action was already applied.", - "already_applied", - ) + return { + "detail": "This action was already applied.", + "code": ApplyErrorCode.ALREADY_APPLIED, + } if action_request.state == InvestigationActionRequestState.WITHDRAWN: - return apply_error( - action_request, - status.HTTP_409_CONFLICT, - "Seer no longer suggests this.", - "withdrawn", - reason=action_request.reason, - ) - return apply_error( - action_request, - status.HTTP_409_CONFLICT, - "This action can no longer be applied.", - "not_suggested", - ) + return { + "detail": "Seer no longer suggests this.", + "code": ApplyErrorCode.WITHDRAWN, + "reason": action_request.reason, + } + return { + "detail": "This action can no longer be applied.", + "code": CannotApplyReason.NOT_SUGGESTED, + } + + +def _error_messages(errors: object) -> list[str]: + if isinstance(errors, dict): + return [message for value in errors.values() for message in _error_messages(value)] + if isinstance(errors, list): + return [message for value in errors for message in _error_messages(value)] + return [str(errors)] def _apply( @@ -277,36 +287,23 @@ def _apply( except (InvestigationActionRequest.DoesNotExist, ValueError): raise ResourceDoesNotExist - match shared_cannot_apply_reason(locked, action_request): - case CannotApplyReason.INVESTIGATION_ARCHIVED: - return apply_error( - action_request, - status.HTTP_400_BAD_REQUEST, - "Archived investigations are read-only.", - CannotApplyReason.INVESTIGATION_ARCHIVED, - ) - case CannotApplyReason.NOT_SUGGESTED: - return _not_suggested_error(action_request) - case CannotApplyReason.UNSUPPORTED_ACTION: - return unsupported_action(action_request) + reason = shared_cannot_apply_reason(locked, action_request) + if reason == CannotApplyReason.NOT_SUGGESTED: + return ApplyResult(action_request, _not_suggested_error(action_request)) kind = get_action_kind(action_request.action_kind) - assert kind is not None - # The same check that decides canApply guards the endpoint, so a kind can't forget it. - reason = kind.cannot_apply_reasons( - [action_request], - investigations={locked.id: locked}, - access=request.access, - user=request.user, - ).get(action_request.id, CannotApplyReason.UNSUPPORTED_ACTION) + if reason is None and kind is not None: + # The same check that decides canApply guards the endpoint, so a kind can't forget it. + reason = kind.cannot_apply_reasons( + [action_request], + investigations={locked.id: locked}, + access=request.access, + user=request.user, + ).get(action_request.id, CannotApplyReason.UNSUPPORTED_ACTION) if reason is not None: - return apply_error( - action_request, - status.HTTP_403_FORBIDDEN - if reason == CannotApplyReason.PERMISSION_DENIED - else status.HTTP_409_CONFLICT, - "This action can't be applied.", - reason, - ) + detail = _REASON_DETAILS.get(reason, "This action can't be applied.") + return ApplyResult(action_request, {"detail": detail, "code": reason}) + assert kind is not None + context = ApplyContext( investigation=locked, run=run, @@ -314,9 +311,45 @@ def _apply( request=request, actor_id=actor_id, ) - if dry_run: - return kind.check(context) or ApplyResult(action_request=action_request) - return kind.apply(context) + try: + if dry_run: + kind.check(context) + return ApplyResult(action_request) + details = kind.apply(context) + except ActionStale as stale: + if not dry_run: + # Keep the error on the row, so Seer sending the same suggestion again can't bring + # it back. + action_request.update( + state=InvestigationActionRequestState.SUPERSEDED, + error={"code": ApplyErrorCode.STALE, "current": stale.current}, + ) + return ApplyResult( + action_request, + { + "detail": "This changed since Seer suggested it.", + "code": ApplyErrorCode.STALE, + "current": stale.current, + }, + ) + except ActionInvalid as invalid: + # The request stays suggested, so the user can try again. + return ApplyResult( + action_request, + { + "detail": "The change wasn't valid.", + "code": ApplyErrorCode.VALIDATION_FAILED, + "messages": _error_messages(invalid.errors), + }, + ) + action_request.update( + state=InvestigationActionRequestState.APPLIED, + applied_by_id=actor_id, + date_applied=timezone.now(), + applied_details=details, + error=None, + ) + return ApplyResult(action_request) def apply_action_request( @@ -329,15 +362,18 @@ def apply_action_request( ) -> ApplyResult: """Apply a suggested action as the requesting user. A dry run only says whether it would work, and doesn't take locks or change anything. - - If an error updates the row, it's returned instead of raised, so the update still commits. """ - with transaction.atomic(using=router.db_for_write(InvestigationActionRequest)): - return _apply( + using = router.db_for_write(InvestigationActionRequest) + with transaction.atomic(using=using): + result = _apply( investigation=investigation, action_request_id=action_request_id, request=request, actor_id=actor_id, dry_run=dry_run, ) + if dry_run: + # Even if a check writes by mistake, a dry run keeps nothing. + transaction.set_rollback(True, using=using) + return result diff --git a/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py b/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py index d2d95d6212c2..09d3e59c4bc3 100644 --- a/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py +++ b/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py @@ -3,7 +3,6 @@ from collections.abc import Sequence from datetime import timedelta from typing import Any -from unittest import mock from django.urls import reverse @@ -15,6 +14,8 @@ InvestigationActionRequestState, InvestigationStatus, ) +from sentry.investigations.services.action_kinds import ACTION_KINDS +from sentry.investigations.services.action_kinds.base import ActionInvalid, ActionStale from sentry.seer.models.run import SeerRunMirrorStatus, SeerRunType from sentry.snuba.dataset import Dataset from sentry.snuba.models import ExtrapolationMode, SnubaQuery, SnubaQueryEventType @@ -25,6 +26,7 @@ from sentry.workflow_engine.models import DataConditionGroup, Detector from sentry.workflow_engine.models.data_condition import Condition from sentry.workflow_engine.types import DetectorPriorityLevel +from tests.sentry.investigations.test_contracts import RecordingKind, fake_action, fake_action_kind pytestmark = [requires_snuba, requires_kafka] @@ -145,13 +147,7 @@ def apply_url(self, action_request_id: int | str, investigation_id: int | None = ) def apply(self, action_request: InvestigationActionRequest, **data: Any) -> Any: - with ( - self.tasks(), - mock.patch( - "sentry.tasks.seer.investigation.dispatch_investigation_orchestration_commands.delay" - ) as self.dispatch, - ): - return self.client.post(self.apply_url(action_request.id), data=data, format="json") + return self.client.post(self.apply_url(action_request.id), data=data, format="json") def snuba_query(self, detector: Detector | None = None) -> SnubaQuery: data_source = (detector or self.detector).data_sources.get() @@ -311,3 +307,94 @@ def test_requires_the_feature(self) -> None: response = self.client.post(self.apply_url(action_request.id), data={}, format="json") assert response.status_code == 404 + + +@with_feature(FEATURE) +class SharedApplyPathTest(APITestCase): + def setUp(self) -> None: + super().setUp() + self.login_as(self.user) + kind = fake_action_kind() + kind.start() + self.addCleanup(kind.stop) + recording = ACTION_KINDS["test_kind"] + assert isinstance(recording, RecordingKind) + self.kind = recording + self.investigation = self.create_investigation( + organization=self.organization, created_by=self.user + ) + self.create_investigation_orchestration_run(investigation=self.investigation) + self.action_request = self.create_investigation_action_request( + investigation=self.investigation, + key="test", + action_kind="test_kind", + action_payload=fake_action(), + ) + + def post(self, **data: Any) -> Any: + url = reverse( + "sentry-api-0-organization-investigation-action-request-apply", + kwargs={ + "organization_id_or_slug": self.organization.slug, + "investigation_id": self.investigation.id, + "action_request_id": self.action_request.id, + }, + ) + return self.client.post(url, data=data, format="json") + + def test_a_dry_run_only_checks(self) -> None: + response = self.post(dryRun=True) + + assert response.status_code == 200, response.data + assert self.kind.calls == ["check"] + self.action_request.refresh_from_db() + assert self.action_request.state == InvestigationActionRequestState.SUGGESTED + + def test_applying_records_what_the_kind_returns(self) -> None: + response = self.post() + + assert response.status_code == 200, response.data + assert self.kind.calls == ["apply"] + self.action_request.refresh_from_db() + assert self.action_request.state == InvestigationActionRequestState.APPLIED + assert self.action_request.applied_by_id == self.user.id + assert self.action_request.applied_details == {"target": "a"} + + def test_a_stale_action_is_superseded_with_what_moved(self) -> None: + self.kind.raises = ActionStale({"window": 600}) + + response = self.post() + + assert response.status_code == 409 + assert response.data == { + "detail": "This changed since Seer suggested it.", + "code": "stale", + "current": {"window": 600}, + } + self.action_request.refresh_from_db() + assert self.action_request.state == InvestigationActionRequestState.SUPERSEDED + assert self.action_request.error == {"code": "stale", "current": {"window": 600}} + + def test_a_stale_dry_run_changes_nothing(self) -> None: + self.kind.raises = ActionStale({"window": 600}) + + response = self.post(dryRun=True) + + assert response.status_code == 409 + self.action_request.refresh_from_db() + assert self.action_request.state == InvestigationActionRequestState.SUGGESTED + assert self.action_request.error is None + + def test_an_invalid_action_stays_suggested(self) -> None: + self.kind.raises = ActionInvalid({"window": ["Pick a supported window."]}) + + response = self.post() + + assert response.status_code == 400 + assert response.data == { + "detail": "The change wasn't valid.", + "code": "validation_failed", + "messages": ["Pick a supported window."], + } + self.action_request.refresh_from_db() + assert self.action_request.state == InvestigationActionRequestState.SUGGESTED diff --git a/tests/sentry/investigations/test_contracts.py b/tests/sentry/investigations/test_contracts.py index 3e29a4a2fbc3..6742bbaefc92 100644 --- a/tests/sentry/investigations/test_contracts.py +++ b/tests/sentry/investigations/test_contracts.py @@ -34,7 +34,11 @@ ) from sentry.investigations.models import Investigation, InvestigationActionRequest from sentry.investigations.services.action_kinds import ACTION_KINDS -from sentry.investigations.services.action_kinds.base import ActionKind, CannotApplyReason +from sentry.investigations.services.action_kinds.base import ( + ActionKind, + ApplyContext, + CannotApplyReason, +) from sentry.users.models.user import User from sentry.users.services.user.model import RpcUser from sentry.utils import json @@ -654,6 +658,10 @@ def __init__(self) -> None: self.upserted: list[InvestigationActionRequest] = [] # The arguments from each cannot_apply_reasons call. self.checked: list[tuple[list[int], list[int], Access, User | RpcUser | AnonymousUser]] = [] + # Which of check and apply ran, in order. + self.calls: list[str] = [] + # What check and apply raise, if anything. + self.raises: Exception | None = None def after_upsert(self, action_request: InvestigationActionRequest) -> None: self.upserted.append(action_request) @@ -671,6 +679,19 @@ def cannot_apply_reasons( ) return {item.id: None for item in action_requests} + def check(self, context: ApplyContext) -> None: + self.calls.append("check") + if self.raises is not None: + raise self.raises + + def apply(self, context: ApplyContext) -> dict[str, object]: + self.calls.append("apply") + if self.raises is not None: + raise self.raises + payload = context.action_request.action_payload + assert payload is not None + return {"target": payload["target"]} + def fake_action(target: str = "a") -> dict[str, Any]: return {"kind": "test_kind", "target": target} From 05c7200b40fe680820258928fc9048ccf0096ce9 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 23 Oct 2026 18:17:19 -0700 Subject: [PATCH 3/4] ref(investigations): Type the error stored on an action request --- .../endpoints/serializers/action_request.py | 9 +++++---- .../investigations/services/action_kinds/base.py | 16 ++++++++++++++-- .../investigations/services/action_requests.py | 3 ++- 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/sentry/investigations/endpoints/serializers/action_request.py b/src/sentry/investigations/endpoints/serializers/action_request.py index 9faba3e16ed8..722344cd5c5a 100644 --- a/src/sentry/investigations/endpoints/serializers/action_request.py +++ b/src/sentry/investigations/endpoints/serializers/action_request.py @@ -3,7 +3,7 @@ from collections import defaultdict from collections.abc import Mapping, MutableMapping, Sequence from datetime import datetime -from typing import Any, TypedDict, override +from typing import Any, TypedDict, cast, override from django.contrib.auth.models import AnonymousUser @@ -19,7 +19,7 @@ get_action_kind, shared_cannot_apply_reason, ) -from sentry.investigations.services.action_kinds.base import CannotApplyReason +from sentry.investigations.services.action_kinds.base import ActionRequestError, CannotApplyReason from sentry.users.api.serializers.user import UserSerializerResponse from sentry.users.models.user import User from sentry.users.services.user.model import RpcUser @@ -62,7 +62,7 @@ class InvestigationActionRequestSerializerResponse(TypedDict): appliedBy: UserSerializerResponse | None appliedDetails: dict[str, Any] | None dateApplied: datetime | None - error: dict[str, Any] | None + error: ActionRequestError | None dateCreated: datetime @@ -201,6 +201,7 @@ def serialize( "appliedBy": attrs["applied_by"], "appliedDetails": obj.applied_details, "dateApplied": obj.date_applied, - "error": obj.error, + # Only the shared apply path writes this column. + "error": cast(ActionRequestError | None, obj.error), "dateCreated": obj.date_added, } diff --git a/src/sentry/investigations/services/action_kinds/base.py b/src/sentry/investigations/services/action_kinds/base.py index b13f86c573dd..8a02b19d5cc9 100644 --- a/src/sentry/investigations/services/action_kinds/base.py +++ b/src/sentry/investigations/services/action_kinds/base.py @@ -33,6 +33,10 @@ class CannotApplyReason(StrEnum): DYNAMIC_DETECTOR = "dynamic_detector" +# One setting an action reads or changes, as JSON. +SettingValue = int | float | str | None + + class ApplyErrorCode(StrEnum): """What else the apply endpoint can say went wrong. The UI switches on these.""" @@ -48,11 +52,19 @@ class ApplyError(TypedDict): # Seer's reason for withdrawing it. reason: NotRequired[str | None] # The settings that moved since Seer suggested the change. - current: NotRequired[dict[str, object]] + current: NotRequired[dict[str, SettingValue]] # Why the change wouldn't save. messages: NotRequired[list[str]] +class ActionRequestError(TypedDict): + """What's stored on a request that went wrong, and shown with it.""" + + code: ApplyErrorCode + current: NotRequired[dict[str, SettingValue]] + messages: NotRequired[list[str]] + + @dataclass(frozen=True) class ApplyResult: action_request: InvestigationActionRequest @@ -73,7 +85,7 @@ class ApplyContext: class ActionStale(Exception): """What the action changes moved since Seer suggested it.""" - def __init__(self, current: dict[str, object]) -> None: + def __init__(self, current: dict[str, SettingValue]) -> None: super().__init__("The action is stale.") self.current = current diff --git a/src/sentry/investigations/services/action_requests.py b/src/sentry/investigations/services/action_requests.py index bf53ed18bb9c..66df9a3693c1 100644 --- a/src/sentry/investigations/services/action_requests.py +++ b/src/sentry/investigations/services/action_requests.py @@ -20,6 +20,7 @@ from sentry.investigations.services.action_kinds import get_action_kind, shared_cannot_apply_reason from sentry.investigations.services.action_kinds.base import ( ActionInvalid, + ActionRequestError, ActionStale, ApplyContext, ApplyError, @@ -322,7 +323,7 @@ def _apply( # it back. action_request.update( state=InvestigationActionRequestState.SUPERSEDED, - error={"code": ApplyErrorCode.STALE, "current": stale.current}, + error=ActionRequestError(code=ApplyErrorCode.STALE, current=stale.current), ) return ApplyResult( action_request, From 52db83c9bb75b80659842f3cb4791bb7ffac80b7 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 23 Oct 2026 19:57:17 -0700 Subject: [PATCH 4/4] fix(investigations): Raise apply errors and type ActionInvalid's messages --- ...tion_investigation_action_request_apply.py | 27 ++-- .../services/action_kinds/base.py | 20 +-- .../services/action_kinds/update_detector.py | 12 +- .../services/action_requests.py | 122 +++++++++--------- ...tion_investigation_action_request_apply.py | 30 ++--- tests/sentry/investigations/test_contracts.py | 4 + 6 files changed, 112 insertions(+), 103 deletions(-) diff --git a/src/sentry/investigations/endpoints/organization_investigation_action_request_apply.py b/src/sentry/investigations/endpoints/organization_investigation_action_request_apply.py index 9127482c2320..effe8723deac 100644 --- a/src/sentry/investigations/endpoints/organization_investigation_action_request_apply.py +++ b/src/sentry/investigations/endpoints/organization_investigation_action_request_apply.py @@ -22,7 +22,11 @@ ) from sentry.investigations.endpoints.validators import ActionRequestApplyValidator from sentry.investigations.models import Investigation -from sentry.investigations.services.action_kinds.base import ApplyErrorCode, CannotApplyReason +from sentry.investigations.services.action_kinds.base import ( + ApplyErrorCode, + ApplyFailed, + CannotApplyReason, +) from sentry.investigations.services.action_requests import apply_action_request from sentry.models.organization import Organization @@ -79,18 +83,19 @@ def post( validator = ActionRequestApplyValidator(data=request.data) if not validator.is_valid(): return Response(validator.errors, status=status.HTTP_400_BAD_REQUEST) - result = apply_action_request( - investigation=investigation, - action_request_id=action_request_id, - request=request, - actor_id=actor_id, - dry_run=validator.validated_data["dry_run"], - ) - if result.error is not None: - return Response(result.error, status=_STATUS.get(result.error["code"], 409)) + try: + action_request = apply_action_request( + investigation=investigation, + action_request_id=action_request_id, + request=request, + actor_id=actor_id, + dry_run=validator.validated_data["dry_run"], + ) + except ApplyFailed as failed: + return Response(failed.error, status=_STATUS.get(failed.error["code"], 409)) body: InvestigationActionRequestApplyResponse = { "actionRequest": serialize( - result.action_request, + action_request, request.user, InvestigationActionRequestSerializer(access=request.access), ), diff --git a/src/sentry/investigations/services/action_kinds/base.py b/src/sentry/investigations/services/action_kinds/base.py index 8a02b19d5cc9..2f2539a45a44 100644 --- a/src/sentry/investigations/services/action_kinds/base.py +++ b/src/sentry/investigations/services/action_kinds/base.py @@ -1,10 +1,10 @@ from __future__ import annotations import abc -from collections.abc import Sequence +from collections.abc import Mapping, Sequence from dataclasses import dataclass from enum import StrEnum -from typing import Any, NotRequired, TypedDict +from typing import NotRequired, TypedDict from django.contrib.auth.models import AnonymousUser from rest_framework.request import Request @@ -65,10 +65,10 @@ class ActionRequestError(TypedDict): messages: NotRequired[list[str]] -@dataclass(frozen=True) -class ApplyResult: - action_request: InvestigationActionRequest - error: ApplyError | None = None +class ApplyFailed(Exception): + def __init__(self, error: ApplyError) -> None: + super().__init__(error["detail"]) + self.error = error @dataclass(frozen=True) @@ -91,11 +91,11 @@ def __init__(self, current: dict[str, SettingValue]) -> None: class ActionInvalid(Exception): - """The change wouldn't save. `errors` are the validator's errors.""" + """The change wouldn't save, and why.""" - def __init__(self, errors: dict[str, Any] | list[Any]) -> None: + def __init__(self, messages: list[str]) -> None: super().__init__("The action is invalid.") - self.errors = errors + self.messages = messages class ActionKind(abc.ABC): @@ -121,7 +121,7 @@ def check(self, context: ApplyContext) -> None: cannot_apply_reasons can leave this as is. """ - def apply(self, context: ApplyContext) -> dict[str, object]: + def apply(self, context: ApplyContext) -> Mapping[str, object]: """Make the change, and return what the activity feed should show about it. Raise ActionStale or ActionInvalid instead of updating the request; the caller records it. """ diff --git a/src/sentry/investigations/services/action_kinds/update_detector.py b/src/sentry/investigations/services/action_kinds/update_detector.py index 555e105e6341..b254427b41b4 100644 --- a/src/sentry/investigations/services/action_kinds/update_detector.py +++ b/src/sentry/investigations/services/action_kinds/update_detector.py @@ -27,7 +27,11 @@ InvestigationActionRequest, InvestigationOrchestrationRun, ) -from sentry.investigations.services.action_kinds.base import ActionKind, CannotApplyReason +from sentry.investigations.services.action_kinds.base import ( + ActionKind, + CannotApplyReason, + SettingValue, +) from sentry.models.organization import Organization from sentry.snuba.models import SnubaQuery from sentry.users.models.user import User @@ -124,8 +128,6 @@ def trim(cls, action: dict[str, Any]) -> dict[str, Any] | None: } -DetectorValue = int | float | str | None - _THRESHOLD_RESULTS: dict[ Literal["highThreshold", "mediumThreshold", "resolutionThreshold"], DetectorPriorityLevel ] = { @@ -211,12 +213,12 @@ def load(cls, detector: Detector, snuba_query: SnubaQuery) -> DetectorState: ) return cls(data_source=data_source, condition_group=condition_group) - def values(self) -> dict[str, DetectorValue]: + def values(self) -> dict[str, SettingValue]: """The monitor's current settings, keyed like Seer's changes. A threshold is left out unless exactly one condition sets it. """ - values: dict[str, DetectorValue] = { + values: dict[str, SettingValue] = { "timeWindowSeconds": self.data_source["timeWindow"], "query": self.data_source["query"], "aggregate": self.data_source["aggregate"], diff --git a/src/sentry/investigations/services/action_requests.py b/src/sentry/investigations/services/action_requests.py index 66df9a3693c1..2c15eefcb20d 100644 --- a/src/sentry/investigations/services/action_requests.py +++ b/src/sentry/investigations/services/action_requests.py @@ -20,12 +20,13 @@ from sentry.investigations.services.action_kinds import get_action_kind, shared_cannot_apply_reason from sentry.investigations.services.action_kinds.base import ( ActionInvalid, + ActionKind, ActionRequestError, ActionStale, ApplyContext, ApplyError, ApplyErrorCode, - ApplyResult, + ApplyFailed, CannotApplyReason, ) @@ -229,11 +230,11 @@ def supersede_block_action_requests(investigation: Investigation, block_keys: Se ) -# The usual detail for a reason; anything a kind refuses gets the last one. _REASON_DETAILS = { CannotApplyReason.INVESTIGATION_ARCHIVED: "Archived investigations are read-only.", CannotApplyReason.UNSUPPORTED_ACTION: "This request has no action to apply.", } +_DEFAULT_REASON_DETAIL = "This action can't be applied." def _not_suggested_error(action_request: InvestigationActionRequest) -> ApplyError: @@ -254,22 +255,39 @@ def _not_suggested_error(action_request: InvestigationActionRequest) -> ApplyErr } -def _error_messages(errors: object) -> list[str]: - if isinstance(errors, dict): - return [message for value in errors.values() for message in _error_messages(value)] - if isinstance(errors, list): - return [message for value in errors for message in _error_messages(value)] - return [str(errors)] +def _check_can_apply( + investigation: Investigation, action_request: InvestigationActionRequest, request: Request +) -> ActionKind: + """Return the request's kind, or raise ApplyFailed if the viewer can't apply it.""" + + reason = shared_cannot_apply_reason(investigation, action_request) + if reason == CannotApplyReason.NOT_SUGGESTED: + raise ApplyFailed(_not_suggested_error(action_request)) + if reason is None: + kind = get_action_kind(action_request.action_kind) + assert kind is not None + # The same check that decides canApply guards the endpoint, so a kind can't forget it. + reason = kind.cannot_apply_reasons( + [action_request], + investigations={investigation.id: investigation}, + access=request.access, + user=request.user, + )[action_request.id] + if reason is None: + return kind + raise ApplyFailed( + {"detail": _REASON_DETAILS.get(reason, _DEFAULT_REASON_DETAIL), "code": reason} + ) def _apply( *, investigation: Investigation, - action_request_id: str, + action_request: InvestigationActionRequest, request: Request, actor_id: int, dry_run: bool, -) -> ApplyResult: +) -> InvestigationActionRequest: investigations = Investigation.objects.select_related("organization") runs = InvestigationOrchestrationRun.objects.all() action_requests = InvestigationActionRequest.objects.all() @@ -279,31 +297,9 @@ def _apply( action_requests = action_requests.select_for_update() locked = investigations.get(id=investigation.id) - try: - run = runs.get(investigation=locked) - except InvestigationOrchestrationRun.DoesNotExist: - raise ResourceDoesNotExist - try: - action_request = action_requests.get(id=action_request_id, investigation=locked) - except (InvestigationActionRequest.DoesNotExist, ValueError): - raise ResourceDoesNotExist - - reason = shared_cannot_apply_reason(locked, action_request) - if reason == CannotApplyReason.NOT_SUGGESTED: - return ApplyResult(action_request, _not_suggested_error(action_request)) - kind = get_action_kind(action_request.action_kind) - if reason is None and kind is not None: - # The same check that decides canApply guards the endpoint, so a kind can't forget it. - reason = kind.cannot_apply_reasons( - [action_request], - investigations={locked.id: locked}, - access=request.access, - user=request.user, - ).get(action_request.id, CannotApplyReason.UNSUPPORTED_ACTION) - if reason is not None: - detail = _REASON_DETAILS.get(reason, "This action can't be applied.") - return ApplyResult(action_request, {"detail": detail, "code": reason}) - assert kind is not None + run = runs.get(investigation=locked) + action_request = action_requests.get(id=action_request.id) + kind = _check_can_apply(locked, action_request, request) context = ApplyContext( investigation=locked, @@ -315,7 +311,7 @@ def _apply( try: if dry_run: kind.check(context) - return ApplyResult(action_request) + return action_request details = kind.apply(context) except ActionStale as stale: if not dry_run: @@ -325,23 +321,21 @@ def _apply( state=InvestigationActionRequestState.SUPERSEDED, error=ActionRequestError(code=ApplyErrorCode.STALE, current=stale.current), ) - return ApplyResult( - action_request, + raise ApplyFailed( { "detail": "This changed since Seer suggested it.", "code": ApplyErrorCode.STALE, "current": stale.current, - }, + } ) except ActionInvalid as invalid: # The request stays suggested, so the user can try again. - return ApplyResult( - action_request, + raise ApplyFailed( { "detail": "The change wasn't valid.", "code": ApplyErrorCode.VALIDATION_FAILED, - "messages": _error_messages(invalid.errors), - }, + "messages": invalid.messages, + } ) action_request.update( state=InvestigationActionRequestState.APPLIED, @@ -350,7 +344,7 @@ def _apply( applied_details=details, error=None, ) - return ApplyResult(action_request) + return action_request def apply_action_request( @@ -360,21 +354,33 @@ def apply_action_request( request: Request, actor_id: int, dry_run: bool = False, -) -> ApplyResult: - """Apply a suggested action as the requesting user. A dry run only says whether it would - work, and doesn't take locks or change anything. +) -> InvestigationActionRequest: + """Apply a suggested action as the requesting user, or raise ApplyFailed. A dry run only + says whether it would work, and doesn't take locks or change anything. """ + try: + action_request = InvestigationActionRequest.objects.get( + id=action_request_id, investigation=investigation + ) + except (InvestigationActionRequest.DoesNotExist, ValueError): + raise ResourceDoesNotExist + using = router.db_for_write(InvestigationActionRequest) with transaction.atomic(using=using): - result = _apply( - investigation=investigation, - action_request_id=action_request_id, - request=request, - actor_id=actor_id, - dry_run=dry_run, - ) - if dry_run: - # Even if a check writes by mistake, a dry run keeps nothing. - transaction.set_rollback(True, using=using) - return result + try: + return _apply( + investigation=investigation, + action_request=action_request, + request=request, + actor_id=actor_id, + dry_run=dry_run, + ) + except ApplyFailed as failed: + error = failed + finally: + if dry_run: + # The detector validator can create environments while validating. + transaction.set_rollback(True, using=using) + # Raise once the transaction commits, so a stale request stays superseded. + raise error diff --git a/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py b/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py index 09d3e59c4bc3..8344cd8e3e0b 100644 --- a/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py +++ b/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py @@ -207,24 +207,6 @@ def test_a_dry_run_runs_the_same_checks(self) -> None: assert response.status_code == 409 assert response.data["code"] == "not_suggested" - def test_a_dry_run_changes_nothing(self) -> None: - action_request = self.create_investigation_action_request( - investigation=self.investigation, - key="task", - action_kind="seer_task", - action_payload={"kind": "seer_task", "prompt": "Go"}, - ) - version = self.investigation.version - - response = self.apply(action_request, dryRun=True) - - assert response.status_code == 400 - assert response.data["code"] == "unsupported_action" - action_request.refresh_from_db() - self.investigation.refresh_from_db() - assert action_request.state == InvestigationActionRequestState.SUGGESTED - assert self.investigation.version == version - def test_rejects_an_unknown_field(self) -> None: response = self.apply(self.suggest(), dryRn=True) @@ -350,6 +332,16 @@ def test_a_dry_run_only_checks(self) -> None: self.action_request.refresh_from_db() assert self.action_request.state == InvestigationActionRequestState.SUGGESTED + def test_a_dry_run_keeps_nothing_a_check_writes(self) -> None: + title = self.action_request.title + self.kind.check_title = "Changed by the check" + + response = self.post(dryRun=True) + + assert response.status_code == 200, response.data + self.action_request.refresh_from_db() + assert self.action_request.title == title + def test_applying_records_what_the_kind_returns(self) -> None: response = self.post() @@ -386,7 +378,7 @@ def test_a_stale_dry_run_changes_nothing(self) -> None: assert self.action_request.error is None def test_an_invalid_action_stays_suggested(self) -> None: - self.kind.raises = ActionInvalid({"window": ["Pick a supported window."]}) + self.kind.raises = ActionInvalid(["Pick a supported window."]) response = self.post() diff --git a/tests/sentry/investigations/test_contracts.py b/tests/sentry/investigations/test_contracts.py index 6742bbaefc92..19e7df0ca0ea 100644 --- a/tests/sentry/investigations/test_contracts.py +++ b/tests/sentry/investigations/test_contracts.py @@ -662,6 +662,8 @@ def __init__(self) -> None: self.calls: list[str] = [] # What check and apply raise, if anything. self.raises: Exception | None = None + # A title for check to write on the request. + self.check_title: str | None = None def after_upsert(self, action_request: InvestigationActionRequest) -> None: self.upserted.append(action_request) @@ -681,6 +683,8 @@ def cannot_apply_reasons( def check(self, context: ApplyContext) -> None: self.calls.append("check") + if self.check_title is not None: + context.action_request.update(title=self.check_title) if self.raises is not None: raise self.raises