From c08da3e0a72ca58f5542c70cca93e59b5b828ad3 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Wed, 28 Oct 2026 09:07:17 -0500 Subject: [PATCH] perf(server): narrow skill list reconciliation reads (#18023) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The skill library lists current company skills and their agent attachments. > - Each list request also reconciles bundled skills, folders, and local files. > - These reads return full agent records and large metadata that their callers do not use. > - Conditional requests do this work before they return an unchanged response. > - This pull request selects the fields each caller needs and avoids unused folder lookups. > - The result is less database transfer and fewer round trips while reconciliation stays current. ## Linked Issues or Issue Description Refs #17941. That change reuses bundled categories within an import. This change removes other repeated work in the same list path. **What happened?** Warm skill-list requests still made 119 SQL calls in a local HTTP fixture. The folder pass returned full skill metadata. The local-source pass also returned remote skills. Attachment counts loaded full agent configurations and monthly spend. Bundled reconciliation resolved folder paths that its release check did not use. **Expected behavior** The list must preserve its current result, attachment counts, access rules, local-file reconciliation, and bundled release checks. Internal reads should return only the data their callers need. **Steps to reproduce** 1. Use a company with many remote skills and agents with large saved configurations. 2. Read its skill list, then repeat the read with its ETag. 3. Measure the queries and response values before the server returns `200` or `304`. 4. Run `pnpm --filter @paperclipai/server exec tsx scripts/benchmark-skill-list.ts --samples=5 --network-delay-ms=3.5` for a synthetic reproduction. **Paperclip version or commit** Baseline service: `e1ba77f06771a27114b3999004b0ca5e3c172dd4`. The branch is based on current master. The paired mode changes only the skill-service module; both versions use the same imported helpers. **Deployment mode** Built from source. Local HTTP server and disposable PostgreSQL 17. No hosted deployment is part of this PR's local proof. ## What Changed - Read only the desired-skill selections of nonterminated company agents for attachment counts. Resolve each agent's references once against the full visible skill list. Preserve IDs, keys, aliases, duplicate handling, and ambiguous slugs. - Read only `sourceKind` from metadata for bundled folder classification. - Limit local-source reconciliation to local-path rows. - Skip folder-path enrichment only for bundled reconciliation. Normal imports and list responses keep their folder paths. - Add real-database byte regressions and a local HTTP benchmark. The benchmark can alternate the old and new services against one database, actor, and process. It checks complete response equality and conditional responses. The company-skill service owns these reads. The query predicates and freshness checks remain at their existing owners. This change adds no cache, schema change, or new background work. ## Verification - The attachment byte regression failed before the fix: 601,418 captured response-value bytes exceeded its 2,000-byte bound. The same case passed after the fix. It checks fresh configuration edits, IDs and keys, duplicate aliases, ambiguous slugs, paused agents, terminated agents, and company scope. - 111 focused service and authorization tests passed against isolated real PostgreSQL 17 databases. They include full result equality, retained stored Markdown and metadata, local-file updates, release checks, project visibility, and import authorization. - The default embedded PostgreSQL probe skipped the first local attempt on this Mac. The completed run used a local-only bootstrap adapter for disposable databases. Queries, migrations, services, authorization, and filesystem work used their real implementations. The adapter is not part of this PR. - In repeated synthetic HTTP byte runs, calls fell from 119 to 98 per request. Result-value bytes fell from about 7.75 MB to 1.64 MB per request, about 79%. These values are measured before Drizzle decoding. They exclude protocol framing and do not measure provider billing. - A separate-process run without the byte observer did not show a latency gain. The paired experiment removes that comparison gap: it alternates both services against the same database and checks identical complete responses. On final commit `3076fe1402085f7286ff58e642d7caca892a5f80`, five observer-free pairs had median times of 2.582 s before and 1.850 s after (28.3% lower). The candidate was faster in all five pairs. Its range was 1.802–2.199 s; the baseline range was 2.327–2.725 s. Every normal response had the same complete body hash, and both versions returned the expected unchanged conditional responses. These are local service/HTTP timings with synthetic data and 3.5 ms delay per network direction. They are not hosted acceptance. - Final-head workspace typecheck and build passed. The full local test command did not complete because embedded PostgreSQL bootstrap failed on this Mac; I stopped that owned runner after 16 native-runtime cases failed. All 16 cases then passed when rerun alone against disposable PostgreSQL 17 using their existing external-database path. The 111 focused PostgreSQL 17 tests above also completed successfully. This does not constitute a completed full local suite; all current-head remote CI gates passed (55 successful checks and 2 skipped). Module-boundary and whitespace checks pass. An initial nested package command used the global pnpm; subsequent commands were pinned to pnpm 9.15.4. - The first canary dry run failed while installing an unrelated standalone package: npm returned 404 for the newly published Rollup 4.66.0 tarball. The exact download subsequently returned HTTP 200. The one retry passed without code changes. - Benchmark commands: add `--capture-values=false` for observer-free timings. Add `--baseline-ref=e1ba77f06771a27114b3999004b0ca5e3c172dd4 --capture-values=false` for paired service acceptance. The normal mode uses the production router. The paired mode uses equivalent default-list HTTP handlers and a synthetic authenticated board actor; it does not test login. ## Risks The SQL selection must stay in sync if the desired-skill format or folder classifier needs new fields. Existing parsing and reference resolution still interpret the selected data. The synthetic fixture has large unused payloads. It proves the mechanism and preserves required content. Its percentage is not a projection of whole-instance transfer or billed savings. Hosted latency still needs verification after merge and deployment. ## Model Used OpenAI GPT-6 through Codex, with repository inspection, shell execution, real PostgreSQL, and local HTTP testing. The host does not expose a precise backend revision or context-window size. ## 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 --- doc/DEVELOPING.md | 30 +++ server/scripts/benchmark-skill-list.ts | 249 ++++++++++++++++++ .../__tests__/company-skills-service.test.ts | 73 +++++ server/src/services/company-skills.ts | 42 +-- 4 files changed, 378 insertions(+), 16 deletions(-) create mode 100644 server/scripts/benchmark-skill-list.ts diff --git a/doc/DEVELOPING.md b/doc/DEVELOPING.md index f06bb5c943b..0c2df84b6fc 100644 --- a/doc/DEVELOPING.md +++ b/doc/DEVELOPING.md @@ -2497,6 +2497,36 @@ identity mismatches. ## App-Shipped Skills Catalog +Skill-library reads still reconcile bundled releases, folders, and direct local +sources on each request. Folder classification reads only its source-kind field, +and local-source reconciliation reads only local sources. Bundled reconciliation +keeps current skill access checks but does not resolve unused folder paths. +Attachment counts read only nonterminated agents' saved desired-skill selections and +resolve them once against the complete visible skill list. Detail reads and +stored inputs remain complete. + +Run the synthetic HTTP comparison through the production skills router: + +```sh +pnpm --filter @paperclipai/server exec tsx scripts/benchmark-skill-list.ts --samples=5 --network-delay-ms=3.5 +``` + +The harness starts disposable PostgreSQL, synthetic skills and agents, and a +loopback HTTP server. It checks both normal and conditional responses and records +inventory phases, SQL calls, and PostgreSQL result-value bytes. These bytes omit +protocol framing and do not measure provider billing. The actor is an authenticated +board fixture; the harness does not test login. To use a task-owned local PostgreSQL +server instead of embedded PostgreSQL, pass `--postgres-port=PORT` and +`--postgres-user=USER`. It creates and removes a separate test database. Keep timing +runs separate from builds and tests; repeat both source versions when host noise +is material. Repeat with `--capture-values=false` for acceptance timings without +the response-value observer. Add `--baseline-ref=COMMIT --capture-values=false` +to alternate the original and current skill services against the same database +and actor. This mode checks full response equality through equivalent local HTTP +handlers. The normal mode uses the production router. Only `company-skills.ts` +differs in paired mode; imported helpers stay at the current checkout. + + Plugin environment drivers can declare `skillKeys` that reference their own manifest-managed skills. Selecting that environment adds those skills to the normal authorized skill selection, including an explicitly empty optional diff --git a/server/scripts/benchmark-skill-list.ts b/server/scripts/benchmark-skill-list.ts new file mode 100644 index 00000000000..b535fb2bed1 --- /dev/null +++ b/server/scripts/benchmark-skill-list.ts @@ -0,0 +1,249 @@ +import assert from "node:assert/strict"; +import { createHash, randomUUID } from "node:crypto"; +import { execFileSync } from "node:child_process"; +import { promises as fs } from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { fileURLToPath, pathToFileURL } from "node:url"; +import { connect, createServer, type Socket } from "node:net"; +import express from "express"; +import { get as httpGet } from "node:http"; +import { context, trace } from "@opentelemetry/api"; +import { AsyncLocalStorageContextManager } from "@opentelemetry/context-async-hooks"; +import { InMemorySpanExporter, SimpleSpanProcessor, TracerProvider } from "@opentelemetry/sdk-trace"; +import { eq } from "drizzle-orm"; +import { authUsers, companies, companyMemberships, companySkills, companySkillSources, + projectSkillSources, projects, applyPendingMigrations, closeRegisteredClients, createDb, + startEmbeddedPostgresTestDatabase } from "../../packages/db/src/index.js"; +import * as schema from "../../packages/db/src/schema/index.js"; +import { createDatabaseResponseCapture, installDatabaseResponseCapture } from "../../packages/db/src/response-capture.js"; +import { withRunExecutionTiming } from "../src/services/execution-preparation-timing.js"; + +const argument = (name: string) => process.argv.find(value => value.startsWith(`${name}=`))?.slice(name.length + 1); +const sampleCount = Number(argument("--samples") ?? 5); +const baselineRef = argument("--baseline-ref"); +const delayMs = Number(argument("--network-delay-ms") ?? 3.5); +const captureArgument = argument("--capture-values") ?? "true"; +assert.ok(["true", "false"].includes(captureArgument)); +const captureValues = captureArgument === "true"; +assert.ok(Number.isInteger(sampleCount) && sampleCount >= 3 && sampleCount <= 10); +assert.ok(Number.isFinite(delayMs) && delayMs >= 0 && delayMs <= 20); +const previous = { home: process.env.PAPERCLIP_HOME, instance: process.env.PAPERCLIP_INSTANCE_ID, + endpoint: process.env.OTEL_EXPORTER_OTLP_ENDPOINT }; +delete process.env.OTEL_EXPORTER_OTLP_ENDPOINT; +const { companySkillService } = await import("../src/services/company-skills.js"); +const { createAgentLifecycle } = await import("../src/services/agent-lifecycle.js"); +const { companySkillRoutes } = await import("../src/routes/company-skills.js"); +const { skillAccessRequestContext, withSkillActor } = await import("../src/services/skill-access.js"); +const home = await fs.mkdtemp(path.join(os.tmpdir(), "skill-list-benchmark-")); +const sourceFiles = [fileURLToPath(import.meta.url), fileURLToPath(new URL("../src/services/company-skills.ts", import.meta.url))]; +const sourceHash = async () => { + const hash = createHash("sha256"); + for (const file of sourceFiles) hash.update(await fs.readFile(file)); + return hash.digest("hex"); +}; +const sourceRevision = execFileSync("git", ["rev-parse", "HEAD"], { cwd: fileURLToPath(new URL("../../", import.meta.url)), encoding: "utf8" }).trim(); +const measuredSourceHash = await sourceHash(); +let database: Awaited> | undefined; +let db: ReturnType | undefined; +let proxy: ReturnType | undefined; +let server: ReturnType["listen"]> | undefined; +let baselineFile: string | undefined; +let baselineRevision: string | undefined; +let baselineSourceHash: string | undefined; +let provider: TracerProvider | undefined; +let manager: AsyncLocalStorageContextManager | undefined; +const sockets = new Set(); +try { + process.env.PAPERCLIP_HOME = home; + process.env.PAPERCLIP_INSTANCE_ID = "default"; + const port = argument("--postgres-port"); + if (port) { + assert.match(port, /^\d{1,5}$/); + assert.ok(Number(port) > 0 && Number(port) < 65536); + const user = encodeURIComponent(argument("--postgres-user") ?? os.userInfo().username); + const name = `skill_list_${randomUUID().replaceAll("-", "")}_test`; + const adminUrl = `postgres://${user}@127.0.0.1:${port}/postgres`; + const connectionString = `postgres://${user}@127.0.0.1:${port}/${name}`; + const admin = createDb(adminUrl, { maxConnections: 1 }).$client; + try { await admin.unsafe(`CREATE DATABASE "${name}"`); } finally { await admin.end({ timeout: 1 }); } + const cleanup = async () => { + await closeRegisteredClients(connectionString); + const client = createDb(adminUrl, { maxConnections: 1 }).$client; + try { await client.unsafe(`DROP DATABASE "${name}"`); } finally { await client.end({ timeout: 1 }); } + }; + database = { connectionString, cleanup }; + await applyPendingMigrations(connectionString); + } else database = await startEmbeddedPostgresTestDatabase("skill-list-benchmark-"); + + const target = new URL(database.connectionString); + const upstreamPort = Number(target.port); + if (delayMs) { + proxy = createServer(client => { + const upstream = connect({ host: "127.0.0.1", port: upstreamPort }); + for (const socket of [client, upstream]) { + socket.setNoDelay(true); sockets.add(socket); + socket.on("error", () => { client.destroy(); upstream.destroy(); }); + socket.on("close", () => sockets.delete(socket)); + } + const relay = (from: Socket, to: Socket) => { + from.on("data", chunk => { from.pause(); setTimeout(() => { + if (to.destroyed) return; + if (to.write(chunk)) from.resume(); + else to.once("drain", () => from.resume()); + }, delayMs); }); + from.on("end", () => setTimeout(() => to.end(), delayMs)); + }; + relay(client, upstream); relay(upstream, client); + }); + await new Promise(resolve => proxy!.listen(0, "127.0.0.1", resolve)); + const address = proxy.address(); assert.ok(address && typeof address !== "string"); + target.port = String(address.port); + } + db = createDb(target.href); + const companyId = randomUUID(), userId = randomUUID(); + await db.insert(companies).values({ id: companyId, name: "Skill list benchmark", issuePrefix: "SLB" }); + await db.insert(authUsers).values({ id: userId, name: "Fixture owner", email: `${userId}@example.invalid`, createdAt: new Date(), updatedAt: new Date() }); + await db.insert(companyMemberships).values({ companyId, principalType: "user", principalId: userId, membershipRole: "owner" }); + const projectRows = await db.insert(projects).values(Array.from({ length: 14 }, (_, i) => ({ companyId, name: `Fixture ${i}` }))).returning(); + const sources = await db.insert(companySkillSources).values(Array.from({ length: 10 }, (_, i) => ({ companyId, + repositoryUrl: `https://example.invalid/fixture-${i}`, fullName: `fixture/${i}`, trackingRef: "main", + accessScope: i < 4 ? "projects" as const : "company" as const }))).returning(); + await db.insert(projectSkillSources).values(sources.slice(0, 8).map((source, i) => ({ companyId, sourceId: source.id, + projectId: projectRows[i % 5]!.id, repositoryUrl: source.repositoryUrl, trackingRef: "main", status: "ready" as const }))); + const skills = await db.insert(companySkills).values(Array.from({ length: 155 }, (_, i) => ({ companyId, + key: `fixture/skill-${i}`, slug: `skill-${i}`, name: `Skill ${String(i).padStart(3, "0")}`, + markdown: "Synthetic unused markdown. ".repeat(280), sourceType: "github", + sourceLocator: "https://example.invalid/immutable-fixture", accessSourceId: sources[i % 10]!.id, + metadata: { unused: "metadata ".repeat(1000) }, fileInventory: [{ path: "SKILL.md", kind: "skill" as const }], + }))).returning(); + const lifecycle = createAgentLifecycle(db); + for (let i = 0; i < 40; i++) await lifecycle.requestHire(companyId, { + name: `Reader ${i}`, adapterType: "process", status: i === 39 ? "terminated" : "paused", + adapterConfig: { instructions: "Synthetic agent context. ".repeat(2600), + paperclipSkillSync: { desiredSkills: [skills[i % 155]!.key, { key: skills[(i + 1) % 155]!.id, versionId: null }] } }, + runtimeConfig: { unused: "runtime ".repeat(1000) }, metadata: { unused: "metadata ".repeat(1000) }, + }, { runnerResolved: true }); + await companySkillService(db).list(companyId); + + const actor = { type: "board" as const, source: "session" as const, userId, companyIds: [companyId], + memberships: [{ companyId, membershipRole: "owner" }] }; + let baselineService: ReturnType | undefined; + if (baselineRef) { + assert.equal(captureValues, false, "Paired acceptance uses --capture-values=false"); + baselineRevision = execFileSync("git", ["rev-parse", "--verify", "--end-of-options", `${baselineRef}^{commit}`], { encoding: "utf8" }).trim(); + const source = execFileSync("git", ["show", `${baselineRevision}:server/src/services/company-skills.ts`], { encoding: "utf8", maxBuffer: 2 * 1024 * 1024 }); + baselineSourceHash = createHash("sha256").update(source).digest("hex"); + baselineFile = fileURLToPath(new URL(`../src/services/.company-skills-benchmark-${randomUUID()}.ts`, import.meta.url)); + await fs.writeFile(baselineFile, source, { flag: "wx", mode: 0o600 }); + const baselineModule = await import(pathToFileURL(baselineFile).href) as { companySkillService: typeof companySkillService }; + baselineService = baselineModule.companySkillService(db); + await withSkillActor(actor, () => baselineService!.list(companyId)); + } + + const exporter = new InMemorySpanExporter(); + provider = new TracerProvider({ spanProcessors: [new SimpleSpanProcessor({ exporter })] }); + manager = new AsyncLocalStorageContextManager().enable(); + assert.ok(context.setGlobalContextManager(manager)); assert.ok(trace.setGlobalTracerProvider(provider)); + process.env.OTEL_EXPORTER_OTLP_ENDPOINT = "http://127.0.0.1:4318"; + const app = express(); + app.use((req, res, next) => { + req.actor = actor; + void withRunExecutionTiming(randomUUID(), () => new Promise(resolve => { res.once("finish", resolve); next(); })).catch(next); + }); + const candidateService = companySkillService(db); + if (baselineService) app.get("/paired/:variant", skillAccessRequestContext, async (req, res) => { + const selected = req.params.variant === "baseline" ? baselineService! : candidateService; + res.json(await selected.list(companyId)); + }); + app.use("/api", companySkillRoutes(db)); + app.use((error: Error, _req: express.Request, res: express.Response, _next: express.NextFunction) => res.status(500).json({ error: error.message })); + server = app.listen(0, "127.0.0.1"); + await new Promise(resolve => server!.once("listening", resolve)); + const address = server.address(); assert.ok(address && typeof address !== "string"); + const url = `http://127.0.0.1:${address.port}/api/companies/${companyId}/skills`; + let queryCount = 0; + db.$client.options.debug = () => { queryCount++; }; + const request = async (etag?: string, requestUrl = url) => { + queryCount = 0; exporter.reset(); + const started = performance.now(); + const response = await new Promise<{ status: number; body: string; etag: string | undefined }>((resolve, reject) => { + const request = httpGet(requestUrl, { headers: etag ? { "if-none-match": etag } : {} }, response => { + const chunks: Buffer[] = []; + response.on("data", chunk => chunks.push(Buffer.from(chunk))); + response.on("error", reject); + response.on("end", () => resolve({ status: response.statusCode!, body: Buffer.concat(chunks).toString("utf8"), etag: response.headers.etag })); + }); + request.setTimeout(20_000, () => request.destroy(new Error("Local benchmark request timed out"))); + request.on("error", reject); + }); + const body = response.body; + const elapsedMs = performance.now() - started; + assert.equal(response.status, etag ? 304 : 200, "Unexpected local skills-list HTTP status"); + if (!etag) { + const items = JSON.parse(body) as Array<{ key: string; attachedAgentCount: number }>; + assert.ok(items.length >= 155); + assert.equal(items.find(item => item.key === skills[0]!.key)?.attachedAgentCount, 1); + assert.equal(items.find(item => item.key === skills[39]!.key)?.attachedAgentCount, 1); + assert.equal(items.find(item => item.key === skills[40]!.key)?.attachedAgentCount, 0); + } + await provider!.forceFlush(); + return { elapsedMs, queryCount, status: response.status, etag: response.etag, + bodyHash: createHash("sha256").update(body).digest("hex"), + phases: exporter.getFinishedSpans().filter(span => span.name.startsWith("skills.")).map(span => ({ name: span.name, + durationMs: span.duration[0] * 1000 + span.duration[1] / 1e6 })) }; + }; + const first = await request(); assert.ok(first.etag); + const events: Array> = [], now = Date.now(); + const capture = captureValues ? createDatabaseResponseCapture({ start: now - 1, end: now + 59_000 }, event => events.push(event)) : undefined; + if (capture) installDatabaseResponseCapture(db, schema, capture); + const samples = []; + try { + for (let i = 0; i < sampleCount; i++) { + const variants = baselineService ? (i % 2 ? ["candidate", "baseline"] : ["baseline", "candidate"]) : ["candidate"]; + for (const variant of variants) { + const requestUrl = baselineService ? `http://127.0.0.1:${address.port}/paired/${variant}` : url; + const result = await request(i % 2 ? first.etag! : undefined, requestUrl); + if (result.status === 200) assert.equal(result.bodyHash, first.bodyHash, "Baseline and candidate must return the complete same list"); + samples.push({ variant, pair: i, ...result }); + console.error(JSON.stringify({ sample: i, variant, elapsedMs: result.elapsedMs, queries: result.queryCount, status: result.status })); + } + } + if (capture) assert.ok(Date.now() < now + 59_000, "Capture expired; repeat with fewer samples or less injected delay"); + } finally { capture?.flush(); } + assert.equal(await sourceHash(), measuredSourceHash, "Source changed during measurement"); + const timings = samples.map(sample => sample.elapsedMs).sort((a, b) => a - b); + const summary = events.find(event => event.event === "paperclip.db_response_capture.summary.v1"); + if (capture) assert.ok(summary && summary.inFlightAtEnd === 0 && summary.overflowCalls === 0); + const rowCount = (await db.select().from(companySkills).where(eq(companySkills.companyId, companyId))).length; + console.log(JSON.stringify({ schema: "paperclip.skill-list-benchmark.v1", sourceRevision, measuredSourceHash, + fixture: { remoteSkills: 155, totalSkills: rowCount, agents: 40, nonterminatedAgents: 39, projects: 14, sources: 10 }, + captureValues, networkDelayMsPerDirection: delayMs, postgresVersion: await db.$client`select version()`, + boundary: "production router and real service; synthetic authenticated board actor; postgres-js response values", + medianMs: timings[Math.floor(timings.length / 2)], minMs: timings[0], maxMs: timings.at(-1), + baselineRevision, baselineSourceHash, + baselineDependencyScope: baselineService ? "Only company-skills.ts differs; imported helpers and runtime dependencies use this checkout for both versions" : null, + variantResults: ["baseline", "candidate"].flatMap(variant => { + const selected = samples.filter(sample => sample.variant === variant); + if (!selected.length) return []; + const durations = selected.map(sample => sample.elapsedMs).sort((a, b) => a - b); + return [{ variant, samples: selected.length, medianMs: durations[Math.floor(durations.length / 2)], minMs: durations[0], maxMs: durations.at(-1) }]; + }), + samples, responseCapture: events }, null, 2)); +} finally { + const failures: unknown[] = []; + const clean = async (operation: () => Promise) => { try { await operation(); } catch (error) { failures.push(error); } }; + await clean(async () => { if (baselineFile) await fs.rm(baselineFile, { force: true }); }); + await clean(async () => { if (server) { server.closeAllConnections(); await new Promise(resolve => server!.close(() => resolve())); } }); + await clean(async () => db?.$client.end({ timeout: 1 })); + for (const socket of sockets) socket.destroy(); + await clean(async () => { if (proxy) await new Promise(resolve => proxy!.close(() => resolve())); }); + await clean(async () => database?.cleanup()); + await clean(async () => provider?.shutdown()); + trace.disable(); context.disable(); manager?.disable(); + for (const [name, value] of Object.entries({ PAPERCLIP_HOME: previous.home, PAPERCLIP_INSTANCE_ID: previous.instance, OTEL_EXPORTER_OTLP_ENDPOINT: previous.endpoint })) { + if (value === undefined) delete process.env[name]; else process.env[name] = value; + } + await clean(() => fs.rm(home, { recursive: true, force: true })); + if (failures.length) throw new AggregateError(failures, "Local benchmark cleanup failed"); +} diff --git a/server/src/__tests__/company-skills-service.test.ts b/server/src/__tests__/company-skills-service.test.ts index c1c9a448232..08961fe7ec6 100644 --- a/server/src/__tests__/company-skills-service.test.ts +++ b/server/src/__tests__/company-skills-service.test.ts @@ -8,6 +8,7 @@ import { agents, authUsers, companies, + costEvents, companySkillVersions, companySkills, createDb, @@ -662,6 +663,78 @@ describeEmbeddedPostgres("companySkillService.list", () => { }); }); + it("counts current skill attachments without loading unrelated agent payloads or spend", async () => { + const companyId = randomUUID(), otherCompanyId = randomUUID(); + await db.insert(companies).values([ + { id: companyId, name: "Attachment projections", issuePrefix: "ATP" }, + { id: otherCompanyId, name: "Other attachments", issuePrefix: "ATO" }, + ]); + const [first, second, ambiguous] = await db.insert(companySkills).values([ + { companyId, key: "fixture/first", slug: "shared", name: "First", markdown: "# First", sourceType: "github" }, + { companyId, key: "fixture/second", slug: "second", name: "Second", markdown: "# Second", sourceType: "github" }, + { companyId, key: "fixture/ambiguous", slug: "shared", name: "Ambiguous", markdown: "# Ambiguous", sourceType: "github" }, + ]).returning(); + if (!first || !second || !ambiguous) throw new Error("Expected attachment fixture"); + const unused = "unused-agent-context".repeat(6_000); + const readerId = randomUUID(); + await db.insert(agents).values([ + { id: readerId, companyId, name: "Reader", adapterType: "process", status: "idle", + adapterConfig: { instructions: unused, paperclipSkillSync: { desiredSkills: [first.key, second.id, second.slug, first.key] } }, + runtimeConfig: { unused }, metadata: { unused } }, + { companyId, name: "Paused reader", adapterType: "process", status: "paused", + adapterConfig: { instructions: unused, paperclipSkillSync: { desiredSkills: [{ key: first.id, versionId: randomUUID() }] } } }, + { companyId, name: "Ambiguous reader", adapterType: "process", status: "idle", + adapterConfig: { paperclipSkillSync: { desiredSkills: ["shared"] } } }, + { companyId, name: "Malformed selection", adapterType: "process", status: "idle", + adapterConfig: { paperclipSkillSync: [first.key] } }, + { companyId, name: "Terminated reader", adapterType: "process", status: "terminated", + adapterConfig: { instructions: unused, paperclipSkillSync: { desiredSkills: [first.key] } } }, + { companyId: otherCompanyId, name: "Other company reader", adapterType: "process", status: "idle", + adapterConfig: { paperclipSkillSync: { desiredSkills: [first.key] } } }, + ]); + await svc.list(companyId); + const events: Record[] = [], now = Date.now(); + const capture = createDatabaseResponseCapture({ start: now - 1, end: now + 59_000 }, event => events.push(event)); + installDatabaseResponseCapture(db, { agents, costEvents }, capture); + let listed: Awaited>; + try { listed = await svc.list(companyId); } finally { capture.flush(); } + expect(listed.find(item => item.id === first.id)?.attachedAgentCount).toBe(2); + expect(listed.find(item => item.id === second.id)?.attachedAgentCount).toBe(1); + expect(listed.find(item => item.id === ambiguous.id)?.attachedAgentCount).toBe(0); + const agentReads = events.filter(event => event.event === "paperclip.db_response_capture.entry.v1" && event.table === "agents"); + expect(agentReads).toHaveLength(1); + expect(Number(agentReads[0]?.responseValueBytes)).toBeLessThan(2_000); + expect(events.filter(event => event.event === "paperclip.db_response_capture.entry.v1" && event.table === "cost_events")).toHaveLength(0); + await db.update(agents).set({ adapterConfig: { paperclipSkillSync: { desiredSkills: [second.key] } } }).where(eq(agents.id, readerId)); + const refreshed = await svc.list(companyId, { q: "First" }); + expect(refreshed.find(item => item.id === first.id)?.attachedAgentCount).toBe(1); + }); + + it("keeps folder classification and stored content while narrowing inventory refresh reads", async () => { + const companyId = randomUUID(); + await db.insert(companies).values({ id: companyId, name: "Inventory projections", issuePrefix: "IVP" }); + const unused = "unused".repeat(11_000); + const savedMarkdown = "# Required detail content\n".repeat(2_000); + const inserted = await db.insert(companySkills).values(Array.from({ length: 10 }, (_, index) => ({ + companyId, key: `fixture/inventory-${index}`, slug: `inventory-${index}`, name: `Inventory ${index}`, + sourceType: "github", markdown: savedMarkdown, + metadata: { unused, ...(index === 0 ? { sourceKind: " paperclip_bundled " } : {}) }, + }))).returning(); + const before = await svc.list(companyId); + expect(before.find(item => item.id === inserted[0]!.id)?.folderPath).toBe("bundled/paperclip-core"); + const events: Record[] = [], now = Date.now(); + const capture = createDatabaseResponseCapture({ start: now - 1, end: now + 59_000 }, event => events.push(event)); + installDatabaseResponseCapture(db, { companySkills }, capture); + let after: Awaited>; + try { after = await svc.list(companyId); } finally { capture.flush(); } + expect(after).toEqual(before); + const reads = events.filter(event => event.event === "paperclip.db_response_capture.entry.v1" && event.table === "company_skills"); + expect(reads.reduce((sum, event) => sum + Number(event.responseValueBytes), 0)).toBeLessThan(950_000); + const stored = await svc.getById(companyId, inserted[0]!.id); + expect(stored?.markdown).toBe(savedMarkdown); + expect(stored?.metadata).toEqual({ unused, sourceKind: " paperclip_bundled " }); + }); + it("does not retouch unchanged bundled skills during list refresh", async () => { const companyId = randomUUID(); await db.insert(companies).values({ diff --git a/server/src/services/company-skills.ts b/server/src/services/company-skills.ts index 1a874631923..a0926193ce4 100644 --- a/server/src/services/company-skills.ts +++ b/server/src/services/company-skills.ts @@ -12,7 +12,7 @@ import path from "node:path"; import os from "node:os"; import { skillFileBytes, snapshotFile, assertSkillSnapshotPath, skillSnapshotHash } from "./skill-snapshot.js"; import { fileURLToPath } from "node:url"; -import { and, asc, desc, eq, inArray, isNull, isNotNull, lt, sql } from "drizzle-orm"; +import { and, asc, desc, eq, inArray, isNull, isNotNull, lt, ne, sql } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; import { agents as agentsTable, @@ -3047,9 +3047,9 @@ export function companySkillService(db: Db, options: { actor?: AuthorizationActo }))) .catch(() => [] as ImportedSkill[]); if (bundledSkills.length === 0) continue; - return upsertImportedSkills(companyId, [...bundledSkills, firstTaskSkill]); + return upsertImportedSkills(companyId, [...bundledSkills, firstTaskSkill], db, { includeFolderPath: false }); } - return upsertImportedSkills(companyId, [firstTaskSkill]); + return upsertImportedSkills(companyId, [firstTaskSkill], db, { includeFolderPath: false }); } async function readBundledSkillReleaseRegistry() { @@ -3132,7 +3132,7 @@ export function companySkillService(db: Db, options: { actor?: AuthorizationActo id: companySkills.id, key: companySkills.key, folderId: companySkills.folderId, - metadata: companySkills.metadata, + metadata: sql>`jsonb_build_object('sourceKind', ${companySkills.metadata}->'sourceKind')`, }) .from(companySkills) .where(eq(companySkills.companyId, companyId)) @@ -3172,7 +3172,7 @@ export function companySkillService(db: Db, options: { actor?: AuthorizationActo metadata: companySkills.metadata, }) .from(companySkills) - .where(eq(companySkills.companyId, companyId)); + .where(and(eq(companySkills.companyId, companyId), eq(companySkills.sourceType, "local_path"))); const skills = rows.map((row) => ({ ...row, sourceType: row.sourceType as CompanySkillSourceType, @@ -3322,7 +3322,15 @@ export function companySkillService(db: Db, options: { actor?: AuthorizationActo ? await folderSvc.descendantIds(companyId, "skill", query.folderId) : new Set([query.folderId]); } - const agentRows = await agents.list(companyId); + const agentRows = await db.select({ + desiredSkills: sql`${agentsTable.adapterConfig}->'paperclipSkillSync'->'desiredSkills'`, + }).from(agentsTable).where(and(eq(agentsTable.companyId, companyId), ne(agentsTable.status, "terminated"))); + const attachedAgentCounts = new Map(); + for (const agent of agentRows) { + for (const key of resolveDesiredSkillKeys(rows, { paperclipSkillSync: { desiredSkills: agent.desiredSkills } })) { + attachedAgentCounts.set(key, (attachedAgentCounts.get(key) ?? 0) + 1); + } + } const q = query.q?.trim().toLowerCase() ?? ""; const categories = new Set( (query.categories ?? []) @@ -3348,13 +3356,7 @@ export function companySkillService(db: Db, options: { actor?: AuthorizationActo } return true; }); - const items = filtered.map((skill) => { - const attachedAgentCount = agentRows.filter((agent) => { - const desiredSkills = resolveDesiredSkillKeys(rows, agent.adapterConfig as Record); - return desiredSkills.includes(skill.key); - }).length; - return toCompanySkillListItem(skill, attachedAgentCount); - }); + const items = filtered.map((skill) => toCompanySkillListItem(skill, attachedAgentCounts.get(skill.key) ?? 0)); const sort = query.sort ?? "alphabetical"; items.sort((left, right) => { if (sort === "recent") return right.updatedAt.getTime() - left.updatedAt.getTime() || left.name.localeCompare(right.name); @@ -3424,13 +3426,20 @@ export function companySkillService(db: Db, options: { actor?: AuthorizationActo return row ? enrichFolderPath(companyId, toCompanySkill(row)) : null; } - async function getByKey(companyId: string, key: string, database: DbOrTransaction = db) { + async function getByKey( + companyId: string, + key: string, + database: DbOrTransaction = db, + options: { includeFolderPath?: boolean } = {}, + ) { const row = await database .select(selectCompanySkillColumns()) .from(companySkills) .where(and(eq(companySkills.companyId, companyId), eq(companySkills.key, key), await visibility(database).condition(companyId))) .then((rows) => rows[0] ?? null); - return row ? enrichFolderPath(companyId, toCompanySkill(row)) : null; + if (!row) return null; + const skill = toCompanySkill(row); + return options.includeFolderPath === false ? skill : enrichFolderPath(companyId, skill); } async function getBySlugIfUnique(companyId: string, slug: string) { @@ -6383,13 +6392,14 @@ export function companySkillService(db: Db, options: { actor?: AuthorizationActo companyId: string, imported: ImportedSkill[], database: DbOrTransaction = db, + options: { includeFolderPath?: boolean } = {}, ): Promise { const out: CompanySkill[] = []; const bundledFolders = new Map>>(); for (const skill of imported) { assertImportedSkillKeyAllowed(skill); assertImportedSkillSourceAllowed(skill); - const existing = await getByKey(companyId, skill.key, database); + const existing = await getByKey(companyId, skill.key, database, options); const existingMeta = existing ? getSkillMeta(existing) : {}; const incomingMeta = skill.metadata && isPlainRecord(skill.metadata) ? skill.metadata : {}; const incomingOwner = asString(incomingMeta.owner);