diff --git a/server/src/__tests__/company-skills-routes.test.ts b/server/src/__tests__/company-skills-routes.test.ts index 3814dc08..049ef559 100644 --- a/server/src/__tests__/company-skills-routes.test.ts +++ b/server/src/__tests__/company-skills-routes.test.ts @@ -3,6 +3,7 @@ import request from "supertest"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { companySkillRoutes } from "../routes/company-skills.js"; import { errorHandler } from "../middleware/index.js"; +import { unprocessable } from "../errors.js"; const mockAgentService = vi.hoisted(() => ({ getById: vi.fn(), @@ -15,6 +16,7 @@ const mockAccessService = vi.hoisted(() => ({ const mockCompanySkillService = vi.hoisted(() => ({ importFromSource: vi.fn(), + deleteSkill: vi.fn(), })); const mockLogActivity = vi.hoisted(() => vi.fn()); @@ -62,6 +64,11 @@ describe("company skill mutation permissions", () => { imported: [], warnings: [], }); + mockCompanySkillService.deleteSkill.mockResolvedValue({ + id: "skill-1", + slug: "find-skills", + name: "Find Skills", + }); mockLogActivity.mockResolvedValue(undefined); mockAccessService.canUser.mockResolvedValue(true); mockAccessService.hasPermission.mockResolvedValue(false); @@ -261,4 +268,28 @@ describe("company skill mutation permissions", () => { "https://github.com/vercel-labs/agent-browser", ); }); + + it("returns a blocking error when attempting to delete a skill still used by agents", async () => { + mockCompanySkillService.deleteSkill.mockRejectedValue( + unprocessable( + 'Cannot delete skill "Find Skills" while it is still used by Builder, Reviewer. Detach it from those agents first.', + ), + ); + + const res = await request(createApp({ + type: "board", + userId: "local-board", + companyIds: ["company-1"], + source: "local_implicit", + isInstanceAdmin: false, + })) + .delete("/api/companies/company-1/skills/skill-1"); + + expect(res.status, JSON.stringify(res.body)).toBe(422); + expect(res.body).toEqual({ + error: 'Cannot delete skill "Find Skills" while it is still used by Builder, Reviewer. Detach it from those agents first.', + }); + expect(mockCompanySkillService.deleteSkill).toHaveBeenCalledWith("company-1", "skill-1"); + expect(mockLogActivity).not.toHaveBeenCalled(); + }); }); diff --git a/server/src/services/company-skills.ts b/server/src/services/company-skills.ts index faea351c..60fc06b4 100644 --- a/server/src/services/company-skills.ts +++ b/server/src/services/company-skills.ts @@ -5,7 +5,7 @@ import { fileURLToPath } from "node:url"; import { and, asc, eq } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; import { companySkills } from "@paperclipai/db"; -import { readPaperclipSkillSyncPreference, writePaperclipSkillSyncPreference } from "@paperclipai/adapter-utils/server-utils"; +import { readPaperclipSkillSyncPreference } from "@paperclipai/adapter-utils/server-utils"; import type { PaperclipSkillEntry } from "@paperclipai/adapter-utils/server-utils"; import type { CompanySkill, @@ -2314,26 +2314,23 @@ export function companySkillService(db: Db) { if (!row) return null; const skill = toCompanySkill(row); + const usedByAgents = await usage(companyId, skill.key); - // Remove from any agent desiredSkills that reference this skill - const agentRows = await agents.list(companyId); - const allSkills = await listFull(companyId); - for (const agent of agentRows) { - const config = agent.adapterConfig as Record; - const preference = readPaperclipSkillSyncPreference(config); - const referencesSkill = preference.desiredSkills.some((ref) => { - const resolved = resolveSkillReference(allSkills, ref); - return resolved.skill?.id === skillId; - }); - if (referencesSkill) { - const filtered = preference.desiredSkills.filter((ref) => { - const resolved = resolveSkillReference(allSkills, ref); - return resolved.skill?.id !== skillId; - }); - await agents.update(agent.id, { - adapterConfig: writePaperclipSkillSyncPreference(config, filtered), - }); - } + if (usedByAgents.length > 0) { + const agentNames = usedByAgents.map((agent) => agent.name).sort((left, right) => left.localeCompare(right)); + throw unprocessable( + `Cannot delete skill "${skill.name}" while it is still used by ${agentNames.join(", ")}. Detach it from those agents first.`, + { + skillId: skill.id, + skillKey: skill.key, + usedByAgents: usedByAgents.map((agent) => ({ + id: agent.id, + name: agent.name, + urlKey: agent.urlKey, + adapterType: agent.adapterType, + })), + }, + ); } // Delete DB row diff --git a/ui/src/api/companySkills.ts b/ui/src/api/companySkills.ts index adbc2117..7377b2fa 100644 --- a/ui/src/api/companySkills.ts +++ b/ui/src/api/companySkills.ts @@ -51,4 +51,8 @@ export const companySkillsApi = { `/companies/${encodeURIComponent(companyId)}/skills/${encodeURIComponent(skillId)}/install-update`, {}, ), + delete: (companyId: string, skillId: string) => + api.delete( + `/companies/${encodeURIComponent(companyId)}/skills/${encodeURIComponent(skillId)}`, + ), }; diff --git a/ui/src/pages/CompanySkills.tsx b/ui/src/pages/CompanySkills.tsx index 11854cfe..cc2d5605 100644 --- a/ui/src/pages/CompanySkills.tsx +++ b/ui/src/pages/CompanySkills.tsx @@ -52,6 +52,7 @@ import { RefreshCw, Save, Search, + Trash2, } from "lucide-react"; type SkillTreeNode = { @@ -503,6 +504,8 @@ function SkillPane({ checkUpdatesPending, onInstallUpdate, installUpdatePending, + onDelete, + deletePending, onSave, savePending, }: { @@ -522,6 +525,8 @@ function SkillPane({ checkUpdatesPending: boolean; onInstallUpdate: () => void; installUpdatePending: boolean; + onDelete: () => void; + deletePending: boolean; onSave: () => void; savePending: boolean; }) { @@ -545,6 +550,10 @@ function SkillPane({ const body = file?.markdown ? stripFrontmatter(file.content) : file?.content ?? ""; const currentPin = shortRef(detail.sourceRef); const latestPin = shortRef(updateStatus?.latestRef); + const removeBlocked = usedBy.length > 0; + const removeDisabledReason = removeBlocked + ? "Detach this skill from all agents before removing it." + : null; return (
@@ -559,17 +568,29 @@ function SkillPane({

{detail.description}

)}
- {detail.editable ? ( - - ) : ( -
{detail.editableReason}
- )} + + {deletePending ? "Removing..." : "Remove"} + + {detail.editable ? ( + + ) : ( +
{detail.editableReason}
+ )} +
@@ -723,7 +744,7 @@ function SkillPane({ ) : file.markdown && viewMode === "preview" ? ( {body} ) : ( -
+          
             {file.content}
           
)} @@ -751,6 +772,9 @@ export function CompanySkills() { const [displayedDetail, setDisplayedDetail] = useState(null); const [displayedFile, setDisplayedFile] = useState(null); const [scanStatusMessage, setScanStatusMessage] = useState(null); + const [deleteOpen, setDeleteOpen] = useState(false); + const [deleteTargetSkillId, setDeleteTargetSkillId] = useState(null); + const [deleteTargetDetail, setDeleteTargetDetail] = useState(null); const parsedRoute = useMemo(() => parseSkillRoute(routePath), [routePath]); const routeSkillId = parsedRoute.skillId; const selectedPath = parsedRoute.filePath; @@ -848,6 +872,20 @@ export function CompanySkills() { const activeDetail = detailQuery.data ?? displayedDetail; const activeFile = fileQuery.data ?? displayedFile; + function openDeleteDialog() { + setDeleteTargetSkillId(selectedSkillId); + setDeleteTargetDetail(activeDetail ?? null); + setDeleteOpen(true); + } + + function closeDeleteDialog(open: boolean) { + setDeleteOpen(open); + if (!open) { + setDeleteTargetSkillId(null); + setDeleteTargetDetail(null); + } + } + const importSkill = useMutation({ mutationFn: (importSource: string) => companySkillsApi.importFromSource(selectedCompanyId!, importSource), onSuccess: async (result) => { @@ -987,6 +1025,44 @@ export function CompanySkills() { }, }); + const deleteSkill = useMutation({ + mutationFn: () => companySkillsApi.delete(selectedCompanyId!, deleteTargetSkillId!), + onSuccess: async (skill) => { + closeDeleteDialog(false); + setDisplayedDetail(null); + setDisplayedFile(null); + await Promise.all([ + queryClient.invalidateQueries({ queryKey: queryKeys.companySkills.list(selectedCompanyId!) }), + ...(deleteTargetSkillId ? [ + queryClient.invalidateQueries({ queryKey: queryKeys.companySkills.detail(selectedCompanyId!, deleteTargetSkillId) }), + queryClient.invalidateQueries({ queryKey: queryKeys.companySkills.updateStatus(selectedCompanyId!, deleteTargetSkillId) }), + ] : []), + ...(deleteTargetSkillId ? [ + queryClient.invalidateQueries({ + queryKey: queryKeys.companySkills.file(selectedCompanyId!, deleteTargetSkillId, selectedPath), + }), + ] : []), + ]); + await queryClient.refetchQueries({ + queryKey: queryKeys.companySkills.list(selectedCompanyId!), + type: "active", + }); + navigate("/skills", { replace: true }); + pushToast({ + tone: "success", + title: "Skill removed", + body: `${skill.name} was removed from the company skill library.`, + }); + }, + onError: (error) => { + pushToast({ + tone: "error", + title: "Remove failed", + body: error instanceof Error ? error.message : "Failed to remove skill.", + }); + }, + }); + if (!selectedCompanyId) { return ; } @@ -1002,6 +1078,54 @@ export function CompanySkills() { return ( <> + + + + Remove skill + + Remove this skill from the company library. If any agents still use it, removal will be blocked until it is detached. + + +
+

+ {deleteTargetDetail + ? `You are about to remove ${deleteTargetDetail.name}.` + : "You are about to remove this skill."} +

+ {deleteTargetDetail?.usedByAgents?.length ? ( +
+ Currently used by {deleteTargetDetail.usedByAgents.map((agent) => agent.name).join(", ")}. +
+ ) : null} + {(deleteTargetDetail?.usedByAgents.length ?? 0) > 0 ? ( +

+ Detach this skill from all agents to enable removal. +

+ ) : null} +
+ + {(deleteTargetDetail?.usedByAgents.length ?? 0) > 0 ? ( + + ) : ( + <> + + + + )} + +
+
+ @@ -1160,6 +1284,8 @@ export function CompanySkills() { checkUpdatesPending={updateStatusQuery.isFetching} onInstallUpdate={() => installUpdate.mutate()} installUpdatePending={installUpdate.isPending} + onDelete={openDeleteDialog} + deletePending={deleteSkill.isPending} onSave={() => saveFile.mutate()} savePending={saveFile.isPending} />