diff --git a/doc/connections/GITHUB-REVIEW-BOT.md b/doc/connections/GITHUB-REVIEW-BOT.md index 4a5c8dd50c9..2d9dc55197b 100644 --- a/doc/connections/GITHUB-REVIEW-BOT.md +++ b/doc/connections/GITHUB-REVIEW-BOT.md @@ -240,6 +240,12 @@ 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. +Receipt recovery compares the complete saved publication request, including long +text and nested coverage entries. Shortened audit previews do not establish +request identity. Changed content or a missing durable publication returns a +conflict without dispatching another write. Exact retries read the current +receipt; the durable worker owns pending publication retries. + 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 diff --git a/maps/agent-access-map/chat-channels.md b/maps/agent-access-map/chat-channels.md index 961b84899b8..68cb515a5b1 100644 --- a/maps/agent-access-map/chat-channels.md +++ b/maps/agent-access-map/chat-channels.md @@ -98,7 +98,10 @@ live GitHub credential recovery. GitHub publication recovery (2026-10-14): assigned-agent `submit_review`, `comment`, and `update_comment` calls reuse their run-, task-, endpoint-, and argument-bound -publication on an exact retry. Fresh gateway policy and provider authority remain +publication on an exact retry. Identity uses the complete durable request rather +than the shortened audit preview. A changed suffix, changed nested entry, or +missing publication cannot authorize another write. Exact retries only read the +receipt; the worker owns pending provider retries. Fresh gateway policy and provider authority remain required. The durable worker resumes transient failures after restart and stops at its retry cap, revoked authority, or a superseded head. A pending receipt is not task completion. Source: diff --git a/maps/feature-map/chat-channels.md b/maps/feature-map/chat-channels.md index 9c01ca4f938..82a0ee9b166 100644 --- a/maps/feature-map/chat-channels.md +++ b/maps/feature-map/chat-channels.md @@ -118,7 +118,11 @@ their App-scoped checks and comments remain independent. For GitHub publication recovery, interrupt a response after GitHub accepts a write and verify one summary, finding, and App-scoped check. Retry the same tool -call and verify it returns the saved receipt. Leave a transient failure queued, +call and verify it returns the saved receipt. Include text longer than 500 +characters and coverage with more than 50 entries. Change only the text suffix +or final coverage entry and verify rejection without another write. Remove the +durable publication and verify that a retry cannot dispatch a new write. +Leave a transient failure queued, restart the instance, and verify publication resumes at its saved deadline without another inbound message. Revoked access and permanent rejections must stop retries. These recipes are separate from live provider proof. diff --git a/server/src/__tests__/chat-channels.integration.test.ts b/server/src/__tests__/chat-channels.integration.test.ts index cd3b83ec975..4a6af5b4cb5 100644 --- a/server/src/__tests__/chat-channels.integration.test.ts +++ b/server/src/__tests__/chat-channels.integration.test.ts @@ -2925,7 +2925,9 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { const tools = await githubBotToolsForSession(db, f.session); const tool = tools.find(tool => tool.upstreamToolName === "submit_review")!; await githubChatReviewService(db, f.providerFetch).execute(f.session, "begin_review", { reviewedCommit: head }); - const assessment = { reviewedCommit: head, score: 5, complete: true, summary: "Looks good", rationale: "Reviewed the changed file", + const assessment = { reviewedCommit: head, score: 5, complete: true, + summary: "Reviewed the changed behavior and its callers. ".repeat(30), + rationale: "Checked the mutation boundary and the client retry contract. ".repeat(30), coverage: { reviewedPaths: ["src/math.ts"], omittedPaths: [], limitations: [] }, findings: [] }; const result = await gateway.executeTool({ sessionToken: session.token, tool: tool.name, parameters: assessment, timeoutMs }) .then(value => ({ value }), error => ({ error })); @@ -2948,6 +2950,89 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { expect(invocation).toMatchObject({ status: "succeeded", errorCode: null, errorMessage: null }); expect(f.responseWrites).toHaveLength(writes); }, 40_000); + it.each(["submit_review", "comment", "update_comment"] as const)("recovers long %s requests without accepting changes hidden by the audit preview", async name => { + const f = await publicationRetryFixture(); + vi.stubGlobal("fetch", f.providerFetch); + onTestFinished(() => vi.unstubAllGlobals()); + const gateway = createToolGatewayService(db); + 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 === name)!; + const text = "Checked the changed behavior, connected callers, and coverage. ".repeat(30); + const parameters = name === "submit_review" + ? { ...f.assessment, summary: text, rationale: text, coverage: { ...f.assessment.coverage, + limitations: Array.from({ length: 51 }, (_, index) => `Coverage limitation ${index}`) } } + : { body: text, idempotencyKey: "long-publication" }; + const first = await gateway.executeTool({ sessionToken: session.token, tool: tool.name, parameters }); + const writes = f.responseWrites.length; + const replay = await gateway.executeTool({ sessionToken: session.token, tool: tool.name, parameters }); + expect(replay).toMatchObject({ invocationId: first.invocationId, result: { data: { status: "processed" } } }); + if (name === "submit_review") { + vi.stubEnv("PAPERCLIP_API_URL", "https://paperclip.example.test"); + onTestFinished(() => vi.unstubAllEnvs()); + await initializeRunIdentity(db, { companyId: f.companyId, runId: f.run.id, issueId: f.conversation.issueId!, + responsibleUserId: "owner-user", messageIds: [String(f.run.contextSnapshot?.wakeCommentId)], cause: "instruction" }); + const servers = await buildPaperclipRuntimeMcpServers({ db, + agent: (await db.select().from(agents).where(eq(agents.id, f.assignedAgentId)))[0], runId: f.run.id }); + const assigned = await createAssignedMcpTools({ gateway, + gatewayPublicId: new URL(servers[0].url).pathname.split("/").at(-1)!, bearerToken: servers[0].token }); + const found = await assigned.execute({ tool: "paperclip_search_assigned_tools", arguments: { query: "Submit a review assessment" } }) as { tools: { name: string }[] }; + const submit = found.tools.filter(candidate => candidate.name.includes("__submit_review_")); + expect(submit).toHaveLength(1); + expect(await assigned.execute({ tool: "paperclip_call_assigned_tool", arguments: { name: submit[0].name, arguments: parameters } })) + .toMatchObject({ data: { status: "processed" } }); + } + expect(f.responseWrites).toHaveLength(writes); + expect(f.responseComments.size).toBe(1); + const changed = name === "submit_review" ? { ...parameters, summary: `${text} Changed conclusion.` } + : { ...parameters, body: `${text} Changed conclusion.` }; + await expect(gateway.executeTool({ sessionToken: session.token, tool: tool.name, parameters: changed })) + .rejects.toMatchObject({ status: 409, reasonCode: "github_idempotency_conflict" }); + if (name === "submit_review") { + const changedCoverage = { ...parameters, coverage: { ...parameters.coverage!, + limitations: [...parameters.coverage!.limitations.slice(0, 50), "Changed final limitation"] } }; + await expect(gateway.executeTool({ sessionToken: session.token, tool: tool.name, parameters: changedCoverage })) + .rejects.toMatchObject({ status: 409, reasonCode: "github_idempotency_conflict" }); + } + await db.insert(toolPolicies).values({ companyId: f.companyId, name: "Revoke publication", policyType: "block", priority: 0, + selectors: { catalogEntryId: tool.catalogEntryId! } }); + await expect(gateway.executeTool({ sessionToken: session.token, tool: tool.name, parameters })).rejects.toThrow(); + expect(f.responseWrites).toHaveLength(writes); + }); + it("reads pending GitHub receipts without dispatch and refuses a missing publication", async () => { + const f = await publicationRetryFixture(); + let attempts = 0; + let unavailable = true; + 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()); + const gateway = createToolGatewayService(db); + 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")!; + const input = { sessionToken: session.token, tool: tool.name, parameters: { ...f.assessment, summary: "Review evidence. ".repeat(50) } }; + const first = await gateway.executeTool(input); + expect(first).toMatchObject({ result: { data: { status: "failed", receipt: { retryable: true } } } }); + unavailable = false; + expect(await gateway.executeTool(input)).toMatchObject({ invocationId: first.invocationId, + result: { data: { status: "failed", receipt: { retryable: true } } } }); + expect(attempts).toBe(1); + const action = await f.action(); + await db.update(chatActions).set({ result: { ...action.result, retryAt: new Date(0).toISOString() } }).where(eq(chatActions.id, action.id)); + await githubChatReviewService(db, f.providerFetch).processPending(); + expect(await gateway.executeTool(input)).toMatchObject({ invocationId: first.invocationId, result: { data: { status: "processed" } } }); + expect(attempts).toBe(2); + expect(f.responseComments.size).toBe(1); + const writes = f.responseWrites.length; + await db.delete(chatActions).where(eq(chatActions.id, action.id)); + await expect(gateway.executeTool(input)).rejects.toMatchObject({ status: 409, reasonCode: "github_publication_unavailable" }); + expect(attempts).toBe(2); + expect(f.responseWrites).toHaveLength(writes); + }); it("does not post a GitHub working comment when the scheduler declines the request", async () => { const f = await reviewBotFixture(); f.wakeup.mockImplementation(async (agentId, opts) => { diff --git a/server/src/services/tool-gateway.ts b/server/src/services/tool-gateway.ts index c08b5f72be9..8a0198bd507 100644 --- a/server/src/services/tool-gateway.ts +++ b/server/src/services/tool-gateway.ts @@ -10220,6 +10220,7 @@ export function createToolGatewayService( sensitiveMode: "redact", promptInjectionMode: "ignore", }); + let recoveredGitHubPublication: { content: string; data: unknown } | undefined; let effectiveParameters: unknown = requestedParameters; let effectiveArgumentsSummary = argumentValidation.summary; @@ -10639,12 +10640,29 @@ export function createToolGatewayService( 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 && + recorded.invocation.toolName === tool.name && ["authorized", "executing", "succeeded", "failed", "timed_out"].includes(recorded.invocation.status); if (gitHubPublicationReplay && gitHubReplayPolicyAllowed && !gitHubReplayBindingMatches) throw new ToolGatewayHttpError(409, "Idempotency key belongs to a different GitHub publication", "github_idempotency_conflict"); const retryingGitHubPublication = gitHubPublicationReplay && accessDecision.allowed && recorded.invocation.approvalState === "not_required" && gitHubReplayBindingMatches; + if (retryingGitHubPublication) { + // Invocation hashes describe shortened audit previews. The durable + // publication checks the complete saved request and current authority. + let receipt; + try { + receipt = await githubChatReviewService(db).readPublicationReceipt( + session, tool.upstreamToolName!, effectiveParameters, invocationId, + ); + } catch (error) { + if (error instanceof HttpError && error.status === 409) + throw new ToolGatewayHttpError(409, error.message, "github_idempotency_conflict"); + throw error; + } + if (!receipt) + throw new ToolGatewayHttpError(409, "The saved GitHub publication is not available yet", "github_publication_unavailable"); + recoveredGitHubPublication = { content: JSON.stringify(receipt), data: receipt }; + } if (recorded.replayed && !retryingSlackRateLimit && !retryingGitHubPublication) { if (gitHubPublicationReplay && !gitHubReplayPolicyAllowed) throw new ToolGatewayHttpError(policyErrorStatus(accessDecision), accessDecision.explanation, accessDecision.reasonCode); @@ -10783,7 +10801,7 @@ export function createToolGatewayService( input.timeoutMs === undefined, ) : null; - const result = connectedMcpExecution + const result = recoveredGitHubPublication ?? (connectedMcpExecution ? connectedMcpExecution.result : tool.providerType === "paperclip_plugin" ? await runWithTimeout( @@ -10802,7 +10820,7 @@ export function createToolGatewayService( : await runWithTimeout( executeBuiltinTool(session, tool, effectiveParameters, invocationId), executionTimeoutMs, - ); + )); const resultValidation = validateToolContent({ value: result,