Merge branches 'feat/skills-gitops-complete' and 'feat/secrets-management-ui' into dev

server/src/routes/secrets.ts

@@ -141,8 +141,8 @@ export function secretRoutes(db: Db) {
       return;
     }
     assertCompanyAccess(req, existing.companyId);
-    const agents = await svc.usages(existing.companyId, id);
+    const used = await svc.usages(existing.companyId, id);
-    res.json({ agents });
+    res.json({ agents: used.agents, skills: used.skills });
   });
 
   router.delete("/secrets/:id", async (req, res) => {

server/src/services/company-skills.ts

@@ -2,7 +2,7 @@ import { createHash } from "node:crypto";
 import { promises as fs } from "node:fs";
 import path from "node:path";
 import { fileURLToPath } from "node:url";
-import { and, asc, eq } from "drizzle-orm";
+import { and, asc, eq, sql } from "drizzle-orm";
 import type { Db } from "@paperclipai/db";
 import { companies, companySkills } from "@paperclipai/db";
 import { readPaperclipSkillSyncPreference } from "@paperclipai/adapter-utils/server-utils";
@@ -2372,21 +2372,27 @@ export function companySkillService(db: Db) {
       const incomingOwner = asString(incomingMeta.owner);
       const incomingRepo = asString(incomingMeta.repo);
       const incomingKind = asString(incomingMeta.sourceKind);
+      // Bundled skills are sourced from the server image and re-upserted by
+      // ensureBundledSkills only. Never let a non-bundled import overwrite a
+      // bundled row, regardless of which org/repo it claims to be from.
       if (
         existing
         && existingMeta.sourceKind === "paperclip_bundled"
-        && incomingKind === "github"
+        && incomingKind !== "paperclip_bundled"
-        && incomingOwner === "paperclipai"
-        && incomingRepo === "paperclip"
       ) {
         out.push(existing);
         continue;
       }
 
+      // Preserve sourceAuthSecretId across re-imports of the same skill. Skip
+      // bundled rows: they should never carry a PAT reference, and preserving
+      // one across a bundled re-upsert would re-attach stale data.
       const metadata = {
         ...(skill.metadata ?? {}),
         skillKey: skill.key,
-        ...(existing?.metadata && typeof (existing.metadata as Record<string, unknown>).sourceAuthSecretId === "string"
+        ...(existing?.metadata
+          && existingMeta.sourceKind !== "paperclip_bundled"
+          && typeof (existing.metadata as Record<string, unknown>).sourceAuthSecretId === "string"
           ? { sourceAuthSecretId: (existing.metadata as Record<string, unknown>).sourceAuthSecretId }
           : {}),
       };
@@ -2464,6 +2470,8 @@ export function companySkillService(db: Db) {
 
     if (authToken && imported.length > 0) {
       for (const skill of imported) {
+        const skillMeta = skill.metadata as Record<string, unknown> | null;
+        if (skillMeta?.sourceKind === "paperclip_bundled") continue;
         const secretName = `skill-pat:${skill.id}`;
         let secretId: string;
         const existing = await secretsSvc.getByName(companyId, secretName);
@@ -2530,10 +2538,24 @@ export function companySkillService(db: Db) {
     const meta = skill.metadata as Record<string, unknown> | null;
     const secretId = typeof meta?.sourceAuthSecretId === "string" ? meta.sourceAuthSecretId : null;
     if (secretId) {
-      try {
+      // Skip cleanup if another skill in the same company still references this
-        await secretsSvc.remove(secretId);
+      // secret. The deleted row is already gone from the table, so any result
-      } catch {
+      // here is a sibling skill we shouldn't orphan.
-        // Best-effort: don't fail the skill deletion if secret cleanup fails
+      const otherSkillRefs = await db
+        .select({ id: companySkills.id })
+        .from(companySkills)
+        .where(and(
+          eq(companySkills.companyId, companyId),
+          sql`${companySkills.metadata} ->> 'sourceAuthSecretId' = ${secretId}`,
+        ))
+        .limit(1);
+      if (otherSkillRefs.length === 0) {
+        try {
+          await secretsSvc.remove(secretId);
+        } catch {
+          // Best-effort: don't fail the skill deletion if secret cleanup fails
+          // (typically blocked by an agent env binding still referencing it).
+        }
       }
     }
 
@@ -2549,6 +2571,9 @@ export function companySkillService(db: Db) {
     if (!skill) return null;
 
     const meta = (skill.metadata ?? {}) as Record<string, unknown>;
+    if (meta.sourceKind === "paperclip_bundled") {
+      throw unprocessable("Cannot configure auth for bundled paperclip skills");
+    }
     const existingSecretId = typeof meta.sourceAuthSecretId === "string" ? meta.sourceAuthSecretId : null;
 
     if (authToken) {

server/src/services/secrets.ts

@@ -1,6 +1,6 @@
-import { and, desc, eq } from "drizzle-orm";
+import { and, desc, eq, sql } from "drizzle-orm";
 import type { Db } from "@paperclipai/db";
-import { companySecrets, companySecretVersions } from "@paperclipai/db";
+import { companySecrets, companySecretVersions, companySkills } from "@paperclipai/db";
 import type { AgentEnvConfig, EnvBinding, SecretProvider } from "@paperclipai/shared";
 import { envBindingSchema } from "@paperclipai/shared";
 import { conflict, notFound, unprocessable } from "../errors.js";
@@ -46,6 +46,8 @@ export function secretService(db: Db) {
   };
 
   type SecretUsageAgent = { id: string; name: string; envKeys: string[] };
+  type SecretUsageSkill = { id: string; name: string; slug: string };
+  type SecretUsages = { agents: SecretUsageAgent[]; skills: SecretUsageSkill[] };
 
   async function getById(id: string) {
     return db
@@ -55,10 +57,10 @@ export function secretService(db: Db) {
       .then((rows) => rows[0] ?? null);
   }
 
-  async function usages(companyId: string, secretId: string): Promise<SecretUsageAgent[]> {
+  async function usages(companyId: string, secretId: string): Promise<SecretUsages> {
     const agents = agentService(db);
     const allAgents = await agents.list(companyId);
-    const out: SecretUsageAgent[] = [];
+    const agentRefs: SecretUsageAgent[] = [];
     for (const agent of allAgents) {
       const config = asRecord(agent.adapterConfig);
       if (!config) continue;
@@ -73,10 +75,28 @@ export function secretService(db: Db) {
         }
       }
       if (matchingKeys.length > 0) {
-        out.push({ id: agent.id, name: agent.name, envKeys: matchingKeys });
+        agentRefs.push({ id: agent.id, name: agent.name, envKeys: matchingKeys });
       }
     }
-    return out;
+
+    const skillRows = await db
+      .select({
+        id: companySkills.id,
+        name: companySkills.name,
+        slug: companySkills.slug,
+      })
+      .from(companySkills)
+      .where(and(
+        eq(companySkills.companyId, companyId),
+        sql`${companySkills.metadata} ->> 'sourceAuthSecretId' = ${secretId}`,
+      ));
+    const skillRefs: SecretUsageSkill[] = skillRows.map((row) => ({
+      id: row.id,
+      name: row.name,
+      slug: row.slug,
+    }));
+
+    return { agents: agentRefs, skills: skillRefs };
   }
 
   async function getByName(companyId: string, name: string) {
@@ -315,11 +335,13 @@ export function secretService(db: Db) {
       const secret = await getById(secretId);
       if (!secret) return null;
       const used = await usages(secret.companyId, secretId);
-      if (used.length > 0) {
+      if (used.agents.length > 0 || used.skills.length > 0) {
-        const names = used.map((agent) => agent.name).sort((left, right) => left.localeCompare(right));
+        const agentNames = used.agents.map((agent) => agent.name);
+        const skillNames = used.skills.map((skill) => skill.name);
+        const names = [...agentNames, ...skillNames].sort((left, right) => left.localeCompare(right));
         throw unprocessable(
-          `Cannot delete secret "${secret.name}" while it is still used by ${names.join(", ")}. Detach it from those agents first.`,
+          `Cannot delete secret "${secret.name}" while it is still used by ${names.join(", ")}. Detach it from those references first.`,
-          { secretId, usedByAgents: used },
+          { secretId, usedByAgents: used.agents, usedBySkills: used.skills },
         );
       }
       await db.delete(companySecrets).where(eq(companySecrets.id, secretId));

ui/src/api/secrets.ts

@@ -23,5 +23,8 @@ export const secretsApi = {
   ) => api.patch<CompanySecret>(`/secrets/${id}`, data),
   remove: (id: string) => api.delete<{ ok: true }>(`/secrets/${id}`),
   usages: (id: string) =>
-    api.get<{ agents: { id: string; name: string; envKeys: string[] }[] }>(`/secrets/${id}/usages`),
+    api.get<{
+      agents: { id: string; name: string; envKeys: string[] }[];
+      skills: { id: string; name: string; slug: string }[];
+    }>(`/secrets/${id}/usages`),
 };

ui/src/pages/CompanySecrets.tsx

@@ -421,7 +421,9 @@ function EditSecretDialog({
   );
 }
 
-type DeleteUsage = { id: string; name: string; envKeys: string[] };
+type DeleteUsageAgent = { id: string; name: string; envKeys: string[] };
+type DeleteUsageSkill = { id: string; name: string; slug: string };
+type DeleteBlock = { agents: DeleteUsageAgent[]; skills: DeleteUsageSkill[] };
 
 function DeleteSecretDialog({
   secret,
@@ -432,7 +434,7 @@ function DeleteSecretDialog({
   onClose: () => void;
   onDeleted: () => void;
 }) {
-  const [blockedBy, setBlockedBy] = useState<DeleteUsage[] | null>(null);
+  const [blockedBy, setBlockedBy] = useState<DeleteBlock | null>(null);
   const [errorMessage, setErrorMessage] = useState<string | null>(null);
 
   const remove = useMutation({
@@ -441,12 +443,16 @@ function DeleteSecretDialog({
     onError: (error: unknown) => {
       if (error instanceof ApiError) {
         const body = error.body as
-          | { details?: { usedByAgents?: DeleteUsage[] } }
+          | { details?: { usedByAgents?: DeleteUsageAgent[]; usedBySkills?: DeleteUsageSkill[] } }
           | null
           | undefined;
-        const usedByAgents = body?.details?.usedByAgents;
+        const usedByAgents = body?.details?.usedByAgents ?? [];
-        if (Array.isArray(usedByAgents) && usedByAgents.length > 0) {
+        const usedBySkills = body?.details?.usedBySkills ?? [];
-          setBlockedBy(usedByAgents);
+        if (
+          (Array.isArray(usedByAgents) && usedByAgents.length > 0)
+          || (Array.isArray(usedBySkills) && usedBySkills.length > 0)
+        ) {
+          setBlockedBy({ agents: usedByAgents, skills: usedBySkills });
           setErrorMessage(error.message);
           return;
         }
@@ -457,6 +463,8 @@ function DeleteSecretDialog({
     },
   });
 
+  const totalBlockers = blockedBy ? blockedBy.agents.length + blockedBy.skills.length : 0;
+
   return (
     <Dialog open onOpenChange={(open) => !open && onClose()}>
       <DialogContent className="sm:max-w-md">
@@ -464,25 +472,33 @@ function DeleteSecretDialog({
           <DialogTitle>Delete "{secret.name}"</DialogTitle>
           <DialogDescription>
             This permanently deletes the secret and all of its versions. Agents
-            that reference it will fail until the binding is removed.
+            and skills that reference it will fail until the binding is removed.
           </DialogDescription>
         </DialogHeader>
 
-        {blockedBy && blockedBy.length > 0 ? (
+        {blockedBy && totalBlockers > 0 ? (
           <div className="space-y-2 rounded-md border border-destructive/50 bg-destructive/5 px-3 py-3 text-xs">
             <p className="font-medium text-destructive">
               Cannot delete — still referenced by:
             </p>
             <ul className="space-y-1 text-muted-foreground">
-              {blockedBy.map((agent) => (
+              {blockedBy.agents.map((agent) => (
-                <li key={agent.id}>
+                <li key={`agent:${agent.id}`}>
                   <span className="font-medium text-foreground">{agent.name}</span>{" "}
+                  <span className="text-muted-foreground/70">agent</span>{" "}
                   ({agent.envKeys.join(", ")})
                 </li>
               ))}
+              {blockedBy.skills.map((skill) => (
+                <li key={`skill:${skill.id}`}>
+                  <span className="font-medium text-foreground">{skill.name}</span>{" "}
+                  <span className="text-muted-foreground/70">skill</span>{" "}
+                  ({skill.slug})
+                </li>
+              ))}
             </ul>
             <p className="text-muted-foreground">
-              Detach this secret from those agents' environment variables first.
+              Detach this secret from those references first.
             </p>
           </div>
         ) : errorMessage ? (