Skip to content

Commit 2f3169e

Browse files
committed
Make grant minting idempotent too; centralize the trigger-name string
Gates mintRepoGrant on a new hasRepoGrant port, checked independently of hasWebhookTrigger, so a retry after a failure between the two steps mints only whichever one the failed attempt didn't finish rather than re-minting a grant that already exists. apps/hub's hasRepoGrant binds to a read against the same grant table mintRepoGrant inserts into. Also pulls the "<repo> pull-request-opened" trigger-name convention into one exported webhookTriggerName helper, used by both the create and the lookup in apps/hub, so they read the same string by construction instead of by two hand-kept copies of it. Documents, rather than changes, that a trigger is never disabled or deleted on GitHub disconnect: re-adding a repo after a reconnect finds its old trigger still live and is skipped, not re-created. Fixes CL-7134.
1 parent 8d72904 commit 2f3169e

4 files changed

Lines changed: 62 additions & 11 deletions

File tree

‎apps/hub/src/index.ts‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,7 @@ import {
174174
isAutomatableWorkflowName,
175175
isConversationalWorkflowName,
176176
validateTriggerFieldsAtCreate,
177+
webhookTriggerName,
177178
workflowCatalogEntry,
178179
workflowDisplayName,
179180
workbenchTemplateLibraryEntries,
@@ -2172,6 +2173,17 @@ export async function createHub(config: HubConfig) {
21722173
});
21732174
return row?.id;
21742175
},
2176+
hasRepoGrant: async (tenantId, repo) => {
2177+
const existing = await db.query.grant.findFirst({
2178+
where: and(
2179+
eq(grantTable.tenantId, tenantId),
2180+
eq(grantTable.resource, `repo:${repo.name}`),
2181+
eq(grantTable.action, "read"),
2182+
),
2183+
columns: { id: true },
2184+
});
2185+
return existing !== undefined;
2186+
},
21752187
mintRepoGrant: async (tenantId, repo) => {
21762188
const memberRole = await db.query.role.findFirst({
21772189
where: and(
@@ -2207,7 +2219,7 @@ export async function createHub(config: HubConfig) {
22072219
const row = await webhookTriggerStore.create({
22082220
id: generateId("workflowRun"),
22092221
tenantId,
2210-
name: `${repo.name} pull-request-opened`,
2222+
name: webhookTriggerName(repo),
22112223
workflowDefinitionId: codeReviewDefinitionId,
22122224
inputTemplate: `Review the pull request at {{pull_request.html_url}}`,
22132225
secret: generateWebhookSecret(),
@@ -2217,7 +2229,7 @@ export async function createHub(config: HubConfig) {
22172229
},
22182230
hasWebhookTrigger: async (tenantId, codeReviewDefinitionId, repo) => {
22192231
const triggers = await webhookTriggerStore.list(tenantId);
2220-
const triggerName = `${repo.name} pull-request-opened`;
2232+
const triggerName = webhookTriggerName(repo);
22212233
return triggers.some(
22222234
(trigger) =>
22232235
trigger.workflowDefinitionId === codeReviewDefinitionId &&

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,11 @@ export type ConnectGithubRoutesDeps = {
8484
* `undefined` when the template's own workflow was never deployed for
8585
* this tenant (a create-flow bug, not something this route can fix). */
8686
resolveCodeReviewDefinitionId(tenantId: string): Promise<string | undefined>;
87+
/** True once this repo already has the `repo:<owner/name>` grant — see
88+
* `./connect-github-setup.ts`'s `ConnectGithubSetupPorts.hasRepoGrant`
89+
* for why this makes a retry between minting the grant and creating
90+
* the trigger safe. */
91+
hasRepoGrant(tenantId: string, repo: GitHubRepoSummary): Promise<boolean>;
8792
/** Mints the `repo:<owner/name>`-scoped grant a launched review run
8893
* needs to read this repo — see `./connect-github-setup.ts`'s
8994
* `ConnectGithubSetupPorts.mintRepoGrant` for the exact resource shape. */
@@ -281,6 +286,7 @@ export function createConnectGithubRoutes(
281286
const introductionsAlreadyPosted =
282287
settingsBefore.selectedRepos.length > 0;
283288
const result = await startReviewingRepos(body.repoIds, state.repos, {
289+
hasRepoGrant: (repo) => deps.hasRepoGrant(tenant.id, repo),
284290
mintRepoGrant: (repo) => deps.mintRepoGrant(tenant.id, repo),
285291
createWebhookTrigger: (repo) =>
286292
deps.createWebhookTrigger(

‎packages/workflow-catalog/src/connect-github-setup.ts‎

Lines changed: 41 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,17 @@
88
import type { GitHubRepoSummary } from "@corbits/github-tools";
99

1010
export interface ConnectGithubSetupPorts {
11+
/**
12+
* True once this repo already has the `repo:<repo.name>` grant —
13+
* checked before minting one, so a retry after a failure between
14+
* minting the grant and creating the trigger never mints a second
15+
* grant for a repo that already has one. The `grant` table
16+
* (`vendor/intx/db`) carries no unique constraint over
17+
* tenant/resource/action, so this read is the only thing standing
18+
* between a retry and a duplicate row. A host binds this to a read
19+
* against the same `grant` table `mintRepoGrant` inserts into.
20+
*/
21+
hasRepoGrant(repo: GitHubRepoSummary): Promise<boolean>;
1122
/**
1223
* Mints one grant scoped to `repo:<repo.name>` (the `owner/name` full
1324
* name — the same `"<type>:<id>"` resource-string shape
@@ -28,11 +39,15 @@ export interface ConnectGithubSetupPorts {
2839
): Promise<{ readonly id: string }>;
2940
/**
3041
* True once this repo already has a live webhook trigger — checked
31-
* before minting anything for it, so a retry after a mid-loop failure
32-
* (a repo 1..N-1 already set up, N onward not) never mints a second
33-
* grant or trigger for the repos a prior attempt already finished. A
34-
* host binds this to a read against `@corbits/webhook-triggers`'
35-
* `WebhookTriggerStore.list`.
42+
* before creating one, so a retry after a mid-loop failure (a repo
43+
* 1..N-1 already set up, N onward not) never mints a second trigger
44+
* for a repo a prior attempt already finished. A host binds this to a
45+
* read against `@corbits/webhook-triggers`' `WebhookTriggerStore.list`.
46+
*
47+
* This is never cleared on GitHub disconnect: nothing here disables or
48+
* deletes a trigger, so re-adding a repo after a reconnect finds its
49+
* old trigger still live and skips it rather than minting a new one —
50+
* intentional, not a gap this module owns closing.
3651
*/
3752
hasWebhookTrigger(repo: GitHubRepoSummary): Promise<boolean>;
3853
/**
@@ -57,9 +72,13 @@ export interface StartReviewingReposResult {
5772
* is a bug in how the connect card's own selection state was built, not
5873
* something to silently drop.
5974
*
60-
* Idempotent by construction: a repo `ports.hasWebhookTrigger` already
61-
* reports true for is skipped entirely, so retrying after a mid-loop
62-
* failure only mints for the repos the failed attempt never reached.
75+
* Idempotent by construction: the grant and the trigger are each gated
76+
* on their own existence check, independently, rather than one check
77+
* guarding both — a retry after a failure between minting the grant and
78+
* creating the trigger must still create the trigger without re-minting
79+
* the grant, and a retry after a failure before the grant was minted
80+
* must still mint it. A repo both checks already report true for is
81+
* skipped entirely.
6382
*/
6483
export async function startReviewingRepos(
6584
repoIds: readonly string[],
@@ -79,10 +98,12 @@ export async function startReviewingRepos(
7998

8099
const createdTriggerIds: string[] = [];
81100
for (const repo of selected) {
101+
if (!(await ports.hasRepoGrant(repo))) {
102+
await ports.mintRepoGrant(repo);
103+
}
82104
if (await ports.hasWebhookTrigger(repo)) {
83105
continue;
84106
}
85-
await ports.mintRepoGrant(repo);
86107
const trigger = await ports.createWebhookTrigger(repo);
87108
createdTriggerIds.push(trigger.id);
88109
}
@@ -91,3 +112,14 @@ export async function startReviewingRepos(
91112

92113
return { createdTriggerIds };
93114
}
115+
116+
/**
117+
* The one place the `webhook_trigger.name` convention for a repo's
118+
* pull-request-opened trigger is spelled out — both
119+
* `createWebhookTrigger`'s insert and `hasWebhookTrigger`'s lookup bind
120+
* against this, in `apps/hub`, so the two can never drift into matching
121+
* different strings.
122+
*/
123+
export function webhookTriggerName(repo: GitHubRepoSummary): string {
124+
return `${repo.name} pull-request-opened`;
125+
}

‎packages/workflow-catalog/src/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ export {
4545
} from "./settings";
4646
export {
4747
startReviewingRepos,
48+
webhookTriggerName,
4849
type ConnectGithubSetupPorts,
4950
type StartReviewingReposResult,
5051
} from "./connect-github-setup";

0 commit comments

Comments
 (0)