From 77f854c081e22dbc036d4af632de9202e8c237ab Mon Sep 17 00:00:00 2001 From: Daniel Sousa Date: Wed, 1 Apr 2026 17:18:01 +0100 Subject: [PATCH] feat(company-skills): implement skill deletion with agent usage check Added functionality to prevent deletion of skills that are still in use by agents. Updated the company skill service to throw an unprocessable error if a skill is attempted to be deleted while still referenced by agents. Enhanced the UI to include a delete button and confirmation dialog, displaying relevant messages based on agent usage. Updated tests to cover the new deletion logic and error handling. --- .../__tests__/company-skills-routes.test.ts | 31 ++++ server/src/services/company-skills.ts | 37 ++--- ui/src/api/companySkills.ts | 4 + ui/src/pages/CompanySkills.tsx | 148 ++++++++++++++++-- 4 files changed, 189 insertions(+), 31 deletions(-) diff --git a/server/src/__tests__/company-skills-routes.test.ts b/server/src/__tests__/company-skills-routes.test.ts index 8ac0785d..93bf6fd1 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()); @@ -45,6 +47,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); @@ -110,4 +117,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 2b97da20..bdc338c1 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, @@ -2298,26 +2298,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}
           />