mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-29 04:25:52 +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>
93 lines
6.7 KiB
TypeScript
93 lines
6.7 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: "error" }).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("does not reuse an older completed review when a newer head is queued", async () => {
|
|
const f = await seedGitHubReviewWait(db);
|
|
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();
|
|
});
|
|
});
|