Files
PaperClipAI/server/src/services/chat-github-checks.ts
T
DottaandPaperclip a8b652b085 fix(github): preserve review delivery through reconnect (#16259)
## 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 <noreply@paperclip.ing>
2026-10-13 22:24:41 -05:00

470 lines
17 KiB
TypeScript

import {
GitHubPublicationLeaseLost,
withGitHubPublicationLease,
} from "./chat-github-publication-lease.js";
import { and, desc, eq, inArray, isNull, or, sql } from "drizzle-orm";
import {
chatActions,
chatDeliveries,
chatEndpoints,
chatEndpointResources,
chatGitHubConfigurations,
chatGitHubReviews,
chatMessageLinks,
companies,
heartbeatRuns,
type Db,
} from "@paperclipai/db";
import type { GitHubReviewEventContext } from "@paperclipai/shared";
import { runtimePublicOrigin } from "./cloud-runtime-identity.js";
import {
githubBotRepositoryToken,
githubBotRequest,
} from "./chat-github-client.js";
/** Check status follows the ordinary chat delivery/task/run. This outbox never
* schedules an agent. Assessments are published by the governed review tool. */
export function githubReviewCheckService(db: Db, fetchImpl = fetch) {
async function enqueue(
endpoint: typeof chatEndpoints.$inferSelect,
event: GitHubReviewEventContext,
admitted: boolean,
reason: string,
) {
const [config] = await db
.select()
.from(chatGitHubConfigurations)
.where(eq(chatGitHubConfigurations.endpointId, endpoint.id));
if (!config?.configuration.toolsEnabled) return;
const policy = {
...config.configuration.defaults,
...config.configuration.repositories[event.repositoryId],
};
if (policy.ratingThreshold === null && !admitted) return;
await db
.insert(chatActions)
.values({
companyId: endpoint.companyId,
endpointId: endpoint.id,
kind: "github_review_check",
providerActionId: `github-check:${event.deliveryId}`,
payload: {
version: 1,
event,
admitted,
reason,
runtimeGeneration:
(endpoint.setup as { runtimeGeneration?: number })
.runtimeGeneration ?? 0,
},
status: "received",
})
.onConflictDoNothing();
}
async function dispatch(id: string) {
const [action] = await db
.select()
.from(chatActions)
.where(
and(
eq(chatActions.id, id),
eq(chatActions.kind, "github_review_check"),
),
)
.limit(1);
if (!action || ["processed", "cancelled"].includes(action.status)) return;
const event = action.payload.event as GitHubReviewEventContext;
await withGitHubPublicationLease(
db,
{
companyId: action.companyId,
endpointId: action.endpointId,
repositoryId: event.repositoryId,
number: event.pullNumber,
},
fetchImpl,
async (lease) => {
const [fresh] = await db
.select()
.from(chatActions)
.where(eq(chatActions.id, id));
if (!fresh || ["processed", "cancelled"].includes(fresh.status)) return;
let attempts = Number(fresh.result?.attempts ?? 0);
try {
const [source] = await db
.select({
endpoint: chatEndpoints,
resource: chatEndpointResources,
configuration: chatGitHubConfigurations.configuration,
companyPrefix: companies.issuePrefix,
})
.from(chatEndpoints)
.innerJoin(companies, eq(companies.id, chatEndpoints.companyId))
.innerJoin(
chatGitHubConfigurations,
eq(chatGitHubConfigurations.endpointId, chatEndpoints.id),
)
.innerJoin(
chatEndpointResources,
eq(chatEndpointResources.endpointId, chatEndpoints.id),
)
.where(
and(
eq(chatEndpoints.id, action.endpointId),
eq(chatEndpoints.companyId, action.companyId),
eq(chatEndpointResources.providerResourceId, event.repository),
),
);
if (
!source ||
!source.configuration.toolsEnabled ||
!["active", "verifying"].includes(source.endpoint.status) ||
!source.resource.enabled ||
source.resource.availability !== "available" ||
String(source.resource.metadata?.providerRepositoryId) !==
event.repositoryId ||
((source.endpoint.setup as { runtimeGeneration?: number })
.runtimeGeneration ?? 0) !== action.payload.runtimeGeneration
) {
await lease.commit(async (tx) => {
await tx
.update(chatActions)
.set({
status: "cancelled",
result: { code: "connection_changed" },
updatedAt: new Date(),
})
.where(eq(chatActions.id, id));
});
return;
}
const [review] = await db
.select()
.from(chatGitHubReviews)
.where(
and(
eq(chatGitHubReviews.endpointId, action.endpointId),
eq(chatGitHubReviews.repositoryId, event.repositoryId),
eq(chatGitHubReviews.pullNumber, event.pullNumber),
eq(chatGitHubReviews.headSha, event.headSha),
),
)
.orderBy(desc(chatGitHubReviews.createdAt))
.limit(1);
if (review?.assessment) {
// The assessment outbox owns the terminal score and its retries.
await lease.commit(async (tx) => {
await tx
.update(chatActions)
.set({
status: "processed",
result: {
code: "assessment_owns_check",
reviewId: review.id,
},
updatedAt: new Date(),
})
.where(eq(chatActions.id, id));
});
return;
}
let run = review?.runId
? (
await db
.select()
.from(heartbeatRuns)
.where(eq(heartbeatRuns.id, review.runId))
)[0]
: undefined;
if (review && !run) {
const [sourceRun] = await db
.select({ run: heartbeatRuns })
.from(chatDeliveries)
.innerJoin(
chatMessageLinks,
eq(chatMessageLinks.deliveryId, chatDeliveries.id),
)
.innerJoin(
heartbeatRuns,
and(
eq(heartbeatRuns.companyId, action.companyId),
eq(heartbeatRuns.agentId, source.endpoint.assignedAgentId),
sql`${heartbeatRuns.contextSnapshot}->>'wakeCommentId' = ${chatMessageLinks.commentId}::text`,
),
)
.where(
and(
eq(chatDeliveries.endpointId, action.endpointId),
sql`${chatDeliveries.normalizedEvent}->'githubAutomatic'->'context'->>'deliveryId' = ${event.deliveryId}`,
),
)
.orderBy(desc(heartbeatRuns.createdAt))
.limit(1);
run = sourceRun?.run;
}
const [delivery] = await db
.select({ state: chatDeliveries.state })
.from(chatDeliveries)
.where(
and(
eq(chatDeliveries.endpointId, action.endpointId),
sql`${chatDeliveries.normalizedEvent}->'githubAutomatic'->'context'->>'deliveryId' = ${event.deliveryId}`,
),
)
.limit(1);
const admissionEnded =
!review &&
(delivery?.state === "filtered" || delivery?.state === "failed");
const terminal =
!!run &&
[
"succeeded",
"failed",
"cancelled",
"timed_out",
"interrupted",
].includes(run.status);
const state =
(!action.payload.admitted && !review) || admissionEnded
? "manual_required"
: terminal
? run!.status === "succeeded"
? "incomplete"
: "error"
: run?.status === "running"
? "running"
: "queued";
const status =
state === "running"
? "in_progress"
: 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"
: state === "incomplete"
? "Agent finished without a complete assessment"
: state === "error"
? "Review execution did not complete"
: state === "running"
? "Agent is reviewing this commit"
: "Waiting for the assigned agent";
const token = await githubBotRepositoryToken(
db,
action.companyId,
action.endpointId,
event.repositoryId,
lease.fetch,
);
const prefix = `/repos/${event.repository.split("/").map(encodeURIComponent).join("/")}`;
const request = <T>(
path: string,
options?: Parameters<typeof githubBotRequest>[3],
) =>
githubBotRequest<T>(
lease.fetch,
token,
`${prefix}${path}`,
options,
);
const current = await request<{ head: { sha: string } }>(
`/pulls/${event.pullNumber}`,
);
if (current.head.sha !== event.headSha) {
if (review)
await lease.commit(async (tx) => {
await tx
.update(chatGitHubReviews)
.set({ state: "superseded", updatedAt: new Date() })
.where(
and(
eq(chatGitHubReviews.id, review.id),
isNull(chatGitHubReviews.assessment),
),
);
});
await lease.commit(async (tx) => {
await tx
.update(chatActions)
.set({
status: "cancelled",
result: { code: "stale_head" },
updatedAt: new Date(),
})
.where(eq(chatActions.id, id));
});
return;
}
if (fresh.result?.state !== state) {
const origin = runtimePublicOrigin();
const detailsPath = review
? `issues/${review.issueId}`
: `apps/chat/${source.endpoint.id}/reviews`;
const detailsUrl = origin
? `${origin}/${encodeURIComponent(source.companyPrefix)}/${detailsPath}`
: null;
const externalId = `${action.endpointId}:${event.pullNumber}:${event.headSha}`;
const checks = await request<{
check_runs: Array<{
id: number;
status?: string;
external_id?: string;
app?: { id?: number };
}>;
}>(
`/commits/${event.headSha}/check-runs?check_name=Paperclip%20Review&per_page=100`,
);
const check = checks.check_runs.find(
(check) =>
check.external_id === externalId &&
String(check.app?.id) === source.endpoint.botExternalId,
);
// GitHub retains a completed check's conclusion when PATCHed back to
// queued/in_progress. A fresh attempt needs a new check run with the
// same stable name; otherwise a prior success still looks passing.
const updateCheck =
check &&
!(check.status === "completed" && status !== "completed");
const posted = await request<{ id: number; html_url: string }>(
updateCheck ? `/check-runs/${check.id}` : "/check-runs",
{
method: updateCheck ? "PATCH" : "POST",
body: {
name: "Paperclip Review",
head_sha: event.headSha,
external_id: externalId,
...(detailsUrl ? { details_url: detailsUrl } : {}),
status,
...(status === "completed"
? { conclusion }
: {}),
output: {
title,
summary: `${title}. ${state === "manual_required" ? "An authorized person can mention the bot to request a review of this head. Automatic review was not authorized by the bot configuration." : "This check follows the assigned Paperclip agent's task execution. Only a validated assessment can produce a passing score."}\n\nCommit: ${event.headSha}`,
},
},
},
);
if (review)
await lease.commit(async (tx) => {
await tx
.update(chatGitHubReviews)
.set({
state,
runId: run?.id ?? review.runId,
checkId: String(posted.id),
checkUrl: posted.html_url,
...(terminal
? { conclusion }
: {}),
updatedAt: new Date(),
})
.where(
and(
eq(chatGitHubReviews.id, review.id),
isNull(chatGitHubReviews.assessment),
),
);
});
}
await lease.commit(async (tx) => {
await tx
.update(chatActions)
.set({
status: status === "completed" ? "processed" : "received",
result: {
state,
attempts: 0,
retryable: true,
retryAt: new Date(Date.now() + 5000).toISOString(),
},
updatedAt: new Date(),
})
.where(eq(chatActions.id, id));
});
} catch (error) {
if (error instanceof GitHubPublicationLeaseLost) return;
attempts++;
await lease.commit(async (tx) => {
await tx
.update(chatActions)
.set({
status: "failed",
result: {
attempts,
retryable: attempts < 8,
retryAt: new Date(
Date.now() + Math.min(300000, 1000 * 2 ** attempts),
).toISOString(),
code: "check_publication_failed",
},
updatedAt: new Date(),
})
.where(eq(chatActions.id, id));
});
}
},
);
}
async function processPending(limit = 10) {
// Check delivery and task admission run independently. A rapid push can
// cancel an obsolete check before admission inserts its review projection.
// Reconcile that late row from the durable cancellation receipt, without
// starting work or borrowing another delivery's authority.
await db
.update(chatGitHubReviews)
.set({
state: "superseded",
updatedAt: new Date(),
})
.where(
and(
inArray(chatGitHubReviews.state, ["queued", "running"]),
isNull(chatGitHubReviews.assessment),
sql`exists (
select 1 from ${chatActions} action
join ${chatDeliveries} delivery
on delivery.id::text = ${chatGitHubReviews.deliveryId}
where action.company_id = ${chatGitHubReviews.companyId}
and action.endpoint_id = ${chatGitHubReviews.endpointId}
and delivery.endpoint_id = action.endpoint_id
and action.kind = 'github_review_check'
and action.status = 'cancelled'
and action.result->>'code' = 'stale_head'
and action.payload->'event'->>'deliveryId' =
delivery.normalized_event->'githubAutomatic'->'context'->>'deliveryId'
)`,
),
);
const rows = await db
.select({ id: chatActions.id })
.from(chatActions)
.where(
and(
eq(chatActions.kind, "github_review_check"),
or(
and(
eq(chatActions.status, "received"),
sql`(${chatActions.result}->>'retryAt' is null or (${chatActions.result}->>'retryAt')::timestamptz <= now())`,
),
and(
eq(chatActions.status, "failed"),
sql`${chatActions.result}->>'retryable' = 'true'`,
sql`(${chatActions.result}->>'retryAt')::timestamptz <= now()`,
),
),
),
)
.orderBy(chatActions.updatedAt)
.limit(limit);
for (const row of rows) await dispatch(row.id);
}
return { enqueue, processPending };
}