From a8b652b0855668e60b7636dcb9be4f20ab39d595 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Tue, 13 Oct 2026 22:24:41 -0500 Subject: [PATCH] fix(github): preserve review delivery through reconnect (#16259) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - GitHub review bots turn authorized repository events into agent tasks and publish the result on the pull request. > - Cloud Apps send callbacks through a relay, but the direct callback recovery worker reported that URL as broken. > - Reconnecting the same App then prevented a new authorized publication from updating its existing working comment. > - Report-only runs could also produce an action-required check when no complete assessment was available. > - This pull request preserves comment identity across reconnects, verifies fresh signed delivery after repair, and keeps report-only results neutral. > - Users can receive the review without changing repository access or making an advisory bot block their pull request. ## Linked Issues or Issue Description **What happened?** A review could be saved in Paperclip while GitHub kept showing “Working on this…”. A same-App reconnect changed the runtime generation and the existing response receipt then failed with “GitHub response binding changed”. Cloud callback recovery also compared the relay URL with a direct tenant URL. An incomplete or failed report-only run could produce an action-required check. **Expected behavior** A fresh authorized attempt updates the same App-owned comment after reconnect. Stale attempts and revoked people remain denied. Cloud relay delivery does not produce a direct-callback mismatch. Report-only checks remain neutral and explain incomplete work honestly. **Steps to reproduce** 1. Accept a GitHub request and publish its working comment. 2. Reconnect the same App before the agent publishes its result. 3. Submit the result and inspect the working comment and check. 4. Separately, let a report-only run end without an assessment and inspect its check conclusion. Related: #16138 addresses the separate classification of local authorization errors as provider credential failures. This change fixes the response binding and report-only paths themselves. ## What Changed - Bind durable response comments to the App and agent. Keep per-attempt runtime generation checks and provider comment ownership checks, including legacy receipts. - Exclude Cloud-managed Apps from the direct webhook recovery scan and its stale callback-mismatch warning. The Cloud inbox owns relay retries. - Let a fresh authenticated event from the bound installation verify webhook delivery after repair. Keep signature, runtime epoch, installation, and credential lease checks. - Keep report-only assessment and terminal-run checks neutral. Preserve incomplete/error text and gated-review behavior. - Add integration regressions for reconnect races, legacy comments, failed runs, Cloud recovery, and foreign or forged webhook traffic. Update connector documentation. - Correct the assigned-tool search guidance to name the actual input fields (`offset` and `schemaOffset`) and show valid arguments. The failed automatic review had an input-schema rejection before tool execution; this clarifies recovery without expanding tool access. ## Verification - `pnpm -r typecheck` passed. - GitHub policy and integration suite: 291 tests passed (`vitest run server/src/services/chat-github-review-policy.test.ts server/src/__tests__/chat-channels.integration.test.ts -t 'GitHub|github'`). - The report-only policy regression failed before the fix and passed after it. - `pnpm build` passed. - Assigned tool discovery: 24 tests passed. Its guidance now maps returned pagination offsets to the correct input fields and tells the model to omit unused optional fields. - Full `pnpm test:run` is running. It encountered a 15-second timeout in the unrelated authenticated Workspace calendar-write test. That test passed on one isolated rerun. - After the review-requested module extraction, server typecheck and both direct/Cloud recovery activity integration checks passed. - All 55 non-skipped CI checks passed for `32e690279d847e44a6946b6c7cfb110fb11aff79`. Greptile scored that commit 5/5 with no unresolved findings. Live staging publication verification follows deployment. ## Risks - A durable response can now survive a same-App reconnect. Fresh authorization, runtime generation checks for each publication attempt, App ownership, and the publication lease still fence writes. - Report-only failures intentionally become neutral. The check still reports the failure; it does not claim a complete assessment. - Cloud-managed callbacks rely on their existing durable relay inbox. The direct callback recovery worker remains unchanged for direct installations. - No schema migration or permission expansion. ## Model Used OpenAI Codex (GPT-6), with code inspection, editing, test execution, and browser verification. The exact deployment model ID and context window are not exposed in this session. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- doc/connections/GITHUB.md | 10 +- .../chat-channels.integration.test.ts | 94 +++++++++++++++++++ server/src/services/chat-channels/activity.ts | 41 +------- .../services/chat-channels/github/activity.ts | 47 ++++++++++ .../github/webhook-authentication.ts | 63 ++++++------- .../chat-channels/github/webhook-ingress.ts | 4 + server/src/services/chat-github-checks.ts | 9 +- .../services/chat-github-response-comments.ts | 9 +- .../chat-github-review-policy.test.ts | 5 +- .../src/services/chat-github-review-policy.ts | 2 +- .../native-runtime/assigned-mcp-tools.ts | 2 +- 11 files changed, 204 insertions(+), 82 deletions(-) create mode 100644 server/src/services/chat-channels/github/activity.ts diff --git a/doc/connections/GITHUB.md b/doc/connections/GITHUB.md index e50df130dd3..352ac9b2b45 100644 --- a/doc/connections/GITHUB.md +++ b/doc/connections/GITHUB.md @@ -47,6 +47,8 @@ incomplete assessments require action. To block merging, separately require this check from the dedicated App in GitHub branch protection or a ruleset. A new head commit needs its own check. Report-only mode publishes a neutral conclusion, which GitHub can accept for a required check; it does not enforce a score requirement. +This includes incomplete assessments and failed runs: their checks remain neutral +and explain the failure without presenting a passing assessment. A passing score does not formally approve a PR. **Approvals and change requests** contains those optional agent permissions. Scheduling filters, ignored files, and inline-comment options have separate disclosures. The header shows the App @@ -213,8 +215,12 @@ claims. It does not exchange codes, track App or installation identity, store GitHub secrets, or interpret bot events. It forwards original webhook bytes, provider headers, and signature in an envelope sealed to the stack before writing the inbox. The stack decrypts and authenticates those bytes through its -existing durable ingress. Only that local check can verify a connection or -admit work. Shared-App account connectors keep the behavior described above. +existing durable ingress. Cloud-managed Apps are excluded from the direct-tenant +callback recovery scanner and its callback-mismatch warning; the Cloud inbox owns +relay delivery retries. Only that local check can verify a connection or +admit work. After a repair, a fresh authenticated event for the bound installation +can verify webhook delivery even when the relay deduplicates a redelivered old ping. +Shared-App account connectors keep the behavior described above. Unverified gateway traffic has request, byte, pending-queue, retention and wake limits. Its transport deduplication includes the body and signature, so a forged diff --git a/server/src/__tests__/chat-channels.integration.test.ts b/server/src/__tests__/chat-channels.integration.test.ts index 1f23e99d0e9..1b9d41d72ed 100644 --- a/server/src/__tests__/chat-channels.integration.test.ts +++ b/server/src/__tests__/chat-channels.integration.test.ts @@ -2731,6 +2731,36 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { expect(f.responseComments.size).toBe(1); expect(f.runtime.endpoints.get(f.endpoint.id)?.posts).toEqual([]); }); + it.each([false, true])("finishes the same GitHub working comment after an authorized same-App reconnect (legacy: %s)", async legacy => { + const f = await toolReplyFixture(); + const originalId = f.responseWrites[0].id; + if (legacy) await db.update(chatActions).set({ payload: { runtimeGeneration: f.endpoint.setup.runtimeGeneration ?? 0, agentId: f.assignedAgentId } }) + .where(and(eq(chatActions.endpointId, f.endpoint.id), eq(chatActions.kind, "github_response_comment"))); + await f.service.configure(f.endpoint.id, { action: "reconnect" }, "owner-user"); + const result = await githubChatReviewService(db, f.providerFetch).execute(f.session, "comment", { + body: "Review complete after reconnect", idempotencyKey: "reconnected-final", + }); + expect(result.status).toBe("processed"); + expect(f.responseWrites.map(write => write.method)).toEqual(["POST", "PATCH"]); + expect(f.responseWrites.every(write => write.id === originalId)).toBe(true); + expect([...f.responseComments.values()][0].body).toContain("Review complete after reconnect"); + }); + it("still fences a GitHub publication when reconnect races the provider read", async () => { + const f = await toolReplyFixture(); + let reconnected = false; + f.setSupplementalProviderFetch(async (input, init) => { + if (!reconnected && String(input).includes("/issues/comments/") && (!init?.method || init.method === "GET")) { + reconnected = true; + await f.service.configure(f.endpoint.id, { action: "reconnect" }, "owner-user"); + } + return undefined; + }); + const result = await githubChatReviewService(db, f.providerFetch).execute(f.session, "comment", { + body: "Stale attempt", idempotencyKey: "reconnect-race", + }); + expect(result.status).toBe("cancelled"); + expect(f.responseWrites).toHaveLength(1); + }); it("adds the progress tool to an existing enabled bot without restoring disabled tools", async () => { const f = await toolReplyFixture(); await db.delete(toolCatalogEntries).where(and(eq(toolCatalogEntries.connectionId, f.endpoint.connectionId), eq(toolCatalogEntries.name, "github_bot:update_comment"))); @@ -4078,6 +4108,22 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { expect((await context.service.listResources(context.endpoint.id)).every(resource => !resource.enabled)).toBe(true); expect(await management.configuration(context.endpoint.id, "owner-user")).toMatchObject({ revision: 1, configuration: { toolsEnabled: false } }); }); + it.each(["current", "foreign", "invalid_signature"] as const)("verifies a repaired GitHub webhook only from a %s installation delivery", async kind => { + const f = await reviewBotFixture(); + const [endpoint] = await readChatEndpointsById(f.endpoint.id); + await db.update(chatEndpoints).set({ setup: { ...endpoint.setup, webhookVerifiedAt: null } }).where(eq(chatEndpoints.id, endpoint.id)); + const response = await f.service.handleWebhook(endpoint.publicId, "github", signedGitHubWebhookRequest({ + event: "issue_comment", delivery: randomUUID(), webhookSecret: kind === "invalid_signature" ? "wrong-secret" : f.webhookSecret, + payload: { action: "created", installation: { id: kind === "foreign" ? 9999 : 2468 }, + repository: { id: 97531, full_name: "paperclipai/paperclip", owner: { id: 1357, login: "paperclipai" } }, + issue: { number: 418 }, comment: { id: 41801, body: "Ordinary comment", user: { id: 42, login: "octocat" } }, sender: { id: 42, login: "octocat" } }, + })); + if (kind === "invalid_signature") expect(response.status).toBe(401); + else expect(response.status).toBeLessThan(300); + const current = await f.service.get(endpoint.id); + if (kind === "current") expect(current.setup.webhookVerifiedAt).toEqual(expect.any(String)); + else expect(current.setup.webhookVerifiedAt).toBeNull(); + }); it("stack rejects forged raw gateway deliveries without changing verification or waking an agent", async () => { const f = await reviewBotFixture(); await db @@ -4922,6 +4968,37 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { })]); expect(await readChatGitHubReviewsByEndpointId(f.endpoint.id)).toHaveLength(0); }); + it.each(["succeeded", "failed", "cancelled"] as const)("keeps a report-only GitHub check neutral after a %s run without an assessment", async status => { + const f = await toolReplyFixture("github:paperclipai/paperclip:418"); + const config = await f.management.configuration(f.endpoint.id, "owner-user"); + await f.management.saveConfiguration(f.endpoint.id, { expectedRevision: config.revision, + configuration: { ...config.configuration, defaults: { ...config.configuration.defaults, ratingThreshold: null } }, + }, "owner-user"); + const event = githubAutomaticReviewEvent({ action: "opened", repository: { id: 97531, full_name: "paperclipai/paperclip" }, sender: { id: 42, login: "octocat" }, + pull_request: { number: 418, title: "Review", body: "", draft: false, base: { sha: "a".repeat(40), ref: "master" }, head: { sha: "b".repeat(40) }, user: { id: 42, login: "octocat" }, labels: [] } }, randomUUID())!; + const [review] = await db.insert(chatGitHubReviews).values({ companyId: f.companyId, endpointId: f.endpoint.id, + issueId: f.conversation.issueId!, runId: f.run.id, repositoryId: "97531", repository: event.repository, pullNumber: 418, headSha: event.headSha, + deliveryId: event.deliveryId, configurationRevision: config.revision + 1, policySnapshot: { ...config.configuration.defaults, ratingThreshold: null }, event }).returning(); + await db.update(heartbeatRuns).set({ status }).where(eq(heartbeatRuns.id, f.run.id)); + const writes: Record[] = []; + f.setSupplementalProviderFetch(async (input, init) => { + const url = String(input); + if (url.endsWith("/pulls/418")) return Response.json({ head: { sha: event.headSha } }); + if (url.includes("/check-runs?")) return Response.json({ check_runs: [] }); + if (url.endsWith("/check-runs") && init?.method === "POST") { + writes.push(JSON.parse(String(init.body))); + return Response.json({ id: 418, html_url: "https://github.com/checks/418" }); + } + return undefined; + }); + const checks = githubReviewCheckService(db, f.providerFetch); + const [endpoint] = await readChatEndpointsById(f.endpoint.id); + await checks.enqueue(endpoint, event, true, "authorized"); + await checks.processPending(); + expect(writes).toEqual([expect.objectContaining({ status: "completed", conclusion: "neutral" })]); + expect(writes[0].output).toMatchObject({ title: status === "succeeded" ? "Agent finished without a complete assessment" : "Review execution did not complete" }); + expect((await db.select().from(chatGitHubReviews).where(eq(chatGitHubReviews.id, review.id)))[0].conclusion).toBe("neutral"); + }); it("uses task-bound bot tools and deterministic checks, then denies revoked people", async () => { const publicOrigin = vi.spyOn(cloudRuntimeIdentity, "runtimePublicOrigin").mockReturnValue("https://current-vanity.example"); onTestFinished(() => publicOrigin.mockRestore()); @@ -12917,6 +12994,23 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { await configured.service.shutdown(); }); + it("leaves Cloud-managed GitHub webhook recovery to the relay", async () => { + const context = await githubPreIngressRecoveryFixture(); + try { + const [endpoint] = await readChatEndpointsById(context.endpoint.id); + await db.update(chatEndpoints).set({ setup: { ...endpoint.setup, + github: { ...endpoint.setup.github, cloudRegistrationId: "cloud-registration" }, + } }).where(eq(chatEndpoints.id, endpoint.id)); + const checkpoint = await context.checkpoint(); + await db.update(chatSdkState).set({ value: { ...(checkpoint.value as Record), outcome: "callback_mismatch" } }).where(eq(chatSdkState.id, checkpoint.id)); + await context.makeScanDue(); + await context.service.processFailedGitHubWebhookDeliveries(5, endpoint.id); + expect((await context.service.listActivity(endpoint.id)).some(row => row.summary === "GitHub webhook recovery needs attention")).toBe(false); + expect(context.requests).toEqual([]); + expect(context.posts).toEqual([]); + } finally { await context.service.shutdown(); } + }); + it("automatically recovers a pre-ingress GitHub failure only through a genuine callback and normal durable workers", async () => { const context = await githubPreIngressRecoveryFixture(); const { service, endpoint } = context; diff --git a/server/src/services/chat-channels/activity.ts b/server/src/services/chat-channels/activity.ts index 0a9824ae577..2faed157013 100644 --- a/server/src/services/chat-channels/activity.ts +++ b/server/src/services/chat-channels/activity.ts @@ -1,7 +1,7 @@ import { providerEffectPayload } from "./provider-effects.js"; import { slackSlashTaskRecoveryPayload } from "./slack/task-start.js"; -import { GITHUB_RECOVERY_STATE_KEY } from "./github/webhook-ingress.js"; +import { githubRecoveryActivity } from "./github/activity.js"; import { and, @@ -19,7 +19,6 @@ import { chatConversations, chatDeliveries, chatPublications, - chatSdkState, chatTeamsFileTransfers, issues, } from "@paperclipai/db"; @@ -270,17 +269,7 @@ export function createChatActivity(db: Db, options: ChatActivityOptions) { ) .orderBy(desc(sql`date_trunc('milliseconds', ${chatActions.updatedAt})`), desc(chatActions.id)) .limit(limit), - db - .select() - .from(chatSdkState) - .where( - and( - eq(chatSdkState.endpointId, endpointId), - before(chatSdkState.updatedAt, chatSdkState.id), - eq(chatSdkState.stateKey, GITHUB_RECOVERY_STATE_KEY), - ), - ) - .limit(1), + githubRecoveryActivity(db, endpointId, page?.before), ]); // A provider effect can be on another page. Replay safety must still // consider every unresolved effect associated with these deliveries. @@ -559,31 +548,7 @@ export function createChatActivity(db: Db, options: ChatActivityOptions) { replayable: false, resolutionActions: [], })), - ...githubRecoveryScans.flatMap((row) => { - const value = row.value as { outcome?: string } | null; - const detail = - value?.outcome === "history_limit_reached" - ? "Recent webhook volume exceeded automatic recovery's bounded scan. No partial history was replayed. Check the App's Recent Deliveries for missed messages." - : value?.outcome === "callback_mismatch" - ? "The GitHub App's callback no longer matches this connection. Reconnect the App to repair it; repository access will not change." - : value?.outcome === "scan_failed" - ? "Paperclip could not check GitHub's failed deliveries. The check will retry with backoff; normal callbacks are still processed." - : null; - return detail - ? [ - { - id: row.id, - kind: "health" as const, - status: "attention", - summary: "GitHub webhook recovery needs attention", - detail, - createdAt: row.updatedAt.toISOString(), - replayable: false, - resolutionActions: [], - }, - ] - : []; - }), + ...githubRecoveryScans, ] .sort((a, b) => b.createdAt.localeCompare(a.createdAt) || b.id.localeCompare(a.id)) .slice(0, limit); diff --git a/server/src/services/chat-channels/github/activity.ts b/server/src/services/chat-channels/github/activity.ts new file mode 100644 index 00000000000..561e1fd7e12 --- /dev/null +++ b/server/src/services/chat-channels/github/activity.ts @@ -0,0 +1,47 @@ +import { and, eq, sql } from "drizzle-orm"; +import { chatEndpoints, chatSdkState, type Db } from "@paperclipai/db"; +import { GITHUB_RECOVERY_STATE_KEY } from "./webhook-ingress.js"; + +/** Direct callback recovery warnings do not describe Cloud relay delivery. */ +export async function githubRecoveryActivity( + db: Db, + endpointId: string, + before?: { createdAt: string; id: string }, +) { + const rows = await db + .select({ id: chatSdkState.id, value: chatSdkState.value, updatedAt: chatSdkState.updatedAt }) + .from(chatSdkState) + .innerJoin(chatEndpoints, eq(chatEndpoints.id, chatSdkState.endpointId)) + .where(and( + eq(chatSdkState.endpointId, endpointId), + eq(chatSdkState.stateKey, GITHUB_RECOVERY_STATE_KEY), + sql`nullif(${chatEndpoints.setup}#>>'{github,cloudRegistrationId}', '') is null`, + before ? sql`(date_trunc('milliseconds', ${chatSdkState.updatedAt}), ${chatSdkState.id}) < (${before.createdAt}::timestamptz, ${before.id}::uuid)` : undefined, + )) + .limit(1); + return rows.flatMap((row) => { + const value = row.value as { outcome?: string } | null; + const detail = + value?.outcome === "history_limit_reached" + ? "Recent webhook volume exceeded automatic recovery's bounded scan. No partial history was replayed. Check the App's Recent Deliveries for missed messages." + : value?.outcome === "callback_mismatch" + ? "The GitHub App's callback no longer matches this connection. Reconnect the App to repair it; repository access will not change." + : value?.outcome === "scan_failed" + ? "Paperclip could not check GitHub's failed deliveries. The check will retry with backoff; normal callbacks are still processed." + : null; + return detail + ? [ + { + id: row.id, + kind: "health" as const, + status: "attention", + summary: "GitHub webhook recovery needs attention", + detail, + createdAt: row.updatedAt.toISOString(), + replayable: false, + resolutionActions: [], + }, + ] + : []; + }); +} diff --git a/server/src/services/chat-channels/github/webhook-authentication.ts b/server/src/services/chat-channels/github/webhook-authentication.ts index 9810d51750d..31bcf6f820e 100644 --- a/server/src/services/chat-channels/github/webhook-authentication.ts +++ b/server/src/services/chat-channels/github/webhook-authentication.ts @@ -4,6 +4,7 @@ import { createHmac, timingSafeEqual } from "node:crypto"; import { eq } from "drizzle-orm"; import { recordChatWebhookReceipt } from "../../chat-webhook-diagnostics.js"; import type { Db } from "@paperclipai/db"; +import type { CredentialMutationLeaseGuard } from "../../chat-credential-mutation-lease.js"; import { chatEndpoints } from "@paperclipai/db"; import { logger } from "../../../middleware/logger.js"; import { logActivity } from "../../activity-log.js"; @@ -34,6 +35,24 @@ export function retryableGitHubWebhookResponse(): Response { } export function createGitHubWebhookAuthentication(db: Db, options: AuthenticationOptions) { const { endpointRecord, runtimeContextForRecord, withCredentialMutationLease, resolveCredentials, reconcileGitHubWebhookRepository, stageGitHubWebhookIngress, processGitHubWebhookIngress, githubIngressAcceptedResponse, scheduleMessageProcessing, backgroundMessageTasks, redactError } = options; + async function recordVerifiedWebhook(endpoint: EndpointRow, credentialLease: CredentialMutationLeaseGuard, providerDeliveryId: string | null) { + const verifiedAt = new Date(); + await db.transaction(async tx => { + // Verification belongs to the same credential lease and ingress epoch + // that authenticated the original provider bytes. + await credentialLease.assertOwned(tx); + await tx.update(chatEndpoints).set({ + setup: { ...endpoint.setup, webhookVerifiedAt: verifiedAt.toISOString() }, + healthMessage: "GitHub webhook verified", updatedAt: verifiedAt, + }).where(eq(chatEndpoints.id, endpoint.id)); + await logActivity(tx as unknown as Db, { + companyId: endpoint.companyId, actorType: "system", actorId: "github-webhook", + action: "chat_endpoint.webhook_verified", entityType: "tool_connection", entityId: endpoint.connectionId, + details: { endpointId: endpoint.id, provider: "github", providerDeliveryId }, + }); + await credentialLease.assertOwned(tx); + }); + } async function authenticateGitHubWebhook(endpoint: EndpointRow, request: Request, scope: GitHubWebhookScope): Promise { const { replayingDurableGitHubIngress, matchesGitHubIngressFence, completeGitHubPreflightBeforeDeadline } = scope; if (!replayingDurableGitHubIngress) { @@ -107,39 +126,7 @@ export function createGitHubWebhookAuthentication(db: Db, options: Authenticatio valid = false; } if (!valid) return false; - const verifiedAt = new Date(); - await db.transaction(async (tx) => { - // Bind the verified receipt to the same lease and secret that - // authenticated it. A reclaimed lease cannot commit a stale - // webhook verification after secret rotation or reconnect. - await credentialLease.assertOwned(tx); - await tx - .update(chatEndpoints) - .set({ - setup: { - ...current.endpoint.setup, - webhookVerifiedAt: verifiedAt.toISOString(), - }, - healthMessage: "GitHub webhook verified", - updatedAt: verifiedAt, - }) - .where(eq(chatEndpoints.id, endpoint.id)); - await logActivity(tx as unknown as Db, { - companyId: current.endpoint.companyId, - actorType: "system", - actorId: "github-webhook", - action: "chat_endpoint.webhook_verified", - entityType: "tool_connection", - entityId: current.endpoint.connectionId, - details: { - endpointId: current.endpoint.id, - provider: "github", - providerDeliveryId: - inspection.headers.get("x-github-delivery") ?? null, - }, - }); - await credentialLease.assertOwned(tx); - }); + await recordVerifiedWebhook(current.endpoint, credentialLease, inspection.headers.get("x-github-delivery")); return true; }, ); @@ -233,6 +220,16 @@ export function createGitHubWebhookAuthentication(db: Db, options: Authenticatio // the endpoint. return "ignored"; } + // A fresh signed event for the bound installation proves delivery + // after repair even when Cloud has already deduplicated the old ping. + // Foreign/lifecycle traffic cannot qualify a changed installation. + if (!current.endpoint.setup.webhookVerifiedAt && + ["active", "verifying"].includes(current.endpoint.status) && + incomingInstallationId !== undefined && + String(incomingInstallationId) === credentials.installationId && + eventType !== "installation") { + await recordVerifiedWebhook(current.endpoint, credentialLease, inspection.headers.get("x-github-delivery")); + } await reconcileGitHubWebhookRepository( current.endpoint, payload, diff --git a/server/src/services/chat-channels/github/webhook-ingress.ts b/server/src/services/chat-channels/github/webhook-ingress.ts index b877445881c..16ff2964438 100644 --- a/server/src/services/chat-channels/github/webhook-ingress.ts +++ b/server/src/services/chat-channels/github/webhook-ingress.ts @@ -1121,6 +1121,10 @@ export function createGitHubWebhookIngress(db: Db, options: GitHubWebhookIngress and( eq(chatEndpoints.provider, "github"), eq(chatEndpoints.status, "active"), + // Cloud Apps target the durable Cloud relay, not the direct tenant + // callback checked by this recovery worker. The Cloud inbox owns + // transport retries for those registrations. + sql`nullif(${chatEndpoints.setup}#>>'{github,cloudRegistrationId}', '') is null`, eq(toolConnections.enabled, true), eq(toolConnections.status, "active"), onlyEndpointId ? eq(chatEndpoints.id, onlyEndpointId) : undefined, diff --git a/server/src/services/chat-github-checks.ts b/server/src/services/chat-github-checks.ts index 54880b73cc4..e8d51487d28 100644 --- a/server/src/services/chat-github-checks.ts +++ b/server/src/services/chat-github-checks.ts @@ -240,6 +240,11 @@ export function githubReviewCheckService(db: Db, fetchImpl = fetch) { : state === "queued" ? "queued" : "completed"; + const policy = review?.policySnapshot ?? { + ...source.configuration.defaults, + ...source.configuration.repositories[event.repositoryId], + }; + const conclusion = policy.ratingThreshold === null ? "neutral" : "action_required"; const title = state === "manual_required" ? "Authorized manual review required" @@ -337,7 +342,7 @@ export function githubReviewCheckService(db: Db, fetchImpl = fetch) { ...(detailsUrl ? { details_url: detailsUrl } : {}), status, ...(status === "completed" - ? { conclusion: "action_required" } + ? { conclusion } : {}), output: { title, @@ -356,7 +361,7 @@ export function githubReviewCheckService(db: Db, fetchImpl = fetch) { checkId: String(posted.id), checkUrl: posted.html_url, ...(terminal - ? { conclusion: "action_required" as const } + ? { conclusion } : {}), updatedAt: new Date(), }) diff --git a/server/src/services/chat-github-response-comments.ts b/server/src/services/chat-github-response-comments.ts index 88bb3c63fde..a3b97356b87 100644 --- a/server/src/services/chat-github-response-comments.ts +++ b/server/src/services/chat-github-response-comments.ts @@ -44,14 +44,16 @@ async function findResponseComment(source: Source, api: Api): Promise Promise, ) { + await assertCurrent(); const response = await stageResponse(db, source); // A delayed acknowledgement or progress retry must never replace a result. if (response.result?.id && (input.acknowledgement || (response.result.final === true && !input.final) || diff --git a/server/src/services/chat-github-review-policy.test.ts b/server/src/services/chat-github-review-policy.test.ts index 6c4b870b955..7fc6e0f9759 100644 --- a/server/src/services/chat-github-review-policy.test.ts +++ b/server/src/services/chat-github-review-policy.test.ts @@ -311,13 +311,14 @@ describe("GitHub score validation", () => { expect(() => validateGitHubReviewAssessment(invalid, context.headSha, defaultGitHubReviewPolicy())) .toThrow(expect.objectContaining({ status: 400, message: expect.stringContaining("coverage.limitations.0") })); }); - it("incomplete reviews never pass even at 5/5 or report-only", () => { + it("keeps report-only conclusions neutral while incomplete gated reviews require action", () => { expect( githubReviewConclusion({ ...assessment(), complete: false }, 5), ).toBe("action_required"); expect( githubReviewConclusion({ ...assessment(), complete: false }, null), - ).toBe("action_required"); + ).toBe("neutral"); + expect(githubReviewConclusion({ ...assessment(), score: 0 }, null)).toBe("neutral"); expect(githubReviewConclusion({ ...assessment(), score: 3 }, 5)).toBe( "failure", ); diff --git a/server/src/services/chat-github-review-policy.ts b/server/src/services/chat-github-review-policy.ts index b513666d9d2..dcc342f5822 100644 --- a/server/src/services/chat-github-review-policy.ts +++ b/server/src/services/chat-github-review-policy.ts @@ -230,8 +230,8 @@ export function githubReviewConclusion( assessment: GitHubReviewAssessment, threshold: GitHubReviewPolicy["ratingThreshold"], ): GitHubReviewConclusion { - if (!assessment.complete || assessment.score === 0) return "action_required"; if (threshold === null) return "neutral"; + if (!assessment.complete || assessment.score === 0) return "action_required"; return assessment.score >= threshold ? "success" : "failure"; } diff --git a/server/src/services/native-runtime/assigned-mcp-tools.ts b/server/src/services/native-runtime/assigned-mcp-tools.ts index 81a8bb8b6e5..d8817f0675f 100644 --- a/server/src/services/native-runtime/assigned-mcp-tools.ts +++ b/server/src/services/native-runtime/assigned-mcp-tools.ts @@ -14,7 +14,7 @@ const SCHEMA_CHUNK_CHARACTERS = 64 * 1024; const ON_DEMAND_TOOLS: ToolDefinition[] = [ { name: SEARCH_TOOL, - description: "Search your assigned app tools by name or description and read their input schemas. Use an empty query to browse. Pass nextOffset to fetch the next page. A large definition returns inputSchemaRef: set schemaTool to that name with an empty query, then pass nextSchemaOffset until null. Concatenate schemaJson chunks and parse JSON. Use paperclip_call_assigned_tool with a returned name and arguments matching its inputSchema.", + description: "Search your assigned app tools by name or description and read their input schemas. Pass {\"query\":\"search terms\"}, or an empty query to browse. Omit unused optional fields instead of sending null. To fetch the next page, pass the returned nextOffset as offset. A large definition returns inputSchemaRef: set schemaTool to that name with an empty query, then pass the returned nextSchemaOffset as schemaOffset until null. Concatenate schemaJson chunks and parse JSON. Use paperclip_call_assigned_tool with a returned name and arguments matching its inputSchema.", inputSchema: { type: "object", additionalProperties: false, properties: {