diff --git a/apps/sim/lib/skills/orchestration/skill-lifecycle.test.ts b/apps/sim/lib/skills/orchestration/skill-lifecycle.test.ts new file mode 100644 index 00000000000..857d4fd194f --- /dev/null +++ b/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() + }) +}) diff --git a/apps/sim/lib/skills/orchestration/skill-lifecycle.ts b/apps/sim/lib/skills/orchestration/skill-lifecycle.ts index 1fd56b048c6..ad2a25f413f 100644 --- a/apps/sim/lib/skills/orchestration/skill-lifecycle.ts +++ b/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: [