From 94c27257ec09cd231030739d9bb2a3b9230ebfb7 Mon Sep 17 00:00:00 2001 From: Dotta Date: Thu, 11 Jun 2026 14:59:59 +0000 Subject: [PATCH] Fix skills store file reads and version snapshots (PAP-10910) - readFile: fall back to stored markdown when a local/catalog source file is missing instead of 500ing on ENOENT; treat an explicit "."/"" github repoSkillDir as repo root instead of appending the slug; fall back to stored markdown when the github fetch fails for SKILL.md - updateFile: snapshot a version after each changed save (skips no-op saves) and thread the acting user/agent through the route - createLocalSkill: seed an "Initial version" so the Versions tab is populated from creation - migration 0100: backfill null install_count and enforce default/not-null Co-Authored-By: Claude Opus 4.7 --- .../0100_skill_install_count_backfill.sql | 3 + packages/db/src/migrations/meta/_journal.json | 7 + .../__tests__/company-skills-service.test.ts | 158 +++++++++++++++++- server/src/routes/company-skills.ts | 1 + server/src/services/company-skills.ts | 42 ++++- 5 files changed, 203 insertions(+), 8 deletions(-) create mode 100644 packages/db/src/migrations/0100_skill_install_count_backfill.sql diff --git a/packages/db/src/migrations/0100_skill_install_count_backfill.sql b/packages/db/src/migrations/0100_skill_install_count_backfill.sql new file mode 100644 index 00000000000..a464c922750 --- /dev/null +++ b/packages/db/src/migrations/0100_skill_install_count_backfill.sql @@ -0,0 +1,3 @@ +UPDATE "company_skills" SET "install_count" = 0 WHERE "install_count" IS NULL;--> statement-breakpoint +ALTER TABLE "company_skills" ALTER COLUMN "install_count" SET DEFAULT 0;--> statement-breakpoint +ALTER TABLE "company_skills" ALTER COLUMN "install_count" SET NOT NULL; diff --git a/packages/db/src/migrations/meta/_journal.json b/packages/db/src/migrations/meta/_journal.json index 10c87e20775..f5e72cf46dd 100644 --- a/packages/db/src/migrations/meta/_journal.json +++ b/packages/db/src/migrations/meta/_journal.json @@ -701,6 +701,13 @@ "when": 1781130000000, "tag": "0099_skills_store_foundation", "breakpoints": true + }, + { + "idx": 100, + "version": "7", + "when": 1781480000000, + "tag": "0100_skill_install_count_backfill", + "breakpoints": true } ] } diff --git a/server/src/__tests__/company-skills-service.test.ts b/server/src/__tests__/company-skills-service.test.ts index e151a23e13e..c26985f3415 100644 --- a/server/src/__tests__/company-skills-service.test.ts +++ b/server/src/__tests__/company-skills-service.test.ts @@ -2,7 +2,7 @@ import { randomUUID } from "node:crypto"; import os from "node:os"; import path from "node:path"; import { promises as fs } from "node:fs"; -import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; +import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; import { agents, companies, companySkills, createDb } from "@paperclipai/db"; import { getEmbeddedPostgresTestSupport, @@ -408,6 +408,16 @@ describeEmbeddedPostgres("companySkillService.list", () => { ]); const version = await svc.createVersion(companyId, skillId, {}, { type: "user", userId: "board" }); + await expect(svc.resolveRequestedSkillEntries(companyId, [ + "pinned-skill", + ])).resolves.toEqual([ + { key: `company/${companyId}/pinned-skill`, versionId: version.id }, + ]); + await expect(svc.resolveRequestedSkillEntries(companyId, [ + { key: "pinned-skill", versionId: null }, + ])).resolves.toEqual([ + { key: `company/${companyId}/pinned-skill`, versionId: version.id }, + ]); await expect(svc.resolveRequestedSkillEntries(companyId, [ { key: "pinned-skill", versionId: version.id }, ])).resolves.toEqual([ @@ -759,4 +769,150 @@ describeEmbeddedPostgres("companySkillService.list", () => { "# Runtime Coach\n\nRecovered from DB.\n", ); }); + + it("falls back to stored markdown when reading SKILL.md from a missing local source", async () => { + const companyId = randomUUID(); + const skillId = randomUUID(); + const skillKey = `company/${companyId}/missing-reader`; + const missingSkillDir = path.join(await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-missing-read-skill-")), "gone"); + cleanupDirs.add(path.dirname(missingSkillDir)); + + await db.insert(companies).values({ + id: companyId, + name: "Paperclip", + issuePrefix: `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`, + requireBoardApprovalForNewAgents: false, + }); + await db.insert(companySkills).values({ + id: skillId, + companyId, + key: skillKey, + slug: "missing-reader", + name: "Missing Reader", + description: null, + markdown: "# Missing Reader\n\nRecovered from DB.\n", + sourceType: "local_path", + sourceLocator: missingSkillDir, + trustLevel: "markdown_only", + compatibility: "compatible", + fileInventory: [ + { path: "SKILL.md", kind: "skill" }, + { path: "references/guide.md", kind: "reference" }, + ], + metadata: { sourceKind: "local_path" }, + }); + await db.insert(agents).values({ + id: randomUUID(), + companyId, + name: "Reader", + role: "engineer", + status: "active", + adapterType: "codex_local", + adapterConfig: { + paperclipSkillSync: { + desiredSkills: [skillKey], + }, + }, + }); + + await expect(svc.readFile(companyId, skillId, "SKILL.md")).resolves.toMatchObject({ + path: "SKILL.md", + content: "# Missing Reader\n\nRecovered from DB.\n", + }); + await expect(svc.readFile(companyId, skillId, "references/guide.md")).rejects.toMatchObject({ + status: 404, + }); + }); + + it("reads root-level SKILL.md for github skills with a '.' repoSkillDir", async () => { + const companyId = randomUUID(); + const skillId = randomUUID(); + await db.insert(companies).values({ + id: companyId, + name: "Paperclip", + issuePrefix: `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`, + requireBoardApprovalForNewAgents: false, + }); + await db.insert(companySkills).values({ + id: skillId, + companyId, + key: `company/${companyId}/root-skill`, + slug: "root-skill", + name: "Root Skill", + description: null, + markdown: "# Root Skill (stored)\n", + sourceType: "github", + sourceLocator: "https://github.com/acme/root-skill", + sourceRef: "main", + trustLevel: "markdown_only", + compatibility: "compatible", + fileInventory: [{ path: "SKILL.md", kind: "skill" }], + metadata: { owner: "acme", repo: "root-skill", ref: "main", repoSkillDir: "." }, + }); + + const requestedUrls: string[] = []; + vi.stubGlobal("fetch", async (url: string | URL) => { + requestedUrls.push(String(url)); + return new Response("# Root Skill (remote)\n", { status: 200 }); + }); + try { + await expect(svc.readFile(companyId, skillId, "SKILL.md")).resolves.toMatchObject({ + content: "# Root Skill (remote)\n", + }); + expect(requestedUrls).toEqual([ + "https://raw.githubusercontent.com/acme/root-skill/main/SKILL.md", + ]); + + vi.stubGlobal("fetch", async () => { + throw new Error("network down"); + }); + await expect(svc.readFile(companyId, skillId, "SKILL.md")).resolves.toMatchObject({ + content: "# Root Skill (stored)\n", + }); + } finally { + vi.unstubAllGlobals(); + } + }); + + it("seeds an initial version on create and snapshots a version on each changed save", async () => { + const companyId = randomUUID(); + await db.insert(companies).values({ + id: companyId, + name: "Paperclip", + issuePrefix: `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`, + requireBoardApprovalForNewAgents: false, + }); + + const skill = await svc.createLocalSkill( + companyId, + { name: "Versioned Editor", description: "Edits with history" }, + { type: "user", userId: "board" }, + ); + expect(skill.currentVersionId).not.toBeNull(); + let versions = await svc.listVersions(companyId, skill.id); + expect(versions).toHaveLength(1); + expect(versions[0]).toMatchObject({ + revisionNumber: 1, + label: "Initial version", + authorUserId: "board", + }); + expect(skill.currentVersionId).toBe(versions[0]!.id); + + const editedMarkdown = "---\nname: Versioned Editor\n---\n\n# Versioned Editor\n\nEdited body.\n"; + await expect(svc.updateFile(companyId, skill.id, "SKILL.md", editedMarkdown, { type: "user", userId: "board" })) + .resolves.toMatchObject({ path: "SKILL.md", content: editedMarkdown }); + versions = await svc.listVersions(companyId, skill.id); + expect(versions).toHaveLength(2); + expect(versions[0]).toMatchObject({ revisionNumber: 2, authorUserId: "board" }); + expect(versions[0]!.fileInventory).toEqual([ + expect.objectContaining({ path: "SKILL.md", content: editedMarkdown }), + ]); + await expect(svc.getById(companyId, skill.id)).resolves.toMatchObject({ + currentVersionId: versions[0]!.id, + }); + + await svc.updateFile(companyId, skill.id, "SKILL.md", editedMarkdown, { type: "user", userId: "board" }); + versions = await svc.listVersions(companyId, skill.id); + expect(versions).toHaveLength(2); + }); }); diff --git a/server/src/routes/company-skills.ts b/server/src/routes/company-skills.ts index 84ce347c7d7..53277ccb611 100644 --- a/server/src/routes/company-skills.ts +++ b/server/src/routes/company-skills.ts @@ -458,6 +458,7 @@ export function companySkillRoutes(db: Db) { skillId, String(req.body.path ?? ""), String(req.body.content ?? ""), + skillActor(req), ); const actor = getActorInfo(req); diff --git a/server/src/services/company-skills.ts b/server/src/services/company-skills.ts index 28faf6d3324..47e2ed3b7fe 100644 --- a/server/src/services/company-skills.ts +++ b/server/src/services/company-skills.ts @@ -3011,12 +3011,15 @@ export function companySkillService(db: Db) { if (skill.sourceType === "local_path" || skill.sourceType === "catalog") { const absolutePath = resolveLocalSkillFilePath(skill, normalizedPath); - if (absolutePath) { - content = await fs.readFile(absolutePath, "utf8"); + const diskContent = absolutePath + ? await fs.readFile(absolutePath, "utf8").catch(() => null) + : null; + if (diskContent !== null) { + content = diskContent; } else if (normalizedPath === "SKILL.md") { content = skill.markdown; } else { - throw notFound("Skill file not found"); + throw notFound("Skill file is unavailable: the skill source directory is missing."); } } else if (skill.sourceType === "github" || skill.sourceType === "skills_sh") { const metadata = getSkillMeta(skill); @@ -3024,12 +3027,25 @@ export function companySkillService(db: Db) { const repo = asString(metadata.repo); const hostname = asString(metadata.hostname) || "github.com"; const ref = skill.sourceRef ?? asString(metadata.ref) ?? "main"; - const repoSkillDir = normalizeGitHubSkillDirectory(asString(metadata.repoSkillDir), skill.slug); + const rawRepoSkillDir = asString(metadata.repoSkillDir); + // An explicit "."/"" repoSkillDir means SKILL.md lives at the repo root; + // only fall back to the slug subdirectory when metadata is absent. + const repoSkillDir = rawRepoSkillDir == null + ? normalizeGitHubSkillDirectory(rawRepoSkillDir, skill.slug) + : normalizeGitHubSkillDirectory(rawRepoSkillDir, ""); if (!owner || !repo) { throw unprocessable("Skill source metadata is incomplete."); } const repoPath = normalizePortablePath(path.posix.join(repoSkillDir, normalizedPath)); - content = await fetchText(resolveRawGitHubUrl(hostname, owner, repo, ref, repoPath)); + try { + content = await fetchText(resolveRawGitHubUrl(hostname, owner, repo, ref, repoPath)); + } catch (error) { + if (normalizedPath === "SKILL.md" && skill.markdown) { + content = skill.markdown; + } else { + throw error; + } + } } else if (skill.sourceType === "url") { if (normalizedPath !== "SKILL.md") { throw notFound("This skill source only exposes SKILL.md"); @@ -3211,10 +3227,17 @@ export function companySkillService(db: Db) { }) .where(and(eq(companySkills.id, forkSource.id), eq(companySkills.companyId, companyId))); } - return row ? toCompanySkill(row) : created; + await createVersion(companyId, created.id, { label: "Initial version" }, actor); + return (await getById(companyId, created.id)) ?? (row ? toCompanySkill(row) : created); } - async function updateFile(companyId: string, skillId: string, relativePath: string, content: string): Promise { + async function updateFile( + companyId: string, + skillId: string, + relativePath: string, + content: string, + actor: SkillActor | null = null, + ): Promise { await ensureSkillInventoryCurrent(companyId); const skill = await getById(companyId, skillId); if (!skill) throw notFound("Skill not found"); @@ -3228,6 +3251,7 @@ export function companySkillService(db: Db) { const absolutePath = resolveLocalSkillFilePath(skill, normalizedPath); if (!absolutePath) throw notFound("Skill file not found"); + const previousContent = await fs.readFile(absolutePath, "utf8").catch(() => null); await fs.mkdir(path.dirname(absolutePath), { recursive: true }); await fs.writeFile(absolutePath, content, "utf8"); @@ -3250,6 +3274,10 @@ export function companySkillService(db: Db) { .where(eq(companySkills.id, skill.id)); } + if (previousContent !== content) { + await createVersion(companyId, skillId, {}, actor); + } + const detail = await readFile(companyId, skillId, normalizedPath); if (!detail) throw notFound("Skill file not found"); return detail;