mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-19 10:36:26 +02:00
## Thinking Path > - Paperclip manages AI agents and prepares their runtime inputs before each turn. > - Shared company skills are part of those inputs for native and legacy adapters. > - Runtime materialization refreshed the full inventory again for every declared file. > - Remote skill directories were also downloaded and rebuilt on every turn. > - Measured preparation took 42–73 seconds while runner execution took 7–9 seconds. > - This change reads the inventory once and reuses validated installed revisions. > - Agents retain their selected skills while repeated preparation avoids upstream work. ## Linked Issues or Issue Description **What happened?** One 114-skill preparation performed 407 inventory refreshes, 48 directory rebuilds, and 388 GitHub file fetches. Reusing existing local copies took 151 ms. **Expected behavior** Each listing refreshes inventory once. Unchanged installed remote revisions reuse complete, validated local copies. Local edits remain visible. Explicit updates select new revisions. **Steps to reproduce** 1. Import GitHub skills with supporting files. 2. Run an agent turn, then run another with the same installed revisions. 3. Observe repeated inventory scans, downloads, and runtime directory replacement before execution. Related prior attempts: #2330 and #9268 (still open; #9268 last updated July 9). Those use a marker compared with `updatedAt`. This patch follows the required content validation, immutable revision, company isolation, atomic publication, and read-only semantics, and removes refresh-per-file multiplication. ## What Changed - Split public file reading from reading an already loaded skill. Runtime listing refreshes inventory once. - Add a company-scoped revision cache with file manifests outside the delivered skill directory. Fingerprints omit cosmetic metadata. - Validate exact file inventory, sizes, and hashes before warm reuse. Reject traversal and symlinks. Stage complete builds and serialize atomic publication across processes. - Preserve local/catalog direct sources, stored Markdown fallback, explicit version snapshots, and legacy mutable-ref compatibility. Report missing supporting files and keep older valid revisions readable. - Clean both runtime layouts on rename/removal and record `skills.prepare` under preparation timing. - Add service/cache regressions and an isolated 114-skill benchmark, including a new-process warm run. ## Verification - Final targeted skill-service/cache/trace validation: 86 tests pass (61 embedded-PostgreSQL service tests, 19 cache tests, 6 trace tests). Database tests executed rather than skipped. Focused skill routes, adapter selection, and native runtime context also pass. - `pnpm -r typecheck` and `pnpm build` pass locally at `22caa1fe4`. - The full `pnpm test:run` matrix passes on supported Linux CI at the final head: [CI run](https://github.com/paperclipai/paperclip/actions/runs/34236097762). Local full-suite execution encountered PostgreSQL startup contention, a random allocated-port boundary, and a socket hang-up; every affected suite passed on an isolated rerun. The interrupted local serialized run is not claimed as a complete local pass. - Repeatable benchmark: `pnpm --filter @paperclipai/server exec tsx ../scripts/benchmark-skill-preparation.ts`. Mixed 114-skill inventory with 429 remote files on Linux: cold 286 ms, warm median 96 ms / maximum 153 ms including a new process. Every warm sample performs one refresh, zero upstream fetches/rebuilds, and reports no missing entries; content assertions pass. - Controlled deployment against the previously deployed revision completed with zero lost runs. Real inventory: 114 skills, 670 declared files; 402 cached files match the prior installed copies byte-for-byte. Ten post-deployment warm preparations: median 129 ms / maximum 208 ms; new-process warm 194 ms, zero downloads/rebuilds/missing entries. - Five sequential real browser questions persisted in 10.6–20.7 s (median 12.2 s), versus 50–83 s before. Skill preparation median 240 ms, with one 2.37 s outlier. Total preparation median 3.337 s / maximum 8.728 s **does not fully meet** the <3 s / <5 s target. The excluded historical-run redaction query takes about 1.36 s per scan at two preparation call sites; wider application latency coincided with the outlier, without a cache rebuild. These residuals are reported rather than discarded. - Disposable skill reimport verified through actual selected-skill runs: the next run read the changed code. Fixture removed and agent configuration verified unchanged. - Greptile 5/5, zero unresolved review threads, all final-head CI checks green. ## Risks - Cold preparation still requires upstream availability for supporting files. An unavailable revision is reported missing and never falls back to an older revision. - Valid older revisions and quarantined invalid entries consume additive disk space until skill cleanup. An abruptly killed publisher can leave a lock that requires operator cleanup after confirming its PID is dead. - Warm validation reads all cached file bytes. Very large inventories still have proportional local I/O cost. - No HTTP API, schema, agent configuration, or first-party Telemetry changes. OpenTelemetry retains its operator endpoint gate. ## Model Used OpenAI GPT-6 in Codex, with reasoning, repository inspection, code editing, and test execution. The exact serving snapshot and context-window size are not exposed in this session. ## 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 (targeted and isolated reruns; full Linux CI matrix passes, local full-run caveats above) - [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>
326 lines
13 KiB
TypeScript
326 lines
13 KiB
TypeScript
import { mkdtemp, mkdir, readFile, readdir, chmod, lstat, rm, stat, writeFile } from "node:fs/promises";
|
||
import { tmpdir } from "node:os";
|
||
import path from "node:path";
|
||
import type { Db } from "@paperclipai/db";
|
||
import {
|
||
PAPERCLIP_OPERATIONAL_SKILL_KEY,
|
||
type PaperclipSkillEntry,
|
||
} from "@paperclipai/adapter-utils/server-utils";
|
||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||
|
||
const serviceMocks = vi.hoisted(() => ({
|
||
exportFiles: vi.fn(),
|
||
getEffectiveProfilesForAgent: vi.fn(),
|
||
}));
|
||
|
||
vi.mock("../agent-instructions.js", () => ({
|
||
agentInstructionsService: () => ({ exportFiles: serviceMocks.exportFiles }),
|
||
}));
|
||
|
||
vi.mock("../tool-access.js", () => ({
|
||
toolAccessService: () => ({
|
||
getEffectiveProfilesForAgent: serviceMocks.getEffectiveProfilesForAgent,
|
||
}),
|
||
}));
|
||
|
||
import { createHash } from "node:crypto";
|
||
import { buildNativeRuntimeContext, resolveNativeRuntimeMcpSnapshot } from "./runtime-context.js";
|
||
|
||
const temporaryRoots: string[] = [];
|
||
let previousPaperclipHome: string | undefined;
|
||
let previousInstanceId: string | undefined;
|
||
|
||
async function makeTreeWritable(target: string): Promise<void> {
|
||
const metadata = await lstat(target).catch(() => null);
|
||
if (!metadata) return;
|
||
if (metadata.isDirectory()) {
|
||
await chmod(target, 0o700);
|
||
for (const entry of await readdir(target)) {
|
||
await makeTreeWritable(path.join(target, entry));
|
||
}
|
||
} else {
|
||
await chmod(target, 0o600);
|
||
}
|
||
}
|
||
|
||
beforeEach(async () => {
|
||
vi.clearAllMocks();
|
||
previousPaperclipHome = process.env.PAPERCLIP_HOME;
|
||
previousInstanceId = process.env.PAPERCLIP_INSTANCE_ID;
|
||
const root = await mkdtemp(path.join(tmpdir(), "paperclip-native-context-"));
|
||
temporaryRoots.push(root);
|
||
process.env.PAPERCLIP_HOME = root;
|
||
process.env.PAPERCLIP_INSTANCE_ID = "runtime_context_test";
|
||
serviceMocks.getEffectiveProfilesForAgent.mockResolvedValue({
|
||
agentId: "agent-1",
|
||
profiles: [],
|
||
entries: [{ effect: "include", connectionId: "connection-1" }],
|
||
bindings: [],
|
||
allowedTools: [{ id: "tool-1", connectionId: "connection-1" }],
|
||
allowedToolNames: ["issues.read"],
|
||
installedConnections: [{
|
||
id: "connection-1",
|
||
transport: "mcp_remote",
|
||
enabled: true,
|
||
status: "active",
|
||
healthStatus: "healthy",
|
||
}],
|
||
});
|
||
});
|
||
|
||
afterEach(async () => {
|
||
if (previousPaperclipHome === undefined) delete process.env.PAPERCLIP_HOME;
|
||
else process.env.PAPERCLIP_HOME = previousPaperclipHome;
|
||
if (previousInstanceId === undefined) delete process.env.PAPERCLIP_INSTANCE_ID;
|
||
else process.env.PAPERCLIP_INSTANCE_ID = previousInstanceId;
|
||
// A rejected Promise.all does not cancel the other materializers. Let their
|
||
// bounded local writes settle before removing the read-only asset tree.
|
||
await new Promise((resolve) => setTimeout(resolve, 25));
|
||
while (temporaryRoots.length) {
|
||
const root = temporaryRoots.pop()!;
|
||
await makeTreeWritable(root);
|
||
await rm(root, { recursive: true, force: true });
|
||
}
|
||
});
|
||
|
||
describe("buildNativeRuntimeContext", () => {
|
||
it.each(["disabled", "degraded"] as const)(
|
||
"omits an unavailable native MCP connection when it is %s without aborting runtime context creation",
|
||
async (unavailableState) => {
|
||
serviceMocks.exportFiles.mockResolvedValue({
|
||
entryFile: "AGENTS.md",
|
||
files: { "AGENTS.md": "Continue work without unavailable apps.\n" },
|
||
});
|
||
serviceMocks.getEffectiveProfilesForAgent.mockResolvedValue({
|
||
agentId: "agent-1",
|
||
profiles: [],
|
||
entries: [{ effect: "include", connectionId: "connection-1" }],
|
||
bindings: [],
|
||
allowedTools: [{ id: "tool-1", connectionId: "connection-1" }],
|
||
allowedToolNames: ["issues.read"],
|
||
installedConnections: [{
|
||
id: "connection-1",
|
||
transport: "mcp_remote",
|
||
enabled: unavailableState !== "disabled",
|
||
status: unavailableState === "disabled" ? "disabled" : "active",
|
||
healthStatus: unavailableState === "degraded" ? "degraded" : "healthy",
|
||
}],
|
||
});
|
||
|
||
const context = await buildNativeRuntimeContext({
|
||
db: {} as Db,
|
||
agent: {
|
||
id: "agent-1",
|
||
companyId: "company-1",
|
||
name: "Reviewer",
|
||
adapterType: "paperclip_runner",
|
||
adapterConfig: {},
|
||
},
|
||
runId: "run-1",
|
||
runtimeConfig: {},
|
||
runtimeSkillEntries: [],
|
||
});
|
||
|
||
expect(context.mcp.bindingId).toBeNull();
|
||
expect(context.mcp.assignmentSetId).toMatch(/^sha256:[a-f0-9]{64}$/);
|
||
},
|
||
);
|
||
|
||
it("keeps healthy native MCP connections when another assigned connection is unavailable", async () => {
|
||
serviceMocks.exportFiles.mockResolvedValue({
|
||
entryFile: "AGENTS.md",
|
||
files: { "AGENTS.md": "Continue work with the apps that are available.\n" },
|
||
});
|
||
serviceMocks.getEffectiveProfilesForAgent.mockResolvedValue({
|
||
agentId: "agent-1",
|
||
profiles: [],
|
||
entries: [
|
||
{ effect: "include", connectionId: "connection-expired" },
|
||
{ effect: "include", connectionId: "connection-healthy" },
|
||
],
|
||
bindings: [],
|
||
allowedTools: [
|
||
{ id: "tool-expired", connectionId: "connection-expired" },
|
||
{ id: "tool-healthy", connectionId: "connection-healthy" },
|
||
],
|
||
allowedToolNames: ["expired.read", "healthy.read"],
|
||
installedConnections: [
|
||
{
|
||
id: "connection-expired",
|
||
transport: "mcp_remote",
|
||
enabled: true,
|
||
status: "active",
|
||
healthStatus: "degraded",
|
||
},
|
||
{
|
||
id: "connection-healthy",
|
||
transport: "mcp_remote",
|
||
enabled: true,
|
||
status: "active",
|
||
healthStatus: "healthy",
|
||
},
|
||
],
|
||
});
|
||
|
||
const context = await buildNativeRuntimeContext({
|
||
db: {} as Db,
|
||
agent: {
|
||
id: "agent-1",
|
||
companyId: "company-1",
|
||
name: "Reviewer",
|
||
adapterType: "paperclip_runner",
|
||
adapterConfig: {},
|
||
},
|
||
runId: "run-1",
|
||
runtimeConfig: {},
|
||
runtimeSkillEntries: [],
|
||
});
|
||
|
||
expect(context.mcp.bindingId).toBe("native-mcp:run-1");
|
||
expect(context.mcp.assignmentSetId).toMatch(/^sha256:[a-f0-9]{64}$/);
|
||
});
|
||
|
||
it("materializes every instruction and selected-skill file as immutable, content-addressed context", async () => {
|
||
serviceMocks.exportFiles.mockResolvedValue({
|
||
entryFile: "AGENTS.md",
|
||
files: {
|
||
"AGENTS.md": "Follow the agent instructions.\n",
|
||
"references/policy.md": "Company policy sibling.\n",
|
||
},
|
||
});
|
||
const sourceRoot = await mkdtemp(path.join(tmpdir(), "paperclip-native-skill-"));
|
||
temporaryRoots.push(sourceRoot);
|
||
const selectedRoot = path.join(sourceRoot, "reviewer");
|
||
const unselectedRoot = path.join(sourceRoot, "unused");
|
||
await mkdir(path.join(selectedRoot, "references"), { recursive: true });
|
||
await mkdir(unselectedRoot, { recursive: true });
|
||
await writeFile(path.join(selectedRoot, "SKILL.md"), "# Reviewer\nUse the checklist.\n");
|
||
await writeFile(path.join(selectedRoot, "references", "checklist.md"), "- Verify tests\n");
|
||
await writeFile(path.join(unselectedRoot, "SKILL.md"), "# Not selected\n");
|
||
const entries: PaperclipSkillEntry[] = [
|
||
{ key: "company-1/reviewer", runtimeName: "reviewer", source: selectedRoot, versionId: "version-1" },
|
||
{ key: "company-1/unused", runtimeName: "unused", source: unselectedRoot, versionId: "version-2" },
|
||
];
|
||
const input = {
|
||
db: {} as Db,
|
||
agent: {
|
||
id: "agent-1",
|
||
companyId: "company-1",
|
||
name: "Reviewer",
|
||
adapterType: "paperclip_runner",
|
||
adapterConfig: {},
|
||
},
|
||
runId: "run-1",
|
||
runtimeConfig: { paperclipSkillSync: { desiredSkills: ["company-1/reviewer"] } },
|
||
runtimeSkillEntries: entries,
|
||
};
|
||
|
||
const context = await buildNativeRuntimeContext(input);
|
||
const repeated = await buildNativeRuntimeContext(input);
|
||
|
||
expect(serviceMocks.exportFiles).toHaveBeenCalledWith(input.agent, { rejectSymlinks: true });
|
||
expect(serviceMocks.getEffectiveProfilesForAgent).toHaveBeenCalledWith("company-1", "agent-1");
|
||
expect(context.instructions.entryPath).toBe("AGENTS.md");
|
||
expect(await readFile(path.join(context.instructions.bundle.rootPath, "AGENTS.md"), "utf8"))
|
||
.toBe("Follow the agent instructions.\n");
|
||
expect(await readFile(path.join(context.instructions.bundle.rootPath, "references", "policy.md"), "utf8"))
|
||
.toBe("Company policy sibling.\n");
|
||
const unselected = await buildNativeRuntimeContext({ ...input, runtimeConfig: {} });
|
||
expect(unselected.skills).toEqual([]);
|
||
expect(context.skills).toHaveLength(1);
|
||
expect(context.skills[0]).toMatchObject({
|
||
key: "company-1/reviewer",
|
||
runtimeName: "reviewer",
|
||
versionId: "version-1",
|
||
});
|
||
expect(await readFile(path.join(context.skills[0]!.bundle.rootPath, "SKILL.md"), "utf8"))
|
||
.toContain("Use the checklist");
|
||
expect(await readFile(path.join(context.skills[0]!.bundle.rootPath, "references", "checklist.md"), "utf8"))
|
||
.toBe("- Verify tests\n");
|
||
expect((await stat(context.instructions.bundle.rootPath)).mode & 0o222).toBe(0);
|
||
expect((await stat(path.join(context.instructions.bundle.rootPath, "AGENTS.md"))).mode & 0o222).toBe(0);
|
||
expect((await stat(context.skills[0]!.bundle.rootPath)).mode & 0o222).toBe(0);
|
||
expect(context.mcp).toMatchObject({
|
||
bindingId: "native-mcp:run-1",
|
||
assignmentSetId: expect.stringMatching(/^sha256:[a-f0-9]{64}$/),
|
||
digest: expect.stringMatching(/^[a-f0-9]{64}$/),
|
||
});
|
||
expect(repeated.aggregateDigest).toBe(context.aggregateDigest);
|
||
expect(repeated.instructions.bundle.rootPath).toBe(context.instructions.bundle.rootPath);
|
||
expect(repeated.skills[0]!.bundle.rootPath).toBe(context.skills[0]!.bundle.rootPath);
|
||
});
|
||
|
||
it("fails closed for a missing assigned skill and omits a stale legacy operational skill", async () => {
|
||
serviceMocks.exportFiles.mockResolvedValue({ entryFile: "AGENTS.md", files: { "AGENTS.md": "Test\n" } });
|
||
const base = {
|
||
db: {} as Db,
|
||
agent: {
|
||
id: "agent-1",
|
||
companyId: "company-1",
|
||
name: "Reviewer",
|
||
adapterType: "paperclip_runner",
|
||
adapterConfig: {},
|
||
},
|
||
runId: "run-1",
|
||
};
|
||
await expect(buildNativeRuntimeContext({
|
||
...base,
|
||
runtimeConfig: { paperclipSkillSync: { desiredSkills: ["company-1/missing"] } },
|
||
runtimeSkillEntries: [{
|
||
key: "company-1/missing",
|
||
runtimeName: "missing",
|
||
source: "/does/not/exist",
|
||
sourceStatus: "missing",
|
||
missingDetail: "assigned skill checkout is unavailable",
|
||
}],
|
||
})).rejects.toThrow("assigned skill checkout is unavailable");
|
||
const supportedRoot = await mkdtemp(path.join(tmpdir(), "paperclip-native-supported-skill-"));
|
||
temporaryRoots.push(supportedRoot);
|
||
await writeFile(path.join(supportedRoot, "SKILL.md"), "# Supported\n");
|
||
const context = await buildNativeRuntimeContext({
|
||
...base,
|
||
runtimeConfig: {
|
||
paperclipSkillSync: {
|
||
desiredSkills: [PAPERCLIP_OPERATIONAL_SKILL_KEY, "company-1/supported"],
|
||
},
|
||
},
|
||
runtimeSkillEntries: [
|
||
{
|
||
key: PAPERCLIP_OPERATIONAL_SKILL_KEY,
|
||
runtimeName: "paperclip",
|
||
source: "/unused",
|
||
},
|
||
{
|
||
key: "company-1/supported",
|
||
runtimeName: "supported",
|
||
source: supportedRoot,
|
||
},
|
||
],
|
||
});
|
||
expect(context.skills.map((skill) => skill.key)).toEqual(["company-1/supported"]);
|
||
});
|
||
});
|
||
|
||
|
||
it("pins both permitted GitHub tool catalogs even when another user’s health probe reports missing credentials", async () => {
|
||
const connections = ["github-A", "github-B"].map((id) => ({
|
||
id, status: "active", enabled: true, healthStatus: "missing_secret", transport: "mcp_remote",
|
||
config: { sourceTemplateKey: "github" }, transportConfig: {},
|
||
}));
|
||
serviceMocks.getEffectiveProfilesForAgent.mockResolvedValue({
|
||
entries: connections.map((connection) => ({ effect: "include", connectionId: connection.id })),
|
||
installedConnections: connections,
|
||
allowedTools: connections.map((connection) => ({ id: `${connection.id}-get_me`, connectionId: connection.id })),
|
||
});
|
||
const limit = vi.fn(async () => [{ responsibleUserId: "A", activeIdentityContextId: "context-A" }]);
|
||
const db = { select: () => ({ from: () => ({ where: () => ({ limit }) }) }) } as unknown as Db;
|
||
const snapshot = await resolveNativeRuntimeMcpSnapshot({ db, agent: { id: "agent-1", companyId: "company-1" }, runId: "run-1" });
|
||
const expected = createHash("sha256").update(JSON.stringify({
|
||
version: 1, agentId: "agent-1", connections: ["github-A", "github-B"],
|
||
tools: ["github-A-get_me", "github-B-get_me"],
|
||
})).digest("hex");
|
||
expect(snapshot.digest).toBe(expected);
|
||
expect(snapshot.bindingId).toBe("native-mcp:run-1");
|
||
expect(limit).toHaveBeenCalledOnce();
|
||
});
|