From 209e2c80eee4daa490a3c75fe722595a2ac89f86 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 11 Aug 2026 15:36:50 -0700 Subject: [PATCH 1/2] fix(skills): only reject a built-in name collision on an actual rename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The built-in-name guard ran on every update that carried a `name`, without comparing it to the skill's current persisted name. Skills created before the guard existed can legitimately carry a built-in's name (they simply shadowed the built-in at read time), and the skill modal always submits the full object including the unchanged name — so every save of such a skill returned 400 with "The skill name ... is reserved by a built-in skill", with no way to fix it short of renaming. Move the guard in `updateSkill` to after the canonical row is loaded and run it only when the submitted name differs from the current one. Creating a skill with a built-in name, and renaming an existing skill into one, are still rejected. The check stays in the shared orchestration primitive because that is the only layer both the internal `/api/skills` adapter (via `performUpdateSkill`) and `updateSkillUseCase` (v2 + Copilot) pass through, and it is where the current name is in hand. --- .../orchestration/skill-lifecycle.test.ts | 108 ++++++++++++++++++ .../skills/orchestration/skill-lifecycle.ts | 15 ++- 2 files changed, 120 insertions(+), 3 deletions(-) create mode 100644 apps/sim/lib/skills/orchestration/skill-lifecycle.test.ts 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..58bdd217667 --- /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.mockImplementation(async () => 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..21f18dc84d6 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,17 @@ export async function updateSkill( const resolved = await resolveEditableSkill(params) if (!resolved.ok) throwSkillFailure(resolved.result) + /** + * Only a rename can newly shadow a built-in, so the guard runs against the + * canonical name rather than the submitted one. Rows predating the guard may + * already carry a built-in's name, and the skill modal always submits the + * full object — re-sending that unchanged name is not a new collision. + */ + 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: [ From c8007cecc4e3517670a26654ba599c12d2ecd91b Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 11 Aug 2026 15:57:57 -0700 Subject: [PATCH 2/2] chore(skills): tidy collision guard cleanup --- .../sim/lib/skills/orchestration/skill-lifecycle.test.ts | 2 +- apps/sim/lib/skills/orchestration/skill-lifecycle.ts | 9 +++------ 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/apps/sim/lib/skills/orchestration/skill-lifecycle.test.ts b/apps/sim/lib/skills/orchestration/skill-lifecycle.test.ts index 58bdd217667..857d4fd194f 100644 --- a/apps/sim/lib/skills/orchestration/skill-lifecycle.test.ts +++ b/apps/sim/lib/skills/orchestration/skill-lifecycle.test.ts @@ -58,7 +58,7 @@ describe('skill lifecycle built-in name collision', () => { beforeEach(() => { vi.clearAllMocks() mockUpsertSkills.mockResolvedValue({ touched: [{ id: SKILL_ID, name: 'x' }] }) - mockGetSkillById.mockImplementation(async () => skillRow(BUILTIN_NAME)) + mockGetSkillById.mockResolvedValue(skillRow(BUILTIN_NAME)) }) it('allows an update that re-sends an existing built-in-colliding name unchanged', async () => { diff --git a/apps/sim/lib/skills/orchestration/skill-lifecycle.ts b/apps/sim/lib/skills/orchestration/skill-lifecycle.ts index 21f18dc84d6..ad2a25f413f 100644 --- a/apps/sim/lib/skills/orchestration/skill-lifecycle.ts +++ b/apps/sim/lib/skills/orchestration/skill-lifecycle.ts @@ -347,12 +347,9 @@ export async function updateSkill( const resolved = await resolveEditableSkill(params) if (!resolved.ok) throwSkillFailure(resolved.result) - /** - * Only a rename can newly shadow a built-in, so the guard runs against the - * canonical name rather than the submitted one. Rows predating the guard may - * already carry a built-in's name, and the skill modal always submits the - * full object — re-sending that unchanged name is not a new collision. - */ + // 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)