Merge pull request #6570 from simstudioai/fix/skills-builtin-name-collision

fix(skills): only reject a built-in name collision on an actual rename

Author: waleedlatif1

apps/sim/lib/skills/orchestration/skill-lifecycle.test.ts

@@ -0,0 +1,108 @@
+/**
+ * @vitest-environment node
+ */
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+const { mockGetSkillActorContext, mockUpsertSkills, mockGetSkillById, mockDeleteSkill } =
+  vi.hoisted(() => ({
+    mockGetSkillActorContext: vi.fn(),
+    mockUpsertSkills: vi.fn(),
+    mockGetSkillById: vi.fn(),
+    mockDeleteSkill: vi.fn(),
+  }))
+
+vi.mock('@/lib/skills/access', () => ({
+  getSkillActorContext: mockGetSkillActorContext,
+}))
+
+vi.mock('@/lib/workflows/skills/operations', () => ({
+  upsertSkills: mockUpsertSkills,
+  getSkillById: mockGetSkillById,
+  deleteSkill: mockDeleteSkill,
+}))
+
+vi.mock('@/lib/posthog/server', () => ({
+  captureServerEvent: vi.fn(),
+}))
+
+vi.mock('@sim/audit', () => ({
+  AuditAction: { SKILL_CREATED: 'skill.created', SKILL_UPDATED: 'skill.updated' },
+  AuditResourceType: { SKILL: 'skill' },
+  recordAudit: vi.fn(),
+}))
+
+import { createSkill, updateSkill } from '@/lib/skills/orchestration/skill-lifecycle'
+
+const WORKSPACE_ID = '11111111-1111-4111-8111-111111111111'
+const USER_ID = '22222222-2222-4222-8222-222222222222'
+const SKILL_ID = '33333333-3333-4333-8333-333333333333'
+
+/** `research` is one of the shipped built-in skill names. */
+const BUILTIN_NAME = 'research'
+
+function skillRow(name: string) {
+  return {
+    id: SKILL_ID,
+    workspaceId: WORKSPACE_ID,
+    name,
+    description: 'desc',
+    content: 'content',
+  }
+}
+
+function actorOwning(name: string) {
+  return { skill: skillRow(name), hasWorkspaceAccess: true, canEdit: true }
+}
+
+describe('skill lifecycle built-in name collision', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    mockUpsertSkills.mockResolvedValue({ touched: [{ id: SKILL_ID, name: 'x' }] })
+    mockGetSkillById.mockResolvedValue(skillRow(BUILTIN_NAME))
+  })
+
+  it('allows an update that re-sends an existing built-in-colliding name unchanged', async () => {
+    mockGetSkillActorContext.mockResolvedValue(actorOwning(BUILTIN_NAME))
+
+    const row = await updateSkill({
+      workspaceId: WORKSPACE_ID,
+      userId: USER_ID,
+      skillId: SKILL_ID,
+      name: BUILTIN_NAME,
+      description: 'updated description',
+      content: 'updated content',
+    })
+
+    expect(row.name).toBe(BUILTIN_NAME)
+    expect(mockUpsertSkills).toHaveBeenCalledTimes(1)
+  })
+
+  it('rejects renaming a skill into a built-in name', async () => {
+    mockGetSkillActorContext.mockResolvedValue(actorOwning('my-skill'))
+
+    await expect(
+      updateSkill({
+        workspaceId: WORKSPACE_ID,
+        userId: USER_ID,
+        skillId: SKILL_ID,
+        name: BUILTIN_NAME,
+      })
+    ).rejects.toThrow(`The skill name "${BUILTIN_NAME}" is reserved by a built-in skill`)
+
+    expect(mockUpsertSkills).not.toHaveBeenCalled()
+  })
+
+  it('rejects creating a skill with a built-in name', async () => {
+    await expect(
+      createSkill({
+        workspaceId: WORKSPACE_ID,
+        userId: USER_ID,
+        name: BUILTIN_NAME,
+        description: 'desc',
+        content: 'content',
+      })
+    ).rejects.toThrow(`The skill name "${BUILTIN_NAME}" is reserved by a built-in skill`)
+
+    expect(mockUpsertSkills).not.toHaveBeenCalled()
+  })
+})

apps/sim/lib/skills/orchestration/skill-lifecycle.ts

@@ -337,9 +337,7 @@ export async function updateSkill(
   }
 
   const invalid =
-    (params.name !== undefined
-      ? (fieldError(skillNameSchema, params.name) ?? builtinNameCollision(params.name))
-      : null) ??
+    (params.name !== undefined ? fieldError(skillNameSchema, params.name) : null) ??
     (params.description !== undefined
       ? fieldError(skillDescriptionSchema, params.description)
       : null) ??
@@ -349,6 +347,14 @@ export async function updateSkill(
   const resolved = await resolveEditableSkill(params)
   if (!resolved.ok) throwSkillFailure(resolved.result)
 
+  // Only a rename can newly shadow a built-in. Rows predating the guard may already carry a
+  // built-in's name, and the modal always resubmits the full object, so compare against the
+  // canonical name rather than rejecting every write that echoes it back.
+  if (params.name !== undefined && params.name !== resolved.skill.name) {
+    const collision = builtinNameCollision(params.name)
+    if (collision) throw new OrchestrationError('validation', collision)
+  }
+
   try {
     await upsertSkills({
       skills: [