From 01206ab4a56c055a0c3bcdfbb9f1e1c55d7e1ed1 Mon Sep 17 00:00:00 2001 From: Dotta Date: Wed, 7 Oct 2026 07:36:53 -0500 Subject: [PATCH] fix(github): recover existing apps and keep member linking separate Explicit manual recovery switches to the existing-App identity path only after vault storage succeeds. Preserve the incomplete registration for local webhook repair. Account-only linking avoids manager setup APIs and retains personal GitHub connection onboarding. Co-Authored-By: Paperclip --- .../chat-channels.integration.test.ts | 31 +++++++++- server/src/services/chat-channels.ts | 22 +++++-- .../pages/apps/chat/GitHubChatSetup.test.tsx | 57 +++++++++++++++++++ ui/src/pages/apps/chat/GitHubChatSetup.tsx | 53 +++++++++++------ 4 files changed, 139 insertions(+), 24 deletions(-) diff --git a/server/src/__tests__/chat-channels.integration.test.ts b/server/src/__tests__/chat-channels.integration.test.ts index 5c43f9cd7e2..6e8ea767812 100644 --- a/server/src/__tests__/chat-channels.integration.test.ts +++ b/server/src/__tests__/chat-channels.integration.test.ts @@ -2108,6 +2108,7 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { webhookSecret, providerFetch, webhookSyncRequests, + privateKey, setSupplementalProviderFetch(value: typeof supplementalProviderFetch) { supplementalProviderFetch = value; }, @@ -2370,7 +2371,7 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { .where(eq(chatGitHubRegistrations.endpointId, bot.id)); expect(session.status).toBe("failed"); }); - it("wizard retains recovery when a manifest vault write saved App credentials but lost OAuth credentials", async () => { + it("wizard retains partial manifest recovery until explicit existing-App recovery succeeds", async () => { const f = await reviewBotFixture(); const returnState = randomUUID(); const [connection] = await db @@ -2420,7 +2421,10 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { fetch: f.providerFetch, connector: () => ({ - githubApp: async () => ({ id: "partial-oauth" }), + githubApp: async () => ({ + id: "partial-oauth", + webhookUrl: "https://gateway.example.test/github-apps/partial-oauth/webhook", + }), claimGitHubApp, }) as unknown as PaperclipCloudConnector, startDirect: f.service.startGitHubRegistration, @@ -2442,6 +2446,29 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { .from(chatGitHubRegistrations) .where(eq(chatGitHubRegistrations.endpointId, f.endpoint.id)); expect(session.status).toBe("failed"); + const resourcesBefore = await f.service.listResources(f.endpoint.id); + await f.service.storeGitHubApp(f.endpoint.id, "owner-user", { + appId: f.endpoint.botExternalId!, + privateKey: f.privateKey, + webhookSecret: f.webhookSecret, + }); + const [recovered] = await db.select().from(chatGitHubRegistrations) + .where(eq(chatGitHubRegistrations.id, session.id)); + expect(recovered.handoff).toMatchObject({ cloudId: "partial-oauth", returnState }); + expect(recovered.handoff?.manifestClaimId).toBeUndefined(); + expect(recovered.status).toBe("failed"); + expect(await wizard.advance(f.endpoint.id, "owner-user")).toMatchObject({ state: "verify" }); + expect(claimGitHubApp).not.toHaveBeenCalled(); + expect(f.webhookSyncRequests).toEqual([ + expect.objectContaining({ + url: "https://gateway.example.test/github-apps/partial-oauth/webhook", + secret: f.webhookSecret, + }), + ]); + expect((await f.service.listResources(f.endpoint.id)).map(({ id, enabled }) => ({ id, enabled }))) + .toEqual(resourcesBefore.map(({ id, enabled }) => ({ id, enabled }))); + expect((await f.service.get(f.endpoint.id)).botExternalId).toBe(f.endpoint.botExternalId); + expect(finish).not.toHaveBeenCalled(); }); it("wizard recovers a renamed enrolled origin only after checking bound state and Cloud authority", async () => { const f = await seedCompany(); diff --git a/server/src/services/chat-channels.ts b/server/src/services/chat-channels.ts index cb68f54a96e..58bb05ab118 100644 --- a/server/src/services/chat-channels.ts +++ b/server/src/services/chat-channels.ts @@ -16,7 +16,7 @@ import { githubChatManagementService } from "./chat-github-management.js"; import { githubReviewCheckService } from "./chat-github-checks.js"; import { githubAutomaticReviewEvent, githubAutomaticAdmission, githubPreviousAssessment } from "./chat-github-events.js"; import { githubReviewPrompt } from "./chat-github-review-policy.js"; -import { chatGitHubConfigurations, chatGitHubReviews } from "@paperclipai/db"; +import { chatGitHubConfigurations, chatGitHubRegistrations, chatGitHubReviews } from "@paperclipai/db"; import type { GitHubReviewEventContext, GitHubReviewPolicy } from "@paperclipai/shared"; import { githubChatReviewService } from "./chat-github-reviews.js"; import { githubChatWizardService } from "./chat-github-wizard.js"; @@ -38213,7 +38213,7 @@ export function chatChannelService(db: Db, options: ChatChannelServiceOptions) { : null; } - async function storeGitHubApp(endpointId: string, userId: string, credentials: Record) { + async function storeGitHubApp(endpointId: string, userId: string, credentials: Record, source: "manifest" | "existing_app" = "existing_app") { const initial = await endpointRecord(endpointId); if (!initial || initial.endpoint.provider !== "github") throw notFound("GitHub bot not found"); await withCredentialMutationLease(initial.endpoint, async credentialLease => { @@ -38257,6 +38257,20 @@ export function chatChannelService(db: Db, options: ChatChannelServiceOptions) { setup: { ...current.setup, github: { ...current.setup.github, stage: "install", appSlug: slug, installationUrl: `https://github.com/apps/${slug}/installations/new`, registrationStatus: "completed" } }, healthMessage: "Install the App on GitHub to continue", updatedAt: new Date(), }).where(eq(chatEndpoints.id, endpointId)); + if (source === "existing_app") { + // Explicit recovery uses the existing-App identity path. Only drop the + // manifest requirement after the same App's credentials are vaulted; + // leave registration incomplete so advance repairs and verifies delivery. + await tx.update(chatGitHubRegistrations).set({ + handoff: sql`${chatGitHubRegistrations.handoff} - 'manifestClaimId'`, + }).where(and( + eq(chatGitHubRegistrations.companyId, record.endpoint.companyId), + eq(chatGitHubRegistrations.endpointId, endpointId), + eq(chatGitHubRegistrations.userId, userId), + inArray(chatGitHubRegistrations.status, ["pending", "exchanging", "failed"]), + sql`${chatGitHubRegistrations.handoff} ? 'manifestClaimId'`, + )); + } await logActivity(tx as unknown as Db, { companyId: record.endpoint.companyId, actorType: "user", actorId: userId, action: "chat_github.app_connected", entityType: "tool_connection", entityId: record.endpoint.connectionId, details: { endpointId, appId: identity.botExternalId } }); }); }); @@ -38278,7 +38292,7 @@ export function chatChannelService(db: Db, options: ChatChannelServiceOptions) { const githubRegistration = githubChatRegistrationService(db, { publicOrigin: () => getPublicBaseUrl(), webhookOrigin: () => getWebhookPublicBaseUrl(), fetch: fetchImpl, - storeApp: async (endpointId, userId, app) => { await storeGitHubApp(endpointId, userId, { appId: app.appId, privateKey: app.privateKey, webhookSecret: app.webhookSecret, ...(app.clientId ? { clientId: app.clientId } : {}), ...(app.clientSecret ? { clientSecret: app.clientSecret } : {}) }); }, + storeApp: async (endpointId, userId, app) => { await storeGitHubApp(endpointId, userId, { appId: app.appId, privateKey: app.privateKey, webhookSecret: app.webhookSecret, ...(app.clientId ? { clientId: app.clientId } : {}), ...(app.clientSecret ? { clientSecret: app.clientSecret } : {}) }, "manifest"); }, }); async function refreshGitHubRepositories(endpointId: string, userId: string) { @@ -38299,7 +38313,7 @@ export function chatChannelService(db: Db, options: ChatChannelServiceOptions) { const githubWizard = githubChatWizardService(db, { origin: () => getPublicBaseUrl() ?? options.githubWizardOrigin ?? null, fetch: fetchImpl, startDirect: githubRegistration.start, completeDirect: githubRegistration.complete, resumeDirect: githubRegistration.resumeRegistration, - storeApp: storeGitHubApp, + storeApp: (id, userId, credentials) => storeGitHubApp(id, userId, credentials, "manifest"), storeCredentials: async (id, userId, additional) => { const record = await endpointRecord(id); if (!record || record.endpoint.provider !== "github") throw notFound("GitHub bot not found"); diff --git a/ui/src/pages/apps/chat/GitHubChatSetup.test.tsx b/ui/src/pages/apps/chat/GitHubChatSetup.test.tsx index fdccb913387..d012cbcf422 100644 --- a/ui/src/pages/apps/chat/GitHubChatSetup.test.tsx +++ b/ui/src/pages/apps/chat/GitHubChatSetup.test.tsx @@ -411,6 +411,63 @@ describe("GitHub App wizard", () => { expect(githubChatApi.confirmIdentity).toHaveBeenCalledWith("draft-1", "42"); expect(githubChatApi.startIdentity).not.toHaveBeenCalled(); }); + it("lets a member add a personal connection without manager setup APIs", async () => { + vi.mocked(githubChatApi.advance).mockRejectedValue( + new Error("Manager permission required"), + ); + await render("resume=draft-1&stage=identity"); + expect( + container.querySelector('a[href="/apps/connect?source=github"]'), + ).toBeTruthy(); + expect(container.textContent).not.toContain("Manager permission required"); + expect(githubChatApi.advance).not.toHaveBeenCalled(); + expect(githubChatApi.startIdentity).not.toHaveBeenCalled(); + await click("Save & exit"); + expect(githubChatApi.saveDraft).not.toHaveBeenCalled(); + }); + it("links a member's verified personal account without advancing bot setup", async () => { + vi.mocked(githubChatApi.personalConnections).mockResolvedValue([ + { + connectionId: "personal-1", + name: "My GitHub", + login: "octocat", + status: "active", + enabled: true, + }, + ]); + vi.mocked(githubChatApi.identity).mockResolvedValue({ + connectionId: "personal-1", + githubUserId: "42", + login: "octocat", + avatarUrl: null, + }); + await render("resume=draft-1&stage=identity"); + const account = container.querySelector( + "#github-personal-account", + ) as HTMLSelectElement; + await act(async () => { + account.value = "personal-1"; + account.dispatchEvent(new Event("change", { bubbles: true })); + }); + await click("Check my account"); + expect(githubChatApi.identity).toHaveBeenLastCalledWith( + "draft-1", + "personal-1", + undefined, + ); + await click("Confirm my account"); + expect(githubChatApi.identity).toHaveBeenLastCalledWith( + "draft-1", + "personal-1", + "42", + ); + expect(container.textContent).toContain( + "You can now mention this bot on GitHub", + ); + expect(githubChatApi.advance).not.toHaveBeenCalled(); + expect(githubChatApi.startIdentity).not.toHaveBeenCalled(); + expect(githubChatApi.confirmIdentity).not.toHaveBeenCalled(); + }); it("completes automatically while reporting runtime readiness separately", async () => { fixture.endpoint = { ...fixture.endpoint, diff --git a/ui/src/pages/apps/chat/GitHubChatSetup.tsx b/ui/src/pages/apps/chat/GitHubChatSetup.tsx index 9193635132a..9904e02b76a 100644 --- a/ui/src/pages/apps/chat/GitHubChatSetup.tsx +++ b/ui/src/pages/apps/chat/GitHubChatSetup.tsx @@ -71,13 +71,14 @@ export function GitHubChatSetup() { }); const [copied, setCopied] = useState(false); const [legacyAccount, setLegacyAccount] = useState(""); + const [identityLinked, setIdentityLinked] = useState(false); const [legacyIdentity, setLegacyIdentity] = useState > | null>(null); const agents = useQuery({ queryKey: ["github-setup-agents", selectedCompanyId], queryFn: () => agentsApi.list(selectedCompanyId!), - enabled: !!selectedCompanyId, + enabled: !!selectedCompanyId && !identityOnly, }); const current = useQuery({ queryKey: ["github-setup", resume], @@ -89,7 +90,7 @@ export function GitHubChatSetup() { const progress = useQuery({ queryKey: ["github-wizard", resume], queryFn: () => githubChatApi.advance(resume!), - enabled: !!bot && !existing, + enabled: !!bot && !existing && !identityOnly, refetchInterval: (query) => ["connected", "create", "identity", "enrollment", "recovery"].includes( query.state.data?.state ?? "", @@ -106,7 +107,9 @@ export function GitHubChatSetup() { const selectedAgent = agents.data?.find( (agent) => agent.id === (bot?.assignedAgentId ?? agentId), ); - const state = progress.data; + const state = identityOnly ? undefined : progress.data; + const existingIdentityMethod = + identityOnly || state?.identityMethod === "existing_connection"; const connected = state?.state === "connected" && !existing; useEffect(() => { setBreadcrumbs([ @@ -146,7 +149,7 @@ export function GitHubChatSetup() { } async function refresh() { await current.refetch(); - await progress.refetch(); + if (!identityOnly) await progress.refetch(); } const appInput = () => ({ name: ( @@ -161,6 +164,7 @@ export function GitHubChatSetup() { const exit = () => void run(async () => { if ( + !identityOnly && bot && !bot.botExternalId && !state?.registration && @@ -225,9 +229,12 @@ export function GitHubChatSetup() { ? "Connect your account" : "Connect GitHub"} - {(error || progress.error) && ( + {(error || (!identityOnly && progress.error) || accounts.error) && (

{error || + (accounts.error instanceof Error + ? accounts.error.message + : undefined) || (progress.error instanceof Error ? progress.error.message : "Could not check GitHub setup. Try again.")} @@ -405,7 +412,15 @@ export function GitHubChatSetup() { ) : state?.state === "identity" || identityOnly ? ( <> - {state?.identity ? ( + {identityOnly && identityLinked ? ( + <> +

+ GitHub account {legacyIdentity?.login}{" "} + connected. You can now mention this bot on GitHub. +

+ {footer()} + + ) : state?.identity ? ( <>

Connect GitHub account {state.identity.login}{" "} @@ -437,6 +452,7 @@ export function GitHubChatSetup() { onChange={(event) => { setLegacyAccount(event.target.value); setLegacyIdentity(null); + setIdentityLinked(false); }} > @@ -461,15 +477,14 @@ export function GitHubChatSetup() { )} )} - {state?.identityMethod === "existing_connection" && - !legacyAccount && ( - - Add a personal GitHub connection, then return to this draft - - )} + {existingIdentityMethod && !legacyAccount && ( + + Add a personal GitHub connection, then return to this draft + + )} {footer( legacyAccount ? legacyIdentity @@ -484,14 +499,16 @@ export function GitHubChatSetup() { legacyIdentity?.githubUserId, ); setLegacyIdentity(observed); - if (legacyIdentity) await refresh(); + if (legacyIdentity) { + setIdentityLinked(true); + await refresh(); + } } else { const result = await githubChatApi.startIdentity(bot.id); window.location.assign(result.authorizationUrl); } }, - state?.identityMethod === "existing_connection" && - !legacyAccount, + existingIdentityMethod && !legacyAccount, )} )}