From d77a64475bad8d9b291dca084d9179ed76201613 Mon Sep 17 00:00:00 2001 From: Dotta Date: Thu, 11 Jun 2026 17:08:28 +0000 Subject: [PATCH] Address Greptile skills service feedback --- .../__tests__/company-skills-service.test.ts | 77 +++++++++ .../heartbeat-runtime-skills.test.ts | 18 ++ server/src/routes/company-skills.ts | 2 +- server/src/services/company-skills.ts | 159 ++++++++++++++---- 4 files changed, 218 insertions(+), 38 deletions(-) diff --git a/server/src/__tests__/company-skills-service.test.ts b/server/src/__tests__/company-skills-service.test.ts index c26985f3415..e8bfe0f7fd6 100644 --- a/server/src/__tests__/company-skills-service.test.ts +++ b/server/src/__tests__/company-skills-service.test.ts @@ -229,6 +229,9 @@ describeEmbeddedPostgres("companySkillService.list", () => { starred: true, starCount: 1, }); + await expect(svc.starSkill(companyId, skillId, { type: "user", userId: null })).rejects.toMatchObject({ + status: 422, + }); const comment = await svc.createComment( companyId, skillId, @@ -245,6 +248,7 @@ describeEmbeddedPostgres("companySkillService.list", () => { )).rejects.toMatchObject({ status: 422 }); await expect(svc.deleteComment(companyId, skillId, comment.id, { type: "user", userId: "board" })) .resolves.toMatchObject({ id: comment.id, deletedAt: expect.any(Date) }); + await expect(svc.listComments(companyId, skillId)).resolves.toEqual([]); await expect(svc.unstarSkill(companyId, skillId, { type: "user", userId: "board" })).resolves.toMatchObject({ starred: false, starCount: 0, @@ -361,6 +365,13 @@ describeEmbeddedPostgres("companySkillService.list", () => { forkedByUserId: "board", }), }); + const versions = await svc.listVersions(companyId, forked.id); + expect(versions).toHaveLength(1); + expect(versions[0]).toMatchObject({ + revisionNumber: 1, + label: "Initial version", + authorUserId: "board", + }); }); it("validates version-aware desired skill selections", async () => { @@ -874,6 +885,72 @@ describeEmbeddedPostgres("companySkillService.list", () => { } }); + it("falls back to slug paths for github skills only when repoSkillDir is absent", async () => { + const companyId = randomUUID(); + const rootSkillId = randomUUID(); + const slugSkillId = 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: rootSkillId, + companyId, + key: `company/${companyId}/empty-root-skill`, + slug: "empty-root-skill", + name: "Empty Root Skill", + description: null, + markdown: "# Empty Root Skill\n", + sourceType: "github", + sourceLocator: "https://github.com/acme/skills", + sourceRef: "main", + trustLevel: "markdown_only", + compatibility: "compatible", + fileInventory: [{ path: "SKILL.md", kind: "skill" }], + metadata: { owner: "acme", repo: "skills", ref: "main", repoSkillDir: "" }, + }, + { + id: slugSkillId, + companyId, + key: `company/${companyId}/slug-skill`, + slug: "slug-skill", + name: "Slug Skill", + description: null, + markdown: "# Slug Skill\n", + sourceType: "github", + sourceLocator: "https://github.com/acme/skills", + sourceRef: "main", + trustLevel: "markdown_only", + compatibility: "compatible", + fileInventory: [{ path: "SKILL.md", kind: "skill" }], + metadata: { owner: "acme", repo: "skills", ref: "main" }, + }, + ]); + + const requestedUrls: string[] = []; + vi.stubGlobal("fetch", async (url: string | URL) => { + requestedUrls.push(String(url)); + return new Response("# Remote Skill\n", { status: 200 }); + }); + try { + await expect(svc.readFile(companyId, rootSkillId, "SKILL.md")).resolves.toMatchObject({ + content: "# Remote Skill\n", + }); + await expect(svc.readFile(companyId, slugSkillId, "SKILL.md")).resolves.toMatchObject({ + content: "# Remote Skill\n", + }); + expect(requestedUrls).toEqual([ + "https://raw.githubusercontent.com/acme/skills/main/SKILL.md", + "https://raw.githubusercontent.com/acme/skills/main/slug-skill/SKILL.md", + ]); + } 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({ diff --git a/server/src/__tests__/heartbeat-runtime-skills.test.ts b/server/src/__tests__/heartbeat-runtime-skills.test.ts index 2a2512da96a..e5a5492846c 100644 --- a/server/src/__tests__/heartbeat-runtime-skills.test.ts +++ b/server/src/__tests__/heartbeat-runtime-skills.test.ts @@ -221,5 +221,23 @@ describeEmbeddedPostgres("heartbeat runtime skill version pins", () => { .resolves.toContain("Version one."); await expect(fs.readFile(path.join(secondSkill!.source, "SKILL.md"), "utf8")) .resolves.toContain("Version two."); + + const firstSkillFile = path.join(firstSkill!.source, "SKILL.md"); + const oldMtime = new Date("2024-01-01T00:00:00.000Z"); + await fs.utimes(firstSkillFile, oldMtime, oldMtime); + + const repeatRun = await heartbeat.invoke(firstAgentId, "on_demand", {}, "manual"); + expect(repeatRun).not.toBeNull(); + expect((await waitForRunToFinish(heartbeat, repeatRun!.id))?.status).toBe("succeeded"); + const repeatedSkill = capturedRuns + .filter((run) => run.agentId === firstAgentId) + .at(-1) + ?.skills.find((entry) => entry.key === skillKey); + expect(repeatedSkill).toMatchObject({ + source: firstSkill!.source, + versionId: versionOne.id, + sourceStatus: "available", + }); + expect((await fs.stat(firstSkillFile)).mtime.toISOString()).toBe(oldMtime.toISOString()); }); }); diff --git a/server/src/routes/company-skills.ts b/server/src/routes/company-skills.ts index 53277ccb611..c6bec7a687e 100644 --- a/server/src/routes/company-skills.ts +++ b/server/src/routes/company-skills.ts @@ -80,7 +80,7 @@ export function companySkillRoutes(db: Db) { return { type: "agent" as const, agentId: req.actor.agentId ?? null }; } if (req.actor.type === "board") { - return { type: "user" as const, userId: req.actor.userId ?? "board" }; + return { type: "user" as const, userId: req.actor.userId ?? null }; } return { type: "system" as const }; } diff --git a/server/src/services/company-skills.ts b/server/src/services/company-skills.ts index cc645c8437a..7355cea28cc 100644 --- a/server/src/services/company-skills.ts +++ b/server/src/services/company-skills.ts @@ -2,7 +2,7 @@ import { createHash, randomUUID } from "node:crypto"; import { promises as fs } from "node:fs"; import path from "node:path"; import { fileURLToPath } from "node:url"; -import { and, asc, desc, eq, sql } from "drizzle-orm"; +import { and, asc, desc, eq, isNull, sql } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; import { companies, companySkillComments, companySkillStars, companySkillVersions, companySkills } from "@paperclipai/db"; import { readPaperclipSkillSyncPreference } from "@paperclipai/adapter-utils/server-utils"; @@ -2636,30 +2636,42 @@ export function companySkillService(db: Db) { await ensureSkillInventoryCurrent(companyId); const skill = await getById(companyId, skillId); if (!skill) throw notFound("Skill not found"); - const [{ nextRevision }] = await db - .select({ - nextRevision: sql`coalesce(max(${companySkillVersions.revisionNumber}), 0) + 1`, - }) - .from(companySkillVersions) - .where(and(eq(companySkillVersions.companyId, companyId), eq(companySkillVersions.companySkillId, skillId))); - const versionRow = await db - .insert(companySkillVersions) - .values({ - companyId, - companySkillId: skillId, - revisionNumber: Number(nextRevision ?? 1), - label: input.label?.trim() || null, - fileInventory: serializeVersionFileInventory(await collectVersionFileInventory(companyId, skill)), - authorAgentId: actor?.type === "agent" ? actor.agentId ?? null : null, - authorUserId: actor?.type === "user" ? actor.userId ?? null : null, - }) - .returning() - .then((rows) => rows[0] ?? null); + const fileInventory = serializeVersionFileInventory(await collectVersionFileInventory(companyId, skill)); + const versionRow = await db.transaction(async (tx) => { + await tx.execute(sql` + select ${companySkills.id} + from ${companySkills} + where ${companySkills.id} = ${skillId} + and ${companySkills.companyId} = ${companyId} + for update + `); + const [{ nextRevision }] = await tx + .select({ + nextRevision: sql`coalesce(max(${companySkillVersions.revisionNumber}), 0) + 1`, + }) + .from(companySkillVersions) + .where(and(eq(companySkillVersions.companyId, companyId), eq(companySkillVersions.companySkillId, skillId))); + const row = await tx + .insert(companySkillVersions) + .values({ + companyId, + companySkillId: skillId, + revisionNumber: Number(nextRevision ?? 1), + label: input.label?.trim() || null, + fileInventory, + authorAgentId: actor?.type === "agent" ? actor.agentId ?? null : null, + authorUserId: actor?.type === "user" ? actor.userId ?? null : null, + }) + .returning() + .then((rows) => rows[0] ?? null); + if (!row) return null; + await tx + .update(companySkills) + .set({ currentVersionId: row.id, updatedAt: new Date() }) + .where(and(eq(companySkills.id, skillId), eq(companySkills.companyId, companyId))); + return row; + }); if (!versionRow) throw notFound("Failed to persist skill version"); - await db - .update(companySkills) - .set({ currentVersionId: versionRow.id, updatedAt: new Date() }) - .where(and(eq(companySkills.id, skillId), eq(companySkills.companyId, companyId))); return toCompanySkillVersion(versionRow); } @@ -2716,7 +2728,11 @@ export function companySkillService(db: Db) { const rows = await db .select() .from(companySkillComments) - .where(and(eq(companySkillComments.companyId, companyId), eq(companySkillComments.companySkillId, skillId))) + .where(and( + eq(companySkillComments.companyId, companyId), + eq(companySkillComments.companySkillId, skillId), + isNull(companySkillComments.deletedAt), + )) .orderBy(asc(companySkillComments.createdAt)); return rows.map((row) => toCompanySkillComment(row)); } @@ -2785,7 +2801,11 @@ export function companySkillService(db: Db) { const row = await db .update(companySkillComments) .set({ body: input.body, updatedAt: new Date() }) - .where(eq(companySkillComments.id, commentId)) + .where(and( + eq(companySkillComments.companyId, companyId), + eq(companySkillComments.companySkillId, skillId), + eq(companySkillComments.id, commentId), + )) .returning() .then((rows) => rows[0] ?? null); if (!row) throw notFound("Skill comment not found"); @@ -2813,7 +2833,11 @@ export function companySkillService(db: Db) { const row = await db .update(companySkillComments) .set({ deletedAt: new Date(), updatedAt: new Date() }) - .where(eq(companySkillComments.id, commentId)) + .where(and( + eq(companySkillComments.companyId, companyId), + eq(companySkillComments.companySkillId, skillId), + eq(companySkillComments.id, commentId), + )) .returning() .then((rows) => rows[0] ?? null); if (!row) throw notFound("Skill comment not found"); @@ -3038,12 +3062,12 @@ 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 rawRepoSkillDir = asString(metadata.repoSkillDir); + const rawRepoSkillDir = typeof metadata.repoSkillDir === "string" ? metadata.repoSkillDir.trim() : null; // 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, ""); + const repoSkillDir = typeof metadata.repoSkillDir === "string" + ? normalizeGitHubSkillDirectory(rawRepoSkillDir, "") + : normalizeGitHubSkillDirectory(rawRepoSkillDir, skill.slug); if (!owner || !repo) { throw unprocessable("Skill source metadata is incomplete."); } @@ -4082,20 +4106,81 @@ export function companySkillService(db: Db) { return skillDir; } + function resolveVersionSnapshotPath(skillDir: string, relativePath: string) { + const normalizedPath = normalizePortablePath(relativePath); + if (!normalizedPath) return null; + const targetPath = path.resolve(skillDir, normalizedPath); + if (targetPath !== skillDir && !targetPath.startsWith(`${skillDir}${path.sep}`)) { + throw unprocessable(`Skill version file path is invalid: ${relativePath}`); + } + return { normalizedPath, targetPath }; + } + + async function listMaterializedFiles(root: string): Promise { + async function walk(dir: string, base: string): Promise { + const entries = await fs.readdir(dir, { withFileTypes: true }); + const out: string[] = []; + for (const entry of entries) { + const relativePath = base ? path.posix.join(base, entry.name) : entry.name; + const absolutePath = path.join(dir, entry.name); + if (entry.isDirectory()) { + out.push(...await walk(absolutePath, relativePath)); + } else if (entry.isFile()) { + out.push(normalizePortablePath(relativePath)); + } else { + out.push(normalizePortablePath(relativePath)); + } + } + return out; + } + + try { + return await walk(root, ""); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") return null; + throw error; + } + } + + async function materializedVersionSnapshotMatches(skillDir: string, version: CompanySkillVersion) { + const expected = new Map(); + let sawSkillFile = false; + for (const entry of version.fileInventory) { + const resolved = resolveVersionSnapshotPath(skillDir, entry.path); + if (!resolved) continue; + expected.set(resolved.normalizedPath, entry.content); + if (resolved.normalizedPath === "SKILL.md") sawSkillFile = true; + } + if (!sawSkillFile) { + throw unprocessable("Company skill version could not be materialized because its SKILL.md snapshot is missing."); + } + + const existingFiles = await listMaterializedFiles(skillDir); + if (!existingFiles || existingFiles.length !== expected.size) return false; + for (const relativePath of existingFiles) { + if (!expected.has(relativePath)) return false; + } + for (const [relativePath, content] of expected.entries()) { + const existingContent = await fs.readFile(path.resolve(skillDir, relativePath), "utf8").catch(() => null); + if (existingContent !== content) return false; + } + return true; + } + async function materializeVersionSnapshot(companyId: string, skill: CompanySkill, version: CompanySkillVersion) { const runtimeRoot = path.resolve(resolveManagedSkillsRoot(companyId), "__versions__"); const skillDir = path.resolve(runtimeRoot, skill.id, version.id); + if (await materializedVersionSnapshotMatches(skillDir, version)) { + return skillDir; + } await fs.rm(skillDir, { recursive: true, force: true }); await fs.mkdir(skillDir, { recursive: true }); let wroteSkillFile = false; for (const entry of version.fileInventory) { - const normalizedPath = normalizePortablePath(entry.path); - if (!normalizedPath) continue; - const targetPath = path.resolve(skillDir, normalizedPath); - if (targetPath !== skillDir && !targetPath.startsWith(`${skillDir}${path.sep}`)) { - throw unprocessable(`Skill version file path is invalid: ${entry.path}`); - } + const resolved = resolveVersionSnapshotPath(skillDir, entry.path); + if (!resolved) continue; + const { normalizedPath, targetPath } = resolved; await fs.mkdir(path.dirname(targetPath), { recursive: true }); await fs.writeFile(targetPath, entry.content, "utf8"); if (normalizedPath === "SKILL.md") wroteSkillFile = true;