diff --git a/apps/web/src/app/api/integrations/github/callback/route.test.ts b/apps/web/src/app/api/integrations/github/callback/route.test.ts index f743ef0e837..a442105bee9 100644 --- a/apps/web/src/app/api/integrations/github/callback/route.test.ts +++ b/apps/web/src/app/api/integrations/github/callback/route.test.ts @@ -93,7 +93,6 @@ jest.mock('@/lib/integrations/db/platform-integrations', () => ({ findGitHubBotLinkIntegrations: jest.fn(), findIntegrationByInstallationId: jest.fn(), findIntegrationByInstallationIdForOwner: jest.fn(), - findPendingInstallationByRequesterId: jest.fn(), upsertPlatformIntegrationForOwner: jest.fn(async () => ({ ok: true })), })); jest.mock('@/lib/integrations/db/github-installations', () => ({ diff --git a/apps/web/src/components/integrations/GitHubIntegrationDetails.tsx b/apps/web/src/components/integrations/GitHubIntegrationDetails.tsx index ceca2108f54..d965fcb14d6 100644 --- a/apps/web/src/components/integrations/GitHubIntegrationDetails.tsx +++ b/apps/web/src/components/integrations/GitHubIntegrationDetails.tsx @@ -596,6 +596,10 @@ function GitHubIntegrationDetailsContent({ }; const handleCancelPending = async () => { + // Cancel exactly the pending row this view rendered; the id is required so + // the request cannot fall back to another pending request. + const pendingIntegrationId = installationData?.installation?.id; + if (!pendingIntegrationId) return; if ( await confirm({ title: 'Cancel this installation request?', @@ -605,17 +609,22 @@ function GitHubIntegrationDetailsContent({ destructive: true, }) ) { - cancelPendingInstallation.mutate(managementInput, { - onSuccess: async () => { - toast.success('Installation request cancelled'); - await refetch(); - }, - onError: error => { - toast.error('Failed to cancel installation request', { - description: error.message, - }); - }, - }); + cancelPendingInstallation.mutate( + organizationId + ? { organizationId, integrationId: pendingIntegrationId } + : { integrationId: pendingIntegrationId }, + { + onSuccess: async () => { + toast.success('Installation request cancelled'); + await refetch(); + }, + onError: error => { + toast.error('Failed to cancel installation request', { + description: error.message, + }); + }, + } + ); } }; diff --git a/apps/web/src/lib/integrations/db/platform-integrations.ts b/apps/web/src/lib/integrations/db/platform-integrations.ts index 6a0791e09c5..68c7b113b91 100644 --- a/apps/web/src/lib/integrations/db/platform-integrations.ts +++ b/apps/web/src/lib/integrations/db/platform-integrations.ts @@ -823,27 +823,13 @@ export async function findPendingInstallations() { } /** - * Find a pending installation by GitHub requester ID - */ -export async function findPendingInstallationByRequesterId(githubRequesterId: string) { - const [result] = await db - .select() - .from(platform_integrations) - .where( - and( - eq(platform_integrations.platform, PLATFORM.GITHUB), - eq(platform_integrations.platform_requester_account_id, githubRequesterId), - eq(platform_integrations.integration_status, INTEGRATION_STATUS.PENDING), - isNull(platform_integrations.platform_installation_id) - ) - ) - .limit(1); - - return result || null; -} - -/** - * Find a pending installation by Kilo user ID + * Find the pending installation a Kilo user most recently requested. + * + * Several pending rows can exist for one user (the pending-target unique + * indexes are per owner, app type, and GitHub target account), so the lookup + * orders explicitly: newest request first, ties broken by id. Without the + * order Postgres may return any matching row, and a resumed view or a cancel + * could address a different request than the one the user saw. */ export async function findPendingInstallationByKiloUserId(kiloUserId: string) { const [result] = await db @@ -857,6 +843,7 @@ export async function findPendingInstallationByKiloUserId(kiloUserId: string) { isNull(platform_integrations.platform_installation_id) ) ) + .orderBy(desc(platform_integrations.created_at), desc(platform_integrations.id)) .limit(1); return result || null; diff --git a/apps/web/src/lib/integrations/github-apps-service.test.ts b/apps/web/src/lib/integrations/github-apps-service.test.ts index 91332ad31a4..f31b04d7821 100644 --- a/apps/web/src/lib/integrations/github-apps-service.test.ts +++ b/apps/web/src/lib/integrations/github-apps-service.test.ts @@ -1,13 +1,20 @@ import { describe, expect, it } from '@jest/globals'; -import { organizations, platform_integrations } from '@kilocode/db/schema'; +import { kilocode_users, organizations, platform_integrations } from '@kilocode/db/schema'; import { eq } from 'drizzle-orm'; import { db } from '@kilocode/web-shared/lib/drizzle'; +import { insertTestUser } from '@kilocode/web-shared/tests/helpers/user.helper'; import { getIntegrationForOrganization, getIntegrationForOwner, getPrimaryGitHubIntegrationForOrganization, } from '@/lib/integrations/db/platform-integrations'; -import { getInstallation, isInstallationGoneError, updateModel } from './github-apps-service'; +import { + cancelPendingInstallation, + checkUserPendingInstallation, + getInstallation, + isInstallationGoneError, + updateModel, +} from './github-apps-service'; describe('getInstallation', () => { it('prefers a healthy installation when the owner has multiple GitHub rows', async () => { @@ -204,6 +211,141 @@ describe('updateModel', () => { }); }); +describe('pending installation selection', () => { + type PendingRow = typeof platform_integrations.$inferInsert; + + function pendingRow(input: { userId: string; accountId: string; createdAt: string }): PendingRow { + return { + owned_by_user_id: input.userId, + platform: 'github', + integration_type: 'app', + platform_installation_id: null, + integration_status: 'pending', + github_app_type: 'standard', + platform_account_id: input.accountId, + platform_account_login: `target-${input.accountId}`, + kilo_requester_user_id: input.userId, + installed_at: input.createdAt, + created_at: input.createdAt, + updated_at: input.createdAt, + }; + } + + async function cleanupUser(userId: string): Promise { + await db + .delete(platform_integrations) + .where(eq(platform_integrations.owned_by_user_id, userId)); + await db.delete(kilocode_users).where(eq(kilocode_users.id, userId)); + } + + it('returns the newest pending request regardless of the order rows were written', async () => { + const user = await insertTestUser(); + try { + // The older request is written first, so an unordered SELECT returns it. + const rows = await db + .insert(platform_integrations) + .values([ + pendingRow({ + userId: user.id, + accountId: 'older', + createdAt: '2026-01-01T00:00:00.000Z', + }), + pendingRow({ + userId: user.id, + accountId: 'newer', + createdAt: '2026-02-01T00:00:00.000Z', + }), + ]) + .returning(); + + const [older, newer] = rows; + + await expect(checkUserPendingInstallation(user.id)).resolves.toMatchObject({ id: newer.id }); + + // Touching the older row moves it to the end of the heap, flipping the + // physical order a second time; the answer must not follow it. + await db + .update(platform_integrations) + .set({ updated_at: '2026-03-01T00:00:00.000Z' }) + .where(eq(platform_integrations.id, older.id)); + + await expect(checkUserPendingInstallation(user.id)).resolves.toMatchObject({ id: newer.id }); + } finally { + await cleanupUser(user.id); + } + }); + + it('breaks equal pending creation times by descending integration id', async () => { + const user = await insertTestUser(); + const [lowerId, higherId] = [crypto.randomUUID(), crypto.randomUUID()].sort(); + const createdAt = '2026-02-01T00:00:00.000Z'; + try { + await db.insert(platform_integrations).values([ + { + ...pendingRow({ userId: user.id, accountId: 'lower-id', createdAt }), + id: lowerId, + }, + { + ...pendingRow({ userId: user.id, accountId: 'higher-id', createdAt }), + id: higherId, + }, + ]); + + await expect(checkUserPendingInstallation(user.id)).resolves.toMatchObject({ id: higherId }); + + await cancelPendingInstallation({ type: 'user', id: user.id }, higherId); + + await expect(checkUserPendingInstallation(user.id)).resolves.toMatchObject({ id: lowerId }); + } finally { + await cleanupUser(user.id); + } + }); + + it('cancels exactly the named pending request and refuses any other id', async () => { + const user = await insertTestUser(); + const otherUser = await insertTestUser(); + try { + const rows = await db + .insert(platform_integrations) + .values([ + pendingRow({ + userId: user.id, + accountId: 'first', + createdAt: '2026-01-01T00:00:00.000Z', + }), + pendingRow({ + userId: user.id, + accountId: 'second', + createdAt: '2026-02-01T00:00:00.000Z', + }), + ]) + .returning(); + const [first, second] = rows; + + await cancelPendingInstallation({ type: 'user', id: user.id }, first.id); + + const remaining = await db + .select({ id: platform_integrations.id }) + .from(platform_integrations) + .where(eq(platform_integrations.owned_by_user_id, user.id)); + expect(remaining).toEqual([{ id: second.id }]); + + await expect( + cancelPendingInstallation({ type: 'user', id: otherUser.id }, second.id) + ).rejects.toMatchObject({ code: 'NOT_FOUND' }); + + const stillPending = await db + .select({ id: platform_integrations.id }) + .from(platform_integrations) + .where(eq(platform_integrations.owned_by_user_id, user.id)); + expect(stillPending).toEqual([{ id: second.id }]); + } finally { + await cleanupUser(user.id); + await cleanupUser(otherUser.id); + } + }); +}); + describe('isInstallationGoneError', () => { it('should return true for 404 Not Found errors', () => { const error = { status: 404, message: 'Not Found' }; diff --git a/apps/web/src/lib/integrations/github-apps-service.ts b/apps/web/src/lib/integrations/github-apps-service.ts index 5b6eb3353ec..079e62e5614 100644 --- a/apps/web/src/lib/integrations/github-apps-service.ts +++ b/apps/web/src/lib/integrations/github-apps-service.ts @@ -205,19 +205,22 @@ export async function listRepositories( } /** - * Cancel a pending installation + * Cancel one pending installation request. + * + * The id is required: a user can hold several pending requests, so the caller + * must name the row it displayed. An id that is not a pending request for this + * owner is refused rather than replaced by another matching row. */ -export async function cancelPendingInstallation(owner: Owner, integrationId?: string) { +export async function cancelPendingInstallation(owner: Owner, integrationId: string) { const ownershipCondition = ownerCondition(owner); - // Find the pending installation const [integration] = await db .select() .from(platform_integrations) .where( and( ownershipCondition, - ...(integrationId ? [eq(platform_integrations.id, integrationId)] : []), + eq(platform_integrations.id, integrationId), eq(platform_integrations.platform, 'github'), eq(platform_integrations.integration_status, INTEGRATION_STATUS.PENDING), isNull(platform_integrations.platform_installation_id) diff --git a/apps/web/src/routers/github-apps-router.test.ts b/apps/web/src/routers/github-apps-router.test.ts index 6072fa934f5..00edd05dc4a 100644 --- a/apps/web/src/routers/github-apps-router.test.ts +++ b/apps/web/src/routers/github-apps-router.test.ts @@ -24,6 +24,11 @@ type InstallationDetails = { const mockGetIntegrationForOwner = jest.fn<(owner: Owner, platform: string) => Promise>(); +// Records the id the router asks for, so a test can prove the lookup matched +// the requested request instead of returning any row of the owner. +const mockGetGitHubIntegrationById = jest.fn< + (owner: Owner, integrationId: string) => Promise +>(async (owner, _integrationId) => mockGetIntegrationForOwner(owner, 'github')); const mockFindIntegrationByInstallationId = jest.fn< ( @@ -56,6 +61,12 @@ const mockSeedUserGithubToken = >(); const mockListIntegrations = jest.fn<(owner: Owner) => Promise>(); const mockUninstallApp = jest.fn<() => Promise<{ success: boolean; message: string }>>(); +const mockCheckUserPendingInstallation = + jest.fn< + (userId: string) => Promise<{ id: string; owned_by_organization_id: string | null } | null> + >(); +const mockCancelPendingInstallation = + jest.fn<(owner: Owner, integrationId: string) => Promise<{ success: boolean }>>(); const mockEnsureOrganizationAccess = jest.fn< ( @@ -82,6 +93,9 @@ const mockBindGitHubIntegrationToCanonicalInstallation = jest.fn(); jest.mock('@/lib/integrations/github-apps-service', () => ({ listIntegrations: (owner: Owner) => mockListIntegrations(owner), uninstallApp: () => mockUninstallApp(), + checkUserPendingInstallation: (userId: string) => mockCheckUserPendingInstallation(userId), + cancelPendingInstallation: (owner: Owner, integrationId: string) => + mockCancelPendingInstallation(owner, integrationId), })); jest.mock('@kilocode/web-shared/routers/organizations/utils', () => ({ @@ -106,8 +120,8 @@ jest.mock('@/lib/integrations/github/install-state', () => ({ jest.mock('@/lib/integrations/db/platform-integrations', () => ({ getIntegrationForOwner: (owner: Owner, platform: string) => mockGetIntegrationForOwner(owner, platform), - getGitHubIntegrationById: (_owner: Owner, _integrationId: string) => - mockGetIntegrationForOwner(_owner, 'github'), + getGitHubIntegrationById: (owner: Owner, integrationId: string) => + mockGetGitHubIntegrationById(owner, integrationId), upsertPlatformIntegrationForOwner: (owner: Owner, details: Record) => mockUpsertPlatformIntegrationForOwner(owner, details), updateRepositoriesForIntegration: (integrationId: string, repositories: unknown[]) => @@ -189,6 +203,15 @@ let createCaller: (ctx: { user: User }) => { organizationId: string; integrationId: string; }) => Promise<{ success: boolean }>; + checkUserPendingInstallation: (input?: { organizationId?: string }) => Promise<{ + hasPending: boolean; + pendingOrganizationId: string | null; + pendingIntegrationId: string | null; + }>; + cancelPendingInstallation: (input: { + organizationId?: string; + integrationId: string; + }) => Promise<{ success: boolean }>; }; beforeAll(async () => { @@ -717,3 +740,76 @@ describe('githubAppsRouter.devAddInstallation', () => { expect(details).not.toHaveProperty('githubUserId'); }); }); + +describe('githubAppsRouter pending installation cancel', () => { + beforeEach(() => { + jest.clearAllMocks(); + mockEnsureOrganizationAccess.mockResolvedValue('member'); + mockGetIntegrationForOwner.mockResolvedValue({ + id: integrationId, + platform_installation_id: '98765', + platform_account_login: 'existing-org', + github_app_type: 'standard', + }); + mockCancelPendingInstallation.mockResolvedValue({ success: true }); + }); + + it('reports the pending row id it describes', async () => { + mockCheckUserPendingInstallation.mockResolvedValue({ + id: integrationId, + owned_by_organization_id: null, + }); + const caller = createCaller({ user: { id: 'user-1', is_admin: false } as User }); + + await expect(caller.checkUserPendingInstallation()).resolves.toEqual({ + hasPending: true, + pendingOrganizationId: null, + pendingIntegrationId: integrationId, + }); + }); + + it('rejects a cancel without an integration id instead of choosing a pending row', async () => { + const caller = createCaller({ user: { id: 'user-1', is_admin: false } as User }); + + await expect( + caller.cancelPendingInstallation({} as { integrationId: string }) + ).rejects.toMatchObject({ code: 'BAD_REQUEST' }); + + expect(mockCancelPendingInstallation).not.toHaveBeenCalled(); + }); + + it('cancels the named request for the requesting user', async () => { + const caller = createCaller({ user: { id: 'user-1', is_admin: false } as User }); + + await expect(caller.cancelPendingInstallation({ integrationId })).resolves.toEqual({ + success: true, + }); + + expect(mockCancelPendingInstallation).toHaveBeenCalledWith( + { type: 'user', id: 'user-1' }, + integrationId + ); + }); + + it('refuses an id that matches no integration for the owner', async () => { + const otherPendingId = '00000000-0000-4000-8000-0000000000aa'; + // The owner does have an integration, but not the one in the request. + mockGetGitHubIntegrationById.mockImplementationOnce(async (_owner, id) => + id === integrationId + ? { + id: integrationId, + platform_installation_id: '98765', + platform_account_login: 'existing-org', + github_app_type: 'standard', + } + : null + ); + const caller = createCaller({ user: { id: 'user-1', is_admin: false } as User }); + + await expect( + caller.cancelPendingInstallation({ integrationId: otherPendingId }) + ).rejects.toMatchObject({ code: 'NOT_FOUND' }); + + expect(mockCancelPendingInstallation).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/web/src/routers/github-apps-router.ts b/apps/web/src/routers/github-apps-router.ts index 1f496963a07..47c21e598b4 100644 --- a/apps/web/src/routers/github-apps-router.ts +++ b/apps/web/src/routers/github-apps-router.ts @@ -449,12 +449,16 @@ export const githubAppsRouter = createTRPCRouter({ return { hasPending: false, pendingOrganizationId: null, + pendingIntegrationId: null, }; } return { hasPending: true, pendingOrganizationId: pendingInstallation.owned_by_organization_id, + // Names the exact pending row this response describes, so a cancel is + // pinned to it instead of to whichever pending row matches the owner. + pendingIntegrationId: pendingInstallation.id, }; }), @@ -533,43 +537,36 @@ export const githubAppsRouter = createTRPCRouter({ return githubAppsService.listBranches(owner, input.integrationId, input.repositoryFullName); }), - // Cancel pending installation + // Cancel one pending installation request, named by its integration id so a + // user with several pending requests cancels the one the client displayed. cancelPendingInstallation: baseProcedure .input( - z - .object({ - organizationId: z.string().uuid().optional(), - integrationId: z.string().uuid().optional(), - }) - .optional() + z.object({ + organizationId: z.string().uuid().optional(), + integrationId: z.string().uuid(), + }) ) .mutation(async ({ ctx, input }) => { - let role: Awaited> | null = null; - if (input?.organizationId) { - role = await ensureOrganizationAccess(ctx, input.organizationId); - if (!input.integrationId) { - throw new TRPCError({ code: 'BAD_REQUEST', message: 'Integration ID is required' }); - } + const role = input.organizationId + ? await ensureOrganizationAccess(ctx, input.organizationId) + : null; + const owner = resolveOwner(ctx, input.organizationId); + const integration = await getGitHubIntegrationById(owner, input.integrationId); + if (!integration) { + throw new TRPCError({ code: 'NOT_FOUND', message: 'Pending installation not found' }); } - const owner = resolveOwner(ctx, input?.organizationId); - if (input?.integrationId) { - const integration = await getGitHubIntegrationById(owner, input.integrationId); - if (!integration) { - throw new TRPCError({ code: 'NOT_FOUND', message: 'Pending installation not found' }); - } - const canCancel = - !input.organizationId || - ctx.user.is_admin || - role === 'owner' || - role === 'admin' || - integration.kilo_requester_user_id === ctx.user.id; - if (!canCancel) { - throw new TRPCError({ code: 'FORBIDDEN', message: 'Cannot cancel this request' }); - } + const canCancel = + !input.organizationId || + ctx.user.is_admin || + role === 'owner' || + role === 'admin' || + integration.kilo_requester_user_id === ctx.user.id; + if (!canCancel) { + throw new TRPCError({ code: 'FORBIDDEN', message: 'Cannot cancel this request' }); } - const result = await githubAppsService.cancelPendingInstallation(owner, input?.integrationId); + const result = await githubAppsService.cancelPendingInstallation(owner, input.integrationId); - if (input?.organizationId) { + if (input.organizationId) { await createAuditLog({ organization_id: input.organizationId, action: 'organization.settings.change', diff --git a/services/kilo-mcp/catalog.json b/services/kilo-mcp/catalog.json index 78f7e9c812d..1ab51f7c08b 100644 --- a/services/kilo-mcp/catalog.json +++ b/services/kilo-mcp/catalog.json @@ -9375,7 +9375,10 @@ "format": "uuid", "pattern": "^([0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[1-8][0-9a-fA-F]{3}-[89abAB][0-9a-fA-F]{3}-[0-9a-fA-F]{12}|00000000-0000-0000-0000-000000000000|ffffffff-ffff-ffff-ffff-ffffffffffff)$" } - } + }, + "required": [ + "integrationId" + ] }, "tags": [ "githubapps",