fix: revoke stale per-repo grants (#874)

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({

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

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);
+  });
+});

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<string, ExistingRepoGrant>();
+  for (const g of existing) existingMap.set(g.repoFullName, g);
+
+  const proposedMap = new Map<string, ProposedRepoGrant>();
+  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 };
+}