-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
[DONT MERGE] PoC local code review #106360
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Add Django settings for CLI bug prediction: - CLI_BUG_PREDICTION_ENABLED: Enable/disable feature - CLI_BUG_PREDICTION_TIMEOUT: 10-minute max processing time - CLI_BUG_PREDICTION_POLL_INTERVAL: 2-second Seer polling interval - CLI_BUG_PREDICTION_USER_RATE_LIMIT: 10 requests per hour per user - CLI_BUG_PREDICTION_ORG_RATE_LIMIT: 100 requests per hour per org Add organizations:cli-bug-prediction feature flag for gradual rollout
Add CliBugPredictionRequestSerializer for validating CLI review requests: - Validates repository info (owner, name, provider, base_commit_sha) - Validates diff size (max 500KB) - Validates file count (max 50 files) - Validates commit SHA format (40-char hex string) Add RepositoryInfoSerializer for repository metadata validation
Add trigger_cli_bug_prediction() to initiate analysis: - POSTs diff and repository metadata to Seer - Returns run_id for tracking Add get_cli_bug_prediction_status() to poll for results: - Checks status of analysis by run_id - Returns status, predictions, and diagnostics when complete Both methods handle timeouts, retries, and error responses
Add comprehensive tests for trigger_cli_bug_prediction(): - Success case with and without commit message - Timeout and max retry error handling - Error status codes (500, etc.) - Invalid JSON response handling - Missing run_id in response Add comprehensive tests for get_cli_bug_prediction_status(): - Pending status - Completed status with predictions and diagnostics - Timeout and max retry error handling - Error status codes (404, etc.) - Invalid JSON response handling - Missing status field in response
Add OrganizationCliBugPredictionEndpoint with full implementation: - Feature flag check (organizations:cli-bug-prediction) - Rate limiting (10/hour per user, 100/hour per org) - Request validation using CliBugPredictionRequestSerializer - Repository resolution with proper scoping - Seer integration (trigger + polling) - Comprehensive error handling and mapping - Logging and metrics throughout The endpoint synchronously polls Seer for up to 10 minutes and returns predictions or appropriate error responses with user-friendly messages
Add URL route for CLI bug prediction endpoint:
- Path: /api/0/organizations/{organization_slug}/bug-prediction/cli-review/
- Maps to OrganizationCliBugPredictionEndpoint
- Named route: sentry-api-0-organization-cli-bug-prediction
Add unit tests covering all endpoint functionality: - Happy path with successful prediction - Feature flag disabled - Request validation (diff size, file count, empty diff) - Commit SHA validation (format, length) - Repository not found - Seer trigger timeout and errors - Seer polling timeout - Seer error mapping (base commit, diff size, file count, clone failed, unknown) - Rate limiting (user and organization) - Optional fields handling All tests use mocks to avoid external dependencies
Add integration tests for end-to-end flow: - Single poll cycle (immediate completion) - Multiple polling cycles with state transitions - Network error recovery during polling - Empty predictions response - Multiple predictions in single response - State transitions (pending -> in_progress -> completed) - Diagnostics data included in response These tests verify the full request-response cycle including the internal polling mechanism with mocked Seer responses
* Renaming things from `cli-bug-prediction*` to `code-review-local*` * The killswtch for the feature was created but was not being used. Added to the endpint. * Resolving some mypy complaints ! Still no local tests end-to-end. There are probably bugs lurking
|
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
| name="sentry-api-0-organization-autofix-automation-settings", | ||
| ), | ||
| re_path( | ||
| r"^(?P<organization_id_or_slug>[^/]+)/code-review/local-review/$", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be /internal endpoint?
| publish_status = { | ||
| "POST": ApiPublishStatus.PRIVATE, | ||
| } | ||
| permission_classes = (OrganizationIntegrationsPermission,) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using this permission requires the org:integrations scope. For Sentry PATs the only level of permission that includes this seems to be the admin level (org:admin)
So if we keep with this permission level only org admins can use the endpoint. Might be OK for the PoC, but certainly not in general
Maybe TODO: reduce level to org:write ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a new permission level for this sort of thing in the future?
|
@sentry review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can place this under src/sentry/seer/code_review/
| Returns 200 with predictions on success, various error codes on failure. | ||
| """ | ||
| # Check if feature is globally enabled | ||
| if not settings.CODE_REVIEW_LOCAL_ENABLED: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Killswitches are officially supported via this:
| def killswitches(): |
One day we will have support for it via options-automator.
c548f3d to
70f0142
Compare
|
TODO: Adjust the serializers to match what the CLI will send us (see getsentry/sentry-cli#3084) |
Renames the "code-review-local" things from "cli-bug-prediction" or "cli-pr-review"
e6540c6 to
59d1dd5
Compare
| # Include the error message from Seer if available | ||
| error_msg = str(e) | ||
| if "Seer error" in error_msg: | ||
| return Response({"detail": error_msg}, status=502) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Stack trace information
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 6 hours ago
In general, to fix information exposure via exceptions, avoid returning raw exception messages to end users. Log the full error server-side (which the code already does with logger.exception) and replace user-facing responses with generic, non-sensitive messages. If you need to surface specific, safe information, use a controlled mapping (e.g., error codes to friendly messages) rather than passing through arbitrary exception text.
Here, the minimal, non-functional-breaking change is to stop returning error_msg directly in the response, while still allowing clients to distinguish the “Seer error” case via status code or a generic message. We can keep the existing logging (including error: str(e)) so developers retain full detail, but change the response body to a generic description such as "Failed to start code review analysis" or a slightly more specific but still generic "Code review service returned an error". Concretely, in src/sentry/seer/code_review/endpoints/code_review_local.py, in the except ValueError as e: block, replace:
174: error_msg = str(e)
175: if "Seer error" in error_msg:
176: return Response({"detail": error_msg}, status=502)
177: return Response({"detail": "Failed to start code review analysis"}, status=502)with logic that does not expose error_msg to the client. No new imports or helpers are required.
-
Copy modified line R173 -
Copy modified lines R176-R179
| @@ -170,10 +170,13 @@ | ||
| "error": str(e), | ||
| }, | ||
| ) | ||
| # Include the error message from Seer if available | ||
| # Return a generic error message to avoid exposing internal details | ||
| error_msg = str(e) | ||
| if "Seer error" in error_msg: | ||
| return Response({"detail": error_msg}, status=502) | ||
| return Response( | ||
| {"detail": "Code review service returned an error"}, | ||
| status=502, | ||
| ) | ||
| return Response({"detail": "Failed to start code review analysis"}, status=502) | ||
| except Exception as e: | ||
| # Catch-all for unexpected errors |
PR Description: Local Code Review API Endpoint
more info: https://linear.app/getsentry/project/expose-pr-review-via-cli-27948ef421c5/issues
Summary
Adds a new Sentry API endpoint that enables AI-code-review on local changes via
sentry-cli(before creating a pull request).This bridges sentry-cli and Seer's ML service to provide early feedback in the development cycle.
What it does
POST /api/0/organizations/{org}/code-review/local-review/Architecture