From 7703220d383354fa5d44db9bea2011b04b12e999 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Sun, 25 Oct 2026 20:52:09 -0700 Subject: [PATCH] perf(github): narrow response authority reads (#17686) Read current GitHub response identity and policy fields without transferring saved message bodies, instructions, or automatic policy wrappers. Preserve company predicates, shared locks, publication leases, malformed input handling, and stored content. Verify workspace typecheck, server build, 414 selected GitHub tests, three focused browser cases, all 55 checks, and Apex 5/5. Preserve the original browser cleanup deadlock failure; always restore experiment settings while reporting cancellation errors. Co-Authored-By: Paperclip --- .../chat-channels.integration.test.ts | 77 +++++++++++++++++++ .../github/response-authority-read.ts | 50 ++++++++++++ .../chat-channels/github/response-target.ts | 8 +- server/src/services/chat-github-access.ts | 15 ++-- server/src/services/chat-github-events.ts | 5 +- .../services/chat-github-response-comments.ts | 10 +-- .../src/services/chat-github-review-policy.ts | 7 +- tests/e2e/agent-chat-projects.spec.ts | 40 +++++++++- tests/e2e/agent-chat.shared.ts | 31 ++++---- 9 files changed, 211 insertions(+), 32 deletions(-) create mode 100644 server/src/services/chat-channels/github/response-authority-read.ts diff --git a/server/src/__tests__/chat-channels.integration.test.ts b/server/src/__tests__/chat-channels.integration.test.ts index beb2c0b436a..f844a18bce3 100644 --- a/server/src/__tests__/chat-channels.integration.test.ts +++ b/server/src/__tests__/chat-channels.integration.test.ts @@ -5,6 +5,8 @@ import { createSystemRecovery } from "../services/system-recovery-state.js"; import { remoteTerminationReceipt } from "../services/remote-execution-termination.js"; import { stageReceiptReactionRemovals } from "../services/chat-receipt-reactions.js"; import type { Db } from "@paperclipai/db"; +import { createDatabaseResponseCapture, installDatabaseResponseCapture } from "@paperclipai/db/response-capture"; +import { githubResponseAuthorityFields } from "../services/chat-channels/github/response-authority-read.js"; import * as activityLogModule from "../services/activity-log.js"; import * as connectionCredentialBindings from "../services/connection-credential-bindings.js"; import { chatSlackManagerGrants, chatSlackRegistrations, toolOauthStates } from "@paperclipai/db"; @@ -2585,6 +2587,81 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { .rejects.toMatchObject({ status: 403 }); }); + it("bounds GitHub response authorization reads with large saved chat content", async () => { + const f = await toolReplyFixture(); + const [delivery] = await db.select().from(chatDeliveries).where(eq(chatDeliveries.endpointId, f.endpoint.id)); + const [resource] = await db.select().from(chatEndpointResources).where(eq(chatEndpointResources.id, f.conversation.resourceId!)); + const [configuration] = await db.select().from(chatGitHubConfigurations).where(eq(chatGitHubConfigurations.endpointId, f.endpoint.id)); + const bulk = "unused-content-for-response-authorization".repeat(8_192); + const setup = { ...f.endpoint.setup, unusedContent: bulk }; + await db.update(chatEndpoints).set({ communicationInstructions: bulk, setup }).where(eq(chatEndpoints.id, f.endpoint.id)); + await db.update(chatConversations).set({ communicationGuidance: bulk }).where(eq(chatConversations.id, f.conversation.id)); + await db.update(chatEndpointResources).set({ metadata: { ...resource.metadata, unusedContent: bulk } }) + .where(eq(chatEndpointResources.id, resource.id)); + await db.update(chatGitHubConfigurations).set({ configuration: { ...configuration.configuration, + defaults: { ...configuration.configuration.defaults, instructions: bulk } } }) + .where(eq(chatGitHubConfigurations.endpointId, f.endpoint.id)); + const normalizedEvent = { ...delivery.normalizedEvent, unusedContent: bulk }; + await db.update(chatDeliveries).set({ normalizedEvent }).where(eq(chatDeliveries.id, delivery.id)); + const events: Array> = [], now = Date.now(); + const capture = createDatabaseResponseCapture({ start: now - 100, end: now + 59_000 }, event => events.push(event)); + installDatabaseResponseCapture(db, { chatMessageLinks, chatDeliveries, chatEndpoints, chatConversations, + chatEndpointResources, chatGitHubConfigurations, chatExternalPrincipals, chatIdentityLinks, + companyMemberships, chatActions, heartbeatRuns }, capture); + try { + const receipt = await githubResponseCommentService(db, f.providerFetch).tryAcknowledge(delivery.id); + expect(receipt?.id).toBeTruthy(); + } finally { capture.flush(); } + expect(Number(events.at(-1)?.responseValueBytes)).toBeLessThan(24_000); + expect(events.filter(event => event.table === "chat_message_links").length).toBeGreaterThan(0); + expect((await db.select({ normalizedEvent: chatDeliveries.normalizedEvent }).from(chatDeliveries) + .where(eq(chatDeliveries.id, delivery.id)))[0].normalizedEvent).toEqual(normalizedEvent); + expect((await db.select({ setup: chatEndpoints.setup }).from(chatEndpoints) + .where(eq(chatEndpoints.id, f.endpoint.id)))[0].setup).toEqual(setup); + }); + + it.each(["githubAutomatic", "githubIssue"])("preserves %s policy context while omitting body content", async key => { + const f = await toolReplyFixture(); + const [delivery] = await db.select().from(chatDeliveries).where(eq(chatDeliveries.endpointId, f.endpoint.id)); + const context = { event: key === "githubIssue" ? "issue_opened" : "opened", repositoryId: "418", repository: "acme/repo", + author: { id: "42", login: "octocat", isBot: false }, sender: { id: "42", login: "octocat" }, + labels: ["review"], draft: false, baseBranch: "master", headSha: "a".repeat(40), + body: "unused-body".repeat(32_768), title: "unused-title".repeat(8_192) }; + const normalizedEvent = { text: "unrelated message text", [key]: { context, revision: 42, + policy: { instructions: "unused-policy-instructions".repeat(32_768), + prompts: { opened: "unused-policy-prompt".repeat(32_768) } }, + } }; + await db.update(chatDeliveries).set({ normalizedEvent }).where(eq(chatDeliveries.id, delivery.id)); + const events: Array> = [], now = Date.now(); + const capture = createDatabaseResponseCapture({ start: now - 100, end: now + 59_000 }, event => events.push(event)); + installDatabaseResponseCapture(db, { chatDeliveries }, capture); + let projected: { event: Record }; + try { + [projected] = await db.select({ event: githubResponseAuthorityFields.delivery.normalizedEvent }) + .from(chatDeliveries).where(eq(chatDeliveries.id, delivery.id)); + } finally { capture.flush(); } + const { body: _body, title: _title, ...policy } = context; + expect(Number(events.at(-1)?.responseValueBytes)).toBeLessThan(2_048); + expect(projected.event).toEqual({ [key]: { context: policy } }); + expect((await db.select({ event: chatDeliveries.normalizedEvent }).from(chatDeliveries) + .where(eq(chatDeliveries.id, delivery.id)))[0].event).toEqual(normalizedEvent); + }); + + it.each([null, false, 7, "invalid-event", []])("preserves malformed GitHub authority input %j", async value => { + const f = await toolReplyFixture(); + const [delivery] = await db.select().from(chatDeliveries).where(eq(chatDeliveries.endpointId, f.endpoint.id)); + await db.update(chatDeliveries).set({ normalizedEvent: sql`${JSON.stringify(value)}::jsonb` }) + .where(eq(chatDeliveries.id, delivery.id)); + await db.update(chatEndpoints).set({ setup: sql`${JSON.stringify(value)}::jsonb` }) + .where(eq(chatEndpoints.id, f.endpoint.id)); + const [projected] = await db.select({ event: githubResponseAuthorityFields.delivery.normalizedEvent }) + .from(chatDeliveries).where(eq(chatDeliveries.id, delivery.id)); + const [endpoint] = await db.select({ setup: githubResponseAuthorityFields.endpoint.setup }) + .from(chatEndpoints).where(eq(chatEndpoints.id, f.endpoint.id)); + expect(projected.event).toEqual(value); + expect(endpoint.setup).toEqual(value); + }); + async function publicationRetryFixture() { const f = await toolReplyFixture("github:paperclipai/paperclip:418", true); const head = "b".repeat(40); diff --git a/server/src/services/chat-channels/github/response-authority-read.ts b/server/src/services/chat-channels/github/response-authority-read.ts new file mode 100644 index 00000000000..e5637bfbba5 --- /dev/null +++ b/server/src/services/chat-channels/github/response-authority-read.ts @@ -0,0 +1,50 @@ +import { chatConversations, chatDeliveries, chatEndpointResources, chatEndpoints, chatGitHubConfigurations } from "@paperclipai/db"; +import { sql } from "drizzle-orm"; + +const authorizationEvent = sql>`case + when jsonb_typeof(${chatDeliveries.normalizedEvent}) = 'object' then ( + select coalesce(jsonb_object_agg(field.key, case + when jsonb_typeof(field.value->'context') = 'object' then + jsonb_build_object('context', (field.value->'context') - 'body' - 'title') + else field.value end), '{}'::jsonb) + from jsonb_each(${chatDeliveries.normalizedEvent}) as field(key, value) + where field.key in ('githubAutomatic', 'githubIssue') + ) else ${chatDeliveries.normalizedEvent} end`; + +export const githubResponseAuthorityFields = { + endpoint: { + id: chatEndpoints.id, + companyId: chatEndpoints.companyId, + assignedAgentId: chatEndpoints.assignedAgentId, + botExternalId: chatEndpoints.botExternalId, + botUsername: chatEndpoints.botUsername, + provider: chatEndpoints.provider, + status: chatEndpoints.status, + allowUnlinkedPeople: chatEndpoints.allowUnlinkedPeople, + setup: sql`case when jsonb_typeof(${chatEndpoints.setup}) = 'object' + then jsonb_build_object('runtimeGeneration', ${chatEndpoints.setup}->'runtimeGeneration') + else ${chatEndpoints.setup} end`, + }, + conversation: { + id: chatConversations.id, + issueId: chatConversations.issueId, + externalThreadId: chatConversations.externalThreadId, + }, + delivery: { + id: chatDeliveries.id, + principalId: chatDeliveries.principalId, + state: chatDeliveries.state, + createdAt: chatDeliveries.createdAt, + normalizedEvent: authorizationEvent, + }, + resource: { + enabled: chatEndpointResources.enabled, + availability: chatEndpointResources.availability, + providerResourceId: chatEndpointResources.providerResourceId, + metadata: sql`case + when jsonb_typeof(${chatEndpointResources.metadata}) = 'object' + then jsonb_build_object('providerRepositoryId', ${chatEndpointResources.metadata}->'providerRepositoryId') + else ${chatEndpointResources.metadata} end`, + }, + toolsEnabled: sql`${chatGitHubConfigurations.configuration}->'toolsEnabled'`, +}; diff --git a/server/src/services/chat-channels/github/response-target.ts b/server/src/services/chat-channels/github/response-target.ts index 1e955b14482..b45ef3c8d81 100644 --- a/server/src/services/chat-channels/github/response-target.ts +++ b/server/src/services/chat-channels/github/response-target.ts @@ -4,9 +4,11 @@ import { chatActions, chatConversations, chatDeliveries, chatEndpoints, type Db import { forbidden, HttpError } from "../../../errors.js"; export type GitHubResponseSource = { - endpoint: typeof chatEndpoints.$inferSelect; - conversation: typeof chatConversations.$inferSelect; - delivery: typeof chatDeliveries.$inferSelect; + endpoint: Pick & { setup: unknown }; + conversation: Pick; + delivery: Pick; number: number; replyId: string | null; repositoryId?: string; diff --git a/server/src/services/chat-github-access.ts b/server/src/services/chat-github-access.ts index c906d75a479..22ba2c44c5b 100644 --- a/server/src/services/chat-github-access.ts +++ b/server/src/services/chat-github-access.ts @@ -1,4 +1,4 @@ -import { and, eq } from "drizzle-orm"; +import { and, eq, sql } from "drizzle-orm"; import { chatEndpoints, chatExternalPrincipals, @@ -13,12 +13,15 @@ type Database = Db | Parameters[0]>[0]; /** Current authority shared by task admission and every later publication. */ export async function githubChatPrincipalAccess( db: Database, - endpoint: typeof chatEndpoints.$inferSelect, + endpoint: Pick, principalId: string, ) { if (endpoint.provider !== "github") return null; const [saved] = await db - .select() + .select({ configuration: sql>`jsonb_build_object( + 'people', ${chatGitHubConfigurations.configuration}->'people', + 'memberAccess', ${chatGitHubConfigurations.configuration}->'memberAccess' + )` }) .from(chatGitHubConfigurations) .where( and( @@ -29,7 +32,7 @@ export async function githubChatPrincipalAccess( .for("share"); if (!saved) return null; // Preserve the existing connector until explicitly upgraded. const [principal] = await db - .select() + .select({ externalId: chatExternalPrincipals.externalId }) .from(chatExternalPrincipals) .where( and( @@ -50,7 +53,7 @@ export async function githubChatPrincipalAccess( (p) => p.githubUserId === principal.externalId, ); const [link] = await db - .select() + .select({ status: chatIdentityLinks.status, paperclipUserId: chatIdentityLinks.paperclipUserId }) .from(chatIdentityLinks) .where( and( @@ -62,7 +65,7 @@ export async function githubChatPrincipalAccess( .for("share"); const activeMember = async (userId: string) => { const [membership] = await db - .select() + .select({ membershipRole: companyMemberships.membershipRole }) .from(companyMemberships) .where( and( diff --git a/server/src/services/chat-github-events.ts b/server/src/services/chat-github-events.ts index 4766843513a..c2656f04b73 100644 --- a/server/src/services/chat-github-events.ts +++ b/server/src/services/chat-github-events.ts @@ -20,6 +20,7 @@ import { import { effectiveGitHubReviewPolicy, githubReviewSchedulingDecision, + type GitHubAutomaticPolicyContext, } from "./chat-github-review-policy.js"; const id = z.union([ @@ -226,8 +227,8 @@ export function githubAutomaticReviewEvent( export async function githubAutomaticAdmission( db: Db | Parameters[0]>[0], - endpoint: typeof chatEndpoints.$inferSelect, - context: GitHubAutomaticEventContext, + endpoint: Pick, + context: GitHubAutomaticPolicyContext, ) { const [saved] = await db .select() diff --git a/server/src/services/chat-github-response-comments.ts b/server/src/services/chat-github-response-comments.ts index 8caff8894b9..c032ade25ba 100644 --- a/server/src/services/chat-github-response-comments.ts +++ b/server/src/services/chat-github-response-comments.ts @@ -11,7 +11,8 @@ import { githubAutomaticAdmission } from "./chat-github-events.js"; import { githubBotRepositoryToken, githubBotRequest } from "./chat-github-client.js"; import { withGitHubPublicationLease } from "./chat-github-publication-lease.js"; import { projectSafeChatPublicationText } from "./chat-publication-projection.js"; -import type { GitHubAutomaticEventContext } from "@paperclipai/shared"; +import type { GitHubAutomaticPolicyContext } from "./chat-github-review-policy.js"; +import { githubResponseAuthorityFields } from "./chat-channels/github/response-authority-read.js"; import { githubRunReplyState } from "./chat-run-publications.js"; import { logActivity } from "./activity-log.js"; import { boundRunIssueId } from "./run-task-binding.js"; @@ -159,8 +160,7 @@ export async function writeGitHubResponseComment( export function githubResponseCommentService(db: Db, fetchImpl = fetch) { async function context(deliveryId: string) { - const [row] = await db.select({ endpoint: chatEndpoints, conversation: chatConversations, - delivery: chatDeliveries, resource: chatEndpointResources, config: chatGitHubConfigurations, + const [row] = await db.select({ ...githubResponseAuthorityFields, assignee: issues.assigneeAgentId, issueStatus: issues.status, sourceMessageId: chatMessageLinks.commentId }).from(chatMessageLinks) .innerJoin(chatDeliveries, and(eq(chatDeliveries.id, chatMessageLinks.deliveryId), eq(chatDeliveries.companyId, chatMessageLinks.companyId))) .innerJoin(chatConversations, and(eq(chatConversations.id, chatMessageLinks.conversationId), eq(chatConversations.companyId, chatMessageLinks.companyId))) @@ -170,13 +170,13 @@ export function githubResponseCommentService(db: Db, fetchImpl = fetch) { .innerJoin(issues, and(eq(issues.id, chatConversations.issueId), eq(issues.companyId, chatEndpoints.companyId))) .where(and(eq(chatMessageLinks.deliveryId, deliveryId), eq(chatMessageLinks.direction, "inbound"), eq(chatEndpoints.provider, "github"))); if (!row || !row.delivery.principalId || !["processing", "processed"].includes(row.delivery.state) || - !["active", "verifying"].includes(row.endpoint.status) || !row.config.configuration.toolsEnabled || + !["active", "verifying"].includes(row.endpoint.status) || !row.toolsEnabled || !row.resource.enabled || row.resource.availability !== "available" || row.assignee !== row.endpoint.assignedAgentId) throw forbidden("GitHub request is no longer authorized"); const access = await githubChatPrincipalAccess(db, row.endpoint, row.delivery.principalId); if (!access?.allowed) throw forbidden("The initiating GitHub person is no longer authorized"); const automatic = (row.delivery.normalizedEvent.githubAutomatic ?? row.delivery.normalizedEvent.githubIssue) as - { context: GitHubAutomaticEventContext } | undefined; + { context: GitHubAutomaticPolicyContext } | undefined; if (automatic && !(await githubAutomaticAdmission(db, row.endpoint, automatic.context))?.allowed) throw forbidden("Automatic GitHub task authority changed"); const match = /^github:([^:]+):(?:issue:)?([1-9][0-9]*)(?::rc:([1-9][0-9]*))?$/.exec(row.conversation.externalThreadId); diff --git a/server/src/services/chat-github-review-policy.ts b/server/src/services/chat-github-review-policy.ts index cfb805e1be2..f014e4ae066 100644 --- a/server/src/services/chat-github-review-policy.ts +++ b/server/src/services/chat-github-review-policy.ts @@ -8,9 +8,14 @@ import { type GitHubReviewConclusion, type GitHubReviewEventContext, type GitHubAutomaticEventContext, + type GitHubIssueEventContext, type GitHubReviewPolicy, } from "@paperclipai/shared"; +export type GitHubAutomaticPolicyContext = + | Pick + | Pick; + export function effectiveGitHubReviewPolicy( configuration: GitHubChatConfiguration, repositoryId: string, @@ -85,7 +90,7 @@ export function githubReviewLineIsInPatch( export function githubReviewSchedulingDecision(input: { configuration: GitHubChatConfiguration; - context: GitHubAutomaticEventContext; + context: GitHubAutomaticPolicyContext; repositoryEnabled: boolean; /** Resolved against current company membership and the user's confirmed link. */ linkedMemberUserId: string | null; diff --git a/tests/e2e/agent-chat-projects.spec.ts b/tests/e2e/agent-chat-projects.spec.ts index c8fcdc2345e..a631d3ea928 100644 --- a/tests/e2e/agent-chat-projects.spec.ts +++ b/tests/e2e/agent-chat-projects.spec.ts @@ -1,10 +1,48 @@ -import { expect, test } from "@playwright/test"; +import { expect, test, type APIRequestContext, type APIResponse } from "@playwright/test"; import { idle, json, send, setup } from "./agent-chat.shared"; test.use({ trace: "retain-on-failure" }); test.setTimeout(120_000); +test("cleanup restores agent chat experiments when cancellation fails", async ({ request }) => { + const original = await json(await request.get("/api/instance/settings/experimental")); + const response = (status: number, body: unknown) => ({ + ok: () => status >= 200 && status < 300, + status: () => status, + text: async () => JSON.stringify(body), + json: async () => body, + }) as APIResponse; + const failingRequest = new Proxy(request, { + get(target, property) { + if (property === "get") return async (...args: Parameters) => + args[0].endsWith("/live-runs") + ? response(200, [{ id: "cleanup-cancellation-fixture" }]) + : target.get(...args); + if (property === "post") return async (...args: Parameters) => + args[0].endsWith("/heartbeat-runs/cleanup-cancellation-fixture/cancel") + ? response(500, { error: "simulated cancellation failure" }) + : target.post(...args); + const value = Reflect.get(target, property, target); + return typeof value === "function" ? value.bind(target) : value; + }, + }); + try { + const f = await setup(failingRequest); + await expect(f.restore()).rejects.toThrow("500"); + const restored = await json(await request.get("/api/instance/settings/experimental")); + expect(restored).toMatchObject({ + enableAgentChat: original.enableAgentChat, + enableClassicTaskInterface: original.enableClassicTaskInterface, + }); + } finally { + await json(await request.patch("/api/instance/settings/experimental", { data: { + enableAgentChat: original.enableAgentChat, + enableClassicTaskInterface: original.enableClassicTaskInterface, + } })); + } +}); + /** * Agent chat project and content coverage: project cards, split handoff, * uploads, Ask/Plan modes, repository selection, plan approval, and shared diff --git a/tests/e2e/agent-chat.shared.ts b/tests/e2e/agent-chat.shared.ts index ab235494eb5..592af7641de 100644 --- a/tests/e2e/agent-chat.shared.ts +++ b/tests/e2e/agent-chat.shared.ts @@ -113,21 +113,24 @@ export async function setup(request: APIRequestContext) { ), }); } finally { - const runs = await json( - await request.get(`/api/companies/${company.id}/live-runs`), - ); - for (const run of runs) - await json( - await request.post(`/api/heartbeat-runs/${run.id}/cancel`), + try { + const runs = await json( + await request.get(`/api/companies/${company.id}/live-runs`), ); - await json( - await request.patch("/api/instance/settings/experimental", { - data: { - enableAgentChat: original.enableAgentChat, - enableClassicTaskInterface: original.enableClassicTaskInterface, - }, - }), - ); + for (const run of runs) + await json( + await request.post(`/api/heartbeat-runs/${run.id}/cancel`), + ); + } finally { + await json( + await request.patch("/api/instance/settings/experimental", { + data: { + enableAgentChat: original.enableAgentChat, + enableClassicTaskInterface: original.enableClassicTaskInterface, + }, + }), + ); + } } }, };