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..237e9affa1 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,28 @@ 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 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. 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 new file mode 100644 index 0000000000..1d8b1a29aa --- /dev/null +++ b/packages/junior-github/migrations/0010_sleepy_slayback.sql @@ -0,0 +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;--> 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 new file mode 100644 index 0000000000..9d25b25191 --- /dev/null +++ b/packages/junior-github/migrations/meta/0010_snapshot.json @@ -0,0 +1,427 @@ +{ + "id": "3156d5c8-917d-416b-a19b-f58e7ee76acc", + "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 + }, + "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": { + "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..2b5dbb5d11 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": 1793563055612, + "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..eeef7878a4 100644 --- a/packages/junior-github/src/db/schema.ts +++ b/packages/junior-github/src/db/schema.ts @@ -89,6 +89,22 @@ 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 }), + /** + * 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), @@ -142,6 +158,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..30ba28a182 100644 --- a/packages/junior-github/src/pull-request-outcomes/store.ts +++ b/packages/junior-github/src/pull-request-outcomes/store.ts @@ -298,3 +298,86 @@ 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), + ); +} + +/** + * 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 claimGitHubPullRequestReadySync( + db: GitHubDb, + target: { number: number; repositoryFullName: string }, + 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 + .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. */ +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(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(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/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..473f1d4429 100644 --- a/packages/junior-github/src/webhooks/installation-requests.ts +++ b/packages/junior-github/src/webhooks/installation-requests.ts @@ -127,3 +127,111 @@ export async function loadRepositoryRole( }, ); } + +const pullRequestSchema = z.object({ + draft: z.boolean().optional(), + html_url: z.string().url(), + mergeable_state: z.string().nullable().optional(), + 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(), + }), +); + +/** 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. Read + * only when GitHub says the pull request can merge cleanly. + */ + approved: 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; +} + +/** + * Read whether a pull request is ready to merge, with a read-only token. + * 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, + pullRequest: { number: number; repositoryFullName: string }, +): Promise { + const { owner, name } = splitRepository( + pullRequest.repositoryFullName, + "pull request merge state", + ); + const token = await issueInstallationToken({ + ...env, + permissions: { pull_requests: "read" }, + repositories: [name], + }); + const read = async (path: string) => + await githubRequest( + "https://api.github.com", + `/repos/${encodeURIComponent(owner)}/${encodeURIComponent(name)}/pulls/${pullRequest.number}${path}`, + { token: token.token }, + ); + 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; + + // 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 ( + 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"); + 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..c9e3586d40 --- /dev/null +++ b/packages/junior-github/src/webhooks/pull-request-ready.ts @@ -0,0 +1,189 @@ +/** + * 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 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, + PluginLogger, +} from "@sentry/junior-plugin-api"; +import { z } from "zod"; +import type { GitHubDb } from "../db/database.js"; +import { + claimGitHubPullRequestReadySync, + releaseGitHubPullRequestReadySync, + setGitHubPullRequestReadyToMerge, +} 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"; + +/** + * 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([ + "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 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", + }; + } + return { + reason: "The pull request is no longer ready to merge.", + 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 items = args.reviewItems.forConversation(opener.conversationId); + const name = `${target.repositoryFullName}#${target.number}`; + const dedupeKey = `pull-request-ready:${name.toLowerCase()}`; + // 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; + // 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; + // 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; + } + } catch (error) { + failure ??= error; + } + requests = await releaseGitHubPullRequestReadySync(args.db, target, { + id, + requests, + }); + } + if (failure !== undefined) throw failure; +} + +/** + * 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..4f1aba01d3 --- /dev/null +++ b/packages/junior-github/tests/pull-request-ready.test.ts @@ -0,0 +1,424 @@ +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 { + 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"; +} + +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", + 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", + () => HttpResponse.json([]), + ), + http.get( + "https://api.github.com/repos/getsentry/junior/pulls/946/reviews", + ({ request }) => { + requests.push("reviews"); + const page = Number(new URL(request.url).searchParams.get("page")); + return HttpResponse.json( + github.reviews.slice((page - 1) * 100, page * 100), + ); + }, + ), + ); +} + +type ReviewItemCall = + | { conversationId: string; kind: "upsert"; notice: unknown } + | { conversationId: string; input: unknown; kind: "close" }; + +function route( + fixture: GitHubFixture, + calls: ReviewItemCall[], + errors: string[] = [], + failures = { close: 0 }, +): PluginRoute { + const reviewItems: PluginConversationReviewItems = { + forConversation(conversationId) { + return { + async upsert(notice) { + 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" }); + }, + }; + }, + }; + 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 failures = { close: 0 }; + const requests: string[] = []; + const github: GitHubState = { + draft: false, + mergeableStates: ["clean"], + merged: false, + reviews: [], + state: "open", + }; + mockGitHub(github, requests); + try { + await seedJuniorPullRequest(fixture, "slack:C123:1712345.0001"); + const webhook = route(fixture, calls, errors, failures); + + // GitHub can merge it, but no one approved it yet. + await webhook.handler(checkSuiteCompleted()); + 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" } }, + ...Array.from({ length: 100 }, () => ({ + 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 GitHub says that it can merge. Merge it, or say why not: ${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]); + + // 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 pull request is no longer ready to merge.", + status: "dismissed", + }, + 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); + + github.merged = true; + github.state = "closed"; + // 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; + 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"]); + 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", + "upsert", + "close", + ]); + expect(errors).toHaveLength(1); + } 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( + { + draft: false, + mergeableStates: ["clean"], + 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..3f7f8b77fe --- /dev/null +++ b/packages/junior-plugin-api/src/review-items.ts @@ -0,0 +1,36 @@ +/** 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; + /** What the notice is about, such as a pull request. Search matches `id`. */ + subject?: { id: string; title?: string; url?: string }; + 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..eaaf817a9b 100644 --- a/packages/junior/src/app.ts +++ b/packages/junior/src/app.ts @@ -111,7 +111,9 @@ import { disconnectStateAdapter } from "@/chat/state/adapter"; import { createProductionConversationWorkOptions, 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"; @@ -566,7 +568,10 @@ export async function createApp( configuredPlugins?.registrations ?? [], ); } - pluginRoutes = getPluginRoutes({ events }); + pluginRoutes = getPluginRoutes({ + events, + 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 077e775e67..12a57146e0 100644 --- a/packages/junior/src/chat/plugins/agent-hooks.ts +++ b/packages/junior/src/chat/plugins/agent-hooks.ts @@ -82,6 +82,8 @@ 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"; export interface ToolHookInput { @@ -937,6 +939,8 @@ function requirePublishedEvent( /** Collect route handlers exposed by plugins for app-level mounting. */ export function getPluginRoutes(options: { + /** Shows a Review item that a plugin route changed. App setup supplies it. */ + showReviewItem?: ShowReviewItem; events: { hasMatch?(event: Event): Promise; neededMatchKeys?(input: { @@ -969,6 +973,14 @@ export function getPluginRoutes(options: { }), }, codeChanges: createCodeChangePublisher(pluginName), + reviewItems: { + forConversation: (conversationId) => + createPluginReviewItems({ + conversationId, + plugin: pluginName, + show: options.showReviewItem ?? (async () => {}), + }), + }, 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..b0abad2b73 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,34 @@ 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 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. +- 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 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`. + ## Person actions `actions.ts` owns what a person can do. The review desk API @@ -159,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/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 new file mode 100644 index 0000000000..a2a87d9081 --- /dev/null +++ b/packages/junior/src/chat/review-items/plugin.ts @@ -0,0 +1,150 @@ +import type { + PluginReviewItems, + 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 type { JuniorDatabase } from "@/db/db"; +import type { ShowReviewItem } from "./changes"; +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 + * `plugin::`, so a plugin changes only its own items, and agents + * 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; + plugin: string; + show: ShowReviewItem; +}): PluginReviewItems { + const { conversationId, show } = args; + const scopedKey = (dedupeKey: string) => + `${PLUGIN_REVIEW_ITEM_KEY_PREFIX}${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 show(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 show(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 show(item.id); + } + }, + }; +} diff --git a/packages/junior/src/chat/tools/review-items.ts b/packages/junior/src/chat/tools/review-items.ts index fb2198a271..23b8c90d21 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 @@ -365,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: { @@ -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 new file mode 100644 index 0000000000..cb2a2ed7ae --- /dev/null +++ b/packages/junior/tests/integration/plugin-review-items.test.ts @@ -0,0 +1,220 @@ +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 { + 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", +): 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 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", + 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 () => { + shown = []; + 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", + show, + }); + + await items.upsert({ ...NOTICE, assigneeIdentityIds: [ana.identityId] }); + const added = await getOpenReviewItemByDedupeKey( + getDb(), + `plugin: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(), + `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, services).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, services).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.", + 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 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( + getDb(), + `plugin: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", + show, + }); + + 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(), + `plugin:github:${NOTICE.dedupeKey}`, + TENANT, + ), + ).toMatchObject({ + assignees: [{ userId: ana.user.id }], + visibility: "private", + }); + }); +});