Skip to content
3 changes: 0 additions & 3 deletions src/sentry/seer/autofix/autofix_agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
from enum import StrEnum
from typing import TYPE_CHECKING, Literal

from django.utils import timezone
from pydantic import BaseModel

from sentry.seer.autofix.artifact_schemas import (
Expand Down Expand Up @@ -247,8 +246,6 @@ def trigger_autofix_explorer(
artifact_schema=artifact_schema,
)

group.update(seer_explorer_autofix_last_triggered=timezone.now())

payload = {
"run_id": run_id,
"group_id": group.id,
Expand Down
69 changes: 39 additions & 30 deletions src/sentry/seer/autofix/on_completion_hook.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import logging
from typing import TYPE_CHECKING

from django.utils import timezone

from sentry import features
from sentry.models.group import Group
from sentry.models.organization import Organization
Expand Down Expand Up @@ -76,13 +78,25 @@
)
return

group_id = state.metadata.get("group_id") if state.metadata else None
if group_id is None:
group = None
else:
try:
group = Group.objects.get(id=group_id)
except Group.DoesNotExist:
group = None

# Send webhook for the completed step
cls._send_step_webhook(organization, run_id, state)
cls._send_step_webhook(organization, run_id, state, group)

cls._maybe_trigger_supergroups_embedding(organization, run_id, state)
cls._maybe_trigger_supergroups_embedding(organization, run_id, state, group)

# Continue the automated pipeline if stopping_point hasn't been reached
cls._maybe_continue_pipeline(organization, run_id, state)
cls._maybe_continue_pipeline(organization, run_id, state, group)

if group is not None:
group.update(seer_explorer_autofix_last_triggered=timezone.now())

Check warning on line 99 in src/sentry/seer/autofix/on_completion_hook.py

View check run for this annotation

@sentry/warden / warden: sentry-security

Group lookup not scoped by organization - potential cross-org IDOR

The code fetches a Group by ID from `state.metadata.get('group_id')` without validating that the group belongs to the provided organization. While the Seer API is trusted, this is inconsistent with the existing pattern in `trigger_push_changes()` (lines 503-505 of autofix_agent.py) which explicitly validates `group_id == group.id`. If Seer ever returns incorrect metadata (bug or manipulation), this could allow updating `seer_explorer_autofix_last_triggered` on a group in another organization, and potentially trigger webhooks/pipelines for the wrong group.
Comment thread
cursor[bot] marked this conversation as resolved.
Outdated
Comment thread
sentry-warden[bot] marked this conversation as resolved.
Outdated

@classmethod
def find_latest_artifact_for_step(cls, state: SeerRunState, key: str) -> Artifact | None:
Expand All @@ -95,19 +109,27 @@
return None

@classmethod
def _send_step_webhook(cls, organization, run_id, state: SeerRunState):
def _send_step_webhook(
cls,
organization: Organization,
run_id: int,
state: SeerRunState,
group: Group | None,
):
"""
Send webhook for the completed step.

Determines which step just completed and sends the appropriate webhook event.
"""
current_step = cls._get_current_step(state)
if group is None:
return

webhook_payload = {"run_id": run_id}
current_step = cls._get_current_step(state)

group_id = state.metadata.get("group_id") if state.metadata else None
if group_id is not None:
webhook_payload["group_id"] = group_id
webhook_payload = {
"run_id": run_id,
"group_id": group.id,
}

# Iterate through blocks in reverse order (most recent first)
# to find which step just completed
Expand Down Expand Up @@ -213,19 +235,15 @@
organization: Organization,
run_id: int,
state: SeerRunState,
group: Group | None,
) -> None:
"""Trigger supergroups embedding if feature flag is enabled."""
current_step = cls._get_current_step(state)
if current_step != AutofixStep.ROOT_CAUSE:
return

group_id = state.metadata.get("group_id") if state.metadata else None
if group_id is None:
return

try:
group = Group.objects.get(id=group_id)
except Group.DoesNotExist:
if group is None:
group_id = state.metadata.get("group_id") if state.metadata else None
logger.warning(
"autofix.supergroup_embedding.group_not_found",
extra={"group_id": group_id},
Expand All @@ -242,7 +260,7 @@
try:
trigger_supergroups_embedding(
organization_id=organization.id,
group_id=group_id,
group_id=group.id,
project_id=group.project_id,
artifact_data=root_cause_artifact.data,
)
Expand All @@ -252,7 +270,7 @@
extra={
"run_id": run_id,
"organization_id": organization.id,
"group_id": group_id,
"group_id": group.id,
},
)

Expand Down Expand Up @@ -289,6 +307,7 @@
organization: Organization,
run_id: int,
state: SeerRunState,
group: Group | None,
) -> None:
"""
Continue to the next step if stopping_point hasn't been reached.
Expand All @@ -307,19 +326,9 @@
return

stopping_point = AutofixStoppingPoint(metadata["stopping_point"])
group_id = metadata.get("group_id")

if not group_id:
logger.warning(
"autofix.on_completion_hook.no_group_id_in_metadata",
extra={"run_id": run_id, "organization_id": organization.id},
)
return

# Get the group
try:
group = Group.objects.get(id=group_id, project__organization=organization)
except Group.DoesNotExist:
if group is None:
group_id = state.metadata.get("group_id") if state.metadata else None
logger.warning(
"autofix.on_completion_hook.group_not_found",
extra={
Expand Down
24 changes: 0 additions & 24 deletions tests/sentry/seer/autofix/test_autofix_agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -354,30 +354,6 @@ def test_trigger_autofix_explorer_passes_group_id_in_metadata(
call_kwargs = mock_client.start_run.call_args.kwargs
assert call_kwargs["metadata"] == {"group_id": self.group.id, "referrer": "unknown"}

@patch("sentry.seer.autofix.autofix_agent.broadcast_webhooks_for_organization.delay")
@patch("sentry.seer.autofix.autofix_agent.SeerExplorerClient")
def test_trigger_autofix_explorer_updates_explorer_last_triggered(
self, mock_client_class, mock_broadcast
):
"""trigger_autofix_explorer sets seer_explorer_autofix_last_triggered on the group."""
mock_client = MagicMock()
mock_client_class.return_value = mock_client
mock_client.start_run.return_value = 123

assert self.group.seer_explorer_autofix_last_triggered is None

trigger_autofix_explorer(
group=self.group,
step=AutofixStep.ROOT_CAUSE,
referrer=AutofixReferrer.UNKNOWN,
run_id=None,
)

self.group.refresh_from_db()

assert self.group.seer_autofix_last_triggered is None
assert self.group.seer_explorer_autofix_last_triggered is not None


class TestTriggerCodingAgentHandoff(TestCase):
"""Tests for trigger_coding_agent_handoff function."""
Expand Down
36 changes: 22 additions & 14 deletions tests/sentry/seer/autofix/test_autofix_on_completion_hook.py
Original file line number Diff line number Diff line change
Expand Up @@ -199,14 +199,14 @@ def setUp(self):
def test_maybe_continue_pipeline_no_metadata(self, mock_trigger):
"""Does not continue when metadata is missing."""
state = run_state(blocks=[root_cause_memory_block()])
AutofixOnCompletionHook._maybe_continue_pipeline(self.organization, 123, state)
AutofixOnCompletionHook._maybe_continue_pipeline(self.organization, 123, state, self.group)
mock_trigger.assert_not_called()

@patch("sentry.seer.autofix.on_completion_hook.trigger_autofix_explorer")
def test_maybe_continue_pipeline_no_stopping_point_in_metadata(self, mock_trigger):
"""Does not continue when stopping_point is missing from metadata."""
state = run_state(blocks=[root_cause_memory_block()], metadata={"group_id": self.group.id})
AutofixOnCompletionHook._maybe_continue_pipeline(self.organization, 123, state)
AutofixOnCompletionHook._maybe_continue_pipeline(self.organization, 123, state, self.group)
mock_trigger.assert_not_called()

@patch("sentry.seer.autofix.on_completion_hook.trigger_autofix_explorer")
Expand All @@ -219,7 +219,7 @@ def test_maybe_continue_pipeline_at_stopping_point(self, mock_trigger):
"stopping_point": AutofixStoppingPoint.ROOT_CAUSE.value,
},
)
AutofixOnCompletionHook._maybe_continue_pipeline(self.organization, 123, state)
AutofixOnCompletionHook._maybe_continue_pipeline(self.organization, 123, state, self.group)
mock_trigger.assert_not_called()

@patch("sentry.seer.autofix.on_completion_hook.get_project_seer_preferences")
Expand All @@ -236,7 +236,7 @@ def test_maybe_continue_pipeline_continues_to_next_step(self, mock_trigger, mock
"stopping_point": AutofixStoppingPoint.CODE_CHANGES.value,
},
)
AutofixOnCompletionHook._maybe_continue_pipeline(self.organization, 123, state)
AutofixOnCompletionHook._maybe_continue_pipeline(self.organization, 123, state, self.group)
mock_trigger.assert_called_once()
call_kwargs = mock_trigger.call_args.kwargs
assert call_kwargs["group"].id == self.group.id
Expand All @@ -257,7 +257,7 @@ def test_maybe_continue_pipeline_pushes_changes_for_open_pr(self, mock_push_chan
"stopping_point": AutofixStoppingPoint.OPEN_PR.value,
},
)
AutofixOnCompletionHook._maybe_continue_pipeline(self.organization, 123, state)
AutofixOnCompletionHook._maybe_continue_pipeline(self.organization, 123, state, self.group)
mock_push_changes.assert_called_once_with(self.group, 123, state=state)


Expand All @@ -284,6 +284,8 @@ class TestAutofixOnCompletionHookWebhooks(TestCase):
def setUp(self):
super().setUp()
self.organization = self.create_organization()
self.project = self.create_project(organization=self.organization)
self.group = self.create_group(project=self.project)

@patch("sentry.seer.autofix.on_completion_hook.broadcast_webhooks_for_organization.delay")
def test_send_step_webhook_artifact_types(self, mock_broadcast):
Expand Down Expand Up @@ -322,7 +324,7 @@ class TestCaseDict(TypedDict):
for i, test_case in enumerate(test_cases):
mock_broadcast.reset_mock()
state = run_state(blocks=[test_case["block"]])
AutofixOnCompletionHook._send_step_webhook(self.organization, run_id, state)
AutofixOnCompletionHook._send_step_webhook(self.organization, run_id, state, self.group)

mock_broadcast.assert_called_once()
call_kwargs = mock_broadcast.call_args.kwargs
Expand All @@ -346,7 +348,7 @@ def test_send_step_webhook_coding(self, mock_broadcast):
code_changes_memory_block(),
]
)
AutofixOnCompletionHook._send_step_webhook(self.organization, 123, state)
AutofixOnCompletionHook._send_step_webhook(self.organization, 123, state, self.group)

mock_broadcast.assert_called_once()
call_kwargs = mock_broadcast.call_args.kwargs
Expand All @@ -365,7 +367,7 @@ def test_send_step_webhook_no_artifacts_no_webhook(self, mock_broadcast):
artifacts=[],
)
state = run_state(blocks=[block])
AutofixOnCompletionHook._send_step_webhook(self.organization, 123, state)
AutofixOnCompletionHook._send_step_webhook(self.organization, 123, state, self.group)

mock_broadcast.assert_not_called()

Expand All @@ -385,7 +387,9 @@ def test_triggers_embedding_on_root_cause(self, mock_trigger_sg):
"""Triggers supergroups embedding when root cause completes with feature flag enabled."""
block = root_cause_memory_block()
state = run_state(blocks=[block], metadata={"group_id": self.group.id})
AutofixOnCompletionHook._maybe_trigger_supergroups_embedding(self.organization, 123, state)
AutofixOnCompletionHook._maybe_trigger_supergroups_embedding(
self.organization, 123, state, self.group
)

mock_trigger_sg.assert_called_once_with(
organization_id=self.organization.id,
Expand All @@ -401,15 +405,19 @@ def test_skips_embedding_when_flag_disabled(self, mock_trigger_sg):
blocks=[root_cause_memory_block()],
metadata={"group_id": self.group.id},
)
AutofixOnCompletionHook._maybe_trigger_supergroups_embedding(self.organization, 123, state)
AutofixOnCompletionHook._maybe_trigger_supergroups_embedding(
self.organization, 123, state, self.group
)

mock_trigger_sg.assert_not_called()

@patch("sentry.seer.autofix.on_completion_hook.trigger_supergroups_embedding")
def test_skips_embedding_when_no_group_id(self, mock_trigger_sg):
"""Does not trigger supergroups embedding when group_id is missing from metadata."""
def test_skips_embedding_when_no_group(self, mock_trigger_sg):
"""Does not trigger supergroups embedding when group is None."""
state = run_state(blocks=[root_cause_memory_block()])
AutofixOnCompletionHook._maybe_trigger_supergroups_embedding(self.organization, 123, state)
AutofixOnCompletionHook._maybe_trigger_supergroups_embedding(
self.organization, 123, state, None
)
Comment thread
cursor[bot] marked this conversation as resolved.
Outdated

mock_trigger_sg.assert_not_called()

Expand Down Expand Up @@ -553,7 +561,7 @@ def test_maybe_continue_pipeline_triggers_handoff_when_configured(
},
)

AutofixOnCompletionHook._maybe_continue_pipeline(self.organization, 123, state)
AutofixOnCompletionHook._maybe_continue_pipeline(self.organization, 123, state, self.group)

mock_trigger_handoff.assert_called_once()

Expand Down
Loading