mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-04 11:01:16 +01:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - GitHub review tasks can retain a passive wait after a review is published. > - The wait check reads current requester, task, agent, repository, and publication bindings. > - It also transfers the complete assessment for one commit comparison. > - Large review summaries and findings add repeated database transfer. > - This pull request selects only the typed reviewed commit value. > - The wait keeps its existing checks and transfers less data. ## Linked Issues or Issue Description **What existing behavior does this improve?** The inherited GitHub review-wait check in the server. **Current behavior** The joined query selects the complete assessment JSON. The caller uses only `assessment.reviewedCommit`. **Proposed behavior** Select `assessment->'reviewedCommit'`. Keep its JSON type and the strict comparison with the head SHA. Keep all joins, filters, ordering, and publication checks. **Reason and benefit** The synthetic PostgreSQL 17 fixture reduces one result DataRow from 6,874 bytes to 423 bytes (93.85%). This measurement excludes response metadata, TLS, and TCP. It does not predict a monthly bill or deployed savings. **Breaking changes** None. No public API, stored data, schema, or execution policy changes. **Additional context** Related work: #17562 narrowed a different external-review candidate query. This PR changes the inherited published-review lookup. The duplicate search found no matching open change. ## What Changed - Select the commit as JSON, rather than text, to preserve strict comparison for legacy numeric, object, array, missing, and null values. - Add real PostgreSQL regressions for narrow responses, unchanged stored assessments, revocation, tenant and agent bindings, and a newer queued head. - Add a synthetic benchmark that captures the real baseline and treatment SQL, compares outputs, and measures raw DataRow bytes. - Document the read contract and benchmark command. ## Verification - Baseline: `663fe667d66d3863d0919a8724f1d99500feef0b`. - `vitest run server/src/__tests__/github-review-wait.test.ts`: 14 tests passed on disposable PostgreSQL 17.11. - Restoring the baseline query makes the response-size regression fail: 8,823 column-value bytes exceed the 1,000-byte bound. Restoring treatment passes. - Benchmark: `pnpm --filter @paperclipai/server exec tsx ../scripts/benchmark-github-review-wait.ts --baseline-ref=663fe667d66d3863d0919a8724f1d99500feef0b`. - The benchmark compares every unchanged SQL column and full service output. It also compares 15 JSON cases and live connection revocation. - Two warmups and seven alternating samples had overlapping latency ranges. No latency improvement is claimed. - `pnpm -r typecheck` and `pnpm build` passed locally. - Twenty existing published-review consumer regressions passed on PostgreSQL 17.11. These cover continued review, a newer head, publication state, and a connection-disable race at final commit. - Final-head focused tests passed again after the benchmark-only review fix. - Final benchmark treatment: `d94410a6e6bb83e727afdf09cff450a91a89edd6`, with an empty source diff. DataRow bytes remained 6,874 to 423. Warm medians were 3.11 ms before and 3.43 ms after, with overlapping ranges. This proves byte reduction, not a latency improvement. - A provenance check with one staged and one unstaged source edit confirmed that the benchmark records both from the documented server working directory. Those temporary edits were removed. - All final-head CI checks passed for `d94410a6e6bb83e727afdf09cff450a91a89edd6`. Greptile scored this exact commit 5/5. There are zero unresolved review threads or recommendations. The independent Paperclip review also scored this commit 5/5. - The first local embedded PostgreSQL attempt failed before assertions because the host exhausted System V semaphores. A direct `initdb` reproduced `could not create semaphores: No space left on device`. The passing focused proof used an owned disposable Docker PostgreSQL 17 container. `pnpm test:run` was not completed locally because of that host limit. The full final-head CI suite passed. - The final CI aggregate briefly reported `in_progress` after all of its steps and the workflow had completed successfully. One rerun of only that aggregate produced a terminal success. The test matrix was not rerun. - The initial frozen install hit the existing upstream patch/lock configuration mismatch. A cached non-frozen local install succeeded. The generated lock change is excluded from this PR. ## Risks The selected commit must retain its JSON type. Using `->>` could turn numeric `1` into text and accept a malformed assessment against head `1`. The regression covers this case. The complete assessment remains available for actual review and publication. Existing fresh authority checks remain in place. No migration is required. ## Model Used OpenAI GPT-6, accessed through Codex. The exact runtime model ID and context-window size were not exposed in this session. Used repository inspection, code execution, and PostgreSQL validation. ## 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>
98 lines
6.0 KiB
TypeScript
98 lines
6.0 KiB
TypeScript
import { createHash } from "node:crypto";
|
|
import { HttpError } from "../../../errors.js";
|
|
import { chatRunSourceMessageId } from "../run-origin.js";
|
|
import { and, desc, eq, or, sql } from "drizzle-orm";
|
|
import { chatActions, chatConversations, chatEndpointResources, chatEndpoints, chatGitHubReviews, chatMessageLinks, heartbeatRuns, toolConnections, type Db } from "@paperclipai/db";
|
|
|
|
export async function hasPublishedGitHubReview(db: Db, binding: {
|
|
companyId: string;
|
|
issueId: string;
|
|
runId: string;
|
|
}): Promise<boolean> {
|
|
const reviews = await db.select({ id: chatGitHubReviews.id, endpointId: chatGitHubReviews.endpointId })
|
|
.from(chatGitHubReviews)
|
|
.where(and(
|
|
eq(chatGitHubReviews.companyId, binding.companyId),
|
|
eq(chatGitHubReviews.issueId, binding.issueId),
|
|
eq(chatGitHubReviews.runId, binding.runId),
|
|
eq(chatGitHubReviews.state, "completed"),
|
|
));
|
|
if (!reviews.length) return false;
|
|
const [publication] = await db.select({ id: chatActions.id }).from(chatActions).where(and(
|
|
eq(chatActions.companyId, binding.companyId),
|
|
or(...reviews.map(review => and(
|
|
eq(chatActions.endpointId, review.endpointId),
|
|
eq(chatActions.providerActionId, `github_publication:${binding.issueId}:assessment:${createHash("sha256").update(JSON.stringify(review.id)).digest("hex")}`),
|
|
))),
|
|
eq(chatActions.kind, "github_review_publication"),
|
|
eq(chatActions.status, "processed"),
|
|
)).limit(1);
|
|
return !!publication;
|
|
}
|
|
|
|
type WaitRun = Pick<typeof heartbeatRuns.$inferSelect,
|
|
"id" | "companyId" | "agentId" | "responsibleUserId" | "activeIdentityContextId" | "nativeIssueId" | "issueId" | "contextSnapshot">;
|
|
|
|
/** Passive review waits inherit the same accepted requester lineage as GitHub
|
|
* tools. This is not a grant to publish, run a monitor, or reuse attachments. */
|
|
export async function readInheritedGitHubReviewWait(db: Db, run: WaitRun) {
|
|
if (!run.activeIdentityContextId || !run.nativeIssueId) return null;
|
|
let commentId: string | null;
|
|
try {
|
|
commentId = await chatRunSourceMessageId(db, run, { provider: "github" });
|
|
} catch (error) {
|
|
if (error instanceof HttpError && error.status === 403) return { kind: "revoked" as const };
|
|
throw error;
|
|
}
|
|
if (!commentId) return null;
|
|
const [review] = await db.select({
|
|
id: chatGitHubReviews.id, endpointId: chatGitHubReviews.endpointId,
|
|
state: chatGitHubReviews.state, repository: chatGitHubReviews.repository,
|
|
repositoryId: chatGitHubReviews.repositoryId, headSha: chatGitHubReviews.headSha,
|
|
reviewedCommit: sql<unknown>`${chatGitHubReviews.assessment}->'reviewedCommit'`, reviewRunId: chatGitHubReviews.runId,
|
|
connectionEnabled: toolConnections.enabled, connectionStatus: toolConnections.status,
|
|
reviewAgentId: heartbeatRuns.agentId, reviewCompanyId: heartbeatRuns.companyId,
|
|
resourceRepository: chatEndpointResources.providerResourceId,
|
|
resourceRepositoryId: sql<string | null>`${chatEndpointResources.metadata}->>'providerRepositoryId'`,
|
|
pullNumber: chatGitHubReviews.pullNumber, thread: chatConversations.externalThreadId,
|
|
}).from(chatMessageLinks)
|
|
.innerJoin(chatConversations, and(eq(chatConversations.id, chatMessageLinks.conversationId),
|
|
eq(chatConversations.companyId, run.companyId), eq(chatConversations.issueId, run.nativeIssueId)))
|
|
.innerJoin(chatEndpoints, and(eq(chatEndpoints.id, chatConversations.endpointId),
|
|
eq(chatEndpoints.companyId, run.companyId), eq(chatEndpoints.provider, "github"),
|
|
eq(chatEndpoints.assignedAgentId, run.agentId)))
|
|
.leftJoin(toolConnections, and(eq(toolConnections.id, chatEndpoints.connectionId),
|
|
eq(toolConnections.companyId, run.companyId)))
|
|
.innerJoin(chatEndpointResources, and(eq(chatEndpointResources.id, chatConversations.resourceId),
|
|
eq(chatEndpointResources.companyId, run.companyId), eq(chatEndpointResources.endpointId, chatEndpoints.id)))
|
|
.innerJoin(chatGitHubReviews, and(eq(chatGitHubReviews.endpointId, chatEndpoints.id),
|
|
eq(chatGitHubReviews.companyId, run.companyId), eq(chatGitHubReviews.issueId, run.nativeIssueId)))
|
|
.leftJoin(heartbeatRuns, eq(heartbeatRuns.id, chatGitHubReviews.runId))
|
|
.where(and(eq(chatMessageLinks.companyId, run.companyId), eq(chatMessageLinks.commentId, commentId),
|
|
eq(chatMessageLinks.direction, "inbound")))
|
|
.orderBy(desc(chatGitHubReviews.createdAt), desc(chatGitHubReviews.id)).limit(1);
|
|
// A newer unreviewed head cannot inherit the prior head's publication.
|
|
if (!review) return null;
|
|
if (!review.connectionEnabled || review.connectionStatus !== "active" || review.state !== "completed" ||
|
|
review.reviewCompanyId !== run.companyId || review.reviewAgentId !== run.agentId ||
|
|
review.reviewedCommit !== review.headSha ||
|
|
review.resourceRepository.toLowerCase() !== review.repository.toLowerCase() ||
|
|
review.resourceRepositoryId !== review.repositoryId ||
|
|
review.thread.toLowerCase() !== `github:${review.repository}:${review.pullNumber}`.toLowerCase()) return { kind: "revoked" as const };
|
|
const [publication] = await db.select({ id: chatActions.id }).from(chatActions).where(and(
|
|
eq(chatActions.companyId, run.companyId), eq(chatActions.endpointId, review.endpointId),
|
|
eq(chatActions.kind, "github_review_publication"), eq(chatActions.status, "processed"),
|
|
eq(chatActions.providerActionId, `github_publication:${run.nativeIssueId}:assessment:${createHash("sha256").update(JSON.stringify(review.id)).digest("hex")}`),
|
|
)).limit(1);
|
|
if (!publication) return { kind: "revoked" as const };
|
|
// The caller must still verify live task ownership, destination and principal
|
|
// policy. This normalized source is used only by the passive-wait boundary.
|
|
return { kind: "published" as const, endpointId: review.endpointId, reviewId: review.id, reviewRunId: review.reviewRunId,
|
|
headSha: review.headSha, context: {
|
|
source: "chat:github", paperclipHarnessCheckedOut: true,
|
|
wakeCommentId: commentId, wakeCommentIds: [commentId],
|
|
paperclipWake: { checkedOutByHarness: true, externalChatProvider: "github",
|
|
issue: { id: run.nativeIssueId }, commentIds: [commentId] },
|
|
} };
|
|
}
|