Skip to content

feat(cursor-origin): Record commits from a push - #124999

Open
wedamija wants to merge 1 commit into
danf/origin-handler-contextfrom
danf/origin-push-webhook
Open

wedamija wants to merge 1 commit into
danf/origin-handler-contextfrom
danf/origin-push-webhook

Conversation

@wedamija

Copy link
Copy Markdown
Member

Handle repository.pushed in the cells, where repository commits are stored. Origin's
payload includes the old and new ref tips but no commit list, so commits are read through
the API. Only branch updates are recorded. Deletions and tags are skipped. A new branch
records only its tip since its existing history is not part of the push.

Handle `repository.pushed` in the cells, where repository commits are stored. Origin's
payload includes the old and new ref tips but no commit list, so commits are read through
the API. Only branch updates are recorded. Deletions and tags are skipped. A new branch
records only its tip since its existing history is not part of the push.
@wedamija
wedamija requested a review from a team as a code owner September 19, 2026 00:42
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 19, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit a888b88. Configure here.

return []

if ref_update.created:
return [head_commit] if head_commit else []

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New branch tip dropped without headCommit

Medium Severity

A newly created branch records nothing when headCommit is missing, even though after is the tip SHA. Regular one-commit updates already fall back to the API in that case, so the create path leaves the tip unrecorded. Later pushes compare against that unrecorded tip and never pick it up.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit a888b88. Configure here.

Comment on lines +88 to +89
installation = integration.get_installation(organization_id=repo.organization_id)
client = installation.get_client()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The _refresh_access_token function, called from a cell silo context, directly queries the control-silo-only Integration model, which will fail in environments without control-silo proxying configured.
Severity: HIGH

Suggested Fix

Ensure that any code path originating from a cell silo that needs to access the Integration model is explicitly proxied to the control silo. The @control_silo_function decorator does not provide this protection in production, so the logic must rely on the proxy client mechanism or another method to route the call correctly.

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/integrations/cursor_origin/push.py#L88-L89

Potential issue: The `repository.pushed` webhook handler now runs in cell silos. Its API
client, `CursorOriginApiClient`, may attempt to refresh its access token by calling
`_refresh_access_token`. This function queries the `Integration` model, which only
exists in the control silo database. The `@control_silo_function` decorator on this
function is a no-op in production. If the environment is not configured to proxy
requests to the control silo, this code path will execute in the cell silo, causing a
database error when it tries to access the `Integration` model.

Did we get this right? 👍 / 👎 to inform future reviews.

Comment on lines +115 to +116
name = repo.config["name"]
comparison = client.compare_commits(name, ref_update.before, after)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The compare_commits API call may receive an empty string for the before parameter on existing branches, which could cause an unhandled API error if the external API rejects it.
Severity: MEDIUM

Suggested Fix

Before calling client.compare_commits, validate that ref_update.before is a non-empty string for branches where ref_update.created is false. If it is empty, log an error and return a 400 status, similar to how other payload validation errors are handled.

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/integrations/cursor_origin/push.py#L115-L116

Potential issue: The `RefUpdate.from_payload` method defaults the `before` field to an
empty string if it's missing from the webhook payload. This potentially empty string is
then passed to `client.compare_commits` for non-created branches. If the external Origin
API rejects an empty string as a valid commit SHA, the API call will fail. Since there
is no specific error handling around this call, the exception will propagate up, causing
the webhook handler to return an HTTP 500 error.

Did we get this right? 👍 / 👎 to inform future reviews.

elif status == "removed":
changes.append({"path": file["filename"], "type": "D"})
elif status == "renamed":
changes.append({"path": file["previousFilename"], "type": "D"})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: A KeyError will occur if a renamed file from the Origin API is missing the optional previousFilename field, causing an unhandled exception in the webhook.
Severity: HIGH

Suggested Fix

Use the .get() method with a default value or check for the existence of the previousFilename key before accessing it. If the key is missing for a renamed file, it should be treated as a payload error and handled gracefully instead of causing a crash.

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/integrations/cursor_origin/repository.py#L48

Potential issue: The `file_changes_from` function processes commit files from the Origin
API. For files with a status of `"renamed"`, it directly accesses the `previousFilename`
key using `file["previousFilename"]`. However, the type definition for this data,
`OriginCommitFile`, marks `previousFilename` as optional (`NotRequired`). If the Origin
API returns a renamed file without this key, a `KeyError` will be raised. This exception
is not caught by the specific `OriginPayloadError` handler and will instead be caught by
a generic `except Exception` block that re-raises, causing the webhook to crash.

Did we get this right? 👍 / 👎 to inform future reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant