From 57c86d386790121e5f8ec1991b460b7ecffd9358 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Tue, 15 Sep 2026 14:15:14 -0700 Subject: [PATCH 1/2] =?UTF-8?q?test(connections):=20red=20=E2=80=94=20reco?= =?UTF-8?q?ncile=20entry=20point=20must=20stay=20deleted=20(CL-7593)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../src/connections-shrink.test.ts | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 packages/connections/src/connections-shrink.test.ts diff --git a/packages/connections/src/connections-shrink.test.ts b/packages/connections/src/connections-shrink.test.ts new file mode 100644 index 0000000000..bffcd59569 --- /dev/null +++ b/packages/connections/src/connections-shrink.test.ts @@ -0,0 +1,22 @@ +// CL-7593: shrink @corbits/connections to the PKCE OAuth client + +// callback surface. Guards the cutover: the one-time CL-7242 +// duplicate-grant cleanup entry point must stay gone (module file and +// export-map subpath) so no caller can drift back onto a custom grant +// flow outside the native credential/grant routes. +import { expect, test } from "bun:test"; + +const here = new URL(".", import.meta.url); + +test("duplicate-grant reconcile entry point stays deleted", async () => { + expect( + await Bun.file( + new URL("./reconcile-duplicate-repo-grants.ts", here), + ).exists(), + ).toBe(false); + const pkg = JSON.parse( + await Bun.file(new URL("../package.json", here)).text(), + ) as { exports?: Record }; + expect( + Object.hasOwn(pkg.exports ?? {}, "./reconcile-duplicate-repo-grants"), + ).toBe(false); +}); From c254de796411cfbccca9fd2c798b5dacaadc09c9 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Tue, 15 Sep 2026 14:16:21 -0700 Subject: [PATCH 2/2] feat(connections)!: delete duplicate-grant reconcile custom flow (CL-7593) --- packages/connections/package.json | 1 - .../reconcile-duplicate-repo-grants.test.ts | 136 ------------------ .../src/reconcile-duplicate-repo-grants.ts | 55 ------- scripts/db-setup.ts | 20 +-- 4 files changed, 4 insertions(+), 208 deletions(-) delete mode 100644 packages/connections/src/reconcile-duplicate-repo-grants.test.ts delete mode 100644 packages/connections/src/reconcile-duplicate-repo-grants.ts diff --git a/packages/connections/package.json b/packages/connections/package.json index c9f9b148f4..85025d53b7 100644 --- a/packages/connections/package.json +++ b/packages/connections/package.json @@ -14,7 +14,6 @@ "./mcp-presets": "./src/mcp-presets.ts", "./connect-github-routes": "./src/connect-github-routes.ts", "./connect-github-setup": "./src/connect-github-setup.ts", - "./reconcile-duplicate-repo-grants": "./src/reconcile-duplicate-repo-grants.ts", "./credential-test": "./src/credential-test.ts", "./token-session": "./src/token-session.ts", "./model-capability": "./src/model-capability.ts", diff --git a/packages/connections/src/reconcile-duplicate-repo-grants.test.ts b/packages/connections/src/reconcile-duplicate-repo-grants.test.ts deleted file mode 100644 index e6a14cae40..0000000000 --- a/packages/connections/src/reconcile-duplicate-repo-grants.test.ts +++ /dev/null @@ -1,136 +0,0 @@ -// DB-gated: skipped when no DATABASE_URL is reachable. Runs against -// its own scratch database. -// -// CL-7242: `reconcileDuplicateRepoGrants` is the one-time cleanup for -// duplicate `repo:` grants a database can already carry -// from before the `repo_review_lease` fix started preventing new -// ones. Proves it keeps exactly one grant per group (never zero, -// matching the "a repo never ends up with fewer grants than it had" -// bar), never touches an unrelated grant, and is safe to re-run. -import { afterAll, beforeAll, expect, test } from "bun:test"; -import postgres from "postgres"; - -import { runMigrations } from "@intx/db"; - -import { e2eDatabaseUrl } from "../../../scripts/e2e/database-url"; -import { reconcileDuplicateRepoGrants } from "./reconcile-duplicate-repo-grants"; -import { dbGate } from "../../../scripts/e2e/db-gate"; - -function scratchUrlFor(e2eUrl: string): string { - const url = new URL(e2eUrl); - const database = url.pathname.replace(/^\//, ""); - url.pathname = `/${database}_reconcile_duplicate_repo_grants_test`; - return url.toString(); -} - -const databaseUrl = e2eDatabaseUrl(); -const describeIfDb = dbGate(databaseUrl, import.meta.path); - -const TENANT_ID = "tnt_reconcile"; -const ROLE_ID = "role_reconcile_member"; - -describeIfDb("reconcileDuplicateRepoGrants", () => { - const scratchUrl = scratchUrlFor( - databaseUrl ?? "postgres://localhost:5432/unused", - ); - const scratchTarget = new URL(scratchUrl); - const scratchDatabase = scratchTarget.pathname.replace(/^\//, ""); - - beforeAll(async () => { - const maintenanceUrl = new URL(scratchUrl); - maintenanceUrl.pathname = "/postgres"; - const maintenance = postgres(maintenanceUrl.toString(), { - max: 1, - onnotice: () => undefined, - }); - try { - await maintenance.unsafe(`DROP DATABASE IF EXISTS "${scratchDatabase}"`); - await maintenance.unsafe(`CREATE DATABASE "${scratchDatabase}"`); - } finally { - await maintenance.end(); - } - const parsed = new URL(scratchUrl); - await runMigrations( - { - host: parsed.hostname, - port: Number(parsed.port || 5432), - user: decodeURIComponent(parsed.username), - password: decodeURIComponent(parsed.password), - database: scratchDatabase, - }, - { schema: "public" }, - ); - const sql = postgres(scratchUrl, { max: 1, onnotice: () => undefined }); - try { - await sql`INSERT INTO "tenant" (id, name, slug, domain) VALUES (${TENANT_ID}, 'Acme', 'acme-reconcile', 'acme-reconcile.example')`; - await sql`INSERT INTO "role" (id, tenant_id, name) VALUES (${ROLE_ID}, ${TENANT_ID}, 'member')`; - } finally { - await sql.end(); - } - }); - - afterAll(async () => { - const maintenanceUrl = new URL(scratchUrl); - maintenanceUrl.pathname = "/postgres"; - const maintenance = postgres(maintenanceUrl.toString(), { - max: 1, - onnotice: () => undefined, - }); - try { - await maintenance.unsafe(`DROP DATABASE IF EXISTS "${scratchDatabase}"`); - } finally { - await maintenance.end(); - } - }); - - test("keeps exactly one grant per duplicated (tenant, resource, action) and leaves other grants alone", async () => { - const sql = postgres(scratchUrl, { max: 1, onnotice: () => undefined }); - try { - // Two duplicate repo grants from the pre-lease race, one pair from - // each generation of `mintRepoGrant`: the direct-insert path this - // PR replaced wrote `'system'`-origin rows; the current - // `mintRepoGrantViaHttp` path writes `'creator'`-origin rows. - await sql` - INSERT INTO "grant" (id, tenant_id, role_id, resource, action, effect, origin, created_at) - VALUES - ('grant_dup_1', ${TENANT_ID}, ${ROLE_ID}, 'repo:acme/widgets', 'read', 'allow', 'system', now() - interval '1 hour'), - ('grant_dup_2', ${TENANT_ID}, ${ROLE_ID}, 'repo:acme/widgets', 'read', 'allow', 'creator', now()) - `; - // A non-duplicated repo grant, a non-repo system grant, and a - // repo grant with an origin this feature never writes -- none - // should ever be touched. - await sql` - INSERT INTO "grant" (id, tenant_id, role_id, resource, action, effect, origin) - VALUES - ('grant_solo', ${TENANT_ID}, ${ROLE_ID}, 'repo:acme/gadgets', 'read', 'allow', 'creator'), - ('grant_unrelated', ${TENANT_ID}, ${ROLE_ID}, 'workbench:room-1', 'read', 'allow', 'system'), - ('grant_other_origin', ${TENANT_ID}, ${ROLE_ID}, 'repo:acme/gizmos', 'read', 'allow', 'role') - `; - - const first = await reconcileDuplicateRepoGrants(scratchUrl); - expect(first.removedIds).toEqual(["grant_dup_2"]); - - const widgetsRows = await sql` - SELECT id FROM "grant" WHERE tenant_id = ${TENANT_ID} AND resource = 'repo:acme/widgets' - `; - expect(widgetsRows).toHaveLength(1); - expect(widgetsRows[0]?.["id"]).toBe("grant_dup_1"); - - const untouchedRows = await sql` - SELECT id FROM "grant" WHERE id IN ('grant_solo', 'grant_unrelated', 'grant_other_origin') - ORDER BY id - `; - expect(untouchedRows.map((r) => r["id"])).toEqual([ - "grant_other_origin", - "grant_solo", - "grant_unrelated", - ]); - - // Idempotent: nothing left to remove on a second run. - const second = await reconcileDuplicateRepoGrants(scratchUrl); - expect(second.removedIds).toEqual([]); - } finally { - await sql.end(); - } - }); -}); diff --git a/packages/connections/src/reconcile-duplicate-repo-grants.ts b/packages/connections/src/reconcile-duplicate-repo-grants.ts deleted file mode 100644 index 2b6f66f7d9..0000000000 --- a/packages/connections/src/reconcile-duplicate-repo-grants.ts +++ /dev/null @@ -1,55 +0,0 @@ -// One-time cleanup for pre-existing duplicate `repo:` -// grants a database can already carry from CL-7242's check-then-act -// race, before this fix's lease (`@corbits/webhook-triggers`' -// `repo_review_lease`) started preventing new ones. -// -// This is ordinary application code, not a migration: a plain DELETE -// against Interchange's own `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), so this creates no re-pin -// debt: a DDL delta would need hand-reapplying at every future -// `vendor/intx/*` re-pin, but a DML statement runs once and leaves -// nothing behind for a re-pin to reconcile. Deliberately safe to -// re-run — a database with no duplicates left just deletes nothing. -import postgres from "postgres"; - -export interface ReconcileDuplicateRepoGrantsReport { - /** Ids of the duplicate rows removed, kept for the caller to log. */ - readonly removedIds: readonly string[]; -} - -/** - * Keeps the lowest-`id` row per `(tenant_id, resource, action)` among - * `repo:`-prefixed grants with an origin `mintRepoGrant` could plausibly - * have written and deletes the rest. `'creator'` is what the current - * `mintRepoGrantViaHttp` path (`apps/hub/src/native-repo-grants.ts`) - * writes; `'system'` is what the direct-insert path this PR replaced - * used to write, so a database carrying duplicates from either - * generation of this feature gets them cleaned up. Never touches any - * other grant family. - */ -export async function reconcileDuplicateRepoGrants( - databaseUrl: string, -): Promise { - const sql = postgres(databaseUrl, { max: 1, onnotice: () => undefined }); - try { - const removed = await sql<{ id: string }[]>` - DELETE FROM "grant" AS g - USING "grant" AS older - WHERE g.origin IN ('system', 'creator') - AND g.resource LIKE 'repo:%' - AND older.origin IN ('system', 'creator') - AND older.resource LIKE 'repo:%' - AND g.tenant_id = older.tenant_id - AND g.resource = older.resource - AND g.action = older.action - AND older.id < g.id - RETURNING g.id - `; - return { removedIds: removed.map((row) => row.id) }; - } finally { - await sql.end(); - } -} diff --git a/scripts/db-setup.ts b/scripts/db-setup.ts index f4010752cc..338488668e 100644 --- a/scripts/db-setup.ts +++ b/scripts/db-setup.ts @@ -30,7 +30,6 @@ import { readdir } from "node:fs/promises"; import { applyChatMigrations } from "../packages/chat/src/migrations"; import { applyWebhookTriggersMigrations } from "../packages/webhook-triggers/src/migrations"; -import { reconcileDuplicateRepoGrants } from "../packages/connections/src/reconcile-duplicate-repo-grants"; import { applyNotifyMigrations } from "../packages/notify/src/migrations"; import { applyInboxMigrations, @@ -107,21 +106,10 @@ async function applyInstalledPackageMigrations( } } - // CL-7242: not a package migration (no schema, no ledger, no DDL at - // all) -- a plain, always-safe-to-re-run DELETE against the - // platform's own `grant` table through @intx/db's published export, - // cleaning up any repo grants CL-7242's race duplicated before this - // fix's lease started preventing new ones. See - // reconcile-duplicate-repo-grants.ts for why this is DML, not DDL, - // and why that distinction is what keeps it off the vendored-delta - // ledger. - const { removedIds } = await reconcileDuplicateRepoGrants(databaseUrl); - if (removedIds.length > 0) { - console.log( - `db-setup: removed ${removedIds.length} duplicate repo grant(s): ` + - removedIds.join(", "), - ); - } + // CL-7593: the one-time CL-7242 duplicate-grant cleanup ran its era + // (the repo_review_lease now prevents new duplicates at write time), + // so db-setup no longer performs it: setup applies migrations only, + // and grant state stays on the native grant routes. await dropRoutinesSchemaAfterDigestHandoff(databaseUrl); }