fix(runner): address skill loading review findings

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

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<void>(resolve => { release = resolve; });
+    const ready = new Promise<void>(resolve => { acquired = resolve; });
+    const lock = withSkillVersionCacheLock(db, f.companyId, async () => { acquired(); await held; });
+    await ready;
+    let timeout: ReturnType<typeof setTimeout> | 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/" : "";

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<string, SkillText>();
+  #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<AuthorizationActor>;
 }) {
   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,

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<Record<string, unknown>> {
@@ -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)) {

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 };
 }

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)` })