mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-28 20:21:50 +01:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Tool and connection operations capture the current accepted requester. > - Capture locks the task and run, then reconciles pending steering. > - Most callers use identity or task fields from the returned run. > - The default query also transfers saved prompts, runner configuration, results, and logs. > - This pull request gives each production caller an explicit read scope. > - Narrow scopes keep their authority checks and return only selected columns. > - The Muse project path still reads its complete saved execution input. ## Linked Issues or Issue Description **What existing behavior does this improve?** Operation-time run identity capture for native tools, gateway sessions, project and workspace access, Slack, voice, connection management, and connection token admission. **Current behavior** These callers use the default full run read. Large saved execution documents cross the database connection even when the caller only needs the current accepted requester or task binding. **Proposed behavior** Use typed identity, task, broker, and project-execution scopes. Keep task-before-run locks, company and agent predicates, active status validation, and pending-steering reconciliation. Return the complete saved native input only where project admission uses it. **Reason and benefit** A synthetic PostgreSQL 17.11 fixture matches a wide run with a 142 KB context and a 77 KB runner profile. Its locked run DataRow falls from 225,101 bytes to 190–605 bytes for authority reads. The project scope returns 66,004 bytes because it must retain a complete 64 KB execution input. These are local DataRow measurements, not a forecast of production billing or latency. **Breaking changes** None to public APIs, saved data, or authority policy. Internal narrow return types omit columns that were not selected. The default full compatibility read and the existing GitHub-credential and authentication scopes keep their current contracts. **Additional context** The public issue and PR duplicate searches found no matching open change. This is one independent query optimization. It does not require other performance PRs. ## What Changed - Add typed scopes for accepted identity metadata, task authorization, token attribution, and saved project execution input. - Assign explicit scopes at all production `captureRunIdentity` call sites. - Keep the full native execution input for Muse project admission while omitting unrelated profile, context, result, and log data. - Narrow the broker's initial read and the existing Slack and connection-management historical reads to the same required fields. - Extend steering and acknowledgement regressions to all new scopes. Add real PostgreSQL projection, malformed JSON, tenant and agent, session-generation, and saved-input coverage. - Add a service-generated SQL benchmark with exact raw DataRow accounting, retained-field equality, plans, and alternating transaction timings. - Document the scope contracts and reproducible benchmark. ## Verification - Baseline: `623f1dc289a14a699ebbe3f09b94179d38fc2ae2`. - `vitest run server/src/__tests__/run-identity-projections.test.ts server/src/__tests__/run-identity.test.ts`: 54 tests passed on an owned disposable PostgreSQL 17.11 database. - The tests retain task-before-run and audit foreign-key lock behavior. Every scope rejects uncertain steering, accepts its durable acknowledgement, and preserves requester replay ordering. - A full-row mutation makes the narrow identity regression fail. The restored treatment passes. - `pnpm --filter @paperclipai/server exec tsx ../scripts/benchmark-run-identity-projections.ts --baseline-ref=623f1dc289a14a699ebbe3f09b94179d38fc2ae2` captures the real locked SQL for both versions. - Every retained column and selected JSON field matches the baseline. Identity contexts match. The complete stored run remains unchanged. Capture keeps the same five queries. | Scope | Before DataRow bytes | After DataRow bytes | Reduction | | --- | ---: | ---: | ---: | | Identity | 225,101 | 190 | 99.92% | | Task authorization | 225,101 | 410 | 99.82% | | Token broker | 225,101 | 605 | 99.73% | | Project execution input | 225,101 | 66,004 | 70.68% | - DataRow bytes include row framing and raw text-format column values. They exclude response metadata, TLS, and TCP. - Two warmups and seven alternating samples measure the complete capture transaction. Several timing ranges overlap. The primary claim is byte reduction; production latency is not verified. - Initial fixture runs exposed an empty external schema and an incomplete synthetic conversation binding. The test setup now applies migrations and supplies the required user/state fields. The final 54 tests passed. - `pnpm -r typecheck` and `pnpm build` passed locally. - Four existing skill-setup tests passed. Four assembled connection-management/native-tool consumer cases passed with real JWT/API-key authentication. These include per-request identity freshness and task-scoped activity authority. - A CI-only stale mock assertion expected two capture arguments; it now verifies the explicit identity scope. A repository-wide capture mock audit found no other stale argument assertions. - Review follow-up: the projection suite uses the existing embedded-support guard while always enabling an explicitly supplied external database. The audit-lock regression merges URL query parameters and retains existing PostgreSQL options. - `vitest run server/src/__tests__/run-identity-projections.test.ts server/src/__tests__/run-identity.test.ts server/src/__tests__/company-skills-routes.test.ts server/src/__tests__/project-skill-setup-identity.test.ts`: 123 tests passed. The external PostgreSQL URL included both `application_name` and `options`; lock tests verified `lock_timeout=1s` and retained `statement_timeout=5s`. - Full local tests are not run because the host's embedded PostgreSQL setup is unavailable. Greptile reviewed `06e1dd53a3fb24df74d5fbfb2ad397f291f1e30f` at 5/5 with zero unresolved threads. The full final-head CI matrix passes. All 58 checks are terminal: 55 success, one neutral, and two optional Storybook skips. - No migration, lockfile, workflow, new polling, or cache change. ## Risks A narrow read must retain each caller's actual fields. The caller audit includes Slack's wake-request fallback, conversation generation, legacy broker attribution, and the Muse parser's complete native input. JSON selection retains value types, including malformed legacy values. The saved-input scope preserves missing-key behavior. The default full read still returns the complete stored row for compatibility. Locks and live authority checks are unchanged. ## 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>
121 lines
7.8 KiB
TypeScript
121 lines
7.8 KiB
TypeScript
import assert from "node:assert/strict";
|
|
import { execFileSync } from "node:child_process";
|
|
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 { captureRunIdentity } from "../server/src/services/run-identity.js";
|
|
import { captureIdentityQueries, seedIdentityProjection, type IdentityQuery } from "../server/src/__tests__/helpers/run-identity-projection-fixture.js";
|
|
|
|
const root = fileURLToPath(new URL("../", import.meta.url));
|
|
const git = (args: string[]) => execFileSync("git", args, { cwd: root, encoding: "utf8" });
|
|
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 = git(["rev-parse", "--verify", `${baselineRef}^{commit}`]).trim();
|
|
const modulePath = "server/src/services/run-identity.ts";
|
|
const unchangedHelpers = ["run-task-binding.ts", "task-retry.ts", "issue-queued-comment-queue.ts"];
|
|
for (const name of unchangedHelpers) {
|
|
const helper = `server/src/services/${name}`;
|
|
assert.equal(git(["show", `${baselineRevision}:${helper}`]), await fs.readFile(path.join(root, helper), "utf8"),
|
|
`Baseline helper ${name} changed; isolate it before benchmarking`);
|
|
}
|
|
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 = git(["show", `${baselineRevision}:${modulePath}`]);
|
|
const moduleUrl = new URL(`../${modulePath}`, import.meta.url);
|
|
baselineDir = await fs.mkdtemp(path.join(root, "server/scripts/.identity-projection-baseline-"));
|
|
const baselineFile = path.join(baselineDir, "run-identity.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/run-identity.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) && /_test$/.test(parsed.pathname),
|
|
"External benchmark database must be a disposable local *_test database");
|
|
await applyPendingMigrations(externalUrl);
|
|
db = createDb(externalUrl);
|
|
} else {
|
|
temporary = await startEmbeddedPostgresTestDatabase("identity-projection-benchmark-");
|
|
db = createDb(temporary.connectionString);
|
|
}
|
|
const f = await seedIdentityProjection(db);
|
|
await db.$client.unsafe("ANALYZE");
|
|
const beforeQueries: IdentityQuery[] = [];
|
|
const before = await baseline.captureRunIdentity(captureIdentityQueries(db, beforeQueries), f);
|
|
const findRunQuery = (queries: IdentityQuery[]) => {
|
|
const query = queries.find(query => query.sql.includes('from "heartbeat_runs"') && query.sql.includes("for no key update"));
|
|
assert.ok(query, "Capture the actual locked identity query");
|
|
return query;
|
|
};
|
|
const beforeQuery = findRunQuery(beforeQueries);
|
|
const dataRowBytes = async (query: IdentityQuery) => {
|
|
const rows = await db!.$client.unsafe(query.sql, query.params as never[]).raw();
|
|
return rows.reduce((total, row) => total + 7 + 4 * row.length + row.reduce((bytes, value) => bytes + (value?.byteLength ?? 0), 0), 0);
|
|
};
|
|
const baselineBytes = await dataRowBytes(beforeQuery);
|
|
async function plan(query: IdentityQuery) {
|
|
const [result] = await db!.$client.unsafe(`EXPLAIN (ANALYZE, BUFFERS, FORMAT JSON) ${query.sql}`, query.params as never[]);
|
|
return result!["QUERY PLAN"];
|
|
}
|
|
const results = [];
|
|
for (const readScope of ["identity", "task_authorization", "token_broker", "project_execution"] as const) {
|
|
const afterQueries: IdentityQuery[] = [];
|
|
const after = await captureRunIdentity(captureIdentityQueries(db, afterQueries), f, { readScope });
|
|
assert.deepEqual(after.context, before.context);
|
|
for (const [key, value] of Object.entries(after.run)) {
|
|
if (key === "runnerProfileJson") assert.deepEqual(value, { nativeExecutionInput: before.run.runnerProfileJson?.nativeExecutionInput });
|
|
else if (key !== "contextSnapshot") assert.deepEqual(value, before.run[key as keyof typeof before.run], `Retained ${key}`);
|
|
else {
|
|
const snapshot = value as Record<string, unknown>;
|
|
for (const [name, selected] of Object.entries(snapshot)) {
|
|
if (name !== "paperclipIssue") assert.deepEqual(selected, before.run.contextSnapshot?.[name] ?? null, `Retained context ${name}`);
|
|
else for (const [nested, item] of Object.entries(selected as Record<string, unknown>))
|
|
assert.deepEqual(item, (before.run.contextSnapshot?.paperclipIssue as Record<string, unknown>)?.[nested] ?? null, `Retained nested ${nested}`);
|
|
}
|
|
}
|
|
}
|
|
const afterQuery = findRunQuery(afterQueries), afterBytes = await dataRowBytes(afterQuery);
|
|
assert.ok(afterBytes < baselineBytes * (readScope === "project_execution" ? 0.5 : 0.05),
|
|
"Fixture acceptance: remove at least 95% of authority bytes or 50% while preserving complete execution input");
|
|
assert.equal(afterQueries.length, beforeQueries.length, "No queries added or removed from identity reconciliation");
|
|
const samples = { baseline: [] as number[], treatment: [] as number[] };
|
|
for (let n = 0; n < 9; n++) {
|
|
for (const version of n % 2 ? ["treatment", "baseline"] as const : ["baseline", "treatment"] as const) {
|
|
const start = performance.now();
|
|
if (version === "baseline") await baseline.captureRunIdentity(db, f);
|
|
else await captureRunIdentity(db, f, { readScope });
|
|
if (n >= 2) samples[version].push(performance.now() - start);
|
|
}
|
|
}
|
|
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) };
|
|
};
|
|
results.push({ readScope, baselineBytes, treatmentBytes: afterBytes, reductionPercent: 100 * (1 - afterBytes / baselineBytes),
|
|
retainedColumnsAndContextEqual: true, identityContextEqual: true, queryCount: afterQueries.length,
|
|
generatedSql: { baseline: beforeQuery, treatment: afterQuery },
|
|
timings: { scope: "complete captureRunIdentity service transaction", warmups: 2, samples: 7, order: "alternating",
|
|
baseline: stats(samples.baseline), treatment: stats(samples.treatment) },
|
|
plans: { baseline: await plan(beforeQuery), treatment: await plan(afterQuery) } });
|
|
}
|
|
assert.deepEqual((await captureRunIdentity(db, f)).run, before.run, "Complete stored run unchanged");
|
|
console.log(JSON.stringify({ baselineRevision, treatmentRevision: git(["rev-parse", "HEAD"]).trim(),
|
|
treatmentDiff: git(["diff", "HEAD", "--", "server/src"]), unchangedHelpers,
|
|
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: { runs: 1, synthetic: true, contextBytes: Buffer.byteLength(JSON.stringify(f.payloads.contextSnapshot)),
|
|
profileBytes: Buffer.byteLength(JSON.stringify(f.payloads.runnerProfileJson)) },
|
|
measurement: "PostgreSQL DataRow messages from raw text-format buffers; excludes metadata, TLS and TCP", results,
|
|
}, 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); }
|
|
}
|