From 1db18f07ac64c477655f28609d49f18fbf261134 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Sat, 31 Oct 2026 18:02:58 +0000 Subject: [PATCH 1/9] feat(review-desk): Add a ready to merge notice for Junior pull requests 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 Co-Authored-By: David Cramer Co-Authored-By: David Cramer --- TERMINOLOGY.md | 2 +- packages/junior-github/README.md | 16 +- .../migrations/0010_sleepy_slayback.sql | 2 + .../migrations/meta/0010_snapshot.json | 408 ++++++++++++++++++ .../migrations/meta/_journal.json | 9 +- packages/junior-github/src/db/schema.ts | 7 + packages/junior-github/src/plugin.ts | 1 + .../src/pull-request-outcomes/store.ts | 31 +- .../src/pull-request-requesters/store.ts | 30 +- .../src/tools/create-pull-request.ts | 19 +- .../junior-github/src/webhooks/check-suite.ts | 17 + .../junior-github/src/webhooks/handler.ts | 18 + .../src/webhooks/installation-requests.ts | 113 +++++ .../src/webhooks/pull-request-match-data.ts | 6 +- .../src/webhooks/pull-request-ready.ts | 181 ++++++++ packages/junior-github/tests/github-db.ts | 1 + .../tests/pull-request-ready.test.ts | 368 ++++++++++++++++ .../tests/webhook-outcomes.test.ts | 14 +- packages/junior-plugin-api/src/index.ts | 1 + packages/junior-plugin-api/src/operations.ts | 3 + .../junior-plugin-api/src/review-items.ts | 43 ++ packages/junior/src/app.ts | 6 +- .../junior/src/chat/plugins/agent-hooks.ts | 12 + .../junior/src/chat/review-items/README.md | 21 + .../junior/src/chat/review-items/plugin.ts | 152 +++++++ .../integration/plugin-review-items.test.ts | 168 ++++++++ 26 files changed, 1622 insertions(+), 27 deletions(-) create mode 100644 packages/junior-github/migrations/0010_sleepy_slayback.sql create mode 100644 packages/junior-github/migrations/meta/0010_snapshot.json create mode 100644 packages/junior-github/src/webhooks/pull-request-ready.ts create mode 100644 packages/junior-github/tests/pull-request-ready.test.ts create mode 100644 packages/junior-plugin-api/src/review-items.ts create mode 100644 packages/junior/src/chat/review-items/plugin.ts create mode 100644 packages/junior/tests/integration/plugin-review-items.test.ts diff --git a/TERMINOLOGY.md b/TERMINOLOGY.md index 2687ee5b05..33ad27da0a 100644 --- a/TERMINOLOGY.md +++ b/TERMINOLOGY.md @@ -130,7 +130,7 @@ Canonical words used across Junior's code and documentation. type as `calendar_event`. - **Item**: a linked object without a more specific native type. - **Review item**: work that needs a person, such as a decision or a notice. - An agent adds it. Its assignees act on it on the review desk. It is not an + An agent or a plugin adds it. Its assignees act on it on the review desk. It is not an Item. - **Review desk**: the dashboard page that lists open Review items. - **Assignee**: a User or Everyone who can act on a Review item. Everyone diff --git a/packages/junior-github/README.md b/packages/junior-github/README.md index ae9d9f729a..1e16a4c697 100644 --- a/packages/junior-github/README.md +++ b/packages/junior-github/README.md @@ -36,7 +36,8 @@ creating another pull request, or replacing existing assignees. The plugin saves each person who asks Junior to open or update a pull request in `junior_github_pull_request_requesters`. Each row has only the core Junior -identity id of that person. The plugin does not copy the user or GitHub login. +identity id of that person. The opener row also has the Conversation where +they asked. The plugin does not copy the user or GitHub login. When a webhook needs the GitHub login, it reads the identity through `ctx.users.readIdentity()` and uses the GitHub identity of the linked user. So a person who links GitHub later is found on their older pull requests too. The @@ -64,6 +65,19 @@ event matches, so the agent sees all of them. A failed load leaves the value out. `authors` and the people involved in approvals and merges share one read of the commits per delivery. +When a Junior pull request is ready to merge, the plugin adds a notice to the +review desk through `ctx.reviewItems`. Ready to merge means that the pull +request is open, is not a draft, has an approval and no open change request, +and every check run on its head commit passed. `pull_request`, +`pull_request_review`, and `check_suite` webhooks make the plugin read these +facts again from GitHub. The notice goes to the opener, in the Conversation +where they asked. `ready_to_merge_at` on the pull request records that the +notice was added, so repeat webhooks add no second notice. A notice that a +person closed does not come back until the pull request is ready again. The +plugin closes the notice when the pull request merges, closes, or is no longer +ready. "Changes requested" adds no notice. A notice failure is logged and does +not fail the webhook. + Deployment events come from GitHub `deployment` and `deployment_status` webhooks: diff --git a/packages/junior-github/migrations/0010_sleepy_slayback.sql b/packages/junior-github/migrations/0010_sleepy_slayback.sql new file mode 100644 index 0000000000..f4baf7f261 --- /dev/null +++ b/packages/junior-github/migrations/0010_sleepy_slayback.sql @@ -0,0 +1,2 @@ +ALTER TABLE "junior_github_pull_request_requesters" ADD COLUMN "conversation_id" text;--> statement-breakpoint +ALTER TABLE "junior_github_pull_requests" ADD COLUMN "ready_to_merge_at" timestamp with time zone; \ No newline at end of file diff --git a/packages/junior-github/migrations/meta/0010_snapshot.json b/packages/junior-github/migrations/meta/0010_snapshot.json new file mode 100644 index 0000000000..e918f6aa00 --- /dev/null +++ b/packages/junior-github/migrations/meta/0010_snapshot.json @@ -0,0 +1,408 @@ +{ + "id": "6b48609b-f71b-4667-8dbb-da286531046e", + "prevId": "6b12b332-4224-4e69-9ab7-51df6a4dd91c", + "version": "7", + "dialect": "postgresql", + "tables": { + "public.junior_github_issues": { + "name": "junior_github_issues", + "schema": "", + "columns": { + "issue_id": { + "name": "issue_id", + "type": "text", + "primaryKey": true, + "notNull": true + }, + "repository_id": { + "name": "repository_id", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "repository_full_name": { + "name": "repository_full_name", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "number": { + "name": "number", + "type": "integer", + "primaryKey": false, + "notNull": true + }, + "state": { + "name": "state", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "state_reason": { + "name": "state_reason", + "type": "text", + "primaryKey": false, + "notNull": false + }, + "conversation_ids": { + "name": "conversation_ids", + "type": "text[]", + "primaryKey": false, + "notNull": true, + "default": "ARRAY[]::text[]" + }, + "opened_at": { + "name": "opened_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": true + }, + "closed_at": { + "name": "closed_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": false + }, + "updated_at": { + "name": "updated_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": true + } + }, + "indexes": { + "junior_github_issues_opened_at_idx": { + "name": "junior_github_issues_opened_at_idx", + "columns": [ + { + "expression": "opened_at", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "concurrently": false, + "method": "btree", + "with": {} + }, + "junior_github_issues_closed_at_idx": { + "name": "junior_github_issues_closed_at_idx", + "columns": [ + { + "expression": "closed_at", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "concurrently": false, + "method": "btree", + "with": {} + }, + "junior_github_issues_open_idx": { + "name": "junior_github_issues_open_idx", + "columns": [ + { + "expression": "issue_id", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "where": "\"junior_github_issues\".\"state\" = 'open'", + "concurrently": false, + "method": "btree", + "with": {} + } + }, + "foreignKeys": {}, + "compositePrimaryKeys": {}, + "uniqueConstraints": {}, + "policies": {}, + "checkConstraints": {}, + "isRLSEnabled": false + }, + "public.junior_github_pull_request_issues": { + "name": "junior_github_pull_request_issues", + "schema": "", + "columns": { + "pull_request_id": { + "name": "pull_request_id", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "issue_repository_full_name": { + "name": "issue_repository_full_name", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "issue_number": { + "name": "issue_number", + "type": "integer", + "primaryKey": false, + "notNull": true + } + }, + "indexes": {}, + "foreignKeys": { + "junior_github_pull_request_issues_pull_request_id_junior_github_pull_requests_pull_request_id_fk": { + "name": "junior_github_pull_request_issues_pull_request_id_junior_github_pull_requests_pull_request_id_fk", + "tableFrom": "junior_github_pull_request_issues", + "tableTo": "junior_github_pull_requests", + "columnsFrom": ["pull_request_id"], + "columnsTo": ["pull_request_id"], + "onDelete": "cascade", + "onUpdate": "no action" + } + }, + "compositePrimaryKeys": { + "junior_github_pull_request_issues_pull_request_id_issue_repository_full_name_issue_number_pk": { + "name": "junior_github_pull_request_issues_pull_request_id_issue_repository_full_name_issue_number_pk", + "columns": [ + "pull_request_id", + "issue_repository_full_name", + "issue_number" + ] + } + }, + "uniqueConstraints": {}, + "policies": {}, + "checkConstraints": {}, + "isRLSEnabled": false + }, + "public.junior_github_pull_request_requesters": { + "name": "junior_github_pull_request_requesters", + "schema": "", + "columns": { + "repository_full_name": { + "name": "repository_full_name", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "number": { + "name": "number", + "type": "integer", + "primaryKey": false, + "notNull": true + }, + "identity_id": { + "name": "identity_id", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "conversation_id": { + "name": "conversation_id", + "type": "text", + "primaryKey": false, + "notNull": false + }, + "created_at": { + "name": "created_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": true + }, + "updated_at": { + "name": "updated_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": true + } + }, + "indexes": {}, + "foreignKeys": {}, + "compositePrimaryKeys": { + "junior_github_pull_request_requesters_repository_full_name_number_identity_id_pk": { + "name": "junior_github_pull_request_requesters_repository_full_name_number_identity_id_pk", + "columns": ["repository_full_name", "number", "identity_id"] + } + }, + "uniqueConstraints": {}, + "policies": {}, + "checkConstraints": {}, + "isRLSEnabled": false + }, + "public.junior_github_pull_requests": { + "name": "junior_github_pull_requests", + "schema": "", + "columns": { + "pull_request_id": { + "name": "pull_request_id", + "type": "text", + "primaryKey": true, + "notNull": true + }, + "repository_id": { + "name": "repository_id", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "repository_full_name": { + "name": "repository_full_name", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "number": { + "name": "number", + "type": "integer", + "primaryKey": false, + "notNull": true + }, + "state": { + "name": "state", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "commit_composition": { + "name": "commit_composition", + "type": "text", + "primaryKey": false, + "notNull": false + }, + "conversation_ids": { + "name": "conversation_ids", + "type": "text[]", + "primaryKey": false, + "notNull": true, + "default": "ARRAY[]::text[]" + }, + "opened_at": { + "name": "opened_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": true + }, + "merged_at": { + "name": "merged_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": false + }, + "closed_at": { + "name": "closed_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": false + }, + "updated_at": { + "name": "updated_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": true + }, + "ready_to_merge_at": { + "name": "ready_to_merge_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": false + } + }, + "indexes": { + "junior_github_pull_requests_opened_at_idx": { + "name": "junior_github_pull_requests_opened_at_idx", + "columns": [ + { + "expression": "opened_at", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "concurrently": false, + "method": "btree", + "with": {} + }, + "junior_github_pull_requests_merged_at_idx": { + "name": "junior_github_pull_requests_merged_at_idx", + "columns": [ + { + "expression": "merged_at", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "concurrently": false, + "method": "btree", + "with": {} + }, + "junior_github_pull_requests_closed_at_idx": { + "name": "junior_github_pull_requests_closed_at_idx", + "columns": [ + { + "expression": "closed_at", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "concurrently": false, + "method": "btree", + "with": {} + }, + "junior_github_pull_requests_open_idx": { + "name": "junior_github_pull_requests_open_idx", + "columns": [ + { + "expression": "pull_request_id", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "where": "\"junior_github_pull_requests\".\"state\" = 'open'", + "concurrently": false, + "method": "btree", + "with": {} + }, + "junior_github_pull_requests_unmerged_conversations_idx": { + "name": "junior_github_pull_requests_unmerged_conversations_idx", + "columns": [ + { + "expression": "conversation_ids", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "where": "\"junior_github_pull_requests\".\"state\" <> 'merged'", + "concurrently": false, + "method": "gin", + "with": {} + } + }, + "foreignKeys": {}, + "compositePrimaryKeys": {}, + "uniqueConstraints": {}, + "policies": {}, + "checkConstraints": {}, + "isRLSEnabled": false + } + }, + "enums": {}, + "schemas": {}, + "sequences": {}, + "roles": {}, + "policies": {}, + "views": {}, + "_meta": { + "columns": {}, + "schemas": {}, + "tables": {} + } +} diff --git a/packages/junior-github/migrations/meta/_journal.json b/packages/junior-github/migrations/meta/_journal.json index 463a340cf9..1be89443f6 100644 --- a/packages/junior-github/migrations/meta/_journal.json +++ b/packages/junior-github/migrations/meta/_journal.json @@ -71,6 +71,13 @@ "when": 1793225353668, "tag": "0009_nostalgic_thing", "breakpoints": true + }, + { + "idx": 10, + "version": "7", + "when": 1793468293713, + "tag": "0010_sleepy_slayback", + "breakpoints": true } ] -} \ No newline at end of file +} diff --git a/packages/junior-github/src/db/schema.ts b/packages/junior-github/src/db/schema.ts index 93d4943446..5da9a44649 100644 --- a/packages/junior-github/src/db/schema.ts +++ b/packages/junior-github/src/db/schema.ts @@ -89,6 +89,11 @@ export const juniorGitHubPullRequests = pgTable( mergedAt: timestamp("merged_at", { withTimezone: true }), closedAt: timestamp("closed_at", { withTimezone: true }), updatedAt: timestamp("updated_at", { withTimezone: true }).notNull(), + /** + * When Junior saw the pull request become ready to merge and added its + * review desk notice. Null when it is not ready. + */ + readyToMergeAt: timestamp("ready_to_merge_at", { withTimezone: true }), }, (table) => [ index("junior_github_pull_requests_opened_at_idx").on(table.openedAt), @@ -142,6 +147,8 @@ export const juniorGitHubPullRequestRequesters = pgTable( number: integer("number").notNull(), /** Core `junior_identities.id` of the person who acted. */ identityId: text("identity_id").notNull(), + /** The Conversation where the person asked Junior to open the pull request. */ + conversationId: text("conversation_id"), createdAt: timestamp("created_at", { withTimezone: true }).notNull(), updatedAt: timestamp("updated_at", { withTimezone: true }).notNull(), }, diff --git a/packages/junior-github/src/plugin.ts b/packages/junior-github/src/plugin.ts index e432e8108e..1a1b6303de 100644 --- a/packages/junior-github/src/plugin.ts +++ b/packages/junior-github/src/plugin.ts @@ -369,6 +369,7 @@ export function githubPlugin( await markPullRequestFeedbackReviewing(installationEnv, target), privateKeyEnv, events: ctx.events, + reviewItems: ctx.reviewItems, webhookSecret: () => readEnv("GITHUB_WEBHOOK_SECRET"), }), ]; diff --git a/packages/junior-github/src/pull-request-outcomes/store.ts b/packages/junior-github/src/pull-request-outcomes/store.ts index f5b5f6c661..3f24729918 100644 --- a/packages/junior-github/src/pull-request-outcomes/store.ts +++ b/packages/junior-github/src/pull-request-outcomes/store.ts @@ -1,4 +1,4 @@ -import { and, eq, lte, ne, sql } from "drizzle-orm"; +import { and, eq, isNotNull, isNull, lte, ne, sql } from "drizzle-orm"; import { z } from "zod"; import type { GitHubDb } from "../db/database.js"; import { @@ -298,3 +298,32 @@ export async function recordGitHubPullRequestLinkedIssues( ); return inserted.some((rows) => rows.length > 0); } + +/** + * Save whether a Junior-owned pull request is ready to merge. Return true + * only when this call changed it, so one caller acts on each change even when + * webhooks arrive together. + */ +export async function markGitHubPullRequestReadyToMerge( + db: GitHubDb, + input: { + number: number; + ready: boolean; + repositoryFullName: string; + }, +): Promise { + const updated = await db + .update(juniorGitHubPullRequests) + .set({ readyToMergeAt: input.ready ? new Date() : null }) + .where( + and( + sql`lower(${juniorGitHubPullRequests.repositoryFullName}) = ${input.repositoryFullName.toLowerCase()}`, + eq(juniorGitHubPullRequests.number, input.number), + input.ready + ? isNull(juniorGitHubPullRequests.readyToMergeAt) + : isNotNull(juniorGitHubPullRequests.readyToMergeAt), + ), + ) + .returning({ pullRequestId: juniorGitHubPullRequests.pullRequestId }); + return updated.length > 0; +} diff --git a/packages/junior-github/src/pull-request-requesters/store.ts b/packages/junior-github/src/pull-request-requesters/store.ts index 643a2f5361..8f953029b6 100644 --- a/packages/junior-github/src/pull-request-requesters/store.ts +++ b/packages/junior-github/src/pull-request-requesters/store.ts @@ -16,6 +16,7 @@ const table = juniorGitHubPullRequestRequesters; export async function recordGitHubPullRequestOpener( db: GitHubDb, input: { + conversationId: string; createdAt: Date; identityId: string; number: number; @@ -26,6 +27,7 @@ export async function recordGitHubPullRequestOpener( await db .insert(table) .values({ + conversationId: input.conversationId, createdAt: input.createdAt, identityId: input.identityId, number: input.number, @@ -34,7 +36,7 @@ export async function recordGitHubPullRequestOpener( }) .onConflictDoUpdate({ target: [table.repositoryFullName, table.number, table.identityId], - set: { createdAt: input.createdAt }, + set: { conversationId: input.conversationId, createdAt: input.createdAt }, }); await db .delete(table) @@ -75,16 +77,23 @@ export async function recordGitHubPullRequestRequester( }); } +/** The first person who asked Junior for a pull request. */ +export interface GitHubPullRequestOpener { + /** Set only for the person who asked Junior to open it. */ + conversationId?: string; + identityId: string; +} + /** - * Load the Junior identity id of the first requester of each pull request. - * This is the person who asked Junior to open it, or, when the create had no - * identity, the first person who asked Junior to update it. + * Load the first requester of each pull request. This is the person who + * asked Junior to open it, or, when the create had no identity, the first + * person who asked Junior to update it. */ export async function listGitHubPullRequestOpeners( db: GitHubDb, input: { numbers: number[]; repositoryFullName: string }, -): Promise> { - const openers = new Map(); +): Promise> { + const openers = new Map(); if (input.numbers.length === 0) return openers; const rows = await db .select() @@ -97,7 +106,14 @@ export async function listGitHubPullRequestOpeners( ) .orderBy(asc(table.createdAt)); for (const row of rows) { - if (!openers.has(row.number)) openers.set(row.number, row.identityId); + if (!openers.has(row.number)) { + openers.set(row.number, { + identityId: row.identityId, + ...(row.conversationId + ? { conversationId: row.conversationId } + : undefined), + }); + } } return openers; } diff --git a/packages/junior-github/src/tools/create-pull-request.ts b/packages/junior-github/src/tools/create-pull-request.ts index 48a88b8d4b..c82144c182 100644 --- a/packages/junior-github/src/tools/create-pull-request.ts +++ b/packages/junior-github/src/tools/create-pull-request.ts @@ -353,11 +353,13 @@ async function recordOpener( ctx: ToolRegistrationHookContext, input: CreateGitHubPullRequestInput, state: { createdAtMs: number; number: number }, - identityId: string | undefined, + requester: { conversationId: string; identityId: string | undefined }, ): Promise { + const { conversationId, identityId } = requester; if (!identityId) return; const repo = parseRepo(input.repo); await recordGitHubPullRequestOpener(ctx.db as GitHubDb, { + conversationId, createdAt: new Date(state.createdAtMs), identityId, number: state.number, @@ -531,12 +533,10 @@ export function createGitHubPullRequestTool( const completedInput = state.input ?? parsedInput; // The call can stop after it saves this state but before it saves // the opener, so a retry saves the opener again. - await recordOpener( - ctx, - completedInput, - state, - (await resolveGitHubRequester(ctx))?.identity?.id, - ); + await recordOpener(ctx, completedInput, state, { + conversationId, + identityId: (await resolveGitHubRequester(ctx))?.identity?.id, + }); const completedResult = { facts: state.facts === undefined @@ -610,7 +610,10 @@ export function createGitHubPullRequestTool( { cause: error }, ); } - await recordOpener(ctx, parsedInput, completedState, identityId); + await recordOpener(ctx, parsedInput, completedState, { + conversationId, + identityId, + }); await assignPullRequestRequester(ctx, parsedInput, result, requester); return await gitHubPullRequestStructuredResult( ctx, diff --git a/packages/junior-github/src/webhooks/check-suite.ts b/packages/junior-github/src/webhooks/check-suite.ts index f026d1a925..cefe24b857 100644 --- a/packages/junior-github/src/webhooks/check-suite.ts +++ b/packages/junior-github/src/webhooks/check-suite.ts @@ -448,6 +448,23 @@ export function normalizeCheckSuiteEvents( }); } +/** The same-repository pull requests of one completed check suite. */ +export function checkSuitePullRequests( + body: unknown, +): Array<{ number: number; repositoryFullName: string }> { + const parsed = checkSuiteWebhookSchema.safeParse(body); + if (!parsed.success || parsed.data.action !== "completed") return []; + const repository = parsed.data.repository; + return parsed.data.check_suite.pull_requests + .filter((pullRequest) => + isCheckSuiteRepositoryPullRequest(pullRequest, repository.id), + ) + .map((pullRequest) => ({ + number: pullRequest.number, + repositoryFullName: repository.full_name, + })); +} + /** Identifiers and event types one completed check suite may publish. */ export function parseCheckSuitePublishTargets(body: unknown): | { diff --git a/packages/junior-github/src/webhooks/handler.ts b/packages/junior-github/src/webhooks/handler.ts index b1ffa60bb0..6bb8e47a2d 100644 --- a/packages/junior-github/src/webhooks/handler.ts +++ b/packages/junior-github/src/webhooks/handler.ts @@ -4,6 +4,7 @@ import type { CodeChangeInput, CodeChangePublisher, PluginConversationAnnotations, + PluginConversationReviewItems, PluginLogger, PluginRoute, EventInput, @@ -37,6 +38,10 @@ import { parseCheckSuitePublishTargets, } from "./check-suite.js"; import { normalizeGitHubEvents } from "./events.js"; +import { + pullRequestReadyTargets, + syncGitHubPullRequestReadyNotices, +} from "./pull-request-ready.js"; import { withGitHubPullRequestMatchData, type GitHubRoleTarget, @@ -172,6 +177,7 @@ export function createGitHubWebhookRoute(args: { markFeedbackReviewing(input: FeedbackTarget): Promise; privateKeyEnv: string; events: EventPublisher; + reviewItems: PluginConversationReviewItems; webhookSecret(): string | undefined; }): PluginRoute { return { @@ -417,6 +423,18 @@ export function createGitHubWebhookRoute(args: { : "item", }); } + await syncGitHubPullRequestReadyNotices({ + db: args.db, + deliveryId, + env: { + appIdEnv: args.appIdEnv, + installationIdEnv: args.installationIdEnv, + privateKeyEnv: args.privateKeyEnv, + }, + log: args.log, + reviewItems: args.reviewItems, + targets: pullRequestReadyTargets(eventName, body), + }); if ( !pullRequestOutcome && !issueOutcome && diff --git a/packages/junior-github/src/webhooks/installation-requests.ts b/packages/junior-github/src/webhooks/installation-requests.ts index 9c64f24057..b6b3330661 100644 --- a/packages/junior-github/src/webhooks/installation-requests.ts +++ b/packages/junior-github/src/webhooks/installation-requests.ts @@ -127,3 +127,116 @@ export async function loadRepositoryRole( }, ); } + +const pullRequestSchema = z.object({ + draft: z.boolean().optional(), + head: z.object({ sha: z.string().min(1) }), + html_url: z.string().url(), + merged: z.boolean(), + state: z.enum(["open", "closed"]), + title: z.string(), +}); + +const reviewsSchema = z.array( + z.object({ + state: z.string(), + user: z.object({ login: z.string() }).nullable(), + }), +); + +const checkRunsSchema = z.object({ + check_runs: z.array( + z.object({ conclusion: z.string().nullable(), status: z.string() }), + ), + total_count: z.number(), +}); + +const PASSING_CHECK_CONCLUSIONS = new Set(["neutral", "skipped", "success"]); + +/** The pull request facts that decide whether it is ready to merge. */ +export interface GitHubPullRequestMergeState { + /** At least one approval, and no reviewer still requests changes. */ + approved: boolean; + /** The head commit has check runs, and every one finished and passed. */ + checksPassed: boolean; + draft: boolean; + state: "closed" | "merged" | "open"; + title: string; + url: string; +} + +/** + * Read whether a pull request is ready to merge, with a read-only token. + * Reviews and checks load only for an open pull request that is not a draft. + * Each reviewer counts by their latest approval, change request, or + * dismissal. Comments do not change the result. + */ +export async function loadPullRequestMergeState( + env: GitHubInstallationEnv, + pullRequest: { number: number; repositoryFullName: string }, +): Promise { + const { owner, name } = splitRepository( + pullRequest.repositoryFullName, + "pull request merge state", + ); + const token = await issueInstallationToken({ + ...env, + permissions: { checks: "read", pull_requests: "read" }, + repositories: [name], + }); + const read = async (path: string) => + await githubRequest( + "https://api.github.com", + `/repos/${encodeURIComponent(owner)}/${encodeURIComponent(name)}${path}`, + { token: token.token }, + ); + const pull = pullRequestSchema.parse( + await read(`/pulls/${pullRequest.number}`), + ); + const state: GitHubPullRequestMergeState = { + approved: false, + checksPassed: false, + draft: pull.draft === true, + state: pull.merged ? "merged" : pull.state, + title: pull.title, + url: pull.html_url, + }; + if (state.state !== "open" || state.draft) return state; + + const reviews = reviewsSchema.parse( + await read(`/pulls/${pullRequest.number}/reviews?per_page=100`), + ); + // A later page can hold a newer change request, so a full page does not pass. + if (reviews.length >= 100) return state; + const latestReviews = new Map(); + for (const review of reviews) { + if ( + review.user && + review.state !== "COMMENTED" && + review.state !== "PENDING" + ) { + latestReviews.set(review.user.login.toLowerCase(), review.state); + } + } + const reviewStates = [...latestReviews.values()]; + state.approved = + reviewStates.includes("APPROVED") && + !reviewStates.includes("CHANGES_REQUESTED"); + if (!state.approved) return state; + + const checks = checkRunsSchema.parse( + await read( + `/commits/${encodeURIComponent(pull.head.sha)}/check-runs?filter=latest&per_page=100`, + ), + ); + // A run past the first page could fail, so a partial list does not pass. + state.checksPassed = + checks.total_count > 0 && + checks.total_count === checks.check_runs.length && + checks.check_runs.every( + (run) => + run.status === "completed" && + PASSING_CHECK_CONCLUSIONS.has(run.conclusion ?? ""), + ); + return state; +} diff --git a/packages/junior-github/src/webhooks/pull-request-match-data.ts b/packages/junior-github/src/webhooks/pull-request-match-data.ts index d5565f51be..511eeeade4 100644 --- a/packages/junior-github/src/webhooks/pull-request-match-data.ts +++ b/packages/junior-github/src/webhooks/pull-request-match-data.ts @@ -120,7 +120,9 @@ async function withJuniorOpeners( repositoryFullName: context.repositoryFullName, }); const logins = new Map(); - for (const identityId of new Set(openers.values())) { + for (const identityId of new Set( + [...openers.values()].map((opener) => opener.identityId), + )) { try { logins.set(identityId, await context.loadGitHubLogin?.(identityId)); } catch (error) { @@ -130,7 +132,7 @@ async function withJuniorOpeners( return events.map((event) => { const number = botNumber(event); if (number === undefined) return event; - const identityId = openers.get(number); + const identityId = openers.get(number)?.identityId; const login = identityId ? logins.get(identityId) : undefined; const { openerEmail: _email, diff --git a/packages/junior-github/src/webhooks/pull-request-ready.ts b/packages/junior-github/src/webhooks/pull-request-ready.ts new file mode 100644 index 0000000000..dbd5a036b5 --- /dev/null +++ b/packages/junior-github/src/webhooks/pull-request-ready.ts @@ -0,0 +1,181 @@ +/** + * The "ready to merge" notice on the review desk for a Junior pull request. + * + * A pull request is ready to merge when it is open, not a draft, approved, + * and its checks pass. Each webhook that can change one of these facts reads + * them again from GitHub. The notice goes to the person who asked Junior to + * open the pull request, in the Conversation where they asked. It closes + * when the pull request merges, closes, or is no longer ready. + */ +import type { + PluginConversationReviewItems, + PluginLogger, +} from "@sentry/junior-plugin-api"; +import { z } from "zod"; +import type { GitHubDb } from "../db/database.js"; +import { markGitHubPullRequestReadyToMerge } from "../pull-request-outcomes/store.js"; +import { listGitHubPullRequestOpeners } from "../pull-request-requesters/store.js"; +import { checkSuitePullRequests } from "./check-suite.js"; +import { + loadPullRequestMergeState, + type GitHubInstallationEnv, + type GitHubPullRequestMergeState, +} from "./installation-requests.js"; +import type { GitHubPullRequestTarget } from "./pull-request-people.js"; + +/** Webhook actions that can change whether a pull request is ready. */ +const READY_ACTIONS: Record> = { + pull_request: new Set([ + "closed", + "converted_to_draft", + "ready_for_review", + "reopened", + "synchronize", + ]), + pull_request_review: new Set(["dismissed", "submitted"]), +}; + +const pullRequestWebhookSchema = z.object({ + action: z.string(), + pull_request: z.object({ number: z.number().int().positive() }), + repository: z.object({ full_name: z.string().min(1) }), +}); + +/** Return the pull requests whose readiness one webhook delivery can change. */ +export function pullRequestReadyTargets( + eventName: string, + body: unknown, +): GitHubPullRequestTarget[] { + if (eventName === "check_suite") return checkSuitePullRequests(body); + const actions = READY_ACTIONS[eventName]; + const parsed = pullRequestWebhookSchema.safeParse(body); + if (!actions || !parsed.success || !actions.has(parsed.data.action)) { + return []; + } + return [ + { + number: parsed.data.pull_request.number, + repositoryFullName: parsed.data.repository.full_name, + }, + ]; +} + +function isReadyToMerge(state: GitHubPullRequestMergeState): boolean { + return ( + state.state === "open" && + !state.draft && + state.approved && + state.checksPassed + ); +} + +function closeReason(state: GitHubPullRequestMergeState): { + reason: string; + status: "done" | "dismissed"; +} { + if (state.state === "merged") { + return { reason: "The pull request merged.", status: "done" }; + } + if (state.state === "closed") { + return { + reason: "The pull request closed without merging.", + status: "dismissed", + }; + } + if (state.draft) { + return { + reason: "The pull request is a draft again.", + status: "dismissed", + }; + } + if (!state.approved) { + return { + reason: + "The pull request is no longer approved, or a reviewer requested changes.", + status: "dismissed", + }; + } + return { + reason: "The checks of the pull request no longer pass.", + status: "dismissed", + }; +} + +async function syncOne( + args: { + db: GitHubDb; + env: GitHubInstallationEnv; + reviewItems: PluginConversationReviewItems; + }, + target: GitHubPullRequestTarget, +): Promise { + const opener = ( + await listGitHubPullRequestOpeners(args.db, { + numbers: [target.number], + repositoryFullName: target.repositoryFullName, + }) + ).get(target.number); + // Only a pull request that a person asked Junior to open has a requester + // and a Conversation for the notice. + if (!opener?.conversationId) return; + const state = await loadPullRequestMergeState(args.env, target); + const ready = isReadyToMerge(state); + // Only the call that changes the saved state acts. So repeat events add + // no second notice, and a notice that a person closed does not come back + // until the pull request is ready again. + if ( + !(await markGitHubPullRequestReadyToMerge(args.db, { ...target, ready })) + ) { + return; + } + const items = args.reviewItems.forConversation(opener.conversationId); + const name = `${target.repositoryFullName}#${target.number}`; + const dedupeKey = `pull-request-ready:${name.toLowerCase()}`; + if (!ready) { + await items.close({ dedupeKey, ...closeReason(state) }); + return; + } + try { + await items.upsert({ + assigneeIdentityIds: [opener.identityId], + body: `**${state.title}** is approved and its checks pass. You can merge it: ${state.url}`, + dedupeKey, + subject: { id: state.url, title: name, url: state.url }, + title: `Ready to merge: ${name}`, + }); + } catch (error) { + // Clear the saved state, so the next event tries again. + await markGitHubPullRequestReadyToMerge(args.db, { + ...target, + ready: false, + }); + throw error; + } +} + +/** + * Add or close the "ready to merge" notice of each Junior pull request that a + * webhook delivery can change. The notice is optional, so a failure is logged + * and does not fail the delivery. + */ +export async function syncGitHubPullRequestReadyNotices(args: { + db: GitHubDb; + deliveryId: string; + env: GitHubInstallationEnv; + log?: Pick; + reviewItems: PluginConversationReviewItems; + targets: GitHubPullRequestTarget[]; +}): Promise { + for (const target of args.targets) { + try { + await syncOne(args, target); + } catch (error) { + args.log?.error("GitHub pull request ready notice failed", { + deliveryId: args.deliveryId, + errorType: error instanceof Error ? error.name : "UnknownError", + number: target.number, + repository: target.repositoryFullName, + }); + } + } +} diff --git a/packages/junior-github/tests/github-db.ts b/packages/junior-github/tests/github-db.ts index d1492782b8..a4f81f3373 100644 --- a/packages/junior-github/tests/github-db.ts +++ b/packages/junior-github/tests/github-db.ts @@ -33,6 +33,7 @@ export async function createGitHubFixture(): Promise { "0006_fat_korvac.sql", "0007_shallow_millenium_guard.sql", "0009_nostalgic_thing.sql", + "0010_sleepy_slayback.sql", ]) { await fixture.execute(await migrationSql(migrationFile)); } diff --git a/packages/junior-github/tests/pull-request-ready.test.ts b/packages/junior-github/tests/pull-request-ready.test.ts new file mode 100644 index 0000000000..6972b39848 --- /dev/null +++ b/packages/junior-github/tests/pull-request-ready.test.ts @@ -0,0 +1,368 @@ +import { createHmac, generateKeyPairSync } from "node:crypto"; +import type { + PluginConversationReviewItems, + PluginRoute, +} from "@sentry/junior-plugin-api"; +import { http, HttpResponse } from "msw"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + juniorGitHubPullRequestRequesters, + juniorGitHubPullRequests, +} from "../src/db/schema"; +import { githubPlugin } from "../src/index"; +import { createGitHubFixture, type GitHubFixture } from "./github-db"; +import { mswServer } from "./msw"; + +const PULL_URL = "https://github.com/getsentry/junior/pull/946"; +const DEDUPE_KEY = "pull-request-ready:getsentry/junior#946"; + +let delivery = 0; + +function signedRequest(body: Record, eventName: string) { + const rawBody = JSON.stringify({ installation: { id: 456 }, ...body }); + const signature = `sha256=${createHmac("sha256", "test-secret") + .update(rawBody) + .digest("hex")}`; + delivery += 1; + return new Request("https://example.test/api/webhooks/github", { + method: "POST", + headers: { + "x-github-delivery": `delivery-${delivery}`, + "x-github-event": eventName, + "x-hub-signature-256": signature, + }, + body: rawBody, + }); +} + +const repository = { + full_name: "getsentry/junior", + id: 2001, + name: "junior", + owner: { login: "getsentry" }, +}; + +function checkSuiteCompleted() { + return signedRequest( + { + action: "completed", + check_suite: { + conclusion: "success", + head_sha: "abcdef1234567890", + id: 77, + pull_requests: [ + { + base: { repo: { id: 2001, name: "junior", url: "u" } }, + number: 946, + }, + ], + }, + repository, + }, + "check_suite", + ); +} + +function reviewSubmitted(state: "approved" | "changes_requested") { + return signedRequest( + { + action: "submitted", + pull_request: { number: 946, user: { login: "sentry-junior[bot]" } }, + repository, + review: { state, user: { login: "reviewer" } }, + }, + "pull_request_review", + ); +} + +function pullRequestAction(action: string, merged = false) { + return signedRequest( + { + action, + pull_request: { + body: "Implemented by Junior", + closed_at: action === "closed" ? "2026-07-03T12:00:00.000Z" : null, + created_at: "2026-07-01T12:00:00.000Z", + id: 1001, + merged, + merged_at: merged ? "2026-07-03T12:00:00.000Z" : null, + number: 946, + updated_at: "2026-07-03T12:00:00.000Z", + user: { login: "sentry-junior[bot]" }, + }, + repository, + sender: { login: "someone" }, + }, + "pull_request", + ); +} + +/** GitHub state that the mocked REST API returns. */ +interface GitHubState { + checks: Array<{ conclusion: string | null; status: string }>; + draft: boolean; + merged: boolean; + reviews: Array<{ state: string; user: { login: string } }>; + state: "closed" | "open"; +} + +function mockGitHub(github: GitHubState, requests: string[]) { + mswServer.use( + http.post( + "https://api.github.com/app/installations/456/access_tokens", + () => + HttpResponse.json({ + expires_at: "2027-01-01T00:00:00.000Z", + token: "installation-token", + }), + ), + http.get("https://api.github.com/repos/getsentry/junior/pulls/946", () => { + requests.push("pull"); + return HttpResponse.json({ + draft: github.draft, + head: { sha: "abcdef1234567890" }, + html_url: PULL_URL, + merged: github.merged, + state: github.state, + title: "Add the review desk notice", + }); + }), + // The merge webhook also classifies the commits. + http.get( + "https://api.github.com/repos/getsentry/junior/pulls/946/commits", + () => HttpResponse.json([]), + ), + http.get( + "https://api.github.com/repos/getsentry/junior/pulls/946/reviews", + () => { + requests.push("reviews"); + return HttpResponse.json(github.reviews); + }, + ), + http.get( + "https://api.github.com/repos/getsentry/junior/commits/abcdef1234567890/check-runs", + () => { + requests.push("checks"); + return HttpResponse.json({ + check_runs: github.checks, + total_count: github.checks.length, + }); + }, + ), + ); +} + +type ReviewItemCall = + | { conversationId: string; kind: "upsert"; notice: unknown } + | { conversationId: string; input: unknown; kind: "close" }; + +function route( + fixture: GitHubFixture, + calls: ReviewItemCall[], + errors: string[] = [], +): PluginRoute { + const reviewItems: PluginConversationReviewItems = { + forConversation(conversationId) { + return { + async upsert(notice) { + calls.push({ conversationId, kind: "upsert", notice }); + }, + async close(input) { + calls.push({ conversationId, input, kind: "close" }); + }, + }; + }, + }; + const [webhook] = githubPlugin().hooks!.routes!({ + annotations: { + forConversation() { + return { + async upsert() {}, + async remove() {}, + async list() { + return []; + }, + }; + }, + }, + codeChanges: { + async associateConversations() {}, + async record() {}, + }, + db: fixture.db(), + log: { + error(message) { + errors.push(message); + }, + info() {}, + warn() {}, + }, + plugin: { name: "github" }, + events: { hasMatch: async () => false, async publish() {} }, + reviewItems, + }); + return webhook!; +} + +async function seedJuniorPullRequest( + fixture: GitHubFixture, + conversationId: string | null, +): Promise { + const at = new Date("2026-07-01T12:00:00.000Z"); + await fixture + .db() + .insert(juniorGitHubPullRequests) + .values({ + conversationIds: ["slack:C123:1712345.0001"], + number: 946, + openedAt: at, + pullRequestId: "1001", + repositoryFullName: "getsentry/junior", + repositoryId: "2001", + state: "open", + updatedAt: at, + }); + await fixture.db().insert(juniorGitHubPullRequestRequesters).values({ + conversationId, + createdAt: at, + identityId: "identity-requester", + number: 946, + repositoryFullName: "getsentry/junior", + updatedAt: at, + }); +} + +describe("ready to merge notice", () => { + beforeEach(() => { + const privateKey = generateKeyPairSync("rsa", { modulusLength: 2048 }) + .privateKey.export({ type: "pkcs8", format: "pem" }) + .toString(); + vi.stubEnv("GITHUB_APP_ID", "123"); + vi.stubEnv("GITHUB_INSTALLATION_ID", "456"); + vi.stubEnv("GITHUB_APP_PRIVATE_KEY", privateKey); + vi.stubEnv( + "GITHUB_APP_BOT_EMAIL", + "264270552+sentry-junior[bot]@users.noreply.github.com", + ); + vi.stubEnv("GITHUB_WEBHOOK_SECRET", "test-secret"); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it("adds one notice for the requester while the pull request is ready, and closes it when that stops", async () => { + const fixture = await createGitHubFixture(); + const calls: ReviewItemCall[] = []; + const errors: string[] = []; + const requests: string[] = []; + const github: GitHubState = { + checks: [{ conclusion: "success", status: "completed" }], + draft: false, + merged: false, + reviews: [], + state: "open", + }; + mockGitHub(github, requests); + try { + await seedJuniorPullRequest(fixture, "slack:C123:1712345.0001"); + const webhook = route(fixture, calls, errors); + + // Checks pass, but no one approved it yet. + await webhook.handler(checkSuiteCompleted()); + expect(calls).toEqual([]); + expect(requests).toEqual(["pull", "reviews"]); + + github.reviews = [ + { state: "CHANGES_REQUESTED", user: { login: "reviewer" } }, + { state: "COMMENTED", user: { login: "reviewer" } }, + { state: "APPROVED", user: { login: "Reviewer" } }, + ]; + await webhook.handler(reviewSubmitted("approved")); + const notice = { + conversationId: "slack:C123:1712345.0001", + kind: "upsert", + notice: { + assigneeIdentityIds: ["identity-requester"], + body: `**Add the review desk notice** is approved and its checks pass. You can merge it: ${PULL_URL}`, + dedupeKey: DEDUPE_KEY, + subject: { + id: PULL_URL, + title: "getsentry/junior#946", + url: PULL_URL, + }, + title: "Ready to merge: getsentry/junior#946", + }, + }; + expect(calls).toEqual([notice]); + + // Repeat events while it stays ready add nothing. + await webhook.handler(checkSuiteCompleted()); + await webhook.handler(reviewSubmitted("approved")); + expect(calls).toEqual([notice]); + + // A new push restarts the checks. + github.checks = [{ conclusion: null, status: "in_progress" }]; + await webhook.handler(pullRequestAction("synchronize")); + expect(calls.at(-1)).toEqual({ + conversationId: "slack:C123:1712345.0001", + input: { + dedupeKey: DEDUPE_KEY, + reason: "The checks of the pull request no longer pass.", + status: "dismissed", + }, + kind: "close", + }); + + github.checks = [{ conclusion: "success", status: "completed" }]; + await webhook.handler(checkSuiteCompleted()); + expect(calls.at(-1)).toEqual(notice); + + github.merged = true; + github.state = "closed"; + await webhook.handler(pullRequestAction("closed", true)); + expect(calls.at(-1)).toEqual({ + conversationId: "slack:C123:1712345.0001", + input: { + dedupeKey: DEDUPE_KEY, + reason: "The pull request merged.", + status: "done", + }, + kind: "close", + }); + expect(calls.map((call) => call.kind)).toEqual([ + "upsert", + "close", + "upsert", + "close", + ]); + expect(errors).toEqual([]); + } finally { + await fixture.close(); + } + }); + + it("reads nothing from GitHub for a pull request with no requester Conversation", async () => { + const fixture = await createGitHubFixture(); + const calls: ReviewItemCall[] = []; + const requests: string[] = []; + mockGitHub( + { + checks: [{ conclusion: "success", status: "completed" }], + draft: false, + merged: false, + reviews: [{ state: "APPROVED", user: { login: "reviewer" } }], + state: "open", + }, + requests, + ); + try { + await seedJuniorPullRequest(fixture, null); + await route(fixture, calls).handler(reviewSubmitted("approved")); + expect(requests).toEqual([]); + expect(calls).toEqual([]); + } finally { + await fixture.close(); + } + }); +}); diff --git a/packages/junior-github/tests/webhook-outcomes.test.ts b/packages/junior-github/tests/webhook-outcomes.test.ts index 50420d0014..1c8754d538 100644 --- a/packages/junior-github/tests/webhook-outcomes.test.ts +++ b/packages/junior-github/tests/webhook-outcomes.test.ts @@ -1838,6 +1838,9 @@ describe("GitHub-owned pull request outcomes", () => { "0004_marvelous_toad_men.sql", "0005_github_cost_associations.sql", "0006_fat_korvac.sql", + "0007_shallow_millenium_guard.sql", + "0009_nostalgic_thing.sql", + "0010_sleepy_slayback.sql", ]) { await fixture.execute(await migrationSql(migration)); } @@ -2747,7 +2750,7 @@ describe("GitHub-owned pull request outcomes", () => { .select() .from(juniorGitHubPullRequestRequesters) .orderBy(juniorGitHubPullRequestRequesters.createdAt) - ).map((row) => [row.number, row.identityId]); + ).map((row) => [row.number, row.identityId, row.conversationId]); try { // GitHub can deliver `opened` before the create call returns. The @@ -2783,7 +2786,7 @@ describe("GitHub-owned pull request outcomes", () => { toolCallId: "call-create", }); expect(await savedRequesters()).toEqual([ - [946, "slack-identity-dcramer"], + [946, "slack-identity-dcramer", "slack:C123:1712345.0001"], ]); // A create that GitHub rejects saves no requester. @@ -2799,7 +2802,7 @@ describe("GitHub-owned pull request outcomes", () => { }), ).rejects.toThrow("HTTP 422"); expect(await savedRequesters()).toEqual([ - [946, "slack-identity-dcramer"], + [946, "slack-identity-dcramer", "slack:C123:1712345.0001"], ]); // An update adds a requester. The opener does not change. @@ -2812,9 +2815,10 @@ describe("GitHub-owned pull request outcomes", () => { ["dcramer", "admin"], ["dcramer", "admin"], ]); + // Only the opener names the Conversation where they asked. expect(await savedRequesters()).toEqual([ - [946, "slack-identity-dcramer"], - [946, "slack-identity-octocat"], + [946, "slack-identity-dcramer", "slack:C123:1712345.0001"], + [946, "slack-identity-octocat", null], ]); // An opener with no linked GitHub account is unknown. The bot is not diff --git a/packages/junior-plugin-api/src/index.ts b/packages/junior-plugin-api/src/index.ts index f3cc533c86..38f5896147 100644 --- a/packages/junior-plugin-api/src/index.ts +++ b/packages/junior-plugin-api/src/index.ts @@ -33,4 +33,5 @@ export * from "./hooks"; export * from "./cli"; export * from "./manifest"; export * from "./registration"; +export * from "./review-items"; export * from "./user-pages"; diff --git a/packages/junior-plugin-api/src/operations.ts b/packages/junior-plugin-api/src/operations.ts index 78b24d1919..70b5bfb724 100644 --- a/packages/junior-plugin-api/src/operations.ts +++ b/packages/junior-plugin-api/src/operations.ts @@ -10,6 +10,7 @@ import type { PluginConversationEventStats, } from "./conversation-events"; import type { CodeChangePublisher } from "./code"; +import type { PluginConversationReviewItems } from "./review-items"; export interface HeartbeatHookContext extends PluginContext { agent: { @@ -161,6 +162,8 @@ export interface RouteRegistrationHookContext extends PluginContext { codeChanges: CodeChangePublisher; /** Core-owned delivery boundary for provider webhook events. */ events: EventPublisher; + /** Review desk items that the plugin adds and closes from its events. */ + reviewItems: PluginConversationReviewItems; users: { /** * Read one stored identity and the user it links to now, with every diff --git a/packages/junior-plugin-api/src/review-items.ts b/packages/junior-plugin-api/src/review-items.ts new file mode 100644 index 0000000000..ca98042ff9 --- /dev/null +++ b/packages/junior-plugin-api/src/review-items.ts @@ -0,0 +1,43 @@ +/** The thing a Review item is about, such as a pull request. */ +export interface PluginReviewItemSubject { + /** Stable id that Search matches, such as a URL. */ + id: string; + title?: string; + url?: string; +} + +/** A notice that a plugin puts on the review desk. */ +export interface PluginReviewNotice { + /** Junior assigns the User linked to each Identity. */ + assigneeIdentityIds: string[]; + /** Markdown details for the person. */ + body: string; + /** + * Stable key for what the notice is about. A plugin has at most one open + * item for each key. + */ + dedupeKey: string; + subject?: PluginReviewItemSubject; + title: string; +} + +/** + * Review items that a plugin adds and closes from its own events. Each item + * belongs to one Conversation. Core owns visibility, assignment rules, and + * the Slack cards. + */ +export interface PluginReviewItems { + /** Add an open notice, or replace the content of the open one with the key. */ + upsert(notice: PluginReviewNotice): Promise; + /** Close the open item with the key. Do nothing when no item is open. */ + close(input: { + dedupeKey: string; + reason: string; + status: "done" | "dismissed"; + }): Promise; +} + +/** Review items of a plugin, for the Conversation that each item belongs to. */ +export interface PluginConversationReviewItems { + forConversation(conversationId: string): PluginReviewItems; +} diff --git a/packages/junior/src/app.ts b/packages/junior/src/app.ts index a1d6979c01..563afc2e5e 100644 --- a/packages/junior/src/app.ts +++ b/packages/junior/src/app.ts @@ -111,6 +111,7 @@ import { disconnectStateAdapter } from "@/chat/state/adapter"; import { createProductionConversationWorkOptions, createProductionSlackWebhookServices, + getProductionSlackAdapter, } from "@/chat/app/production"; import type { ConversationWorkCallbackOptions } from "@/chat/app/conversation-work"; import { createAgentRunner } from "@/chat/runtime/agent-runner"; @@ -566,7 +567,10 @@ export async function createApp( configuredPlugins?.registrations ?? [], ); } - pluginRoutes = getPluginRoutes({ events }); + pluginRoutes = getPluginRoutes({ + events, + getSlackAdapter: getProductionSlackAdapter, + }); if (dashboard && !dashboard.disabled) { pluginApiRoutes = getPluginApiRoutes(); } diff --git a/packages/junior/src/chat/plugins/agent-hooks.ts b/packages/junior/src/chat/plugins/agent-hooks.ts index 077e775e67..e2df111fd3 100644 --- a/packages/junior/src/chat/plugins/agent-hooks.ts +++ b/packages/junior/src/chat/plugins/agent-hooks.ts @@ -1,3 +1,4 @@ +import type { SlackAdapter } from "@chat-adapter/slack"; import { createPluginCache } from "@/chat/plugins/cache"; import type { OwnedObjectAnnotation } from "@sentry/junior-plugin-api"; import { @@ -82,6 +83,7 @@ import { listWorkspaceNamesByRepository, } from "@/chat/workspaces/store"; import { createCodeChangePublisher } from "@/chat/code/publisher"; +import { createPluginReviewItems } from "@/chat/review-items/plugin"; import { coreTaskRegistrations } from "@/chat/briefs/registration"; export interface ToolHookInput { @@ -937,6 +939,8 @@ function requirePublishedEvent( /** Collect route handlers exposed by plugins for app-level mounting. */ export function getPluginRoutes(options: { + /** Slack adapter for review item cards, which routes post outside Slack work. */ + getSlackAdapter?: () => SlackAdapter; events: { hasMatch?(event: Event): Promise; neededMatchKeys?(input: { @@ -969,6 +973,14 @@ export function getPluginRoutes(options: { }), }, codeChanges: createCodeChangePublisher(pluginName), + reviewItems: { + forConversation: (conversationId) => + createPluginReviewItems({ + conversationId, + getSlackAdapter: options.getSlackAdapter, + plugin: pluginName, + }), + }, users: { readIdentity: readIdentityById }, events: { async hasMatch(event) { diff --git a/packages/junior/src/chat/review-items/README.md b/packages/junior/src/chat/review-items/README.md index 3c1784d68a..7d06085cc6 100644 --- a/packages/junior/src/chat/review-items/README.md +++ b/packages/junior/src/chat/review-items/README.md @@ -16,6 +16,7 @@ behavior. actions"). - A person replies in its thread (see "Reply in the thread"). - An agent closes it with `updateReviewItem`. + - The plugin that added it closes it. - Junior purges the Conversation that added it. The item becomes `dismissed` with the reason `Conversation purged`. An item from an Automation run stays open, because a later run can still change it. @@ -95,6 +96,26 @@ description holds the rules for when to ask and when to wait. `review-desk` source. A child agent cannot use them. It reports to its parent with `reportBlocked`, and the parent adds the item. +## Plugins + +A plugin adds and closes notices from its own events through +`ctx.reviewItems` in its routes (`PluginReviewItems` in +`@sentry/junior-plugin-api`). `plugin.ts` implements it. The plugin owns the +decision. Core owns the rules below. + +- Each notice belongs to one Conversation. It links that Conversation, and it + is private when that Conversation is not public. +- Core adds the plugin name to the dedupe key, so a plugin finds and changes + only its own items. +- The plugin names assignees by Identity. Core assigns the linked User. A + private notice goes only to people in its Conversation. +- A repeat with the same key updates the open item. +- A plugin close sends no message to the linked Conversation. The provider + event that closed the item can reach that Conversation through a Watch. + +For example, the GitHub plugin adds a notice when a Junior pull request is +ready to merge. See `packages/junior-github/README.md`. + ## Person actions `actions.ts` owns what a person can do. The review desk API diff --git a/packages/junior/src/chat/review-items/plugin.ts b/packages/junior/src/chat/review-items/plugin.ts new file mode 100644 index 0000000000..51affc699a --- /dev/null +++ b/packages/junior/src/chat/review-items/plugin.ts @@ -0,0 +1,152 @@ +import type { SlackAdapter } from "@chat-adapter/slack"; +import type { + PluginReviewItems, + PluginReviewNotice, +} from "@sentry/junior-plugin-api"; +import { readConversationAccessFromSql } from "@/api/conversations/access"; +import { getDb } from "@/chat/db"; +import { readIdentityByIdFromSql } from "@/chat/plugins/viewer"; +import { syncReviewItemSlackCards } from "@/chat/slack/review-item-card-sync"; +import type { JuniorDatabase } from "@/db/db"; +import { + closeReviewItem, + getOpenReviewItemByDedupeKey, + insertReviewItem, + updateReviewItemContent, + type ReviewItemContent, +} from "./store"; + +/** + * Resolve the Users to assign. An Identity without a User has no one to + * assign. A private item goes only to people in its Conversation, the same + * rule that `requireReviewItemAssignees` applies to agents. + */ +async function assigneeUserIds( + db: JuniorDatabase, + identityIds: readonly string[], + conversationId: string, + isPrivate: boolean, +): Promise { + const userIds: string[] = []; + for (const identityId of new Set(identityIds)) { + const user = (await readIdentityByIdFromSql(db, identityId))?.user; + if (!user) continue; + if ( + isPrivate && + !(await readConversationAccessFromSql(db, [conversationId], user)).get( + conversationId, + )?.isParticipant + ) { + throw new Error( + "A private review item can be assigned only to people in its Conversation", + ); + } + userIds.push(user.id); + } + return userIds; +} + +/** + * Connect a plugin to the review desk for one Conversation. Dedupe keys are + * scoped to the plugin, so a plugin changes only its own items. An item is + * private when the Conversation is not public. A webhook runs outside Slack + * work, so pass `getSlackAdapter` to post each card with the token of its + * workspace. + */ +export function createPluginReviewItems(args: { + conversationId: string; + getSlackAdapter?: () => SlackAdapter; + plugin: string; +}): PluginReviewItems { + const { conversationId, getSlackAdapter } = args; + const scopedKey = (dedupeKey: string) => `${args.plugin}:${dedupeKey}`; + const readAccess = async (db: JuniorDatabase) => { + const access = ( + await readConversationAccessFromSql(db, [conversationId]) + ).get(conversationId); + if (!access) throw new Error(`Conversation ${conversationId} is not found`); + return access; + }; + return { + async upsert(notice: PluginReviewNotice) { + const db = getDb(); + const access = await readAccess(db); + const existing = await getOpenReviewItemByDedupeKey( + db, + scopedKey(notice.dedupeKey), + access.tenant, + ); + // An item from a private Conversation stays private. + const isPrivate = + access.visibility !== "public" || existing?.visibility === "private"; + if ( + existing && + isPrivate && + existing.addedByConversationId !== conversationId + ) { + throw new Error( + "A private review item links only to the Conversation that added it", + ); + } + const content: ReviewItemContent = { + assignedToEveryone: false, + assigneeUserIds: await assigneeUserIds( + db, + notice.assigneeIdentityIds, + conversationId, + isPrivate, + ), + body: notice.body, + conversationIds: [ + ...new Set([conversationId, ...(existing?.conversationIds ?? [])]), + ], + kind: "notice", + title: notice.title, + visibility: isPrivate ? "private" : "public", + ...(notice.subject ? { subject: notice.subject } : undefined), + }; + if (existing) { + await updateReviewItemContent(db, { + ...content, + id: existing.id, + nowMs: Date.now(), + }); + await syncReviewItemSlackCards({ + getSlackAdapter, + itemId: existing.id, + }); + return; + } + const item = await insertReviewItem(db, { + ...content, + addedByConversationId: conversationId, + dedupeKey: scopedKey(notice.dedupeKey), + nowMs: Date.now(), + ...(access.tenant ? { tenant: access.tenant } : undefined), + }); + // Undefined means a concurrent upsert added the item first. + if (item) { + await syncReviewItemSlackCards({ getSlackAdapter, itemId: item.id }); + } + }, + async close(input) { + const db = getDb(); + const access = await readAccess(db); + const item = await getOpenReviewItemByDedupeKey( + db, + scopedKey(input.dedupeKey), + access.tenant, + ); + if (!item) return; + const closed = await closeReviewItem(db, { + id: item.id, + nowMs: Date.now(), + reason: input.reason, + status: input.status, + }); + if (closed) { + await syncReviewItemSlackCards({ getSlackAdapter, itemId: item.id }); + } + }, + }; +} diff --git a/packages/junior/tests/integration/plugin-review-items.test.ts b/packages/junior/tests/integration/plugin-review-items.test.ts new file mode 100644 index 0000000000..16296cbeda --- /dev/null +++ b/packages/junior/tests/integration/plugin-review-items.test.ts @@ -0,0 +1,168 @@ +import type { User } from "@sentry/junior-plugin-api"; +import { afterEach, describe, expect, it } from "vitest"; +import { closeDb, getConversationStore, getDb } from "@/chat/db"; +import { resolveViewerUserFromSql } from "@/chat/plugins/viewer"; +import { createPluginReviewItems } from "@/chat/review-items/plugin"; +import { + getOpenReviewItemByDedupeKey, + getReviewItem, +} from "@/chat/review-items/store"; +import { juniorConversationParticipants, juniorIdentities } from "@/db/schema"; + +const TENANT = { provider: "slack", providerTenantId: "T123" }; + +async function conversation( + conversationId: string, + visibility: "public" | "private", +): Promise { + await getConversationStore().recordActivity({ + conversationId, + destination: { platform: "slack", teamId: "T123", channelId: "C1" }, + nowMs: Date.parse("2026-10-01T12:00:00.000Z"), + source: "slack", + visibility, + }); +} + +/** Add a User with one Slack Identity, and return the Identity id. */ +async function person( + email: string, +): Promise<{ identityId: string; user: User }> { + const user = await resolveViewerUserFromSql(getDb(), email); + if (!user) throw new Error(`no user for ${email}`); + const identityId = `identity:${email}`; + await getDb() + .insert(juniorIdentities) + .values({ + createdAt: new Date(1), + id: identityId, + kind: "user", + provider: "slack", + providerSubjectId: email, + providerTenantId: "T123", + updatedAt: new Date(1), + userId: user.id, + }); + return { identityId, user }; +} + +const NOTICE = { + body: "**Fix the thing** is approved and its checks pass.", + dedupeKey: "pull-request-ready:getsentry/junior#946", + subject: { + id: "https://github.com/getsentry/junior/pull/946", + title: "getsentry/junior#946", + url: "https://github.com/getsentry/junior/pull/946", + }, + title: "Ready to merge: getsentry/junior#946", +}; + +describe("plugin review items", () => { + afterEach(async () => { + await closeDb(); + }); + + it("keeps one open notice per plugin key and closes it", async () => { + await conversation("slack:C1:1.000001", "public"); + const ana = await person("ana@example.com"); + const items = createPluginReviewItems({ + conversationId: "slack:C1:1.000001", + plugin: "github", + }); + + await items.upsert({ ...NOTICE, assigneeIdentityIds: [ana.identityId] }); + const added = await getOpenReviewItemByDedupeKey( + getDb(), + `github:${NOTICE.dedupeKey}`, + TENANT, + ); + expect(added).toMatchObject({ + addedByConversationId: "slack:C1:1.000001", + assignees: [{ email: "ana@example.com", userId: ana.user.id }], + conversationIds: ["slack:C1:1.000001"], + kind: "notice", + status: "open", + subject: NOTICE.subject, + title: NOTICE.title, + visibility: "public", + }); + + // A repeat updates the open item. + await items.upsert({ + ...NOTICE, + assigneeIdentityIds: [ana.identityId], + title: "Still ready to merge", + }); + expect( + await getOpenReviewItemByDedupeKey( + getDb(), + `github:${NOTICE.dedupeKey}`, + TENANT, + ), + ).toMatchObject({ id: added!.id, title: "Still ready to merge" }); + + await items.close({ + dedupeKey: NOTICE.dedupeKey, + reason: "The pull request merged.", + status: "done", + }); + expect(await getReviewItem(getDb(), added!.id)).toMatchObject({ + closeReason: "The pull request merged.", + status: "done", + }); + // A close with no open item does nothing. + await items.close({ + dedupeKey: NOTICE.dedupeKey, + reason: "Again.", + status: "dismissed", + }); + expect(await getReviewItem(getDb(), added!.id)).toMatchObject({ + closeReason: "The pull request merged.", + }); + + // The key is scoped to the plugin, so another plugin has its own item. + await createPluginReviewItems({ + conversationId: "slack:C1:1.000001", + plugin: "linear", + }).upsert({ ...NOTICE, assigneeIdentityIds: [] }); + expect( + await getOpenReviewItemByDedupeKey( + getDb(), + `linear:${NOTICE.dedupeKey}`, + TENANT, + ), + ).toMatchObject({ assignees: [], status: "open" }); + }); + + it("assigns a private notice only to people in its Conversation", async () => { + await conversation("slack:D1:1.000001", "private"); + const ana = await person("ana@example.com"); + const ben = await person("ben@example.com"); + await getDb() + .insert(juniorConversationParticipants) + .values({ + lastMessageAt: new Date(1), + rootConversationId: "slack:D1:1.000001", + userId: ana.user.id, + }); + const items = createPluginReviewItems({ + conversationId: "slack:D1:1.000001", + plugin: "github", + }); + + await expect( + items.upsert({ ...NOTICE, assigneeIdentityIds: [ben.identityId] }), + ).rejects.toThrow("only to people in its Conversation"); + await items.upsert({ ...NOTICE, assigneeIdentityIds: [ana.identityId] }); + expect( + await getOpenReviewItemByDedupeKey( + getDb(), + `github:${NOTICE.dedupeKey}`, + TENANT, + ), + ).toMatchObject({ + assignees: [{ userId: ana.user.id }], + visibility: "private", + }); + }); +}); From cf7f3854b024fa9b931697e213d3cbe40e099114 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Sat, 31 Oct 2026 18:17:29 +0000 Subject: [PATCH 2/9] fix(review-desk): Retry a failed ready to merge notice close 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 --- .../src/webhooks/pull-request-ready.ts | 26 +++++++++---------- .../tests/pull-request-ready.test.ts | 15 +++++++++-- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/packages/junior-github/src/webhooks/pull-request-ready.ts b/packages/junior-github/src/webhooks/pull-request-ready.ts index dbd5a036b5..ed686fd33e 100644 --- a/packages/junior-github/src/webhooks/pull-request-ready.ts +++ b/packages/junior-github/src/webhooks/pull-request-ready.ts @@ -131,23 +131,23 @@ async function syncOne( const items = args.reviewItems.forConversation(opener.conversationId); const name = `${target.repositoryFullName}#${target.number}`; const dedupeKey = `pull-request-ready:${name.toLowerCase()}`; - if (!ready) { - await items.close({ dedupeKey, ...closeReason(state) }); - return; - } try { - await items.upsert({ - assigneeIdentityIds: [opener.identityId], - body: `**${state.title}** is approved and its checks pass. You can merge it: ${state.url}`, - dedupeKey, - subject: { id: state.url, title: name, url: state.url }, - title: `Ready to merge: ${name}`, - }); + if (ready) { + await items.upsert({ + assigneeIdentityIds: [opener.identityId], + body: `**${state.title}** is approved and its checks pass. You can merge it: ${state.url}`, + dedupeKey, + subject: { id: state.url, title: name, url: state.url }, + title: `Ready to merge: ${name}`, + }); + } else { + await items.close({ dedupeKey, ...closeReason(state) }); + } } catch (error) { - // Clear the saved state, so the next event tries again. + // Undo the saved state, so the next event tries again. await markGitHubPullRequestReadyToMerge(args.db, { ...target, - ready: false, + ready: !ready, }); throw error; } diff --git a/packages/junior-github/tests/pull-request-ready.test.ts b/packages/junior-github/tests/pull-request-ready.test.ts index 6972b39848..3633585c15 100644 --- a/packages/junior-github/tests/pull-request-ready.test.ts +++ b/packages/junior-github/tests/pull-request-ready.test.ts @@ -160,6 +160,7 @@ function route( fixture: GitHubFixture, calls: ReviewItemCall[], errors: string[] = [], + failures = { close: 0 }, ): PluginRoute { const reviewItems: PluginConversationReviewItems = { forConversation(conversationId) { @@ -168,6 +169,10 @@ function route( calls.push({ conversationId, kind: "upsert", notice }); }, async close(input) { + if (failures.close > 0) { + failures.close -= 1; + throw new Error("review desk is down"); + } calls.push({ conversationId, input, kind: "close" }); }, }; @@ -255,6 +260,7 @@ describe("ready to merge notice", () => { const fixture = await createGitHubFixture(); const calls: ReviewItemCall[] = []; const errors: string[] = []; + const failures = { close: 0 }; const requests: string[] = []; const github: GitHubState = { checks: [{ conclusion: "success", status: "completed" }], @@ -266,7 +272,7 @@ describe("ready to merge notice", () => { mockGitHub(github, requests); try { await seedJuniorPullRequest(fixture, "slack:C123:1712345.0001"); - const webhook = route(fixture, calls, errors); + const webhook = route(fixture, calls, errors, failures); // Checks pass, but no one approved it yet. await webhook.handler(checkSuiteCompleted()); @@ -320,6 +326,11 @@ describe("ready to merge notice", () => { github.merged = true; github.state = "closed"; + // A failed close is tried again on the next event. + failures.close = 1; + await webhook.handler(pullRequestAction("closed", true)); + expect(calls.at(-1)).toEqual(notice); + expect(errors).toEqual(["GitHub pull request ready notice failed"]); await webhook.handler(pullRequestAction("closed", true)); expect(calls.at(-1)).toEqual({ conversationId: "slack:C123:1712345.0001", @@ -336,7 +347,7 @@ describe("ready to merge notice", () => { "upsert", "close", ]); - expect(errors).toEqual([]); + expect(errors).toHaveLength(1); } finally { await fixture.close(); } From ebccfdd2189b502be6fc95e687d8109dafb8e2f9 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Sun, 1 Nov 2026 19:35:35 +0000 Subject: [PATCH 3/9] ref(review-desk): Use GitHub's merge state for the ready to merge notice 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 --- packages/junior-github/README.md | 11 ++- .../src/webhooks/installation-requests.ts | 82 ++++++++----------- .../src/webhooks/pull-request-ready.ts | 40 +++------ .../tests/pull-request-ready.test.ts | 43 +++++----- .../junior-plugin-api/src/review-items.ts | 11 +-- .../junior/src/chat/review-items/README.md | 2 + 6 files changed, 80 insertions(+), 109 deletions(-) diff --git a/packages/junior-github/README.md b/packages/junior-github/README.md index 1e16a4c697..caf955de3f 100644 --- a/packages/junior-github/README.md +++ b/packages/junior-github/README.md @@ -68,10 +68,13 @@ the commits per delivery. When a Junior pull request is ready to merge, the plugin adds a notice to the review desk through `ctx.reviewItems`. Ready to merge means that the pull request is open, is not a draft, has an approval and no open change request, -and every check run on its head commit passed. `pull_request`, -`pull_request_review`, and `check_suite` webhooks make the plugin read these -facts again from GitHub. The notice goes to the opener, in the Conversation -where they asked. `ready_to_merge_at` on the pull request records that the +and GitHub's `mergeable_state` is `clean`. `clean` covers required reviews, +checks, commit statuses, and conflicts. `pull_request`, `pull_request_review`, +and `check_suite` webhooks make the plugin read these facts again from +GitHub. GitHub computes `mergeable_state` after a change. When it is still +unknown after a second read, the plugin changes nothing, and a later webhook +decides. The notice goes to the opener, in the Conversation where they asked. +It tells them to merge the pull request, or to say why not. `ready_to_merge_at` on the pull request records that the notice was added, so repeat webhooks add no second notice. A notice that a person closed does not come back until the pull request is ready again. The plugin closes the notice when the pull request merges, closes, or is no longer diff --git a/packages/junior-github/src/webhooks/installation-requests.ts b/packages/junior-github/src/webhooks/installation-requests.ts index b6b3330661..c1774ab96e 100644 --- a/packages/junior-github/src/webhooks/installation-requests.ts +++ b/packages/junior-github/src/webhooks/installation-requests.ts @@ -130,8 +130,8 @@ export async function loadRepositoryRole( const pullRequestSchema = z.object({ draft: z.boolean().optional(), - head: z.object({ sha: z.string().min(1) }), html_url: z.string().url(), + mergeable_state: z.string().nullable().optional(), merged: z.boolean(), state: z.enum(["open", "closed"]), title: z.string(), @@ -144,22 +144,20 @@ const reviewsSchema = z.array( }), ); -const checkRunsSchema = z.object({ - check_runs: z.array( - z.object({ conclusion: z.string().nullable(), status: z.string() }), - ), - total_count: z.number(), -}); - -const PASSING_CHECK_CONCLUSIONS = new Set(["neutral", "skipped", "success"]); - /** The pull request facts that decide whether it is ready to merge. */ export interface GitHubPullRequestMergeState { - /** At least one approval, and no reviewer still requests changes. */ + /** + * At least one approval, and no reviewer still requests changes. Read + * only when GitHub says the pull request can merge cleanly. + */ approved: boolean; - /** The head commit has check runs, and every one finished and passed. */ - checksPassed: boolean; draft: boolean; + /** + * GitHub's `mergeable_state`, such as `clean` or `blocked`. `clean` means + * that the required reviews and every check and status passed, and there + * are no conflicts. Undefined when GitHub has not computed it yet. + */ + mergeableState?: string; state: "closed" | "merged" | "open"; title: string; url: string; @@ -167,9 +165,10 @@ export interface GitHubPullRequestMergeState { /** * Read whether a pull request is ready to merge, with a read-only token. - * Reviews and checks load only for an open pull request that is not a draft. - * Each reviewer counts by their latest approval, change request, or - * dismissal. Comments do not change the result. + * GitHub computes `mergeable_state` after a change, so an unknown state is + * read once more. Reviews load only for an open pull request that is not a + * draft and can merge cleanly. Each reviewer counts by their latest + * approval, change request, or dismissal. Comments do not change the result. */ export async function loadPullRequestMergeState( env: GitHubInstallationEnv, @@ -181,31 +180,36 @@ export async function loadPullRequestMergeState( ); const token = await issueInstallationToken({ ...env, - permissions: { checks: "read", pull_requests: "read" }, + permissions: { pull_requests: "read" }, repositories: [name], }); const read = async (path: string) => await githubRequest( "https://api.github.com", - `/repos/${encodeURIComponent(owner)}/${encodeURIComponent(name)}${path}`, + `/repos/${encodeURIComponent(owner)}/${encodeURIComponent(name)}/pulls/${pullRequest.number}${path}`, { token: token.token }, ); - const pull = pullRequestSchema.parse( - await read(`/pulls/${pullRequest.number}`), - ); - const state: GitHubPullRequestMergeState = { - approved: false, - checksPassed: false, - draft: pull.draft === true, - state: pull.merged ? "merged" : pull.state, - title: pull.title, - url: pull.html_url, + const readPull = async () => { + const pull = pullRequestSchema.parse(await read("")); + const mergeableState = + pull.mergeable_state && pull.mergeable_state !== "unknown" + ? pull.mergeable_state + : undefined; + return { + approved: false, + draft: pull.draft === true, + ...(mergeableState ? { mergeableState } : undefined), + state: pull.merged ? ("merged" as const) : pull.state, + title: pull.title, + url: pull.html_url, + }; }; + let state: GitHubPullRequestMergeState = await readPull(); if (state.state !== "open" || state.draft) return state; + if (!state.mergeableState) state = await readPull(); + if (state.mergeableState !== "clean") return state; - const reviews = reviewsSchema.parse( - await read(`/pulls/${pullRequest.number}/reviews?per_page=100`), - ); + const reviews = reviewsSchema.parse(await read("/reviews?per_page=100")); // A later page can hold a newer change request, so a full page does not pass. if (reviews.length >= 100) return state; const latestReviews = new Map(); @@ -222,21 +226,5 @@ export async function loadPullRequestMergeState( state.approved = reviewStates.includes("APPROVED") && !reviewStates.includes("CHANGES_REQUESTED"); - if (!state.approved) return state; - - const checks = checkRunsSchema.parse( - await read( - `/commits/${encodeURIComponent(pull.head.sha)}/check-runs?filter=latest&per_page=100`, - ), - ); - // A run past the first page could fail, so a partial list does not pass. - state.checksPassed = - checks.total_count > 0 && - checks.total_count === checks.check_runs.length && - checks.check_runs.every( - (run) => - run.status === "completed" && - PASSING_CHECK_CONCLUSIONS.has(run.conclusion ?? ""), - ); return state; } diff --git a/packages/junior-github/src/webhooks/pull-request-ready.ts b/packages/junior-github/src/webhooks/pull-request-ready.ts index ed686fd33e..4be6b40f6e 100644 --- a/packages/junior-github/src/webhooks/pull-request-ready.ts +++ b/packages/junior-github/src/webhooks/pull-request-ready.ts @@ -2,10 +2,11 @@ * The "ready to merge" notice on the review desk for a Junior pull request. * * A pull request is ready to merge when it is open, not a draft, approved, - * and its checks pass. Each webhook that can change one of these facts reads - * them again from GitHub. The notice goes to the person who asked Junior to - * open the pull request, in the Conversation where they asked. It closes - * when the pull request merges, closes, or is no longer ready. + * and GitHub's merge state is `clean`. Each webhook that can change one of + * these facts reads them again from GitHub. The notice goes to the person + * who asked Junior to open the pull request, in the Conversation where they + * asked. It closes when the pull request merges, closes, or is no longer + * ready. */ import type { PluginConversationReviewItems, @@ -60,15 +61,6 @@ export function pullRequestReadyTargets( ]; } -function isReadyToMerge(state: GitHubPullRequestMergeState): boolean { - return ( - state.state === "open" && - !state.draft && - state.approved && - state.checksPassed - ); -} - function closeReason(state: GitHubPullRequestMergeState): { reason: string; status: "done" | "dismissed"; @@ -82,21 +74,8 @@ function closeReason(state: GitHubPullRequestMergeState): { status: "dismissed", }; } - if (state.draft) { - return { - reason: "The pull request is a draft again.", - status: "dismissed", - }; - } - if (!state.approved) { - return { - reason: - "The pull request is no longer approved, or a reviewer requested changes.", - status: "dismissed", - }; - } return { - reason: "The checks of the pull request no longer pass.", + reason: "The pull request is no longer ready to merge.", status: "dismissed", }; } @@ -119,7 +98,10 @@ async function syncOne( // and a Conversation for the notice. if (!opener?.conversationId) return; const state = await loadPullRequestMergeState(args.env, target); - const ready = isReadyToMerge(state); + const open = state.state === "open" && !state.draft; + // GitHub has not computed the merge state yet. A later event decides. + if (open && !state.mergeableState) return; + const ready = open && state.mergeableState === "clean" && state.approved; // Only the call that changes the saved state acts. So repeat events add // no second notice, and a notice that a person closed does not come back // until the pull request is ready again. @@ -135,7 +117,7 @@ async function syncOne( if (ready) { await items.upsert({ assigneeIdentityIds: [opener.identityId], - body: `**${state.title}** is approved and its checks pass. You can merge it: ${state.url}`, + body: `**${state.title}** is approved, and GitHub says that it can merge. Merge it, or say why not: ${state.url}`, dedupeKey, subject: { id: state.url, title: name, url: state.url }, title: `Ready to merge: ${name}`, diff --git a/packages/junior-github/tests/pull-request-ready.test.ts b/packages/junior-github/tests/pull-request-ready.test.ts index 3633585c15..10de92e80f 100644 --- a/packages/junior-github/tests/pull-request-ready.test.ts +++ b/packages/junior-github/tests/pull-request-ready.test.ts @@ -99,8 +99,9 @@ function pullRequestAction(action: string, merged = false) { /** GitHub state that the mocked REST API returns. */ interface GitHubState { - checks: Array<{ conclusion: string | null; status: string }>; draft: boolean; + /** Each pull request read takes the next value. The last one repeats. */ + mergeableStates: Array; merged: boolean; reviews: Array<{ state: string; user: { login: string } }>; state: "closed" | "open"; @@ -118,10 +119,14 @@ function mockGitHub(github: GitHubState, requests: string[]) { ), http.get("https://api.github.com/repos/getsentry/junior/pulls/946", () => { requests.push("pull"); + const mergeableState = + github.mergeableStates.length > 1 + ? github.mergeableStates.shift() + : github.mergeableStates[0]; return HttpResponse.json({ draft: github.draft, - head: { sha: "abcdef1234567890" }, html_url: PULL_URL, + mergeable_state: mergeableState, merged: github.merged, state: github.state, title: "Add the review desk notice", @@ -139,16 +144,6 @@ function mockGitHub(github: GitHubState, requests: string[]) { return HttpResponse.json(github.reviews); }, ), - http.get( - "https://api.github.com/repos/getsentry/junior/commits/abcdef1234567890/check-runs", - () => { - requests.push("checks"); - return HttpResponse.json({ - check_runs: github.checks, - total_count: github.checks.length, - }); - }, - ), ); } @@ -263,8 +258,8 @@ describe("ready to merge notice", () => { const failures = { close: 0 }; const requests: string[] = []; const github: GitHubState = { - checks: [{ conclusion: "success", status: "completed" }], draft: false, + mergeableStates: ["clean"], merged: false, reviews: [], state: "open", @@ -274,7 +269,7 @@ describe("ready to merge notice", () => { await seedJuniorPullRequest(fixture, "slack:C123:1712345.0001"); const webhook = route(fixture, calls, errors, failures); - // Checks pass, but no one approved it yet. + // GitHub can merge it, but no one approved it yet. await webhook.handler(checkSuiteCompleted()); expect(calls).toEqual([]); expect(requests).toEqual(["pull", "reviews"]); @@ -290,7 +285,7 @@ describe("ready to merge notice", () => { kind: "upsert", notice: { assigneeIdentityIds: ["identity-requester"], - body: `**Add the review desk notice** is approved and its checks pass. You can merge it: ${PULL_URL}`, + body: `**Add the review desk notice** is approved, and GitHub says that it can merge. Merge it, or say why not: ${PULL_URL}`, dedupeKey: DEDUPE_KEY, subject: { id: PULL_URL, @@ -307,20 +302,28 @@ describe("ready to merge notice", () => { await webhook.handler(reviewSubmitted("approved")); expect(calls).toEqual([notice]); - // A new push restarts the checks. - github.checks = [{ conclusion: null, status: "in_progress" }]; + // GitHub has not computed the merge state after a push. Two reads + // stay unknown, so nothing changes, and a later event decides. + requests.length = 0; + github.mergeableStates = [null, "unknown"]; await webhook.handler(pullRequestAction("synchronize")); + expect(requests).toEqual(["pull", "pull"]); + expect(calls).toEqual([notice]); + + // The second read has the merge state: the new checks still run. + github.mergeableStates = ["unknown", "blocked"]; + await webhook.handler(checkSuiteCompleted()); expect(calls.at(-1)).toEqual({ conversationId: "slack:C123:1712345.0001", input: { dedupeKey: DEDUPE_KEY, - reason: "The checks of the pull request no longer pass.", + reason: "The pull request is no longer ready to merge.", status: "dismissed", }, kind: "close", }); - github.checks = [{ conclusion: "success", status: "completed" }]; + github.mergeableStates = ["clean"]; await webhook.handler(checkSuiteCompleted()); expect(calls.at(-1)).toEqual(notice); @@ -359,8 +362,8 @@ describe("ready to merge notice", () => { const requests: string[] = []; mockGitHub( { - checks: [{ conclusion: "success", status: "completed" }], draft: false, + mergeableStates: ["clean"], merged: false, reviews: [{ state: "APPROVED", user: { login: "reviewer" } }], state: "open", diff --git a/packages/junior-plugin-api/src/review-items.ts b/packages/junior-plugin-api/src/review-items.ts index ca98042ff9..3f7f8b77fe 100644 --- a/packages/junior-plugin-api/src/review-items.ts +++ b/packages/junior-plugin-api/src/review-items.ts @@ -1,11 +1,3 @@ -/** The thing a Review item is about, such as a pull request. */ -export interface PluginReviewItemSubject { - /** Stable id that Search matches, such as a URL. */ - id: string; - title?: string; - url?: string; -} - /** A notice that a plugin puts on the review desk. */ export interface PluginReviewNotice { /** Junior assigns the User linked to each Identity. */ @@ -17,7 +9,8 @@ export interface PluginReviewNotice { * item for each key. */ dedupeKey: string; - subject?: PluginReviewItemSubject; + /** What the notice is about, such as a pull request. Search matches `id`. */ + subject?: { id: string; title?: string; url?: string }; title: string; } diff --git a/packages/junior/src/chat/review-items/README.md b/packages/junior/src/chat/review-items/README.md index 7d06085cc6..2d3358e13f 100644 --- a/packages/junior/src/chat/review-items/README.md +++ b/packages/junior/src/chat/review-items/README.md @@ -110,6 +110,8 @@ decision. Core owns the rules below. - The plugin names assignees by Identity. Core assigns the linked User. A private notice goes only to people in its Conversation. - A repeat with the same key updates the open item. +- A plugin notice stays open when its Conversation starts a new Turn. Only + the plugin or a person closes it. - A plugin close sends no message to the linked Conversation. The provider event that closed the item can reach that Conversation through a Watch. From bde1f4ff45a864eb02979d127e0ecebab075cfca Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Sun, 1 Nov 2026 19:40:24 +0000 Subject: [PATCH 4/9] fix(review-desk): Keep agents out of plugin review items 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:: 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 --- .../junior/src/chat/review-items/README.md | 15 +++--- .../junior/src/chat/review-items/access.ts | 10 +++- .../junior/src/chat/review-items/plugin.ts | 8 +++- .../junior/src/chat/tools/review-items.ts | 11 +++++ .../integration/plugin-review-items.test.ts | 46 +++++++++++++++++-- 5 files changed, 76 insertions(+), 14 deletions(-) diff --git a/packages/junior/src/chat/review-items/README.md b/packages/junior/src/chat/review-items/README.md index 2d3358e13f..1443d6d3f9 100644 --- a/packages/junior/src/chat/review-items/README.md +++ b/packages/junior/src/chat/review-items/README.md @@ -105,15 +105,18 @@ decision. Core owns the rules below. - Each notice belongs to one Conversation. It links that Conversation, and it is private when that Conversation is not public. -- Core adds the plugin name to the dedupe key, so a plugin finds and changes - only its own items. +- Core stores the dedupe key as `plugin::`, so a plugin finds and + changes only its own items. The `plugin:` prefix is reserved. An agent + cannot add an item with it, and cannot change an item that has it + (`canAgentChangeReviewItem`). Only the plugin or a person changes the item. - The plugin names assignees by Identity. Core assigns the linked User. A private notice goes only to people in its Conversation. - A repeat with the same key updates the open item. -- A plugin notice stays open when its Conversation starts a new Turn. Only - the plugin or a person closes it. -- A plugin close sends no message to the linked Conversation. The provider - event that closed the item can reach that Conversation through a Watch. +- A plugin notice stays open when its Conversation starts a new Turn. +- A plugin notice closes quietly. A plugin close sends no message to the + linked Conversation, so the close does not start a Turn. It only syncs the + Slack cards. The provider event that closed the item can reach the + Conversation through a Watch. For example, the GitHub plugin adds a notice when a Junior pull request is ready to merge. See `packages/junior-github/README.md`. diff --git a/packages/junior/src/chat/review-items/access.ts b/packages/junior/src/chat/review-items/access.ts index 53498ff980..a9ea2fb5b0 100644 --- a/packages/junior/src/chat/review-items/access.ts +++ b/packages/junior/src/chat/review-items/access.ts @@ -184,16 +184,24 @@ export async function readReviewItemAgentScope( }; } +/** + * Dedupe keys with this prefix belong to plugins. Only the plugin that added + * the item, or a person, can change it. An agent cannot use the prefix. + */ +export const PLUGIN_REVIEW_ITEM_KEY_PREFIX = "plugin:"; + /** * Whether an agent can change an item. Only an agent in the tenant of the * item can change it. A public item changes only from a public * Conversation, so private content never reaches its readers. A private - * item changes only from the Conversation or Automation that added it. + * item changes only from the Conversation or Automation that added it. An + * agent never changes an item that a plugin added. */ export function canAgentChangeReviewItem( item: ReviewItem, scope: ReviewItemAgentScope, ): boolean { + if (item.dedupeKey.startsWith(PLUGIN_REVIEW_ITEM_KEY_PREFIX)) return false; if (!isSameTenant(item.tenant, scope.tenant)) return false; if (item.visibility === "public") return !scope.inPrivateConversation; return ( diff --git a/packages/junior/src/chat/review-items/plugin.ts b/packages/junior/src/chat/review-items/plugin.ts index 51affc699a..39a0df2922 100644 --- a/packages/junior/src/chat/review-items/plugin.ts +++ b/packages/junior/src/chat/review-items/plugin.ts @@ -4,6 +4,7 @@ import type { PluginReviewNotice, } from "@sentry/junior-plugin-api"; import { readConversationAccessFromSql } from "@/api/conversations/access"; +import { PLUGIN_REVIEW_ITEM_KEY_PREFIX } from "@/chat/review-items/access"; import { getDb } from "@/chat/db"; import { readIdentityByIdFromSql } from "@/chat/plugins/viewer"; import { syncReviewItemSlackCards } from "@/chat/slack/review-item-card-sync"; @@ -48,7 +49,9 @@ async function assigneeUserIds( /** * Connect a plugin to the review desk for one Conversation. Dedupe keys are - * scoped to the plugin, so a plugin changes only its own items. An item is + * `plugin::`, so a plugin changes only its own items, and agents + * cannot change them. A close sends no message to the Conversation. It only + * syncs the Slack cards. An item is * private when the Conversation is not public. A webhook runs outside Slack * work, so pass `getSlackAdapter` to post each card with the token of its * workspace. @@ -59,7 +62,8 @@ export function createPluginReviewItems(args: { plugin: string; }): PluginReviewItems { const { conversationId, getSlackAdapter } = args; - const scopedKey = (dedupeKey: string) => `${args.plugin}:${dedupeKey}`; + const scopedKey = (dedupeKey: string) => + `${PLUGIN_REVIEW_ITEM_KEY_PREFIX}${args.plugin}:${dedupeKey}`; const readAccess = async (db: JuniorDatabase) => { const access = ( await readConversationAccessFromSql(db, [conversationId]) diff --git a/packages/junior/src/chat/tools/review-items.ts b/packages/junior/src/chat/tools/review-items.ts index fb2198a271..accb48d751 100644 --- a/packages/junior/src/chat/tools/review-items.ts +++ b/packages/junior/src/chat/tools/review-items.ts @@ -9,6 +9,7 @@ import { } from "@/chat/review-items/changes"; import { canAgentChangeReviewItem, + PLUGIN_REVIEW_ITEM_KEY_PREFIX, canReadReviewItem, readReviewItemAgentScope, requireReviewItemAssignees, @@ -230,6 +231,11 @@ export function createAddReviewItemTool( outputSchema: juniorToolOutputSchema, execute: async (input, options) => { requireRequest(input.kind, Boolean(input.request)); + if (input.dedupe_key?.startsWith(PLUGIN_REVIEW_ITEM_KEY_PREFIX)) { + throw new ToolInputError( + `A \`dedupe_key\` that starts with \`${PLUGIN_REVIEW_ITEM_KEY_PREFIX}\` belongs to a plugin. Use a different key.`, + ); + } const db = getDb(); const scope = await readReviewItemAgentScope(context); // A retry of the same tool call keeps the same key, so it updates the @@ -418,6 +424,11 @@ export function createUpdateReviewItemTool( if (item.status !== "open" && !retriesClose) { throw new ToolInputError(`Review item ${item.id} is already closed.`); } + if (item.dedupeKey.startsWith(PLUGIN_REVIEW_ITEM_KEY_PREFIX)) { + throw new ToolInputError( + "A plugin added this item. Only the plugin or a person can change it.", + ); + } if (!canAgentChangeReviewItem(item, scope)) { throw new ToolInputError( "This Conversation cannot change this item. A public item changes only from a public Conversation. A private item changes only from the Conversation or Automation that added it.", diff --git a/packages/junior/tests/integration/plugin-review-items.test.ts b/packages/junior/tests/integration/plugin-review-items.test.ts index 16296cbeda..4e36ba41c6 100644 --- a/packages/junior/tests/integration/plugin-review-items.test.ts +++ b/packages/junior/tests/integration/plugin-review-items.test.ts @@ -7,6 +7,11 @@ import { getOpenReviewItemByDedupeKey, getReviewItem, } from "@/chat/review-items/store"; +import { + createAddReviewItemTool, + createUpdateReviewItemTool, +} from "@/chat/tools/review-items"; +import { createConversationWorkQueueTestAdapter } from "../fixtures/conversation-work"; import { juniorConversationParticipants, juniorIdentities } from "@/db/schema"; const TENANT = { provider: "slack", providerTenantId: "T123" }; @@ -47,7 +52,7 @@ async function person( } const NOTICE = { - body: "**Fix the thing** is approved and its checks pass.", + body: "**Fix the thing** is approved, and GitHub says that it can merge. Merge it, or say why not.", dedupeKey: "pull-request-ready:getsentry/junior#946", subject: { id: "https://github.com/getsentry/junior/pull/946", @@ -73,7 +78,7 @@ describe("plugin review items", () => { await items.upsert({ ...NOTICE, assigneeIdentityIds: [ana.identityId] }); const added = await getOpenReviewItemByDedupeKey( getDb(), - `github:${NOTICE.dedupeKey}`, + `plugin:github:${NOTICE.dedupeKey}`, TENANT, ); expect(added).toMatchObject({ @@ -96,11 +101,42 @@ describe("plugin review items", () => { expect( await getOpenReviewItemByDedupeKey( getDb(), - `github:${NOTICE.dedupeKey}`, + `plugin:github:${NOTICE.dedupeKey}`, TENANT, ), ).toMatchObject({ id: added!.id, title: "Still ready to merge" }); + // An agent in another public Conversation of the tenant cannot take over + // the plugin key, or change the plugin notice. + await conversation("slack:C1:1.000002", "public"); + const agent = { + conversationId: "slack:C1:1.000002", + resolveActorIdentity: async () => ({ + identity: { id: ana.identityId } as any, + user: ana.user, + }), + source: { kind: "slack" } as any, + }; + await expect( + createAddReviewItemTool(agent).execute!( + { + body: "Taken over.", + dedupe_key: `plugin:github:${NOTICE.dedupeKey}`, + kind: "notice", + title: "Taken over", + } as any, + {} as any, + ), + ).rejects.toThrow("belongs to a plugin"); + await expect( + createUpdateReviewItemTool(agent, () => + createConversationWorkQueueTestAdapter(), + ).execute!( + { id: added!.id, reason: "Not needed.", status: "dismissed" } as any, + {} as any, + ), + ).rejects.toThrow("Only the plugin or a person can change it"); + await items.close({ dedupeKey: NOTICE.dedupeKey, reason: "The pull request merged.", @@ -128,7 +164,7 @@ describe("plugin review items", () => { expect( await getOpenReviewItemByDedupeKey( getDb(), - `linear:${NOTICE.dedupeKey}`, + `plugin:linear:${NOTICE.dedupeKey}`, TENANT, ), ).toMatchObject({ assignees: [], status: "open" }); @@ -157,7 +193,7 @@ describe("plugin review items", () => { expect( await getOpenReviewItemByDedupeKey( getDb(), - `github:${NOTICE.dedupeKey}`, + `plugin:github:${NOTICE.dedupeKey}`, TENANT, ), ).toMatchObject({ From dc6326c95f7bec74d48944a8a546fe1266a4e0fd Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Sun, 1 Nov 2026 19:47:04 +0000 Subject: [PATCH 5/9] fix(review-desk): Run ready to merge webhooks for one pull request in 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 --- packages/junior-github/README.md | 23 ++++---- .../src/pull-request-outcomes/store.ts | 52 ++++++++++------- .../src/webhooks/pull-request-ready.ts | 44 +++++++------- .../tests/pull-request-ready.test.ts | 58 ++++++++++++++----- 4 files changed, 108 insertions(+), 69 deletions(-) diff --git a/packages/junior-github/README.md b/packages/junior-github/README.md index caf955de3f..8996fe5dbc 100644 --- a/packages/junior-github/README.md +++ b/packages/junior-github/README.md @@ -70,16 +70,19 @@ review desk through `ctx.reviewItems`. Ready to merge means that the pull request is open, is not a draft, has an approval and no open change request, and GitHub's `mergeable_state` is `clean`. `clean` covers required reviews, checks, commit statuses, and conflicts. `pull_request`, `pull_request_review`, -and `check_suite` webhooks make the plugin read these facts again from -GitHub. GitHub computes `mergeable_state` after a change. When it is still -unknown after a second read, the plugin changes nothing, and a later webhook -decides. The notice goes to the opener, in the Conversation where they asked. -It tells them to merge the pull request, or to say why not. `ready_to_merge_at` on the pull request records that the -notice was added, so repeat webhooks add no second notice. A notice that a -person closed does not come back until the pull request is ready again. The -plugin closes the notice when the pull request merges, closes, or is no longer -ready. "Changes requested" adds no notice. A notice failure is logged and does -not fail the webhook. +and `check_suite` webhooks make the plugin read these facts again from GitHub. +GitHub computes `mergeable_state` after a change. When it is still unknown +after a second read, the plugin changes nothing, and a later webhook decides. +The notice goes to the opener, in the Conversation where they asked. It tells +them to merge the pull request, or to say why not. `ready_to_merge_at` on the +pull request records that the notice was added, so repeat webhooks add no +second notice. Each webhook locks the pull request row in one transaction +while it reads GitHub and adds or closes the notice. So webhooks for one pull +request run one at a time, and a failure rolls back `ready_to_merge_at`. A +notice that a person closed does not come back until the pull request is ready +again. The plugin closes the notice when the pull request merges, closes, or +is no longer ready. "Changes requested" adds no notice. A notice failure is +logged and does not fail the webhook. Deployment events come from GitHub `deployment` and `deployment_status` webhooks: diff --git a/packages/junior-github/src/pull-request-outcomes/store.ts b/packages/junior-github/src/pull-request-outcomes/store.ts index 3f24729918..ecdde5b6ed 100644 --- a/packages/junior-github/src/pull-request-outcomes/store.ts +++ b/packages/junior-github/src/pull-request-outcomes/store.ts @@ -1,4 +1,4 @@ -import { and, eq, isNotNull, isNull, lte, ne, sql } from "drizzle-orm"; +import { and, eq, lte, ne, sql } from "drizzle-orm"; import { z } from "zod"; import type { GitHubDb } from "../db/database.js"; import { @@ -300,30 +300,40 @@ export async function recordGitHubPullRequestLinkedIssues( } /** - * Save whether a Junior-owned pull request is ready to merge. Return true - * only when this call changed it, so one caller acts on each change even when - * webhooks arrive together. + * Lock the row of a Junior pull request until the transaction ends, and read + * whether it is saved as ready to merge. So webhooks for one pull request + * decide its notice one at a time. Undefined when Junior does not own the + * pull request. */ -export async function markGitHubPullRequestReadyToMerge( +export async function lockGitHubPullRequestReadyToMerge( db: GitHubDb, - input: { - number: number; - ready: boolean; - repositoryFullName: string; - }, -): Promise { - const updated = await db - .update(juniorGitHubPullRequests) - .set({ readyToMergeAt: input.ready ? new Date() : null }) + target: { number: number; repositoryFullName: string }, +): Promise { + const [row] = await db + .select({ readyToMergeAt: juniorGitHubPullRequests.readyToMergeAt }) + .from(juniorGitHubPullRequests) .where( and( - sql`lower(${juniorGitHubPullRequests.repositoryFullName}) = ${input.repositoryFullName.toLowerCase()}`, - eq(juniorGitHubPullRequests.number, input.number), - input.ready - ? isNull(juniorGitHubPullRequests.readyToMergeAt) - : isNotNull(juniorGitHubPullRequests.readyToMergeAt), + sql`lower(${juniorGitHubPullRequests.repositoryFullName}) = ${target.repositoryFullName.toLowerCase()}`, + eq(juniorGitHubPullRequests.number, target.number), ), ) - .returning({ pullRequestId: juniorGitHubPullRequests.pullRequestId }); - return updated.length > 0; + .for("update"); + return row ? row.readyToMergeAt !== null : undefined; +} + +/** Save whether a Junior pull request is ready to merge. */ +export async function setGitHubPullRequestReadyToMerge( + db: GitHubDb, + target: { number: number; ready: boolean; repositoryFullName: string }, +): Promise { + await db + .update(juniorGitHubPullRequests) + .set({ readyToMergeAt: target.ready ? new Date() : null }) + .where( + and( + sql`lower(${juniorGitHubPullRequests.repositoryFullName}) = ${target.repositoryFullName.toLowerCase()}`, + eq(juniorGitHubPullRequests.number, target.number), + ), + ); } diff --git a/packages/junior-github/src/webhooks/pull-request-ready.ts b/packages/junior-github/src/webhooks/pull-request-ready.ts index 4be6b40f6e..6c4b00a5d0 100644 --- a/packages/junior-github/src/webhooks/pull-request-ready.ts +++ b/packages/junior-github/src/webhooks/pull-request-ready.ts @@ -14,7 +14,10 @@ import type { } from "@sentry/junior-plugin-api"; import { z } from "zod"; import type { GitHubDb } from "../db/database.js"; -import { markGitHubPullRequestReadyToMerge } from "../pull-request-outcomes/store.js"; +import { + lockGitHubPullRequestReadyToMerge, + setGitHubPullRequestReadyToMerge, +} from "../pull-request-outcomes/store.js"; import { listGitHubPullRequestOpeners } from "../pull-request-requesters/store.js"; import { checkSuitePullRequests } from "./check-suite.js"; import { @@ -97,23 +100,25 @@ async function syncOne( // Only a pull request that a person asked Junior to open has a requester // and a Conversation for the notice. if (!opener?.conversationId) return; - const state = await loadPullRequestMergeState(args.env, target); - const open = state.state === "open" && !state.draft; - // GitHub has not computed the merge state yet. A later event decides. - if (open && !state.mergeableState) return; - const ready = open && state.mergeableState === "clean" && state.approved; - // Only the call that changes the saved state acts. So repeat events add - // no second notice, and a notice that a person closed does not come back - // until the pull request is ready again. - if ( - !(await markGitHubPullRequestReadyToMerge(args.db, { ...target, ready })) - ) { - return; - } const items = args.reviewItems.forConversation(opener.conversationId); const name = `${target.repositoryFullName}#${target.number}`; const dedupeKey = `pull-request-ready:${name.toLowerCase()}`; - try { + // The row lock makes webhooks for one pull request run one at a time, and + // each reads GitHub after the one before it is done. A failure rolls back + // the saved state, so the next event tries again. + await args.db.transaction(async (tx) => { + const saved = await lockGitHubPullRequestReadyToMerge(tx, target); + if (saved === undefined) return; + const state = await loadPullRequestMergeState(args.env, target); + const open = state.state === "open" && !state.draft; + // GitHub has not computed the merge state yet. A later event decides. + if (open && !state.mergeableState) return; + const ready = open && state.mergeableState === "clean" && state.approved; + // Act only on a change. So repeat events add no second notice, and a + // notice that a person closed does not come back until the pull request + // is ready again. + if (ready === saved) return; + await setGitHubPullRequestReadyToMerge(tx, { ...target, ready }); if (ready) { await items.upsert({ assigneeIdentityIds: [opener.identityId], @@ -125,14 +130,7 @@ async function syncOne( } else { await items.close({ dedupeKey, ...closeReason(state) }); } - } catch (error) { - // Undo the saved state, so the next event tries again. - await markGitHubPullRequestReadyToMerge(args.db, { - ...target, - ready: !ready, - }); - throw error; - } + }); } /** diff --git a/packages/junior-github/tests/pull-request-ready.test.ts b/packages/junior-github/tests/pull-request-ready.test.ts index 10de92e80f..dde2cd8042 100644 --- a/packages/junior-github/tests/pull-request-ready.test.ts +++ b/packages/junior-github/tests/pull-request-ready.test.ts @@ -102,6 +102,8 @@ interface GitHubState { draft: boolean; /** Each pull request read takes the next value. The last one repeats. */ mergeableStates: Array; + /** When set, the next pull request read waits for it. */ + holdNextPullRead?: Promise; merged: boolean; reviews: Array<{ state: string; user: { login: string } }>; state: "closed" | "open"; @@ -117,21 +119,27 @@ function mockGitHub(github: GitHubState, requests: string[]) { token: "installation-token", }), ), - http.get("https://api.github.com/repos/getsentry/junior/pulls/946", () => { - requests.push("pull"); - const mergeableState = - github.mergeableStates.length > 1 - ? github.mergeableStates.shift() - : github.mergeableStates[0]; - return HttpResponse.json({ - draft: github.draft, - html_url: PULL_URL, - mergeable_state: mergeableState, - merged: github.merged, - state: github.state, - title: "Add the review desk notice", - }); - }), + http.get( + "https://api.github.com/repos/getsentry/junior/pulls/946", + async () => { + requests.push("pull"); + const mergeableState = + github.mergeableStates.length > 1 + ? github.mergeableStates.shift() + : github.mergeableStates[0]; + const hold = github.holdNextPullRead; + github.holdNextPullRead = undefined; + await hold; + return HttpResponse.json({ + draft: github.draft, + html_url: PULL_URL, + mergeable_state: mergeableState, + merged: github.merged, + state: github.state, + title: "Add the review desk notice", + }); + }, + ), // The merge webhook also classifies the commits. http.get( "https://api.github.com/repos/getsentry/junior/pulls/946/commits", @@ -323,6 +331,24 @@ describe("ready to merge notice", () => { kind: "close", }); + // Two webhooks run at the same time. The first reads "clean", but its + // read is slow. The second reads "blocked". The second waits for the + // first, so the notice ends closed, as GitHub's latest state says. + let release!: () => void; + github.holdNextPullRead = new Promise((resolve) => { + release = resolve; + }); + github.mergeableStates = ["clean", "blocked"]; + const slow = webhook.handler(checkSuiteCompleted()); + const fast = webhook.handler(checkSuiteCompleted()); + await new Promise((resolve) => setTimeout(resolve, 100)); + release(); + await Promise.all([slow, fast]); + expect(calls.slice(-2).map((call) => call.kind)).toEqual([ + "upsert", + "close", + ]); + github.mergeableStates = ["clean"]; await webhook.handler(checkSuiteCompleted()); expect(calls.at(-1)).toEqual(notice); @@ -349,6 +375,8 @@ describe("ready to merge notice", () => { "close", "upsert", "close", + "upsert", + "close", ]); expect(errors).toHaveLength(1); } finally { From 20af605ad9999d152aab577d7487a2b9d59aa8ce Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Sun, 1 Nov 2026 20:00:29 +0000 Subject: [PATCH 6/9] fix(review-desk): Check pull request readiness without a long transaction 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 --- packages/junior-github/README.md | 17 ++-- .../migrations/0010_sleepy_slayback.sql | 5 +- .../migrations/meta/0010_snapshot.json | 21 ++++- .../migrations/meta/_journal.json | 2 +- packages/junior-github/src/db/schema.ts | 11 +++ .../src/pull-request-outcomes/store.ts | 90 ++++++++++++++----- .../src/webhooks/pull-request-ready.ts | 79 ++++++++++------ 7 files changed, 167 insertions(+), 58 deletions(-) diff --git a/packages/junior-github/README.md b/packages/junior-github/README.md index 8996fe5dbc..237e9affa1 100644 --- a/packages/junior-github/README.md +++ b/packages/junior-github/README.md @@ -76,13 +76,16 @@ after a second read, the plugin changes nothing, and a later webhook decides. The notice goes to the opener, in the Conversation where they asked. It tells them to merge the pull request, or to say why not. `ready_to_merge_at` on the pull request records that the notice was added, so repeat webhooks add no -second notice. Each webhook locks the pull request row in one transaction -while it reads GitHub and adds or closes the notice. So webhooks for one pull -request run one at a time, and a failure rolls back `ready_to_merge_at`. A -notice that a person closed does not come back until the pull request is ready -again. The plugin closes the notice when the pull request merges, closes, or -is no longer ready. "Changes requested" adds no notice. A notice failure is -logged and does not fail the webhook. +second notice. 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 then reads GitHub again, until no new request came in. +`ready_sync_id` names that check, and a check older than one minute no longer +blocks others. No database connection stays open during the GitHub calls. +`ready_to_merge_at` changes only after the notice changes, so after a failure +the next webhook tries again. A notice that a person closed does not come back +until the pull request is ready again. The plugin closes the notice when the +pull request merges, closes, or is no longer ready. "Changes requested" adds +no notice. A notice failure is logged and does not fail the webhook. Deployment events come from GitHub `deployment` and `deployment_status` webhooks: diff --git a/packages/junior-github/migrations/0010_sleepy_slayback.sql b/packages/junior-github/migrations/0010_sleepy_slayback.sql index f4baf7f261..1d8b1a29aa 100644 --- a/packages/junior-github/migrations/0010_sleepy_slayback.sql +++ b/packages/junior-github/migrations/0010_sleepy_slayback.sql @@ -1,2 +1,5 @@ ALTER TABLE "junior_github_pull_request_requesters" ADD COLUMN "conversation_id" text;--> statement-breakpoint -ALTER TABLE "junior_github_pull_requests" ADD COLUMN "ready_to_merge_at" timestamp with time zone; \ No newline at end of file +ALTER TABLE "junior_github_pull_requests" ADD COLUMN "ready_to_merge_at" timestamp with time zone;--> statement-breakpoint +ALTER TABLE "junior_github_pull_requests" ADD COLUMN "ready_sync_requests" integer DEFAULT 0 NOT NULL;--> statement-breakpoint +ALTER TABLE "junior_github_pull_requests" ADD COLUMN "ready_sync_id" integer;--> statement-breakpoint +ALTER TABLE "junior_github_pull_requests" ADD COLUMN "ready_sync_at" timestamp with time zone; \ No newline at end of file diff --git a/packages/junior-github/migrations/meta/0010_snapshot.json b/packages/junior-github/migrations/meta/0010_snapshot.json index e918f6aa00..9d25b25191 100644 --- a/packages/junior-github/migrations/meta/0010_snapshot.json +++ b/packages/junior-github/migrations/meta/0010_snapshot.json @@ -1,5 +1,5 @@ { - "id": "6b48609b-f71b-4667-8dbb-da286531046e", + "id": "3156d5c8-917d-416b-a19b-f58e7ee76acc", "prevId": "6b12b332-4224-4e69-9ab7-51df6a4dd91c", "version": "7", "dialect": "postgresql", @@ -305,6 +305,25 @@ "type": "timestamp with time zone", "primaryKey": false, "notNull": false + }, + "ready_sync_requests": { + "name": "ready_sync_requests", + "type": "integer", + "primaryKey": false, + "notNull": true, + "default": 0 + }, + "ready_sync_id": { + "name": "ready_sync_id", + "type": "integer", + "primaryKey": false, + "notNull": false + }, + "ready_sync_at": { + "name": "ready_sync_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": false } }, "indexes": { diff --git a/packages/junior-github/migrations/meta/_journal.json b/packages/junior-github/migrations/meta/_journal.json index 1be89443f6..2b5dbb5d11 100644 --- a/packages/junior-github/migrations/meta/_journal.json +++ b/packages/junior-github/migrations/meta/_journal.json @@ -75,7 +75,7 @@ { "idx": 10, "version": "7", - "when": 1793468293713, + "when": 1793563055612, "tag": "0010_sleepy_slayback", "breakpoints": true } diff --git a/packages/junior-github/src/db/schema.ts b/packages/junior-github/src/db/schema.ts index 5da9a44649..eeef7878a4 100644 --- a/packages/junior-github/src/db/schema.ts +++ b/packages/junior-github/src/db/schema.ts @@ -94,6 +94,17 @@ export const juniorGitHubPullRequests = pgTable( * review desk notice. Null when it is not ready. */ readyToMergeAt: timestamp("ready_to_merge_at", { withTimezone: true }), + /** + * Webhooks that asked to check whether the pull request is ready to + * merge. The check runs again when this changes during a check. + */ + readySyncRequests: integer("ready_sync_requests").notNull().default(0), + /** + * The request that runs the current check. Null when no check runs. + */ + readySyncId: integer("ready_sync_id"), + /** When the current check started. */ + readySyncAt: timestamp("ready_sync_at", { withTimezone: true }), }, (table) => [ index("junior_github_pull_requests_opened_at_idx").on(table.openedAt), diff --git a/packages/junior-github/src/pull-request-outcomes/store.ts b/packages/junior-github/src/pull-request-outcomes/store.ts index ecdde5b6ed..d532f82087 100644 --- a/packages/junior-github/src/pull-request-outcomes/store.ts +++ b/packages/junior-github/src/pull-request-outcomes/store.ts @@ -299,27 +299,51 @@ export async function recordGitHubPullRequestLinkedIssues( return inserted.some((rows) => rows.length > 0); } +function pullRequestRow(target: { + number: number; + repositoryFullName: string; +}) { + return and( + sql`lower(${juniorGitHubPullRequests.repositoryFullName}) = ${target.repositoryFullName.toLowerCase()}`, + eq(juniorGitHubPullRequests.number, target.number), + ); +} + /** - * Lock the row of a Junior pull request until the transaction ends, and read - * whether it is saved as ready to merge. So webhooks for one pull request - * decide its notice one at a time. Undefined when Junior does not own the - * pull request. + * Ask to check whether a Junior pull request is ready to merge, and claim the + * check when no other webhook runs one. A claim older than `staleMs` is free. + * Return the claim, or undefined when another webhook runs the check or + * Junior does not own the pull request. The webhook that runs the check sees + * the new request in `releaseGitHubPullRequestReadySync`. */ -export async function lockGitHubPullRequestReadyToMerge( +export async function claimGitHubPullRequestReadySync( db: GitHubDb, target: { number: number; repositoryFullName: string }, -): Promise { + options: { now: Date; staleMs: number }, +): Promise<{ id: number; ready: boolean; requests: number } | undefined> { + const t = juniorGitHubPullRequests; + const staleBefore = new Date(options.now.getTime() - options.staleMs); + const free = sql`(${t.readySyncId} is null or ${t.readySyncAt} < ${staleBefore.toISOString()}::timestamptz)`; const [row] = await db - .select({ readyToMergeAt: juniorGitHubPullRequests.readyToMergeAt }) - .from(juniorGitHubPullRequests) - .where( - and( - sql`lower(${juniorGitHubPullRequests.repositoryFullName}) = ${target.repositoryFullName.toLowerCase()}`, - eq(juniorGitHubPullRequests.number, target.number), - ), - ) - .for("update"); - return row ? row.readyToMergeAt !== null : undefined; + .update(t) + .set({ + readySyncRequests: sql`${t.readySyncRequests} + 1`, + // Each request has its own number, so it is the id of the claim. + readySyncId: sql`case when ${free} then ${t.readySyncRequests} + 1 else ${t.readySyncId} end`, + readySyncAt: sql`case when ${free} then ${options.now.toISOString()}::timestamptz else ${t.readySyncAt} end`, + }) + .where(pullRequestRow(target)) + .returning({ + id: t.readySyncId, + readyToMergeAt: t.readyToMergeAt, + requests: t.readySyncRequests, + }); + if (!row || row.id !== row.requests) return undefined; + return { + id: row.id, + ready: row.readyToMergeAt !== null, + requests: row.requests, + }; } /** Save whether a Junior pull request is ready to merge. */ @@ -330,10 +354,34 @@ export async function setGitHubPullRequestReadyToMerge( await db .update(juniorGitHubPullRequests) .set({ readyToMergeAt: target.ready ? new Date() : null }) + .where(pullRequestRow(target)); +} + +/** + * End a claimed check. When a webhook asked again during the check, keep the + * claim and return the new request count, so the caller checks once more. + * Return undefined when the claim ended or another webhook took it. + */ +export async function releaseGitHubPullRequestReadySync( + db: GitHubDb, + target: { number: number; repositoryFullName: string }, + claim: { id: number; requests?: number }, +): Promise { + const t = juniorGitHubPullRequests; + const claimed = and(pullRequestRow(target), eq(t.readySyncId, claim.id)); + const released = await db + .update(t) + .set({ readySyncAt: null, readySyncId: null }) .where( - and( - sql`lower(${juniorGitHubPullRequests.repositoryFullName}) = ${target.repositoryFullName.toLowerCase()}`, - eq(juniorGitHubPullRequests.number, target.number), - ), - ); + claim.requests === undefined + ? claimed + : and(claimed, eq(t.readySyncRequests, claim.requests)), + ) + .returning({ number: t.number }); + if (released.length > 0) return undefined; + const [row] = await db + .select({ requests: t.readySyncRequests }) + .from(t) + .where(claimed); + return row?.requests; } diff --git a/packages/junior-github/src/webhooks/pull-request-ready.ts b/packages/junior-github/src/webhooks/pull-request-ready.ts index 6c4b00a5d0..9bbdb6b291 100644 --- a/packages/junior-github/src/webhooks/pull-request-ready.ts +++ b/packages/junior-github/src/webhooks/pull-request-ready.ts @@ -15,7 +15,8 @@ import type { import { z } from "zod"; import type { GitHubDb } from "../db/database.js"; import { - lockGitHubPullRequestReadyToMerge, + claimGitHubPullRequestReadySync, + releaseGitHubPullRequestReadySync, setGitHubPullRequestReadyToMerge, } from "../pull-request-outcomes/store.js"; import { listGitHubPullRequestOpeners } from "../pull-request-requesters/store.js"; @@ -27,6 +28,12 @@ import { } from "./installation-requests.js"; import type { GitHubPullRequestTarget } from "./pull-request-people.js"; +/** + * A check normally takes a few seconds. After this time, a check that + * stopped without its release no longer blocks new checks. + */ +const READY_SYNC_STALE_MS = 60_000; + /** Webhook actions that can change whether a pull request is ready. */ const READY_ACTIONS: Record> = { pull_request: new Set([ @@ -103,34 +110,52 @@ async function syncOne( const items = args.reviewItems.forConversation(opener.conversationId); const name = `${target.repositoryFullName}#${target.number}`; const dedupeKey = `pull-request-ready:${name.toLowerCase()}`; - // The row lock makes webhooks for one pull request run one at a time, and - // each reads GitHub after the one before it is done. A failure rolls back - // the saved state, so the next event tries again. - await args.db.transaction(async (tx) => { - const saved = await lockGitHubPullRequestReadyToMerge(tx, target); - if (saved === undefined) return; - const state = await loadPullRequestMergeState(args.env, target); - const open = state.state === "open" && !state.draft; - // GitHub has not computed the merge state yet. A later event decides. - if (open && !state.mergeableState) return; - const ready = open && state.mergeableState === "clean" && state.approved; - // Act only on a change. So repeat events add no second notice, and a - // notice that a person closed does not come back until the pull request - // is ready again. - if (ready === saved) return; - await setGitHubPullRequestReadyToMerge(tx, { ...target, ready }); - if (ready) { - await items.upsert({ - assigneeIdentityIds: [opener.identityId], - body: `**${state.title}** is approved, and GitHub says that it can merge. Merge it, or say why not: ${state.url}`, - dedupeKey, - subject: { id: state.url, title: name, url: state.url }, - title: `Ready to merge: ${name}`, + // One webhook at a time checks a pull request. A webhook that arrives + // during a check asks again, and the running webhook checks once more with + // a new GitHub read. No connection or lock stays open during the calls. + const claim = await claimGitHubPullRequestReadySync(args.db, target, { + now: new Date(), + staleMs: READY_SYNC_STALE_MS, + }); + if (!claim) return; + const { id } = claim; + let saved = claim.ready; + let requests: number | undefined = claim.requests; + try { + while (requests !== undefined) { + const state = await loadPullRequestMergeState(args.env, target); + const open = state.state === "open" && !state.draft; + const ready = open && state.mergeableState === "clean" && state.approved; + // Act only on a change. So repeat events add no second notice, and a + // notice that a person closed does not come back until the pull + // request is ready again. When GitHub has not computed the merge state + // yet, nothing changes, and a later event decides. + if ((!open || state.mergeableState) && ready !== saved) { + if (ready) { + await items.upsert({ + assigneeIdentityIds: [opener.identityId], + body: `**${state.title}** is approved, and GitHub says that it can merge. Merge it, or say why not: ${state.url}`, + dedupeKey, + subject: { id: state.url, title: name, url: state.url }, + title: `Ready to merge: ${name}`, + }); + } else { + await items.close({ dedupeKey, ...closeReason(state) }); + } + // Save after the notice changes, so a failure leaves the old state + // and the next event tries again. + await setGitHubPullRequestReadyToMerge(args.db, { ...target, ready }); + saved = ready; + } + requests = await releaseGitHubPullRequestReadySync(args.db, target, { + id, + requests, }); - } else { - await items.close({ dedupeKey, ...closeReason(state) }); } - }); + } catch (error) { + await releaseGitHubPullRequestReadySync(args.db, target, { id }); + throw error; + } } /** From f76392faba546729ba96afd1db40b48be8f183c3 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Sun, 1 Nov 2026 20:11:01 +0000 Subject: [PATCH 7/9] fix(review-desk): Keep webhooks that arrive during a failed ready check 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 --- .../src/pull-request-outcomes/store.ts | 8 ++----- .../src/webhooks/pull-request-ready.ts | 21 +++++++++++-------- .../tests/pull-request-ready.test.ts | 15 +++++++++---- 3 files changed, 25 insertions(+), 19 deletions(-) diff --git a/packages/junior-github/src/pull-request-outcomes/store.ts b/packages/junior-github/src/pull-request-outcomes/store.ts index d532f82087..30ba28a182 100644 --- a/packages/junior-github/src/pull-request-outcomes/store.ts +++ b/packages/junior-github/src/pull-request-outcomes/store.ts @@ -365,18 +365,14 @@ export async function setGitHubPullRequestReadyToMerge( export async function releaseGitHubPullRequestReadySync( db: GitHubDb, target: { number: number; repositoryFullName: string }, - claim: { id: number; requests?: number }, + claim: { id: number; requests: number }, ): Promise { const t = juniorGitHubPullRequests; const claimed = and(pullRequestRow(target), eq(t.readySyncId, claim.id)); const released = await db .update(t) .set({ readySyncAt: null, readySyncId: null }) - .where( - claim.requests === undefined - ? claimed - : and(claimed, eq(t.readySyncRequests, claim.requests)), - ) + .where(and(claimed, eq(t.readySyncRequests, claim.requests))) .returning({ number: t.number }); if (released.length > 0) return undefined; const [row] = await db diff --git a/packages/junior-github/src/webhooks/pull-request-ready.ts b/packages/junior-github/src/webhooks/pull-request-ready.ts index 9bbdb6b291..c9e3586d40 100644 --- a/packages/junior-github/src/webhooks/pull-request-ready.ts +++ b/packages/junior-github/src/webhooks/pull-request-ready.ts @@ -121,8 +121,11 @@ async function syncOne( const { id } = claim; let saved = claim.ready; let requests: number | undefined = claim.requests; - try { - while (requests !== undefined) { + // A failed check still checks the requests that came in during it. The + // first failure is thrown after the last check. + let failure: unknown; + while (requests !== undefined) { + try { const state = await loadPullRequestMergeState(args.env, target); const open = state.state === "open" && !state.draft; const ready = open && state.mergeableState === "clean" && state.approved; @@ -147,15 +150,15 @@ async function syncOne( await setGitHubPullRequestReadyToMerge(args.db, { ...target, ready }); saved = ready; } - requests = await releaseGitHubPullRequestReadySync(args.db, target, { - id, - requests, - }); + } catch (error) { + failure ??= error; } - } catch (error) { - await releaseGitHubPullRequestReadySync(args.db, target, { id }); - throw error; + requests = await releaseGitHubPullRequestReadySync(args.db, target, { + id, + requests, + }); } + if (failure !== undefined) throw failure; } /** diff --git a/packages/junior-github/tests/pull-request-ready.test.ts b/packages/junior-github/tests/pull-request-ready.test.ts index dde2cd8042..f0f05ef2bc 100644 --- a/packages/junior-github/tests/pull-request-ready.test.ts +++ b/packages/junior-github/tests/pull-request-ready.test.ts @@ -355,12 +355,19 @@ describe("ready to merge notice", () => { github.merged = true; github.state = "closed"; - // A failed close is tried again on the next event. + // The close fails once. A webhook that arrives during the failed + // check is not lost: the check runs again and closes the notice. failures.close = 1; - await webhook.handler(pullRequestAction("closed", true)); - expect(calls.at(-1)).toEqual(notice); + let releaseMerge!: () => void; + github.holdNextPullRead = new Promise((resolve) => { + releaseMerge = resolve; + }); + const failing = webhook.handler(pullRequestAction("closed", true)); + const queued = webhook.handler(pullRequestAction("closed", true)); + await new Promise((resolve) => setTimeout(resolve, 100)); + releaseMerge(); + await Promise.all([failing, queued]); expect(errors).toEqual(["GitHub pull request ready notice failed"]); - await webhook.handler(pullRequestAction("closed", true)); expect(calls.at(-1)).toEqual({ conversationId: "slack:C123:1712345.0001", input: { From ebd0eacb623a81cfc7cbc2dc58ba8f2b58eaccd7 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Sun, 1 Nov 2026 21:50:50 +0000 Subject: [PATCH 8/9] ref(review-desk): Show plugin review items through app setup 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 --- packages/junior/src/app.ts | 3 ++- .../junior/src/chat/plugins/agent-hooks.ts | 8 +++---- .../junior/src/chat/review-items/README.md | 10 +++++--- .../junior/src/chat/review-items/plugin.ts | 24 +++++++------------ .../junior/src/chat/tools/review-items.ts | 2 +- .../integration/plugin-review-items.test.ts | 24 +++++++++++++++---- 6 files changed, 43 insertions(+), 28 deletions(-) diff --git a/packages/junior/src/app.ts b/packages/junior/src/app.ts index 563afc2e5e..eaaf817a9b 100644 --- a/packages/junior/src/app.ts +++ b/packages/junior/src/app.ts @@ -113,6 +113,7 @@ import { createProductionSlackWebhookServices, getProductionSlackAdapter, } from "@/chat/app/production"; +import { createSlackReviewItemShow } from "@/chat/slack/review-item-card-sync"; import type { ConversationWorkCallbackOptions } from "@/chat/app/conversation-work"; import { createAgentRunner } from "@/chat/runtime/agent-runner"; import { getAttachmentStorage } from "@/chat/attachments/storage"; @@ -569,7 +570,7 @@ export async function createApp( } pluginRoutes = getPluginRoutes({ events, - getSlackAdapter: getProductionSlackAdapter, + showReviewItem: createSlackReviewItemShow(getProductionSlackAdapter), }); if (dashboard && !dashboard.disabled) { pluginApiRoutes = getPluginApiRoutes(); diff --git a/packages/junior/src/chat/plugins/agent-hooks.ts b/packages/junior/src/chat/plugins/agent-hooks.ts index e2df111fd3..12a57146e0 100644 --- a/packages/junior/src/chat/plugins/agent-hooks.ts +++ b/packages/junior/src/chat/plugins/agent-hooks.ts @@ -1,4 +1,3 @@ -import type { SlackAdapter } from "@chat-adapter/slack"; import { createPluginCache } from "@/chat/plugins/cache"; import type { OwnedObjectAnnotation } from "@sentry/junior-plugin-api"; import { @@ -83,6 +82,7 @@ import { listWorkspaceNamesByRepository, } from "@/chat/workspaces/store"; import { createCodeChangePublisher } from "@/chat/code/publisher"; +import type { ShowReviewItem } from "@/chat/review-items/changes"; import { createPluginReviewItems } from "@/chat/review-items/plugin"; import { coreTaskRegistrations } from "@/chat/briefs/registration"; @@ -939,8 +939,8 @@ function requirePublishedEvent( /** Collect route handlers exposed by plugins for app-level mounting. */ export function getPluginRoutes(options: { - /** Slack adapter for review item cards, which routes post outside Slack work. */ - getSlackAdapter?: () => SlackAdapter; + /** Shows a Review item that a plugin route changed. App setup supplies it. */ + showReviewItem?: ShowReviewItem; events: { hasMatch?(event: Event): Promise; neededMatchKeys?(input: { @@ -977,8 +977,8 @@ export function getPluginRoutes(options: { forConversation: (conversationId) => createPluginReviewItems({ conversationId, - getSlackAdapter: options.getSlackAdapter, plugin: pluginName, + show: options.showReviewItem ?? (async () => {}), }), }, users: { readIdentity: readIdentityById }, diff --git a/packages/junior/src/chat/review-items/README.md b/packages/junior/src/chat/review-items/README.md index 1443d6d3f9..b0abad2b73 100644 --- a/packages/junior/src/chat/review-items/README.md +++ b/packages/junior/src/chat/review-items/README.md @@ -114,9 +114,12 @@ decision. Core owns the rules below. - A repeat with the same key updates the open item. - A plugin notice stays open when its Conversation starts a new Turn. - A plugin notice closes quietly. A plugin close sends no message to the - linked Conversation, so the close does not start a Turn. It only syncs the - Slack cards. The provider event that closed the item can reach the + linked Conversation, so the close does not start a Turn. It only shows the + new state through the `show` function that app setup gives + `getPluginRoutes`. The provider event that closed the item can reach the Conversation through a Watch. +- A plugin notice never counts as an item that needs a person + (`reviewItemNeedsUser`), because it is a notice. For example, the GitHub plugin adds a notice when a Junior pull request is ready to merge. See `packages/junior-github/README.md`. @@ -185,7 +188,8 @@ so an Event that was sent does not go again. Every change goes through one path: `deliverReviewItemChange` in `changes.ts`. The agent tools, the person actions, and a reply in the -thread call it after they save the change. +thread call it after they save the change. A plugin change sends no +messages, so it calls only `show` (see "Plugins"). - First, every surface shows the new state. App setup gives the `show` function. Today it is the Slack card sync. Review item code does not import diff --git a/packages/junior/src/chat/review-items/plugin.ts b/packages/junior/src/chat/review-items/plugin.ts index 39a0df2922..a2a87d9081 100644 --- a/packages/junior/src/chat/review-items/plugin.ts +++ b/packages/junior/src/chat/review-items/plugin.ts @@ -1,4 +1,3 @@ -import type { SlackAdapter } from "@chat-adapter/slack"; import type { PluginReviewItems, PluginReviewNotice, @@ -7,8 +6,8 @@ import { readConversationAccessFromSql } from "@/api/conversations/access"; import { PLUGIN_REVIEW_ITEM_KEY_PREFIX } from "@/chat/review-items/access"; import { getDb } from "@/chat/db"; import { readIdentityByIdFromSql } from "@/chat/plugins/viewer"; -import { syncReviewItemSlackCards } from "@/chat/slack/review-item-card-sync"; import type { JuniorDatabase } from "@/db/db"; +import type { ShowReviewItem } from "./changes"; import { closeReviewItem, getOpenReviewItemByDedupeKey, @@ -50,18 +49,16 @@ async function assigneeUserIds( /** * Connect a plugin to the review desk for one Conversation. Dedupe keys are * `plugin::`, so a plugin changes only its own items, and agents - * cannot change them. A close sends no message to the Conversation. It only - * syncs the Slack cards. An item is - * private when the Conversation is not public. A webhook runs outside Slack - * work, so pass `getSlackAdapter` to post each card with the token of its - * workspace. + * cannot change them. A change sends no message to the Conversation. It + * only shows the new state through `show`. An item is private when the + * Conversation is not public. */ export function createPluginReviewItems(args: { conversationId: string; - getSlackAdapter?: () => SlackAdapter; plugin: string; + show: ShowReviewItem; }): PluginReviewItems { - const { conversationId, getSlackAdapter } = args; + const { conversationId, show } = args; const scopedKey = (dedupeKey: string) => `${PLUGIN_REVIEW_ITEM_KEY_PREFIX}${args.plugin}:${dedupeKey}`; const readAccess = async (db: JuniorDatabase) => { @@ -115,10 +112,7 @@ export function createPluginReviewItems(args: { id: existing.id, nowMs: Date.now(), }); - await syncReviewItemSlackCards({ - getSlackAdapter, - itemId: existing.id, - }); + await show(existing.id); return; } const item = await insertReviewItem(db, { @@ -130,7 +124,7 @@ export function createPluginReviewItems(args: { }); // Undefined means a concurrent upsert added the item first. if (item) { - await syncReviewItemSlackCards({ getSlackAdapter, itemId: item.id }); + await show(item.id); } }, async close(input) { @@ -149,7 +143,7 @@ export function createPluginReviewItems(args: { status: input.status, }); if (closed) { - await syncReviewItemSlackCards({ getSlackAdapter, itemId: item.id }); + await show(item.id); } }, }; diff --git a/packages/junior/src/chat/tools/review-items.ts b/packages/junior/src/chat/tools/review-items.ts index accb48d751..23b8c90d21 100644 --- a/packages/junior/src/chat/tools/review-items.ts +++ b/packages/junior/src/chat/tools/review-items.ts @@ -371,7 +371,7 @@ export function createUpdateReviewItemTool( ) { return zodTool({ description: - "Change an open review desk item, or close it. Set `status` to `done` or `dismissed` with a `reason`, for example when its pull request merged. An item also closes when a person answers, dismisses, or replies in its thread, when its Conversation is purged, or when its Automation is deleted. You cannot submit the answer to an action item: only an assignee can.", + "Change an open review desk item, or close it. Set `status` to `done` or `dismissed` with a `reason`, for example when its pull request merged. An item also closes when a person answers, dismisses, or replies in its thread, when its Conversation is purged, when its Automation is deleted, or when the plugin that added it closes it. You cannot submit the answer to an action item: only an assignee can.", exposure: "deferred", source: REVIEW_DESK_TOOL_SOURCE, annotations: { diff --git a/packages/junior/tests/integration/plugin-review-items.test.ts b/packages/junior/tests/integration/plugin-review-items.test.ts index 4e36ba41c6..cb2a2ed7ae 100644 --- a/packages/junior/tests/integration/plugin-review-items.test.ts +++ b/packages/junior/tests/integration/plugin-review-items.test.ts @@ -11,11 +11,22 @@ import { createAddReviewItemTool, createUpdateReviewItemTool, } from "@/chat/tools/review-items"; +import { createSlackReviewItemToolServices } from "@/chat/slack/review-item-tools"; import { createConversationWorkQueueTestAdapter } from "../fixtures/conversation-work"; import { juniorConversationParticipants, juniorIdentities } from "@/db/schema"; const TENANT = { provider: "slack", providerTenantId: "T123" }; +const services = createSlackReviewItemToolServices( + createConversationWorkQueueTestAdapter, +); + +/** Item ids that plugin changes showed, in order. */ +let shown: string[] = []; +const show = async (itemId: string) => { + shown.push(itemId); +}; + async function conversation( conversationId: string, visibility: "public" | "private", @@ -64,6 +75,7 @@ const NOTICE = { describe("plugin review items", () => { afterEach(async () => { + shown = []; await closeDb(); }); @@ -73,6 +85,7 @@ describe("plugin review items", () => { const items = createPluginReviewItems({ conversationId: "slack:C1:1.000001", plugin: "github", + show, }); await items.upsert({ ...NOTICE, assigneeIdentityIds: [ana.identityId] }); @@ -118,7 +131,7 @@ describe("plugin review items", () => { source: { kind: "slack" } as any, }; await expect( - createAddReviewItemTool(agent).execute!( + createAddReviewItemTool(agent, services).execute!( { body: "Taken over.", dedupe_key: `plugin:github:${NOTICE.dedupeKey}`, @@ -129,9 +142,7 @@ describe("plugin review items", () => { ), ).rejects.toThrow("belongs to a plugin"); await expect( - createUpdateReviewItemTool(agent, () => - createConversationWorkQueueTestAdapter(), - ).execute!( + createUpdateReviewItemTool(agent, services).execute!( { id: added!.id, reason: "Not needed.", status: "dismissed" } as any, {} as any, ), @@ -155,11 +166,15 @@ describe("plugin review items", () => { expect(await getReviewItem(getDb(), added!.id)).toMatchObject({ closeReason: "The pull request merged.", }); + // The add, the update, and the close each show the item. The second + // close changes nothing, so it shows nothing. + expect(shown).toEqual([added!.id, added!.id, added!.id]); // The key is scoped to the plugin, so another plugin has its own item. await createPluginReviewItems({ conversationId: "slack:C1:1.000001", plugin: "linear", + show, }).upsert({ ...NOTICE, assigneeIdentityIds: [] }); expect( await getOpenReviewItemByDedupeKey( @@ -184,6 +199,7 @@ describe("plugin review items", () => { const items = createPluginReviewItems({ conversationId: "slack:D1:1.000001", plugin: "github", + show, }); await expect( From cc3674962ab836a6d13a9510ae0dc8ca74af5f43 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Sun, 1 Nov 2026 22:00:32 +0000 Subject: [PATCH 9/9] fix(review-desk): Read every page of reviews for the ready check 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 --- .../src/webhooks/installation-requests.ts | 13 ++++++++++--- .../junior-github/tests/pull-request-ready.test.ts | 13 ++++++++++--- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/packages/junior-github/src/webhooks/installation-requests.ts b/packages/junior-github/src/webhooks/installation-requests.ts index c1774ab96e..473f1d4429 100644 --- a/packages/junior-github/src/webhooks/installation-requests.ts +++ b/packages/junior-github/src/webhooks/installation-requests.ts @@ -209,9 +209,16 @@ export async function loadPullRequestMergeState( if (!state.mergeableState) state = await readPull(); if (state.mergeableState !== "clean") return state; - const reviews = reviewsSchema.parse(await read("/reviews?per_page=100")); - // A later page can hold a newer change request, so a full page does not pass. - if (reviews.length >= 100) return state; + // A later page can hold a newer review, so read every page. GitHub lists + // reviews oldest first. + const reviews: z.infer = []; + for (let page = 1; ; page++) { + const batch = reviewsSchema.parse( + await read(`/reviews?per_page=100&page=${page}`), + ); + reviews.push(...batch); + if (batch.length < 100) break; + } const latestReviews = new Map(); for (const review of reviews) { if ( diff --git a/packages/junior-github/tests/pull-request-ready.test.ts b/packages/junior-github/tests/pull-request-ready.test.ts index f0f05ef2bc..4f1aba01d3 100644 --- a/packages/junior-github/tests/pull-request-ready.test.ts +++ b/packages/junior-github/tests/pull-request-ready.test.ts @@ -147,9 +147,12 @@ function mockGitHub(github: GitHubState, requests: string[]) { ), http.get( "https://api.github.com/repos/getsentry/junior/pulls/946/reviews", - () => { + ({ request }) => { requests.push("reviews"); - return HttpResponse.json(github.reviews); + const page = Number(new URL(request.url).searchParams.get("page")); + return HttpResponse.json( + github.reviews.slice((page - 1) * 100, page * 100), + ); }, ), ); @@ -282,9 +285,13 @@ describe("ready to merge notice", () => { expect(calls).toEqual([]); expect(requests).toEqual(["pull", "reviews"]); + // The approval is on the second page of reviews. github.reviews = [ { state: "CHANGES_REQUESTED", user: { login: "reviewer" } }, - { state: "COMMENTED", user: { login: "reviewer" } }, + ...Array.from({ length: 100 }, () => ({ + state: "COMMENTED", + user: { login: "reviewer" }, + })), { state: "APPROVED", user: { login: "Reviewer" } }, ]; await webhook.handler(reviewSubmitted("approved"));