From a6d81b67cb43253d4120a90d0b40ed372f7e4aa5 Mon Sep 17 00:00:00 2001 From: Dotta Date: Wed, 14 Oct 2026 08:14:05 -0500 Subject: [PATCH] fix(github): preserve approved publication recovery Co-Authored-By: Paperclip --- doc/connections/GITHUB-REVIEW-BOT.md | 5 +++ .../chat-channels.integration.test.ts | 40 +++++++++++++++++++ server/src/services/chat-github-reviews.ts | 26 +++++++++++- server/src/services/tool-gateway.ts | 22 ++++++++-- 4 files changed, 87 insertions(+), 6 deletions(-) diff --git a/doc/connections/GITHUB-REVIEW-BOT.md b/doc/connections/GITHUB-REVIEW-BOT.md index c5c487eac20..4a5c8dd50c9 100644 --- a/doc/connections/GITHUB-REVIEW-BOT.md +++ b/doc/connections/GITHUB-REVIEW-BOT.md @@ -240,6 +240,11 @@ still pending. Claim publication only after `status: processed`. Receipt recover does not itself mark a task Done; the agent must finish its task after confirming the receipt. +For a call that required human approval, the durable publication keeps that +verified approval for its exact operation. An exact caller retry only reads the +current receipt; the worker owns provider retries. A new policy denial or revoked +authority still stops recovery, and new content still requires its own approval. + ## Hosted ingress Dedicated Apps use Cloud as a sealed transport gateway. Cloud routes opaque diff --git a/server/src/__tests__/chat-channels.integration.test.ts b/server/src/__tests__/chat-channels.integration.test.ts index 374002c2336..24fa9b9ac7f 100644 --- a/server/src/__tests__/chat-channels.integration.test.ts +++ b/server/src/__tests__/chat-channels.integration.test.ts @@ -2594,6 +2594,46 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { )))[0]; return { ...f, service, head, assessment, provider, action, inline, checks, writes }; } + it.each([false, true])("recovers an approved queued GitHub review without another approval (revoked: %s)", async revoked => { + const f = await publicationRetryFixture(); + let unavailable = true; + let attempts = 0; + f.setSupplementalProviderFetch(async (input, init) => { + if (String(input).includes("/issues/comments/") && init?.method === "PATCH") { + attempts++; + if (unavailable) return new Response("Unavailable", { status: 503, headers: { "retry-after": "120" } }); + } + return f.provider(input, init); + }); + vi.stubGlobal("fetch", f.providerFetch); + onTestFinished(() => { vi.unstubAllGlobals(); }); + await initializeRunIdentity(db, { companyId: f.companyId, runId: f.run.id, issueId: f.conversation.issueId!, + responsibleUserId: null, cause: "company_default" }); + const { createToolGatewayService } = await import("../services/tool-gateway.js"); + const gateway = createToolGatewayService(db, { toolActionSigningSecret: "github-approval-retry-test-secret" }); + const session = await gateway.createSession({ companyId: f.companyId, agentId: f.assignedAgentId, runId: f.run.id }); + const tool = (await githubBotToolsForSession(db, f.session)).find(tool => tool.upstreamToolName === "submit_review")!; + await db.insert(toolPolicies).values({ companyId: f.companyId, name: "Review publication approval", policyType: "require_approval", + priority: 0, selectors: { catalogEntryId: tool.catalogEntryId! } }); + await expect(gateway.executeTool({ sessionToken: session.token, tool: tool.name, parameters: f.assessment })) + .rejects.toMatchObject({ reasonCode: "approval_required" }); + const [approval] = await db.select().from(toolActionRequests).where(eq(toolActionRequests.companyId, f.companyId)); + expect(await gateway.approveActionRequest({ companyId: f.companyId, actionRequestId: approval.id, actor: { userId: "owner-user" } })) + .toMatchObject({ status: "executed" }); + const action = await f.action(); + expect(action).toMatchObject({ status: "failed", result: { retryable: true } }); + unavailable = false; + if (revoked) await db.insert(toolPolicies).values({ companyId: f.companyId, name: "Revoke approved publication", policyType: "block", + priority: -1, selectors: { catalogEntryId: tool.catalogEntryId! } }); + await db.update(chatActions).set({ result: { ...action.result, retryAt: new Date(0).toISOString() } }).where(eq(chatActions.id, action.id)); + await f.service.processPending(); + expect((await f.action()).status).toBe(revoked ? "cancelled" : "processed"); + if (revoked) await expect(gateway.executeTool({ sessionToken: session.token, tool: tool.name, parameters: f.assessment })).rejects.toThrow(); + else expect(await gateway.executeTool({ sessionToken: session.token, tool: tool.name, parameters: f.assessment })) + .toMatchObject({ status: "replayed", result: { data: { status: "processed", score: 3 } } }); + expect(attempts).toBe(revoked ? 1 : 2); + expect(await db.select().from(toolActionRequests).where(eq(toolActionRequests.companyId, f.companyId))).toHaveLength(1); + }, 25_000); it("recovers lost inline and check responses without duplicate GitHub publications", async () => { const f = await publicationRetryFixture(); const lost = new Set(); diff --git a/server/src/services/chat-github-reviews.ts b/server/src/services/chat-github-reviews.ts index b14066f8d64..9367e02f1b3 100644 --- a/server/src/services/chat-github-reviews.ts +++ b/server/src/services/chat-github-reviews.ts @@ -545,6 +545,28 @@ export function githubChatReviewService(db: Db, fetchImpl = fetch) { } return receipt(actionId); } + async function readPublicationReceipt(session: Session, name: string, input: unknown, invocationId: string) { + const source = await scope(session); + const [action] = await db.select().from(chatActions).where(and( + eq(chatActions.companyId, session.companyId), eq(chatActions.endpointId, source.endpoint.id), + eq(chatActions.conversationId, source.conversation.id), eq(chatActions.kind, "github_review_publication"), + sql`${chatActions.payload}->>'invocationId' = ${invocationId}`, sql`${chatActions.payload}->>'toolName' = ${name}`, + )); + if (!action) return null; + const binding = action.payload.session as Session; + if (binding.companyId !== session.companyId || binding.agentId !== session.agentId || binding.runId !== session.runId || + binding.issueId !== session.issueId || hash(action.payload.parameters) !== hash(input)) + throw conflict("This GitHub invocation is bound to a different publication"); + await assertPublicationAuthority(source, action); + const result = await receipt(action.id); + if (name !== "submit_review") return result; + const [review] = await db.select().from(chatGitHubReviews).where(and( + eq(chatGitHubReviews.companyId, session.companyId), eq(chatGitHubReviews.id, String(action.payload.reviewId)), + eq(chatGitHubReviews.runId, session.runId!), eq(chatGitHubReviews.endpointId, source.endpoint.id), + )); + if (!review?.assessment) throw conflict("GitHub publication assessment is unavailable"); + return { reviewId: review.id, ...result, score: review.assessment.score, conclusion: review.conclusion }; + } async function execute( session: Session, name: string, @@ -977,7 +999,7 @@ export function githubChatReviewService(db: Db, fetchImpl = fetch) { ); if ( invocation?.approvalState === "approved" && - ["executing", "completed"].includes(invocation.status) + ["executing", "completed", "succeeded", "failed", "timed_out"].includes(invocation.status) ) return; } @@ -1527,5 +1549,5 @@ export function githubChatReviewService(db: Db, fetchImpl = fetch) { )); return row?.at ? new Date(row.at).getTime() : null; } - return { execute, processPending, nextPendingAt, scope }; + return { execute, readPublicationReceipt, processPending, nextPendingAt, scope }; } diff --git a/server/src/services/tool-gateway.ts b/server/src/services/tool-gateway.ts index 7158277f34e..b107011c696 100644 --- a/server/src/services/tool-gateway.ts +++ b/server/src/services/tool-gateway.ts @@ -10150,12 +10150,21 @@ export function createToolGatewayService( argumentsHash: argumentValidation.summary.sha256 ?? "", }); if (replay?.matched) { + let replayResult: unknown = replay.result; + if (tool.providerType === "paperclip_github_chat" && ["comment", "update_comment", "submit_review"].includes(tool.upstreamToolName ?? "")) { + const access = await policyService.decide(policyInputForTool({ session, tool, parameters: effectiveParameters, consumeRateLimit: false })); + if (!access.allowed && access.decision !== "require_approval") + throw new ToolGatewayHttpError(policyErrorStatus(access), access.explanation, access.reasonCode); + const receipt = await githubChatReviewService(db).readPublicationReceipt(session, tool.upstreamToolName ?? "", effectiveParameters, replay.invocationId); + if (receipt) replayResult = validateToolContent({ value: { content: JSON.stringify(receipt), data: receipt }, + direction: "result", sensitiveMode: "redact", promptInjectionMode: "block" }).value; + } return { invocationId: replay.invocationId, status: "replayed" as const, tool: virtualToolName ?? tool.name, targetTool: virtualToolName ? tool.name : undefined, - result: replay.result, + result: replayResult, }; } } @@ -10546,14 +10555,19 @@ export function createToolGatewayService( tool.providerType === "paperclip_github_chat" && ["comment", "update_comment", "submit_review"].includes(tool.upstreamToolName ?? "") && recorded.invocation.approvalState === "not_required"; - const retryingGitHubPublication = gitHubPublicationReplay && accessDecision.allowed && + const gitHubReplayPolicyAllowed = accessDecision.allowed; + const gitHubReplayBindingMatches = recorded.invocation.agentId === session.agentId && recorded.invocation.runId === session.runId && recorded.invocation.issueId === session.issueId && recorded.invocation.connectionId === tool.connectionId && recorded.invocation.toolName === tool.name && recorded.invocation.argumentsHash === effectiveArgumentsSummary.sha256 && ["authorized", "executing", "succeeded", "failed", "timed_out"].includes(recorded.invocation.status); - if (gitHubPublicationReplay && accessDecision.allowed && !retryingGitHubPublication) + if (gitHubPublicationReplay && gitHubReplayPolicyAllowed && !gitHubReplayBindingMatches) throw new ToolGatewayHttpError(409, "Idempotency key belongs to a different GitHub publication", "github_idempotency_conflict"); - if (recorded.replayed && !retryingSlackRateLimit && !gitHubPublicationReplay) { + const retryingGitHubPublication = gitHubPublicationReplay && accessDecision.allowed && + recorded.invocation.approvalState === "not_required" && gitHubReplayBindingMatches; + if (recorded.replayed && !retryingSlackRateLimit && !retryingGitHubPublication) { + if (gitHubPublicationReplay && !gitHubReplayPolicyAllowed) + throw new ToolGatewayHttpError(policyErrorStatus(accessDecision), accessDecision.explanation, accessDecision.reasonCode); await writeAudit({ session, companyId: session.companyId,