diff --git a/src/inngest/functions/maintainer-discover.test.ts b/src/inngest/functions/maintainer-discover.test.ts index 602ce195..8e8cba24 100644 --- a/src/inngest/functions/maintainer-discover.test.ts +++ b/src/inngest/functions/maintainer-discover.test.ts @@ -1,6 +1,11 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { getInstallOctokit } from '@/lib/github/app'; -import { decideOrgGrant, reconcileGrants } from '@/lib/maintainer/discover'; +import { + decideOrgGrant, + decideRepoGrant, + reconcileGrants, + reconcileRepoGrants, +} from '@/lib/maintainer/discover'; import { cacheGet } from '@/lib/cache'; import { maintainerDiscover } from './maintainer-discover'; import { sb, wire } from './__tests__/test-helpers'; @@ -11,6 +16,7 @@ vi.mock('@/lib/maintainer/discover', () => ({ decideOrgGrant: vi.fn(), decideRepoGrant: vi.fn(), reconcileGrants: vi.fn(), + reconcileRepoGrants: vi.fn(), })); vi.mock('@/lib/cache', () => ({ cacheGet: vi.fn(), cacheSet: vi.fn() })); @@ -184,6 +190,8 @@ describe('maintainerDiscover', () => { }; vi.mocked(getInstallOctokit).mockResolvedValue(octokit as never); vi.mocked(decideOrgGrant).mockReturnValue(null); + vi.mocked(decideRepoGrant).mockReturnValue(null); + vi.mocked(reconcileRepoGrants).mockReturnValue({ toUpsert: [], toDelete: [] }); vi.mocked(reconcileGrants).mockReturnValue({ toUpsert: [], toDelete: [1], @@ -204,6 +212,107 @@ describe('maintainerDiscover', () => { ); }); + it('deletes stale per-repo grants when a user is fully revoked', async () => { + const installUsers = sb({ + delete: vi.fn().mockReturnThis(), + eq: vi.fn().mockReturnThis(), + in: vi.fn().mockResolvedValue({}), + }); + + const userRepos = sb({ + select: vi.fn().mockReturnThis(), + eq: vi.fn().mockReturnThis(), + delete: vi.fn().mockReturnThis(), + in: vi.fn().mockResolvedValue({}), + insert: vi.fn().mockResolvedValue({}), + then: (resolve: (v: unknown) => void) => + Promise.resolve({ + data: [{ repo_full_name: 'test-org/repo-1', permission_level: 'admin' }], + error: null, + }).then(resolve), + }); + + wire({ + github_installation_users: installUsers, + installation_repositories: sb({ + select: vi.fn().mockReturnThis(), + eq: vi.fn().mockResolvedValue({ + data: [{ repo_full_name: 'test-org/repo-1' }], + }), + }), + installation_user_repos: userRepos, + }); + + let selectCallCount = 0; + installUsers.select = vi.fn().mockReturnThis(); + installUsers.eq = vi.fn().mockImplementation(() => { + selectCallCount += 1; + if (selectCallCount <= 2) { + return { + ...installUsers, + then: (resolve: (v: unknown) => void) => { + if (selectCallCount === 1) { + return Promise.resolve({ + data: [ + { + installation_id: 1, + github_installations: { + id: 1, + account_type: 'Organization', + account_login: 'test-org', + uninstalled_at: null, + }, + }, + ], + }).then(resolve); + } + return Promise.resolve({ data: [] }).then(resolve); + }, + }; + } + return installUsers; + }); + + const octokit = { + orgs: { + getMembershipForUser: vi.fn().mockRejectedValue(new Error('404')), + }, + repos: { + getCollaboratorPermissionLevel: vi.fn().mockResolvedValue({ data: { permission: 'read' } }), + }, + }; + vi.mocked(getInstallOctokit).mockResolvedValue(octokit as never); + vi.mocked(decideOrgGrant).mockReturnValue(null); + vi.mocked(decideRepoGrant).mockReturnValue(null); + vi.mocked(reconcileRepoGrants).mockReturnValue({ + toUpsert: [], + toDelete: ['test-org/repo-1'], + }); + vi.mocked(reconcileGrants).mockReturnValue({ + toUpsert: [], + toDelete: [1], + }); + + const result = await run({ event: ev(), step }); + + expect(reconcileRepoGrants).toHaveBeenCalledWith( + [{ repoFullName: 'test-org/repo-1', permissionLevel: 'admin' }], + [], + ); + expect(userRepos.in).toHaveBeenCalledWith('repo_full_name', ['test-org/repo-1']); + expect(userRepos.insert).not.toHaveBeenCalled(); + expect(installUsers.in).toHaveBeenCalledWith('installation_id', [1]); + + expect(result).toEqual( + expect.objectContaining({ + user: 'u1', + installs: 1, + toUpsert: 0, + toDelete: 1, + }), + ); + }); + it('skips recently discovered users in sweep', async () => { wire({ github_installation_users: sb({ diff --git a/src/inngest/functions/maintainer-discover.ts b/src/inngest/functions/maintainer-discover.ts index 0574c600..d6455422 100644 --- a/src/inngest/functions/maintainer-discover.ts +++ b/src/inngest/functions/maintainer-discover.ts @@ -6,7 +6,9 @@ import { decideOrgGrant, decideRepoGrant, reconcileGrants, + reconcileRepoGrants, type ProposedGrant, + type ProposedRepoGrant, } from '@/lib/maintainer/discover'; import { cacheGet, cacheSet } from '@/lib/cache'; @@ -207,27 +209,49 @@ async function discoverForUser( } } - if (highestRepoGrant && repoGrants.length > 0) { - proposed.push({ - installationId: install.id, - permissionLevel: highestRepoGrant, - source: 'membership_check', - }); + // Reconcile per-repo grants unconditionally. This runs even when the user + // holds no grant on this install anymore, so rows for demoted/removed + // collaborators are deleted instead of persisting forever. + const { data: existingRepoRows } = await sb + .from('installation_user_repos') + .select('repo_full_name, permission_level') + .eq('installation_id', install.id) + .eq('user_id', userId); + + const { toUpsert: repoUpsert, toDelete: repoDelete } = reconcileRepoGrants( + (existingRepoRows ?? []).map((r) => ({ + repoFullName: r.repo_full_name, + permissionLevel: r.permission_level as 'admin' | 'maintain', + })), + repoGrants.map((g): ProposedRepoGrant => ({ repoFullName: g.repo, permissionLevel: g.perm })), + ); + if (repoDelete.length > 0) { await sb .from('installation_user_repos') .delete() .eq('installation_id', install.id) - .eq('user_id', userId); + .eq('user_id', userId) + .in('repo_full_name', repoDelete); + } + if (repoUpsert.length > 0) { await sb.from('installation_user_repos').insert( - repoGrants.map((g) => ({ + repoUpsert.map((g) => ({ installation_id: install.id, user_id: userId, - repo_full_name: g.repo, - permission_level: g.perm, + repo_full_name: g.repoFullName, + permission_level: g.permissionLevel, })), ); } + + if (highestRepoGrant) { + proposed.push({ + installationId: install.id, + permissionLevel: highestRepoGrant, + source: 'membership_check', + }); + } } const { data: existing } = await sb diff --git a/src/lib/maintainer/discover.test.ts b/src/lib/maintainer/discover.test.ts index 06e8104f..7a22bb60 100644 --- a/src/lib/maintainer/discover.test.ts +++ b/src/lib/maintainer/discover.test.ts @@ -3,6 +3,7 @@ import { decideOrgGrant, decideRepoGrant, reconcileGrants, + reconcileRepoGrants, type ExistingGrant, type ProposedGrant, } from './discover'; @@ -100,3 +101,55 @@ describe('reconcileGrants', () => { expect(toDelete).toEqual([2]); // no longer confirmed }); }); + +describe('reconcileRepoGrants', () => { + const existing = (repoFullName: string, permissionLevel: 'admin' | 'maintain') => ({ + repoFullName, + permissionLevel, + }); + + it('adds a new per-repo grant', () => { + const { toUpsert, toDelete } = reconcileRepoGrants( + [], + [{ repoFullName: 'a/repo', permissionLevel: 'admin' }], + ); + expect(toUpsert).toEqual([{ repoFullName: 'a/repo', permissionLevel: 'admin' }]); + expect(toDelete).toHaveLength(0); + }); + + it('deletes every stale row on a full revocation (empty proposed set)', () => { + const { toUpsert, toDelete } = reconcileRepoGrants( + [existing('a/repo', 'admin'), existing('b/repo', 'maintain')], + [], + ); + expect(toUpsert).toHaveLength(0); + expect(toDelete.sort()).toEqual(['a/repo', 'b/repo']); + }); + + it('drops only the repos no longer granted (partial revocation)', () => { + const { toUpsert, toDelete } = reconcileRepoGrants( + [existing('a/repo', 'admin'), existing('b/repo', 'maintain')], + [{ repoFullName: 'a/repo', permissionLevel: 'admin' }], + ); + expect(toUpsert).toHaveLength(0); + expect(toDelete).toEqual(['b/repo']); + }); + + it('keeps unchanged grants out of the upsert list (no churn)', () => { + const { toUpsert, toDelete } = reconcileRepoGrants( + [existing('a/repo', 'admin')], + [{ repoFullName: 'a/repo', permissionLevel: 'admin' }], + ); + expect(toUpsert).toHaveLength(0); + expect(toDelete).toHaveLength(0); + }); + + it('upserts when the permission level changes', () => { + const { toUpsert, toDelete } = reconcileRepoGrants( + [existing('a/repo', 'admin')], + [{ repoFullName: 'a/repo', permissionLevel: 'maintain' }], + ); + expect(toUpsert).toEqual([{ repoFullName: 'a/repo', permissionLevel: 'maintain' }]); + expect(toDelete).toHaveLength(0); + }); +}); diff --git a/src/lib/maintainer/discover.ts b/src/lib/maintainer/discover.ts index 955b8e15..dee9c080 100644 --- a/src/lib/maintainer/discover.ts +++ b/src/lib/maintainer/discover.ts @@ -73,3 +73,45 @@ export function reconcileGrants( return { toUpsert, toDelete }; } + +export type ExistingRepoGrant = { + repoFullName: string; + permissionLevel: 'admin' | 'maintain'; +}; + +export type ProposedRepoGrant = { + repoFullName: string; + permissionLevel: 'admin' | 'maintain'; +}; + +/** + * Diff existing `installation_user_repos` rows against the freshly-computed + * per-repo grants for one (installation_id, user_id) pair and return what to + * insert vs delete. Runs unconditionally so a full revocation (empty proposed + * set) deletes every stale row instead of silently keeping access. + */ +export function reconcileRepoGrants( + existing: readonly ExistingRepoGrant[], + proposed: readonly ProposedRepoGrant[], +): { toUpsert: ProposedRepoGrant[]; toDelete: string[] } { + const existingMap = new Map(); + for (const g of existing) existingMap.set(g.repoFullName, g); + + const proposedMap = new Map(); + for (const g of proposed) proposedMap.set(g.repoFullName, g); + + const toUpsert: ProposedRepoGrant[] = []; + for (const [repo, prop] of proposedMap) { + const ex = existingMap.get(repo); + if (!ex || ex.permissionLevel !== prop.permissionLevel) { + toUpsert.push(prop); + } + } + + const toDelete: string[] = []; + for (const repo of existingMap.keys()) { + if (!proposedMap.has(repo)) toDelete.push(repo); + } + + return { toUpsert, toDelete }; +}