From bac60d9d3164c2dee10df7c96d2cbabbde2fa686 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Mon, 7 Sep 2026 09:07:09 -0500 Subject: [PATCH] fix: preserve GitHub sign-in and show connected repository access (#12993) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - GitHub connections give agents an account with selected repository access. > - Fresh local instances enroll with production Paperclip Cloud. > - Enrollment could finish while the GitHub OAuth profile remained disabled. > - Setup then switched to a personal access token form without explanation. > - This change preserves sign-in intent and shows the connected account and repositories. ## Linked Issues or Issue Description Related: #12907, #12943, #12947. Existing open GitHub connection work was checked. No duplicate was found. **What happened?** After Cloud enrollment, a fresh test-drive asked for a GitHub key. Production did not advertise the managed GitHub profile. Staging did. The permissions page also omitted the authenticated username and repository names. **Expected behavior** Continue with GitHub OAuth when available. Explain unavailable sign-in and allow retry otherwise. Show the GitHub username and complete accessible repository list. **Steps to reproduce** Start a fresh test-drive. Choose GitHub and complete instance enrollment while the Cloud GitHub profile is disabled. Open an existing GitHub connection's permissions page. ## What Changed - Preserve managed sign-in intent when the gallery omits its profile. - Refresh the selected gallery entry on retry without resetting the audience. - Fetch all pages of GitHub installations and repositories. - Store only repository IDs, full names, and installation IDs in grant metadata. - Show the GitHub username, repository list, management link, and refresh action. - Discard the repository snapshot after newer installation lifecycle events. Preserve snapshots verified after delayed events. - Lock and re-read grant metadata when applying installation events or saving refreshed access. Patch only webhook fields for other events. Reject snapshots if access changed during the external fetch, using unique access revisions even when timestamps collide. - Show repository installation recovery for managed OAuth even when the app also offers an advanced PAT method. - Update tests and the GitHub connection runbook. No SQL migration is required. ## Verification - Local typecheck, build, and token gates passed. All latest-head CI gates passed, including the complete test matrix and browser suites. Greptile is 5/5 with no unresolved findings. - All 382 focused setup, permissions, metadata, service, and webhook tests passed across final runs. One socket-hang-up test passed on rerun with the full service suite. Final service, metadata, and webhook checks passed all 230 tests. - The broad local suite was stopped after failures. Seven workspace-runtime exposure and control-conflict failures reproduce on base commit `54a99d884`. The broad run also overlapped local iteration; final focused tests and clean-checkout CI are tracked separately. - Browser: a fresh production-backed instance completed enrollment, retried after profile enablement, reached GitHub consent, recovered from a missing installation, and completed OAuth. - Browser: the permissions page showed the authenticated username and the selected private test repository. A real `get_me` call returned the same account. Reading the selected repository passed; reading an unselected private repository failed with 404. - Browser: a second fresh instance completed enrollment and OAuth without a PAT form or unavailable state. Its username and repository list survived reload and refresh. A real get_me call on the final code returned the displayed account. ## Risks - Repository names are now stored in company-scoped grant metadata and shown with that credential. They are display data, not authorization data. - Large selections require more GitHub API calls. A failed later page rejects the refresh rather than reporting a partial list. - Older grants and webhook-invalidated snapshots require Refresh access to load the list. - Cloud profile enablement is separate deployment configuration. This PR does not change OAuth scopes or GitHub App permissions. ## Model Used OpenAI GPT-6 (`gpt-6-astra`) via Codex. Reasoning, code execution, and browser tools were used. The exact context window size was not exposed. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- doc/connections/GITHUB.md | 27 ++++-- packages/db/src/schema/tool_access.ts | 3 + packages/shared/src/types/tool-access.ts | 3 + .../github-connection-events.test.ts | 24 ++++- .../__tests__/github-grant-metadata.test.ts | 67 +++++++++----- .../src/__tests__/tool-access-service.test.ts | 35 +++++++- .../src/services/github-connection-events.ts | 47 ++++++++-- server/src/services/tool-access.ts | 88 +++++++++++++------ .../connections/ConnectionSetupFlow.tsx | 50 ++++++++--- ui/src/pages/apps/AppDetail.test.tsx | 27 +++++- ui/src/pages/apps/AppsConnect.test.tsx | 49 +++++++---- .../apps/app-detail/IdentitiesSection.tsx | 23 ++++- 12 files changed, 340 insertions(+), 103 deletions(-) diff --git a/doc/connections/GITHUB.md b/doc/connections/GITHUB.md index eb894ce016..a85a062c5b 100644 --- a/doc/connections/GITHUB.md +++ b/doc/connections/GITHUB.md @@ -69,15 +69,26 @@ installation-health failure, not as token expiry. ## Repository access -OAuth completion verifies `/user`, `/user/installations`, and each -installation's accessible repository count. Setup remains incomplete until at -least one installation and repository are available. Paperclip stores user and -installation summaries, not a repository-name cache. GitHub stays authoritative: -removed repository access fails immediately even if a displayed count is stale. +OAuth completion verifies `/user`, every page of `/user/installations`, and every +page of each installation's accessible repositories. Setup remains incomplete +until at least one installation and repository are available. Paperclip stores +the authenticated username and a grant-scoped display snapshot containing only +repository IDs, full names, and installation IDs. GitHub stays authoritative: +this snapshot never authorizes repository access. -The Apps UI links to GitHub's installation management page and offers -**Refresh access**. Selected repositories are recommended. Choosing all -repositories requires an explicit warning in setup. +The permissions page shows the authenticated GitHub account and the complete +accessible repository list. **Refresh access** reloads it from GitHub. Older +grants and grants invalidated by newer installation lifecycle events prompt for a +refresh instead of presenting a stale list. The page links to GitHub's +installation management page. Selected repositories are recommended; all- +repository access retains its warning. + +Fresh local test-drives use production Paperclip Cloud. Instance enrollment +and provider enablement are separate: enrollment alone does not enable GitHub +OAuth. Production must advertise the `github.code` profile (see Cloud's +`docs/github-connector-deploy-bootstrap.md`). If it is unavailable, setup +preserves the sign-in intent and offers a retry instead of silently switching +to a personal access token. A successful retry preserves the chosen audience. ## Webhooks diff --git a/packages/db/src/schema/tool_access.ts b/packages/db/src/schema/tool_access.ts index 939921e26d..59261f2241 100644 --- a/packages/db/src/schema/tool_access.ts +++ b/packages/db/src/schema/tool_access.ts @@ -194,9 +194,12 @@ export const connectionGrants = pgTable( repositorySelection: "all" | "selected" | "mixed" | "none"; installationIds: string[]; installationOwnerLogins: string[]; + /** Repository metadata visible to this credential; refreshed from GitHub. */ + repositories?: Array<{ id: string; fullName: string; installationId: string }>; installationUrl?: string; managementUrl?: string; appSlug?: string; + accessRevision?: string; lastAccessRefreshAt?: string; lastWebhookAt?: string; webhookHealth?: "pending" | "healthy" | "unhealthy"; diff --git a/packages/shared/src/types/tool-access.ts b/packages/shared/src/types/tool-access.ts index e2b288ae8d..8bb22a496c 100644 --- a/packages/shared/src/types/tool-access.ts +++ b/packages/shared/src/types/tool-access.ts @@ -224,9 +224,12 @@ export interface ConnectionGrant { repositorySelection: "all" | "selected" | "mixed" | "none"; installationIds: string[]; installationOwnerLogins: string[]; + /** Repository metadata visible to this credential; refreshed from GitHub. */ + repositories?: Array<{ id: string; fullName: string; installationId: string }>; installationUrl?: string; managementUrl?: string; appSlug?: string; + accessRevision?: string; lastAccessRefreshAt?: string; lastWebhookAt?: string; webhookHealth?: "pending" | "healthy" | "unhealthy"; diff --git a/server/src/__tests__/github-connection-events.test.ts b/server/src/__tests__/github-connection-events.test.ts index f8a12ece61..aef6432061 100644 --- a/server/src/__tests__/github-connection-events.test.ts +++ b/server/src/__tests__/github-connection-events.test.ts @@ -92,6 +92,7 @@ describeEmbeddedPostgres.sequential("GitHub connection event delivery", () => { repositorySelection: "selected", installationIds: ["101"], installationOwnerLogins: ["paperclipai"], + repositories: [{ id: "203", fullName: "paperclipai/removed", installationId: "101" }], webhookHealth: "pending", }, }, @@ -206,7 +207,7 @@ describeEmbeddedPostgres.sequential("GitHub connection event delivery", () => { expect(connector.acknowledgeEvents).toHaveBeenCalledTimes(2); }); - it("applies installation repository deltas transactionally and never reapplies a processed delivery", async () => { + it.each([false, true])("applies installation events once without discarding newer verified access (refreshed: %s)", async (refreshed) => { const companyId = randomUUID(); const applicationId = randomUUID(); const connectionId = randomUUID(); @@ -252,6 +253,7 @@ describeEmbeddedPostgres.sequential("GitHub connection event delivery", () => { repositorySelection: "selected", installationIds: ["101"], installationOwnerLogins: ["paperclipai"], + repositories: [{ id: "203", fullName: "paperclipai/removed", installationId: "101" }], webhookHealth: "pending", }, }, @@ -280,7 +282,16 @@ describeEmbeddedPostgres.sequential("GitHub connection event delivery", () => { refresh: vi.fn(), revoke: vi.fn(), setWebhookBinding: vi.fn(async () => undefined), - leaseEvents: vi.fn(async () => ({ leaseId: `lease-${++poll}`, events: [leasedEvent] })), + leaseEvents: vi.fn(async () => { + if (refreshed) { + const [latest] = await db.select().from(connectionGrants).where(eq(connectionGrants.id, grantId)); + await db.update(connectionGrants).set({ providerTenant: { + ...latest!.providerTenant, + github: { ...latest!.providerTenant!.github!, lastAccessRefreshAt: "2026-09-04T12:00:02.000Z" }, + } }).where(eq(connectionGrants.id, grantId)); + } + return ({ leaseId: `lease-${++poll}`, events: [leasedEvent] }); + }), acknowledgeEvents: vi.fn(async () => 1), } as unknown as PaperclipCloudConnector; let currentTime = new Date("2026-09-04T12:00:05.000Z"); @@ -292,12 +303,17 @@ describeEmbeddedPostgres.sequential("GitHub connection event delivery", () => { await expect(service.pollOnce()).resolves.toMatchObject({ processed: 1, duplicate: 0, failed: 0 }); unsubscribe(); let [grant] = await db.select().from(connectionGrants).where(eq(connectionGrants.id, grantId)); - expect(grant?.providerTenant?.github).toMatchObject({ repositoryCount: 4, webhookHealth: "healthy" }); + expect(grant?.providerTenant?.github).toMatchObject({ repositoryCount: refreshed ? 3 : 4, webhookHealth: "healthy" }); + if (refreshed) { + expect(grant?.providerTenant?.github?.repositories).toHaveLength(1); + } else { + expect(grant?.providerTenant?.github?.repositories).toBeUndefined(); + } currentTime = new Date(currentTime.getTime() + 6_000); await expect(service.pollOnce()).resolves.toMatchObject({ processed: 0, duplicate: 1, failed: 0 }); [grant] = await db.select().from(connectionGrants).where(eq(connectionGrants.id, grantId)); - expect(grant?.providerTenant?.github?.repositoryCount).toBe(4); + expect(grant?.providerTenant?.github?.repositoryCount).toBe(refreshed ? 3 : 4); const [receipt] = await db.select().from(connectionEventDeliveries).where(eq( connectionEventDeliveries.providerDeliveryId, leasedEvent.id, diff --git a/server/src/__tests__/github-grant-metadata.test.ts b/server/src/__tests__/github-grant-metadata.test.ts index 35e5d01671..b77976f337 100644 --- a/server/src/__tests__/github-grant-metadata.test.ts +++ b/server/src/__tests__/github-grant-metadata.test.ts @@ -1,51 +1,62 @@ import { describe, expect, it, vi } from "vitest"; import { loadGitHubGrantMetadata } from "../services/tool-access.js"; -function json(value: unknown, status = 200): Response { +function json(value: unknown, next = false): Response { return new Response(JSON.stringify(value), { - status, - headers: { "content-type": "application/json" }, + headers: { + "content-type": "application/json", + ...(next ? { link: '; rel="next"' } : {}), + }, }); } describe("GitHub grant metadata", () => { - it("keeps only user and installation summaries while counting accessible repositories", async () => { + it("lists every page of installations and repositories, persisting only display metadata", async () => { const request = vi.fn(async (input) => { - const url = String(input); - if (url.endsWith("/user")) return json({ id: 42, login: "octocat", avatar_url: "https://avatars.example/octocat" }); - if (url.includes("/user/installations?")) { - return json({ - installations: [ - { id: 101, repository_selection: "selected", html_url: "https://github.com/settings/installations/101", account: { login: "paperclipai" } }, - { id: 102, repository_selection: "all", account: { login: "octocat" } }, - ], - }); + const url = new URL(String(input)); + const secondPage = url.searchParams.get("page") === "2"; + if (url.pathname === "/user") return json({ id: 42, login: "octocat", avatar_url: "https://avatars.example/octocat" }); + if (url.pathname === "/user/installations") { + return json({ installations: secondPage + ? [{ id: 102, repository_selection: "all", account: { login: "octocat" } }] + : [{ id: 101, repository_selection: "selected", html_url: "https://github.com/settings/installations/101", account: { login: "paperclipai" } }], + }, !secondPage); } - if (url.includes("/user/installations/101/repositories")) { - return json({ total_count: 2, repositories: [{ full_name: "paperclipai/private-name-must-not-persist" }] }); + if (url.pathname === "/user/installations/101/repositories") { + return json({ total_count: 2, repositories: secondPage + ? [{ id: 2, full_name: "paperclipai/b", description: "must-not-persist", clone_url: "must-not-persist" }] + : [{ id: 1, full_name: "paperclipai/a" }], + }, !secondPage); } - if (url.includes("/user/installations/102/repositories")) return json({ total_count: 5 }); - return json({}, 404); + if (url.pathname === "/user/installations/102/repositories") { + return json({ total_count: 1, repositories: [{ id: 3, full_name: "octocat/c" }] }); + } + throw new Error(`Unexpected GitHub path: ${url.pathname}`); }); const metadata = await loadGitHubGrantMetadata("ghu_secret", request, "paperclip-development"); - expect(metadata).toMatchObject({ userId: "42", login: "octocat", installationCount: 2, - repositoryCount: 7, + repositoryCount: 3, repositorySelection: "mixed", installationIds: ["101", "102"], installationOwnerLogins: ["paperclipai", "octocat"], + repositories: [ + { id: "3", fullName: "octocat/c", installationId: "102" }, + { id: "1", fullName: "paperclipai/a", installationId: "101" }, + { id: "2", fullName: "paperclipai/b", installationId: "101" }, + ], installationUrl: "https://github.com/apps/paperclip-development/installations/new", managementUrl: "https://github.com/settings/installations/101", appSlug: "paperclip-development", webhookHealth: "pending", }); - expect(JSON.stringify(metadata)).not.toContain("private-name-must-not-persist"); - expect(request).toHaveBeenCalledTimes(4); - for (const [, init] of request.mock.calls) { + expect(JSON.stringify(metadata)).not.toContain("must-not-persist"); + expect(request).toHaveBeenCalledTimes(6); + for (const [input, init] of request.mock.calls) { + expect(new URL(String(input)).origin).toBe("https://api.github.com"); expect(new Headers(init?.headers).get("authorization")).toBe("Bearer ghu_secret"); } }); @@ -54,9 +65,19 @@ describe("GitHub grant metadata", () => { const request = vi.fn(async (input) => String(input).endsWith("/user") ? json({ id: 42, login: "octocat" }) : json({ installations: [] })); - await expect(loadGitHubGrantMetadata("ghu_secret", request)).rejects.toMatchObject({ details: expect.objectContaining({ code: "github_installation_required" }), }); }); + + it("does not report a partial repository list when a later page fails", async () => { + const request = vi.fn() + .mockResolvedValueOnce(json({ id: 42, login: "octocat" })) + .mockResolvedValueOnce(json({ installations: [{ id: 101, repository_selection: "selected" }] })) + .mockResolvedValueOnce(json({ repositories: [{ id: 1, full_name: "octocat/a" }] }, true)) + .mockResolvedValueOnce(new Response("Unavailable", { status: 503 })); + await expect(loadGitHubGrantMetadata("ghu_secret", request)).rejects.toMatchObject({ + details: expect.objectContaining({ code: "github_access_check_failed" }), + }); + }); }); diff --git a/server/src/__tests__/tool-access-service.test.ts b/server/src/__tests__/tool-access-service.test.ts index a5709c0fd2..62150dc800 100644 --- a/server/src/__tests__/tool-access-service.test.ts +++ b/server/src/__tests__/tool-access-service.test.ts @@ -5103,7 +5103,7 @@ describeEmbeddedPostgres("tool access service", () => { } }, 15_000); - it("binds a non-expiring managed GitHub identity and installation to one agent", async () => { + it.each(["none", "event", "same-time-refresh"])("binds a managed GitHub identity and protects refresh from concurrent access changes (%s)", async (concurrentChange) => { const company = await createCompany(db); const userId = `github-manager-${randomUUID()}`; await grantBoardUser(db, company.id, userId, [], "owner"); @@ -5114,6 +5114,7 @@ describeEmbeddedPostgres("tool access service", () => { const githubDefinition = getConnectableAppDefinition("github")!; const previousOwnershipAvailability = githubDefinition.ownershipAvailability; githubDefinition.ownershipAvailability = { ...previousOwnershipAvailability, platform_shared: true }; + let beforeRepositoryResponse = async () => {}; vi.spyOn(globalThis, "fetch").mockImplementation(async (url) => { const href = String(url); if (href === "https://api.github.com/user") { @@ -5128,7 +5129,8 @@ describeEmbeddedPostgres("tool access service", () => { }] }); } if (href.includes("https://api.github.com/user/installations/101/repositories?")) { - return mcpHttpResponse({ total_count: 3, repositories: [{ full_name: "paperclipai/do-not-store" }] }); + await beforeRepositoryResponse(); + return mcpHttpResponse({ total_count: 3, repositories: [1, 2, 3].map((id) => ({ id, full_name: `paperclipai/repo-${id}`, description: "do-not-store" })) }); } if (href === GITHUB_CONNECTOR_PROFILES["github.code"].serverUrl) { return mcpHttpResponse({ @@ -5226,6 +5228,33 @@ describeEmbeddedPostgres("tool access service", () => { eq(toolConnectionInstalls.targetType, "agent"), eq(toolConnectionInstalls.targetId, agent.id), ))).resolves.toHaveLength(1); + vi.mocked(connector.setWebhookBinding).mockClear(); + if (concurrentChange !== "none") { + beforeRepositoryResponse = async () => { + const [latest] = await db.select().from(connectionGrants).where(eq(connectionGrants.id, grant!.id)); + await db.update(connectionGrants).set({ providerTenant: { + ...latest!.providerTenant, + github: { + ...latest!.providerTenant!.github!, + accessRevision: randomUUID(), + // Simulate a refresh with identical timestamps, so only the unique + // access revision can distinguish its newer access snapshot. + ...(concurrentChange === "event" ? { lastWebhookAt: new Date().toISOString() } : {}), + installationIds: [], installationCount: 0, repositoryCount: 0, + repositorySelection: "none", repositories: undefined, webhookHealth: "unhealthy", + }, + } }).where(eq(connectionGrants.id, grant!.id)); + }; + await expect(service.checkHealth(connected.connectionId, actor)) + .rejects.toThrow("GitHub access changed during refresh. Try again."); + const [latest] = await db.select().from(connectionGrants).where(eq(connectionGrants.id, grant!.id)); + expect(latest?.providerTenant?.github).toMatchObject({ installationIds: [], repositoryCount: 0, webhookHealth: "unhealthy" }); + expect(latest?.providerTenant?.github?.repositories).toBeUndefined(); + expect(connector.setWebhookBinding).not.toHaveBeenCalled(); + } else { + await expect(service.checkHealth(connected.connectionId, actor)).resolves.toMatchObject({ connection: { healthStatus: "ok" } }); + expect(connector.setWebhookBinding).toHaveBeenCalled(); + } } finally { githubDefinition.ownershipAvailability = previousOwnershipAvailability; } @@ -5261,7 +5290,7 @@ describeEmbeddedPostgres("tool access service", () => { }] }); } if (href.includes("https://api.github.com/user/installations/101/repositories?")) { - return mcpHttpResponse({ total_count: 1, repositories: [] }); + return mcpHttpResponse({ total_count: 1, repositories: [{ id: 1, full_name: "paperclipai/repo-1" }] }); } if (href === GITHUB_CONNECTOR_PROFILES["github.code"].serverUrl) { return mcpHttpResponse({ diff --git a/server/src/services/github-connection-events.ts b/server/src/services/github-connection-events.ts index 22c27c9579..177492ea69 100644 --- a/server/src/services/github-connection-events.ts +++ b/server/src/services/github-connection-events.ts @@ -1,3 +1,4 @@ +import { randomUUID } from "node:crypto"; import { connectionEventDeliveries, connectionGrants, @@ -269,7 +270,28 @@ export function githubConnectionEventService( } async function applyInstallationEvent(database: Db, binding: GitHubBinding, event: LeasedEvent) { - const github = binding.providerTenant.github!; + // Bindings are loaded before the Cloud request. Lock and read the grant + // again so a refresh completed during that request cannot be overwritten. + const [currentGrant] = await database.select().from(connectionGrants).where(and( + eq(connectionGrants.id, binding.grantId), + eq(connectionGrants.companyId, binding.companyId), + eq(connectionGrants.status, "active"), + )).for("update").limit(1); + const currentProviderTenant = currentGrant?.providerTenant; + const github = currentProviderTenant?.github; + if (!github) return; + // A newly bound instance can receive installation events from before OAuth + // verified its repository list. Those events must not erase newer access. + if (Date.parse(github.lastAccessRefreshAt ?? "") > Date.parse(event.createdAt)) { + await database.update(connectionGrants).set({ + providerTenant: { + ...currentProviderTenant, + github: { ...github, lastWebhookAt: now().toISOString(), webhookHealth: "healthy" }, + }, + updatedAt: now(), + }).where(and(eq(connectionGrants.id, binding.grantId), eq(connectionGrants.companyId, binding.companyId))); + return; + } const unavailable = event.event === "installation" && (event.action === "deleted" || event.action === "suspend"); const installationIds = unavailable ? github.installationIds.filter((id) => id !== binding.installationId) @@ -285,9 +307,13 @@ export function githubConnectionEventService( : github.repositoryCount + added - removed, ); const providerTenant = { - ...binding.providerTenant, + ...currentProviderTenant, github: { ...github, + // Lifecycle webhooks carry IDs, not the user token’s complete repository view. + // Discard the snapshot until Refresh access verifies it again. + accessRevision: randomUUID(), + repositories: undefined, installationIds, installationCount: installationIds.length, repositoryCount, @@ -353,12 +379,19 @@ export function githubConnectionEventService( const github = binding.providerTenant.github; if (!github) continue; await database.update(connectionGrants).set({ - providerTenant: { - ...binding.providerTenant, - github: { ...github, lastWebhookAt: touchedAt.toISOString(), webhookHealth: "healthy" }, - }, + // Update only webhook fields; a concurrent access/token refresh + // owns the remaining metadata and must not be replaced here. + providerTenant: sql`jsonb_set(${connectionGrants.providerTenant}, '{github}', + (${connectionGrants.providerTenant}->'github') || ${JSON.stringify({ + lastWebhookAt: touchedAt.toISOString(), webhookHealth: "healthy", + })}::jsonb)`, updatedAt: touchedAt, - }).where(and(eq(connectionGrants.id, binding.grantId), eq(connectionGrants.companyId, companyId))); + }).where(and( + eq(connectionGrants.id, binding.grantId), + eq(connectionGrants.companyId, companyId), + eq(connectionGrants.status, "active"), + sql`${connectionGrants.providerTenant}->'github' is not null`, + )); } } const finishedAt = now(); diff --git a/server/src/services/tool-access.ts b/server/src/services/tool-access.ts index 284b3510f2..d117e6f6a9 100644 --- a/server/src/services/tool-access.ts +++ b/server/src/services/tool-access.ts @@ -1942,13 +1942,16 @@ export async function loadGitHubGrantMetadata( repositorySelection: "all" | "selected" | "mixed" | "none"; installationIds: string[]; installationOwnerLogins: string[]; + repositories: Array<{ id: string; fullName: string; installationId: string }>; installationUrl: string; managementUrl: string; appSlug?: string; + accessRevision: string; lastAccessRefreshAt: string; webhookHealth: "pending"; }> { - const github = async (path: string): Promise> => { + const accessRefreshStartedAt = new Date().toISOString(); + const github = async (path: string): Promise<{ data: Record; hasNext: boolean }> => { const response = await request(`https://api.github.com${path}`, { headers: { accept: "application/vnd.github+json", @@ -1965,21 +1968,30 @@ export async function loadGitHubGrantMetadata( } const value = await response.json() as unknown; if (!recordValue(value)) throw unprocessable("GitHub returned invalid account metadata", { code: "github_bad_response" }); - return value; + return { data: value, hasNext: /;\s*rel="next"/.test(response.headers.get("link") ?? "") }; }; - const user = await github("/user"); + const list = async (path: string, key: string): Promise[]> => { + const items: Record[] = []; + for (let page = 1; ; page += 1) { + const { data, hasNext } = await github(`${path}?per_page=100&page=${page}`); + const batch = data[key]; + if (!Array.isArray(batch) || !batch.every(recordValue) || (hasNext && batch.length === 0)) { + throw unprocessable("GitHub returned invalid access metadata", { code: "github_bad_response" }); + } + items.push(...batch); + if (!hasNext) return items; + } + }; + const { data: user } = await github("/user"); const userId = githubId(user.id); const login = typeof user.login === "string" ? user.login : null; if (!userId || !login) throw unprocessable("GitHub returned invalid account metadata", { code: "github_bad_response" }); - const installationsResponse = await github("/user/installations?per_page=100"); - const installations = Array.isArray(installationsResponse.installations) - ? installationsResponse.installations.filter(recordValue).slice(0, 100) - : []; + const installations = await list("/user/installations", "installations"); const installationIds: string[] = []; const owners = new Set(); const selections = new Set<"all" | "selected">(); const managementUrls = new Set(); - let repositoryCount = 0; + const repositories = new Map(); for (const installation of installations) { const installationId = githubId(installation.id); if (!installationId) continue; @@ -1991,11 +2003,16 @@ export async function loadGitHubGrantMetadata( if (typeof account?.login === "string") owners.add(account.login); const managementUrl = githubInstallationManagementUrl(installation.html_url); if (managementUrl) managementUrls.add(managementUrl); - const repositories = await github(`/user/installations/${installationId}/repositories?per_page=1`); - if (typeof repositories.total_count === "number" && Number.isSafeInteger(repositories.total_count) && repositories.total_count >= 0) { - repositoryCount += repositories.total_count; + for (const repository of await list(`/user/installations/${installationId}/repositories`, "repositories")) { + const id = githubId(repository.id); + const fullName = typeof repository.full_name === "string" ? repository.full_name : ""; + if (!id || !/^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(fullName)) { + throw unprocessable("GitHub returned invalid repository metadata", { code: "github_bad_response" }); + } + repositories.set(id, { id, fullName, installationId }); } } + const repositoryCount = repositories.size; if (installationIds.length === 0 || repositoryCount === 0) { const installationUrl = appSlug ? `https://github.com/apps/${appSlug}/installations/new` @@ -2018,12 +2035,14 @@ export async function loadGitHubGrantMetadata( repositorySelection: selections.size > 1 ? "mixed" : selections.values().next().value ?? "none", installationIds, installationOwnerLogins: [...owners], + repositories: [...repositories.values()].sort((a, b) => a.fullName.localeCompare(b.fullName)), installationUrl, managementUrl: managementUrls.size === 1 ? managementUrls.values().next().value! : "https://github.com/settings/installations", ...(appSlug ? { appSlug } : {}), - lastAccessRefreshAt: new Date().toISOString(), + accessRevision: randomUUID(), + lastAccessRefreshAt: accessRefreshStartedAt, webhookHealth: "pending", }; } @@ -8584,21 +8603,36 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} throw retryError; } } - const previousGitHub = grant.providerTenant?.github; - const providerTenant = { - ...(grant.providerTenant ?? {}), - github: { - ...metadata, - ...(previousGitHub?.lastWebhookAt ? { lastWebhookAt: previousGitHub.lastWebhookAt } : {}), - webhookHealth: previousGitHub?.webhookHealth ?? metadata.webhookHealth, - }, - }; - const [updated] = await db.update(connectionGrants).set({ - providerTenant, - status: "active", - updatedAt: now(), - }).where(and(eq(connectionGrants.id, grant.id), eq(connectionGrants.companyId, grant.companyId))).returning(); - if (!updated) throw notFound("GitHub authorization not found"); + const { updated, previousGitHub } = await db.transaction(async (tx) => { + const [currentGrant] = await tx.select().from(connectionGrants).where(and( + eq(connectionGrants.id, grant.id), eq(connectionGrants.companyId, grant.companyId), + )).for("update").limit(1); + if (!currentGrant || currentGrant.status === "revoked") throw notFound("GitHub authorization not found"); + const previousGitHub = currentGrant.providerTenant?.github; + const initialGitHub = grant.providerTenant?.github; + // No lock is held during provider requests. Reject a snapshot if another + // refresh or webhook changed access while those requests were in flight. + if (previousGitHub?.accessRevision !== initialGitHub?.accessRevision + || previousGitHub?.lastWebhookAt !== initialGitHub?.lastWebhookAt + || previousGitHub?.lastAccessRefreshAt !== initialGitHub?.lastAccessRefreshAt) { + throw conflict("GitHub access changed during refresh. Try again.", { code: "github_access_changed" }); + } + const providerTenant = { + ...(currentGrant.providerTenant ?? {}), + github: { + ...metadata, + ...(previousGitHub?.lastWebhookAt ? { lastWebhookAt: previousGitHub.lastWebhookAt } : {}), + webhookHealth: previousGitHub?.webhookHealth ?? metadata.webhookHealth, + }, + }; + const [updated] = await tx.update(connectionGrants).set({ + providerTenant, + status: "active", + updatedAt: now(), + }).where(and(eq(connectionGrants.id, grant.id), eq(connectionGrants.companyId, grant.companyId))).returning(); + if (!updated) throw notFound("GitHub authorization not found"); + return { updated, previousGitHub }; + }); const cloudConnector = currentCloudConnector(); const subject = updated.kind === "agent" && updated.subjectAgentId diff --git a/ui/src/features/connections/ConnectionSetupFlow.tsx b/ui/src/features/connections/ConnectionSetupFlow.tsx index 1886358b70..b97791408e 100644 --- a/ui/src/features/connections/ConnectionSetupFlow.tsx +++ b/ui/src/features/connections/ConnectionSetupFlow.tsx @@ -961,7 +961,12 @@ export function ConnectionSetupFlow({ [entry, galleryQuery.data, linkUrl], ); - const entryAutomaticOAuthMethod = automaticOAuthMethod(entry); + const selectedSetupMethod = entry ? getAvailableConnectionMethod(entry, connectionMethodKey || null) : null; + // Apps with an advanced PAT option still need OAuth progress and recovery + // screens when their selected method is managed sign-in. + const entryAutomaticOAuthMethod = selectedSetupMethod && connectionMethodSupportsAutomaticOAuth(selectedSetupMethod) + ? selectedSetupMethod + : automaticOAuthMethod(entry); const automaticOAuthEntry = credentialSource === "paperclip_vault" && entryAutomaticOAuthMethod ? entry : null; const directOAuthEntry = credentialSource === "paperclip_vault" && canUseAutomaticOAuthFastPath(entry) ? entry : null; const directOAuthLookupPending = Boolean(directOAuthSource) && ( @@ -1222,9 +1227,8 @@ export function ConnectionSetupFlow({ ); // The enrollment lookup decides whether a hidden managed method means // "enroll this instance" or "that Cloud profile is unavailable here". - // Do not choose a method until that distinction is known: retaining the - // hidden method key after an active enrollment produces an empty setup - // screen with a permanently disabled generic Connect button. + // Preserve managed sign-in intent in both cases; the setup screen explains + // unavailable profiles rather than downgrading to a credential form. if ( requestedDefinitionUsesManagedConnector && !requestedEntryAdvertisesManagedConnector @@ -1234,7 +1238,6 @@ export function ConnectionSetupFlow({ const initialMethod = ( requestedDefinitionUsesManagedConnector && !requestedEntryAdvertisesManagedConnector - && connectorEnrollmentQuery.data?.configured !== true ? recommendedManagedConnectorMethod(fullRequestedDefinition) : null ) ?? recommendedSetupConnectionMethod(methods); @@ -1312,14 +1315,10 @@ export function ConnectionSetupFlow({ hasPrefilledLink: Boolean(prefill.link), zapierSource, })); - } else if ( - connectorEnrollmentQuery.data?.configured === true - && connectionMethodKey - && !methods.some((candidate) => candidate.key === connectionMethodKey) - ) { - // A failed enrollment lookup can select the hidden pre-enrollment - // method. Replace it after a successful refetch proves that the instance - // is enrolled and the current Cloud gallery does not advertise it. + } else if (entryAdvertisesManagedConnector !== requestedEntryAdvertisesManagedConnector) { + // A capability refresh must replace the stale gallery entry as well as + // its method, while preserving the chosen audience and wizard step. + setEntry(requestedEntry); setConnectionMethodKey(initialMethod?.key ?? ""); setConfigValues(defaultMethodConfig(initialMethod)); } @@ -1346,6 +1345,7 @@ export function ConnectionSetupFlow({ connectionMethodKey, credentialSource, entry?.slug, + entryAdvertisesManagedConnector, galleryQuery.data, galleryQuery.isLoading, navigate, @@ -1689,6 +1689,14 @@ export function ConnectionSetupFlow({ && (directOAuthEntry || oauthPhase !== "entry"), ); + const managedConnectorUnavailable = Boolean( + step === "key" + && entry + && requestedDefinitionUsesManagedConnector + && !entryAdvertisesManagedConnector + && connectorEnrollmentQuery.data?.configured === true + ); + const showConnectorEnrollmentStep = Boolean( step === "key" && entry @@ -1938,7 +1946,21 @@ export function ConnectionSetupFlow({ /> )} - {step === "key" && entry && showConnectorEnrollmentStep ? ( + {managedConnectorUnavailable && entry ? ( +
+

{entry.name} sign-in is unavailable

+

+ This instance is connected to Paperclip, but {entry.name} sign-in is not currently available. Try again shortly or contact your instance administrator. +

+
+ + +
+
+ ) : step === "key" && entry && showConnectorEnrollmentStep ? (
diff --git a/ui/src/pages/apps/AppDetail.test.tsx b/ui/src/pages/apps/AppDetail.test.tsx index e5c45cadfc..4cd70abe4f 100644 --- a/ui/src/pages/apps/AppDetail.test.tsx +++ b/ui/src/pages/apps/AppDetail.test.tsx @@ -281,6 +281,7 @@ function dedicatedGitHubGrant( repositorySelection: "selected", installationIds: ["456"], installationOwnerLogins: ["paperclipai"], + repositories: [{ id: "789", fullName: "paperclipai/test-repo", installationId: "456" }], managementUrl: "https://github.com/settings/installations/456", webhookHealth: "pending", lastWebhookAt: null, @@ -1455,6 +1456,28 @@ describe("AppDetail", () => { expect(findButton("Revoke")).toBeUndefined(); }); + it("shows the personal GitHub username and every accessible repository", async () => { + mockParams.tab = "permissions"; + getConnectionMock.mockResolvedValue(perUserConnection()); + listConnectionGrantsMock.mockResolvedValue({ + connection: { id: "conn-1", uid: "conn-1" }, + grants: [dedicatedGitHubGrant({ kind: "user", subjectAgentId: null, subjectUserId: "user-1" }, { + repositoryCount: 2, + repositories: [ + { id: "1", fullName: "paperclipai/first", installationId: "456" }, + { id: "2", fullName: "paperclipai/second", installationId: "456" }, + ], + })], + capabilities: fullCapabilities(), currentUserId: "user-1", members: [], + }); + await renderAppDetail(); + expect(container.textContent).toContain("@dottabot"); + expect(container.textContent).toContain("2 selected repositories"); + expect(container.querySelectorAll('ul[aria-label="Accessible GitHub repositories"] li')).toHaveLength(2); + expect(container.textContent).toContain("paperclipai/first"); + expect(container.textContent).toContain("paperclipai/second"); + }); + it("shows dedicated GitHub access as compact action rows and links to the agent", async () => { mockParams.tab = "permissions"; getConnectionMock.mockResolvedValue(connection({ @@ -1473,7 +1496,9 @@ describe("AppDetail", () => { expect(container.querySelector('a[href="/agents/coder"]')?.textContent).toContain("Used only by Coder"); expect(container.textContent).toContain("Repositories"); - expect(container.textContent).toContain("1 selected repositories"); + expect(container.textContent).toContain("1 selected repository"); + expect(container.querySelector('a[href="https://github.com/dottabot"]')?.textContent).toBe("@dottabot"); + expect(container.querySelector('a[href="https://github.com/paperclipai/test-repo"]')?.textContent).toBe("paperclipai/test-repo"); expect(container.querySelector( 'a[href="https://github.com/settings/installations/456"]', )?.textContent).toBe("Manage repositories on GitHub"); diff --git a/ui/src/pages/apps/AppsConnect.test.tsx b/ui/src/pages/apps/AppsConnect.test.tsx index fc20c65fe4..c1102a17ec 100644 --- a/ui/src/pages/apps/AppsConnect.test.tsx +++ b/ui/src/pages/apps/AppsConnect.test.tsx @@ -852,7 +852,7 @@ describe("AppsConnect — Connect with a link (M4 frame)", () => { expect(container.textContent).not.toContain("Connect with Paperclip"); }); - it("uses GitHub's advertised PAT fallback when an enrolled Cloud omits the managed profile", async () => { + it("explains unavailable GitHub sign-in without silently switching to a PAT", async () => { mockSearch.value = "source=github&stage=setup&cloud_connector=enrolled"; listGalleryMock.mockResolvedValueOnce({ apps: [{ @@ -864,19 +864,19 @@ describe("AppsConnect — Connect with a link (M4 frame)", () => { await render(); - expect(container.textContent).toContain("Your GitHub key"); - expect(container.textContent).not.toContain("Connect with Paperclip"); - expect(container.textContent).not.toContain("Continue to GitHub"); - const connect = buttonByText("Connect"); - expect(connect?.disabled).toBe(true); - const tokenInput = container.querySelector('input[type="password"]'); - expect(tokenInput).toBeTruthy(); - await act(async () => setInputValue(tokenInput!, "github_pat_test")); + expect(container.textContent).toContain("GitHub sign-in is unavailable"); + expect(container.textContent).not.toContain("Your GitHub key"); + expect(container.querySelector('input[type="password"]')).toBeNull(); + expect(buttonByText("Try again")?.disabled).toBe(false); + + listGalleryMock.mockResolvedValue({ apps: [GITHUB_MANAGED] }); + await act(async () => buttonByText("Try again")!.click()); await flushReact(); - expect(connect?.disabled).toBe(false); + expect(container.textContent).toContain("Continue to GitHub"); + expect(container.textContent).not.toContain("GitHub sign-in is unavailable"); }); - it("replaces a hidden managed method after enrollment recovery reveals an advertised PAT fallback", async () => { + it("keeps GitHub sign-in intent when enrollment recovery reveals an unavailable profile", async () => { mockSearch.value = "source=github&stage=setup&cloud_connector=enrolled"; listGalleryMock.mockResolvedValueOnce({ apps: [{ @@ -907,10 +907,9 @@ describe("AppsConnect — Connect with a link (M4 frame)", () => { }); await flushReact(); - expect(container.textContent).toContain("Your GitHub key"); - expect(container.textContent).not.toContain("Connect with Paperclip"); - expect(container.textContent).not.toContain("Continue to GitHub"); - expect(buttonByText("Connect")?.disabled).toBe(true); + expect(container.textContent).toContain("GitHub sign-in is unavailable"); + expect(container.textContent).not.toContain("Your GitHub key"); + expect(buttonByText("Try again")?.disabled).toBe(false); }); it("preserves a dedicated agent identity across the full-page enrollment callback", async () => { @@ -1645,6 +1644,26 @@ describe("AppsConnect — Connect with a link (M4 frame)", () => { ); }); + it("shows installation recovery for GitHub even when an advanced PAT method is available", async () => { + const connectionId = "22222222-2222-4222-8222-222222222222"; + mockSearch.value = `source=github&resume=${connectionId}&oauth=failed&code=github_installation_required&installation_url=https%3A%2F%2Fgithub.com%2Fapps%2Fpaperclip-for-github%2Finstallations%2Fnew`; + listGalleryMock.mockResolvedValue({ apps: [GITHUB_MANAGED] }); + listApplicationsMock.mockResolvedValue({ applications: [{ id: "app-github", status: "draft", metadata: { sourceTemplateKey: "github" } }] }); + listConnectionsMock.mockResolvedValue({ connections: [{ + id: connectionId, applicationId: "app-github", authKind: "oauth", credentialPolicy: "per_user", status: "draft", + config: { sourceTemplateKey: "github", connectionMethodKey: "managed" }, transportConfig: {}, + }] }); + await render(); + await flushReact(); + expect(container.textContent).toContain("Install Paperclip and grant at least one repository"); + expect(container.querySelector('a[href="https://github.com/apps/paperclip-for-github/installations/new"]')?.textContent).toBe("Install Paperclip on GitHub"); + expect(container.textContent).not.toContain("Your GitHub key"); + await act(async () => buttonByText("Try again")!.click()); + await flushReact(); + expect(startOAuthMock).toHaveBeenCalledWith(connectionId, { asCurrentUser: true }); + expect(connectAppMock).not.toHaveBeenCalled(); + }); + it("returns a declined OAuth draft to the same one-action resume checkpoint", async () => { mockSearch.value = "source=notion&resume=22222222-2222-4222-8222-222222222222&oauth=denied&code=oauth_authorization_denied"; listGalleryMock.mockResolvedValueOnce({ apps: [NOTION] }); diff --git a/ui/src/pages/apps/app-detail/IdentitiesSection.tsx b/ui/src/pages/apps/app-detail/IdentitiesSection.tsx index 2f49e0c15a..f2abaf0af9 100644 --- a/ui/src/pages/apps/app-detail/IdentitiesSection.tsx +++ b/ui/src/pages/apps/app-detail/IdentitiesSection.tsx @@ -300,9 +300,15 @@ function GitHubConnectionSummary({ : null; const repositorySummary = github.repositorySelection === "none" ? "No repositories selected" - : `${github.repositoryCount} selected repositories`; + : `${github.repositoryCount} selected ${github.repositoryCount === 1 ? "repository" : "repositories"}`; return (
+
+
GitHub account
+ + @{github.login} + +
Repositories
@@ -327,6 +333,21 @@ function GitHubConnectionSummary({ ) : null}
+
+ {github.repositories ? ( + + ) : ( +

Refresh access to load the current repository list.

+ )} +
Refresh access