feat(autofix): Add pr_iteration endpoint step and operator activity#117218
Conversation
680b16b to
5b2ee97
Compare
8f21984 to
2dd9635
Compare
5b2ee97 to
ad7c62e
Compare
2dd9635 to
c4da8d7
Compare
ad7c62e to
6daae98
Compare
905c091 to
c4da8d7
Compare
ad7c62e to
91aba65
Compare
c4da8d7 to
39bd905
Compare
39bd905 to
84f4687
Compare
d11ddf4 to
8cbe651
Compare
5842a6e to
9687a5b
Compare
8cbe651 to
1a608b0
Compare
11bbfa9 to
03037bc
Compare
2fce8d3 to
e8eebdf
Compare
03037bc to
c26e4e4
Compare
e8eebdf to
5bc7bf9
Compare
ae176eb to
a824c10
Compare
Zylphrex
left a comment
There was a problem hiding this comment.
A few things but overall LGTM
Does this need to be added to the on completion hook as well so it responds in slack and we fire a webhook? Can be in a follow up, just wondering what the plan is.
| message=user_context, | ||
| source={ | ||
| "type": "user-ui", | ||
| "user_id": request.user.id, |
There was a problem hiding this comment.
Just a thought but why not just store the user here and avoid having to hydrate the user later?
This is manual process so my expectations are that this wont take up that much storage anyways
| pull_requests = event_payload.get("pull_requests", []) | ||
| if pull_requests: | ||
| activity_data["pull_requests"] = pull_requests | ||
| elif event_type == SentryAppEventType.SEER_ITERATION_COMPLETED: |
There was a problem hiding this comment.
Can you explain what this activity does?
There was a problem hiding this comment.
mostly here for parity with the other steps having activities, but i don't think this ends up doing anything right now
there is code that seems like it'll handle activities in the future, that's behind a feature flag and there's currently no handlers implemented for these activities
there's also an activity log in the UI where we show these, that's behind a feature flag
a824c10 to
8b754c1
Compare
| if step == AutofixStep.PR_ITERATION and run_state is not None: | ||
| if step == AutofixStep.PR_ITERATION: | ||
| if run_state is None or not run_state.repo_pr_states: | ||
| raise PrIterationNoPullRequestException() |
There was a problem hiding this comment.
PR iteration allows non-created PRs
Medium Severity
pr_iteration is only blocked when repo_pr_states is empty, not when every entry lacks a created PR. Runs with only creating or error states still pass validation and return 202, which conflicts with the documented requirement for at least one created pull request.
Reviewed by Cursor Bugbot for commit 8b754c1. Configure here.
Send the ITERATION_COMPLETED webhook when a PR_ITERATION step finishes, including the pull request payload, code changes, and iteration index, and record the iteration index on the introspection analytics event. Add introspect_iteration to evaluate whether revised iteration changes are ready to update the existing pull request, and route PR_ITERATION through it. Extract the pull-request and code-changes payload builders into shared helpers. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Accept the pr_iteration step in the explorer autofix endpoint, gated behind the autofix-pr-iteration feature flag. The step requires an existing run with at least one created pull request, otherwise it returns a 400. Map the SEER_ITERATION_COMPLETED webhook to a SEER_ITERATION_COMPLETED group activity in the operator, carrying the pull requests and iteration index. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
_hydrate_feedback_users assumed every block message carried a metadata dict, but messages can have metadata set to null. Calling .get() on the result crashed when hydrating feedback users. Coalesce metadata to an empty dict before reading the feedback field. Co-Authored-By: Claude <noreply@anthropic.com>
Patch get_autofix_run_state (not get_autofix_agent_state) in pr_iteration tests, and add the feedback=None kwarg to trigger_autofix_agent call assertions.
8b754c1 to
09b1bb2
Compare
| iteration_index: int | None = None | ||
| if step == AutofixStep.PR_ITERATION and run_state is not None: | ||
| if step == AutofixStep.PR_ITERATION: |
There was a problem hiding this comment.
Bug: The get_iteration_for_insert_index function lacks bounds checking for insert_index and safe access for metadata["iteration_index"], risking unhandled exceptions.
Severity: HIGH
Suggested Fix
Add validation to ensure insert_index is within the bounds of the state.blocks list before access. When retrieving iteration_index from a block's metadata, use the .get() method with a default value or wrap the access in a try...except KeyError block to prevent unhandled exceptions.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/sentry/seer/autofix/autofix_agent.py#L388-L389
Potential issue: In the `pr_iteration` flow, the function
`get_iteration_for_insert_index` uses a user-provided `insert_index` to access
`state.blocks[insert_index]` without performing a bounds check. It also accesses
`metadata["iteration_index"]` without a safe fallback like `.get()`. An out-of-bounds
`insert_index` will raise an `IndexError`, and a missing `iteration_index` key will
raise a `KeyError`. Neither of these exceptions are caught by the endpoint's existing
exception handlers, which will cause an unhandled 500 server error on the public API.
Did we get this right? 👍 / 👎 to inform future reviews.
| user_id: int | ||
| # The serialized user, resolved at write time so the read path doesn't have | ||
| # to hydrate it. ``None`` if the user could not be serialized. | ||
| user: NotRequired[Any] |
There was a problem hiding this comment.
Self-serialized user (all emails, options, flags) stored in Seer feedback metadata and returned to other org members
When submitting pr_iteration feedback, the endpoint serializes the requesting user with as_user=serialize_generic_user(request.user). Because as_user equals the target user, UserSerializer._user_is_requester is True and the result is a UserSerializerResponseSelf containing the user's full registered email list, user options, and internal flags. This payload is stored in feedback.source["user"], embedded into the Seer message's prompt_metadata["feedback"], and round-tripped back through block.dict() in the GET endpoint, where any authenticated org member with group-read access can read another member's personal emails, options, and flags. Only a minimal attribution subset (id, name, avatarUrl) is needed.
Evidence
group_ai_autofix.py:329-331callsuser_service.serialize_many(filter={"user_ids":[request.user.id]}, as_user=serialize_generic_user(request.user)); sinceas_useris the same user,UserSerializer._user_is_requesterreturns True.users/api/serializers/user.py:193-203then populatesemails(allUserEmailrows),options, andflags, producingUserSerializerResponseSelfrather than the public response.- The result is stored as
feedback.source["user"](Feedbackinautofix_agent.py) and embedded viaprompt_metadata["feedback"] = json.dumps({... "source": feedback.source ...})atautofix_agent.py:418. Message.metadataisdict[str, str] | None(agent/client_models.py:33) and is part ofMemoryBlock; the GET handler returnsblocks = [block.dict() for block in state.blocks]atgroup_ai_autofix.py:442, surfacingmessage.metadata.feedbackto any org member with group-read access.
Also found at 2 additional locations
src/sentry/seer/endpoints/group_ai_autofix.py:68-69src/sentry/seer/endpoints/group_ai_autofix.py:326-342
Identified by Warden wrdn-data-exfil, wrdn-pii · GE3-KTX
- serialize with users with as_user = None to avoid storing too much information in run state feedback - correct logic and update check of pr_iteration_enabled in group_ai_autofix to check metadata in run state, when run state is available: the desired behaviour is to respect the metadata.pr_iteration_enabled field, over the value of the feature flag when the endpoint is called
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d36c8a5. Configure here.
| return Response( | ||
| {"detail": "run_id is required for pr_iteration"}, | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) |
There was a problem hiding this comment.
Missing endpoint-level feature flag check for pr_iteration
Medium Severity
The endpoint checks that resolved_run_id is not None for pr_iteration, but never checks features.has("organizations:autofix-pr-iteration", ...) before calling trigger_autofix_agent. The test test_pr_iteration_requires_feature_flag asserts mock_trigger_explorer.assert_not_called() and expects a 403, but since trigger_autofix_agent is mocked (and won't raise PrIterationNotEnabledException), the endpoint proceeds normally and returns 202. An endpoint-level feature flag gate is needed here to match the documented intended behavior.
Reviewed by Cursor Bugbot for commit d36c8a5. Configure here.
| and user_context is not None | ||
| and request.user | ||
| and request.user.is_authenticated | ||
| ): | ||
| # Serialize the user here on write so the read path (GET) doesn't have |
There was a problem hiding this comment.
Bug: The check user_context is not None allows empty strings, creating unnecessary Feedback objects and triggering RPC calls even when no meaningful context is provided.
Severity: LOW
Suggested Fix
Change the condition at line 317 from if user_context is not None to if user_context. This truthy check will correctly filter out empty strings and prevent the creation of meaningless feedback objects.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/sentry/seer/endpoints/group_ai_autofix.py#L317-L321
Potential issue: When a client sends an empty string for `user_context`, the condition
`user_context is not None` evaluates to true. This results in the creation and storage
of a `Feedback` object with an empty message. This behavior leads to several
inefficiencies: an unnecessary RPC call to `user_service.serialize_many()`, storage of
meaningless feedback data in the run's metadata, and a misleading `has_user_context`
metric being set to `"yes"`. The intended behavior, as seen in `build_step_prompt`, is
likely to only process feedback when `user_context` is a non-empty string.


Accept the pr_iteration step in the explorer autofix endpoint, gated behind
the autofix-pr-iteration feature flag. The step requires an existing run with
at least one created pull request, otherwise it returns a 400. Map the
SEER_ITERATION_COMPLETED webhook to a SEER_ITERATION_COMPLETED group activity
in the operator, carrying the pull requests and iteration index.
Also, transform user IDs stored in run state feedback into serialized users for frontend