Make start-reviewing repos idempotent

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,14 +2219,23 @@ 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(),
           createdBy: principalId,
         });
         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 ?? {};

packages/chat-ui/test/connect-github-flow.test.tsx

@@ -68,13 +68,19 @@ function buildHarness() {
   let subscriber: ((state: ConnectGithubQuery) => void) | undefined;
 
   const setupPorts: ConnectGithubSetupPorts = {
+    async hasRepoGrant() {
+      return false;
+    },
     async mintRepoGrant(repo) {
       grantedRepos.push(repo.name);
     },
     async createWebhookTrigger(repo) {
       createdTriggerRepos.push(repo.name);
       return { id: `trg_${repo.id}` };
     },
+    async hasWebhookTrigger() {
+      return false;
+    },
     async persistSelectedRepos(repoIds) {
       persistedRepoIds = repoIds;
     },

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: [],

packages/workflow-catalog/src/connect-github-routes.test.ts

@@ -80,6 +80,8 @@ function buildApp(overrides: Partial<ConnectGithubRoutesDeps> = {}) {
     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<ConnectGithubRoutesDeps> = {}) {
       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,

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<string | undefined>;
+  /** True once this repo already has the `repo:<owner/name>` 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<boolean>;
   /** Mints the `repo:<owner/name>`-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<boolean>;
   /** 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,

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,13 +18,19 @@ function fakePorts() {
   const createdTriggerRepos: string[] = [];
   let persistedRepoIds: readonly string[] | undefined;
   const ports: ConnectGithubSetupPorts = {
+    async hasRepoGrant() {
+      return false;
+    },
     async mintRepoGrant(repo) {
       grantedRepos.push(repo.name);
     },
     async createWebhookTrigger(repo) {
       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<string>();
+  const existingTriggerRepoNames = new Set<string>();
+  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");
+  });
 });