From c9d04cba601541cca506f5b72aa6f7272a25379f Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Fri, 6 Nov 2026 19:17:18 -0600 Subject: [PATCH] fix(runner): address skill loading review findings --- doc/runner-created-skills.md | 7 +- ...ve-skill-library-tools.integration.test.ts | 72 ++++++++++++++++++- .../native-skill-library-tools.ts | 57 +++++++++++---- .../paperclip-runner-tool-authority.ts | 7 +- server/src/services/skill-package-runtime.ts | 27 ++++--- server/src/services/skill-package-store.ts | 2 +- 6 files changed, 143 insertions(+), 29 deletions(-) diff --git a/doc/runner-created-skills.md b/doc/runner-created-skills.md index 42c6d5f78d3..a52c5eb68b7 100644 --- a/doc/runner-created-skills.md +++ b/doc/runner-created-skills.md @@ -26,8 +26,11 @@ return an error instead of silently substituting different content. Loads use the existing saved package reader, including repository skill wrappers and supporting files. Text files above 16 MiB are rejected before body reads; -accepted files are read in full for each page. Supporting paths use the saved -package limit of 4,096 characters plus the generated repository prefix. +accepted text is cached for the run with a 64 MiB budget and at most 64 files. +Later pages reuse cached text and still check current access. Evicted files are +read again. Repository reads use verified warm files or isolated recovery without +holding the company startup lock. Supporting paths use the saved package limit +of 4,096 characters plus the generated repository prefix. Binary files and unsafe paths are rejected. Access is checked against the current run and the existing company and project source boundaries, including after file I/O. Loaded content does not change persistent diff --git a/server/src/services/native-runtime/native-skill-library-tools.integration.test.ts b/server/src/services/native-runtime/native-skill-library-tools.integration.test.ts index b08ef59c772..6f73bbe1508 100644 --- a/server/src/services/native-runtime/native-skill-library-tools.integration.test.ts +++ b/server/src/services/native-runtime/native-skill-library-tools.integration.test.ts @@ -9,7 +9,8 @@ import { agents, companies, companySkills, companySkillVersions, companySkillSou import { startRecoveryPostgres } from "../../__tests__/helpers/recovery-postgres.js"; import { createDb } from "@paperclipai/db"; import { PaperclipRunnerToolAuthority } from "./paperclip-runner-tool-authority.js"; -import { skillPackageStore } from "../skill-package-store.js"; +import { skillPackagePaths, skillPackageStore } from "../skill-package-store.js"; +import { withSkillVersionCacheLock } from "../skill-version-cache.js"; import { getStorageService } from "../../storage/index.js"; describe("native company skill discovery", () => { @@ -63,6 +64,16 @@ describe("native company skill discovery", () => { expect((await db.select().from(agents).where(eq(agents.id, f.agentId)))[0]?.adapterConfig).toEqual({ provider: "codex" }); }); + it("keeps default inline file reads compatible with existing callers", async () => { + const f = await fixture(), s = await skill(f.companyId); + const store = skillPackageStore(db); + const version = await store.version(f.companyId, s.id, s.versionId); + expect(version).toBeTruthy(); + expect(await store.readVersionFile(version!, "SKILL.md")).toMatchObject({ content: s.text }); + expect(await store.readVersionFile(version!, "references/checks.md", false)) + .toMatchObject({ content: "Check CI and the review result." }); + }); + it("bounds and paginates search results and treats SQL wildcard characters literally", async () => { const f = await fixture(); await skill(f.companyId, "review-a"); await skill(f.companyId, "review-b"); await skill(f.companyId, "review-c"); @@ -108,6 +119,65 @@ describe("native company skill discovery", () => { finally { fault.mockRestore(); } }); + it.each([false, true])("reuses text between pages and still rechecks access (repository: %s)", async repository => { + const f = await fixture(), s = await skill(f.companyId); + const content = "x".repeat(16 * 1024 * 1024); + const packageId = await skillPackageStore(db).prepare(f.companyId, [ + { path: "SKILL.md", kind: "skill", content: "---\nname: review\ndescription: Review.\n---\n\nRead checks." }, + { path: "checks.md", kind: "reference", content }, + ]); + await db.update(companySkillVersions).set({ fileInventory: [], ...(repository + ? { repositorySnapshotId: packageId, repositorySkillPath: "SKILL.md" } + : { packageSnapshotId: packageId }) }).where(eq(companySkillVersions.id, s.versionId)); + const path = `${repository ? ".paperclip-repository/" : ""}checks.md`; + const open = vi.spyOn(fs, "open"), read = vi.spyOn(fs, "readFile"); + const bodyReads = () => [...open.mock.calls, ...read.mock.calls].filter(args => String(args[0]).endsWith("/checks.md")).length; + try { + const first = await f.call("load_skill", { skill: s.id, path }) as any; + const reads = bodyReads(); + expect(reads).toBeGreaterThan(0); + expect(first.byteSize).toBe(content.length); + expect(await f.call("load_skill", { skill: s.id, versionId: s.versionId, path, offset: first.nextOffset })) + .toMatchObject({ text: content.slice(6000, 12000), sha256: null }); + expect(bodyReads()).toBe(reads); + const sourceId = randomUUID(); + await db.insert(companySkillSources).values({ id: sourceId, companyId: f.companyId, repositoryUrl: "https://github.com/example/private", fullName: "example/private", trackingRef: "HEAD", accessScope: "projects" }); + await db.update(companySkills).set({ accessSourceId: sourceId }).where(eq(companySkills.id, s.id)); + expect(await f.call("load_skill", { skill: s.id, versionId: s.versionId, path, offset: 12000 })) + .toMatchObject({ outcome: "failed", code: "runner_skill_not_found" }); + } finally { open.mockRestore(); read.mockRestore(); f.authority.close(); } + }); + + it.each([false, true])("reads repository files without waiting for the startup lock (cold: %s)", async cold => { + const f = await fixture(), s = await skill(f.companyId); + const store = skillPackageStore(db); + const packageId = await store.prepare(f.companyId, [ + { path: "SKILL.md", kind: "skill", content: "---\nname: review\ndescription: Review.\n---\n\nRead checks." }, + { path: "checks.md", kind: "reference", content: "Repository checks." }, + ]); + await db.update(companySkillVersions).set({ fileInventory: [], repositorySnapshotId: packageId, repositorySkillPath: "SKILL.md" }) + .where(eq(companySkillVersions.id, s.versionId)); + if (cold) { + await rm(skillPackagePaths(f.companyId, packageId).directory, { recursive: true, force: true }); + } + let release!: () => void, acquired!: () => void; + const held = new Promise(resolve => { release = resolve; }); + const ready = new Promise(resolve => { acquired = resolve; }); + const lock = withSkillVersionCacheLock(db, f.companyId, async () => { acquired(); await held; }); + await ready; + let timeout: ReturnType | undefined; + try { + await Promise.race([ + (async () => { + expect(await f.call("load_skill", { skill: s.id })).toMatchObject({ path: "SKILL.md" }); + expect(await f.call("load_skill", { skill: s.id, versionId: s.versionId, path: ".paperclip-repository/checks.md" })) + .toMatchObject({ text: "Repository checks." }); + })(), + new Promise((_, reject) => { timeout = setTimeout(() => reject(new Error("Skill read waited for the startup lock")), 3000); }), + ]); + } finally { clearTimeout(timeout); release(); await lock; f.authority.close(); } + }); + it.each(["inline", "package", "repository"])("rejects oversized text before reading its body (%s)", async storage => { const f = await fixture(), s = await skill(f.companyId); const root = storage === "repository" ? "skills/review/" : ""; diff --git a/server/src/services/native-runtime/native-skill-library-tools.ts b/server/src/services/native-runtime/native-skill-library-tools.ts index b78929b5d90..03db8b30ce5 100644 --- a/server/src/services/native-runtime/native-skill-library-tools.ts +++ b/server/src/services/native-runtime/native-skill-library-tools.ts @@ -12,6 +12,33 @@ import { REPOSITORY_CONTENT_DIR } from "../skill-repository-package.js"; import { settleRunnerBridgeRead } from "./runner-bridge-tools.js"; const MAX_TEXT_FILE_BYTES = 16 * 1024 * 1024; +type SkillText = { text: string; sha256: string; byteSize: number }; +/** Owned by one run authority. Retain at most 64 MiB of text and 64 entries. */ +export class SkillLibraryReadCache { + #entries = new Map(); + #bytes = 0; + #closed = false; + #size(key: string, value: SkillText) { return (key.length + value.text.length) * 2 + 128; } + #assertOpen() { if (this.#closed) throw new Error("runner_skill_scope_closed"); } + close() { this.#closed = true; this.#entries.clear(); this.#bytes = 0; } + get(key: string) { + this.#assertOpen(); + const value = this.#entries.get(key); + if (value) { this.#entries.delete(key); this.#entries.set(key, value); } + return value; + } + set(key: string, value: SkillText) { + this.#assertOpen(); + const prior = this.#entries.get(key); + if (prior) { this.#bytes -= this.#size(key, prior); this.#entries.delete(key); } + const bytes = this.#size(key, value); + while (this.#entries.size && (this.#entries.size >= 64 || this.#bytes + bytes > 64 * 1024 * 1024)) { + const oldest = this.#entries.keys().next().value!; + this.#bytes -= this.#size(oldest, this.#entries.get(oldest)!); this.#entries.delete(oldest); + } + if (bytes <= 64 * 1024 * 1024) { this.#entries.set(key, value); this.#bytes += bytes; } + } +} const relativePath = z.string().min(1).max(MAX_SKILL_PACKAGE_PATH_LENGTH + REPOSITORY_CONTENT_DIR.length + 1).refine(value => { try { assertSkillSnapshotPath(value); return true; } catch { return false; } }, "Use a relative skill file path without traversal"); @@ -45,7 +72,7 @@ const summaryColumns = { const fail = (code: string, message: string) => ({ outcome: "failed", code, message }); export async function executeSkillLibraryTool(input: { - db: Db; companyId: string; tool: string; arguments: unknown; + db: Db; companyId: string; tool: string; arguments: unknown; cache: SkillLibraryReadCache; authorize: () => Promise; }) { const actor = await input.authorize(); @@ -87,24 +114,30 @@ export async function executeSkillLibraryTool(input: { const result = await settleRunnerBridgeRead(async () => { const runtime = skillPackageRuntime(input.db); const files = version.repositorySnapshotId ? await runtime.inventory(version) : await packages.versionInventory(version); - let file; - try { - file = version.repositorySnapshotId ? await runtime.readFile(version, args.path, MAX_TEXT_FILE_BYTES) - : await packages.readVersionFile(version, args.path, true, MAX_TEXT_FILE_BYTES); - } catch (error) { - if (error instanceof SkillPackageFileTooLarge) return fail("runner_skill_file_size_limit", "This tool reads text files up to 16 MiB. Use a smaller supporting file."); - throw error; + const cacheKey = JSON.stringify([input.companyId, versionId, args.path]); + let content = input.cache.get(cacheKey); + if (!content) { + let file; + try { + file = version.repositorySnapshotId ? await runtime.readFile(version, args.path, MAX_TEXT_FILE_BYTES) + : await packages.readVersionFile(version, args.path, true, MAX_TEXT_FILE_BYTES); + } catch (error) { + if (error instanceof SkillPackageFileTooLarge) return fail("runner_skill_file_size_limit", "This tool reads text files up to 16 MiB. Use a smaller supporting file."); + throw error; + } + if (!file) return fail("runner_skill_file_not_found", "The requested file is not in this skill version."); + if (file.encoding === "base64") return fail("runner_skill_binary_file", "This tool reads UTF-8 text. The requested skill file is binary."); + content = { text: file.content, sha256: createHash("sha256").update(file.content).digest("hex"), byteSize: Buffer.byteLength(file.content) }; + input.cache.set(cacheKey, content); } - if (!file) return fail("runner_skill_file_not_found", "The requested file is not in this skill version."); - if (file.encoding === "base64") return fail("runner_skill_binary_file", "This tool reads UTF-8 text. The requested skill file is binary."); - const text = file.content; + const { text } = content; const splitsCharacter = (offset: number) => offset > 0 && offset < text.length && /[\uD800-\uDBFF]/u.test(text[offset - 1]!) && /[\uDC00-\uDFFF]/u.test(text[offset]!); if (args.offset > text.length || splitsCharacter(args.offset)) return fail("runner_skill_invalid_offset", "Use nextOffset from the previous page."); let end = Math.min(args.offset + 6000, text.length); if (splitsCharacter(end)) end--; return { ...skill, skillId: skill.id, versionId, path: args.path, text: text.slice(args.offset, end), - sha256: args.offset === 0 && args.fileOffset === 0 ? createHash("sha256").update(text).digest("hex") : null, byteSize: Buffer.byteLength(text), + sha256: args.offset === 0 && args.fileOffset === 0 ? content.sha256 : null, byteSize: content.byteSize, nextOffset: end < text.length ? end : null, files: files.slice(args.fileOffset, args.fileOffset + 50), fileCount: files.length, nextFileOffset: args.fileOffset + 50 < files.length ? args.fileOffset + 50 : null, diff --git a/server/src/services/native-runtime/paperclip-runner-tool-authority.ts b/server/src/services/native-runtime/paperclip-runner-tool-authority.ts index 5530d3e32de..c9b918a8e01 100644 --- a/server/src/services/native-runtime/paperclip-runner-tool-authority.ts +++ b/server/src/services/native-runtime/paperclip-runner-tool-authority.ts @@ -14,7 +14,7 @@ import { executeNativeWorkflowTool } from "./native-workflow-tools.js"; import { assertNoCaseHandoffMutation } from "../heartbeat/case-handoff-lifecycle.js"; import { handoffCaseSchema } from "@paperclipai/shared"; import { skillAccessService } from "../skill-access.js"; -import { SKILL_LIBRARY_SCHEMAS, SKILL_LIBRARY_TOOL_DEFINITIONS, executeSkillLibraryTool } from "./native-skill-library-tools.js"; +import { SKILL_LIBRARY_SCHEMAS, SKILL_LIBRARY_TOOL_DEFINITIONS, SkillLibraryReadCache, executeSkillLibraryTool } from "./native-skill-library-tools.js"; import { withExternalExecutionAdmission, type ExternalAdmissionTransaction } from "../../modules/external-agents/index.js"; import { callTaskWorkspaceTool, pendingTaskRepositoryContinuation } from "../task-workspace-tools.js"; import { TaskAttachmentReadCache, TASK_ATTACHMENT_DEFINITIONS, TASK_ATTACHMENT_SCHEMAS, listTaskAttachments, readTaskAttachment } from "./runner-task-attachments.js"; @@ -314,7 +314,8 @@ function hireAgentResult(value: unknown, provider: unknown) { export class PaperclipRunnerToolAuthority { #taskAttachmentCache = new TaskAttachmentReadCache(); - close() { this.#taskAttachmentCache.close(); } + #skillLibraryCache = new SkillLibraryReadCache(); + close() { this.#taskAttachmentCache.close(); this.#skillLibraryCache.close(); } constructor(readonly db: Db, readonly binding: Binding) {} definitions(): Array> { @@ -599,7 +600,7 @@ export class PaperclipRunnerToolAuthority { } if (SKILL_LIBRARY_SCHEMAS.has(call.tool)) { if (!this.definitions().some(definition => definition.name === call.tool)) throw forbidden("Tool is not available to this run"); - return executeSkillLibraryTool({ db: this.db, companyId: this.binding.companyId, tool: call.tool, arguments: call.arguments, + return executeSkillLibraryTool({ db: this.db, companyId: this.binding.companyId, tool: call.tool, arguments: call.arguments, cache: this.#skillLibraryCache, authorize: async () => { const current = await this.#boundContext({ tool: call.tool }); return this.#privacyActor(current.run); } }); } if (RUNNER_BRIDGE_SCHEMAS.has(call.tool)) { diff --git a/server/src/services/skill-package-runtime.ts b/server/src/services/skill-package-runtime.ts index 49c15ea9708..79e94b26101 100644 --- a/server/src/services/skill-package-runtime.ts +++ b/server/src/services/skill-package-runtime.ts @@ -37,6 +37,12 @@ function originalFiles(version: StoredSkillVersion, manifest: SkillPackageManife } export function skillPackageRuntime(db: Db) { const store = skillPackageStore(db); + function readOriginalFile(version: StoredSkillVersion, sourcePath: string, maxBytes?: number, projected?: ProjectedFile) { + // Use verified warm reads and isolated recovery, without the company-wide + // preparation lock. The projection has already resolved the source path. + return store.readVersionFile({ ...version, packageSnapshotId: version.repositorySnapshotId ?? version.packageSnapshotId, + packageFileMap: projected ? [{ path: sourcePath, sourcePath, kind: projected.kind, encoding: projected.encoding }] : null }, sourcePath, true, maxBytes); + } async function inventory(version: StoredSkillVersion) { const id = version.repositorySnapshotId ?? version.packageSnapshotId; if (!id) throw new Error("Skill version has no package"); @@ -63,13 +69,13 @@ export function skillPackageRuntime(db: Db) { if (!version.repositorySkillPath) throw new Error("Repository skill entrypoint is missing"); const wanted = new Set([version.repositorySkillPath, path.posix.join(path.posix.dirname(version.repositorySkillPath), "agents/openai.yaml")]); for (const file of manifest.files.filter(file => wanted.has(file.path))) assertSkillPackageFileSize(file.size, maxBytes); - const generated = await store.withPackage(version.companyId, id, async (root, manifest) => { - const selected: CompanySkillVersionFileInventoryEntry[] = []; - for (const file of manifest.files.filter(file => wanted.has(file.path))) selected.push({ - ...file, content: (await fs.readFile(path.join(root, file.path))).toString(file.encoding), - }); - return repositorySkillProjection(selected, version.repositorySkillPath!).filter(file => !file.path.startsWith(`${REPOSITORY_CONTENT_DIR}/`)); - }); + const selected: CompanySkillVersionFileInventoryEntry[] = []; + for (const file of manifest.files.filter(file => wanted.has(file.path))) { + const original = await readOriginalFile(version, file.path, maxBytes); + if (!original) throw new Error("Repository skill file is missing"); + selected.push(original); + } + const generated = repositorySkillProjection(selected, version.repositorySkillPath!).filter(file => !file.path.startsWith(`${REPOSITORY_CONTENT_DIR}/`)); for (const file of generated) { const bytes = skillFileBytes(file); generatedFiles.push({ path: file.path, kind: file.kind, encoding: file.encoding ?? "utf8", executable: Boolean(file.executable), @@ -118,9 +124,10 @@ export function skillPackageRuntime(db: Db) { ?? (await projection(version, maxBytes)).find(file => file.path === relative); if (!file) return null; assertSkillPackageFileSize(file.size, maxBytes); - const bytes = file.generated ?? await store.withPackage(version.companyId, - (version.repositorySnapshotId ?? version.packageSnapshotId)!, root => fs.readFile(path.join(root, file.sourcePath!))); - return { path: file.path, kind: file.kind, content: bytes.toString(file.encoding), encoding: file.encoding, executable: file.executable }; + const content = file.generated ? file.generated.toString(file.encoding) + : (await readOriginalFile(version, file.sourcePath!, maxBytes, file))?.content; + if (content === undefined) return null; + return { path: file.path, kind: file.kind, content, encoding: file.encoding, executable: file.executable }; } return { inventory, materialize, readFile }; } diff --git a/server/src/services/skill-package-store.ts b/server/src/services/skill-package-store.ts index 4c141d1334f..5f0f03a8a73 100644 --- a/server/src/services/skill-package-store.ts +++ b/server/src/services/skill-package-store.ts @@ -649,7 +649,7 @@ export function skillPackageStore(db: Db, suppliedStorage?: StorageService, cach // an oversized inline body never crosses into the server process. const [row] = await db.select({ ...skillVersionMetadataColumns, file: sql<(CompanySkillVersionFileInventoryEntry & { tooLarge?: boolean }) | null>`( - select case when octet_length(f->>'content') > ${maxBytes} + select case when octet_length(f->>'content')::bigint > ${maxBytes}::bigint then jsonb_build_object('tooLarge', true) else f end from jsonb_array_elements(${companySkillVersions.fileInventory}) f where f->>'path' = ${relativePath} limit 1)` })