Close the GitHub connect grant/webhook-trigger duplication race

[TheGreatAxios]

Summary

Fixes CL-7242 (High): startReviewingRepos's hasRepoGrant/mintRepoGrant and hasWebhookTrigger/createWebhookTrigger checks are a fast path for a sequential retry (CL-7134's guards), not a lock. Two truly concurrent calls for the same repo — a double-click on "Start reviewing," or a client retrying an in-flight request rather than a failed one — can both read "not set up yet" before either write lands, so both mint a repo:<owner/name> grant and both create a live webhook trigger for the same repo. Audited and verified directly.

What a duplicate grant actually means for the user: not an untidy row. Two identically-scoped repo:<owner/name> allow grants means the room's access to that repo depends on which of two indistinguishable rows a later revoke happens to touch — revoking one leaves the other standing, so "disconnect this repo" can silently fail to actually revoke access. A duplicate trigger is the paired failure: two live triggers with different secrets for the same repo can double-launch the code-review workflow on every PR, or intermittently fail signature verification if GitHub's configured webhook secret doesn't match the one the person was shown.

Design note: Interchange's grant table is never touched

An earlier revision of this PR added a partial unique index directly to Interchange's own grant table (first as a vendor/intx/db migration, then as a workbench-owned migration issuing DDL against it). Both were wrong for the same reason: grants are Interchange's — we use them, we do not modify their schema, and not even a workbench-owned migration should issue DDL against a table we don't own. This revision reverts every trace of that: vendor/ has zero diff against origin/main (check:killdates's recorded hash for vendor/intx/db is unchanged), and mintRepoGrant is back to the exact plain insert Interchange's own POST /grants route uses.

The concurrency guard now lives entirely in a new workbench-owned table: webhook_triggers.repo_review_lease (our own schema, no relationship to any Interchange table — every workbench package already stores a platform id as a plain text column, never a cross-schema FK; see packages/chat/src/schema.ts's own header, packages/bench/src/schema.ts's, packages/routines/src/schema.ts's, etc., all explicitly documenting "plain text identifier, not a foreign key" — I confirmed via grep -rn ".references(" packages/ apps/ that zero workbench-owned tables anywhere in this repo declare a cross-schema FK to Interchange core. A cross-schema FK from our lease table to Interchange's tenant table would itself be the kind of hard schema coupling to a re-pinned vendor tree the "don't touch their schema" rule exists to avoid — closer to the thing being banned than the thing being asked for.)

startReviewingRepos acquires this lease first, per repo — only the winner runs the rest of that repo's body (hasRepoGrant/mintRepoGrant/hasWebhookTrigger/createWebhookTrigger, otherwise byte-for-byte what CL-7134 left them as), so two concurrent callers can never both reach it for the same repo. The lease is released in a finally as soon as that body finishes — success or failure — so a legitimate retry is never blocked by its own prior attempt. A database-side compare-and-swap (INSERT ... ON CONFLICT (tenant_id, repo) DO UPDATE ... WHERE leased_at < now() - interval '2 minutes') also lets a lease older than two minutes be stolen, so a process that crashes mid-work self-heals on the next attempt instead of permanently stranding that repo's setup. This is deliberately answering the CL-7213 precedent ("do not leave a record asserting something that is not true"): the lease never asserts "the grant/trigger exist" — only ever "someone claimed this repo's setup as of leased_at" — so it can never lie about completion, only about currency, and staleness reclaim bounds how long a lie about currency can matter.

The paired webhook_trigger fix stays in our own schema and is unaffected by any of the above: a unique index on webhook_trigger(tenant_id, workflow_definition_id, name) (our table, uncontroversial), reconciling any pre-existing duplicates first. The store gains ensure() — insert-first with onConflictDoNothing and a re-select on conflict, returning the real persisted secret — distinct from create(), which keeps its plain-insert contract so the generic management API never hands a caller someone else's trigger under a different inputTemplate; a duplicate create() now 409s. apps/hub's createWebhookTrigger port binds to ensure() as defense-in-depth even though the lease already makes this call-site single-flight per repo.

Reconciling pre-existing duplicate grants (a database that already has them from before this fix ships) is packages/workflow-catalog/src/reconcile-duplicate-repo-grants.ts — ordinary application code, not a migration: a one-time, always-safe-to-re-run DELETE against Interchange's grant table issued through @intx/db's published grant export, the exact same table object apps/hub/src/index.ts already reads and writes through for every other grant operation in this codebase. No schema is touched (no ALTER TABLE, no index, no constraint) — a DDL delta would need hand-reapplying at every future vendor/intx/* re-pin forever; this DML statement runs once and leaves nothing behind to reconcile. Keeps the lowest-id row per (tenant, resource, action) group, so a repo never ends up with fewer grants than it had. scripts/db-setup.ts calls it once per setup run and logs what it removed.

Should mintRepoGrant route through Interchange's own grants API instead of a direct insert?

Assessed, not done here — this would be a larger change than CL-7242 should carry, and I'd rather report it than scope-creep this PR:

• vendor/intx/hub-api/src/routes/grants.ts's POST / is an HTTP route, not an importable function — there's no in-process API surface to call directly; using it would mean apps/hub calling itself over loopback HTTP or extracting a new reusable non-HTTP grant-creation function from hub-api (neither exists today).
• It's gated by requireGrant("grant:*", "create") — the calling principal must already hold a grant:*/create grant. Our GitHub connect flow's calling context doesn't have (or need) that today; routing through this route would mean granting the connect flow (or some internal system caller) the ability to create any grant in the tenant, which is an authorization-model change with its own blast radius, not a mechanical swap.
• Its handler does no conflict handling of its own (no onConflictDoNothing, nothing) — routing through it would not make duplication impossible on its own; the lease is still what would have to serialize it, so this doesn't remove any of the work in this PR, only relocates one insert.
• mintRepoGrant isn't the only hand-rolled db.insert(grantTable) in this codebase — packages/chat/src/workbench-tenancy.ts and packages/folded-runs/src/launch.ts do the same thing. A real fix here would be systemic, not scoped to one call site.

Recommend filing this as its own ticket if it's worth pursuing.

CL-7134 overlap

No contradiction, no duplication. CL-7134 (Done, PR #445) added the hasRepoGrant/hasWebhookTrigger check-then-act guards themselves, closing a mid-loop-failure-plus-retry gap. This PR adds the atomicity backstop that guard explicitly lacked against true concurrency. The checks stay in place, unchanged, as the fast path for a sequential retry; only the correctness guarantee under concurrency moved to the lease.

Regression tests (confirmed red before each fix existed, per-file)

All against real local Postgres:

• packages/webhook-triggers/test/repo-review-lease.drizzle.test.ts — two concurrent acquire() calls for the same (tenant, repo) settle on exactly one winner; a stale lease (backdated past 2 minutes) can be stolen; a fresh one cannot; release() lets an immediate reacquire succeed.
• packages/webhook-triggers/test/store.drizzle.test.ts — two concurrent ensure() calls settle on exactly one trigger, same id and secret.
• packages/webhook-triggers/test/management-routes.test.ts — a second create() for an existing name/definition 409s.
• packages/workflow-catalog/src/connect-github-setup.test.ts — two concurrent startReviewingRepos calls sharing lease-backed fakes (synchronous check-and-set, same race shape as the real DB compare-and-swap) mint exactly one grant and one trigger for the same repo — the pure reconstruction of the audit's own deleted reproduction, manually confirmed red (removing the lease gate reproduces 2 grants) before restoring.
• packages/workflow-catalog/test/connect-github-setup.race.test.ts — the same reconstruction against real, production-shaped ports (the real RepoReviewLeaseStore, a bare grant insert, WebhookTriggerStore.ensure()) on a scratch database, stable across repeated runs.
• packages/workflow-catalog/test/reconcile-duplicate-repo-grants.test.ts — seeds duplicate + unrelated grants, confirms exactly the duplicates are removed (lowest id kept), unrelated rows untouched, and a second run is a no-op.

Notes for reviewers

• bun.lock picks up 4 unrelated tool-package version bumps and one nested peer-resolution swap, already present on origin/main's stale lockfile before this branch; bun install (needed for new devDependencies) synced that pre-existing drift. Not introduced here.
• scripts/checks/no-product-tenancy.ts's allowlist gains an explicit ruling for repo_review_lease (webhook-triggers' schema file now declares 2 tables instead of 1) — required by the check, not incidental.
• No Interchange defects to report — Interchange's own API surface (@intx/db's published exports, the grants route) worked exactly as documented; the assessment above is a scope/authorization question, not a defect.

Test plan

• cd packages/webhook-triggers && bun run typecheck && bun test
• cd packages/workflow-catalog && bun run typecheck && bun test
• cd apps/hub && bun run typecheck && bun test
• cd vendor/intx/db && bun run typecheck && bun test (confirmed unaffected — zero diff against origin/main)
• bunx prettier --check .
• bun run lint (0 errors; pre-existing unrelated warnings only)
• bun run check:killdates (vendor/intx/db hash unchanged)
• bun run check:structural
• CI (pushed, not polling — do not merge)

[TheGreatAxios]

Merge-order hazard between this PR and the other one touching scripts/checks/kill-dates.txt.

PR #503 (CL-7226) recomputes the content hash for all 14 vendor/intx/* rows, because it fixed hashDirectory to exclude dist/ — the previously recorded hashes were computed on a tree where local build output existed, and a fresh CI checkout has none, so 14 rows mismatched.

PR #508 (CL-7242) adds a new migration and a schema change under vendor/intx/db, which changes that package's tree content and therefore its hash.

Each PR computed its hash from a tree that does not contain the other's changes. They will conflict textually on the vendor/intx/db row, and — more importantly — resolving that conflict by picking either side's literal hash will be wrong, because the correct post-merge hash is of a tree containing both the dist/ exclusion fix and the new migration.

Whichever of these merges second must recompute rather than resolve by hand:

bun -e 'import { hashDirectory } from "./scripts/checks/lib/tree-hash"; console.log(hashDirectory("vendor/intx/db"));'

then verify with bun run check:killdates. Do this on a tree with no dist/ present, or after #503's exclusion fix has landed — otherwise the value is environment-dependent, which is the exact bug #503 fixed.

Recording this rather than leaving it to be discovered as a red check after merge.