diff --git a/doc/connections/GITHUB-REVIEW-BOT.md b/doc/connections/GITHUB-REVIEW-BOT.md index 2d9dc55197b..f63732c56a4 100644 --- a/doc/connections/GITHUB-REVIEW-BOT.md +++ b/doc/connections/GITHUB-REVIEW-BOT.md @@ -40,6 +40,13 @@ Installation may return before GitHub's separately delivered signed webhook ping. Paperclip keeps the same draft and credentials, shows a waiting state, and continues automatically after authenticating the ping. Refreshing or resuming that draft must not create another App or bypass signature verification. +While this Cloud setup screen is waiting, its authenticated advance request also +drains the existing sealed event queue. It can bypass an older idle-connection +backoff, at most once every five seconds. Setup and background requests share +one in-flight lease. The normal stack webhook handler still authenticates the +original GitHub bytes before setup can continue. This adds no background timer; +closing the screen stops these readiness requests. Existing background recovery +and its sleep restrictions remain unchanged. New connections default to authorized mentions, advisory reviews, linked-member access, guests off, and enabled bot GitHub tools. Existing instructions, explicit diff --git a/maps/feature-map/connection-setup.md b/maps/feature-map/connection-setup.md index 22306048e8a..d27fc36c657 100644 --- a/maps/feature-map/connection-setup.md +++ b/maps/feature-map/connection-setup.md @@ -205,6 +205,15 @@ configuration changes, saved retry deadlines, rollback, and parking pending repair while an endpoint or connection is inactive. The provider responses are fixtures. Settled maintenance leaves no discovery timer or database scans. +GitHub Cloud setup also covers an older empty-queue backoff, concurrent setup and +background leases, and retry after a Cloud error in the +[event poller regression](../../server/src/services/github-connection-events.test.ts). +Channel integration checks that active setup drains its signed ping through the +normal webhook handler; a forged ping still cannot verify the endpoint. These +fixtures do not establish live Cloud delivery. For live proof, start a disposable +App after an idle personal GitHub connection, complete installation, and confirm +that the same draft advances after authenticating the queued ping. + Manual: complete setup for one named provider, reload endpoint settings, link the intended sender, and send a disposable message. Verify one task and a useful reply in the originating conversation. Inspect an unlinked sender's denial and diff --git a/server/src/__tests__/chat-channels.integration.test.ts b/server/src/__tests__/chat-channels.integration.test.ts index 4fd186e6271..beb2c0b436a 100644 --- a/server/src/__tests__/chat-channels.integration.test.ts +++ b/server/src/__tests__/chat-channels.integration.test.ts @@ -24,7 +24,7 @@ import { toolActionRequests, toolInvocations, toolPolicies } from "@paperclipai/ 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 { dispatchGitHubBotCloudEvent } from "../services/chat-github-cloud-ingress.js"; +import { dispatchGitHubBotCloudEvent, registerGitHubBotCloudSetupPoller } from "../services/chat-github-cloud-ingress.js"; import { githubChatManagementService } from "../services/chat-github-management.js"; import { githubChatReviewService } from "../services/chat-github-reviews.js"; import { resolveGitHubReviewInstructions } from "../services/chat-channels/github/review-instructions.js"; @@ -5135,13 +5135,22 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { (await f.service.get(f.endpoint.id)).setup.webhookVerifiedAt, ).toBeNull(); expect(f.wakeup).not.toHaveBeenCalled(); - event.payload.githubApp.headers["x-hub-signature-256"] = - `sha256=${createHmac("sha256", f.webhookSecret).update(body).digest("hex")}`; - expect(await dispatchGitHubBotCloudEvent(db, event)).toBe(true); - expect( - (await f.service.get(f.endpoint.id)).setup.webhookVerifiedAt, - ).toBeTruthy(); - expect(f.wakeup).not.toHaveBeenCalled(); + const poll = vi.fn(async () => { await dispatchGitHubBotCloudEvent(db, event); }); + const unregister = registerGitHubBotCloudSetupPoller(db, poll); + try { + const waiting = await f.service.githubWizard.advance(f.endpoint.id, "owner-user"); + expect(waiting.state).toBe("verify"); + expect(poll).toHaveBeenCalledOnce(); + expect((await f.service.get(f.endpoint.id)).setup.webhookVerifiedAt).toBeNull(); + event.payload.githubApp.headers["x-hub-signature-256"] = + `sha256=${createHmac("sha256", f.webhookSecret).update(body).digest("hex")}`; + const ready = await f.service.githubWizard.advance(f.endpoint.id, "owner-user"); + expect(ready.state).not.toBe("verify"); + expect((await f.service.get(f.endpoint.id)).setup.webhookVerifiedAt).toBeTruthy(); + expect(f.wakeup).not.toHaveBeenCalled(); + await f.service.githubWizard.advance(f.endpoint.id, "owner-user"); + expect(poll).toHaveBeenCalledTimes(2); + } finally { unregister(); } }); it("legacy name-only registration preserves an explicit empty repository selection", async () => { const fixture = await seedCompany(); diff --git a/server/src/__tests__/github-connection-events.test.ts b/server/src/__tests__/github-connection-events.test.ts index 77d34f15e6a..6bcd2913396 100644 --- a/server/src/__tests__/github-connection-events.test.ts +++ b/server/src/__tests__/github-connection-events.test.ts @@ -7,6 +7,7 @@ import { connectionGrants, createDb, externalObjects, + EMBEDDED_POSTGRES_TEST_TIMEOUT_MS, toolApplications, toolConnections, } from "@paperclipai/db"; @@ -30,7 +31,7 @@ describeEmbeddedPostgres("GitHub connection event delivery", () => { beforeAll(async () => { tempDb = await startEmbeddedPostgresTestDatabase("paperclip-github-events-"); db = createDb(tempDb.connectionString); - }, 20_000); + }, EMBEDDED_POSTGRES_TEST_TIMEOUT_MS); afterEach(async () => { await db.delete(activityLog); @@ -46,7 +47,7 @@ describeEmbeddedPostgres("GitHub connection event delivery", () => { await tempDb?.cleanup(); }); - it("applies a normalized merged PR once while retaining undecryptable siblings for retry", async () => { + it.each([false, true])("applies a normalized merged PR once while retaining undecryptable siblings for retry (active setup=%s)", async activeSetup => { const companyId = randomUUID(); const userId = `github-owner-${randomUUID()}`; const applicationId = randomUUID(); @@ -151,8 +152,21 @@ describeEmbeddedPostgres("GitHub connection event delivery", () => { let currentTime = new Date("2026-09-04T12:00:05.000Z"); const service = githubConnectionEventService(db, { connector, now: () => currentTime }); + if (activeSetup) { + // An existing personal connection can leave a long empty-queue backoff + // before a new dedicated App starts its signed-ping verification. + for (const delay of [0, 10_000, 20_000, 40_000, 80_000]) { + currentTime = new Date(currentTime.getTime() + delay); + vi.mocked(connector.leaseEvents).mockResolvedValueOnce(null); + await service.pollOnce(); + } + currentTime = new Date(currentTime.getTime() + 5_000); + await expect(service.pollOnce()).resolves.toMatchObject({ leased: 0 }); + expect(connector.leaseEvents).toHaveBeenCalledTimes(5); + } + const warning = vi.spyOn(logger, "warn").mockImplementation(() => undefined); - await expect(service.pollOnce()).resolves.toMatchObject({ leased: 2, processed: 1, duplicate: 0, failed: 1 }); + await expect(service.pollOnce({ forSetup: activeSetup })).resolves.toMatchObject({ leased: 2, processed: 1, duplicate: 0, failed: 1 }); expect(warning.mock.calls).toEqual([[{ deliveryId: "undecryptable-delivery", reason: "inner_event_authentication_failed", }, "GitHub connector event retained for retry"]]); diff --git a/server/src/index.ts b/server/src/index.ts index e83d603e2aa..207f342a2ae 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -136,6 +136,7 @@ import { } from "./services/device-login-reaper.js"; import { createProductionSetupTokenReaper } from "./services/setup-token-reaper.js"; import { localAiLoginService } from "./services/local-ai-login.js"; +import { registerGitHubBotCloudSetupPoller } from "./services/chat-github-cloud-ingress.js"; import { resolveWorktreeRunExecutionActivationState } from "./services/instance-settings.js"; import { parseAdapterRegistryEnv, @@ -1419,9 +1420,9 @@ async function startServerWithDatabaseTeardown( ?? process.env.PAPERCLIP_TOOL_RUNTIME_TRUSTED_HOST ?? null, }); - const scheduleGitHubConnectionEventPoll = () => { + const scheduleGitHubConnectionEventPoll = (forSetup = false) => { if (isIdleTaskDrainActive() || heartbeatSchedulerStopped) return; - trackHeartbeatSchedulerWork(githubConnectionEvents.pollOnce() + const work = githubConnectionEvents.pollOnce({ forSetup }) .then((result) => { if (result.leased > 0 || result.failed > 0) { logger.info(result, "GitHub connection event poll completed"); @@ -1429,8 +1430,13 @@ async function startServerWithDatabaseTeardown( }) .catch((err) => { logger.error({ err }, "GitHub connection event poll failed"); - })); + }); + trackHeartbeatSchedulerWork(work); + return work; }; + server.on("close", registerGitHubBotCloudSetupPoller(db, async () => { + await scheduleGitHubConnectionEventPoll(true); + })); const scheduleGitHubConnectionContinuitySweep = () => { if (isIdleTaskDrainActive() || heartbeatSchedulerStopped) return; trackHeartbeatSchedulerWork(tools.sweepGitHubConnectionContinuity() diff --git a/server/src/services/chat-github-cloud-ingress.ts b/server/src/services/chat-github-cloud-ingress.ts index b1493d64779..2f8e75eb8f9 100644 --- a/server/src/services/chat-github-cloud-ingress.ts +++ b/server/src/services/chat-github-cloud-ingress.ts @@ -7,6 +7,18 @@ type Event = SealedConnectorEvents["events"][number]; type Endpoint = typeof chatEndpoints.$inferSelect; type Handler = (endpoint: Endpoint, request: Request) => Promise; const handlers = new WeakMap(); +const setupPollers = new WeakMap Promise>(); + +/** The active wizard reuses the server-owned, serialized event drain. */ +export function registerGitHubBotCloudSetupPoller(db: Db, poll: () => Promise) { + setupPollers.set(db, poll); + return () => { if (setupPollers.get(db) === poll) setupPollers.delete(db); }; +} + +export async function pollGitHubBotCloudSetupEvents(db: Db): Promise { + await setupPollers.get(db)?.(); +} + export function registerGitHubBotCloudIngress(db: Db, handler: Handler) { handlers.set(db, handler); return () => { diff --git a/server/src/services/chat-github-wizard.ts b/server/src/services/chat-github-wizard.ts index 5f63f68db8c..701d77cba51 100644 --- a/server/src/services/chat-github-wizard.ts +++ b/server/src/services/chat-github-wizard.ts @@ -33,6 +33,7 @@ import { type PaperclipCloudConnector, } from "./paperclip-cloud-connector.js"; import { paperclipCloudConnectorEnrollmentStatus } from "./paperclip-cloud-connector-enrollment.js"; +import { pollGitHubBotCloudSetupEvents } from "./chat-github-cloud-ingress.js"; /** Loopback aliases are equivalent locally, but Cloud requires the approved exact origin. */ export function githubWizardBrowserOrigin( @@ -920,6 +921,10 @@ export function githubChatWizardService( // Keep the same vaulted App in the wizard instead of throwing JSON from // the browser callback or relaxing the configure action's signature gate. bot = await endpoint(id, userId); + if (!bot.setup.webhookVerifiedAt && bot.setup.github?.cloudRegistrationId) { + await pollGitHubBotCloudSetupEvents(db); + bot = await endpoint(id, userId); + } if (!bot.setup.webhookVerifiedAt) return { endpointId: id, diff --git a/server/src/services/github-connection-events.test.ts b/server/src/services/github-connection-events.test.ts new file mode 100644 index 00000000000..93031ecae62 --- /dev/null +++ b/server/src/services/github-connection-events.test.ts @@ -0,0 +1,74 @@ +import { describe, expect, it, vi } from "vitest"; +import { chatEndpoints, type Db } from "@paperclipai/db"; +import { githubConnectionEventService } from "./github-connection-events.js"; +import type { PaperclipCloudConnector } from "./paperclip-cloud-connector.js"; + +function fixture() { + let time = Date.parse("2026-09-04T12:00:00Z"); + const bindings = [{ + grant: { id: "grant", companyId: "company", kind: "user", subjectUserId: "owner", status: "active", + providerTenant: { github: { installationIds: ["101"] } } }, + connection: { id: "connection", config: { sourceTemplateKey: "github", oauth: { connectorProfile: "github.code" } } }, + }]; + const bots: Array<{ companyId: string }> = []; + // Only SQL is stubbed. Backoff, lease admission and concurrent execution use + // the real service; relational persistence has its own Postgres suite. + const select = vi.fn(() => ({ from: (table: unknown) => table === chatEndpoints + ? { where: async () => bots } + : { innerJoin: () => ({ where: async () => bindings }) } })); + const leaseEvents = vi.fn().mockResolvedValue(null); + const connector = { leaseEvents } as unknown as PaperclipCloudConnector; + const service = githubConnectionEventService({ select } as unknown as Db, { connector, now: () => new Date(time) }); + return { service, leaseEvents, select, bots, advance: (ms: number) => { time += ms; } }; +} + +describe("GitHub event polling during active App setup", () => { + it("reaches Cloud during an old empty-queue backoff without expanding idle polling", async () => { + const f = fixture(); + for (const delay of [0, 10_000, 20_000, 40_000, 80_000]) { + f.advance(delay); + await f.service.pollOnce(); + } + expect(f.leaseEvents).toHaveBeenCalledTimes(5); + const queries = f.select.mock.calls.length; + f.bots.push({ companyId: "company" }); + f.advance(5_000); + for (let index = 0; index < 3; index++) await f.service.pollOnce(); + expect(f.select).toHaveBeenCalledTimes(queries); + expect(f.leaseEvents).toHaveBeenCalledTimes(5); + await f.service.pollOnce({ forSetup: true }); + expect(f.leaseEvents).toHaveBeenCalledTimes(6); + const setupQueries = f.select.mock.calls.length; + await f.service.pollOnce({ forSetup: true }); + expect(f.leaseEvents).toHaveBeenCalledTimes(6); + expect(f.select).toHaveBeenCalledTimes(setupQueries); + f.advance(5_000); + await f.service.pollOnce({ forSetup: true }); + expect(f.leaseEvents).toHaveBeenCalledTimes(7); + }); + + it("shares one lease between setup and scheduled callers while Cloud is slow", async () => { + const f = fixture(); + let complete!: (value: null) => void; + f.leaseEvents.mockImplementationOnce(() => new Promise(resolve => { complete = resolve; })); + const scheduled = f.service.pollOnce(); + const setup = f.service.pollOnce({ forSetup: true }); + await vi.waitFor(() => expect(f.leaseEvents).toHaveBeenCalledTimes(1)); + expect(complete).toBeTypeOf("function"); + complete(null); + await Promise.all([scheduled, setup]); + expect(f.leaseEvents).toHaveBeenCalledTimes(1); + }); + + it("releases the shared attempt after an error and retries only after the setup throttle", async () => { + const f = fixture(); + f.leaseEvents.mockRejectedValueOnce(new Error("Cloud unavailable")); + await expect(f.service.pollOnce({ forSetup: true })).rejects.toThrow("Cloud unavailable"); + await f.service.pollOnce({ forSetup: true }); + await f.service.pollOnce(); + expect(f.leaseEvents).toHaveBeenCalledTimes(1); + f.advance(5_000); + await f.service.pollOnce({ forSetup: true }); + expect(f.leaseEvents).toHaveBeenCalledTimes(2); + }); +}); diff --git a/server/src/services/github-connection-events.ts b/server/src/services/github-connection-events.ts index 04022c647a1..6848875815f 100644 --- a/server/src/services/github-connection-events.ts +++ b/server/src/services/github-connection-events.ts @@ -221,6 +221,8 @@ export function githubConnectionEventService( const now = options.now ?? (() => new Date()); let nextPollAt = 0; let emptyPolls = 0; + let nextSetupPollAt = 0; + let inFlight: Promise | null = null; async function activeBindings() { const rows = await db.select({ grant: connectionGrants, connection: toolConnections }) @@ -458,88 +460,101 @@ export function githubConnectionEventService( return "processed" as const; } - return { - async pollOnce(): Promise { - if (now().getTime() < nextPollAt) { - return { leased: 0, processed: 0, duplicate: 0, ignored: 0, failed: 0 }; - } - const bindings = await activeBindings(); - const bots = await db.select().from(chatEndpoints).where(and(eq(chatEndpoints.provider, "github"), sql`${chatEndpoints.setup}->'github'->>'cloudRegistrationId' is not null`, sql`${chatEndpoints.status} <> 'archived'`)); - if (bindings.length === 0 && bots.length === 0) { - nextPollAt = now().getTime() + 5_000; - return { leased: 0, processed: 0, duplicate: 0, ignored: 0, failed: 0 }; - } - const config = options.connector ? null : paperclipCloudConnectorConfigFromEnv(options.env); - const connector = options.connector ?? (config ? createPaperclipCloudConnector({ config }) : null); - if (!connector) { - nextPollAt = now().getTime() + 5 * 60_000; - return { leased: 0, processed: 0, duplicate: 0, ignored: 0, failed: 0 }; - } - const first = bindings[0] ?? { subject: "github-bot-events", companyId: bots[0]!.companyId }; - const lease = await connector.leaseEvents({ subject: first.subject, companyId: first.companyId }); - if (!lease) { - emptyPolls += 1; - nextPollAt = now().getTime() + Math.min(bots.length ? 15_000 : 5 * 60_000, 5_000 * (2 ** Math.min(emptyPolls, 6))); - return { leased: 0, processed: 0, duplicate: 0, ignored: 0, failed: 0 }; - } - emptyPolls = 0; + async function runPoll(): Promise { + const bindings = await activeBindings(); + const bots = await db.select().from(chatEndpoints).where(and(eq(chatEndpoints.provider, "github"), sql`${chatEndpoints.setup}->'github'->>'cloudRegistrationId' is not null`, sql`${chatEndpoints.status} <> 'archived'`)); + if (bindings.length === 0 && bots.length === 0) { nextPollAt = now().getTime() + 5_000; - const result: GitHubConnectionEventPollResult = { - leased: lease.events.length + (lease.failedEventIds?.length ?? 0), - processed: 0, - duplicate: 0, - ignored: 0, - failed: lease.failedEventIds?.length ?? 0, - }; - const acknowledge: string[] = []; - for (const event of lease.events) { - if (event.payload.githubApp !== undefined) { - try { await dispatchGitHubBotCloudEvent(db, event); acknowledge.push(event.id); result.processed += 1; } - catch { result.failed += 1; } - continue; - } - const matched = bindings.filter((binding) => event.bindingIds.includes(binding.id)); - if (matched.length === 0) { - result.ignored += 1; - acknowledge.push(event.id); - continue; - } - try { - const companies = new Map(); - for (const binding of matched) companies.set(binding.companyId, [...(companies.get(binding.companyId) ?? []), binding]); - for (const [companyId, companyBindings] of companies) { - const status = await processForCompany(companyId, companyBindings, event); - result[status] += 1; - } - acknowledge.push(event.id); - if (event.event === "installation" && (event.action === "deleted" || event.action === "suspend")) { - await Promise.all(matched.map((binding) => connector.setWebhookBinding({ - subject: binding.subject, - companyId: binding.companyId, - id: binding.id, - installationId: binding.installationId, - connectionId: binding.connectionId, - grantId: binding.grantId, - active: false, - }))); - } - } catch { - result.failed += 1; - } + return { leased: 0, processed: 0, duplicate: 0, ignored: 0, failed: 0 }; + } + const config = options.connector ? null : paperclipCloudConnectorConfigFromEnv(options.env); + const connector = options.connector ?? (config ? createPaperclipCloudConnector({ config }) : null); + if (!connector) { + nextPollAt = now().getTime() + 5 * 60_000; + return { leased: 0, processed: 0, duplicate: 0, ignored: 0, failed: 0 }; + } + const first = bindings[0] ?? { subject: "github-bot-events", companyId: bots[0]!.companyId }; + const lease = await connector.leaseEvents({ subject: first.subject, companyId: first.companyId }); + if (!lease) { + emptyPolls += 1; + nextPollAt = now().getTime() + Math.min(bots.length ? 15_000 : 5 * 60_000, 5_000 * (2 ** Math.min(emptyPolls, 6))); + return { leased: 0, processed: 0, duplicate: 0, ignored: 0, failed: 0 }; + } + emptyPolls = 0; + nextPollAt = now().getTime() + 5_000; + const result: GitHubConnectionEventPollResult = { + leased: lease.events.length + (lease.failedEventIds?.length ?? 0), + processed: 0, + duplicate: 0, + ignored: 0, + failed: lease.failedEventIds?.length ?? 0, + }; + const acknowledge: string[] = []; + for (const event of lease.events) { + if (event.payload.githubApp !== undefined) { + try { await dispatchGitHubBotCloudEvent(db, event); acknowledge.push(event.id); result.processed += 1; } + catch { result.failed += 1; } + continue; } - if (acknowledge.length > 0) { - await connector.acknowledgeEvents({ - subject: first.subject, - companyId: first.companyId, - leaseId: lease.leaseId, - deliveryIds: acknowledge, - }); + const matched = bindings.filter((binding) => event.bindingIds.includes(binding.id)); + if (matched.length === 0) { + result.ignored += 1; + acknowledge.push(event.id); + continue; } - for (const deliveryId of lease.failedEventIds ?? []) { - logger.warn({ deliveryId, reason: "inner_event_authentication_failed" }, - "GitHub connector event retained for retry"); + try { + const companies = new Map(); + for (const binding of matched) companies.set(binding.companyId, [...(companies.get(binding.companyId) ?? []), binding]); + for (const [companyId, companyBindings] of companies) { + const status = await processForCompany(companyId, companyBindings, event); + result[status] += 1; + } + acknowledge.push(event.id); + if (event.event === "installation" && (event.action === "deleted" || event.action === "suspend")) { + await Promise.all(matched.map((binding) => connector.setWebhookBinding({ + subject: binding.subject, + companyId: binding.companyId, + id: binding.id, + installationId: binding.installationId, + connectionId: binding.connectionId, + grantId: binding.grantId, + active: false, + }))); + } + } catch { + result.failed += 1; } - return result; + } + if (acknowledge.length > 0) { + await connector.acknowledgeEvents({ + subject: first.subject, + companyId: first.companyId, + leaseId: lease.leaseId, + deliveryIds: acknowledge, + }); + } + for (const deliveryId of lease.failedEventIds ?? []) { + logger.warn({ deliveryId, reason: "inner_event_authentication_failed" }, + "GitHub connector event retained for retry"); + } + return result; + } + + return { + pollOnce(input: { forSetup?: boolean } = {}): Promise { + // A manager actively awaiting a dedicated App's signed ping must not + // inherit a personal connection's old empty-queue backoff. This is a + // bounded readiness probe, not an additional background polling loop. + if (inFlight) return inFlight; + const time = now().getTime(); + if (time < (input.forSetup ? nextSetupPollAt : nextPollAt)) { + return Promise.resolve({ leased: 0, processed: 0, duplicate: 0, ignored: 0, failed: 0 }); + } + nextSetupPollAt = time + 5_000; + nextPollAt = Math.max(nextPollAt, nextSetupPollAt); + const work = runPoll().finally(() => { if (inFlight === work) inFlight = null; }); + inFlight = work; + return work; }, }; }