Files
PaperClipAI/server/src/__tests__/github-review-wait.test.ts
T
DottaandPaperclip 2cbfc35a07 perf(github): reuse the inherited review wait query template (#18965)
## Thinking Path

> - Paperclip is the open source app people use to manage AI agents for
work.
> - External review waits check current GitHub review and publication
bindings.
> - The GitHub service rebuilds the same seven-table Drizzle query on
each check.
> - A sampled production window attributed 6.3% of SQL serialization CPU
to this query shape.
> - This pull request prepares that query once per exact database or
transaction object.
> - Each call still reads current provenance, review, connection and
publication state.
> - The local service benchmark reduces median client CPU per read by
24.35%.

## Linked Issues or Issue Description

**What existing behavior does this improve?**

Repeated `readInheritedGitHubReviewWait` calls from external review wait
reads and attention retention.

**Subsystem affected**

Server GitHub chat review waits.

**Current behavior**

Every call constructs and serializes the same joined SELECT. The
database driver already supports prepared statements. That setting does
not reuse the JavaScript query template.

**Proposed behavior**

Compile the existing selector once for each supplied database,
transaction or savepoint object. Bind company, issue, agent and comment
IDs on each execution. Keep every current database read and check.

**Reason and benefit**

Remove repeated query construction and serialization CPU. Do not cache
rows or authorization decisions. Exact open-PR searches found no
duplicate query-template change.

## What Changed

- Add an exact-handle `WeakMap` for the GitHub review selector and
execute it with fresh placeholders.
- Preserve the SQL projection, joins, JSON types, latest-review order
and all current authority checks.
- Extend real PostgreSQL tests for new bindings, warm-cache revocation,
latest heads and transaction/savepoint rollback.
- Capture prepared executions in the existing test helper.
- Add an actual-service benchmark and a report with every timing sample
and setup failure.

## Verification

- `github-review-wait.test.ts`: 16 tests passed on real PostgreSQL.
- `external-chat-wait.integration.test.ts -t 'reads small current
external review waits without saved prompts'`: four affected cases
passed; 269 unrelated cases skipped.
- Pinned baseline: `e54f9fbf828e06e385e0c62dc381ccb46190aa30`. Final
source SHA256:
`fe647e7fe2aed82609e8f1d66a0109e7a10ee28313ae2ef218ba3ab39b5cd105`.
- Exclusive-window benchmark: 500 warmups per version, nine alternating
pairs of 100 complete service reads on disposable PostgreSQL. Median
client CPU: 1.00336 → 0.75908 ms/read (24.35% less). Median wall time:
2.87089 → 2.38731 ms/read (16.84% less). CPU excludes the PostgreSQL
process.
- Every generated SQL statement, parameter, query count and complete
result matched. Warm joined-selector compilations per ten service reads:
10 → 0.
- See `doc/performance/2026-11-04-prepared-github-review-wait.md` for
reproduction, all samples and measurement limits.
- Setup record: loopback required execution outside the sandbox. The
fresh embedded-Postgres dependency lacked macOS dylib aliases; aliases
to existing identical bytes restored the fixture. The SDK import needed
a current workspace SDK/shared build. A broad external-chat suite run
was interrupted and replaced by the affected test filter. Those attempts
are not counted as passing suites.
- Greptile: 5/5 on current head
`d2e6ae8814c3a56c380bd5d60c47e6f98642f085`, with no actionable issues or
inline comments.
- Current-head CI is green at
`d2e6ae8814c3a56c380bd5d60c47e6f98642f085`: 52 successful checks, three
intentional skips and the successful Snyk status. This includes
typecheck, build, the full test matrix, end-to-end shards and the
aggregate gate.
- Combined local validation at `4ab58ff70b` includes both measured
production templates and final test fixes. Full recursive `pnpm -r
typecheck` passed in 39.864 seconds. Three real PostgreSQL files passed
35/35 tests (five chat cases, 16 GitHub review cases and 14 shared
prepared-query cases). The combined build passed earlier with the same
production template bytes.
- The initial combined typecheck failed on a separate chat test typing
error and build setup. The later passing run resolves that attempt. The
original full local `pnpm test:run` was deliberately interrupted after
six chat shards passed and partial general tests ran; it is not a
passing full local suite. Each PR has independent current-head CI for
broad coverage. The initial loopback sandbox restriction was resolved by
an escalated run of the same source.
- Local benchmark/test dependencies were locked at the baseline; the
rebase adds the upstream lockfile refresh and UI paste fix only.

## Risks

- A prepared query captures its session. The exact supplied handle is
the cache key so transactions and savepoints do not borrow the root
session.
- Placeholder mistakes could cross bindings. Tests alternate independent
company/issue/agent/comment fixtures on the same handle.
- The benchmark has one synthetic eligible fixture with warm sequential
reads. It does not predict production endpoint latency or whole-server
CPU savings.
- No schema, index, database driver setting or public API changes.

## Model Used

OpenAI `gpt-6.1-sol` through Codex. The context-window size is not
exposed by the session. Used reasoning, tool calls, source editing and
code execution. Independent Codex review found no correctness issues.

## 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-11-04 08:29:59 -06:00

126 lines
8.6 KiB
TypeScript

import { randomUUID } from "node:crypto";
import { eq } from "drizzle-orm";
import { afterAll, beforeAll, describe, expect, it } from "vitest";
import { agents, chatActions, chatConversations, chatEndpointResources, chatEndpoints, chatGitHubReviews,
companies, createDb, heartbeatRuns, runIdentityContexts, toolConnections, type Db } from "@paperclipai/db";
import { startEmbeddedPostgresTestDatabase } from "./helpers/embedded-postgres.js";
import { captureReviewWaitQueries, seedGitHubReviewWait, setGitHubReviewAssessment,
type CapturedReviewWaitQuery } from "./helpers/github-review-wait.js";
import { readInheritedGitHubReviewWait } from "../services/chat-channels/github/review-wait.js";
describe("inherited GitHub review wait projection", () => {
let temporary: Awaited<ReturnType<typeof startEmbeddedPostgresTestDatabase>>;
let db: Db;
beforeAll(async () => {
if (process.env.PAPERCLIP_TEST_DATABASE_URL) {
db = createDb(process.env.PAPERCLIP_TEST_DATABASE_URL);
} else {
temporary = await startEmbeddedPostgresTestDatabase("github-review-wait-");
db = createDb(temporary.connectionString);
}
}, 30_000);
afterAll(async () => { await db?.$client.end({ timeout: 0 }); await temporary?.cleanup(); });
it("reads the typed commit without transferring the assessment document", async () => {
const f = await seedGitHubReviewWait(db);
const assessment = { reviewedCommit: f.headSha, summary: "Large summary ".repeat(600), findings: [{ body: "retained" }] };
await setGitHubReviewAssessment(db, f.reviewId, assessment);
const queries: CapturedReviewWaitQuery[] = [];
expect(await readInheritedGitHubReviewWait(captureReviewWaitQueries(db, queries), f.run)).toMatchObject({
kind: "published", reviewId: f.reviewId, headSha: f.headSha, context: { wakeCommentId: f.commentId },
});
const query = queries.find(query => query.sql.includes('join "chat_github_reviews"'))!;
const rows = await db.$client.unsafe(query.sql, query.params as never[]).raw();
expect(rows).toHaveLength(1);
expect(rows[0]!.reduce((bytes, value) => bytes + (value?.byteLength ?? 0), 0)).toBeLessThan(1_000);
const [stored] = await db.select({ assessment: chatGitHubReviews.assessment }).from(chatGitHubReviews)
.where(eq(chatGitHubReviews.id, f.reviewId));
expect(stored!.assessment).toEqual(assessment);
});
it("preserves strict equality for malformed, missing, and valid JSON values", async () => {
const f = await seedGitHubReviewWait(db);
for (const assessment of [null, false, 123, f.headSha, [], [f.headSha], {}, { reviewedCommit: null },
{ reviewedCommit: 1 }, { reviewedCommit: true }, { reviewedCommit: [f.headSha] },
{ reviewedCommit: { value: f.headSha } }, { reviewedCommit: ` ${f.headSha}` },
{ reviewedCommit: "2".repeat(40) }, { reviewedCommit: f.headSha }]) {
await setGitHubReviewAssessment(db, f.reviewId, assessment);
const expected = (assessment as { reviewedCommit?: unknown } | null)?.reviewedCommit === f.headSha;
expect((await readInheritedGitHubReviewWait(db, f.run))?.kind).toBe(expected ? "published" : "revoked");
}
await db.$client.unsafe("UPDATE chat_github_reviews SET head_sha = '1', assessment = '{\"reviewedCommit\":1}' WHERE id = $1", [f.reviewId]);
expect(await readInheritedGitHubReviewWait(db, f.run)).toEqual({ kind: "revoked" });
await db.$client.unsafe("UPDATE chat_github_reviews SET assessment = NULL WHERE id = $1", [f.reviewId]);
expect(await readInheritedGitHubReviewWait(db, f.run)).toEqual({ kind: "revoked" });
});
it.each(["disabled", "inactive", "review_state", "review_agent", "repository", "repository_id", "thread", "publication", "identity", "endpoint_agent", "company"] as const)(
"continues to reject a changed %s binding", async change => {
const f = await seedGitHubReviewWait(db);
expect((await readInheritedGitHubReviewWait(db, f.run))?.kind).toBe("published");
if (change === "disabled") await db.update(toolConnections).set({ enabled: false }).where(eq(toolConnections.id, f.connectionId));
if (change === "inactive") await db.update(toolConnections).set({ status: "disabled" }).where(eq(toolConnections.id, f.connectionId));
if (change === "review_state") await db.update(chatGitHubReviews).set({ state: "queued" }).where(eq(chatGitHubReviews.id, f.reviewId));
if (change === "review_agent" || change === "endpoint_agent") {
const [other] = await db.insert(agents).values({ companyId: f.companyId, name: "Other", adapterType: "process" }).returning();
if (change === "review_agent") await db.update(heartbeatRuns).set({ agentId: other!.id }).where(eq(heartbeatRuns.id, f.reviewRunId));
else await db.update(chatEndpoints).set({ assignedAgentId: other!.id }).where(eq(chatEndpoints.id, f.endpointId));
}
if (change === "repository") await db.update(chatEndpointResources).set({ providerResourceId: "other/repository" }).where(eq(chatEndpointResources.id, f.resourceId));
if (change === "repository_id") await db.update(chatEndpointResources).set({ metadata: { providerRepositoryId: "43" } }).where(eq(chatEndpointResources.id, f.resourceId));
if (change === "thread") await db.update(chatConversations).set({ externalThreadId: "github:synthetic/repository:8" }).where(eq(chatConversations.id, f.conversationId));
if (change === "publication") await db.update(chatActions).set({ status: "received" }).where(eq(chatActions.id, f.publicationId));
if (change === "identity") await db.update(runIdentityContexts).set({ status: "rejected" }).where(eq(runIdentityContexts.id, f.activeIdentityContextId!));
if (change === "company") {
const otherCompanyId = randomUUID();
await db.insert(companies).values({ id: otherCompanyId, name: "Other", issuePrefix: otherCompanyId });
f.run = { ...f.run, companyId: otherCompanyId };
}
expect((await readInheritedGitHubReviewWait(db, f.run))?.kind).not.toBe("published");
},
);
it("binds each call afresh while reusing the compiled query", async () => {
const first = await seedGitHubReviewWait(db), second = await seedGitHubReviewWait(db);
const queries: CapturedReviewWaitQuery[] = [];
const captured = captureReviewWaitQueries(db, queries);
for (const f of [first, second, first]) {
expect(await readInheritedGitHubReviewWait(captured, f.run)).toMatchObject({ reviewId: f.reviewId });
}
const joined = queries.filter(query => query.sql.includes('join "chat_github_reviews"'));
expect(joined).toHaveLength(3);
expect(joined[0]).toEqual(joined[2]);
expect(joined[0]!.sql).toBe(joined[1]!.sql);
expect(joined[0]!.params).not.toEqual(joined[1]!.params);
});
it("keeps transaction and savepoint reads on their own sessions", async () => {
const f = await seedGitHubReviewWait(db);
expect((await readInheritedGitHubReviewWait(db, f.run))?.kind).toBe("published");
await expect(db.transaction(async tx => {
const scoped = tx as unknown as Db;
await tx.update(toolConnections).set({ enabled: false }).where(eq(toolConnections.id, f.connectionId));
expect(await readInheritedGitHubReviewWait(scoped, f.run)).toEqual({ kind: "revoked" });
await expect(tx.transaction(async savepoint => {
await savepoint.update(toolConnections).set({ enabled: true }).where(eq(toolConnections.id, f.connectionId));
expect((await readInheritedGitHubReviewWait(savepoint as unknown as Db, f.run))?.kind).toBe("published");
throw new Error("rollback savepoint");
})).rejects.toThrow("rollback savepoint");
expect(await readInheritedGitHubReviewWait(scoped, f.run)).toEqual({ kind: "revoked" });
throw new Error("rollback transaction");
})).rejects.toThrow("rollback transaction");
expect((await readInheritedGitHubReviewWait(db, f.run))?.kind).toBe("published");
});
it("does not reuse an older completed review when a newer head is queued", async () => {
const f = await seedGitHubReviewWait(db);
expect((await readInheritedGitHubReviewWait(db, f.run))?.kind).toBe("published");
const [review] = await db.select().from(chatGitHubReviews).where(eq(chatGitHubReviews.id, f.reviewId));
await db.insert(chatGitHubReviews).values({ ...review!, id: randomUUID(), deliveryId: randomUUID(),
headSha: "2".repeat(40), state: "queued", createdAt: new Date(review!.createdAt.getTime() + 1_000) });
expect(await readInheritedGitHubReviewWait(db, f.run)).toEqual({ kind: "revoked" });
expect(await readInheritedGitHubReviewWait(db, { ...f.run, nativeIssueId: null })).toBeNull();
expect(await readInheritedGitHubReviewWait(db, { ...f.run, activeIdentityContextId: null })).toBeNull();
});
});