diff --git a/packages/@n8n/api-types/src/agents/__tests__/skills.schema.test.ts b/packages/@n8n/api-types/src/agents/__tests__/skills.schema.test.ts index 6251e47b247..1e43f683dd0 100644 --- a/packages/@n8n/api-types/src/agents/__tests__/skills.schema.test.ts +++ b/packages/@n8n/api-types/src/agents/__tests__/skills.schema.test.ts @@ -2,6 +2,7 @@ import { CreateSkillDto, ListSkillsQueryDto, SaveSkillDto, + SkillNewNameQueryDto, SKILLS_MAX_PAGE_SIZE, } from '../skills.schema'; @@ -81,3 +82,15 @@ describe('SaveSkillDto', () => { }, ); }); + +describe('SkillNewNameQueryDto', () => { + it('trims the requested name', () => { + expect(SkillNewNameQueryDto.parse({ scope: 'instance', name: ' New skill ' }).name).toBe( + 'New skill', + ); + }); + + it('refuses a name of only spaces', () => { + expect(SkillNewNameQueryDto.safeParse({ scope: 'instance', name: ' ' }).success).toBe(false); + }); +}); diff --git a/packages/@n8n/api-types/src/agents/agent-skill.schema.ts b/packages/@n8n/api-types/src/agents/agent-skill.schema.ts index 5ed1ff1cad8..18d4a02c06d 100644 --- a/packages/@n8n/api-types/src/agents/agent-skill.schema.ts +++ b/packages/@n8n/api-types/src/agents/agent-skill.schema.ts @@ -63,8 +63,10 @@ const agentSkillReferencesSchema = z * config as `{ type: 'skill', id }` refs (mirroring tasks), so it is * intentionally not part of the body. */ +export const AGENT_SKILL_NAME_MAX_LENGTH = 128; + export const agentSkillShape = { - name: z.string().min(1).max(128), + name: z.string().min(1).max(AGENT_SKILL_NAME_MAX_LENGTH), description: z.string().min(1).max(512), // Measured in characters like the name, description and reference limits, // so the editor can show the user the same count the server enforces. diff --git a/packages/@n8n/api-types/src/agents/skills.schema.ts b/packages/@n8n/api-types/src/agents/skills.schema.ts index bf7279c7d1b..b8d9e8553c4 100644 --- a/packages/@n8n/api-types/src/agents/skills.schema.ts +++ b/packages/@n8n/api-types/src/agents/skills.schema.ts @@ -1,7 +1,12 @@ import { z } from 'zod'; -import { agentSkillSchema, agentSkillShape } from './agent-skill.schema'; +import { + AGENT_SKILL_NAME_MAX_LENGTH, + agentSkillSchema, + agentSkillShape, +} from './agent-skill.schema'; import type { AgentSkill } from './types'; +import type { ProjectType } from '../schemas/project.schema'; import { paginationSchema } from '../dto/pagination/pagination.dto'; import { Z } from '../zod-class'; @@ -24,6 +29,14 @@ export type SkillScope = z.infer; export type SkillSource = 'ui' | 'agent'; +/** The user who created a skill, as the settings list names them. */ +export type SkillUserDto = { + id: string; + email: string | null; + firstName: string | null; + lastName: string | null; +}; + /** One row of the skills list. Name and description come from the latest version. */ export type SkillListItem = { id: string; @@ -33,10 +46,14 @@ export type SkillListItem = { /** The project of a project skill, or the project of the agent of an agent skill. */ projectId: string | null; projectName: string | null; + /** Lets the editor name another member's personal project, which it does not list. */ + projectType: ProjectType | null; /** Set for an agent skill: the only agent that can use it. */ agentId: string | null; agentName: string | null; source: SkillSource; + /** Null when the creator is unknown or the user no longer exists. */ + createdBy: SkillUserDto | null; /** Number of the latest version, the one following agents run. */ latestVersion: number; /** Distinct agents that use the skill: draft refs plus published pins. */ @@ -100,6 +117,25 @@ export class ListSkillsQueryDto extends Z.class({ attachableTo: z.string().max(36).optional(), }) {} +/** + * Asks for a name that no skill in the scope has yet, as `/workflows/new` does for a + * workflow: the requested name, or the same name with the first free number after it. + */ +export class SkillNewNameQueryDto extends Z.class({ + scope: skillScopeSchema, + /** Required for a project skill, absent otherwise. */ + projectId: z.string().max(36).optional(), + /** Required for an agent skill, absent otherwise. */ + agentId: z.string().max(36).optional(), + /** Trimmed first, so a name of only spaces is refused instead of suggested as "". */ + name: z.string().trim().min(1).max(AGENT_SKILL_NAME_MAX_LENGTH), +}) {} + +export type SkillNewNameResponse = { name: string }; + +/** The 409 message for a skill name that its scope already holds. The editor matches it. */ +export const SKILL_NAME_TAKEN_MESSAGE = 'That name is taken, choose another'; + export class CreateSkillDto extends Z.class({ scope: skillScopeSchema, /** Required for a project skill, absent otherwise. */ @@ -120,4 +156,11 @@ export class SaveSkillDto extends Z.class({ instructions: agentSkillShape.instructions.optional(), references: agentSkillShape.references.optional(), baseSkillHash: z.string().min(1), + /** + * Moves the skill to the instance or to a project. Refused with 409 while an agent + * uses it. An agent skill is made from inside its agent, so `agent` is not a choice. + */ + scope: z.enum(['project', 'instance']).optional(), + /** Required when `scope` is `project`, absent otherwise. */ + projectId: z.string().max(36).optional(), }) {} diff --git a/packages/cli/src/modules/agents/__tests__/agent-skill-refs.service.test.ts b/packages/cli/src/modules/agents/__tests__/agent-skill-refs.service.test.ts index 9b613b5e6e0..b6a414bab86 100644 --- a/packages/cli/src/modules/agents/__tests__/agent-skill-refs.service.test.ts +++ b/packages/cli/src/modules/agents/__tests__/agent-skill-refs.service.test.ts @@ -463,7 +463,7 @@ describe('AgentSkillRefsService', () => { service.createSkills(agent, [body], 'user-1', availableTo, ctx), ).resolves.toEqual(['skill_created']); expect(skillService.create).toHaveBeenCalledWith( - { target, skill: body, source: 'agent', createdById: 'user-1' }, + { target, skill: body, source: 'agent', createdById: 'user-1', checkNameTaken: true }, ctx, ); }); @@ -478,6 +478,19 @@ describe('AgentSkillRefsService', () => { { projectId: null, agentId: 'agent-1' }, 'agent-1', ctx, + undefined, + ); + }); + + it('checks the name that the same save gives the skill', async () => { + await service.setAvailability(agent, 'skill_a', 'project', ctx, 'Condense'); + + expect(skillService.changeTarget).toHaveBeenCalledWith( + 'skill_a', + { projectId: agent.projectId, agentId: null }, + 'agent-1', + ctx, + 'Condense', ); }); }); diff --git a/packages/cli/src/modules/agents/__tests__/agents-builder-tools.service.test.ts b/packages/cli/src/modules/agents/__tests__/agents-builder-tools.service.test.ts index d9cdb305f27..487aa681c4c 100644 --- a/packages/cli/src/modules/agents/__tests__/agents-builder-tools.service.test.ts +++ b/packages/cli/src/modules/agents/__tests__/agents-builder-tools.service.test.ts @@ -49,6 +49,7 @@ import { BUILDER_TOOLS } from '../builder/builder-tool-names'; import type { Agent } from '../entities/agent.entity'; import type { AgentSecureRuntime } from '../runtime/agent-secure-runtime'; import { getAgentConfigHash } from '../utils/agent-config-hash'; +import { SkillNameTakenError } from '../skills/skill-name-taken.error'; import * as checkAccess from '@/permissions.ee/check-access'; import type { InstanceAiCredentialService } from '@n8n/instance-ai'; import { createAgentSkillRefsService } from './test-utils/stored-agent-config'; @@ -1844,6 +1845,42 @@ describe('AgentsBuilderToolsService', () => { }); }); + it('names a taken skill name instead of asking for a retry', async () => { + const { service, agentsService } = makeService(); + agentsService.createAndAttachSkills.mockRejectedValue( + new SkillNameTakenError('Summarize Meetings'), + ); + + const result = await getCreateSkillsTool(service).handler!( + { skills: [makeSkillInput('Summarize Meetings')] }, + ctx, + ); + + expect(result).toEqual({ + ok: false, + errors: [ + { + message: expect.stringContaining( + 'The skill name "Summarize Meetings" is taken by another skill in the same scope', + ), + }, + ], + }); + }); + + it('tells the model that names are unique in the project', () => { + const { service } = makeService(); + + const tool = getCreateSkillsTool(service); + const nameSchema = ( + tool.inputSchema as unknown as { + shape: { skills: { element: { shape: { name: { description?: string } } } } }; + } + ).shape.skills.element.shape.name; + + expect(nameSchema.description).toContain('unique in the project'); + }); + it('rejects an empty skills array via the input schema', () => { const { service } = makeService(); @@ -2012,6 +2049,30 @@ describe('AgentsBuilderToolsService', () => { }); }); + it('names a taken skill name on a rename instead of the stale skill error', async () => { + const { service, agentsService } = makeService(); + agentsService.updateSkill.mockRejectedValue(new SkillNameTakenError('Create tickets')); + + const result = await getUpdateSkillTool(service).handler!( + { + skillId: 'skill_create_tickets', + baseSkillHash: 'hash', + updates: { name: 'Create tickets' }, + }, + ctx, + ); + + expect(result).toEqual({ + ok: false, + errors: [ + { + message: expect.stringContaining('The skill name "Create tickets" is taken'), + }, + ], + }); + expect(JSON.stringify(result)).not.toContain('Skill changed'); + }); + it('rejects instructions and instructionEdits together', () => { const { service } = makeService(); const inputSchema = getUpdateSkillTool(service).inputSchema as unknown as { diff --git a/packages/cli/src/modules/agents/__tests__/skill.service.test.ts b/packages/cli/src/modules/agents/__tests__/skill.service.test.ts index 309c8e59dc3..d5f09403149 100644 --- a/packages/cli/src/modules/agents/__tests__/skill.service.test.ts +++ b/packages/cli/src/modules/agents/__tests__/skill.service.test.ts @@ -100,6 +100,8 @@ describe('SkillService', () => { vi.resetAllMocks(); txRunner.run.mockImplementation(async (_ctx, fn) => await fn(TX_CTX)); skills.findLatestSaved.mockResolvedValue(new Map()); + skills.findLatestNamesInTarget.mockResolvedValue([]); + skills.findLatestSummaries.mockResolvedValue(new Map()); skills.findVersionsByIds.mockResolvedValue(new Map()); skills.findDependencies.mockResolvedValue(new Map()); skills.findByIds.mockResolvedValue([]); @@ -231,6 +233,75 @@ describe('SkillService', () => { ).rejects.toThrow(UserError); expect(skills.createSkill).not.toHaveBeenCalled(); }); + + it.each(['Brand voice', 'brand-voice', ' BRAND_voice '])( + 'refuses "%s" when another skill in the same scope has a name like it', + async (name) => { + skills.findLatestNamesInTarget.mockResolvedValue([ + { skillId: 'skill_x', name: 'Brand voice' }, + ]); + + await expect( + service.create({ + target: { projectId: PROJECT, agentId: null }, + skill: { name, description: 'd', instructions: 'i' }, + source: 'ui', + createdById: user.id, + checkNameTaken: true, + }), + ).rejects.toThrow('That name is taken, choose another'); + expect(skills.findLatestNamesInTarget).toHaveBeenCalledWith( + { projectId: PROJECT, agentId: null }, + expect.anything(), + ); + expect(skills.createSkill).not.toHaveBeenCalled(); + }, + ); + + it('lets a copy keep the name of the skill it copies', async () => { + skills.findLatestNamesInTarget.mockResolvedValue([ + { skillId: 'skill_x', name: 'Brand voice' }, + ]); + + await service.create({ + target: { projectId: null, agentId: 'agent-2' }, + skill: { name: 'Brand voice', description: 'd', instructions: 'i' }, + source: 'agent', + createdById: user.id, + }); + + expect(skills.findLatestNamesInTarget).not.toHaveBeenCalled(); + expect(skills.createSkill).toHaveBeenCalled(); + }); + }); + + describe('freeName', () => { + const target = { projectId: PROJECT, agentId: null }; + + it('keeps a name that the scope does not hold', async () => { + expect(await service.freeName('New skill', target)).toBe('New skill'); + expect(skills.findLatestNamesInTarget).toHaveBeenCalledWith(target, {}); + }); + + it('adds the first free number, by the same rule as the name check', async () => { + skills.findLatestNamesInTarget.mockResolvedValue([ + { skillId: 'a', name: 'new-skill' }, + { skillId: 'b', name: 'New skill 2' }, + { skillId: 'c', name: 'New skill 4' }, + ]); + + expect(await service.freeName('New skill', target)).toBe('New skill 3'); + }); + + it('shortens a long name so the number fits the length limit', async () => { + const long = 'x'.repeat(128); + skills.findLatestNamesInTarget.mockResolvedValue([{ skillId: 'a', name: long }]); + + const name = await service.freeName(long, target); + + expect(name).toHaveLength(128); + expect(name.endsWith(' 2')).toBe(true); + }); }); describe('save', () => { @@ -291,6 +362,31 @@ describe('SkillService', () => { ); }); + it('refuses a rename to a name that another skill in its scope has', async () => { + const baseSkillHash = givenLatest(versionRow(skill, 2)); + skills.findLatestNamesInTarget.mockResolvedValue([ + { skillId: 'skill_a', name: 'Brand voice' }, + { skillId: 'skill_x', name: 'Code review' }, + ]); + + await expect( + service.save('skill_a', { baseSkillHash, name: 'code-review' }, user.id), + ).rejects.toThrow('That name is taken, choose another'); + expect(skills.findLatestNamesInTarget).toHaveBeenCalledWith( + { projectId: PROJECT, agentId: null }, + TX_CTX, + ); + expect(skills.insertSavedVersion).not.toHaveBeenCalled(); + }); + + it('does not check the name when the save keeps it', async () => { + const baseSkillHash = givenLatest(versionRow(skill, 2)); + + await service.save('skill_a', { baseSkillHash, instructions: 'new' }, user.id); + + expect(skills.findLatestNamesInTarget).not.toHaveBeenCalled(); + }); + it('removes references set to an empty list', async () => { const baseSkillHash = givenLatest( versionRow(skill, 2, {}, [{ path: 'references/a.md', content: 'a' }]), @@ -511,6 +607,65 @@ describe('SkillService', () => { ).rejects.toThrow('Other agents use this skill.'); expect(skills.updateTarget).not.toHaveBeenCalled(); }); + + it('refuses to move a skill into a scope that holds a name like it', async () => { + skills.findUsingAgentIds.mockResolvedValue([]); + skills.findLatestSummaries.mockResolvedValue( + new Map([['skill_a', { name: 'Brand voice', description: 'd', version: 1 }]]), + ); + skills.findLatestNamesInTarget.mockResolvedValue([ + { skillId: 'skill_x', name: 'brand_voice' }, + ]); + + await expect( + service.changeTarget('skill_a', { projectId: null, agentId: null }, undefined), + ).rejects.toThrow('That name is taken, choose another'); + expect(skills.findLatestNamesInTarget).toHaveBeenCalledWith( + { projectId: null, agentId: null }, + TX_CTX, + ); + expect(skills.updateTarget).not.toHaveBeenCalled(); + }); + + it('checks the name that a rename in the same request gives the skill', async () => { + skills.findUsingAgentIds.mockResolvedValue([]); + + await service.changeTarget( + 'skill_a', + { projectId: null, agentId: null }, + undefined, + {}, + 'Fresh name', + ); + + expect(skills.findLatestSummaries).not.toHaveBeenCalled(); + expect(skills.findLatestNamesInTarget).toHaveBeenCalledWith( + { projectId: null, agentId: null }, + TX_CTX, + ); + expect(skills.updateTarget).toHaveBeenCalled(); + }); + + it('refuses to move a skill that any agent uses when no agent is kept', async () => { + skills.findUsingAgentIds.mockResolvedValue(['agent-1']); + + await expect( + service.changeTarget('skill_a', { projectId: null, agentId: null }, undefined), + ).rejects.toThrow('Agents use this skill.'); + expect(skills.updateTarget).not.toHaveBeenCalled(); + }); + + it('moves an unused skill when no agent is kept', async () => { + skills.findUsingAgentIds.mockResolvedValue([]); + + await service.changeTarget('skill_a', { projectId: 'p1', agentId: null }, undefined); + + expect(skills.updateTarget).toHaveBeenCalledWith( + 'skill_a', + { projectId: 'p1', agentId: null }, + TX_CTX, + ); + }); }); describe('publish', () => { diff --git a/packages/cli/src/modules/agents/__tests__/skills-api.service.test.ts b/packages/cli/src/modules/agents/__tests__/skills-api.service.test.ts index d902cc1025d..f81724c9bbd 100644 --- a/packages/cli/src/modules/agents/__tests__/skills-api.service.test.ts +++ b/packages/cli/src/modules/agents/__tests__/skills-api.service.test.ts @@ -1,7 +1,7 @@ /* eslint-disable @typescript-eslint/unbound-method -- mock-based tests reference unbound methods */ import type { CreateSkillDto, ListSkillsQueryDto, SaveSkillDto } from '@n8n/api-types'; import type { ProjectScopeService } from '@n8n/backend-services'; -import type { User } from '@n8n/db'; +import type { OperationContext, TransactionRunner, User, UserRepository } from '@n8n/db'; import { ForbiddenError, NotFoundError, UserError } from '@n8n/errors'; import { hasGlobalScope } from '@n8n/permissions'; import { mock } from 'vitest-mock-extended'; @@ -25,12 +25,16 @@ vi.mock('@n8n/permissions', async (importOriginal) => ({ const user = mock({ id: 'user-1' }); const DATE = new Date('2026-10-01T00:00:00.000Z'); -function skill(id: string, target: Partial> = {}): Skill { +function skill( + id: string, + target: Partial> = {}, +): Skill { return mock({ id, projectId: target.projectId ?? null, agentId: target.agentId ?? null, source: 'ui', + createdById: target.createdById ?? null, createdAt: DATE, updatedAt: DATE, }); @@ -71,11 +75,21 @@ describe('SkillsApiService', () => { const repository = mock(); const skillService = mock(); const projectScopeService = mock(); - const service = new SkillsApiService(skillService, repository, projectScopeService); + const txRunner = mock(); + const userRepository = mock(); + const service = new SkillsApiService( + skillService, + repository, + projectScopeService, + txRunner, + userRepository, + ); + const TX_CTX = { trx: {} } as OperationContext; let visible: Skill[]; beforeEach(() => { vi.resetAllMocks(); + txRunner.run.mockImplementation(async (_ctx, fn) => await fn(TX_CTX)); visible = Object.keys(NAMES).map((id) => skill(id)); projectScopeService.getProjectIds.mockResolvedValue(['project-1']); vi.mocked(hasGlobalScope).mockReturnValue(false); @@ -102,8 +116,8 @@ describe('SkillsApiService', () => { new Map(visible.filter((s) => ids.includes(s.id)).map((s) => [s.id, row(s, 2)] as const)), ); repository.countUsingAgents.mockResolvedValue(new Map()); - repository.findProjectNames.mockImplementation( - async (ids) => new Map(ids.map((id) => [id, 'Team'] as const)), + repository.findProjects.mockImplementation( + async (ids) => new Map(ids.map((id) => [id, { name: 'Team', type: 'team' as const }])), ); repository.findUsage.mockResolvedValue({ drafts: [], pins: [], hiddenAgents: 0 }); repository.findAgents.mockImplementation( @@ -119,6 +133,39 @@ describe('SkillsApiService', () => { }); describe('list', () => { + it('names the creator of each skill, with one user query for the page', async () => { + visible = [ + skill('skill_a', { createdById: 'user-2' }), + skill('skill_b', { createdById: 'user-2' }), + skill('skill_c', { createdById: 'user-gone' }), + ]; + userRepository.findManyByIds.mockResolvedValue([ + mock({ + id: 'user-2', + email: 'ada@example.com', + firstName: 'Ada', + lastName: 'Lovelace', + }), + ]); + + const result = await service.list(user, query()); + + expect(userRepository.findManyByIds).toHaveBeenCalledTimes(1); + expect(userRepository.findManyByIds).toHaveBeenCalledWith(['user-2', 'user-gone']); + expect(result.data.map((item) => item.createdBy)).toEqual([ + { id: 'user-2', email: 'ada@example.com', firstName: 'Ada', lastName: 'Lovelace' }, + { id: 'user-2', email: 'ada@example.com', firstName: 'Ada', lastName: 'Lovelace' }, + null, + ]); + }); + + it('skips the user query when no skill has a creator', async () => { + const result = await service.list(user, query()); + + expect(userRepository.findManyByIds).not.toHaveBeenCalled(); + expect(result.data.every((item) => item.createdBy === null)).toBe(true); + }); + it('passes the filters and the page to the repository', async () => { const result = await service.list( user, @@ -193,7 +240,9 @@ describe('SkillsApiService', () => { it('describes each skill', async () => { visible = [skill('skill_a', { projectId: 'project-1' })]; repository.countUsingAgents.mockResolvedValue(new Map([['skill_a', 2]])); - repository.findProjectNames.mockResolvedValue(new Map([['project-1', 'Team']])); + repository.findProjects.mockResolvedValue( + new Map([['project-1', { name: 'Team', type: 'team' as const }]]), + ); skillService.accessCheck.mockImplementation(async (_user, op) => () => op === 'update'); const [item] = (await service.list(user, query())).data; @@ -205,9 +254,11 @@ describe('SkillsApiService', () => { scope: 'project', projectId: 'project-1', projectName: 'Team', + projectType: 'team', agentId: null, agentName: null, source: 'ui', + createdBy: null, latestVersion: 2, usedByAgents: 2, canEdit: true, @@ -219,7 +270,9 @@ describe('SkillsApiService', () => { it('describes an agent skill with its agent and the project of the agent', async () => { visible = [skill('skill_a', { agentId: 'agent-1' })]; - repository.findProjectNames.mockResolvedValue(new Map([['project-1', 'Team']])); + repository.findProjects.mockResolvedValue( + new Map([['project-1', { name: 'Team', type: 'team' as const }]]), + ); const check = vi.fn().mockReturnValue(true); skillService.accessCheck.mockResolvedValue(check); @@ -332,6 +385,7 @@ describe('SkillsApiService', () => { projectId: 'project-1', }); expect(skillService.create).toHaveBeenCalledWith({ + checkNameTaken: true, target: { projectId: null, agentId: 'agent-1' }, skill: body, source: 'ui', @@ -393,12 +447,12 @@ describe('SkillsApiService', () => { // A global projectSkill:create passes the scope check for any project id. it('refuses a project skill for a project that does not exist', async () => { vi.mocked(userHasScopes).mockResolvedValue(true); - repository.findProjectNames.mockResolvedValue(new Map()); + repository.findProjects.mockResolvedValue(new Map()); await expect( service.create(user, payload({ scope: 'project', projectId: 'project-gone' })), ).rejects.toThrow(NotFoundError); - expect(repository.findProjectNames).toHaveBeenCalledWith(['project-gone']); + expect(repository.findProjects).toHaveBeenCalledWith(['project-gone']); expect(skillService.create).not.toHaveBeenCalled(); }); }); @@ -452,6 +506,63 @@ describe('SkillsApiService', () => { service.update(user, 'skill_a', update({ instructions: 'New.' })), ).rejects.toThrow(NotFoundError); }); + + it('moves the skill and saves it in one transaction, then refreshes after the commit', async () => { + vi.mocked(userHasScopes).mockResolvedValue(true); + repository.findProjects.mockResolvedValue( + new Map([['project-1', { name: 'Team', type: 'team' as const }]]), + ); + skillService.save.mockResolvedValue(saved); + + await service.update( + user, + 'skill_a', + update({ scope: 'project', projectId: 'project-1', baseSkillHash: 'base' }), + ); + + expect(skillService.changeTarget).toHaveBeenCalledWith( + 'skill_a', + { projectId: 'project-1', agentId: null }, + undefined, + TX_CTX, + undefined, + ); + expect(skillService.save).toHaveBeenCalledWith( + 'skill_a', + { baseSkillHash: 'base' }, + user.id, + TX_CTX, + ); + expect(skillService.refreshFollowingAgents).toHaveBeenCalledWith(['agent-1']); + // The move runs first, so the save checks a rename in the new scope. + expect(skillService.changeTarget.mock.invocationCallOrder[0]).toBeLessThan( + skillService.save.mock.invocationCallOrder[0], + ); + }); + + it('does not move a skill that is already in the requested scope', async () => { + vi.mocked(hasGlobalScope).mockReturnValue(true); + skillService.save.mockResolvedValue(saved); + + await service.update(user, 'skill_a', update({ scope: 'instance', baseSkillHash: 'base' })); + + expect(skillService.changeTarget).not.toHaveBeenCalled(); + expect(txRunner.run).not.toHaveBeenCalled(); + }); + + it('refuses a move to a place where the user may not create skills', async () => { + vi.mocked(userHasScopes).mockResolvedValue(false); + + await expect( + service.update( + user, + 'skill_a', + update({ scope: 'project', projectId: 'project-1', baseSkillHash: 'base' }), + ), + ).rejects.toThrow(ForbiddenError); + expect(skillService.changeTarget).not.toHaveBeenCalled(); + expect(skillService.save).not.toHaveBeenCalled(); + }); }); describe('delete', () => { diff --git a/packages/cli/src/modules/agents/__tests__/skills.controller.test.ts b/packages/cli/src/modules/agents/__tests__/skills.controller.test.ts index 26dac7815e0..76d9efc74f8 100644 --- a/packages/cli/src/modules/agents/__tests__/skills.controller.test.ts +++ b/packages/cli/src/modules/agents/__tests__/skills.controller.test.ts @@ -4,10 +4,11 @@ import { Container } from '@n8n/di'; import { SkillsController } from '../skills.controller'; /** - * The skills routes carry no scope decorator: `SkillsApiService` checks each skill row. + * The skills routes carry no scope decorator: `SkillsApiService` checks each skill row, + * or for `newName` the create permission in the requested scope, as `create` does. * A new route must add that check on purpose, so the set of routes is pinned here. */ -const ROW_CHECKED_HANDLERS = ['create', 'delete', 'get', 'list', 'update']; +const ROW_CHECKED_HANDLERS = ['create', 'delete', 'get', 'list', 'newName', 'update']; describe('SkillsController routes', () => { const metadata = Container.get(ControllerRegistryMetadata).getControllerMetadata( diff --git a/packages/cli/src/modules/agents/agent-skill-refs.service.ts b/packages/cli/src/modules/agents/agent-skill-refs.service.ts index 1fad9144270..0705ffd6de9 100644 --- a/packages/cli/src/modules/agents/agent-skill-refs.service.ts +++ b/packages/cli/src/modules/agents/agent-skill-refs.service.ts @@ -359,6 +359,7 @@ export class AgentSkillRefsService { skill, source: 'agent', createdById: creatorId, + checkNameTaken: true, }, ctx, ), @@ -446,8 +447,16 @@ export class AgentSkillRefsService { skillId: string, availableTo: AgentSkillAvailability, ctx: OperationContext, + /** The name the skill has after this request, when the same request renames it. */ + nameAfterMove?: string, ): Promise { - await this.skillService.changeTarget(skillId, targetFor(agent, availableTo), agent.id, ctx); + await this.skillService.changeTarget( + skillId, + targetFor(agent, availableTo), + agent.id, + ctx, + nameAfterMove, + ); } private async writeLegacyDraft( diff --git a/packages/cli/src/modules/agents/agent-skills.service.ts b/packages/cli/src/modules/agents/agent-skills.service.ts index a8c43d341f3..e04958cb1f7 100644 --- a/packages/cli/src/modules/agents/agent-skills.service.ts +++ b/packages/cli/src/modules/agents/agent-skills.service.ts @@ -277,7 +277,14 @@ export class AgentSkillsService { if (pinned) markAgentDraftDirty(entity); const saved = await this.transactionRunner.run({}, async (ctx) => { if (nextAvailability) { - await this.agentSkillRefs.setAvailability(entity, skillId, nextAvailability, ctx); + // The move checks the name the skill has after the save, not the old one. + await this.agentSkillRefs.setAvailability( + entity, + skillId, + nextAvailability, + ctx, + updated.name, + ); } if (pinned) { await saveAgentDraftFenced(this.agentRepository, entity, ctx); diff --git a/packages/cli/src/modules/agents/builder/agents-builder-tools.service.ts b/packages/cli/src/modules/agents/builder/agents-builder-tools.service.ts index 4f2e1004330..884a5b02851 100644 --- a/packages/cli/src/modules/agents/builder/agents-builder-tools.service.ts +++ b/packages/cli/src/modules/agents/builder/agents-builder-tools.service.ts @@ -95,6 +95,7 @@ import { composeJsonConfig } from '../json-config/agent-config-composition'; import { listAiGatewayManagedCredentialTypes } from '../json-config/reconcile-node-tool-gateway-credentials'; import { AgentSecureRuntime } from '../runtime/agent-secure-runtime'; import { getAgentConfigHash, isSameAgentConfig } from '../utils/agent-config-hash'; +import { SkillNameTakenError, skillNameTakenToolMessage } from '../skills/skill-name-taken.error'; const STALE_CONFIG_ERROR: ConfigValidationError = { path: '(root)', @@ -181,6 +182,12 @@ type EditorLockFailure = { ok: false; errors: ConfigValidationError[] }; const STALE_SKILL_ERROR_MESSAGE = 'Skill changed. Call agent-context with type "skill", then retry with its skill and skillHash.'; +/** A taken name is a conflict too, but a retry with a fresh hash would fail the same way. */ +function updateSkillErrorMessage(error: unknown, message: string): string { + if (error instanceof SkillNameTakenError) return skillNameTakenToolMessage(error.skillName); + return error instanceof ConflictError ? STALE_SKILL_ERROR_MESSAGE : message; +} + /** LLM-facing follow-up guidance for this builder surface (CLI skill-based tools). */ const CLI_AGENT_CONFIG_MESSAGES: AgentConfigValidationMessages = { emptyInstructionsFollowUp: 'saving the config again.', @@ -191,7 +198,10 @@ const CLI_AGENT_CONFIG_MESSAGES: AgentConfigValidationMessages = { const createSkillInputSchema = z .object({ - name: agentSkillSchema.shape.name.describe('Human-readable skill name'), + name: agentSkillSchema.shape.name.describe( + 'Human-readable skill name. It must be unique in the project: no other skill of this ' + + 'project may have a name like it (case, spaces, hyphens and underscores do not count).', + ), description: agentSkillSchema.shape.description.describe(SKILL_DESCRIPTION_RULE), instructions: agentSkillSchema.shape.instructions.describe(SKILL_BODY_GUIDANCE), references: agentSkillSchema.shape.references @@ -1179,10 +1189,7 @@ export class AgentsBuilderToolsService { }; } catch (e) { const message = e instanceof Error ? e.message : String(e); - return { - ok: false, - errors: [{ message: e instanceof ConflictError ? STALE_SKILL_ERROR_MESSAGE : message }], - }; + return { ok: false, errors: [{ message: updateSkillErrorMessage(e, message) }] }; } }) .build(); @@ -1241,10 +1248,13 @@ export class AgentsBuilderToolsService { skills: created.skills.map(({ id, skill }) => ({ id, name: skill.name })), }; } catch (e) { - return { - ok: false, - errors: [{ message: e instanceof Error ? e.message : String(e) }], - }; + const message = + e instanceof SkillNameTakenError + ? `${skillNameTakenToolMessage(e.skillName)} The batch is all-or-nothing, so send every skill again.` + : e instanceof Error + ? e.message + : String(e); + return { ok: false, errors: [{ message }] }; } }) .build(); diff --git a/packages/cli/src/modules/agents/repositories/skill.repository.ts b/packages/cli/src/modules/agents/repositories/skill.repository.ts index e485f9122cc..2e31221e853 100644 --- a/packages/cli/src/modules/agents/repositories/skill.repository.ts +++ b/packages/cli/src/modules/agents/repositories/skill.repository.ts @@ -204,6 +204,30 @@ export class SkillRepository extends BaseRepository { ); } + /** + * The name of the latest version of every skill in one target: the same agent, the + * same project (its agent skills not included) or the instance. For the name check. + */ + async findLatestNamesInTarget( + target: SkillTarget, + ctx: OperationContext = {}, + ): Promise> { + const query = highestVersionsQuery(this.managerFor(ctx)) + .innerJoin(Skill, 'skill', 'skill.id = version.skillId') + .select(['version.skillId', 'version.name']); + if (target.agentId) { + query.andWhere('skill.agentId = :agentId', { agentId: target.agentId }); + } else if (target.projectId) { + query + .andWhere('skill.projectId = :projectId', { projectId: target.projectId }) + .andWhere('skill.agentId IS NULL'); + } else { + query.andWhere('skill.projectId IS NULL').andWhere('skill.agentId IS NULL'); + } + const versions = await query.getMany(); + return versions.map(({ skillId, name }) => ({ skillId, name })); + } + /** Version rows by id, keyed by version id. A pinned ref reads them. */ async findVersionsByIds( versionIds: string[], @@ -648,17 +672,23 @@ export class SkillRepository extends BaseRepository { return new Map([...agentsBySkill].map(([skillId, agents]) => [skillId, agents.size])); } - async findProjectNames( + /** Name and type of each project, keyed by project id. A missing project is absent. */ + async findProjects( projectIds: string[], ctx: OperationContext = {}, - ): Promise> { + ): Promise>> { const manager = this.managerFor(ctx); const projects = await inChunks( projectIds, async (chunk) => - await manager.find(Project, { where: { id: In(chunk) }, select: ['id', 'name'] }), + await manager.find(Project, { + where: { id: In(chunk) }, + select: ['id', 'name', 'type'], + }), + ); + return new Map( + projects.map((project) => [project.id, { name: project.name, type: project.type }]), ); - return new Map(projects.map((project) => [project.id, project.name])); } /** Name and project of each agent, keyed by agent id. An agent skill takes its project from them. */ @@ -784,20 +814,24 @@ async function inChunks(ids: string[], read: (chunk: string[]) => Promise { + const highest = qb + .subQuery() + .select('MAX(later.version)') + .from(SkillVersion, 'later') + .where('later.skillId = version.skillId') + .getQuery(); + return `version.version = ${highest}`; + }); +} + /** The version rows of these skills that have the highest number of their skill. */ function latestVersionsQuery(manager: EntityManager, skillIds: string[]) { - return manager - .createQueryBuilder(SkillVersion, 'version') - .where('version.skillId IN (:...skillIds)', { skillIds }) - .andWhere((qb) => { - const highest = qb - .subQuery() - .select('MAX(later.version)') - .from(SkillVersion, 'later') - .where('later.skillId = version.skillId') - .getQuery(); - return `version.version = ${highest}`; - }); + return highestVersionsQuery(manager).andWhere('version.skillId IN (:...skillIds)', { + skillIds, + }); } async function insertVersion( diff --git a/packages/cli/src/modules/agents/skills.controller.ts b/packages/cli/src/modules/agents/skills.controller.ts index 6435f184398..3229549ff51 100644 --- a/packages/cli/src/modules/agents/skills.controller.ts +++ b/packages/cli/src/modules/agents/skills.controller.ts @@ -1,4 +1,9 @@ -import { CreateSkillDto, ListSkillsQueryDto, SaveSkillDto } from '@n8n/api-types'; +import { + CreateSkillDto, + ListSkillsQueryDto, + SaveSkillDto, + SkillNewNameQueryDto, +} from '@n8n/api-types'; import type { AuthenticatedRequest } from '@n8n/db'; import { Body, @@ -42,6 +47,12 @@ export class SkillsController { return await this.skillsApi.list(req.user, query); } + /** Registered before `/:skillId`, which would take `new-name` as an id. */ + @Get('/new-name') + async newName(req: AuthenticatedRequest, _res: Response, @Query query: SkillNewNameQueryDto) { + return await this.skillsApi.newName(req.user, query); + } + @Get('/:skillId') async get(req: AuthenticatedRequest, _res: Response, @Param('skillId') skillId: string) { return await this.skillsApi.get(req.user, skillId); diff --git a/packages/cli/src/modules/agents/skills/skill-name-taken.error.ts b/packages/cli/src/modules/agents/skills/skill-name-taken.error.ts new file mode 100644 index 00000000000..bac9e1b564d --- /dev/null +++ b/packages/cli/src/modules/agents/skills/skill-name-taken.error.ts @@ -0,0 +1,21 @@ +import { SKILL_NAME_TAKEN_MESSAGE } from '@n8n/api-types'; +import { ConflictError } from '@n8n/errors'; + +/** + * A skill name is taken in its scope. It is a conflict like a stale save, but AI tools + * must not report it as one: a retry with the same name fails again. So the error + * carries the name, and each tool tells the model which name to change. + */ +export class SkillNameTakenError extends ConflictError { + constructor(readonly skillName: string) { + super(SKILL_NAME_TAKEN_MESSAGE); + } +} + +/** The message AI tools return for a taken name. Nothing is saved when it happens. */ +export function skillNameTakenToolMessage(skillName: string): string { + return ( + `The skill name "${skillName}" is taken by another skill in the same scope, so nothing ` + + 'was saved. Choose a different name and retry.' + ); +} diff --git a/packages/cli/src/modules/agents/skills/skill.service.ts b/packages/cli/src/modules/agents/skills/skill.service.ts index aa656044fbd..9488daead13 100644 --- a/packages/cli/src/modules/agents/skills/skill.service.ts +++ b/packages/cli/src/modules/agents/skills/skill.service.ts @@ -1,4 +1,4 @@ -import type { AgentSkill } from '@n8n/api-types'; +import { AGENT_SKILL_NAME_MAX_LENGTH, skillNameKey, type AgentSkill } from '@n8n/api-types'; import { Logger } from '@n8n/backend-common'; import { ProjectScopeService } from '@n8n/backend-services'; import { TransactionRunner, type OperationContext, type User } from '@n8n/db'; @@ -20,6 +20,7 @@ import { } from '../repositories/skill.repository'; import { getAgentSkillHash } from '../utils/agent-config-hash'; import { skillContentHash, type SkillContent } from './skill-content-hash'; +import { SkillNameTakenError } from './skill-name-taken.error'; import { findIntroducedNameClashes, renameIntroducesClash } from './skill-names'; /** A skill ref in an agent config. A ref with `versionId` runs that version. */ @@ -161,11 +162,24 @@ export class SkillService { // ---------------------------------------------------------------- writes /** Creates a skill with its v1. Returns the new id. */ + /** + * `checkNameTaken` is for a name that a user or the builder chose. A copy of an agent + * skill for a new agent keeps the name of the skill it copies, so it skips the check. + */ async create( - input: { target: SkillTarget; skill: AgentSkill; source: SkillSource; createdById: string }, + input: { + target: SkillTarget; + skill: AgentSkill; + source: SkillSource; + createdById: string; + checkNameTaken?: boolean; + }, ctx: OperationContext = {}, ): Promise { assertHasInstructions(input.skill); + if (input.checkNameTaken) { + await this.assertNameFree(input.skill.name, input.target, undefined, ctx); + } const id = `skill_${generateNanoId()}`; await this.skillRepository.createSkill( { id, target: input.target, source: input.source, createdById: input.createdById }, @@ -213,7 +227,10 @@ export class SkillService { const result = { ...unchanged, created: false, skill: current, followingAgentIds: [] }; return { saved: result, following: [] }; } - if (skill.name !== latest.version.name) { + // A rename that only changes case or separators keeps the name key, so it can + // clash with nothing new. The legacy rule skipped it too. + if (skillNameKey(skill.name) !== skillNameKey(latest.version.name)) { + await this.assertNameFree(skill.name, latest.skill, skillId, txCtx); await this.assertRenameKeepsNamesApart(skillId, latest.version.name, skill.name, txCtx); } const version = await this.skillRepository.nextVersionNumber(skillId, txCtx); @@ -257,6 +274,50 @@ export class SkillService { } } + /** + * The requested name if its scope has no name like it, else the name with the first + * free number after it ("New skill 2"), as workflow names get. + */ + async freeName(requested: string, target: SkillTarget, ctx: OperationContext = {}) { + const taken = new Set( + ( + await this.skillRepository.findLatestNamesInTarget( + { projectId: target.projectId, agentId: target.agentId }, + ctx, + ) + ).map(({ name }) => skillNameKey(name)), + ); + const base = requested.trim(); + if (!taken.has(skillNameKey(base))) return base; + for (let n = 2; ; n++) { + const suffix = ` ${n}`; + const candidate = `${base.slice(0, AGENT_SKILL_NAME_MAX_LENGTH - suffix.length)}${suffix}`; + if (!taken.has(skillNameKey(candidate))) return candidate; + } + } + + /** + * Refuses a name that is hard to tell apart from another skill's name in the same + * scope: one agent, one project or the instance, by the same rule as inside one agent. + */ + private async assertNameFree( + name: string, + target: SkillTarget, + exceptSkillId: string | undefined, + ctx: OperationContext, + ): Promise { + const key = skillNameKey(name); + const names = await this.skillRepository.findLatestNamesInTarget( + { projectId: target.projectId, agentId: target.agentId }, + ctx, + ); + if ( + names.some((other) => other.skillId !== exceptSkillId && skillNameKey(other.name) === key) + ) { + throw new SkillNameTakenError(name); + } + } + private async assertRenameKeepsNamesApart( skillId: string, oldName: string, @@ -416,14 +477,17 @@ export class SkillService { /** * Moves a skill to a project, one agent or the instance. Refused while an agent other - * than `keepAgentId` uses it. The edit lock makes a concurrent attach wait, so the check - * and the move see the same agents. + * than `keepAgentId` uses it, or while any agent uses it when there is no `keepAgentId`. + * The edit lock makes a concurrent attach wait, so the check and the move see the same + * agents. */ async changeTarget( skillId: string, target: SkillTarget, - keepAgentId: string, + keepAgentId: string | undefined, ctx: OperationContext = {}, + /** The name the skill has after this request, when the same request renames it. */ + nameAfterMove?: string, ): Promise { await this.txRunner.run(ctx, async (txCtx) => { await this.skillRepository.lockForEdit([skillId], txCtx); @@ -432,9 +496,16 @@ export class SkillService { ); if (others.length > 0) { throw new ConflictError( - 'Other agents use this skill. Detach it from those agents first, then change where it can be added.', + keepAgentId + ? 'Other agents use this skill. Detach it from those agents first, then change where it can be added.' + : 'Agents use this skill. Detach it from every agent first, then change where it can be added.', ); } + // The new scope must not already hold a name like the one the skill has after the move. + const name = + nameAfterMove ?? + (await this.skillRepository.findLatestSummaries([skillId], txCtx)).get(skillId)?.name; + if (name) await this.assertNameFree(name, target, skillId, txCtx); await this.skillRepository.updateTarget(skillId, target, txCtx); }); } diff --git a/packages/cli/src/modules/agents/skills/skills-api.service.ts b/packages/cli/src/modules/agents/skills/skills-api.service.ts index b6e29aa4d7e..784f2f9db3c 100644 --- a/packages/cli/src/modules/agents/skills/skills-api.service.ts +++ b/packages/cli/src/modules/agents/skills/skills-api.service.ts @@ -7,9 +7,12 @@ import type { SkillSaveResponse, SaveSkillDto, SkillScope, + SkillUserDto, + SkillNewNameQueryDto, + SkillNewNameResponse, } from '@n8n/api-types'; import { ProjectScopeService } from '@n8n/backend-services'; -import type { User } from '@n8n/db'; +import { TransactionRunner, UserRepository, type User } from '@n8n/db'; import { Service } from '@n8n/di'; import { ForbiddenError, NotFoundError, UserError } from '@n8n/errors'; import { hasGlobalScope } from '@n8n/permissions'; @@ -38,6 +41,8 @@ export class SkillsApiService { private readonly skillService: SkillService, private readonly skillRepository: SkillRepository, private readonly projectScopeService: ProjectScopeService, + private readonly txRunner: TransactionRunner, + private readonly userRepository: UserRepository, ) {} async list(user: User, query: ListSkillsQueryDto): Promise { @@ -64,9 +69,16 @@ export class SkillsApiService { return await this.toDetail(user, await this.requireAccess(user, skillId, 'read')); } + /** A free name for a new skill. Needs the create permission there, so it shows no other scope. */ + async newName(user: User, query: SkillNewNameQueryDto): Promise { + const target = await this.targetForCreate(user, query); + return { name: await this.skillService.freeName(query.name, target) }; + } + async create(user: User, payload: CreateSkillDto): Promise { const target = await this.targetForCreate(user, payload); const id = await this.skillService.create({ + checkNameTaken: true, target, skill: payload.skill, source: 'ui', @@ -80,12 +92,37 @@ export class SkillsApiService { /** Saves the changes as the next version. Unchanged content creates nothing. */ async update(user: User, skillId: string, payload: SaveSkillDto): Promise { - await this.requireAccess(user, skillId, 'update'); - const { followingAgentIds: _following, ...saved } = await this.skillService.save( - skillId, - { ...payload }, - user.id, - ); + const skill = await this.requireAccess(user, skillId, 'update'); + const { scope, projectId, ...update } = payload; + const target = scope ? await this.targetForCreate(user, { scope, projectId }) : undefined; + const moves = + target !== undefined && + (target.projectId !== skill.projectId || target.agentId !== skill.agentId); + + // A move takes the skill out of its scope, as a delete does, so it needs both rights + // there. A custom role can grant update without delete. + if (moves && !(await this.skillService.canAccess(user, skill, 'delete'))) { + throw new ForbiddenError('You are not allowed to move this skill out of its scope.'); + } + + if (!moves) { + const { followingAgentIds: _following, ...saved } = await this.skillService.save( + skillId, + update, + user.id, + ); + return { id: skillId, ...saved, skillHash: getAgentSkillHash(saved.skill) }; + } + + // One transaction, so a refused save does not leave the skill moved. The move runs + // first and checks the final name in the new scope only: a name that only the old + // scope holds does not block it. The save then checks a rename in the new scope too. + const { followingAgentIds, ...saved } = await this.txRunner.run({}, async (ctx) => { + await this.skillService.changeTarget(skillId, target, undefined, ctx, update.name); + return await this.skillService.save(skillId, update, user.id, ctx); + }); + // `save` leaves the refresh to the caller when it joins a transaction. + await this.skillService.refreshFollowingAgents(followingAgentIds); return { id: skillId, ...saved, skillHash: getAgentSkillHash(saved.skill) }; } @@ -146,7 +183,11 @@ export class SkillsApiService { return { agentId, projectId: agent.projectId }; } - private async targetForCreate(user: User, payload: CreateSkillDto): Promise { + /** Where a create or a move puts the skill. The user needs the create permission there. */ + private async targetForCreate( + user: User, + payload: Pick, + ): Promise { switch (payload.scope) { case 'instance': if (payload.projectId || payload.agentId) { @@ -162,7 +203,7 @@ export class SkillsApiService { if (payload.agentId) throw new UserError('A project skill has no agent.'); await this.assertCanCreateIn(user, projectId); // A global scope passes without looking at the project, so check that it exists. - if (!(await this.skillRepository.findProjectNames([projectId])).has(projectId)) { + if (!(await this.skillRepository.findProjects([projectId])).has(projectId)) { throw new NotFoundError('Project not found'); } // A personal project keeps its own skills, as a team project does. @@ -195,10 +236,11 @@ export class SkillsApiService { ); const projectOf = (skill: Skill) => skill.projectId ?? (skill.agentId ? (agents.get(skill.agentId)?.projectId ?? null) : null); - const [latest, usage, projects, canEdit, canDelete] = await Promise.all([ + const [latest, usage, projects, creators, canEdit, canDelete] = await Promise.all([ this.skillRepository.findLatestSummaries(ids), this.skillRepository.countUsingAgents(ids), - this.skillRepository.findProjectNames(skills.flatMap((skill) => projectOf(skill) ?? [])), + this.skillRepository.findProjects(skills.flatMap((skill) => projectOf(skill) ?? [])), + this.findCreators(skills), this.skillService.accessCheck(user, 'update'), this.skillService.accessCheck(user, 'delete'), ]); @@ -213,10 +255,12 @@ export class SkillsApiService { description: summary.description, scope: scopeOf(skill), projectId, - projectName: projectId ? (projects.get(projectId) ?? null) : null, + projectName: projectId ? (projects.get(projectId)?.name ?? null) : null, + projectType: projectId ? (projects.get(projectId)?.type ?? null) : null, agentId: skill.agentId, agentName: skill.agentId ? (agents.get(skill.agentId)?.name ?? null) : null, source: skill.source, + createdBy: skill.createdById ? (creators.get(skill.createdById) ?? null) : null, latestVersion: summary.version, usedByAgents: usage.get(skill.id) ?? 0, canEdit: canEdit(skill, projectId), @@ -227,4 +271,22 @@ export class SkillsApiService { } return items; } + + /** The creators of a page of skills, in one query. */ + private async findCreators(skills: Skill[]): Promise> { + const ids = [...new Set(skills.flatMap((skill) => skill.createdById ?? []))]; + if (ids.length === 0) return new Map(); + const users = await this.userRepository.findManyByIds(ids); + return new Map( + users.map((creator) => [ + creator.id, + { + id: creator.id, + email: creator.email ?? null, + firstName: creator.firstName ?? null, + lastName: creator.lastName ?? null, + }, + ]), + ); + } } diff --git a/packages/cli/src/modules/mcp/__tests__/agent-tools.service.test.ts b/packages/cli/src/modules/mcp/__tests__/agent-tools.service.test.ts index 3099718dfb7..bf0aad8aaac 100644 --- a/packages/cli/src/modules/mcp/__tests__/agent-tools.service.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/agent-tools.service.test.ts @@ -30,6 +30,8 @@ vi.mock('@/modules/agents/json-config/mcp-client-factory', () => ({ import { CredentialsService } from '@/credentials/credentials.service'; import { ConflictError } from '@n8n/errors'; + +import { SkillNameTakenError } from '@/modules/agents/skills/skill-name-taken.error'; import { AgentConfigPreparationService } from '@/modules/agents/agent-config-preparation.service'; import { AgentConfigService } from '@/modules/agents/agent-config.service'; import { AgentCustomToolsService } from '@/modules/agents/agent-custom-tools.service'; @@ -725,6 +727,26 @@ describe('McpAgentToolsService', () => { }); }); + it('names a taken skill name instead of reporting a stale config', async () => { + agentSkillsService.createAndAttachSkill.mockRejectedValue(new SkillNameTakenError('Skill')); + // Another edit changed the agent while the rejected create ran. + const latestConfig = { ...composedConfig, instructions: 'Newer work' }; + agentConfigService.getConfig.mockResolvedValue(latestConfig); + + const result = await callTool( + 'mutate_agent', + mutateInput({ type: 'skill.upsert', skill: { name: 'Skill', body: 'do it' } }), + ); + + expect(result.structuredContent).toMatchObject({ + ok: false, + code: 'skill_name_taken', + agentId: 'agent-1', + configHash: getAgentConfigHash(latestConfig), + message: expect.stringContaining('The skill name "Skill" is taken'), + }); + }); + it('updates an existing skill when skillId is given', async () => { agentSkillsService.updateSkill.mockResolvedValue({ id: 'skill-1' } as never); diff --git a/packages/cli/src/modules/mcp/tools/agents/agent-tools.service.ts b/packages/cli/src/modules/mcp/tools/agents/agent-tools.service.ts index e802285a206..2188f9763a5 100644 --- a/packages/cli/src/modules/mcp/tools/agents/agent-tools.service.ts +++ b/packages/cli/src/modules/mcp/tools/agents/agent-tools.service.ts @@ -37,6 +37,10 @@ import { AgentModelCatalogService } from '@/modules/agents/agent-model-catalog.s import { AgentPublishService } from '@/modules/agents/agent-publish.service'; import { AgentSkillRefsService } from '@/modules/agents/agent-skill-refs.service'; import { AgentSkillsService } from '@/modules/agents/agent-skills.service'; +import { + SkillNameTakenError, + skillNameTakenToolMessage, +} from '@/modules/agents/skills/skill-name-taken.error'; import { AgentTaskService } from '@/modules/agents/agent-task.service'; import { AgentTestRunService, @@ -703,6 +707,17 @@ export class McpAgentToolsService { try { mutation = await this.applyMutation(user, input, config, projectId, configHash); } catch (error) { + // A taken name is a conflict too, but a retry with a fresh config would fail the same way. + if (error instanceof SkillNameTakenError) { + return { + ok: false, + code: 'skill_name_taken', + agentId: input.agentId, + // The agent may have changed meanwhile, so send the hash the next call needs. + configHash: await this.fetchConfigHash(projectId, input.agentId), + message: skillNameTakenToolMessage(error.skillName), + }; + } if (!(error instanceof ConflictError)) throw error; const latestConfigHash = await this.fetchConfigHash(projectId, input.agentId); if (latestConfigHash === configHash) throw error; diff --git a/packages/cli/test/integration/agents/agent-skills-hub.test.ts b/packages/cli/test/integration/agents/agent-skills-hub.test.ts index ed8b8c38101..dd799f2928d 100644 --- a/packages/cli/test/integration/agents/agent-skills-hub.test.ts +++ b/packages/cli/test/integration/agents/agent-skills-hub.test.ts @@ -246,6 +246,32 @@ describe('Agent skills in the skills hub', () => { ).rejects.toThrow('This agent uses an older version of this skill.'); }); + it('refuses a skill from the agent builder whose name a skill of its project has', async () => { + await createSkill(body('Triage')); + const agent = await createAgent([]); + + await expect( + agentSkillsService.createAndAttachSkill(agent.id, project.id, body('TRIAGE'), { + user: editor, + modifiedBy: 'builder', + }), + ).rejects.toThrow('That name is taken, choose another'); + }); + + it('lets the agent builder use a name that only an instance skill has', async () => { + await createSkill(body('Triage'), { projectId: null }); + const agent = await createAgent([]); + + const created = await agentSkillsService.createAndAttachSkill( + agent.id, + project.id, + body('Triage'), + { user: editor, modifiedBy: 'builder' }, + ); + + expect(created.id).toBeDefined(); + }); + it('creates, updates and detaches a skill from the agent builder', async () => { const agent = await createAgent([]); const context = { user: editor, modifiedBy: 'builder' as const }; @@ -411,6 +437,39 @@ describe('Agent skills in the skills hub', () => { await expect(update('agent')).rejects.toThrow('Other agents use this skill.'); }); + it('renames and moves a skill in one save to a name that only the old scope holds', async () => { + const agent = await createAgent([]); + const context = { user: editor, modifiedBy: 'user' as const }; + const created = await agentSkillsService.createAndAttachSkill( + agent.id, + project.id, + body('Summarize'), + context, + ); + const update = async (updates: Partial, availableTo: 'project' | 'agent') => + await agentSkillsService.updateSkill( + agent.id, + project.id, + created.id, + updates, + context, + undefined, + undefined, + availableTo, + ); + await update({}, 'agent'); + await createSkill(body('summarize')); + + await update({ name: 'Condense' }, 'project'); + + expect((await Container.get(SkillRepository).findByIds([created.id]))[0]).toMatchObject({ + projectId: project.id, + agentId: null, + }); + const { skills } = await skillRefs.readDraft(await reload(agent), {}); + expect(skills[created.id]?.name).toBe('Condense'); + }); + it('shares the skills of a duplicated agent', async () => { const skill = await createSkill(body('Shared')); const original = await createAgent([skill]); diff --git a/packages/cli/test/integration/agents/skill.repository.test.ts b/packages/cli/test/integration/agents/skill.repository.test.ts index 19626098c27..f167dec6664 100644 --- a/packages/cli/test/integration/agents/skill.repository.test.ts +++ b/packages/cli/test/integration/agents/skill.repository.test.ts @@ -1,4 +1,9 @@ -import { createTeamProject, testDb, testModules } from '@n8n/backend-test-utils'; +import { + createTeamProject, + getPersonalProject, + testDb, + testModules, +} from '@n8n/backend-test-utils'; import { TransactionRunner, type Project, type User } from '@n8n/db'; import { Container } from '@n8n/di'; import { randomUUID } from 'node:crypto'; @@ -177,6 +182,27 @@ describe('SkillRepository', () => { }); }); + describe('findLatestNamesInTarget', () => { + it('returns the latest name of each skill in one target only', async () => { + const instance = await createSkill({}, content({ name: 'Old name' })); + await skills.insertSavedVersion(instance, 2, content({ name: 'New name' }), null); + const inProject = await createSkill({ projectId: project.id }, content({ name: 'P' })); + const agent = await createAgent('Agent', { projectId: project.id }); + const ofAgent = await createSkill({ agentId: agent.id }, content({ name: 'A' })); + + expect(await skills.findLatestNamesInTarget({ projectId: null, agentId: null })).toEqual([ + { skillId: instance, name: 'New name' }, + ]); + // The agent skills of a project are not project skills. + expect( + await skills.findLatestNamesInTarget({ projectId: project.id, agentId: null }), + ).toEqual([{ skillId: inProject, name: 'P' }]); + expect(await skills.findLatestNamesInTarget({ projectId: null, agentId: agent.id })).toEqual([ + { skillId: ofAgent, name: 'A' }, + ]); + }); + }); + describe('findVisiblePage', () => { const firstPage = { skip: 0 }; const ids = (page: { skills: Array<{ id: string }> }) => page.skills.map((skill) => skill.id); @@ -666,14 +692,22 @@ describe('SkillRepository', () => { }); }); - describe('findProjectNames', () => { - it('maps project ids to names', async () => { - const names = await skills.findProjectNames([project.id, project.id, otherProject.id]); + describe('findProjects', () => { + it('maps project ids to names and types', async () => { + const personal = await getPersonalProject(user); - expect(names).toEqual( + const projects = await skills.findProjects([ + project.id, + project.id, + otherProject.id, + personal.id, + ]); + + expect(projects).toEqual( new Map([ - [project.id, 'Team'], - [otherProject.id, 'Other team'], + [project.id, { name: 'Team', type: 'team' }], + [otherProject.id, { name: 'Other team', type: 'team' }], + [personal.id, { name: personal.name, type: 'personal' }], ]), ); }); diff --git a/packages/cli/test/integration/agents/skills.controller.test.ts b/packages/cli/test/integration/agents/skills.controller.test.ts index c6a237f8df3..38efe7e3509 100644 --- a/packages/cli/test/integration/agents/skills.controller.test.ts +++ b/packages/cli/test/integration/agents/skills.controller.test.ts @@ -7,8 +7,10 @@ import { setSkillsHubFlag } from '@/modules/agents/__tests__/test-utils/skills-h import { AgentHistoryRepository } from '@/modules/agents/repositories/agent-history.repository'; import { AgentRepository } from '@/modules/agents/repositories/agent.repository'; import { SkillRepository } from '@/modules/agents/repositories/skill.repository'; +import { SkillService } from '@/modules/agents/skills/skill.service'; -import { createMember, createOwner } from '../shared/db/users'; +import { createCustomRoleWithScopeSlugs } from '../shared/db/roles'; +import { createMember, createOwner, createUser } from '../shared/db/users'; import type { SuperAgentTest } from '../shared/types'; import { setupTestServer } from '../shared/utils'; @@ -129,6 +131,7 @@ describe('SkillsController', () => { agentName: 'Support bot', projectId: project.id, projectName: 'Team', + projectType: 'team', latestVersion: 1, canEdit: true, canDelete: true, @@ -223,6 +226,17 @@ describe('SkillsController', () => { }); describe('list', () => { + it('names the user who created each skill', async () => { + await create(owner, { scope: 'instance', skill: body }); + + const response = await owner.get('/skills').expect(200); + + expect(response.body.data.data[0].createdBy).toMatchObject({ + id: ownerUser.id, + email: ownerUser.email, + }); + }); + it('lists what the user may see, searched and paged on the server', async () => { const agent = await newAgent('Support bot', project.id); await create(editor, { scope: 'agent', agentId: agent.id, skill: { ...body, name: 'Mine' } }); @@ -388,6 +402,296 @@ describe('SkillsController', () => { }); }); + describe('unique names', () => { + it('refuses a name like one that another skill in the same scope has', async () => { + await create(editor, { scope: 'project', projectId: project.id, skill: body }); + + const response = await editor + .post('/skills') + .send({ scope: 'project', projectId: project.id, skill: { ...body, name: 'brand-voice' } }) + .expect(409); + + expect(response.body.message).toBe('That name is taken, choose another'); + }); + + it('allows the same name in another scope', async () => { + await create(editor, { scope: 'project', projectId: project.id, skill: body }); + + await create(owner, { scope: 'instance', skill: { ...body, name: 'brand-voice' } }); + }); + + it('allows the same name in two team projects', async () => { + const other = await createTeamProject('Other team'); + + await create(owner, { scope: 'project', projectId: project.id, skill: body }); + await create(owner, { scope: 'project', projectId: other.id, skill: body }); + }); + + it('allows a rename that only changes case, even when another skill shares the name', async () => { + // The startup copy can leave two skills with one name key in a scope. + const { id, skillHash } = await create(owner, { scope: 'instance', skill: body }); + await Container.get(SkillService).create({ + target: { projectId: null, agentId: null }, + skill: { ...body, name: 'brand voice' }, + source: 'ui', + createdById: ownerUser.id, + }); + + await owner + .patch(`/skills/${id}`) + .send({ name: 'BRAND VOICE', baseSkillHash: skillHash }) + .expect(200); + }); + + it('renames and moves a skill to a name that only the old scope holds', async () => { + await create(owner, { scope: 'project', projectId: project.id, skill: body }); + const { id, skillHash } = await create(owner, { + scope: 'project', + projectId: project.id, + skill: { ...body, name: 'Code review' }, + }); + + await owner + .patch(`/skills/${id}`) + .send({ name: 'brand-voice', scope: 'instance', baseSkillHash: skillHash }) + .expect(200); + const read = await owner.get(`/skills/${id}`).expect(200); + + expect(read.body.data).toMatchObject({ name: 'brand-voice', scope: 'instance' }); + }); + + it('refuses to move a skill into a scope that holds a name like it', async () => { + await create(owner, { scope: 'instance', skill: body }); + const { id, skillHash } = await create(owner, { + scope: 'project', + projectId: project.id, + skill: { ...body, name: 'BRAND VOICE' }, + }); + + await owner + .patch(`/skills/${id}`) + .send({ scope: 'instance', baseSkillHash: skillHash }) + .expect(409); + const read = await owner.get(`/skills/${id}`).expect(200); + + expect(read.body.data).toMatchObject({ scope: 'project', projectId: project.id }); + }); + + it('refuses a rename to a taken name and allows a save that keeps the name', async () => { + await create(owner, { scope: 'instance', skill: { ...body, name: 'Code review' } }); + const { id, skillHash } = await create(owner, { scope: 'instance', skill: body }); + + await owner + .patch(`/skills/${id}`) + .send({ name: 'Code Review', baseSkillHash: skillHash }) + .expect(409); + await owner + .patch(`/skills/${id}`) + .send({ instructions: 'Be very brief.', baseSkillHash: skillHash }) + .expect(200); + }); + }); + + describe('new name', () => { + const newName = async (agent: SuperAgentTest, query: Record, status = 200) => + (await agent.get('/skills/new-name').query(query).expect(status)).body.data as { + name: string; + }; + + it('numbers a default name that its scope already holds', async () => { + await create(editor, { + scope: 'project', + projectId: project.id, + skill: { ...body, name: 'New skill' }, + }); + await create(editor, { + scope: 'project', + projectId: project.id, + skill: { ...body, name: 'New skill 2' }, + }); + + const free = await newName(editor, { + scope: 'project', + projectId: project.id, + name: 'New skill', + }); + + expect(free).toEqual({ name: 'New skill 3' }); + }); + + it('looks at the requested scope only', async () => { + await create(owner, { scope: 'instance', skill: { ...body, name: 'New skill' } }); + + const free = await newName(editor, { + scope: 'project', + projectId: project.id, + name: 'New skill', + }); + + expect(free).toEqual({ name: 'New skill' }); + }); + + it('ignores the names of another project', async () => { + const other = await createTeamProject('Other team'); + await create(owner, { + scope: 'project', + projectId: other.id, + skill: { ...body, name: 'New skill' }, + }); + + const free = await newName(editor, { + scope: 'project', + projectId: project.id, + name: 'New skill', + }); + + expect(free).toEqual({ name: 'New skill' }); + }); + + it('needs the create permission in that scope', async () => { + await newName(viewer, { scope: 'project', projectId: project.id, name: 'New skill' }, 403); + await newName(member, { scope: 'instance', name: 'New skill' }, 403); + }); + + it('refuses a user outside the project, for a project and for an agent', async () => { + const agent = await newAgent('Support bot', project.id); + + await newName(outsider, { scope: 'project', projectId: project.id, name: 'New skill' }, 403); + await newName(outsider, { scope: 'agent', agentId: agent.id, name: 'New skill' }, 403); + }); + }); + + describe('change scope', () => { + it('moves an unused skill and saves its content in the same request', async () => { + const { id, skillHash } = await create(editor, { + scope: 'project', + projectId: project.id, + skill: body, + }); + + const saved = await owner + .patch(`/skills/${id}`) + .send({ instructions: 'Be very brief.', scope: 'instance', baseSkillHash: skillHash }) + .expect(200); + const read = await owner.get(`/skills/${id}`).expect(200); + + expect(saved.body.data).toMatchObject({ version: 2, created: true }); + expect(read.body.data).toMatchObject({ scope: 'instance', projectId: null }); + }); + + it('moves a skill without a content change', async () => { + const other = await createTeamProject('Other team'); + const { id, skillHash } = await create(owner, { + scope: 'project', + projectId: project.id, + skill: body, + }); + + const saved = await owner + .patch(`/skills/${id}`) + .send({ scope: 'project', projectId: other.id, baseSkillHash: skillHash }) + .expect(200); + const read = await owner.get(`/skills/${id}`).expect(200); + + expect(saved.body.data).toMatchObject({ version: 1, created: false }); + expect(read.body.data).toMatchObject({ scope: 'project', projectId: other.id }); + }); + + it('needs the create permission where the skill goes', async () => { + const { id, skillHash } = await create(editor, { + scope: 'project', + projectId: project.id, + skill: body, + }); + + await editor + .patch(`/skills/${id}`) + .send({ scope: 'instance', baseSkillHash: skillHash }) + .expect(403); + const read = await editor.get(`/skills/${id}`).expect(200); + + expect(read.body.data).toMatchObject({ scope: 'project', projectId: project.id }); + }); + + it('needs a role in the team project where the skill goes', async () => { + const other = await createTeamProject('Other team'); + const { id, skillHash } = await create(editor, { + scope: 'project', + projectId: project.id, + skill: body, + }); + + await editor + .patch(`/skills/${id}`) + .send({ scope: 'project', projectId: other.id, baseSkillHash: skillHash }) + .expect(403); + const read = await editor.get(`/skills/${id}`).expect(200); + + expect(read.body.data).toMatchObject({ scope: 'project', projectId: project.id }); + }); + + it('needs the delete permission where the skill comes from', async () => { + // A custom role that may create and edit skills everywhere but delete none. + const role = await createCustomRoleWithScopeSlugs( + [ + 'skill:create', + 'skill:read', + 'skill:update', + 'projectSkill:create', + 'projectSkill:read', + 'projectSkill:update', + ], + { roleType: 'global' }, + ); + const noDelete = server.authAgentFor(await createUser({ role })); + const { id, skillHash } = await create(owner, { scope: 'instance', skill: body }); + + await noDelete + .patch(`/skills/${id}`) + .send({ scope: 'project', projectId: project.id, baseSkillHash: skillHash }) + .expect(403); + await noDelete + .patch(`/skills/${id}`) + .send({ instructions: 'Be very brief.', baseSkillHash: skillHash }) + .expect(200); + }); + + it('answers 409 while an agent uses the skill and saves nothing', async () => { + const { id, skillHash } = await create(editor, { + scope: 'project', + projectId: project.id, + skill: body, + }); + await attachToNewAgent(id, 'Support bot', project.id); + + await owner + .patch(`/skills/${id}`) + .send({ instructions: 'Be very brief.', scope: 'instance', baseSkillHash: skillHash }) + .expect(409); + const read = await owner.get(`/skills/${id}`).expect(200); + + expect(read.body.data).toMatchObject({ scope: 'project', latestVersion: 1 }); + }); + + it('needs a project id to move a skill to a project', async () => { + const { id, skillHash } = await create(owner, { scope: 'instance', skill: body }); + + await owner + .patch(`/skills/${id}`) + .send({ scope: 'project', baseSkillHash: skillHash }) + .expect(400); + }); + + it('does not move a skill to one agent', async () => { + const { id, skillHash } = await create(owner, { scope: 'instance', skill: body }); + + await owner + .patch(`/skills/${id}`) + .send({ scope: 'agent', baseSkillHash: skillHash }) + .expect(400); + }); + }); + describe('delete', () => { it('deletes an unused skill', async () => { const { id } = await create(editor, { scope: 'project', projectId: project.id, skill: body }); diff --git a/packages/frontend/@n8n/frontend-constants/src/views.ts b/packages/frontend/@n8n/frontend-constants/src/views.ts index d9547b6a2e5..edd5cfb02a7 100644 --- a/packages/frontend/@n8n/frontend-constants/src/views.ts +++ b/packages/frontend/@n8n/frontend-constants/src/views.ts @@ -98,5 +98,6 @@ export enum VIEWS { RESOLVERS = 'Resolvers', SETTINGS_CONTEXT = 'SettingsContext', SETTINGS_CONTEXT_PREFERENCES = 'SettingsContextPreferences', + SETTINGS_CONTEXT_SKILLS = 'SettingsContextSkills', RESOURCE_CENTER = 'ResourceCenter', } diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index d473e241f6a..b530db9e741 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -3434,6 +3434,53 @@ "settings.context.description": "A centralized place to manage reusable information and resources. Applied across n8n assistant, workflows, agents, and connected AI tools", "settings.context.comingSoon": "Coming soon", "settings.context.skills.title": "Skills", + "settings.context.skills.subtitle": "Teach the assistant and agents how to work", + "settings.context.skills.description": "Give the n8n assistant and your agents a shared way to handle specific tasks.", + "settings.context.skills.back": "Back to Context", + "settings.context.skills.count": "{count} skill | {count} skills", + "settings.context.skills.search.placeholder": "Search skills by name...", + "settings.context.skills.filter.label": "Filter by scope", + "settings.context.skills.filter.all": "All scopes", + "settings.context.skills.filter.instance": "Global", + "settings.context.skills.filter.project": "Projects", + "settings.context.skills.filter.agent": "Agents", + "settings.context.skills.columns.skill": "Skill", + "settings.context.skills.columns.scope": "Scope", + "settings.context.skills.columns.agents": "Agents", + "settings.context.skills.columns.createdBy": "Created by", + "settings.context.skills.columns.updatedAt": "Last updated", + "settings.context.skills.scope.instance": "Global", + "settings.context.skills.scope.personalProject": "Personal project", + "settings.context.skills.scope.project": "Project", + "settings.context.skills.scope.agent": "{name} only", + "settings.context.skills.scope.agentFallback": "One agent only", + "settings.context.skills.agents.label": "Show the agents that use this skill", + "settings.context.skills.agents.hidden": "{count} agent in other projects | {count} agents in other projects", + "settings.context.skills.actions.create": "Create skill", + "settings.context.skills.actions.edit": "Edit", + "settings.context.skills.actions.view": "View", + "settings.context.skills.actions.delete": "Delete", + "settings.context.skills.readOnly": "You can use but not change this skill", + "settings.context.skills.empty.title": "No skills yet", + "settings.context.skills.empty.description": "Add a skill by hand or upload a SKILL.md file. Agents in the scope of the skill can then use it.", + "settings.context.skills.modal.scope.label": "Where can this skill be added?", + "settings.context.skills.modal.scope.tooltip": "A global skill can be added to any agent. A project skill can be added to the agents of that project. An agent skill belongs to one agent.", + "settings.context.skills.modal.scope.inUse": "Used by {count} agent. Remove the skill from it to change where it is available. | Used by {count} agents. Remove the skill from them to change where it is available.", + "settings.context.skills.delete.inUse": "Used by {count} agent. Remove the skill from the agent first. | Used by {count} agents. Remove the skill from each agent first.", + "settings.context.skills.delete.confirm.title": "Delete skill?", + "settings.context.skills.delete.confirm.message": "You cannot undo this.", + "settings.context.skills.delete.confirm.button": "Delete", + "settings.context.skills.delete.cancel": "Cancel", + "settings.context.skills.delete.success": "Skill deleted", + "settings.context.skills.save.noChanges": "Nothing to save", + "settings.context.skills.create.hidden": "Skill created. Your search or filter hides it.", + "settings.context.skills.save.hidden": "Skill saved. Your search or filter hides it.", + "settings.context.skills.error.load": "Could not load skills", + "settings.context.skills.error.loadOne": "Could not load the skill", + "settings.context.skills.error.agents": "Could not load the agents", + "settings.context.skills.error.save": "Could not save the skill", + "settings.context.skills.error.nameTaken": "That name is taken, choose another", + "settings.context.skills.error.delete": "Could not delete the skill", "settings.context.sources.title": "Sources", "settings.context.preferences.title": "Preferences", "settings.context.preferences.subtitle": "Make AI work more like you do", diff --git a/packages/frontend/editor-ui/src/app/composables/useSettingsItems.test.ts b/packages/frontend/editor-ui/src/app/composables/useSettingsItems.test.ts index 7006b48bd8d..36566fe5137 100644 --- a/packages/frontend/editor-ui/src/app/composables/useSettingsItems.test.ts +++ b/packages/frontend/editor-ui/src/app/composables/useSettingsItems.test.ts @@ -17,7 +17,7 @@ const settingsSidebarItems = ref([]); const promotionsFlag = ref('false'); const canUserAccessRouteByName = vi.hoisted(() => vi.fn<(name: string) => boolean>(() => true)); -const contextPreferencesEnabled = vi.hoisted(() => ({ value: true })); +const contextEnabled = vi.hoisted(() => ({ value: true })); const openTopUpMock = vi.hoisted(() => vi.fn()); vi.mock('vue-router', () => ({ useRouter: vi.fn(() => ({})) })); @@ -25,7 +25,7 @@ vi.mock('./useUserHelpers', () => ({ useUserHelpers: vi.fn(() => ({ canUserAccessRouteByName })), })); vi.mock('@/features/settings/context/context.utils', () => ({ - isContextPreferencesEnabled: () => contextPreferencesEnabled.value, + isContextEnabled: () => contextEnabled.value, })); vi.mock('./useAiGateway', () => ({ useAiGateway: vi.fn(() => ({ balance: computed(() => balance.value) })), @@ -83,7 +83,7 @@ describe('useSettingsItems', () => { activeModules.value = []; promotionsFlag.value = 'false'; canUserAccessRouteByName.mockReturnValue(true); - contextPreferencesEnabled.value = true; + contextEnabled.value = true; }); describe('sidebar order', () => { @@ -186,7 +186,7 @@ describe('useSettingsItems', () => { }); it('is hidden when the flag is off, because the route guard does not run here', () => { - contextPreferencesEnabled.value = false; + contextEnabled.value = false; expect(idsOf()).not.toContain('settings-context'); }); diff --git a/packages/frontend/editor-ui/src/app/composables/useSettingsItems.ts b/packages/frontend/editor-ui/src/app/composables/useSettingsItems.ts index 5f5e3dd74a6..71d7e51a42f 100644 --- a/packages/frontend/editor-ui/src/app/composables/useSettingsItems.ts +++ b/packages/frontend/editor-ui/src/app/composables/useSettingsItems.ts @@ -6,7 +6,7 @@ import { computed } from 'vue'; import type { IMenuSettingItem } from '@n8n/frontend-module-sdk'; import { useI18n } from '@n8n/i18n'; import { VIEWS } from '../constants'; -import { isContextPreferencesEnabled } from '@/features/settings/context/context.utils'; +import { isContextEnabled } from '@/features/settings/context/context.utils'; import { useUIStore } from '../stores/ui.store'; import { useSettingsStore } from '@n8n/stores/settings.store'; import { hasPermission } from '../utils/rbac/permissions'; @@ -218,8 +218,7 @@ export function useSettingsItems() { icon: 'brain', label: i18n.baseText('settings.context.title'), position: 'top', - available: - isContextPreferencesEnabled() && canUserAccessRouteByName(VIEWS.SETTINGS_CONTEXT), + available: isContextEnabled() && canUserAccessRouteByName(VIEWS.SETTINGS_CONTEXT), route: { to: { name: VIEWS.SETTINGS_CONTEXT } }, preview: true, }, diff --git a/packages/frontend/editor-ui/src/features/agents/__tests__/AgentSkillModal.test.ts b/packages/frontend/editor-ui/src/features/agents/__tests__/AgentSkillModal.test.ts index 0b4ffa2f0f7..9772553e595 100644 --- a/packages/frontend/editor-ui/src/features/agents/__tests__/AgentSkillModal.test.ts +++ b/packages/frontend/editor-ui/src/features/agents/__tests__/AgentSkillModal.test.ts @@ -5,6 +5,7 @@ import { createTestingPinia } from '@pinia/testing'; import { mockedStore } from '@/__tests__/utils'; import { useUIStore } from '@/app/stores/ui.store'; import { configure, fireEvent, waitFor } from '@testing-library/vue'; +import userEvent from '@testing-library/user-event'; import { AGENT_SKILL_INSTRUCTIONS_MAX_LENGTH, AGENT_SKILL_REFERENCE_MAX_COUNT, @@ -85,12 +86,18 @@ function renderModal({ skill, skillId, existingSkillNames, + settings, ...hubOptions }: { onConfirm?: AgentSkillModalData['onConfirm']; skill?: AgentSkill; skillId?: string; existingSkillNames?: string[]; + /** Opens the modal as Settings > Context > Skills does: no agent, plus these fields. */ + settings?: Pick< + AgentSkillModalData, + 'readOnly' | 'scopeOptions' | 'initialScope' | 'scopeLockedReason' | 'getDefaultName' + >; } & Pick< AgentSkillModalData, | 'hubSkillsEnabled' @@ -126,8 +133,7 @@ function renderModal({ props: { modalName: MODAL_NAME, data: { - projectId: 'p1', - agentId: 'a1', + ...(settings ?? { projectId: 'p1', agentId: 'a1' }), skill, skillId, existingSkillNames, @@ -138,6 +144,12 @@ function renderModal({ }); } +const SAVED_SKILL: AgentSkill = { + name: 'brand-voice', + description: 'Write clear customer-facing copy.', + instructions: 'Keep it short.', +}; + describe('AgentSkillModal', () => { let uiStore: ReturnType>; @@ -432,6 +444,90 @@ describe('AgentSkillModal', () => { expect(uiStore.closeModal).toHaveBeenCalledWith(MODAL_NAME); }); + it('focuses the name when a new skill is started by hand', async () => { + const { getByTestId } = renderModal(); + + await fireEvent.click(getByTestId('agent-skill-add-manually')); + + await waitFor(() => expect(getByTestId('agent-modal-title-input')).toHaveFocus()); + }); + + it('stays open with the edits and the name error when the save is refused', async () => { + const onConfirm = vi.fn().mockResolvedValue({ saved: false, nameError: 'Name taken' }); + const { getByTestId, queryByTestId } = renderModal({ + onConfirm, + skill: SAVED_SKILL, + skillId: 's1', + }); + await fireEvent.update(getByTestId('agent-modal-title-input'), 'Summarize'); + + await fireEvent.click(getByTestId('agent-skill-create-save')); + + await waitFor(() => + expect(getByTestId('agent-modal-title-error')).toHaveTextContent('Name taken'), + ); + expect(uiStore.closeModal).not.toHaveBeenCalled(); + expect(getByTestId('agent-modal-title-input')).toHaveValue('Summarize'); + expect(getByTestId('agent-modal-title-input')).toHaveFocus(); + + await fireEvent.update(getByTestId('agent-modal-title-input'), 'Condense'); + expect(queryByTestId('agent-modal-title-error')).not.toBeInTheDocument(); + }); + + it('locks the editor while the save is pending', async () => { + let finish: (value?: undefined) => void = () => {}; + const onConfirm = vi.fn( + async () => await new Promise((resolve) => (finish = resolve)), + ); + const { getByTestId, queryByTestId } = renderModal({ + onConfirm, + skill: SAVED_SKILL, + skillId: 's1', + }); + await fireEvent.update(getByTestId('agent-modal-title-input'), 'Summarize'); + + await fireEvent.click(getByTestId('agent-skill-create-save')); + + await waitFor(() => expect(getByTestId('agent-modal-title-input')).toBeDisabled()); + expect(queryByTestId('agent-skill-add-reference')).not.toBeInTheDocument(); + finish(); + await waitFor(() => expect(uiStore.closeModal).toHaveBeenCalledWith(MODAL_NAME)); + }); + + it('clears a refused name when the user goes back and starts again', async () => { + const onConfirm = vi.fn().mockResolvedValue({ saved: false, nameError: 'Name taken' }); + const { getByTestId, queryByTestId } = renderModal({ onConfirm }); + await fireEvent.click(getByTestId('agent-skill-add-manually')); + await fireEvent.update( + getByTestId('agent-skill-description-input').querySelector('input')!, + 'Use for research', + ); + await fireEvent.update(getByTestId('agent-skill-instructions-editor'), 'Read the guide'); + await fireEvent.click(getByTestId('agent-skill-create-save')); + await waitFor(() => expect(getByTestId('agent-modal-title-error')).toBeInTheDocument()); + + await fireEvent.click(getByTestId('agent-modal-back')); + await fireEvent.click(getByTestId('agent-skill-add-manually')); + + expect(queryByTestId('agent-modal-title-error')).not.toBeInTheDocument(); + }); + + it('stays open when the save fails, and closes once it succeeds', async () => { + const onConfirm = vi + .fn() + .mockRejectedValueOnce(new Error('Network down')) + .mockResolvedValueOnce(undefined); + const { getByTestId } = renderModal({ onConfirm, skill: SAVED_SKILL, skillId: 's1' }); + await fireEvent.update(getByTestId('agent-modal-title-input'), 'Summarize'); + + await fireEvent.click(getByTestId('agent-skill-create-save')); + await waitFor(() => expect(onConfirm).toHaveBeenCalledTimes(1)); + expect(uiStore.closeModal).not.toHaveBeenCalled(); + + await fireEvent.click(getByTestId('agent-skill-create-save')); + await waitFor(() => expect(uiStore.closeModal).toHaveBeenCalledWith(MODAL_NAME)); + }); + it('creates a skill on the agent without the picker or the skills API when the skills hub is off', async () => { const onConfirm = vi.fn(); const { container, getByTestId, queryByTestId } = renderModal({ @@ -990,6 +1086,169 @@ describe('AgentSkillModal', () => { }); }); +describe('AgentSkillModal from Settings', () => { + beforeEach(() => { + vi.clearAllMocks(); + createTestingPinia({ stubActions: false }); + const uiStore = mockedStore(useUIStore); + uiStore.openModal(MODAL_NAME); + uiStore.closeModal = vi.fn(); + }); + + it('shows a read-only skill without edit controls', () => { + const { getByTestId, queryByTestId } = renderModal({ + skillId: 's1', + skill: SAVED_SKILL, + settings: { readOnly: true }, + }); + + expect(getByTestId('agent-skill-read-only-callout')).toHaveTextContent( + 'settings.context.skills.readOnly', + ); + expect(queryByTestId('agent-modal-title-input')).not.toBeInTheDocument(); + expect(queryByTestId('agent-skill-create-save')).not.toBeInTheDocument(); + expect(queryByTestId('agent-skill-add-reference')).not.toBeInTheDocument(); + }); + + it('returns the chosen scope with a new skill', async () => { + const onConfirm = vi.fn(); + const { getByTestId } = renderModal({ + onConfirm, + settings: { + scopeOptions: [ + { value: 'project:personal-1', label: 'Personal project', icon: 'user' }, + { value: 'project:team-1', label: 'Marketing', icon: 'layers' }, + ], + initialScope: 'project:team-1', + }, + }); + await fireEvent.change(getByTestId('agent-skill-skill-md-file-input'), { + target: { + files: [new File(['---\nname: Research\ndescription: Use it\n---\nBody'], 'SKILL.md')], + }, + }); + await waitFor(() => expect(getByTestId('agent-skill-viewer')).toBeInTheDocument()); + + // Pick the option that is not the initial one, so the test fails if the choice is lost. + await userEvent.click(getByTestId('agent-skill-scope-select')); + const option = await waitFor(() => { + const match = [...document.querySelectorAll('.el-select-dropdown__item')].find((item) => + item.textContent?.includes('Personal project'), + ); + if (!match) throw new Error('Expected the Personal project option'); + return match; + }); + await userEvent.click(option); + await fireEvent.click(getByTestId('agent-skill-create-save')); + + expect(onConfirm).toHaveBeenCalledWith({ + id: undefined, + skill: { name: 'Research', description: 'Use it', instructions: 'Body' }, + scope: 'project:personal-1', + }); + // Settings has no agent, so the builder's import event is not sent. + expect(trackImportedSkill).not.toHaveBeenCalled(); + }); + + it('shows the scope of a saved skill without letting the user change it', () => { + const { getByTestId } = renderModal({ + skillId: 's1', + skill: SAVED_SKILL, + settings: { + scopeOptions: [{ value: 'agent', label: 'FirstClassDarwin only', icon: 'bot' }], + initialScope: 'agent', + scopeLockedReason: 'You cannot change it.', + }, + }); + + expect(getByTestId('agent-skill-scope')).toBeInTheDocument(); + expect(getByTestId('agent-skill-scope-select').querySelector('input')).toBeDisabled(); + }); + + it('asks the server for a free default name when a skill is started by hand', async () => { + const getDefaultName = vi.fn().mockResolvedValue('New skill 3'); + const { getByTestId } = renderModal({ + settings: { + scopeOptions: [{ value: 'instance', label: 'Global', icon: 'globe' }], + initialScope: 'instance', + getDefaultName, + }, + }); + + await fireEvent.click(getByTestId('agent-skill-add-manually')); + + expect(getDefaultName).toHaveBeenCalledWith('instance'); + await waitFor(() => expect(getTitleInput(document.body)).toHaveValue('New skill 3')); + }); + + it('blocks Save until the default name lookup answers', async () => { + let answer: (name: string) => void = () => {}; + const { getByTestId } = renderModal({ + settings: { + scopeOptions: [{ value: 'instance', label: 'Global', icon: 'globe' }], + getDefaultName: vi.fn( + async () => await new Promise((resolve) => (answer = resolve)), + ), + }, + }); + + await fireEvent.click(getByTestId('agent-skill-add-manually')); + + expect(getByTestId('agent-skill-create-save')).toBeDisabled(); + answer('New skill 2'); + await waitFor(() => expect(getByTestId('agent-skill-create-save')).not.toBeDisabled()); + }); + + it('ignores an answer that a newer lookup replaced', async () => { + const answers: Array<(name: string) => void> = []; + const { getByTestId } = renderModal({ + settings: { + scopeOptions: [{ value: 'instance', label: 'Global', icon: 'globe' }], + getDefaultName: vi.fn( + async () => await new Promise((resolve) => answers.push(resolve)), + ), + }, + }); + + await fireEvent.click(getByTestId('agent-skill-add-manually')); + await fireEvent.click(getByTestId('agent-modal-back')); + await fireEvent.click(getByTestId('agent-skill-add-manually')); + answers[1]('New skill 3'); + await waitFor(() => expect(getTitleInput(document.body)).toHaveValue('New skill 3')); + answers[0]('New skill 2'); + await new Promise(process.nextTick); + + expect(getTitleInput(document.body)).toHaveValue('New skill 3'); + }); + + it('keeps the local default name when the server does not answer', async () => { + const { getByTestId } = renderModal({ + settings: { + scopeOptions: [{ value: 'instance', label: 'Global', icon: 'globe' }], + getDefaultName: vi.fn().mockRejectedValue(new Error('offline')), + }, + }); + + await fireEvent.click(getByTestId('agent-skill-add-manually')); + await new Promise(process.nextTick); + + expect(getTitleInput(document.body)).toHaveValue('New skill'); + }); + + it('does not ask for a default name when an existing skill is edited', () => { + const getDefaultName = vi.fn(); + renderModal({ skillId: 's1', skill: SAVED_SKILL, settings: { getDefaultName } }); + + expect(getDefaultName).not.toHaveBeenCalled(); + }); + + it('keeps the scope field out of the agent builder', () => { + const { queryByTestId } = renderModal({ skillId: 's1', skill: SAVED_SKILL }); + + expect(queryByTestId('agent-skill-scope')).not.toBeInTheDocument(); + }); +}); + function makeListItem( overrides: Pick & Partial, ): SkillListItem { @@ -997,9 +1256,11 @@ function makeListItem( description: `${overrides.name} description`, projectId: overrides.scope === 'project' ? 'p1' : null, projectName: overrides.scope === 'project' ? 'Marketing' : null, + projectType: overrides.scope === 'project' ? 'team' : null, agentId: null, agentName: null, source: 'ui', + createdBy: null, latestVersion: 1, usedByAgents: 0, canEdit: true, diff --git a/packages/frontend/editor-ui/src/features/agents/__tests__/useAgentCapabilitiesActions.test.ts b/packages/frontend/editor-ui/src/features/agents/__tests__/useAgentCapabilitiesActions.test.ts index 60f3f3d297e..57b0bac1ae8 100644 --- a/packages/frontend/editor-ui/src/features/agents/__tests__/useAgentCapabilitiesActions.test.ts +++ b/packages/frontend/editor-ui/src/features/agents/__tests__/useAgentCapabilitiesActions.test.ts @@ -632,9 +632,11 @@ describe('useAgentCapabilitiesActions — skills hub', () => { scope: 'instance', projectId: null, projectName: null, + projectType: null, agentId: null, agentName: null, source: 'ui', + createdBy: null, latestVersion: 1, usedByAgents: 0, canEdit: false, diff --git a/packages/frontend/editor-ui/src/features/agents/__tests__/utils/AgentModalTestStub.ts b/packages/frontend/editor-ui/src/features/agents/__tests__/utils/AgentModalTestStub.ts index 75d9223da2a..d5e2817f09e 100644 --- a/packages/frontend/editor-ui/src/features/agents/__tests__/utils/AgentModalTestStub.ts +++ b/packages/frontend/editor-ui/src/features/agents/__tests__/utils/AgentModalTestStub.ts @@ -19,6 +19,14 @@ export const AgentModalTestStub = defineComponent({ disableOutsidePointerEvents: { type: Boolean, default: true }, }, emits: ['update:open', 'update:title', 'back', 'interactOutside'], + methods: { + /** Mirrors `AgentModal.focusTitle`. */ + focusTitle() { + (this.$el as HTMLElement | null) + ?.querySelector('[data-testid="agent-modal-title-input"]') + ?.focus(); + }, + }, template: `
import cloneDeep from 'lodash/cloneDeep'; import isEqual from 'lodash/isEqual'; -import { computed, onMounted, ref } from 'vue'; +import { computed, nextTick, onMounted, ref, useTemplateRef, watch } from 'vue'; import { AGENT_SKILL_INSTRUCTIONS_MAX_LENGTH, AGENT_SKILL_REFERENCE_MAX_COUNT, @@ -21,6 +21,7 @@ import { N8nText, N8nTooltip, } from '@n8n/design-system'; +import type { IconName } from '@n8n/design-system'; import { useI18n, type BaseTextKey } from '@n8n/i18n'; import { useRootStore } from '@n8n/stores/useRootStore'; @@ -42,9 +43,18 @@ import AgentModalMultiStep from './modals/AgentModalMultiStep.vue'; const SKILL_FILE = 'SKILL.md'; +export type AgentSkillModalScopeOption = { value: string; label: string; icon: IconName }; + +/** What a failed `onConfirm` returns. The modal stays open and shows `nameError` on the name. */ +export type AgentSkillConfirmFailure = { saved: false; nameError?: string }; + +/** `undefined` closes the modal. A callback without a `return` gives it. */ +export type AgentSkillConfirmResult = AgentSkillConfirmFailure | undefined; + export type AgentSkillModalData = { - projectId: string; - agentId: string; + /** Set when opened from an agent. Absent when opened from Settings > Context > Skills. */ + projectId?: string; + agentId?: string; skill?: AgentSkill; skillId?: string; /** @@ -56,11 +66,31 @@ export type AgentSkillModalData = { getAttachedSkills?: () => Array<{ id: string; name: string }>; projectLabel?: string; canCreateInstanceSkills?: boolean; + /** + * Settings only: where the skill can be added. On create the user picks one and the + * value comes back in `onConfirm`. The agent builder uses `availableTo` instead. + */ + scopeOptions?: AgentSkillModalScopeOption[]; + initialScope?: string; + /** + * Settings only: a default name that no skill in the scope has yet, as new workflows + * get one. Called with the chosen scope value when the user starts a skill by hand. + */ + getDefaultName?: (scope: string) => Promise; + /** Locks the scope field and explains why in a tooltip. */ + scopeLockedReason?: string; + /** Shows the skill without edit controls, for a user who can use but not change it. */ + readOnly?: boolean; + /** + * Called on Save. A promise keeps the modal open until it settles. It closes unless the + * result is an `AgentSkillConfirmFailure`, so a refused save keeps what the user typed. + */ onConfirm: (payload: { id?: string; skill: AgentSkill; availableTo?: AgentSkillAvailability; - }) => void; + scope?: string; + }) => AgentSkillConfirmResult | Promise; outdated?: boolean; onUseLatestVersion?: (id: string) => void; onAttach?: (skill: SkillListItem) => Promise; @@ -78,6 +108,7 @@ const rootStore = useRootStore(); const { showError } = useToast(); const agentTelemetry = useAgentTelemetry(); const modalOpen = computed(() => uiStore.modalsById[props.modalName]?.open === true); +const agentModal = useTemplateRef>('agentModal'); const existingNameKeys = computed(() => { const names = props.data.getAttachedSkills @@ -111,6 +142,55 @@ const step = ref<'select' | 'preview' | 'upload' | 'manual' | 'edit'>( props.data.skill || props.data.skillId ? 'edit' : hasPicker ? 'select' : 'upload', ); const isImporting = ref(false); +const isSaving = ref(false); +/** A name error from the server, such as a taken name. Typing a new name clears it. */ +const serverNameError = ref(''); +const scope = ref(props.data.initialScope ?? props.data.scopeOptions?.[0]?.value ?? ''); + +/** The last default name the server gave. A name the user typed is never replaced. */ +const serverDefaultName = ref(null); +/** Save waits for the lookup, so a taken local default cannot fail after the modal closes. */ +const defaultNameLoading = ref(false); +let latestNameRequest = 0; + +async function refreshDefaultName() { + if (!props.data.getDefaultName || props.data.skillId) return; + const current = skill.value.name; + if (current !== getDefaultSkillName() && current !== serverDefaultName.value) return; + const request = ++latestNameRequest; + const requestedScope = scope.value; + defaultNameLoading.value = true; + try { + const name = await props.data.getDefaultName(requestedScope); + // Only the newest answer for the scope still selected counts, and only while the + // user has not typed a name of their own. + if (request !== latestNameRequest || scope.value !== requestedScope) return; + if (skill.value.name !== current) return; + serverDefaultName.value = name; + skill.value = { ...skill.value, name }; + } catch { + // Keep the local default. The save still refuses a taken name. + } finally { + if (request === latestNameRequest) defaultNameLoading.value = false; + } +} + +function onAddManually() { + step.value = 'manual'; + void refreshDefaultName(); +} + +// A new scope can hold other names, so a default name follows the scope. +watch(scope, () => { + if (step.value === 'manual') void refreshDefaultName(); +}); +const showScope = computed(() => !!props.data.scopeOptions?.length && step.value !== 'preview'); +const canChangeScope = computed( + () => showScope.value && !props.data.scopeLockedReason && !props.data.readOnly, +); +const selectedScopeIcon = computed( + () => props.data.scopeOptions?.find((option) => option.value === scope.value)?.icon, +); const availableTo = ref('project'); const librarySkills = ref([]); @@ -168,12 +248,17 @@ const skillAccessFailed = ref(false); const isOutdated = computed(() => isEditing.value && !!props.data.outdated); const isReadOnly = computed( () => + props.data.readOnly === true || skillAccessLoading.value || skillAccessFailed.value || skillAccess.value?.canEdit === false || isOutdated.value, ); -const cannotEdit = computed(() => skillAccess.value?.canEdit === false || isOutdated.value); +const cannotEdit = computed( + () => props.data.readOnly === true || skillAccess.value?.canEdit === false || isOutdated.value, +); +/** Save sends a snapshot, so the editor is locked until the answer comes back. */ +const editorLocked = computed(() => isReadOnly.value || isSaving.value); const savedTarget = computed(() => { if (!skillAccess.value) return undefined; if (skillAccess.value.agentId) return 'agent'; @@ -279,8 +364,8 @@ const validationErrors = computed>>(() const visibleErrors = computed(() => submitted.value || openedWithMissingContent.value ? validationErrors.value : {}, ); -const visibleNameError = computed(() => - submitted.value ? (validationErrors.value.name ?? '') : '', +const visibleNameError = computed( + () => (submitted.value ? validationErrors.value.name : undefined) ?? serverNameError.value, ); const canSave = computed( () => formIsValid.value && !validationErrors.value.name && !isReadOnly.value, @@ -289,7 +374,8 @@ const hasChanges = computed( () => !isEditing.value || !isEqual(skill.value, initialSkill) || - (showAvailabilitySelect.value && availableTo.value !== savedTarget.value), + (showAvailabilitySelect.value && availableTo.value !== savedTarget.value) || + (canChangeScope.value && scope.value !== props.data.initialScope), ); function getSimilarNameReason(name: string): string | undefined { @@ -309,6 +395,8 @@ function getPickerCategory(librarySkill: SkillListItem): 'project' | 'this-agent } async function loadLibrarySkills() { + // The picker lists what one agent may attach. Settings opens the modal without an agent. + if (!props.data.agentId) return; libraryLoading.value = true; try { librarySkills.value = await listAttachableSkills(rootStore.restApiContext, props.data.agentId); @@ -394,6 +482,7 @@ onMounted(() => { function onSkillUploaded(importedSkill: AgentSkill) { skill.value = normalizeSkill(importedSkill); + serverNameError.value = ''; selectedPath.value = SKILL_FILE; submitted.value = false; step.value = 'edit'; @@ -405,6 +494,7 @@ function onBack() { return; } skill.value = { name: getDefaultSkillName(), description: '', instructions: '' }; + serverNameError.value = ''; selectedPath.value = SKILL_FILE; submitted.value = false; formIsValid.value = false; @@ -413,6 +503,7 @@ function onBack() { function onSkillUpdate(updates: Partial) { if (isReadOnly.value) return; + if (updates.name !== undefined && updates.name !== skill.value.name) serverNameError.value = ''; skill.value = normalizeSkill({ ...skill.value, ...updates }); if ( selectedPath.value !== SKILL_FILE && @@ -468,6 +559,8 @@ function onImportSkill(payload: { referenceCount?: number; error?: string; }) { + // The import event belongs to the agent builder. Settings has no agent to name. + if (!props.data.agentId) return; agentTelemetry.trackImportedSkill({ agentId: props.data.agentId, ...payload, @@ -478,7 +571,8 @@ function closeModal() { uiStore.closeModal(props.modalName); } -function onSave() { +async function onSave() { + if (defaultNameLoading.value || isSaving.value) return; submitted.value = true; if (!canSave.value) return; @@ -490,14 +584,43 @@ function onSave() { }); const targetChanged = !isEditing.value || availableTo.value !== savedTarget.value; - props.data.onConfirm({ - id: props.data.skillId, - skill: payload, - ...(showAvailabilitySelect.value && targetChanged ? { availableTo: availableTo.value } : {}), - }); + isSaving.value = true; + let result: AgentSkillConfirmResult; + try { + result = await props.data.onConfirm({ + id: props.data.skillId, + skill: payload, + ...(showAvailabilitySelect.value && targetChanged ? { availableTo: availableTo.value } : {}), + ...(canChangeScope.value ? { scope: scope.value } : {}), + }); + } catch { + // The caller reports its own errors. Keep the modal open with the user's edits. + return; + } finally { + isSaving.value = false; + } + if (result?.saved === false) { + if (result.nameError) { + serverNameError.value = result.nameError; + await focusName(); + } + return; + } closeModal(); } +async function focusName() { + await nextTick(); + agentModal.value?.focusTitle(); +} + +// A new skill gets its name first, so the name takes the focus when it can be edited. +watch(step, async (next) => { + if ((next === 'manual' || next === 'edit') && !isEditing.value && !isReadOnly.value) { + await focusName(); + } +}); + function onUseLatestVersion() { if (!props.data.skillId) return; const skillId = props.data.skillId; @@ -519,6 +642,7 @@ function onRemove() {