From f46360a35137cad2cdcf13d03edc639bb4ad500e Mon Sep 17 00:00:00 2001 From: Dotta Date: Wed, 7 Oct 2026 00:47:00 -0500 Subject: [PATCH] fix(github): preserve legacy repository selection and qualify fixtures Co-Authored-By: Paperclip --- doc/plans/2026-10-06-github-app-wizard.md | 17 +++-- .../chat-channels.integration.test.ts | 75 ++++++++++--------- server/src/services/chat-channels.ts | 4 +- .../src/services/chat-github-registration.ts | 21 +++++- tests/e2e/chat-adapters-ui-providers.spec.ts | 10 ++- 5 files changed, 75 insertions(+), 52 deletions(-) diff --git a/doc/plans/2026-10-06-github-app-wizard.md b/doc/plans/2026-10-06-github-app-wizard.md index d899b4d42a8..0b5a4a4d386 100644 --- a/doc/plans/2026-10-06-github-app-wizard.md +++ b/doc/plans/2026-10-06-github-app-wizard.md @@ -106,7 +106,7 @@ enrollment handoff passed. The test did not authorize Cloud enrollment or create a provider App. The focused wizard and retained clipboard/recovery UI suites pass (37 tests). -Backend wizard coverage passes (14 cases), including both owner types, +GitHub backend coverage passes (189 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 @@ -115,16 +115,19 @@ browser journey passes through manual recovery, identity confirmation, automatic completion, settings, and reconnect. 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 +and three suites with database-startup timeouts. All ten failing suites passed +in isolation (303 tests). The original full run remains a failed run; it 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. +covered by the updated focused checks. The full GitHub fixture also verifies +installation permissions and preserved legacy tool authority. The browser +report attaches screenshots of both wizard screens. -Cloud's final standard `npm test` passes: 2,559 tests and 73 environment-dependent +Cloud's final standard `npm test` passes: 2,561 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 +callback destinations after old hosts stop resolving, PKCE consent and denial, expired claims, and webhook +secret retirement with nonblocking cleanup, including overlapping repairs. The Postgres concurrency case requires a disposable database and was not run here. Before rollout, apply the additive Cloud migration, enable the dedicated-App diff --git a/server/src/__tests__/chat-channels.integration.test.ts b/server/src/__tests__/chat-channels.integration.test.ts index 91a86cc900a..0dfe8757166 100644 --- a/server/src/__tests__/chat-channels.integration.test.ts +++ b/server/src/__tests__/chat-channels.integration.test.ts @@ -8,7 +8,7 @@ import { githubChatManagementService } from "../services/chat-github-management. import { githubChatReviewService } from "../services/chat-github-reviews.js"; import { githubReviewCheckService } from "../services/chat-github-checks.js"; import { githubAutomaticReviewEvent } from "../services/chat-github-events.js"; -import { githubBotToolsForSession } from "../services/chat-github-tools.js"; +import { githubBotToolsForSession, syncGitHubBotTools } from "../services/chat-github-tools.js"; import { resolveGitHubOperationCredentials } from "../services/github-operation-credentials.js"; import { initializeRunIdentity } from "../services/run-identity.js"; import { chatGitHubRegistrations, chatGitHubReviews, toolCatalogEntries } from "@paperclipai/db"; @@ -1875,6 +1875,7 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { > > & { wakeup?: ChatChannelServiceOptions["heartbeat"]["wakeup"] } = {}, useVerifiedAppId = false, + registerWithManifest = false, ) { let setupComplete = false; const deferredSchedule = overrides.scheduleDeferredWork; @@ -1891,16 +1892,21 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { }, ]; let appPermissions: Record = { + contents: "read", + checks: "write", issues: "write", metadata: "read", pull_requests: "write", }; let installationPermissions: Record = { + contents: "read", + checks: "write", issues: "write", metadata: "read", pull_requests: "write", }; let appEvents = [ + "pull_request", "github_app_authorization", "installation", "installation_repositories", @@ -1959,6 +1965,8 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { { status: 200, headers: { "content-type": "application/json" } }, ); } + if (url === `https://api.github.com/app/installations/${installationId}`) + return Response.json({ permissions: installationPermissions, suspended_at: null }); if (url === "https://api.github.com/app/installations?per_page=100") { return new Response( JSON.stringify( @@ -2041,6 +2049,8 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { endpoint.publicId, webhookSecret, ); + if (registerWithManifest) + await context.service.startGitHubRegistration(endpoint.id, "owner-user", "Maya"); await context.service.configure( endpoint.id, { @@ -2052,10 +2062,17 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { }, "owner-user", ); + if (!useVerifiedAppId) { + const [configured] = await db.select().from(chatEndpoints).where(eq(chatEndpoints.id, endpoint.id)); + // Legacy transport fixtures already have explicit tool authority. Do not + // save a review configuration: that would switch their admission model. + await db.transaction((tx) => syncGitHubBotTools(tx, configured!, "owner-user", true)); + } const resources = await context.service.listResources(endpoint.id); - await context.service.replaceResources(endpoint.id, [ - { id: resources[0]!.id, enabled: true }, - ]); + if (!registerWithManifest) + await context.service.replaceResources(endpoint.id, [ + { id: resources[0]!.id, enabled: true }, + ]); setupComplete = true; const callbacks = context.runtime.configurations.get( endpoint.id, @@ -2858,39 +2875,11 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { }); 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 context = await configuredGitHubEndpoint(f, {}, true, 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), - }); + expect(current.setup.github?.initialRepositoryImportPending).toBe(true); + await db.update(chatGitHubRegistrations).set({ status: "completed" }) + .where(eq(chatGitHubRegistrations.endpointId, current.id)); const result = await context.service.githubWizard.advance( current.id, "owner-user", @@ -2978,6 +2967,20 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { ); expect(f.wakeup).not.toHaveBeenCalled(); }); + it("legacy name-only registration preserves an explicit empty repository selection", async () => { + const fixture = await seedCompany(); + const { service } = createService(new FakeChatSdkRuntime(), fakeSlackFetch(), { + publicBaseUrl: "https://reviews.example.test", + }); + const bot = await service.create(fixture.companyId, { + provider: "github", assignedAgentId: fixture.assignedAgentId, + }, "owner-user"); + await service.replaceResources(bot.id, [], "owner-user"); + await service.startGitHubRegistration(bot.id, "owner-user", "Maya"); + const current = await service.get(bot.id); + expect(current.setup.github?.repositorySelectionSaved).toBe(true); + expect(current.setup.github?.initialRepositoryImportPending).not.toBe(true); + }); it("uses the manifest state parameter and rejects expired, reused, and wrong-origin registrations", async () => { const fixture = await seedCompany(); const { service } = createService( diff --git a/server/src/services/chat-channels.ts b/server/src/services/chat-channels.ts index 4eded12693e..dacea199ce3 100644 --- a/server/src/services/chat-channels.ts +++ b/server/src/services/chat-channels.ts @@ -27963,7 +27963,7 @@ export function chatChannelService(db: Db, options: ChatChannelServiceOptions) { ) { const initial = await endpointRecord(endpointId); if (!initial) throw notFound("Chat endpoint not found"); - if (updates.length === 0) return listResources(endpointId); + if (updates.length === 0 && initial.endpoint.provider !== "github") return listResources(endpointId); if (initial.endpoint.provider === "imessage-photon" && initial.endpoint.botExternalId?.startsWith("photon-project:") && updates.some((entry) => entry.enabled)) throw unprocessable("Photon shared channels support direct messages only; groups cannot be enabled"); await withCredentialMutationLease( @@ -28045,7 +28045,7 @@ export function chatChannelService(db: Db, options: ChatChannelServiceOptions) { ); 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) + if (changes.length > 0 || (endpoint.provider === "github" && !endpoint.setup.github?.repositorySelectionSaved)) await logActivity( tx as unknown as Db, { diff --git a/server/src/services/chat-github-registration.ts b/server/src/services/chat-github-registration.ts index e11a1fd09ae..fd402b18fec 100644 --- a/server/src/services/chat-github-registration.ts +++ b/server/src/services/chat-github-registration.ts @@ -1,7 +1,8 @@ import { createHash, randomBytes } from "node:crypto"; -import { and, eq, gt } from "drizzle-orm"; +import { and, eq, gt, sql } from "drizzle-orm"; import { chatEndpoints, + chatEndpointResources, chatGitHubRegistrations, companies, companyMemberships, @@ -111,11 +112,25 @@ export function githubChatRegistrationService( const state = randomBytes(32).toString("base64url"); const expiresAt = new Date(Date.now() + 30 * 60_000); await db.transaction(async (tx) => { - await tx - .select({ id: chatEndpoints.id }) + const [lockedEndpoint] = await tx + .select() .from(chatEndpoints) .where(eq(chatEndpoints.id, endpointId)) .for("update"); + if (!lockedEndpoint || lockedEndpoint.status === "archived") + throw notFound("GitHub bot not found"); + if (lockedEndpoint.botExternalId) + throw conflict("This bot already has a GitHub App. Reconnect its existing credentials."); + const [existingResource] = await tx + .select({ id: chatEndpointResources.id }) + .from(chatEndpointResources) + .where(eq(chatEndpointResources.endpointId, endpointId)) + .limit(1); + if (!lockedEndpoint.setup.github?.repositorySelectionSaved && !existingResource) + await tx.update(chatEndpoints).set({ + setup: sql`jsonb_set(${chatEndpoints.setup}, '{github}', coalesce(${chatEndpoints.setup}->'github', '{}'::jsonb) || '{"initialRepositoryImportPending":true}'::jsonb)`, + updatedAt: new Date(), + }).where(eq(chatEndpoints.id, endpointId)); await tx .update(chatGitHubRegistrations) .set({ status: "failed", consumedAt: new Date() }) diff --git a/tests/e2e/chat-adapters-ui-providers.spec.ts b/tests/e2e/chat-adapters-ui-providers.spec.ts index 3b5c158b424..09c852b5f36 100644 --- a/tests/e2e/chat-adapters-ui-providers.spec.ts +++ b/tests/e2e/chat-adapters-ui-providers.spec.ts @@ -1,5 +1,5 @@ import { readFile } from "node:fs/promises"; -import { expect, test, type Page } from "@playwright/test"; +import { expect, test, type Page, type TestInfo } from "@playwright/test"; import { expectMinimumProviderSetup, @@ -18,12 +18,13 @@ import { GITHUB_PRIVATE_KEY_PASTE_FIXTURE, } from "./chat-adapters-ui.shared"; -async function exerciseGitHubReviewSetup(page: Page, mock: ChatMock, seed: Seed, provider: ProviderCase) { +async function exerciseGitHubReviewSetup(page: Page, mock: ChatMock, seed: Seed, provider: ProviderCase, testInfo: TestInfo) { await expect(page.getByRole("heading", { name: "Choose agent", exact: true })).toBeVisible(); 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 testInfo.attach("Choose agent — inline low-trust warning", { body: await page.screenshot(), contentType: "image/png" }); 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(); @@ -33,6 +34,7 @@ async function exerciseGitHubReviewSetup(page: Page, mock: ChatMock, seed: Seed, 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 testInfo.attach("Connect GitHub — organization", { body: await page.screenshot(), contentType: "image/png" }); 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"); @@ -204,7 +206,7 @@ test.describe.serial("native chat adapter UI", () => { for (const provider of PROVIDERS) { test(`${provider.name}: catalog, setup, and connection management tabs`, async ({ page, - }) => { + }, testInfo) => { const mock = await installChatControlPlaneMock(page, provider, seed, { enableChatConnectors: true, }); @@ -268,7 +270,7 @@ test.describe.serial("native chat adapter UI", () => { } if (provider.provider === "github") { - await exerciseGitHubReviewSetup(page, mock, seed, provider); + await exerciseGitHubReviewSetup(page, mock, seed, provider, testInfo); return; }