From 54c5e7cb41bb7357e856087650c0c7fe64f4589d Mon Sep 17 00:00:00 2001
From: Chris Farhood <chris@farhood.org>
Date: Thu, 9 Apr 2026 15:57:36 -0400
Subject: [PATCH] fix(skills): atomic deleteBySource + PAT secret cleanup on
 skill deletion

- Pre-check all skills for agent usage before deleting any in deleteBySource
  to prevent partial/failed deletions
- Delete (rotate to empty) the skill-pat:<skillId> secret when a skill is
  deleted to prevent orphaned PAT secrets

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---
 server/src/services/company-skills.ts | 33 +++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/server/src/services/company-skills.ts b/server/src/services/company-skills.ts
index c94b9ffa..587adc68 100644
--- a/server/src/services/company-skills.ts
+++ b/server/src/services/company-skills.ts
@@ -2397,6 +2397,17 @@ export function companySkillService(db: Db) {
     // Clean up materialized runtime files
     await fs.rm(resolveRuntimeSkillMaterializedPath(companyId, skill), { recursive: true, force: true });
 
+    // Delete associated PAT secret if present
+    const meta = skill.metadata as Record<string, unknown> | null;
+    const secretId = typeof meta?.sourceAuthSecretId === "string" ? meta.sourceAuthSecretId : null;
+    if (secretId) {
+      try {
+        await secretsSvc.remove(secretId);
+      } catch {
+        // Best-effort: don't fail the skill deletion if secret cleanup fails
+      }
+    }
+
     return skill;
   }
 
@@ -2454,6 +2465,28 @@ export function companySkillService(db: Db) {
       .where(and(eq(companySkills.companyId, companyId), eq(companySkills.sourceLocator, sourceLocator)));
     if (rows.length === 0) return [];
 
+    // Pre-check all skills for agent usage before deleting any (atomicity)
+    const skills = rows.map(toCompanySkill);
+    for (const skill of skills) {
+      const usedByAgents = await usage(companyId, skill.key);
+      if (usedByAgents.length > 0) {
+        const agentNames = usedByAgents.map((agent) => agent.name).sort((left, right) => left.localeCompare(right));
+        throw unprocessable(
+          `Cannot delete skills from "${sourceLocator}" because skill "${skill.name}" 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,
+            })),
+          },
+        );
+      }
+    }
+
     const deleted: CompanySkill[] = [];
     for (const row of rows) {
       const result = await deleteSkill(companyId, row.id);