From 87945c8cf3a4109323eec00d35bd22671cb1185c Mon Sep 17 00:00:00 2001 From: Dotta Date: Wed, 7 Oct 2026 00:22:55 -0500 Subject: [PATCH] fix(connections): preserve wizard recovery and repository restrictions Co-Authored-By: Paperclip --- doc/plans/2026-10-06-github-app-wizard.md | 33 +- packages/db/src/schema/chat_github.ts | 2 + packages/shared/src/types/chat-channels.ts | 2 + packages/shared/src/types/github-app-setup.ts | 1 + .../chat-channels.integration.test.ts | 404 ++++++++++++++++-- server/src/__tests__/openapi-routes.test.ts | 15 + server/src/routes/openapi.ts | 20 +- server/src/services/chat-channels.ts | 7 +- server/src/services/chat-github-wizard.ts | 218 ++++++++-- tests/e2e/chat-adapters-ui-providers.spec.ts | 57 ++- tests/e2e/chat-adapters-ui.shared.ts | 14 +- .../apps/chat/ChatEndpoint.clipboard.test.tsx | 64 +-- .../pages/apps/chat/GitHubChatSetup.test.tsx | 25 +- ui/src/pages/apps/chat/GitHubChatSetup.tsx | 74 ++-- ui/storybook/prototypes/github-chat/README.md | 3 +- 15 files changed, 750 insertions(+), 189 deletions(-) diff --git a/doc/plans/2026-10-06-github-app-wizard.md b/doc/plans/2026-10-06-github-app-wizard.md index 948be25266..d899b4d42a 100644 --- a/doc/plans/2026-10-06-github-app-wizard.md +++ b/doc/plans/2026-10-06-github-app-wizard.md @@ -105,20 +105,27 @@ Save & exit, resume without a reload, full reload, and the conditional Cloud enrollment handoff passed. The test did not authorize Cloud enrollment or create a provider App. -The focused UI suite has ten passing tests. Backend coverage includes both -owner types, interrupted registration and vault storage, initial repository -import, preserved settings, tenant binding, legacy callbacks, Cloud webhook -repair, and automatic completion with runtime warnings. Typecheck, build, -and UI token gates pass. Full Vitest qualification is running on the review -branch. +The focused wizard and retained clipboard/recovery UI suites pass (37 tests). +Backend wizard coverage passes (14 cases), including both owner types, +interrupted registration and vault storage, empty saved repository selections, +localhost configuration, concurrent identity attempts, enrolled-origin recovery, +tenant binding, and legacy callbacks. The complete OpenAPI suite passes (13 +tests). Workspace typecheck, build, and UI token gates pass. The revised GitHub +browser journey passes through manual recovery, identity confirmation, automatic +completion, settings, and reconnect. -Cloud qualification passed its current service suite (200 tests) and the -remaining current test files (2,352 tests, 73 environment-dependent skips). -Further focused cases cover replacement installation and dedicated identity -consent with PKCE, denial, and single-use state. The Postgres concurrency case -requires a disposable database and was not run here. Generated test output -must be cleaned before the suite: stale files from earlier builds caused two -false failures against deleted upstream tests. +The full local Vitest run reported 16,080 passing tests, nine failing tests, +and three suites with database-startup timeouts. Its failing suites are being +rerun in isolation; this is not a claim of a green full suite. CI initially +identified outdated wizard expectations and missing API documentation, both now +covered by the updated focused checks. + +Cloud's final standard `npm test` passes: 2,559 tests and 73 environment-dependent +skips. Dedicated-App regressions cover normalized concurrent starts, stale +installation redeliveries, atomic inbox/state transitions, current enrolled +callback destinations, PKCE consent and denial, expired claims, and webhook +secret retirement. The Postgres concurrency case requires a disposable +database and was not run here. Before rollout, apply the additive Cloud migration, enable the dedicated-App capability, then deploy the synchronized instance migration and wizard through diff --git a/packages/db/src/schema/chat_github.ts b/packages/db/src/schema/chat_github.ts index 2e567736a3..2faa2b715a 100644 --- a/packages/db/src/schema/chat_github.ts +++ b/packages/db/src/schema/chat_github.ts @@ -71,6 +71,8 @@ export const chatGitHubRegistrations = pgTable( redemptionId: string; returnState: string; webhookSecretHash?: string; + identityLeaseId?: string; + identityLeaseExpiresAt?: string; identityStateHash?: string; identityExpiresAt?: string; identityRedemptionId?: string; diff --git a/packages/shared/src/types/chat-channels.ts b/packages/shared/src/types/chat-channels.ts index e550931880..fd01c9f9b1 100644 --- a/packages/shared/src/types/chat-channels.ts +++ b/packages/shared/src/types/chat-channels.ts @@ -199,6 +199,8 @@ export interface ChatEndpointSetupState { appName?: string; cloudRegistrationId?: string; initialRepositoriesImported?: boolean; + repositorySelectionSaved?: boolean; + initialRepositoryImportPending?: boolean; }; step: "choose_agent" | "provider_setup" | "test" | "complete"; /** Server-generated boundary; only provider events at or after this time can complete setup. */ diff --git a/packages/shared/src/types/github-app-setup.ts b/packages/shared/src/types/github-app-setup.ts index 9b156b61ca..e202086f6d 100644 --- a/packages/shared/src/types/github-app-setup.ts +++ b/packages/shared/src/types/github-app-setup.ts @@ -5,6 +5,7 @@ export type GitHubAppOwner = { export type GitHubAppRegistrationInput = GitHubAppOwner & { name: string }; export type GitHubAppCloudState = GitHubAppOwner & { id: string; + returnOrigin?: string; status: "pending" | "exchanging" | "credentials" | "installed" | "failed"; expiresAt: string; appId?: string; diff --git a/server/src/__tests__/chat-channels.integration.test.ts b/server/src/__tests__/chat-channels.integration.test.ts index f0366a9c59..91a86cc900 100644 --- a/server/src/__tests__/chat-channels.integration.test.ts +++ b/server/src/__tests__/chat-channels.integration.test.ts @@ -1,5 +1,6 @@ import { toolActionRequests, toolInvocations } from "@paperclipai/db"; import { GitHubPublicationLeaseLost, withGitHubPublicationLease } from "../services/chat-github-publication-lease.js"; +import { secretService } from "../services/secrets.js"; import { githubChatWizardService } from "../services/chat-github-wizard.js"; import type { PaperclipCloudConnector } from "../services/paperclip-cloud-connector.js"; import { dispatchGitHubBotCloudEvent } from "../services/chat-github-cloud-ingress.js"; @@ -2242,23 +2243,21 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { { provider: "github", assignedAgentId: f.assignedAgentId }, "owner-user", ); - await db - .insert(chatGitHubRegistrations) - .values({ - companyId: f.companyId, - endpointId: bot.id, - userId: "owner-user", - stateHash: randomUUID(), - trustedOrigin: "http://127.0.0.1:3104", - ownerType: "personal", - status: "exchanging", - expiresAt: new Date(Date.now() + 60_000), - handoff: { - cloudId: "claim-recovery", - returnState: "bound", - redemptionId: "same-receipt", - }, - }); + await db.insert(chatGitHubRegistrations).values({ + companyId: f.companyId, + endpointId: bot.id, + userId: "owner-user", + stateHash: randomUUID(), + trustedOrigin: "http://127.0.0.1:3104", + ownerType: "personal", + status: "exchanging", + expiresAt: new Date(Date.now() + 60_000), + handoff: { + cloudId: "claim-recovery", + returnState: "bound", + redemptionId: "same-receipt", + }, + }); const claimGitHubApp = vi.fn(async () => ({ kind: "github_app", registrationId: "claim-recovery", @@ -2317,6 +2316,69 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { .where(eq(chatGitHubRegistrations.endpointId, bot.id)), ).toHaveLength(1); }); + it("wizard recovers a renamed enrolled origin only after checking bound state and Cloud authority", async () => { + const f = await seedCompany(); + const { service } = createService(); + const bot = await service.create( + f.companyId, + { provider: "github", assignedAgentId: f.assignedAgentId }, + "owner-user", + ); + await db + .insert(chatGitHubRegistrations) + .values({ + companyId: f.companyId, + endpointId: bot.id, + userId: "owner-user", + stateHash: createHash("sha256").update("bound-state").digest("hex"), + trustedOrigin: "https://old.example.test", + status: "pending", + expiresAt: new Date(Date.now() + 60_000), + handoff: { + cloudId: "origin-recovery", + returnState: "bound-state", + redemptionId: "same-receipt", + }, + }); + let enrolledOrigin = "https://wrong.example.test"; + const githubApp = vi.fn(async () => ({ + id: "origin-recovery", + status: "pending", + returnOrigin: enrolledOrigin, + registrationUrl: "https://github.com/settings/apps/new", + manifest: { public: false }, + expiresAt: new Date(Date.now() + 60_000).toISOString(), + })); + const wizard = githubChatWizardService(db, { + origin: () => "https://new.example.test", + connector: () => ({ githubApp }) as unknown as PaperclipCloudConnector, + startDirect: service.startGitHubRegistration, + storeApp: service.storeGitHubApp, + storeCredentials: async () => {}, + refreshRepositories: service.refreshGitHubRepositories, + resources: service.listResources, + replaceResources: service.replaceResources, + configure: async () => {}, + finish: async () => {}, + }); + await expect( + wizard.cloudCallback("unbound-state", "origin-recovery"), + ).rejects.toThrow("did not match"); + expect(githubApp).not.toHaveBeenCalled(); + await expect( + wizard.cloudCallback("bound-state", "origin-recovery"), + ).rejects.toThrow("did not match"); + enrolledOrigin = "https://new.example.test"; + expect( + await wizard.cloudCallback("bound-state", "origin-recovery"), + ).toContain("https://new.example.test/"); + const [session] = await db + .select() + .from(chatGitHubRegistrations) + .where(eq(chatGitHubRegistrations.endpointId, bot.id)); + expect(session!.trustedOrigin).toBe(enrolledOrigin); + expect(session!.handoff?.cloudId).toBe("origin-recovery"); + }); it("wizard keeps localhost draft choices while waiting for Cloud enrollment", async () => { const f = await seedCompany(); const { service } = createService(); @@ -2363,21 +2425,19 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { const context = await configuredGitHubEndpoint(f, {}, true); const bot = await context.service.get(context.endpoint.id), cloudId = "registration-repair"; - await db - .insert(chatGitHubRegistrations) - .values({ - companyId: f.companyId, - endpointId: bot.id, - userId: "owner-user", - stateHash: randomUUID(), - trustedOrigin: "https://reviews.example.test", - ownerType: "organization", - ownerLogin: "paperclipai", - appName: "Reviewer", - status: "failed", - expiresAt: new Date(), - handoff: { cloudId, returnState: "bound", redemptionId: "receipt" }, - }); + await db.insert(chatGitHubRegistrations).values({ + companyId: f.companyId, + endpointId: bot.id, + userId: "owner-user", + stateHash: randomUUID(), + trustedOrigin: "https://reviews.example.test", + ownerType: "organization", + ownerLogin: "paperclipai", + appName: "Reviewer", + status: "failed", + expiresAt: new Date(), + handoff: { cloudId, returnState: "bound", redemptionId: "receipt" }, + }); const webhookUrl = `https://cloud.example.test/v1/connector/webhooks/github/apps/${cloudId}`; let repaired = false; const githubApp = vi.fn( @@ -2540,21 +2600,20 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { (await f.service.githubWizard.advance(f.endpoint.id, "owner-user")).state, ).toBe("connected"); }); - it("imports initial GitHub access without a second picker and leaves later additions disabled", async () => { + it("wizard preserves an explicit empty repository selection at revision zero", async () => { const f = await seedCompany(); const context = await configuredGitHubEndpoint(f, {}, true); - const current = await context.service.get(context.endpoint.id); - const first = await context.service.listResources(current.id); + const resources = await context.service.listResources(context.endpoint.id); await context.service.replaceResources( - current.id, - first.map((resource) => ({ id: resource.id, enabled: false })), + context.endpoint.id, + resources.map((resource) => ({ id: resource.id, enabled: false })), "owner-user", ); await db .insert(chatGitHubRegistrations) .values({ companyId: f.companyId, - endpointId: current.id, + endpointId: context.endpoint.id, userId: "owner-user", stateHash: randomUUID(), trustedOrigin: "https://reviews.example.test", @@ -2563,6 +2622,275 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { status: "completed", expiresAt: new Date(Date.now() + 60_000), }); + await context.service.githubWizard.advance(context.endpoint.id, "owner-user"); + expect( + (await context.service.listResources(context.endpoint.id)).map( + (resource) => resource.enabled, + ), + ).toEqual([false]); + // The import fence also protects a stale caller that already planned an import. + await context.service.replaceResources( + context.endpoint.id, + resources.map((resource) => ({ id: resource.id, enabled: true })), + "owner-user", + { initialGitHubImport: true }, + ); + expect( + (await context.service.listResources(context.endpoint.id)).map( + (resource) => resource.enabled, + ), + ).toEqual([false]); + }); + it("wizard configures Cloud webhook delivery without a public instance URL", async () => { + const f = await seedCompany(); + const context = await configuredGitHubEndpoint(f, {}, true); + const current = await context.service.get(context.endpoint.id); + await db + .update(chatEndpoints) + .set({ + status: "draft", + setup: { + ...current.setup, + github: { + ...current.setup.github, + cloudRegistrationId: "cloud-localhost", + }, + }, + }) + .where(eq(chatEndpoints.id, current.id)); + const local = createService(new FakeChatSdkRuntime(), context.providerFetch, { + publicBaseUrl: null, + webhookPublicBaseUrl: null, + }); + expect( + ( + await local.service.configure( + current.id, + { action: "configure" }, + "owner-user", + ) + ).status, + ).not.toBe("error"); + const direct = await local.service.create( + f.companyId, + { provider: "github", assignedAgentId: f.assignedAgentId }, + "owner-user", + ); + await expect( + local.service.configure(direct.id, { action: "configure" }, "owner-user"), + ).rejects.toThrow("public"); + }); + it("wizard serializes identity starts and callbacks without overwriting PKCE or handoff state", async () => { + const f = await seedCompany(); + const context = await configuredGitHubEndpoint(f, {}, true); + const connectedAppId = (await context.service.get(context.endpoint.id)) + .botExternalId; + const vault = secretService(db); + const [connection] = await db + .select() + .from(toolConnections) + .where(eq(toolConnections.id, context.endpoint.connectionId)); + const refs = [...connection!.credentialSecretRefs]; + for (const key of ["clientId", "clientSecret"]) { + const secret = await vault.create(f.companyId, { + name: `wizard-${key}-${randomUUID()}`, + provider: "local_encrypted", + value: `test-${key}`, + }); + await vault.createBinding({ + companyId: f.companyId, + secretId: secret.id, + targetType: "tool_connection", + targetId: connection!.id, + configPath: `credentials.${key}`, + }); + refs.push({ + configPath: `credentials.${key}`, + secretId: secret.id, + versionSelector: "latest", + }); + } + await db + .update(toolConnections) + .set({ credentialSecretRefs: refs }) + .where(eq(toolConnections.id, connection!.id)); + await db + .insert(chatGitHubRegistrations) + .values({ + companyId: f.companyId, + endpointId: context.endpoint.id, + userId: "owner-user", + stateHash: randomUUID(), + trustedOrigin: "https://reviews.example.test", + status: "completed", + expiresAt: new Date(Date.now() + 60_000), + handoff: { + cloudId: "", + returnState: "preserve-return", + redemptionId: "preserve-receipt", + webhookSecretHash: "preserve-hash", + }, + }); + let entered!: () => void, + release!: () => void, + callbackEntered!: () => void, + callbackRelease!: () => void; + const storing = new Promise((resolve) => { + entered = resolve; + }); + const storeGate = new Promise((resolve) => { + release = resolve; + }); + const exchanging = new Promise((resolve) => { + callbackEntered = resolve; + }); + const exchangeGate = new Promise((resolve) => { + callbackRelease = resolve; + }); + let writes = 0, + verifier = "", + verifierSecretId = ""; + const wizard = githubChatWizardService(db, { + origin: () => "https://reviews.example.test", + connector: () => null, + startDirect: context.service.startGitHubRegistration, + storeApp: context.service.storeGitHubApp, + storeCredentials: async (_id, _user, values) => { + if (++writes === 1) { + entered(); + await storeGate; + } + verifier = values.identityCodeVerifier!; + if (verifierSecretId) { + await vault.rotate(verifierSecretId, { value: verifier }); + return; + } + const secret = await vault.create(f.companyId, { + name: `wizard-verifier-${randomUUID()}`, + provider: "local_encrypted", + value: verifier, + }); + verifierSecretId = secret.id; + await vault.createBinding({ + companyId: f.companyId, + secretId: secret.id, + targetType: "tool_connection", + targetId: connection!.id, + configPath: "credentials.identityCodeVerifier", + }); + await db + .update(toolConnections) + .set({ + credentialSecretRefs: [ + ...refs, + { + configPath: "credentials.identityCodeVerifier", + secretId: secret.id, + versionSelector: "latest", + }, + ], + }) + .where(eq(toolConnections.id, connection!.id)); + }, + fetch: (async (input, init) => { + if (String(input).endsWith("/access_token")) { + expect(JSON.parse(String(init?.body)).code_verifier).toBe(verifier); + callbackEntered(); + await exchangeGate; + return Response.json({ access_token: "transient" }); + } + if (String(input).endsWith("/user")) + return Response.json({ + id: 1357, + login: "octocat", + avatar_url: "https://example.test/avatar", + }); + if (String(input).includes("/user/installations?")) + return Response.json({ + installations: [{ id: 2468, app_id: Number(connectedAppId) }], + }); + throw new Error(`Unexpected identity request ${String(input)}`); + }) as typeof fetch, + refreshRepositories: context.service.refreshGitHubRepositories, + resources: context.service.listResources, + replaceResources: context.service.replaceResources, + configure: async () => {}, + finish: async () => {}, + }); + const first = wizard.startIdentity(context.endpoint.id, "owner-user"); + await Promise.race([ + storing, + first.then(() => { + throw new Error("Expected a paused vault write"); + }), + ]); + await expect( + wizard.startIdentity(context.endpoint.id, "owner-user"), + ).rejects.toThrow("in progress"); + release(); + const authorization = await first, + state = new URL(authorization.authorizationUrl).searchParams.get("state")!; + const callback = wizard.identityCallback(state, "code"); + await Promise.race([ + exchanging, + callback.then(() => { + throw new Error("Expected a paused OAuth exchange"); + }), + ]); + await expect( + wizard.startIdentity(context.endpoint.id, "owner-user"), + ).rejects.toThrow("in progress"); + callbackRelease(); + await callback; + const [session] = await db + .select() + .from(chatGitHubRegistrations) + .where(eq(chatGitHubRegistrations.endpointId, context.endpoint.id)); + expect(session!.handoff).toMatchObject({ + returnState: "preserve-return", + redemptionId: "preserve-receipt", + webhookSecretHash: "preserve-hash", + identity: { githubUserId: "1357", login: "octocat" }, + }); + expect(session!.handoff?.identityStateHash).toBeUndefined(); + expect(session!.handoff?.identityLeaseId).toBeUndefined(); + expect(writes).toBe(2); + }); + it("imports initial GitHub access without a second picker and leaves later additions disabled", async () => { + const f = await seedCompany(); + const context = await configuredGitHubEndpoint(f, {}, true); + const current = await context.service.get(context.endpoint.id); + const first = await context.service.listResources(current.id); + // Inventory before any user selection: simulate a fresh provider import, + // rather than an explicit user decision to disable every repository. + await db + .update(chatEndpointResources) + .set({ enabled: false }) + .where(eq(chatEndpointResources.endpointId, current.id)); + await db + .update(chatEndpoints) + .set({ + setup: { + ...current.setup, + github: { + ...current.setup.github, + repositorySelectionSaved: undefined, + initialRepositoryImportPending: true, + }, + }, + }) + .where(eq(chatEndpoints.id, current.id)); + await db.insert(chatGitHubRegistrations).values({ + companyId: f.companyId, + endpointId: current.id, + userId: "owner-user", + stateHash: randomUUID(), + trustedOrigin: "https://reviews.example.test", + ownerType: "organization", + ownerLogin: "paperclipai", + status: "completed", + expiresAt: new Date(Date.now() + 60_000), + }); const result = await context.service.githubWizard.advance( current.id, "owner-user", diff --git a/server/src/__tests__/openapi-routes.test.ts b/server/src/__tests__/openapi-routes.test.ts index ac83f76c55..3e02c834ee 100644 --- a/server/src/__tests__/openapi-routes.test.ts +++ b/server/src/__tests__/openapi-routes.test.ts @@ -808,6 +808,21 @@ describe("openapi routes", () => { expect(replacement.responses["422"]).toBeDefined(); }); + it("documents the manager-only GitHub wizard and compatible owner-aware registration", () => { + const { spec } = loadSpecRoutes(); + for (const [suffix, method] of [["draft", "put"], ["setup", "post"], ["identity/start", "post"], ["identity/confirm", "post"]]) { + const operation = spec.paths[`/api/chat-endpoints/{endpointId}/github/${suffix}`][method]; + expect(operation["x-paperclip-authorization"]).toEqual({ actor: "board" }); + expect(operation.security).toEqual([{ BoardSessionAuth: [] }, { BoardApiKeyAuth: [] }]); + } + const registration = spec.paths["/api/chat-endpoints/{endpointId}/github/registration"].post; + const input = registration.requestBody.content["application/json"].schema; + expect(input.properties.ownerType.enum).toEqual(["personal", "organization"]); + expect(input.required).not.toContain("ownerType"); + const response = registration.responses["200"].content["application/json"].schema; + expect(response.oneOf ?? response.anyOf).toHaveLength(2); + }); + it("documents auth and reviewed response-code invariants", () => { const { spec } = loadSpecRoutes(); diff --git a/server/src/routes/openapi.ts b/server/src/routes/openapi.ts index 982f916f06..997a9d2634 100644 --- a/server/src/routes/openapi.ts +++ b/server/src/routes/openapi.ts @@ -2288,6 +2288,16 @@ const githubConfigurationResponseSchema = z.object({ updatedAt: z.string().optional(), }); const githubPersonResponseSchema = z.object({ githubUserId: z.string(), login: z.string() }); +const githubWizardInputSchema = z.object({ name: z.string().trim().min(1).max(34), ownerType: z.enum(["personal", "organization"]), ownerLogin: z.string().regex(/^[A-Za-z0-9](?:[A-Za-z0-9-]{0,37}[A-Za-z0-9])?$/).optional() }).strict(); +const githubWizardCheckSchema = z.object({ key: z.string(), label: z.string(), ok: z.boolean(), detail: z.string() }); +const githubWizardResponseSchema = z.object({ + endpointId: z.string().uuid(), state: z.enum(["create", "install", "identity", "verify", "connected", "recovery", "enrollment"]), + registration: z.object({ registrationUrl: z.string().url(), manifest: z.record(z.string(), z.unknown()), expiresAt: z.string() }).optional(), + installationUrl: z.string().url().optional(), identity: githubPersonResponseSchema.extend({ avatarUrl: z.string().nullable() }).optional(), + identityLinked: z.boolean().optional(), identityMethod: z.enum(["dedicated_app", "existing_connection"]).optional(), + verification: z.object({ ready: z.boolean(), checks: z.array(githubWizardCheckSchema) }).optional(), + runtimeChecks: z.array(githubWizardCheckSchema).optional(), message: z.string().optional(), +}); const githubBotOperations: Array<{ method: string; suffix: string; @@ -2349,10 +2359,14 @@ const githubBotOperations: Array<{ }, { method: "post", suffix: "registration", summary: "Prepare GitHub App manifest registration", - description: "Creates expiring single-use state bound to the current user, company, endpoint, and trusted HTTPS origin. Return data contains the manifest and registration URL, never private App credentials. Response is not cached.", - body: z.object({ name: z.string().trim().min(1).max(34) }).strict(), - response: z.object({ expiresAt: z.string(), registrationUrl: z.string().url(), manifest: z.record(z.string(), z.unknown()) }), + description: "Starts or resumes a private App under a personal account or organization. Supplying ownerType uses the two-screen wizard and Cloud callbacks, including localhost; omitting it preserves legacy direct registration. State remains bound to the member, company, draft, connection, agent, and trusted origin. Credentials are never returned.", + body: githubWizardInputSchema.partial({ ownerType: true }).refine(value => value.ownerType !== "organization" || !!value.ownerLogin, "Enter the GitHub organization"), + response: z.union([z.object({ expiresAt: z.string(), registrationUrl: z.string().url(), manifest: z.record(z.string(), z.unknown()) }), githubWizardResponseSchema]), }, + { method: "put", suffix: "draft", summary: "Save GitHub App setup choices", description: "Saves the App name and ownership choice on the same assigned draft. Requires connection-management access; creates no provider resources.", body: githubWizardInputSchema, response: chatEndpointResponseSchema }, + { method: "post", suffix: "setup", summary: "Advance GitHub App setup", description: "Resumes credential delivery, installation discovery, repository import, and verification. Returns redacted progress or the required human action. Runtime execution is reported separately.", response: githubWizardResponseSchema }, + { method: "post", suffix: "identity/start", summary: "Authorize your identity with the dedicated GitHub App", description: "Starts a member-bound OAuth authorization with PKCE. Does not grant bot execution access through personal credentials.", response: z.object({ authorizationUrl: z.string().url() }) }, + { method: "post", suffix: "identity/confirm", summary: "Confirm the observed GitHub identity", description: "Confirms only the signed-in configuring member's expiring observed identity and resumes the same setup draft.", body: z.object({ githubUserId: z.string().regex(/^[1-9][0-9]*$/) }).strict(), response: githubWizardResponseSchema }, { method: "post", suffix: "app", summary: "Connect an existing GitHub App", description: "Validates App identity with GitHub and vaults write-only credentials server-side. Installation and signed webhook delivery must still be verified.", diff --git a/server/src/services/chat-channels.ts b/server/src/services/chat-channels.ts index 08bd506661..4eded12693 100644 --- a/server/src/services/chat-channels.ts +++ b/server/src/services/chat-channels.ts @@ -9530,7 +9530,7 @@ export function chatChannelService(db: Db, options: ChatChannelServiceOptions) { ) { throw unprocessable("Unsupported chat endpoint setup action"); } - if (!getWebhookPublicBaseUrl() && endpoint.provider !== "discord" && endpoint.provider !== "imessage-photon") { + if (!getWebhookPublicBaseUrl() && endpoint.provider !== "discord" && endpoint.provider !== "imessage-photon" && !(endpoint.provider === "github" && endpoint.setup.github?.cloudRegistrationId)) { throw unprocessable( `A public HTTPS Paperclip URL is required before connecting ${PROVIDER_LABELS[endpoint.provider]}`, ); @@ -27959,6 +27959,7 @@ export function chatChannelService(db: Db, options: ChatChannelServiceOptions) { endpointId: string, updates: Array<{ id: string; enabled: boolean }>, actorUserId?: string | null, + options?: { initialGitHubImport?: boolean }, ) { const initial = await endpointRecord(endpointId); if (!initial) throw notFound("Chat endpoint not found"); @@ -27977,6 +27978,7 @@ export function chatChannelService(db: Db, options: ChatChannelServiceOptions) { companyId: chatEndpoints.companyId, connectionId: chatEndpoints.connectionId, provider: chatEndpoints.provider, + setup: chatEndpoints.setup, }) .from(chatEndpoints) .where( @@ -27987,6 +27989,7 @@ export function chatChannelService(db: Db, options: ChatChannelServiceOptions) { ) .for("no key update"); if (!endpoint) throw notFound("Chat endpoint not found"); + if (options?.initialGitHubImport && endpoint.provider === "github" && (endpoint.setup.github?.repositorySelectionSaved || !endpoint.setup.github?.initialRepositoryImportPending)) return; const rows = await tx .select({ id: chatEndpointResources.id, @@ -28040,6 +28043,8 @@ export function chatChannelService(db: Db, options: ChatChannelServiceOptions) { eq(chatEndpointResources.id, entry.id), ), ); + if (endpoint.provider === "github") + await tx.update(chatEndpoints).set({ setup: sql`jsonb_set(${chatEndpoints.setup}, '{github}', coalesce(${chatEndpoints.setup}->'github', '{}'::jsonb) || '{"repositorySelectionSaved":true,"initialRepositoryImportPending":false}'::jsonb)`, updatedAt: new Date() }).where(eq(chatEndpoints.id, endpointId)); if (changes.length > 0) await logActivity( tx as unknown as Db, diff --git a/server/src/services/chat-github-wizard.ts b/server/src/services/chat-github-wizard.ts index 94de7f2305..869515acba 100644 --- a/server/src/services/chat-github-wizard.ts +++ b/server/src/services/chat-github-wizard.ts @@ -81,6 +81,7 @@ export function githubChatWizardService( endpointId: string, updates: Array<{ id: string; enabled: boolean }>, userId: string, + options?: { initialGitHubImport?: boolean }, ) => Promise; configure: (endpointId: string, userId: string) => Promise; finish: (endpointId: string, userId: string) => Promise; @@ -206,6 +207,13 @@ export function githubChatWizardService( ); } await saveDraft(id, userId, input); + // Only a fresh registration may import GitHub's initial selection. Old + // drafts without this marker retain even an entirely disabled selection. + if ( + !bot.setup.github?.repositorySelectionSaved && + !(await options.resources(id)).length + ) + await setup(bot, { initialRepositoryImportPending: true }); const cloud = connector(); if (!cloud) { if (origin().startsWith("https://")) @@ -717,6 +725,8 @@ export function githubChatWizardService( session && bot.status !== "active" && saved.revision === 0 && + bot.setup.github?.initialRepositoryImportPending === true && + !bot.setup.github?.repositorySelectionSaved && !resources.some((resource) => resource.enabled) ) { await options.replaceResources( @@ -725,6 +735,7 @@ export function githubChatWizardService( .filter((resource) => resource.availability === "available") .map((resource) => ({ id: resource.id, enabled: true })), userId, + { initialGitHubImport: true }, ); } if (session && saved.revision === 0 && bot.status !== "active") @@ -828,7 +839,95 @@ export function githubChatWizardService( .limit(1); return !!link; } + async function withIdentityLease( + sessionId: string, + work: (assertOwned: () => Promise) => Promise, + ): Promise { + const leaseId = nonce(); + const leaseUntil = () => new Date(Date.now() + 120_000).toISOString(); + const [claimed] = await db + .update(chatGitHubRegistrations) + .set({ + handoff: sql`coalesce(${chatGitHubRegistrations.handoff}, '{"cloudId":"","returnState":"","redemptionId":""}'::jsonb) || ${JSON.stringify({ identityLeaseId: leaseId, identityLeaseExpiresAt: leaseUntil() })}::jsonb`, + }) + .where( + and( + eq(chatGitHubRegistrations.id, sessionId), + sql`(${chatGitHubRegistrations.handoff}->>'identityLeaseExpiresAt' is null or ${chatGitHubRegistrations.handoff}->>'identityLeaseExpiresAt' <= ${new Date().toISOString()})`, + ), + ) + .returning({ id: chatGitHubRegistrations.id }); + if (!claimed) + throw conflict( + "Another account-linking attempt is in progress. Try again shortly.", + ); + let lost = false; + const assertOwned = async () => { + const [owned] = await db + .select({ id: chatGitHubRegistrations.id }) + .from(chatGitHubRegistrations) + .where( + and( + eq(chatGitHubRegistrations.id, sessionId), + sql`${chatGitHubRegistrations.handoff}->>'identityLeaseId' = ${leaseId}`, + sql`${chatGitHubRegistrations.handoff}->>'identityLeaseExpiresAt' > ${new Date().toISOString()}`, + ), + ); + if (lost || !owned) + throw conflict("Account-linking changed. Connect your account again."); + }; + const renewal = setInterval(() => { + void db + .update(chatGitHubRegistrations) + .set({ + handoff: sql`jsonb_set(${chatGitHubRegistrations.handoff}, '{identityLeaseExpiresAt}', ${JSON.stringify(leaseUntil())}::jsonb)`, + }) + .where( + and( + eq(chatGitHubRegistrations.id, sessionId), + sql`${chatGitHubRegistrations.handoff}->>'identityLeaseId' = ${leaseId}`, + ), + ) + .returning({ id: chatGitHubRegistrations.id }) + .then((rows) => { + if (!rows.length) lost = true; + }) + .catch(() => { + lost = true; + }); + }, 30_000); + renewal.unref(); + try { + return await work(assertOwned); + } finally { + clearInterval(renewal); + await db + .update(chatGitHubRegistrations) + .set({ + handoff: sql`${chatGitHubRegistrations.handoff} - 'identityLeaseId' - 'identityLeaseExpiresAt'`, + }) + .where( + and( + eq(chatGitHubRegistrations.id, sessionId), + sql`${chatGitHubRegistrations.handoff}->>'identityLeaseId' = ${leaseId}`, + ), + ); + } + } async function startIdentity(id: string, userId: string) { + const bot = await endpoint(id, userId), + session = await registration(bot); + if (!session || session.userId !== userId) + throw forbidden("The configuring member must link their GitHub account"); + return withIdentityLease(session.id, (assertOwned) => + startIdentityLocked(id, userId, assertOwned), + ); + } + async function startIdentityLocked( + id: string, + userId: string, + assertOwned: () => Promise, + ) { const bot = await endpoint(id, userId), session = await registration(bot); if (!session || session.userId !== userId) @@ -843,23 +942,15 @@ export function githubChatWizardService( ); const verifier = nonce(), state = nonce(); + await assertOwned(); await options.storeCredentials(id, userId, { identityCodeVerifier: verifier, }); + await assertOwned(); await db .update(chatGitHubRegistrations) .set({ - handoff: { - ...(session.handoff ?? { - cloudId: "", - returnState: "", - redemptionId: nonce(), - }), - identityStateHash: hash(state), - identityExpiresAt: new Date(Date.now() + 10 * 60_000).toISOString(), - identityRedemptionId: nonce(), - identity: undefined, - }, + handoff: sql`(${chatGitHubRegistrations.handoff} - 'identity') || ${JSON.stringify({ identityStateHash: hash(state), identityExpiresAt: new Date(Date.now() + 10 * 60_000).toISOString(), identityRedemptionId: nonce() })}::jsonb`, }) .where(eq(chatGitHubRegistrations.id, session.id)); const challenge = createHash("sha256").update(verifier).digest("base64url"); @@ -895,6 +986,26 @@ export function githubChatWizardService( session: typeof chatGitHubRegistrations.$inferSelect, code: string, redirectUri: string, + ) { + return withIdentityLease(session.id, async (assertOwned) => { + const [current] = await db + .select() + .from(chatGitHubRegistrations) + .where(eq(chatGitHubRegistrations.id, session.id)); + if ( + !current || + current.handoff?.identityStateHash !== + session.handoff?.identityStateHash + ) + return resume(await endpoint(session.endpointId, session.userId)); + return completeIdentityLocked(current, code, redirectUri, assertOwned); + }); + } + async function completeIdentityLocked( + session: typeof chatGitHubRegistrations.$inferSelect, + code: string, + redirectUri: string, + assertOwned: () => Promise, ) { const bot = await endpoint(session.endpointId, session.userId); if ( @@ -980,21 +1091,20 @@ export function githubChatWizardService( ) throw conflict("GitHub returned an invalid account"); // OAuth token remains transient; only an expiring observed identity is saved. + await assertOwned(); + const identity = { + githubUserId: String(user.id), + login: user.login, + avatarUrl: user.avatar_url ?? null, + expiresAt: new Date(Date.now() + 10 * 60_000).toISOString(), + }; await db .update(chatGitHubRegistrations) .set({ - handoff: { - ...session.handoff!, - identityStateHash: undefined, - identity: { - githubUserId: String(user.id), - login: user.login, - avatarUrl: user.avatar_url ?? null, - expiresAt: new Date(Date.now() + 10 * 60_000).toISOString(), - }, - }, + handoff: sql`jsonb_set(${chatGitHubRegistrations.handoff}, '{identity}', ${JSON.stringify(identity)}::jsonb)`, }) .where(eq(chatGitHubRegistrations.id, session.id)); + await assertOwned(); await options.storeCredentials(bot.id, session.userId, { identityCodeVerifier: nonce(), }); @@ -1037,9 +1147,44 @@ export function githubChatWizardService( sql`${chatGitHubRegistrations.handoff}->>'cloudId' = ${registrationId}`, ) .limit(1); - if (!session?.handoff || session.trustedOrigin !== origin()) + if ( + !session?.handoff || + (session.stateHash !== hash(state) && + session.handoff.identityStateHash !== hash(state)) + ) throw forbidden("GitHub return did not match this instance"); const bot = await endpoint(session.endpointId, session.userId); + if (session.trustedOrigin !== origin()) { + const cloud = connector(); + if (!cloud) throw forbidden("GitHub return did not match this instance"); + const current = await cloud.githubApp({ + subject: session.userId, + companyId: session.companyId, + binding: { action: "status", id: registrationId }, + }); + // A manager-authenticated Cloud lookup, rather than a callback parameter, + // proves the current enrolled destination after an instance rename. + if (current.returnOrigin !== origin()) + throw forbidden("GitHub return did not match this instance"); + await db + .update(chatGitHubRegistrations) + .set({ trustedOrigin: origin() }) + .where( + and( + eq(chatGitHubRegistrations.id, session.id), + eq(chatGitHubRegistrations.trustedOrigin, session.trustedOrigin), + ), + ); + await logActivity(db, { + companyId: session.companyId, + actorType: "system", + actorId: "chat-github-setup", + action: "chat_github.registration_origin_recovered", + entityType: "tool_connection", + entityId: bot.connectionId, + details: { registrationId: session.id }, + }); + } if (session.handoff.identityStateHash === hash(state)) { if (!claimId) { await db @@ -1047,7 +1192,12 @@ export function githubChatWizardService( .set({ handoff: sql`${chatGitHubRegistrations.handoff} - 'identityStateHash'`, }) - .where(eq(chatGitHubRegistrations.id, session.id)); + .where( + and( + eq(chatGitHubRegistrations.id, session.id), + sql`${chatGitHubRegistrations.handoff}->>'identityStateHash' = ${hash(state)}`, + ), + ); return resume(bot); } const cloud = connector(); @@ -1125,11 +1275,23 @@ export function githubChatWizardService( throw conflict( "Connect your GitHub account again before confirming its identity", ); - await management.linkObservedIdentity(id, userId, candidate); - await db - .update(chatGitHubRegistrations) - .set({ handoff: { ...session.handoff!, identity: undefined } }) - .where(eq(chatGitHubRegistrations.id, session.id)); + await withIdentityLease(session.id, async (assertOwned) => { + const [current] = await db + .select() + .from(chatGitHubRegistrations) + .where(eq(chatGitHubRegistrations.id, session.id)); + if ( + current?.handoff?.identity?.githubUserId !== githubUserId || + current.handoff.identity.expiresAt !== candidate.expiresAt + ) + throw conflict("GitHub identity changed. Connect your account again."); + await assertOwned(); + await management.linkObservedIdentity(id, userId, candidate); + await db + .update(chatGitHubRegistrations) + .set({ handoff: sql`${chatGitHubRegistrations.handoff} - 'identity'` }) + .where(eq(chatGitHubRegistrations.id, session.id)); + }); return advance(id, userId); } return { diff --git a/tests/e2e/chat-adapters-ui-providers.spec.ts b/tests/e2e/chat-adapters-ui-providers.spec.ts index 4b8cfca57d..3b5c158b42 100644 --- a/tests/e2e/chat-adapters-ui-providers.spec.ts +++ b/tests/e2e/chat-adapters-ui-providers.spec.ts @@ -20,45 +20,44 @@ import { async function exerciseGitHubReviewSetup(page: Page, mock: ChatMock, seed: Seed, provider: ProviderCase) { await expect(page.getByRole("heading", { name: "Choose agent", exact: true })).toBeVisible(); - await page.getByRole("button", { name: "Choose an agent", exact: true }).click(); + await page.getByRole("button", { name: "Select agent…", exact: true }).click(); await page.getByRole("button", { name: "Select Maya", exact: true }).click(); await expect(page.getByText("Maya is not configured for low-trust review")).toBeVisible(); await expect(page.getByRole("link", { name: "Learn about low-trust agents" })).toHaveAttribute("href", /trust-and-low-trust-review/); + await page.getByRole("button", { name: "Change Maya to a low trust agent" }).click(); + await expect(page.getByText("Maya is not configured for low-trust review")).toHaveCount(0); await page.getByRole("button", { name: "Continue", exact: true }).click(); await expect.poll(() => mock.createdWithAgentId).toBe(seed.agentId); - await expect(page.getByRole("heading", { name: "Connect GitHub App", exact: true })).toBeVisible(); - await page.getByRole("button", { name: "Use an existing App" }).click(); + await expect(page.getByRole("heading", { name: "Connect GitHub", exact: true })).toBeVisible(); + await expect(page.getByLabel("GitHub account")).toHaveValue("personal"); + await expect(page.getByLabel("App name", { exact: true })).toHaveValue("Maya"); + await page.getByLabel("GitHub account").selectOption("organization"); + await page.getByLabel("Organization", { exact: true }).fill("paperclip-ai"); + await page.getByRole("button", { name: "Save & exit", exact: true }).click(); + await page.goto(`/${seed.prefix}/apps/chat/connect?provider=github&purpose=chat&resume=endpoint-github`); + await expect(page.getByLabel("GitHub account")).toHaveValue("organization"); + await expect(page.getByLabel("Organization", { exact: true })).toHaveValue("paperclip-ai"); + await expect(page.getByRole("button", { name: "Select agent…", exact: true })).toHaveCount(0); + // Existing-App recovery stays available and preserves entered credentials after failure. + await page.getByRole("button", { name: "I already have an App" }).click(); await page.getByLabel("App ID", { exact: true }).fill("123456"); await page.getByLabel("Private key", { exact: true }).fill(GITHUB_PRIVATE_KEY_PASTE_FIXTURE); await page.getByLabel("Webhook secret", { exact: true }).fill("github-webhook-secret"); - await page.getByRole("button", { name: "Connect App", exact: true }).click(); + await page.getByRole("button", { name: "Connect existing App", exact: true }).click(); await expect(page.getByRole("alert")).toContainText("GitHub rejected the supplied App credentials."); await expect(page.getByLabel("Private key", { exact: true })).toHaveValue(GITHUB_PRIVATE_KEY_PASTE_FIXTURE); - await page.getByRole("button", { name: "Connect App", exact: true }).click(); - await expect(page.getByRole("heading", { name: "Install GitHub App", exact: true })).toBeVisible(); - await expect(page.getByText(provider.resourceLabel, { exact: true })).toHaveCount(0); - await page.getByRole("button", { name: "I’ve installed the App" }).click(); - await expect(page.getByRole("heading", { name: "Select repositories", exact: true })).toBeVisible(); - await expect(page.getByRole("link", { name: "Configure access on GitHub" })).toHaveAttribute("href", "https://github.com/settings/installations/2468"); - await page.getByRole("button", { name: "Refresh access" }).click(); - await expect.poll(() => mock.githubRepositoryRefreshes).toBe(2); - await page.getByRole("switch", { name: provider.resourceLabel, exact: true }).click(); - await page.getByRole("button", { name: "Save repositories" }).click(); - await expect(page.getByRole("heading", { name: "Verify connection & tools", exact: true })).toBeVisible(); - await page.getByRole("button", { name: "Verify connection", exact: true }).click(); - await expect(page.getByRole("button", { name: "Continue", exact: true })).toBeDisabled(); - await page.getByRole("button", { name: "Assign this bot’s GitHub tools" }).click(); - await page.getByRole("button", { name: "Continue", exact: true }).click(); + await page.getByRole("button", { name: "Connect existing App", exact: true }).click(); await expect(page.getByRole("heading", { name: "Connect your account", exact: true })).toBeVisible(); - await page.getByLabel("Your GitHub connection").selectOption("personal-github"); - await page.getByRole("button", { name: "Verify my account" }).click(); + await page.getByLabel("Existing GitHub connection").selectOption("personal-github"); + await expect(page.getByRole("button", { name: "Check my account" })).toBeVisible(); + await page.getByRole("button", { name: "Check my account" }).click(); expect(mock.githubIdentityConfirmed).toBe(false); - await page.getByRole("button", { name: "Confirm this is my account" }).click(); + await page.getByRole("button", { name: "Confirm my account" }).click(); await expect.poll(() => mock.githubIdentityConfirmed).toBe(true); - await expect(page.getByRole("heading", { name: "Configure behavior", exact: true })).toBeVisible(); - await page.getByRole("button", { name: "Save behavior" }).click(); - await expect(page.getByRole("heading", { name: "Try it", exact: true })).toBeVisible(); - await page.getByRole("button", { name: "Finish without test" }).click(); + await expect(page.getByRole("heading", { name: "GitHub connected", exact: true })).toBeVisible(); + await expect(page.getByText(provider.resourceLabel, { exact: true })).toBeVisible(); + await expect(page.getByRole("button", { name: /Finish|Verify connection|Refresh access|Save repositories/ })).toHaveCount(0); + await page.getByRole("link", { name: "Connection settings", exact: true }).click(); await expect(page).toHaveURL(/\/apps\/chat\/endpoint-github\/settings$/); const nav = page.getByRole("navigation", { name: "Chat connection" }); for (const tab of ["Settings", "Access", "Reviews", "Conversations", "Activity"]) { @@ -82,8 +81,8 @@ async function exerciseGitHubReviewSetup(page: Page, mock: ChatMock, seed: Seed, await page.reload(); await page.getByText("Connection health and controls", { exact: true }).click(); await page.getByRole("button", { name: "Reconnect", exact: true }).click(); - await expect(page.getByRole("heading", { name: "Connect GitHub App", exact: true })).toBeVisible(); - await expect(page.getByText(/Leave the credentials blank to keep them/)).toBeVisible(); + await expect(page.getByRole("heading", { name: "Connect GitHub", exact: true })).toBeVisible(); + await expect(page.getByText(/Leave these blank to reuse/)).toBeVisible(); await expect(page.getByRole("button", { name: "Choose an agent", exact: true })).toHaveCount(0); await expect(page.getByLabel("Private key", { exact: true })).toHaveValue(""); const reconnectResponse = page.waitForResponse((response) => @@ -95,7 +94,7 @@ async function exerciseGitHubReviewSetup(page: Page, mock: ChatMock, seed: Seed, const reconnected = await reconnectResponse; expect(reconnected.request().postDataJSON()).toEqual({ action: "reconnect" }); expect(await reconnected.json()).toMatchObject({ assignedAgentId: seed.agentId, assignedAgentName: "Maya" }); - await expect(page.getByRole("heading", { name: "Verify connection & tools", exact: true })).toBeVisible(); + await expect(page.getByRole("heading", { name: "GitHub connected", exact: true })).toBeVisible(); mock.setStatus("active"); await page.goto(`/${seed.prefix}/apps/chat/endpoint-github/settings`); await expect(page.getByText(/Maya is permanently assigned to this bot/)).toBeVisible(); diff --git a/tests/e2e/chat-adapters-ui.shared.ts b/tests/e2e/chat-adapters-ui.shared.ts index be27e72cb6..bcd30d6b54 100644 --- a/tests/e2e/chat-adapters-ui.shared.ts +++ b/tests/e2e/chat-adapters-ui.shared.ts @@ -425,6 +425,18 @@ export async function installChatControlPlaneMock( if (provider.provider === "github" && pathname.startsWith(`/api/chat-endpoints/${endpoint.id}/github/`)) { const operation = pathname.split("/github/")[1]; const body = method === "GET" ? {} : bodyOf(route); + if (operation === "setup") { + if (!githubAppConnected) { await fulfill(route, { endpointId: endpoint.id, state: "create" }); return; } + if (!state.githubIdentityConfirmed) { await fulfill(route, { endpointId: endpoint.id, state: "identity", identityMethod: "existing_connection" }); return; } + githubConfiguration.configuration.toolsEnabled = true; + resource.enabled = true; + Object.assign(endpoint, { status: "active", botExternalId: "123456", resources: [resource], setup: { ...endpoint.setup, step: "complete" } }); + await fulfill(route, { endpointId: endpoint.id, state: "connected", identityLinked: true, verification: { ready: true, checks: [] } }); return; + } + if (operation === "draft") { + Object.assign(endpoint.setup, { github: { ...endpoint.setup.github, appName: body.name, ownerType: body.ownerType, ownerLogin: body.ownerLogin } }); + await fulfill(route, endpoint); return; + } if (operation === "configuration") { if (method === "PUT") { expect(body.expectedRevision).toBe(githubConfiguration.revision); githubConfiguration = { revision: githubConfiguration.revision + 1, configuration: body.configuration as typeof githubConfiguration.configuration }; } await fulfill(route, githubConfiguration); return; @@ -438,7 +450,7 @@ export async function installChatControlPlaneMock( state.configuredCredentialKeys = Object.keys(body).sort(); if (state.setupAttempts === 1) { await fulfill(route, { error: "GitHub rejected the supplied App credentials." }, 422); return; } githubAppConnected = true; - Object.assign(endpoint, { status: "attention", botUsername: "maya-paperclip[bot]" }); + Object.assign(endpoint, { status: "attention", botExternalId: "123456", botUsername: "maya-paperclip[bot]", botLabel: "Maya" }); Object.assign(endpoint.setup, { github: { stage: "install", appSlug: "maya-paperclip", installationUrl: "https://github.com/apps/maya-paperclip/installations/new", managementUrl: "https://github.com/settings/installations/2468" } }); await fulfill(route, endpoint); return; } diff --git a/ui/src/pages/apps/chat/ChatEndpoint.clipboard.test.tsx b/ui/src/pages/apps/chat/ChatEndpoint.clipboard.test.tsx index 2db55be955..5116d38411 100644 --- a/ui/src/pages/apps/chat/ChatEndpoint.clipboard.test.tsx +++ b/ui/src/pages/apps/chat/ChatEndpoint.clipboard.test.tsx @@ -37,6 +37,7 @@ vi.mock("@/api/chatEndpoints", () => ({ chatEndpointsApi: mocks })); vi.mock("@/api/githubChat", () => ({ githubChatApi: { configuration: async () => ({ revision: 0, configuration: {} }), progress: async () => mocks.get(), + advance: async () => ({ endpointId: "endpoint-a", state: "create" }), } })); vi.mock("@/api/auth", () => ({ authApi: { getSession: async () => ({ user: { id: "owner-user", name: "Owner" } }) } })); vi.mock("@/api/health", () => ({ healthApi: { get: async () => ({ deploymentMode: "authenticated" }) } })); @@ -587,13 +588,13 @@ describe("chat setup and identity-link clipboard actions", () => { expect(container.querySelector("h1")?.textContent).toBe("Add Slack credentials"); }); - it.each(["slack", "github", "discord", "telegram", "microsoft-teams"] as const)( + it.each(["slack", "discord", "telegram", "microsoft-teams"] as const)( "lets %s setup revisit the agent without duplicating the connection or losing provider fields", async (provider) => { await render(provider); - const nav = container.querySelector(provider === "github" ? 'nav[aria-label="Setup progress"]' : 'aside nav[aria-label="Connection setup progress"]')!; + const nav = container.querySelector('aside nav[aria-label="Connection setup progress"]')!; expect(nav).not.toBeNull(); - if (provider !== "github") expect(container.querySelector("main nav")).toBeNull(); + expect(container.querySelector("main nav")).toBeNull(); const steps = [...nav.querySelectorAll("button")]; expect(steps[1].getAttribute("aria-current")).toBe("step"); expect(steps[2].disabled).toBe(true); @@ -607,55 +608,30 @@ describe("chat setup and identity-link clipboard actions", () => { expect([...container.querySelectorAll("main input")]).toEqual(fields); expect(mocks.create).not.toHaveBeenCalled(); await click("1Choose agent"); - await click(provider === "slack" ? "2Create Slack app" : provider === "github" ? "2Connect GitHub App" : "2Connect provider"); + await click(provider === "slack" ? "2Create Slack app" : "2Connect provider"); expect(steps[1].getAttribute("aria-current")).toBe("step"); }, ); - it("copies the one-time webhook secret through the same fallback", async () => { + it("resumes an assigned GitHub draft in the two-screen wizard without creating a connection", async () => { await render("github"); - await click("Use an existing App"); - await click("Generate webhook secret"); - await click("Copy webhook secret"); - expect(copied).toEqual([secret]); - expect(writeText).not.toHaveBeenCalled(); + const steps = [...container.querySelectorAll('nav[aria-label="Setup progress"] button')]; + expect(steps).toHaveLength(2); + expect(steps[1].getAttribute("aria-current")).toBe("step"); + expect(container.querySelector("h1")?.textContent).toBe("Connect GitHub"); + expect(container.querySelector("input#github-app-name")?.getAttribute("value")).toBe("Maya"); + expect(mocks.create).not.toHaveBeenCalled(); }); - it("reports a failed secret copy without exposing the secret or an unhandled rejection", async () => { + it("keeps manual existing-App recovery write-only without generating or copying credentials", async () => { await render("github"); - await click("Use an existing App"); - await click("Generate webhook secret"); - execCommand.mockReturnValue(false); - await click("Copy webhook secret"); - expect(mocks.pushToast).toHaveBeenLastCalledWith({ - title: "Couldn't copy to clipboard", - body: "Select and copy the value manually.", - tone: "error", - }); - expect(JSON.stringify(mocks.pushToast.mock.calls)).not.toContain(secret); - }); - - // The toast provider drops an identical toast raised inside 3.5 seconds, so - // a reader clicking twice against a blocked clipboard would see the failure - // once and then nothing. The inline state has to answer every click. - it("still shows a repeated copy failure the toast would have deduplicated", async () => { - await render("github"); - await click("Use an existing App"); - await click("Generate webhook secret"); - execCommand.mockReturnValue(false); - - await click("Copy webhook secret"); - expect(container.textContent).toContain("Couldn’t copy"); - - // Same failure again, well inside the dedupe window: the second toast is - // suppressed, so the button itself is the only thing left to say so. - const toastsAfterFirst = mocks.pushToast.mock.calls.length; - await click("Couldn’t copy — select it manually"); - expect(container.textContent).toContain("Couldn’t copy"); - expect(container.textContent).not.toContain("Webhook secret copied"); - expect(mocks.pushToast.mock.calls.length).toBeGreaterThanOrEqual( - toastsAfterFirst, - ); + await click("I already have an App"); + expect(container.querySelector('input[type="password"]')).not.toBeNull(); + expect(container.querySelector("textarea")?.value).toBe(""); + expect(container.textContent).not.toContain("Copy webhook secret"); + expect(container.textContent).not.toContain("Generate webhook secret"); + expect(mocks.generateSetupSecret).not.toHaveBeenCalled(); + expect(copied).toEqual([]); }); it("copies the private identity link with fallback and preserves success feedback", async () => { diff --git a/ui/src/pages/apps/chat/GitHubChatSetup.test.tsx b/ui/src/pages/apps/chat/GitHubChatSetup.test.tsx index e3628b87bd..fdccb91338 100644 --- a/ui/src/pages/apps/chat/GitHubChatSetup.test.tsx +++ b/ui/src/pages/apps/chat/GitHubChatSetup.test.tsx @@ -61,7 +61,12 @@ vi.mock("@/api/agents", () => ({ agentsApi: { list: vi.fn(), get: vi.fn(), updatePermissions: vi.fn() }, })); vi.mock("@/api/chatEndpoints", () => ({ - chatEndpointsApi: { create: vi.fn(), get: vi.fn(), listResources: vi.fn() }, + chatEndpointsApi: { + create: vi.fn(), + get: vi.fn(), + listResources: vi.fn(), + setup: vi.fn(), + }, })); vi.mock("@/api/githubChat", () => ({ githubChatApi: { @@ -173,6 +178,24 @@ describe("GitHub App wizard", () => { await new Promise((resolve) => setTimeout(resolve, 20)); }); } + it("reconnects the same App using vaulted credentials without requiring another paste", async () => { + fixture.endpoint.status = "attention"; + fixture.endpoint.botExternalId = "1234"; + vi.mocked(chatEndpointsApi.setup).mockResolvedValue(fixture.endpoint); + await render("resume=draft-1&reconnect=1"); + const button = [...container.querySelectorAll("button")].find( + (button) => button.textContent === "Reconnect App", + )!; + expect(button.disabled).toBe(false); + expect(container.querySelector("#github-webhook-secret")).toBeNull(); + await act(async () => { + button.click(); + }); + expect(chatEndpointsApi.setup).toHaveBeenCalledWith("draft-1", { + action: "reconnect", + }); + expect(githubChatApi.connectApp).not.toHaveBeenCalled(); + }); async function render(query = "agentId=reviewer") { await act(async () => root.render( diff --git a/ui/src/pages/apps/chat/GitHubChatSetup.tsx b/ui/src/pages/apps/chat/GitHubChatSetup.tsx index 9a71344161..9193635132 100644 --- a/ui/src/pages/apps/chat/GitHubChatSetup.tsx +++ b/ui/src/pages/apps/chat/GitHubChatSetup.tsx @@ -54,6 +54,7 @@ export function GitHubChatSetup() { const queryClient = useQueryClient(); const resume = params.get("resume"); const identityOnly = params.get("stage") === "identity"; + const reconnect = params.get("reconnect") === "1"; const [agentId, setAgentId] = useState(params.get("agentId") ?? ""); const [ownerType, setOwnerType] = useState<"personal" | "organization">( "personal", @@ -106,7 +107,7 @@ export function GitHubChatSetup() { (agent) => agent.id === (bot?.assignedAgentId ?? agentId), ); const state = progress.data; - const connected = state?.state === "connected"; + const connected = state?.state === "connected" && !existing; useEffect(() => { setBreadcrumbs([ { label: "Connectors", href: "/apps" }, @@ -211,7 +212,7 @@ export function GitHubChatSetup() { step={bot ? 1 : 0} availableStep={bot ? 1 : 0} onSelect={() => {}} - disabled={busy} + disabled takeover /> )} @@ -220,7 +221,7 @@ export function GitHubChatSetup() { ? "Choose agent" : connected ? "GitHub connected" - : identityOnly + : identityOnly || state?.state === "identity" ? "Connect your account" : "Connect GitHub"} @@ -333,7 +334,9 @@ export function GitHubChatSetup() { ) : existing ? ( <>

- Use the credentials from this App’s GitHub settings. + {reconnect + ? "Leave these blank to reuse this App’s stored credentials, or enter both fields to repair them." + : "Use the credentials from this App’s GitHub settings."}

- {!["active", "paused", "revoked"].includes(bot.status) && ( - <> - - - setCredentials({ - ...credentials, - webhookSecret: event.target.value, - }) - } - /> - - )} + {!reconnect && + !["active", "paused", "revoked"].includes(bot.status) && ( + <> + + + setCredentials({ + ...credentials, + webhookSecret: event.target.value, + }) + } + /> + + )} {footer( - "Connect existing App", + reconnect ? "Reconnect App" : "Connect existing App", async () => { - if (["active", "paused", "revoked"].includes(bot.status)) + if ( + reconnect || + ["active", "paused", "revoked"].includes(bot.status) + ) await chatEndpointsApi.setup(bot.id, { action: "reconnect", - credentials: { - appId: credentials.appId, - privateKey: credentials.privateKey, - }, + ...(credentials.appId.trim() || credentials.privateKey.trim() + ? { + credentials: { + appId: credentials.appId, + privateKey: credentials.privateKey, + }, + } + : {}), }); else await githubChatApi.connectApp(bot.id, credentials); setCredentials({ appId: "", privateKey: "", webhookSecret: "" }); setExisting(false); await refresh(); }, - !credentials.appId.trim() || - !credentials.privateKey.trim() || - (!["active", "paused", "revoked"].includes(bot.status) && - !credentials.webhookSecret.trim()), + reconnect + ? !!(credentials.appId.trim() || credentials.privateKey.trim()) && + (!credentials.appId.trim() || !credentials.privateKey.trim()) + : !credentials.appId.trim() || + !credentials.privateKey.trim() || + (!["active", "paused", "revoked"].includes(bot.status) && + !credentials.webhookSecret.trim()), )} ) : state?.state === "identity" || identityOnly ? ( diff --git a/ui/storybook/prototypes/github-chat/README.md b/ui/storybook/prototypes/github-chat/README.md index a7bcdb297f..05594178a6 100644 --- a/ui/storybook/prototypes/github-chat/README.md +++ b/ui/storybook/prototypes/github-chat/README.md @@ -1,7 +1,8 @@ # GitHub chat and review bot — design approval preview > Historical design prototype: the production wizard now starts with agent selection, -> followed by setup-method selection with copying or assignment to a setup helper. +> followed by Connect GitHub for personal or organization App creation and installation. +> Setup methods, copied prompts and setup helpers are deferred. > Use the running app and `ui/src/pages/apps/chat/GitHubChatSetup.tsx` for the current flow. Open **Apps / GitHub chat & reviews / 00 · Start here**. All provider handoffs,