Skip to content

feat(review-desk): Add a ready to merge notice for Junior pull requests - #2492

Merged
dcramer merged 9 commits into
mainfrom
feat/review-desk-pr-ready
Nov 1, 2026
Merged

dcramer merged 9 commits into
mainfrom
feat/review-desk-pr-ready

Conversation

@sentry-junior

@sentry-junior sentry-junior Bot commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

Part of #2488 (track 3, scenario C1).

When a pull request that Junior opened is ready to merge, a notice Review item now appears on the review desk for the person who asked for it. The notice tells them to merge the pull request, or to say why not. It closes by itself when the pull request merges, closes, or is no longer ready. "Changes requested" does not add an item.

Plugin contract

packages/junior-plugin-api/src/review-items.ts adds a small provider-neutral contract. Plugins get it as RouteRegistrationHookContext.reviewItems:

interface PluginConversationReviewItems {
  forConversation(conversationId: string): PluginReviewItems;
}
interface PluginReviewItems {
  upsert(notice: PluginReviewNotice): Promise<void>; // add, or replace the open item with the key
  close(input: { dedupeKey: string; reason: string; status: "done" | "dismissed" }): Promise<void>;
}
interface PluginReviewNotice {
  assigneeIdentityIds: string[];
  body: string;
  dedupeKey: string; // at most one open item per key, per plugin
  subject?: { id: string; title?: string; url?: string };
  title: string;
}

Core puts this in packages/junior/src/chat/review-items/plugin.ts, built on review-items/store.ts. Core scopes the dedupe key to the plugin and takes visibility from the Conversation. It resolves Identities to Users and keeps the rule that a private item can only go to a participant. After each change it calls the show function that app setup passes to getPluginRoutes (today the Slack card sync), so review item code imports no Slack code. A plugin change sends no messages to linked Conversations. A plugin notice never counts as an item that needs a person (reviewItemNeedsUser). Core has no GitHub names or logic.

The plugin: dedupe key prefix belongs to plugins. An agent cannot add an item with that prefix, and it cannot change or close an item that a plugin added (review-items/access.ts, tools/review-items.ts). Only the plugin or a person can. The review items README and the updateReviewItem description list "the plugin that added it closes it" as a close path.

How readiness is decided (GitHub plugin)

packages/junior-github/src/webhooks/pull-request-ready.ts handles these webhooks for Junior pull requests:

  • pull_request: closed, converted_to_draft, ready_for_review, reopened, synchronize
  • pull_request_review: submitted, dismissed
  • check_suite: completed

For each one, loadPullRequestMergeState reads the current state from GitHub with a read-only installation token. The result does not depend on the model. A pull request is ready to merge when all of these are true:

  • Open and not a draft: from GET /pulls/{n}.
  • GitHub can merge it: mergeable_state is clean. This covers required reviews, checks, commit statuses, and conflicts. GitHub computes this value after a change. When it is still unknown after a second read, the plugin changes nothing, and a later webhook decides.
  • Approved: we read every page of reviews and take each reviewer's latest review that is not a comment or pending. At least one of these must be APPROVED, and none can be CHANGES_REQUESTED.

When the pull request becomes ready, the plugin adds the notice for the opener, in the Conversation where they asked. The dedupe key is the pull request. When it merges, the plugin closes the notice as done. When it closes or is no longer ready, the plugin closes the notice as dismissed. The requester record now also stores the opener's Conversation, and the create pull request tool fills it in.

State and webhooks that arrive together

Migration 0010 adds these columns to the stored pull request:

  • ready_to_merge_at: the notice was added. Repeat webhooks add no second notice. A notice that a person closed does not come back until the pull request is ready again. The value changes only after the notice changes, so after a failure the next webhook tries again.
  • ready_sync_requests, ready_sync_id, ready_sync_at: one webhook at a time checks a pull request. A webhook that arrives during a check adds 1 to ready_sync_requests and stops. The webhook that runs the check reads GitHub again until no new request came in, also when a check fails. ready_sync_id names the check. A check older than one minute no longer blocks others.

No database connection or lock stays open during the GitHub and review desk calls. A notice failure is logged and does not fail the webhook.

Checks

via @dcramer.

--

View Session [Sentry]

@vercel

vercel Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
junior-docs Ready Ready Preview Nov 1, 2026 10:01pm UTC

Request Review

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale Bugbot comment from a previous run.

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale Bugbot comment from a previous run.

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale Bugbot comment from a previous run.

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale Bugbot comment from a previous run.

sentry-junior Bot and others added 8 commits November 1, 2026 21:45
Add a small provider-neutral contract to the plugin API, so a plugin
route can add, update, and close review desk notices for one
Conversation (`ctx.reviewItems.forConversation(id).upsert/close`). Core
implements it over the review item store. Core scopes dedupe keys to the
plugin, sets visibility from the Conversation, assigns the Users of the
given Identities, and keeps a private notice for people in its
Conversation.

The GitHub plugin uses it for Junior pull requests. On `pull_request`,
`pull_request_review`, and `check_suite` webhooks it reads the pull
request, its reviews, and its check runs from GitHub. When the pull
request is open, not a draft, approved, and its checks pass, it adds one
notice for the opener in the Conversation where they asked. It closes
the notice when the pull request merges, closes, or is no longer ready.
`ready_to_merge_at` makes only the call that changes readiness act, so
repeat webhooks add no second notice.

Part of #2488

Co-Authored-By: David Cramer <[email protected]>

Co-Authored-By: David Cramer <[email protected]>

Co-Authored-By: David Cramer <[email protected]>
The ready marker changed before the notice closed. A failed close left
the marker cleared, so later events saw no change and the notice stayed
open. Undo the marker when either the add or the close fails.

Co-Authored-By: David Cramer <[email protected]>
Decide readiness from mergeable_state instead of a custom check run list.
clean already covers required reviews, checks, commit statuses, and
conflicts. An unknown state is read once more. If it stays unknown, the
saved state does not change, and a later webhook decides.

Keep the approval rule. Tell the person to merge, or to say why not.
Inline the subject type of the plugin notice, which has one use.

Part of #2488

Co-Authored-By: David Cramer <[email protected]>
Plugin dedupe keys were the plugin name and the key. An agent could use
the same free-text key, then update or close the plugin notice from any
public Conversation in the tenant.

Store plugin keys as plugin:<name>:<key> and reserve the plugin: prefix.
addReviewItem rejects a key with the prefix, and an agent cannot change
an item that has it. Document that a plugin notice closes quietly: it
syncs the Slack cards and sends no message.

Part of #2488

Co-Authored-By: David Cramer <[email protected]>
… order

A compare-and-set on ready_to_merge_at did not cover the GitHub read or
the review item write. Two webhooks that read different states could
leave the notice and the saved state out of step, and later webhooks
then skipped the repair.

Lock the pull request row in one transaction while the webhook reads
GitHub, saves the state, and adds or closes the notice. Webhooks for one
pull request now run one at a time, and each reads GitHub after the one
before it is done. A failed add or close rolls back the saved state.

Part of #2488

Co-Authored-By: David Cramer <[email protected]>
…tion

The row lock held a pool connection while the webhook called GitHub and
the review desk. The review desk write needs a second connection from a
pool of three, so a burst of webhooks could wait forever.

Claim the check in one short update instead. A webhook that arrives
during a check adds a request and stops. The webhook that runs the check
reads GitHub again until no new request came in. The request number is
the claim id, and a claim older than one minute is free. Save
ready_to_merge_at only after the notice changes.

Part of #2488

Co-Authored-By: David Cramer <[email protected]>
A webhook that arrived while a ready to merge check failed only raised the
request count. The failed check released the claim without another check,
so the notice could stay wrong when that webhook was the last event.

Now each check attempt ends with the conditional release. Queued requests
get a new check, and the first failure is thrown after the last one.

Co-Authored-By: David Cramer <[email protected]>
Main now keeps Review item code free of Slack imports
(no-review-items-to-slack), and app setup gives the show function.
Plugin review items now take that show function from getPluginRoutes
instead of syncing Slack cards directly.

The README and the updateReviewItem description now list the plugin that
added an item as one more way it closes.

Co-Authored-By: David Cramer <[email protected]>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

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 ebd0eac. Configure here.

A pull request with a full first page of reviews counted as not approved.
So a clean, approved pull request could lose its ready to merge notice.
Now the check reads every page, and the latest review of each person
decides.

Co-Authored-By: David Cramer <[email protected]>
@dcramer
dcramer merged commit d3c6f3c into main Nov 1, 2026
62 of 64 checks passed
@dcramer
dcramer deleted the feat/review-desk-pr-ready branch November 1, 2026 22:17

This branch was successfully deployed

1 active deployment
Preview – junior-docs — cc367496 Deployed Nov 1, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk: high PR risk score: high

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant