diff --git a/apps/hub/src/index.ts b/apps/hub/src/index.ts index 66db0cc6d..9195c60f9 100644 --- a/apps/hub/src/index.ts +++ b/apps/hub/src/index.ts @@ -174,6 +174,7 @@ import { isAutomatableWorkflowName, isConversationalWorkflowName, validateTriggerFieldsAtCreate, + webhookTriggerName, workflowCatalogEntry, workflowDisplayName, workbenchTemplateLibraryEntries, @@ -2172,6 +2173,17 @@ export async function createHub(config: HubConfig) { }); return row?.id; }, + hasRepoGrant: async (tenantId, repo) => { + const existing = await db.query.grant.findFirst({ + where: and( + eq(grantTable.tenantId, tenantId), + eq(grantTable.resource, `repo:${repo.name}`), + eq(grantTable.action, "read"), + ), + columns: { id: true }, + }); + return existing !== undefined; + }, mintRepoGrant: async (tenantId, repo) => { const memberRole = await db.query.role.findFirst({ where: and( @@ -2207,7 +2219,7 @@ export async function createHub(config: HubConfig) { const row = await webhookTriggerStore.create({ id: generateId("workflowRun"), tenantId, - name: `${repo.name} pull-request-opened`, + name: webhookTriggerName(repo), workflowDefinitionId: codeReviewDefinitionId, inputTemplate: `Review the pull request at {{pull_request.html_url}}`, secret: generateWebhookSecret(), @@ -2215,6 +2227,15 @@ export async function createHub(config: HubConfig) { }); return { id: row.id }; }, + hasWebhookTrigger: async (tenantId, codeReviewDefinitionId, repo) => { + const triggers = await webhookTriggerStore.list(tenantId); + const triggerName = webhookTriggerName(repo); + return triggers.some( + (trigger) => + trigger.workflowDefinitionId === codeReviewDefinitionId && + trigger.name === triggerName, + ); + }, getTemplateSettings: async (tenantId, workbenchId) => { const row = await chatStore.getWorkbenchSettings(tenantId, workbenchId); const settings = row?.settings ?? {}; diff --git a/packages/chat-ui/test/connect-github-flow.test.tsx b/packages/chat-ui/test/connect-github-flow.test.tsx index 665e539ab..b7678342a 100644 --- a/packages/chat-ui/test/connect-github-flow.test.tsx +++ b/packages/chat-ui/test/connect-github-flow.test.tsx @@ -68,6 +68,9 @@ function buildHarness() { let subscriber: ((state: ConnectGithubQuery) => void) | undefined; const setupPorts: ConnectGithubSetupPorts = { + async hasRepoGrant() { + return false; + }, async mintRepoGrant(repo) { grantedRepos.push(repo.name); }, @@ -75,6 +78,9 @@ function buildHarness() { createdTriggerRepos.push(repo.name); return { id: `trg_${repo.id}` }; }, + async hasWebhookTrigger() { + return false; + }, async persistSelectedRepos(repoIds) { persistedRepoIds = repoIds; }, diff --git a/packages/workflow-catalog/src/connect-github-credential-link.test.ts b/packages/workflow-catalog/src/connect-github-credential-link.test.ts index 1a4cb8da4..5d38af494 100644 --- a/packages/workflow-catalog/src/connect-github-credential-link.test.ts +++ b/packages/workflow-catalog/src/connect-github-credential-link.test.ts @@ -139,8 +139,10 @@ describe("the room GitHub connect card reads what its own submit writes", () => githubDescriptor.displayName, ), resolveCodeReviewDefinitionId: async () => "wfd_code_review", + hasRepoGrant: async () => false, mintRepoGrant: async () => {}, createWebhookTrigger: async () => ({ id: "trg_1" }), + hasWebhookTrigger: async () => false, getTemplateSettings: async () => ({ pendingConnections: [], selectedRepos: [], @@ -182,8 +184,10 @@ describe("the room GitHub connect card reads what its own submit writes", () => log: () => {}, resolveGithubConfig: buildResolveGithubConfig(store, githubDescriptor.id), resolveCodeReviewDefinitionId: async () => "wfd_code_review", + hasRepoGrant: async () => false, mintRepoGrant: async () => {}, createWebhookTrigger: async () => ({ id: "trg_1" }), + hasWebhookTrigger: async () => false, getTemplateSettings: async () => ({ pendingConnections: [], selectedRepos: [], diff --git a/packages/workflow-catalog/src/connect-github-routes.test.ts b/packages/workflow-catalog/src/connect-github-routes.test.ts index 255e0e73c..1074a8c6a 100644 --- a/packages/workflow-catalog/src/connect-github-routes.test.ts +++ b/packages/workflow-catalog/src/connect-github-routes.test.ts @@ -80,6 +80,8 @@ function buildApp(overrides: Partial = {}) { log: () => {}, resolveGithubConfig: async () => githubConfig, resolveCodeReviewDefinitionId: async () => "wfd_code_review", + hasRepoGrant: async (tenantId, repo) => + grants.some((g) => g.tenantId === tenantId && g.repo.id === repo.id), mintRepoGrant: async (tenantId, repo) => { grants.push({ tenantId, repo }); }, @@ -92,6 +94,8 @@ function buildApp(overrides: Partial = {}) { triggers.push({ tenantId, repo }); return { id: `trg_${repo.id}` }; }, + hasWebhookTrigger: async (_tenantId, _definitionId, repo) => + triggers.some((t) => t.repo.id === repo.id), getTemplateSettings: async () => settings, persistSelectedRepos: async ( _tenantId, diff --git a/packages/workflow-catalog/src/connect-github-routes.ts b/packages/workflow-catalog/src/connect-github-routes.ts index d62e04a0d..c160a8036 100644 --- a/packages/workflow-catalog/src/connect-github-routes.ts +++ b/packages/workflow-catalog/src/connect-github-routes.ts @@ -84,6 +84,11 @@ export type ConnectGithubRoutesDeps = { * `undefined` when the template's own workflow was never deployed for * this tenant (a create-flow bug, not something this route can fix). */ resolveCodeReviewDefinitionId(tenantId: string): Promise; + /** True once this repo already has the `repo:` grant — see + * `./connect-github-setup.ts`'s `ConnectGithubSetupPorts.hasRepoGrant` + * for why this makes a retry between minting the grant and creating + * the trigger safe. */ + hasRepoGrant(tenantId: string, repo: GitHubRepoSummary): Promise; /** Mints the `repo:`-scoped grant a launched review run * needs to read this repo — see `./connect-github-setup.ts`'s * `ConnectGithubSetupPorts.mintRepoGrant` for the exact resource shape. */ @@ -97,6 +102,15 @@ export type ConnectGithubRoutesDeps = { codeReviewDefinitionId: string, repo: GitHubRepoSummary, ): Promise<{ readonly id: string }>; + /** True once this repo already has a live webhook trigger for the + * resolved code-review definition — see + * `./connect-github-setup.ts`'s `ConnectGithubSetupPorts.hasWebhookTrigger` + * for why this makes a retry after a mid-loop failure safe. */ + hasWebhookTrigger( + tenantId: string, + codeReviewDefinitionId: string, + repo: GitHubRepoSummary, + ): Promise; /** The room's current `template/*` settings — read before every * `start-reviewing` write so the state read and the persisted patch * never race a stale pending-connections list. */ @@ -272,6 +286,7 @@ export function createConnectGithubRoutes( const introductionsAlreadyPosted = settingsBefore.selectedRepos.length > 0; const result = await startReviewingRepos(body.repoIds, state.repos, { + hasRepoGrant: (repo) => deps.hasRepoGrant(tenant.id, repo), mintRepoGrant: (repo) => deps.mintRepoGrant(tenant.id, repo), createWebhookTrigger: (repo) => deps.createWebhookTrigger( @@ -280,6 +295,8 @@ export function createConnectGithubRoutes( codeReviewDefinitionId, repo, ), + hasWebhookTrigger: (repo) => + deps.hasWebhookTrigger(tenant.id, codeReviewDefinitionId, repo), persistSelectedRepos: async (repoIds) => { const settings = await deps.getTemplateSettings( tenant.id, diff --git a/packages/workflow-catalog/src/connect-github-setup.test.ts b/packages/workflow-catalog/src/connect-github-setup.test.ts index b073ed674..f7aee456d 100644 --- a/packages/workflow-catalog/src/connect-github-setup.test.ts +++ b/packages/workflow-catalog/src/connect-github-setup.test.ts @@ -2,6 +2,7 @@ import { describe, expect, test } from "bun:test"; import { startReviewingRepos, + webhookTriggerName, type ConnectGithubSetupPorts, } from "./connect-github-setup"; import type { GitHubRepoSummary } from "@corbits/github-tools"; @@ -17,6 +18,9 @@ function fakePorts() { const createdTriggerRepos: string[] = []; let persistedRepoIds: readonly string[] | undefined; const ports: ConnectGithubSetupPorts = { + async hasRepoGrant() { + return false; + }, async mintRepoGrant(repo) { grantedRepos.push(repo.name); }, @@ -24,6 +28,9 @@ function fakePorts() { createdTriggerRepos.push(repo.name); return { id: `trg_${repo.id}` }; }, + async hasWebhookTrigger() { + return false; + }, async persistSelectedRepos(repoIds) { persistedRepoIds = repoIds; }, @@ -36,6 +43,41 @@ function fakePorts() { }; } +/** + * A grant-store- and `WebhookTriggerStore`-backed fake: `hasRepoGrant` + * and `hasWebhookTrigger` each reflect what `mintRepoGrant` and + * `createWebhookTrigger` have actually persisted so far — independently + * of each other, the same as the real ports bind against the real + * `grant` table and `WebhookTriggerStore.list`. Used to prove a retry + * after a mid-loop failure is idempotent regardless of exactly where in + * a repo's two steps the failure landed. + */ +function fakeBackedPorts() { + const grantedRepoNames = new Set(); + const existingTriggerRepoNames = new Set(); + const grantedRepos: string[] = []; + const createdTriggerRepos: string[] = []; + const ports: ConnectGithubSetupPorts = { + async hasRepoGrant(repo) { + return grantedRepoNames.has(repo.name); + }, + async mintRepoGrant(repo) { + grantedRepos.push(repo.name); + grantedRepoNames.add(repo.name); + }, + async createWebhookTrigger(repo) { + createdTriggerRepos.push(repo.name); + existingTriggerRepoNames.add(repo.name); + return { id: `trg_${repo.id}` }; + }, + async hasWebhookTrigger(repo) { + return existingTriggerRepoNames.has(repo.name); + }, + async persistSelectedRepos() {}, + }; + return { ports, grantedRepos, createdTriggerRepos }; +} + describe("startReviewingRepos", () => { test("mints a grant and a webhook trigger per selected repo, then persists the selection", async () => { const fake = fakePorts(); @@ -63,4 +105,99 @@ describe("startReviewingRepos", () => { ).rejects.toThrow(/not in the listed repos/); expect(fake.grantedRepos).toEqual([]); }); + + test("retrying after a mid-loop failure never mints a duplicate grant or trigger", async () => { + const fake = fakeBackedPorts(); + const failingPorts: ConnectGithubSetupPorts = { + ...fake.ports, + async mintRepoGrant(repo) { + if (repo.name === "acme/gadgets") { + throw new Error("mint failed"); + } + await fake.ports.mintRepoGrant(repo); + }, + }; + + await expect( + startReviewingRepos(["1", "2", "3"], REPOS, failingPorts), + ).rejects.toThrow(/mint failed/); + + // The first repo made it through before the failure; the other two + // never got a grant or a trigger. + expect(fake.grantedRepos).toEqual(["acme/widgets"]); + expect(fake.createdTriggerRepos).toEqual(["acme/widgets"]); + + // Retrying the same selection (as the route's "Try again" does) + // skips the repo that's already set up and only mints for the rest. + const result = await startReviewingRepos( + ["1", "2", "3"], + REPOS, + fake.ports, + ); + + expect(fake.grantedRepos).toEqual([ + "acme/widgets", + "acme/gadgets", + "acme/sprockets", + ]); + expect(fake.createdTriggerRepos).toEqual([ + "acme/widgets", + "acme/gadgets", + "acme/sprockets", + ]); + expect(result.createdTriggerIds).toEqual(["trg_2", "trg_3"]); + }); + + test("retrying after a failure between minting the grant and creating the trigger never re-mints the grant", async () => { + const fake = fakeBackedPorts(); + const failingPorts: ConnectGithubSetupPorts = { + ...fake.ports, + async createWebhookTrigger(repo) { + if (repo.name === "acme/gadgets") { + throw new Error("trigger create failed"); + } + return fake.ports.createWebhookTrigger(repo); + }, + }; + + await expect( + startReviewingRepos(["1", "2", "3"], REPOS, failingPorts), + ).rejects.toThrow(/trigger create failed/); + + // The failing repo's grant was minted before the trigger create blew + // up; the repo after it was never reached at all. + expect(fake.grantedRepos).toEqual(["acme/widgets", "acme/gadgets"]); + expect(fake.createdTriggerRepos).toEqual(["acme/widgets"]); + + const result = await startReviewingRepos( + ["1", "2", "3"], + REPOS, + fake.ports, + ); + + // acme/gadgets' grant is not re-minted on retry — only its missing + // trigger is created. + expect(fake.grantedRepos).toEqual([ + "acme/widgets", + "acme/gadgets", + "acme/sprockets", + ]); + expect(fake.createdTriggerRepos).toEqual([ + "acme/widgets", + "acme/gadgets", + "acme/sprockets", + ]); + expect(result.createdTriggerIds).toEqual(["trg_2", "trg_3"]); + }); +}); + +describe("webhookTriggerName", () => { + test("names a repo's trigger consistently, the one convention both the create and the lookup bind against", () => { + const repo: GitHubRepoSummary = { + id: "1", + name: "acme/widgets", + openPullRequestCount: 0, + }; + expect(webhookTriggerName(repo)).toBe("acme/widgets pull-request-opened"); + }); }); diff --git a/packages/workflow-catalog/src/connect-github-setup.ts b/packages/workflow-catalog/src/connect-github-setup.ts index ad49fda96..6632fc580 100644 --- a/packages/workflow-catalog/src/connect-github-setup.ts +++ b/packages/workflow-catalog/src/connect-github-setup.ts @@ -8,6 +8,17 @@ import type { GitHubRepoSummary } from "@corbits/github-tools"; export interface ConnectGithubSetupPorts { + /** + * True once this repo already has the `repo:` grant — + * checked before minting one, so a retry after a failure between + * minting the grant and creating the trigger never mints a second + * grant for a repo that already has one. The `grant` table + * (`vendor/intx/db`) carries no unique constraint over + * tenant/resource/action, so this read is the only thing standing + * between a retry and a duplicate row. A host binds this to a read + * against the same `grant` table `mintRepoGrant` inserts into. + */ + hasRepoGrant(repo: GitHubRepoSummary): Promise; /** * Mints one grant scoped to `repo:` (the `owner/name` full * name — the same `":"` resource-string shape @@ -26,6 +37,19 @@ export interface ConnectGithubSetupPorts { createWebhookTrigger( repo: GitHubRepoSummary, ): Promise<{ readonly id: string }>; + /** + * True once this repo already has a live webhook trigger — checked + * before creating one, so a retry after a mid-loop failure (a repo + * 1..N-1 already set up, N onward not) never mints a second trigger + * for a repo a prior attempt already finished. A host binds this to a + * read against `@corbits/webhook-triggers`' `WebhookTriggerStore.list`. + * + * This is never cleared on GitHub disconnect: nothing here disables or + * deletes a trigger, so re-adding a repo after a reconnect finds its + * old trigger still live and skips it rather than minting a new one — + * intentional, not a gap this module owns closing. + */ + hasWebhookTrigger(repo: GitHubRepoSummary): Promise; /** * Records which repos this room is reviewing — the `template/*` * settings namespace's `selectedRepos` key (`./settings.ts`'s @@ -42,11 +66,19 @@ export interface StartReviewingReposResult { } /** - * Mints one grant and one webhook trigger per selected repo, then - * records the selection. `repoIds` must all resolve against `repos` — - * a caller passing an id `repos` doesn't carry is a bug in how the - * connect card's own selection state was built, not something to - * silently drop. + * Mints one grant and one webhook trigger per selected repo that doesn't + * already have one, then records the selection. `repoIds` must all + * resolve against `repos` — a caller passing an id `repos` doesn't carry + * is a bug in how the connect card's own selection state was built, not + * something to silently drop. + * + * Idempotent by construction: the grant and the trigger are each gated + * on their own existence check, independently, rather than one check + * guarding both — a retry after a failure between minting the grant and + * creating the trigger must still create the trigger without re-minting + * the grant, and a retry after a failure before the grant was minted + * must still mint it. A repo both checks already report true for is + * skipped entirely. */ export async function startReviewingRepos( repoIds: readonly string[], @@ -66,7 +98,12 @@ export async function startReviewingRepos( const createdTriggerIds: string[] = []; for (const repo of selected) { - await ports.mintRepoGrant(repo); + if (!(await ports.hasRepoGrant(repo))) { + await ports.mintRepoGrant(repo); + } + if (await ports.hasWebhookTrigger(repo)) { + continue; + } const trigger = await ports.createWebhookTrigger(repo); createdTriggerIds.push(trigger.id); } @@ -75,3 +112,14 @@ export async function startReviewingRepos( return { createdTriggerIds }; } + +/** + * The one place the `webhook_trigger.name` convention for a repo's + * pull-request-opened trigger is spelled out — both + * `createWebhookTrigger`'s insert and `hasWebhookTrigger`'s lookup bind + * against this, in `apps/hub`, so the two can never drift into matching + * different strings. + */ +export function webhookTriggerName(repo: GitHubRepoSummary): string { + return `${repo.name} pull-request-opened`; +} diff --git a/packages/workflow-catalog/src/index.ts b/packages/workflow-catalog/src/index.ts index 27bf230bc..17f484d50 100644 --- a/packages/workflow-catalog/src/index.ts +++ b/packages/workflow-catalog/src/index.ts @@ -45,6 +45,7 @@ export { } from "./settings"; export { startReviewingRepos, + webhookTriggerName, type ConnectGithubSetupPorts, type StartReviewingReposResult, } from "./connect-github-setup";