mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-28 12:04:41 +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>
129 lines
8.3 KiB
TypeScript
129 lines
8.3 KiB
TypeScript
import assert from "node:assert/strict";
|
|
import { execFileSync } from "node:child_process";
|
|
import { randomBytes } from "node:crypto";
|
|
import { promises as fs } from "node:fs";
|
|
import path from "node:path";
|
|
import { fileURLToPath, pathToFileURL } from "node:url";
|
|
import { applyPendingMigrations, createDb, startEmbeddedPostgresTestDatabase } from "../packages/db/src/index.js";
|
|
import { readInheritedGitHubReviewWait } from "../server/src/services/chat-channels/github/review-wait.js";
|
|
import { captureReviewWaitQueries, seedGitHubReviewWait, setGitHubReviewAssessment,
|
|
revokeReviewWaitConnection, type CapturedReviewWaitQuery } from "../server/src/__tests__/helpers/github-review-wait.js";
|
|
|
|
const baselineRef = process.argv.find(arg => arg.startsWith("--baseline-ref="))?.slice(15);
|
|
assert.ok(baselineRef, "Pass an explicit pre-fix --baseline-ref=<commit>");
|
|
const baselineRevision = execFileSync("git", ["rev-parse", "--verify", `${baselineRef}^{commit}`], { encoding: "utf8" }).trim();
|
|
const treatmentRevision = execFileSync("git", ["rev-parse", "HEAD"], { encoding: "utf8" }).trim();
|
|
const modulePath = "server/src/services/chat-channels/github/review-wait.ts";
|
|
const helperPath = "server/src/services/chat-channels/run-origin.ts";
|
|
const root = fileURLToPath(new URL("../", import.meta.url));
|
|
assert.equal(execFileSync("git", ["show", `${baselineRevision}:${helperPath}`], { encoding: "utf8" }),
|
|
await fs.readFile(path.join(root, helperPath), "utf8"), "Baseline provenance helper must be unchanged");
|
|
const keepAlive = setInterval(() => {}, 1_000);
|
|
let temporary: Awaited<ReturnType<typeof startEmbeddedPostgresTestDatabase>> | undefined;
|
|
let db: ReturnType<typeof createDb> | undefined;
|
|
let baselineDir: string | undefined;
|
|
try {
|
|
const oldSource = execFileSync("git", ["show", `${baselineRevision}:${modulePath}`], { encoding: "utf8" });
|
|
const moduleUrl = new URL(`../${modulePath}`, import.meta.url);
|
|
baselineDir = await fs.mkdtemp(path.join(root, "server/scripts/.review-wait-baseline-"));
|
|
const baselineFile = path.join(baselineDir, "review-wait.ts");
|
|
await fs.writeFile(baselineFile, oldSource.replace(/from "(\.{1,2}\/[^\"]+)"/g,
|
|
(_match, specifier: string) => `from ${JSON.stringify(fileURLToPath(new URL(specifier, moduleUrl)))}`));
|
|
const baseline = await import(pathToFileURL(baselineFile).href) as typeof import("../server/src/services/chat-channels/github/review-wait.js");
|
|
const externalUrl = process.env.PAPERCLIP_TEST_DATABASE_URL;
|
|
if (externalUrl) {
|
|
const parsed = new URL(externalUrl);
|
|
assert.ok(["127.0.0.1", "localhost"].includes(parsed.hostname) && parsed.pathname.startsWith("/review_wait_benchmark"),
|
|
"External benchmark database must be a disposable local review_wait_benchmark database");
|
|
await applyPendingMigrations(externalUrl);
|
|
db = createDb(externalUrl);
|
|
} else {
|
|
temporary = await startEmbeddedPostgresTestDatabase("github-review-wait-benchmark-");
|
|
db = createDb(temporary.connectionString);
|
|
}
|
|
const f = await seedGitHubReviewWait(db);
|
|
const summary = randomBytes(4_800).toString("base64");
|
|
await setGitHubReviewAssessment(db, f.reviewId, { reviewedCommit: f.headSha, summary, findings: [] });
|
|
await db.$client.unsafe("ANALYZE");
|
|
|
|
async function capture(reader: typeof readInheritedGitHubReviewWait) {
|
|
const queries: CapturedReviewWaitQuery[] = [];
|
|
const result = await reader(captureReviewWaitQueries(db!, queries), f.run);
|
|
const query = queries.find(query => query.sql.includes('join "chat_github_reviews"'));
|
|
assert.ok(query, "Capture the real joined service query");
|
|
return { result, query };
|
|
}
|
|
const before = await capture(baseline.readInheritedGitHubReviewWait), after = await capture(readInheritedGitHubReviewWait);
|
|
assert.deepEqual(after.result, before.result);
|
|
assert.equal(before.result?.kind, "published");
|
|
const rawBefore = await db.$client.unsafe(before.query.sql, before.query.params as never[]).raw();
|
|
const rawAfter = await db.$client.unsafe(after.query.sql, after.query.params as never[]).raw();
|
|
assert.equal(rawBefore.length, 1);
|
|
assert.equal(rawAfter.length, 1);
|
|
const normalizedBefore = rawBefore[0]!.map(value => value?.toString() ?? null);
|
|
const normalizedAfter = rawAfter[0]!.map(value => value?.toString() ?? null);
|
|
normalizedBefore[6] = JSON.stringify(JSON.parse(normalizedBefore[6]!).reviewedCommit);
|
|
assert.deepEqual(normalizedAfter, normalizedBefore, "Every unchanged SQL column and selected commit must match");
|
|
const dataRowBytes = (rows: Buffer[][]) => rows.reduce((total, row) => total + 7 + 4 * row.length +
|
|
row.reduce((bytes, value) => bytes + (value?.byteLength ?? 0), 0), 0);
|
|
const baselineDataRowBytes = dataRowBytes(rawBefore), treatmentDataRowBytes = dataRowBytes(rawAfter);
|
|
assert.ok(treatmentDataRowBytes < baselineDataRowBytes * 0.2, "Fixture acceptance: remove at least 80% of DataRow bytes");
|
|
|
|
const equivalenceCases = [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 }];
|
|
for (const assessment of equivalenceCases) {
|
|
await setGitHubReviewAssessment(db, f.reviewId, assessment);
|
|
assert.deepEqual(await readInheritedGitHubReviewWait(db, f.run), await baseline.readInheritedGitHubReviewWait(db, f.run));
|
|
}
|
|
await setGitHubReviewAssessment(db, f.reviewId, { reviewedCommit: f.headSha, summary, findings: [] });
|
|
await revokeReviewWaitConnection(db, f);
|
|
assert.deepEqual(await readInheritedGitHubReviewWait(db, f.run), { kind: "revoked" });
|
|
assert.deepEqual(await baseline.readInheritedGitHubReviewWait(db, f.run), { kind: "revoked" });
|
|
|
|
await db.$client.unsafe("UPDATE tool_connections SET enabled = true WHERE id = $1::uuid", [f.connectionId]);
|
|
assert.equal((await readInheritedGitHubReviewWait(db, f.run))?.kind, "published");
|
|
|
|
async function timed(query: CapturedReviewWaitQuery) {
|
|
const start = performance.now();
|
|
await db!.$client.unsafe(query.sql, query.params as never[]).values();
|
|
return performance.now() - start;
|
|
}
|
|
const samples = { baseline: [] as number[], treatment: [] as number[] };
|
|
for (let n = 0; n < 9; n++) {
|
|
const versions = n % 2 ? ["treatment", "baseline"] as const : ["baseline", "treatment"] as const;
|
|
for (const version of versions) {
|
|
const elapsed = await timed(version === "baseline" ? before.query : after.query);
|
|
if (n >= 2) samples[version].push(elapsed);
|
|
}
|
|
}
|
|
const stats = (values: number[]) => {
|
|
const sorted = [...values].sort((a, b) => a - b);
|
|
return { samplesMs: values, medianMs: sorted[Math.floor(sorted.length / 2)], minMs: sorted[0], maxMs: sorted.at(-1) };
|
|
};
|
|
async function plan(query: CapturedReviewWaitQuery) {
|
|
const [result] = await db!.$client.unsafe(`EXPLAIN (ANALYZE, BUFFERS, FORMAT JSON) ${query.sql}`, query.params as never[]);
|
|
return result!["QUERY PLAN"];
|
|
}
|
|
console.log(JSON.stringify({ baselineRevision, treatmentRevision,
|
|
treatmentDiff: execFileSync("git", ["diff", "HEAD", "--", modulePath], { cwd: root, encoding: "utf8" }),
|
|
postgresVersion: (await db.$client.unsafe("SELECT version()"))[0]!.version,
|
|
settings: await db.$client.unsafe("SELECT name, setting FROM pg_settings WHERE name IN ('block_size','shared_buffers','work_mem','jit') ORDER BY name"),
|
|
fixture: { reviews: 1, assessmentSummaryBytes: Buffer.byteLength(summary), synthetic: true },
|
|
generatedQueries: { baseline: before.query, treatment: after.query },
|
|
correctness: { completeOutputEqual: true, unchangedColumnsEqual: true, malformedJsonCases: equivalenceCases.length,
|
|
connectionRevocationEqual: true },
|
|
bytes: { measurement: "PostgreSQL DataRow messages from raw text-format column buffers; excludes metadata, TLS and TCP",
|
|
baselineDataRowBytes, treatmentDataRowBytes, reductionPercent: 100 * (1 - treatmentDataRowBytes / baselineDataRowBytes) },
|
|
timings: { warmups: 2, samples: 7, order: "alternating", baseline: stats(samples.baseline), treatment: stats(samples.treatment) },
|
|
plans: { baseline: await plan(before.query), treatment: await plan(after.query) },
|
|
}, null, 2));
|
|
} finally {
|
|
try {
|
|
await db?.$client.end({ timeout: 0 });
|
|
await temporary?.cleanup();
|
|
if (baselineDir) await fs.rm(baselineDir, { recursive: true, force: true });
|
|
} finally { clearInterval(keepAlive); }
|
|
}
|