From 04f4afe3dcbf2583b2be9183b72631a609168e27 Mon Sep 17 00:00:00 2001 From: Roman Makowiak Date: Tue, 13 Oct 2026 20:28:55 +0000 Subject: [PATCH] feat(core): Add skills repository, service and REST API (no-changelog) (#40653) Co-authored-by: Ricardo Espinoza Co-authored-by: Claude Opus 5.5 --- .../agents/__tests__/skills.schema.test.ts | 74 ++ packages/@n8n/api-types/src/agents/index.ts | 1 + .../api-types/src/agents/skills.schema.ts | 122 +++ .../scope-information.test.ts.snap | 10 + packages/@n8n/permissions/src/constants.ee.ts | 4 + .../src/roles/__tests__/skill-scope.test.ts | 100 +++ .../src/roles/custom-role-scopes.ee.ts | 6 + .../src/roles/scopes/global-scopes.ee.ts | 10 + .../src/roles/scopes/project-scopes.ee.ts | 17 + .../@n8n/permissions/src/scope-information.ts | 16 + .../get-resource-permissions.test.ts | 4 + .../__tests__/skill-content-hash.test.ts | 72 ++ .../agents/__tests__/skill-names.test.ts | 67 ++ .../agents/__tests__/skill.service.test.ts | 841 ++++++++++++++++++ .../__tests__/skills-api.service.test.ts | 484 ++++++++++ .../__tests__/skills.controller.test.ts | 28 + .../cli/src/modules/agents/agents.module.ts | 1 + .../agents/repositories/agent.repository.ts | 39 + .../agents/repositories/skill.repository.ts | 666 ++++++++++++++ .../src/modules/agents/skills.controller.ts | 47 + .../agents/skills/skill-content-hash.ts | 55 ++ .../src/modules/agents/skills/skill-names.ts | 47 + .../modules/agents/skills/skill.service.ts | 502 +++++++++++ .../agents/skills/skills-api.service.ts | 221 +++++ .../agents/skill.repository.test.ts | 734 +++++++++++++++ .../agents/skills.controller.test.ts | 400 +++++++++ .../frontend/@n8n/i18n/src/locales/en.json | 9 + .../frontend/@n8n/stores/src/rbac.store.ts | 2 + .../roles/project/projectRoleScopes.ts | 1 + 29 files changed, 4580 insertions(+) create mode 100644 packages/@n8n/api-types/src/agents/__tests__/skills.schema.test.ts create mode 100644 packages/@n8n/api-types/src/agents/skills.schema.ts create mode 100644 packages/@n8n/permissions/src/roles/__tests__/skill-scope.test.ts create mode 100644 packages/cli/src/modules/agents/__tests__/skill-content-hash.test.ts create mode 100644 packages/cli/src/modules/agents/__tests__/skill-names.test.ts create mode 100644 packages/cli/src/modules/agents/__tests__/skill.service.test.ts create mode 100644 packages/cli/src/modules/agents/__tests__/skills-api.service.test.ts create mode 100644 packages/cli/src/modules/agents/__tests__/skills.controller.test.ts create mode 100644 packages/cli/src/modules/agents/repositories/skill.repository.ts create mode 100644 packages/cli/src/modules/agents/skills.controller.ts create mode 100644 packages/cli/src/modules/agents/skills/skill-content-hash.ts create mode 100644 packages/cli/src/modules/agents/skills/skill-names.ts create mode 100644 packages/cli/src/modules/agents/skills/skill.service.ts create mode 100644 packages/cli/src/modules/agents/skills/skills-api.service.ts create mode 100644 packages/cli/test/integration/agents/skill.repository.test.ts create mode 100644 packages/cli/test/integration/agents/skills.controller.test.ts 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 new file mode 100644 index 00000000000..a4b59565991 --- /dev/null +++ b/packages/@n8n/api-types/src/agents/__tests__/skills.schema.test.ts @@ -0,0 +1,74 @@ +import { + CreateSkillDto, + ListSkillsQueryDto, + SaveSkillDto, + SKILLS_MAX_PAGE_SIZE, +} from '../skills.schema'; + +describe('ListSkillsQueryDto', () => { + it('leaves take unset when the client sends none, so the whole list comes back', () => { + expect(ListSkillsQueryDto.parse({})).toEqual({ skip: 0, take: undefined }); + }); + + it('caps take at the page limit', () => { + expect(ListSkillsQueryDto.parse({ take: '500' }).take).toBe(SKILLS_MAX_PAGE_SIZE); + }); + + it('parses skip, take and the filters', () => { + expect( + ListSkillsQueryDto.parse({ + skip: '10', + take: '5', + search: ' voice ', + scope: 'project', + projectId: 'p1', + }), + ).toEqual({ skip: 10, take: 5, search: 'voice', scope: 'project', projectId: 'p1' }); + }); + + it.each([['0'], ['-1'], ['abc']])('rejects take "%s"', (take) => { + expect(ListSkillsQueryDto.safeParse({ take }).success).toBe(false); + }); + + it('parses the project an agent attaches skills for', () => { + expect(ListSkillsQueryDto.parse({ attachableTo: 'p1' })).toMatchObject({ attachableTo: 'p1' }); + }); + + it.each(['team', 'user'])('rejects the unknown scope %s', (scope) => { + expect(ListSkillsQueryDto.safeParse({ scope }).success).toBe(false); + }); +}); + +describe('CreateSkillDto', () => { + const skill = { name: 'Brand voice', description: 'How we write', instructions: 'Be brief.' }; + + it.each([ + [{ scope: 'project', projectId: 'p1' }], + [{ scope: 'agent', agentId: 'a1' }], + [{ scope: 'instance' }], + ])('accepts %o', (target) => { + expect(CreateSkillDto.safeParse({ ...target, skill }).success).toBe(true); + }); + + it('rejects a skill without instructions', () => { + expect( + CreateSkillDto.safeParse({ scope: 'instance', skill: { ...skill, instructions: '' } }) + .success, + ).toBe(false); + }); +}); + +describe('SaveSkillDto', () => { + it('accepts changed fields with the base hash', () => { + expect(SaveSkillDto.safeParse({ instructions: 'Be brief.', baseSkillHash: 'h1' }).success).toBe( + true, + ); + }); + + it.each([[{ instructions: 'Be brief.' }], [{ instructions: 'Be brief.', baseSkillHash: '' }]])( + 'rejects a save without a base hash: %o', + (payload) => { + expect(SaveSkillDto.safeParse(payload).success).toBe(false); + }, + ); +}); diff --git a/packages/@n8n/api-types/src/agents/index.ts b/packages/@n8n/api-types/src/agents/index.ts index 77b31bba4fb..d459d9f65c4 100644 --- a/packages/@n8n/api-types/src/agents/index.ts +++ b/packages/@n8n/api-types/src/agents/index.ts @@ -7,6 +7,7 @@ export * from './agent-json-config.schema'; export * from './agent-node-tool-operations'; export * from './agent-personalisation'; export * from './agent-skill.schema'; +export * from './skills.schema'; export * from './child-trace'; export type * from './background-job'; export * from './inline-agent-config.schema'; diff --git a/packages/@n8n/api-types/src/agents/skills.schema.ts b/packages/@n8n/api-types/src/agents/skills.schema.ts new file mode 100644 index 00000000000..f3b4b4a4408 --- /dev/null +++ b/packages/@n8n/api-types/src/agents/skills.schema.ts @@ -0,0 +1,122 @@ +import { z } from 'zod'; + +import { agentSkillSchema, agentSkillShape } from './agent-skill.schema'; +import type { AgentSkill } from './types'; +import { paginationSchema } from '../dto/pagination/pagination.dto'; +import { Z } from '../zod-class'; + +export const SKILLS_MAX_PAGE_SIZE = 100; + +/** Optional on purpose: without `take` the caller gets every visible skill. */ +const optionalTake = z + .string() + .optional() + .transform((value) => (value === undefined || value === '' ? undefined : Number(value))) + .refine((value) => value === undefined || (Number.isInteger(value) && value > 0), { + message: 'Param `take` must be a positive integer', + }) + .transform((value) => (value === undefined ? undefined : Math.min(value, SKILLS_MAX_PAGE_SIZE))); + +/** Who owns a skill: one project, one agent (only that agent can use it), or the instance. */ +export const skillScopeSchema = z.enum(['project', 'agent', 'instance']); + +export type SkillScope = z.infer; + +export type SkillSource = 'ui' | 'agent'; + +/** One row of the skills list. Name and description come from the latest version. */ +export type SkillListItem = { + id: string; + name: string; + description: string; + scope: SkillScope; + /** The project of a project skill, or the project of the agent of an agent skill. */ + projectId: string | null; + projectName: string | null; + /** Set for an agent skill: the only agent that can use it. */ + agentId: string | null; + agentName: string | null; + source: SkillSource; + /** Number of the latest version, the one following agents run. */ + latestVersion: number; + /** Distinct agents that use the skill: draft refs plus published pins. */ + usedByAgents: number; + canEdit: boolean; + canDelete: boolean; + createdAt: string; + updatedAt: string; +}; + +export type SkillListResponse = { + count: number; + data: SkillListItem[]; +}; + +/** The agents that use a skill, limited to the agents the caller may read. */ +export type SkillUsage = { + /** Agents whose draft references the skill. */ + drafts: Array<{ agentId: string; agentName: string; projectId: string }>; + /** Agents with a published version that pins the skill, one row each. */ + pins: Array<{ + agentId: string; + agentName: string; + projectId: string; + /** The skill version the current published version runs. Null when only older ones pin it. */ + version: number | null; + }>; + /** Agents in projects the caller may not read. */ + hiddenAgents: number; +}; + +/** One skill with the content of its latest version. */ +export type SkillDetail = SkillListItem & { + skill: AgentSkill; + /** Hash of `skill`. Send it back as `baseSkillHash` so a save cannot overwrite a newer version. */ + skillHash: string; + usedBy: SkillUsage; +}; + +export type SkillSaveResponse = { + id: string; + versionId: string; + version: number; + /** False when the content matched the latest version and nothing was saved. */ + created: boolean; + /** The content of the latest version after the save. */ + skill: AgentSkill; + skillHash: string; +}; + +export class ListSkillsQueryDto extends Z.class({ + skip: paginationSchema.skip, + take: optionalTake, + search: z.string().trim().max(200).optional(), + scope: skillScopeSchema.optional(), + projectId: z.string().max(36).optional(), + /** + * The id of an agent: the skills it may attach, which are its project's skills, its own + * agent skills and instance skills. Not combined with `scope` or `projectId`. + */ + attachableTo: z.string().max(36).optional(), +}) {} + +export class CreateSkillDto 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(), + skill: agentSkillSchema, +}) {} + +/** + * Saves the next version of a skill. `baseSkillHash` is the `skillHash` the editor + * started from: a save based on an older version is refused with 409. + */ +export class SaveSkillDto extends Z.class({ + name: agentSkillShape.name.optional(), + description: agentSkillShape.description.optional(), + instructions: agentSkillShape.instructions.optional(), + references: agentSkillShape.references.optional(), + baseSkillHash: z.string().min(1), +}) {} diff --git a/packages/@n8n/permissions/src/__tests__/__snapshots__/scope-information.test.ts.snap b/packages/@n8n/permissions/src/__tests__/__snapshots__/scope-information.test.ts.snap index 250208c05ef..e36a73f45f2 100644 --- a/packages/@n8n/permissions/src/__tests__/__snapshots__/scope-information.test.ts.snap +++ b/packages/@n8n/permissions/src/__tests__/__snapshots__/scope-information.test.ts.snap @@ -23,6 +23,16 @@ exports[`Scope Information > ensure scopes are defined correctly 1`] = ` "projectAiPreference:update", "projectAiPreference:delete", "projectAiPreference:list", + "skill:create", + "skill:read", + "skill:update", + "skill:delete", + "skill:list", + "projectSkill:create", + "projectSkill:read", + "projectSkill:update", + "projectSkill:delete", + "projectSkill:list", "annotationTag:create", "annotationTag:read", "annotationTag:update", diff --git a/packages/@n8n/permissions/src/constants.ee.ts b/packages/@n8n/permissions/src/constants.ee.ts index 74dcedc0b03..bb812d1a1f0 100644 --- a/packages/@n8n/permissions/src/constants.ee.ts +++ b/packages/@n8n/permissions/src/constants.ee.ts @@ -7,6 +7,10 @@ export const RESOURCES = { // `projectAiPreference`: rows of a project. A user's own rows need no scope. aiPreference: [...DEFAULT_OPERATIONS] as const, projectAiPreference: [...DEFAULT_OPERATIONS] as const, + // Skills. `skill`: instance skills. `projectSkill`: the skills of a project and of + // its agents. + skill: [...DEFAULT_OPERATIONS] as const, + projectSkill: [...DEFAULT_OPERATIONS] as const, annotationTag: [...DEFAULT_OPERATIONS] as const, auditLogs: ['manage'] as const, banner: ['dismiss'] as const, diff --git a/packages/@n8n/permissions/src/roles/__tests__/skill-scope.test.ts b/packages/@n8n/permissions/src/roles/__tests__/skill-scope.test.ts new file mode 100644 index 00000000000..e8b1de2986a --- /dev/null +++ b/packages/@n8n/permissions/src/roles/__tests__/skill-scope.test.ts @@ -0,0 +1,100 @@ +import { RESOURCES } from '@/constants.ee'; +import { + GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS, + PROJECT_CUSTOM_ROLE_OPERATIONS, +} from '@/roles/custom-role-scopes.ee'; +import { + GLOBAL_ADMIN_SCOPES, + GLOBAL_CHAT_USER_SCOPES, + GLOBAL_MEMBER_SCOPES, + GLOBAL_OWNER_SCOPES, +} from '@/roles/scopes/global-scopes.ee'; +import { + PERSONAL_PROJECT_OWNER_SCOPES, + PROJECT_EDITOR_SCOPES, + PROJECT_VIEWER_SCOPES, + REGULAR_PROJECT_ADMIN_SCOPES, +} from '@/roles/scopes/project-scopes.ee'; +import { scopeInformation } from '@/scope-information'; + +/** + * Skill scopes follow the AI preference split. `skill` covers instance skills. + * `projectSkill` covers the skills of one project and of its agents. Every user may read + * instance skills. + */ +describe('skill scopes', () => { + const write = ['create', 'update', 'delete']; + + it('offer the default operations on both resources', () => { + for (const resource of ['skill', 'projectSkill'] as const) { + expect(RESOURCES[resource]).toEqual( + expect.arrayContaining(['create', 'read', 'update', 'delete', 'list']), + ); + } + }); + + it('are granted in full to owners and admins, who manage instance skills', () => { + for (const scopes of [GLOBAL_OWNER_SCOPES, GLOBAL_ADMIN_SCOPES]) { + expect(scopes).toEqual( + expect.arrayContaining([ + ...['create', 'read', 'update', 'delete', 'list'].map((op) => `skill:${op}`), + ...['create', 'read', 'update', 'delete', 'list'].map((op) => `projectSkill:${op}`), + ]), + ); + } + }); + + it('are withheld from members and chat users, whose own skills need no scope', () => { + for (const scopes of [GLOBAL_MEMBER_SCOPES, GLOBAL_CHAT_USER_SCOPES]) { + expect( + scopes.filter((scope) => scope.startsWith('skill:') || scope.startsWith('projectSkill:')), + ).toEqual([]); + } + }); + + it('let project admins, personal owners and editors manage project skills', () => { + for (const scopes of [ + REGULAR_PROJECT_ADMIN_SCOPES, + PERSONAL_PROJECT_OWNER_SCOPES, + PROJECT_EDITOR_SCOPES, + ]) { + expect(scopes).toEqual( + expect.arrayContaining( + ['create', 'read', 'update', 'delete', 'list'].map((op) => `projectSkill:${op}`), + ), + ); + } + }); + + it('let project viewers only read and list project skills', () => { + expect(PROJECT_VIEWER_SCOPES).toEqual( + expect.arrayContaining(['projectSkill:read', 'projectSkill:list']), + ); + expect( + PROJECT_VIEWER_SCOPES.filter((scope) => write.some((op) => scope === `projectSkill:${op}`)), + ).toEqual([]); + }); + + it('can be given to custom roles, so a role without them cannot manage skills', () => { + expect(GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS.settings.Manage).toEqual( + expect.arrayContaining(['skill:create', 'skill:read', 'skill:update', 'skill:delete']), + ); + expect(PROJECT_CUSTOM_ROLE_OPERATIONS.projectSkill).toEqual( + expect.arrayContaining(['read', 'update', 'create', 'delete']), + ); + }); + + it('are described, so the custom-role picker can label them', () => { + for (const scope of [ + 'skill:read', + 'skill:update', + 'projectSkill:read', + 'projectSkill:update', + ] as const) { + expect(scopeInformation[scope]).toEqual({ + displayName: expect.any(String), + description: expect.any(String), + }); + } + }); +}); diff --git a/packages/@n8n/permissions/src/roles/custom-role-scopes.ee.ts b/packages/@n8n/permissions/src/roles/custom-role-scopes.ee.ts index b5329b85d45..2048f374e8f 100644 --- a/packages/@n8n/permissions/src/roles/custom-role-scopes.ee.ts +++ b/packages/@n8n/permissions/src/roles/custom-role-scopes.ee.ts @@ -61,6 +61,7 @@ export const PROJECT_CUSTOM_ROLE_OPERATIONS = { ], projectVariable: ['read', 'update', 'create', 'delete'], projectAiPreference: ['read', 'update', 'create', 'delete'], + projectSkill: ['read', 'update', 'create', 'delete'], } as const satisfies { [R in keyof typeof RESOURCES]?: ReadonlyArray<(typeof RESOURCES)[R][number]>; }; @@ -113,6 +114,11 @@ export const GLOBAL_CUSTOM_ROLE_SCOPE_GROUPS = { 'aiPreference:update', 'aiPreference:delete', 'aiPreference:list', + 'skill:create', // Context (instance skills) + 'skill:read', + 'skill:update', + 'skill:delete', + 'skill:list', 'chatHub:manage', // Chat 'aiAssistant:manage', // n8n Assistant 'instanceAi:manage', diff --git a/packages/@n8n/permissions/src/roles/scopes/global-scopes.ee.ts b/packages/@n8n/permissions/src/roles/scopes/global-scopes.ee.ts index 0cbc861f2ff..660f1c3118d 100644 --- a/packages/@n8n/permissions/src/roles/scopes/global-scopes.ee.ts +++ b/packages/@n8n/permissions/src/roles/scopes/global-scopes.ee.ts @@ -109,6 +109,16 @@ export const GLOBAL_OWNER_SCOPES: Scope[] = [ 'projectAiPreference:update', 'projectAiPreference:delete', 'projectAiPreference:list', + 'skill:create', + 'skill:read', + 'skill:update', + 'skill:delete', + 'skill:list', + 'projectSkill:create', + 'projectSkill:read', + 'projectSkill:update', + 'projectSkill:delete', + 'projectSkill:list', 'workflow:create', 'workflow:read', 'workflow:export', diff --git a/packages/@n8n/permissions/src/roles/scopes/project-scopes.ee.ts b/packages/@n8n/permissions/src/roles/scopes/project-scopes.ee.ts index d2f227d322a..a81d945a3f5 100644 --- a/packages/@n8n/permissions/src/roles/scopes/project-scopes.ee.ts +++ b/packages/@n8n/permissions/src/roles/scopes/project-scopes.ee.ts @@ -78,6 +78,11 @@ export const REGULAR_PROJECT_ADMIN_SCOPES: Scope[] = [ 'projectAiPreference:create', 'projectAiPreference:update', 'projectAiPreference:delete', + 'projectSkill:list', + 'projectSkill:read', + 'projectSkill:create', + 'projectSkill:update', + 'projectSkill:delete', ]; export const PERSONAL_PROJECT_OWNER_SCOPES: Scope[] = [ @@ -139,6 +144,11 @@ export const PERSONAL_PROJECT_OWNER_SCOPES: Scope[] = [ 'projectAiPreference:create', 'projectAiPreference:update', 'projectAiPreference:delete', + 'projectSkill:list', + 'projectSkill:read', + 'projectSkill:create', + 'projectSkill:update', + 'projectSkill:delete', ]; export const PROJECT_EDITOR_SCOPES: Scope[] = [ @@ -198,6 +208,11 @@ export const PROJECT_EDITOR_SCOPES: Scope[] = [ 'projectAiPreference:create', 'projectAiPreference:update', 'projectAiPreference:delete', + 'projectSkill:list', + 'projectSkill:read', + 'projectSkill:create', + 'projectSkill:update', + 'projectSkill:delete', ]; export const PROJECT_VIEWER_SCOPES: Scope[] = [ @@ -226,6 +241,8 @@ export const PROJECT_VIEWER_SCOPES: Scope[] = [ 'projectVariable:read', 'projectAiPreference:list', 'projectAiPreference:read', + 'projectSkill:list', + 'projectSkill:read', ]; export const PROJECT_CHAT_USER_SCOPES: Scope[] = ['agent:execute', 'workflow:execute-chat']; diff --git a/packages/@n8n/permissions/src/scope-information.ts b/packages/@n8n/permissions/src/scope-information.ts index eadd6f66a40..58fc6d52dd8 100644 --- a/packages/@n8n/permissions/src/scope-information.ts +++ b/packages/@n8n/permissions/src/scope-information.ts @@ -27,6 +27,22 @@ export const scopeInformation: Partial> = { description: 'Allows reading the AI preferences saved for the instance, the user, and their projects.', }, + 'skill:read': { + displayName: 'Read Skills', + description: 'Allows reading instance skills.', + }, + 'skill:update': { + displayName: 'Update Skills', + description: 'Allows editing and saving instance skills.', + }, + 'projectSkill:read': { + displayName: 'Read Project Skills', + description: 'Allows reading the skills of a project.', + }, + 'projectSkill:update': { + displayName: 'Update Project Skills', + description: 'Allows editing and saving the skills of a project.', + }, 'agent:create': { displayName: 'Create Agent', description: 'Allows creating new agents in a project.', diff --git a/packages/@n8n/permissions/src/utilities/__tests__/get-resource-permissions.test.ts b/packages/@n8n/permissions/src/utilities/__tests__/get-resource-permissions.test.ts index cf7e157828a..8def00c65db 100644 --- a/packages/@n8n/permissions/src/utilities/__tests__/get-resource-permissions.test.ts +++ b/packages/@n8n/permissions/src/utilities/__tests__/get-resource-permissions.test.ts @@ -10,6 +10,8 @@ describe('permissions', () => { aiAssistant: {}, aiPreference: {}, projectAiPreference: {}, + skill: {}, + projectSkill: {}, annotationTag: {}, auditLogs: {}, banner: {}, @@ -102,6 +104,8 @@ describe('permissions', () => { aiAssistant: {}, aiPreference: {}, projectAiPreference: {}, + skill: {}, + projectSkill: {}, annotationTag: {}, auditLogs: {}, banner: {}, diff --git a/packages/cli/src/modules/agents/__tests__/skill-content-hash.test.ts b/packages/cli/src/modules/agents/__tests__/skill-content-hash.test.ts new file mode 100644 index 00000000000..e6f2a7dfa4b --- /dev/null +++ b/packages/cli/src/modules/agents/__tests__/skill-content-hash.test.ts @@ -0,0 +1,72 @@ +import { skillContentHash, type SkillContent } from '../skills/skill-content-hash'; + +const content: SkillContent = { + name: 'Brand voice', + description: 'Write in our voice', + instructions: 'Use short sentences.', + frontmatter: { 'allowed-tools': 'Read Bash(git diff:*)' }, + files: [ + { path: 'references/z.md', content: 'z' }, + { path: 'references/a.md', content: 'a' }, + ], +}; + +describe('skillContentHash', () => { + // The data migration (MigrateAgentSkillsToHub.contentHash, CONTEXT-225) must produce the + // same value for the same skill. Its test must pin this input to this literal too. + it('pins the hash that the data migration must also write', () => { + expect(skillContentHash(content)).toBe( + 'fb5a3e8555463ac71ac16733c2b9fdf41d0e15977ac1583646823b0d6d855ab2', + ); + }); + + it('ignores the order of the files', () => { + expect(skillContentHash({ ...content, files: [...content.files].reverse() })).toBe( + skillContentHash(content), + ); + }); + + // A locale-aware sort puts `b.md` first and gives a different hash than the migration. + it('sorts file paths by code unit, so an upper-case path comes first', () => { + const files = [ + { path: 'references/b.md', content: 'b' }, + { path: 'references/B.md', content: 'B' }, + ]; + + expect(skillContentHash({ ...content, files })).toBe( + '030387c0a4c961a2a79c3b896a18fb1b45c883b3ecb57e1a94e25898def497a4', + ); + }); + + it('ignores the order of the frontmatter keys', () => { + expect(skillContentHash({ ...content, frontmatter: { b: '2', a: '1' } })).toBe( + skillContentHash({ ...content, frontmatter: { a: '1', b: '2' } }), + ); + }); + + it('ignores the key order of nested frontmatter maps', () => { + expect( + skillContentHash({ ...content, frontmatter: { metadata: { b: 2, a: [{ y: 1, x: 2 }] } } }), + ).toBe( + skillContentHash({ ...content, frontmatter: { metadata: { a: [{ x: 2, y: 1 }], b: 2 } } }), + ); + }); + + it('keeps the order of frontmatter lists', () => { + expect(skillContentHash({ ...content, frontmatter: { tags: ['a', 'b'] } })).not.toBe( + skillContentHash({ ...content, frontmatter: { tags: ['b', 'a'] } }), + ); + }); + + it('changes when the name changes', () => { + expect(skillContentHash({ ...content, name: 'Brand tone' })).not.toBe( + skillContentHash(content), + ); + }); + + it('changes when a file changes', () => { + const files = [{ path: 'references/a.md', content: 'changed' }, content.files[0]]; + + expect(skillContentHash({ ...content, files })).not.toBe(skillContentHash(content)); + }); +}); diff --git a/packages/cli/src/modules/agents/__tests__/skill-names.test.ts b/packages/cli/src/modules/agents/__tests__/skill-names.test.ts new file mode 100644 index 00000000000..a0158e4ef7b --- /dev/null +++ b/packages/cli/src/modules/agents/__tests__/skill-names.test.ts @@ -0,0 +1,67 @@ +import { + findIntroducedNameClashes, + findNameClashGroups, + renameIntroducesClash, + skillNameKey, +} from '../skills/skill-names'; + +describe('skillNameKey', () => { + it.each([ + ['Brand voice', 'brand-voice'], + ['Brand voice', 'brand_voice'], + ['Brand voice', ' BRAND voice '], + ['Brand voice', 'brand -_ voice'], + ])('treats "%s" and "%s" as the same name', (left, right) => { + expect(skillNameKey(left)).toBe(skillNameKey(right)); + }); + + it('keeps other characters apart', () => { + expect(skillNameKey('Brand voice')).not.toBe(skillNameKey('Brand voices')); + expect(skillNameKey('brand.voice')).not.toBe(skillNameKey('brand voice')); + }); +}); + +describe('findIntroducedNameClashes', () => { + it('returns an added name that clashes with an existing name', () => { + expect(findIntroducedNameClashes(['Brand voice'], ['brand-voice', 'Pricing'])).toEqual([ + 'brand-voice', + ]); + }); + + it('returns an added name that clashes with another added name', () => { + expect(findIntroducedNameClashes([], ['Pricing', 'pricing'])).toEqual(['pricing']); + }); + + it('ignores clashes among the existing names', () => { + expect(findIntroducedNameClashes(['Pricing', 'pricing'], ['Brand voice'])).toEqual([]); + }); +}); + +describe('renameIntroducesClash', () => { + it('is true when the new name clashes with another skill of the agent', () => { + expect(renameIntroducesClash(['Pricing'], 'Prices', 'pricing')).toBe(true); + }); + + it('is false when the new name differs from the old one only in case or separators', () => { + expect(renameIntroducesClash(['brand-voice'], 'Brand voice', 'brand_voice')).toBe(false); + }); + + it('is false when no other skill has a similar name', () => { + expect(renameIntroducesClash(['Pricing'], 'Brand voice', 'Tone')).toBe(false); + }); +}); + +describe('findNameClashGroups', () => { + it('groups the names that clash, as written', () => { + expect( + findNameClashGroups(['Brand voice', 'Pricing', 'brand-voice', 'pricing', 'Tone']), + ).toEqual([ + ['Brand voice', 'brand-voice'], + ['Pricing', 'pricing'], + ]); + }); + + it('returns no group when all names differ', () => { + expect(findNameClashGroups(['Brand voice', 'Pricing'])).toEqual([]); + }); +}); diff --git a/packages/cli/src/modules/agents/__tests__/skill.service.test.ts b/packages/cli/src/modules/agents/__tests__/skill.service.test.ts new file mode 100644 index 00000000000..d84b9639770 --- /dev/null +++ b/packages/cli/src/modules/agents/__tests__/skill.service.test.ts @@ -0,0 +1,841 @@ +/* eslint-disable @typescript-eslint/unbound-method -- mock-based tests reference unbound methods */ +import type { ProjectScopeService } from '@n8n/backend-services'; +import type { OperationContext, TransactionRunner, User } from '@n8n/db'; +import { ConflictError, ForbiddenError, NotFoundError, UserError } from '@n8n/errors'; +import { hasGlobalScope } from '@n8n/permissions'; +import { mock } from 'vitest-mock-extended'; + +import { userHasScopes } from '@/permissions.ee/check-access'; + +import type { AgentUpdateBroadcaster } from '../agent-update-broadcaster'; +import type { SkillFile } from '../entities/skill-file.entity'; +import type { SkillVersion } from '../entities/skill-version.entity'; +import type { Skill } from '../entities/skill.entity'; +import type { AgentRepository } from '../repositories/agent.repository'; +import type { ResolvedSkillRow, SkillRepository } from '../repositories/skill.repository'; +import { skillContentHash } from '../skills/skill-content-hash'; +import { getAgentSkillHash } from '../utils/agent-config-hash'; +import { SkillService, toAgentSkill, toSkillContent } from '../skills/skill.service'; + +const clearRuntimes = vi.fn(); +vi.mock('../agent-runtime-cache.service', () => ({ AgentRuntimeCacheService: class {} })); +vi.mock('@n8n/di', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + Container: { ...actual.Container, get: vi.fn(() => ({ clearRuntimes })) }, + }; +}); +vi.mock('@/permissions.ee/check-access', () => ({ userHasScopes: vi.fn() })); +vi.mock('@n8n/permissions', async (importOriginal) => ({ + ...(await importOriginal()), + hasGlobalScope: vi.fn(), +})); + +const PROJECT = 'project-1'; + +async function expectError( + promise: Promise, + errorClass: new (...args: never[]) => Error, + message: string, +) { + const error: unknown = await promise.then( + () => undefined, + (caught: unknown) => caught, + ); + expect(error).toBeInstanceOf(errorClass); + expect(error).toHaveProperty('message', message); +} +const TX_CTX = { trx: {} } as OperationContext; + +function skillRow(id: string, target: Partial> = {}): Skill { + return mock({ id, projectId: target.projectId ?? null, agentId: target.agentId ?? null }); +} + +function versionRow( + skill: Skill, + version: number, + overrides: Partial = {}, + files: Array<{ path: string; content: string }> = [], +): ResolvedSkillRow { + const base = { + name: 'Brand voice', + description: 'Write in our voice', + instructions: 'Use short sentences.', + frontmatter: null, + ...overrides, + }; + const contentHash = + overrides.contentHash ?? + skillContentHash({ + name: base.name, + description: base.description, + instructions: base.instructions, + frontmatter: base.frontmatter, + files, + }); + return { + skill, + version: mock({ + id: `${skill.id}-v${version}`, + skillId: skill.id, + version, + ...base, + contentHash, + }), + files: files.map((file) => mock(file)), + }; +} + +describe('SkillService', () => { + const skills = mock(); + const agents = mock(); + const txRunner = mock(); + const broadcaster = mock(); + const projectScope = mock(); + const service = new SkillService(mock(), skills, agents, txRunner, broadcaster, projectScope); + const user = mock({ id: 'user-1' }); + + beforeEach(() => { + vi.resetAllMocks(); + txRunner.run.mockImplementation(async (_ctx, fn) => await fn(TX_CTX)); + skills.findLatestSaved.mockResolvedValue(new Map()); + skills.findVersionsByIds.mockResolvedValue(new Map()); + skills.findDependencies.mockResolvedValue(new Map()); + skills.findByIds.mockResolvedValue([]); + skills.findFollowingAgents.mockResolvedValue([]); + skills.findFollowingAgentIds.mockResolvedValue([]); + agents.markDraftChangedIfInSync.mockResolvedValue([]); + agents.findProjectIdsByIds.mockResolvedValue([]); + }); + + describe('conversions', () => { + it('ignores allowed tools on write', () => { + expect( + toSkillContent({ + name: 'n', + description: 'd', + instructions: 'i', + allowedTools: ['Read', 'Grep'], + references: [{ path: 'references/a.md', content: 'a' }], + }), + ).toEqual({ + name: 'n', + description: 'd', + instructions: 'i', + frontmatter: null, + files: [{ path: 'references/a.md', content: 'a' }], + }); + }); + + it('keeps the frontmatter as given and reads no allowed tools from it', () => { + const row = versionRow(skillRow('skill_a'), 1, { + frontmatter: { 'allowed-tools': 'Read Grep', license: 'MIT' }, + }); + + expect(toSkillContent(toAgentSkill(row), row.version.frontmatter).frontmatter).toEqual({ + 'allowed-tools': 'Read Grep', + license: 'MIT', + }); + expect(toAgentSkill(row)).toEqual({ + name: 'Brand voice', + description: 'Write in our voice', + instructions: 'Use short sentences.', + }); + }); + }); + + describe('resolveForAgentDraft', () => { + it('runs the latest saved version for a following ref', async () => { + const skill = skillRow('skill_a'); + skills.findLatestSaved.mockResolvedValue( + new Map([['skill_a', versionRow(skill, 2, { instructions: 'v2' })]]), + ); + + const resolved = await service.resolveForAgentDraft([{ id: 'skill_a' }]); + + expect(resolved.skill_a.instructions).toBe('v2'); + }); + + it('runs the pinned version for a ref with versionId', async () => { + const skill = skillRow('skill_a'); + const v1 = versionRow(skill, 1, { instructions: 'v1' }); + skills.findLatestSaved.mockResolvedValue( + new Map([['skill_a', versionRow(skill, 2, { instructions: 'v2' })]]), + ); + skills.findVersionsByIds.mockResolvedValue(new Map([[v1.version.id, v1]])); + + const resolved = await service.resolveForAgentDraft([ + { id: 'skill_a', versionId: v1.version.id }, + ]); + + expect(resolved.skill_a.instructions).toBe('v1'); + }); + + it('falls back to the latest version when the pin belongs to another skill', async () => { + const foreign = versionRow(skillRow('skill_b'), 1, { instructions: 'other skill' }); + skills.findLatestSaved.mockResolvedValue( + new Map([['skill_a', versionRow(skillRow('skill_a'), 2, { instructions: 'v2' })]]), + ); + skills.findVersionsByIds.mockResolvedValue(new Map([[foreign.version.id, foreign]])); + + const resolved = await service.resolveForAgentDraft([ + { id: 'skill_a', versionId: foreign.version.id }, + ]); + + expect(resolved.skill_a.instructions).toBe('v2'); + }); + + it('leaves out a ref to a missing skill', async () => { + expect(await service.resolveForAgentDraft([{ id: 'skill_gone' }])).toEqual({}); + }); + }); + + describe('resolvePinned', () => { + it('returns the pinned versions keyed by skill id', async () => { + skills.findPinned.mockResolvedValue( + new Map([['skill_a', versionRow(skillRow('skill_a'), 1, { instructions: 'v1' })]]), + ); + + const resolved = await service.resolvePinned('agent-version-1'); + + expect(resolved).toEqual({ skill_a: expect.objectContaining({ instructions: 'v1' }) }); + }); + }); + + describe('create', () => { + it('creates the skill with an id in the agent skill format', async () => { + const id = await service.create({ + target: { projectId: PROJECT, agentId: null }, + skill: { name: 'n', description: 'd', instructions: 'i' }, + source: 'ui', + createdById: user.id, + }); + + expect(id).toMatch(/^skill_[A-Za-z0-9]{16}$/); + expect(skills.createSkill).toHaveBeenCalledWith( + { id, target: { projectId: PROJECT, agentId: null }, source: 'ui', createdById: user.id }, + expect.objectContaining({ name: 'n', frontmatter: null }), + expect.anything(), + ); + }); + + it('refuses blank instructions', async () => { + await expect( + service.create({ + target: { projectId: PROJECT, agentId: null }, + skill: { name: 'n', description: 'd', instructions: ' ' }, + source: 'ui', + createdById: user.id, + }), + ).rejects.toThrow(UserError); + expect(skills.createSkill).not.toHaveBeenCalled(); + }); + }); + + describe('save', () => { + const skill = skillRow('skill_a', { projectId: PROJECT }); + + /** Mocks the latest version and returns its hash, the base an editor sends back. */ + function givenLatest(latest: ResolvedSkillRow): string { + skills.findLatestSaved.mockImplementation(async (ids) => { + const rows = new Map(); + if (ids.includes('skill_a')) rows.set('skill_a', latest); + return rows; + }); + skills.nextVersionNumber.mockResolvedValue(3); + skills.insertSavedVersion.mockResolvedValue('new-version-id'); + return getAgentSkillHash(toAgentSkill(latest)); + } + + it('creates nothing when the content matches the latest version', async () => { + const baseSkillHash = givenLatest(versionRow(skill, 2)); + + const result = await service.save( + 'skill_a', + { baseSkillHash, instructions: 'Use short sentences.' }, + user.id, + ); + + expect(result).toMatchObject({ versionId: 'skill_a-v2', version: 2, created: false }); + expect(skills.insertSavedVersion).not.toHaveBeenCalled(); + expect(agents.markDraftChangedIfInSync).not.toHaveBeenCalled(); + }); + + it('saves changed content as the next version under an edit lock', async () => { + const baseSkillHash = givenLatest(versionRow(skill, 2)); + + const result = await service.save( + 'skill_a', + { + baseSkillHash, + instructions: 'new', + references: [{ path: 'references/a.md', content: 'a' }], + }, + user.id, + ); + + expect(result).toMatchObject({ versionId: 'new-version-id', version: 3, created: true }); + expect(result.skill).toMatchObject({ name: 'Brand voice', instructions: 'new' }); + expect(skills.lockForEdit).toHaveBeenCalledWith(['skill_a'], TX_CTX); + expect(skills.insertSavedVersion).toHaveBeenCalledWith( + 'skill_a', + 3, + expect.objectContaining({ + name: 'Brand voice', + instructions: 'new', + files: [{ path: 'references/a.md', content: 'a' }], + }), + user.id, + TX_CTX, + ); + }); + + it('removes references set to an empty list', async () => { + const baseSkillHash = givenLatest( + versionRow(skill, 2, {}, [{ path: 'references/a.md', content: 'a' }]), + ); + + const result = await service.save('skill_a', { baseSkillHash, references: [] }, user.id); + + expect(result.skill).not.toHaveProperty('references'); + }); + + it('ignores allowed tools in the update', async () => { + const baseSkillHash = givenLatest( + versionRow(skill, 2, { frontmatter: { 'allowed-tools': 'Read' } }), + ); + + const result = await service.save( + 'skill_a', + { baseSkillHash, instructions: 'new', allowedTools: ['Bash'] }, + user.id, + ); + + expect(result.skill).not.toHaveProperty('allowedTools'); + expect(skills.insertSavedVersion).toHaveBeenCalledWith( + 'skill_a', + 3, + expect.objectContaining({ frontmatter: { 'allowed-tools': 'Read' } }), + user.id, + TX_CTX, + ); + }); + + it('keeps frontmatter keys the editor does not know', async () => { + const baseSkillHash = givenLatest( + versionRow(skill, 2, { frontmatter: { license: 'MIT', 'allowed-tools': 'Read' } }), + ); + + await service.save('skill_a', { baseSkillHash, instructions: 'new' }, user.id); + + expect(skills.insertSavedVersion).toHaveBeenCalledWith( + 'skill_a', + 3, + expect.objectContaining({ frontmatter: { license: 'MIT', 'allowed-tools': 'Read' } }), + user.id, + TX_CTX, + ); + }); + + it('accepts the hash of the latest version as the base', async () => { + const latest = versionRow(skill, 2); + const baseSkillHash = givenLatest(latest); + + const result = await service.save('skill_a', { instructions: 'new', baseSkillHash }, user.id); + + expect(result.created).toBe(true); + }); + + // The check runs under the edit lock, so two saves from the same base cannot both pass. + it('refuses a stale base hash after taking the edit lock', async () => { + givenLatest(versionRow(skill, 2)); + + await expect( + service.save('skill_a', { instructions: 'new', baseSkillHash: 'stale' }, user.id), + ).rejects.toThrow(ConflictError); + expect(skills.lockForEdit.mock.invocationCallOrder[0]).toBeLessThan( + skills.findLatestSaved.mock.invocationCallOrder[0], + ); + expect(skills.insertSavedVersion).not.toHaveBeenCalled(); + }); + + it('refuses blank instructions', async () => { + const baseSkillHash = givenLatest(versionRow(skill, 2)); + + await expect( + service.save('skill_a', { baseSkillHash, instructions: ' ' }, user.id), + ).rejects.toThrow(UserError); + }); + + it('marks the following agents in the same transaction', async () => { + const baseSkillHash = givenLatest(versionRow(skill, 2)); + skills.findFollowingAgentIds.mockResolvedValue(['agent-1', 'agent-2']); + + await service.save('skill_a', { baseSkillHash, instructions: 'new' }, user.id); + + expect(skills.findFollowingAgentIds).toHaveBeenCalledWith(['skill_a'], TX_CTX); + expect(agents.markDraftChangedIfInSync).toHaveBeenCalledWith(['agent-1', 'agent-2'], TX_CTX); + }); + + it('clears the runtime and pushes an update for every following agent after the commit', async () => { + const baseSkillHash = givenLatest(versionRow(skill, 2)); + skills.findFollowingAgentIds.mockResolvedValue(['agent-1', 'agent-2']); + // agent-2 already had unpublished changes, so only agent-1 is marked. + agents.markDraftChangedIfInSync.mockResolvedValue(['agent-1']); + agents.findProjectIdsByIds.mockResolvedValue([ + { id: 'agent-1', projectId: PROJECT }, + { id: 'agent-2', projectId: PROJECT }, + ]); + + await service.save('skill_a', { baseSkillHash, instructions: 'new' }, user.id); + + expect(clearRuntimes.mock.calls).toEqual([['agent-1'], ['agent-2']]); + expect(broadcaster.notify).toHaveBeenCalledTimes(2); + expect(broadcaster.notify).toHaveBeenCalledWith({ + projectId: PROJECT, + agentId: 'agent-2', + source: 'user', + }); + }); + + it('rejects a rename that clashes on a following agent', async () => { + skills.nextVersionNumber.mockResolvedValue(3); + skills.findFollowingAgents.mockResolvedValue([{ id: 'agent-1', name: 'Sales bot' }]); + skills.findDependencies.mockResolvedValue( + new Map([ + [ + 'agent-1', + [ + { skillId: 'skill_a', versionId: null }, + { skillId: 'skill_b', versionId: null }, + ], + ], + ]), + ); + skills.findLatestSaved.mockImplementation( + async (ids) => + new Map( + ids.map((id) => [ + id, + versionRow(skillRow(id), 2, { name: id === 'skill_b' ? 'Pricing' : 'Brand voice' }), + ]), + ), + ); + + const baseSkillHash = getAgentSkillHash( + toAgentSkill(versionRow(skillRow('skill_a'), 2, { name: 'Brand voice' })), + ); + + await expectError( + service.save('skill_a', { baseSkillHash, name: 'pricing' }, user.id), + UserError, + 'Cannot rename to "pricing": Sales bot already uses a skill with a name like that.', + ); + expect(skills.insertSavedVersion).not.toHaveBeenCalled(); + }); + + it('allows a rename that clashes on no agent', async () => { + const baseSkillHash = givenLatest(versionRow(skill, 2)); + + await expect( + service.save('skill_a', { baseSkillHash, name: 'Tone' }, user.id), + ).resolves.toMatchObject({ + created: true, + }); + }); + + it('throws NotFoundError for a missing skill', async () => { + await expect( + service.save('skill_gone', { name: 'x', baseSkillHash: 'any' }, user.id), + ).rejects.toThrow(NotFoundError); + }); + }); + + describe('publish', () => { + it('locks the skills and pins the version each ref resolves to', async () => { + const a = skillRow('skill_a'); + const b = skillRow('skill_b'); + const pinnedB = versionRow(b, 1); + skills.findLatestSaved.mockResolvedValue( + new Map([ + ['skill_a', versionRow(a, 3)], + ['skill_b', versionRow(b, 2)], + ]), + ); + skills.findVersionsByIds.mockResolvedValue(new Map([[pinnedB.version.id, pinnedB]])); + + const snapshot = await service.snapshotForPublish( + [{ id: 'skill_a' }, { id: 'skill_b', versionId: pinnedB.version.id }], + TX_CTX, + ); + await service.pinForPublish('agent-version-1', snapshot.versionByRef, TX_CTX); + + expect(skills.lockForPublish).toHaveBeenCalledWith(['skill_a', 'skill_b'], TX_CTX); + expect(Object.keys(snapshot.skills)).toEqual(['skill_a', 'skill_b']); + expect(skills.insertPins).toHaveBeenCalledWith( + [ + { + agentVersionId: 'agent-version-1', + skillId: 'skill_a', + skillVersionId: 'skill_a-v3', + }, + { + agentVersionId: 'agent-version-1', + skillId: 'skill_b', + skillVersionId: 'skill_b-v1', + }, + ], + TX_CTX, + ); + expect(skills.insertSavedVersion).not.toHaveBeenCalled(); + }); + }); + + describe('restoreFromVersion', () => { + it('pins each ref to the version that agent version used and writes no skill', async () => { + const v1 = versionRow(skillRow('skill_a'), 1); + skills.findPinned.mockResolvedValue(new Map([['skill_a', v1]])); + + const refs = await service.restoreFromVersion('agent-version-1', [ + { id: 'skill_a' }, + { id: 'skill_unpinned', versionId: 'stale' }, + ]); + + expect(refs).toEqual([{ id: 'skill_a', versionId: 'skill_a-v1' }, { id: 'skill_unpinned' }]); + expect(skills.insertSavedVersion).not.toHaveBeenCalled(); + }); + }); + + describe('isAttachable', () => { + const agent = { id: 'agent-1', projectId: PROJECT }; + + it.each([ + ['an instance skill', {}, true], + ['a skill of the agent project', { projectId: PROJECT }, true], + ['a skill of another project', { projectId: 'project-2' }, false], + ['a skill of this agent', { agentId: 'agent-1' }, true], + ['a skill of another agent', { agentId: 'agent-2' }, false], + ])('is %s attachable: %s', (_label, target, expected) => { + expect(service.isAttachable(skillRow('skill_a', target), agent)).toBe(expected); + }); + }); + + describe('checkRefs', () => { + const agent = { id: 'agent-1', projectId: PROJECT }; + + function givenAgentRefs(refs: Array<{ skillId: string; versionId?: string }>) { + skills.findDependencies.mockResolvedValue( + new Map([['agent-1', refs.map((ref) => ({ versionId: null, ...ref }))]]), + ); + } + + beforeEach(() => givenAgentRefs([{ skillId: 'skill_old' }])); + + function givenSkills(rows: Array<{ skill: Skill; name: string }>) { + skills.findByIds.mockResolvedValue(rows.map((row) => row.skill)); + skills.findLatestSaved.mockImplementation( + async (ids) => + new Map( + rows + .filter((row) => ids.includes(row.skill.id)) + .map((row) => [row.skill.id, versionRow(row.skill, 1, { name: row.name })]), + ), + ); + } + + it("reads the agent's current refs from its dependency rows", async () => { + await service.checkRefs(agent, []); + + expect(skills.findDependencies).toHaveBeenCalledWith(['agent-1'], {}); + }); + + it('keeps a new ref to an attachable skill', async () => { + givenSkills([ + { skill: skillRow('skill_old', { projectId: PROJECT }), name: 'Pricing' }, + { skill: skillRow('skill_new'), name: 'Tone' }, + ]); + + const refs = await service.checkRefs(agent, [{ id: 'skill_old' }, { id: 'skill_new' }]); + + expect(refs).toEqual([{ id: 'skill_old' }, { id: 'skill_new' }]); + }); + + it('drops a new ref to a missing skill', async () => { + givenSkills([{ skill: skillRow('skill_old', { projectId: PROJECT }), name: 'Pricing' }]); + + const refs = await service.checkRefs(agent, [{ id: 'skill_old' }, { id: 'skill_gone' }]); + + expect(refs).toEqual([{ id: 'skill_old' }]); + }); + + it('keeps a ref the agent already had even when its skill is gone', async () => { + const refs = await service.checkRefs(agent, [{ id: 'skill_old' }]); + + expect(refs).toEqual([{ id: 'skill_old' }]); + }); + + it.each([ + ['a skill of another agent', { agentId: 'agent-2' }], + ['a skill of another project', { projectId: 'project-2' }], + ])('rejects a new ref to %s', async (_label, target) => { + givenSkills([{ skill: skillRow('skill_new', target), name: 'Tone' }]); + + await expectError( + service.checkRefs(agent, [{ id: 'skill_new' }]), + UserError, + 'Skill "Tone" (skill_new) is not available to this agent. An agent can use the skills of its own project, its own skills and instance skills.', + ); + }); + + it('rejects a new ref whose name clashes with a skill of the agent', async () => { + givenSkills([ + { skill: skillRow('skill_old', { projectId: PROJECT }), name: 'Brand voice' }, + { skill: skillRow('skill_new'), name: 'brand-voice' }, + ]); + + await expectError( + service.checkRefs(agent, [{ id: 'skill_old' }, { id: 'skill_new' }]), + UserError, + 'This agent already has a skill with a name like "brand-voice". Rename one of them, or detach the other skill first.', + ); + }); + + it('keeps a clash the agent already had', async () => { + givenAgentRefs([{ skillId: 'skill_a' }, { skillId: 'skill_b' }]); + givenSkills([ + { skill: skillRow('skill_a', { projectId: PROJECT }), name: 'Pricing' }, + { skill: skillRow('skill_b', { projectId: PROJECT }), name: 'pricing' }, + ]); + + const refs = await service.checkRefs(agent, [{ id: 'skill_a' }, { id: 'skill_b' }]); + + expect(refs).toHaveLength(2); + }); + + it('keeps a pin the agent already had and drops a pin it did not have', async () => { + givenAgentRefs([{ skillId: 'skill_a', versionId: 'v1' }, { skillId: 'skill_b' }]); + givenSkills([ + { skill: skillRow('skill_a', { projectId: PROJECT }), name: 'Pricing' }, + { skill: skillRow('skill_b', { projectId: PROJECT }), name: 'Tone' }, + ]); + + const refs = await service.checkRefs(agent, [ + { id: 'skill_a', versionId: 'v1' }, + { id: 'skill_b', versionId: 'v9' }, + ]); + + expect(refs).toEqual([{ id: 'skill_a', versionId: 'v1' }, { id: 'skill_b' }]); + }); + + it('lets a save clear a pin', async () => { + givenAgentRefs([{ skillId: 'skill_a', versionId: 'v1' }]); + givenSkills([{ skill: skillRow('skill_a', { projectId: PROJECT }), name: 'Pricing' }]); + + expect(await service.checkRefs(agent, [{ id: 'skill_a' }])).toEqual([{ id: 'skill_a' }]); + }); + }); + + describe('canEdit', () => { + it('checks projectSkill:update on the project of a project skill', async () => { + vi.mocked(userHasScopes).mockResolvedValue(true); + + expect(await service.canEdit(user, skillRow('skill_a', { projectId: PROJECT }))).toBe(true); + expect(userHasScopes).toHaveBeenCalledWith(user, ['projectSkill:update'], false, { + projectId: PROJECT, + }); + }); + + it('checks projectSkill:update on the project of the agent of an agent skill', async () => { + vi.mocked(userHasScopes).mockResolvedValue(true); + skills.findAgents.mockResolvedValue( + new Map([['agent-1', { name: 'Support bot', projectId: PROJECT }]]), + ); + + expect(await service.canEdit(user, skillRow('skill_a', { agentId: 'agent-1' }))).toBe(true); + expect(skills.findAgents).toHaveBeenCalledWith(['agent-1'], {}); + expect(userHasScopes).toHaveBeenCalledWith(user, ['projectSkill:update'], false, { + projectId: PROJECT, + }); + }); + + it('needs skill:update for an instance skill', async () => { + vi.mocked(hasGlobalScope).mockReturnValue(true); + + expect(await service.canEdit(user, skillRow('skill_a'))).toBe(true); + expect(hasGlobalScope).toHaveBeenCalledWith(user, 'skill:update'); + }); + + it('refuses an instance skill without skill:update', async () => { + vi.mocked(hasGlobalScope).mockReturnValue(false); + + expect(await service.canEdit(user, skillRow('skill_a'))).toBe(false); + }); + }); + + describe('canAccess', () => { + it('lets every user read an instance skill', async () => { + vi.mocked(hasGlobalScope).mockReturnValue(false); + + expect(await service.canAccess(user, skillRow('skill_a'), 'read')).toBe(true); + }); + + it('needs skill:read for an agent skill whose agent is gone', async () => { + vi.mocked(hasGlobalScope).mockReturnValue(false); + skills.findAgents.mockResolvedValue(new Map()); + + expect(await service.canAccess(user, skillRow('skill_a', { agentId: 'gone' }), 'read')).toBe( + false, + ); + expect(hasGlobalScope).toHaveBeenCalledWith(user, 'skill:read'); + }); + + it('checks the scope of the operation on the project of a project skill', async () => { + vi.mocked(userHasScopes).mockResolvedValue(false); + + expect( + await service.canAccess(user, skillRow('skill_a', { projectId: PROJECT }), 'delete'), + ).toBe(false); + expect(userHasScopes).toHaveBeenCalledWith(user, ['projectSkill:delete'], false, { + projectId: PROJECT, + }); + }); + + it('needs skill:delete to delete an instance skill', async () => { + vi.mocked(hasGlobalScope).mockReturnValue(true); + + expect(await service.canAccess(user, skillRow('skill_a'), 'delete')).toBe(true); + expect(hasGlobalScope).toHaveBeenCalledWith(user, 'skill:delete'); + }); + }); + + describe('accessCheck', () => { + it('reads the projects of the operation once and checks project skills against them', async () => { + projectScope.getProjectIds.mockResolvedValue([PROJECT]); + vi.mocked(hasGlobalScope).mockReturnValue(false); + + const canUpdate = await service.accessCheck(user, 'update'); + + expect(canUpdate(skillRow('skill_a', { projectId: PROJECT }), PROJECT)).toBe(true); + expect(canUpdate(skillRow('skill_b', { projectId: 'other-project' }), 'other-project')).toBe( + false, + ); + // An agent skill is checked against the project of its agent. + expect(canUpdate(skillRow('skill_c', { agentId: 'agent-1' }), PROJECT)).toBe(true); + expect(projectScope.getProjectIds).toHaveBeenCalledTimes(1); + expect(projectScope.getProjectIds).toHaveBeenCalledWith(user, ['projectSkill:update']); + expect(userHasScopes).not.toHaveBeenCalled(); + }); + + it('allows every project skill when the global role grants the scope', async () => { + projectScope.getProjectIds.mockResolvedValue(null); + + const canDelete = await service.accessCheck(user, 'delete'); + + expect(canDelete(skillRow('skill_a', { projectId: 'any-project' }), 'any-project')).toBe( + true, + ); + }); + + it('applies the canAccess rules to instance skills', async () => { + projectScope.getProjectIds.mockResolvedValue([]); + vi.mocked(hasGlobalScope).mockReturnValue(false); + + const canUpdate = await service.accessCheck(user, 'update'); + const canRead = await service.accessCheck(user, 'read'); + + expect(canUpdate(skillRow('skill_c'), null)).toBe(false); + expect(canRead(skillRow('skill_c'), null)).toBe(true); + }); + }); + + describe('assertCanEdit', () => { + it('names the skills the user cannot edit', async () => { + vi.mocked(hasGlobalScope).mockReturnValue(false); + skills.findByIds.mockResolvedValue([skillRow('skill_a')]); + skills.findLatestSaved.mockResolvedValue( + new Map([['skill_a', versionRow(skillRow('skill_a'), 1, { name: 'Tone' })]]), + ); + + await expectError( + service.assertCanEdit(user, ['skill_a']), + ForbiddenError, + 'You can use but not edit "Tone" (skill_a). Ask someone who can edit this skill, or detach it from this agent.', + ); + }); + }); + + describe('deleteSkill', () => { + it('deletes a skill nobody uses, under an edit lock', async () => { + skills.findByIds.mockResolvedValue([skillRow('skill_a')]); + skills.findUsage.mockResolvedValue({ drafts: [], pins: [], hiddenAgents: 0 }); + + await service.deleteSkill('skill_a'); + + expect(skills.findUsage).toHaveBeenCalledWith('skill_a', 'all', TX_CTX); + expect(skills.lockForEdit).toHaveBeenCalledWith(['skill_a'], TX_CTX); + expect(skills.deleteSkill).toHaveBeenCalledWith('skill_a', TX_CTX); + }); + + it('refuses while agents use the skill and names them', async () => { + skills.findByIds.mockResolvedValue([skillRow('skill_a')]); + skills.findLatestSaved.mockResolvedValue( + new Map([['skill_a', versionRow(skillRow('skill_a'), 2)]]), + ); + skills.findUsage.mockResolvedValue({ + drafts: [{ agentId: 'agent-1', agentName: 'Support bot', projectId: PROJECT }], + pins: [ + { agentId: 'agent-2', agentName: 'Sales bot', projectId: PROJECT, version: 2 }, + { agentId: 'agent-3', agentName: 'Old bot', projectId: PROJECT, version: null }, + ], + hiddenAgents: 0, + }); + + await expectError( + service.deleteSkill('skill_a'), + ConflictError, + 'Skill "Brand voice" is used by Support bot (draft), Sales bot (published v2), Old bot (older published version) and cannot be deleted.', + ); + expect(skills.deleteSkill).not.toHaveBeenCalled(); + }); + + it('names only the agents of the given projects and counts the others', async () => { + skills.findByIds.mockResolvedValue([skillRow('skill_a')]); + skills.findLatestSaved.mockResolvedValue( + new Map([['skill_a', versionRow(skillRow('skill_a'), 2)]]), + ); + skills.findUsage.mockResolvedValue({ + drafts: [{ agentId: 'agent-1', agentName: 'Support bot', projectId: PROJECT }], + pins: [], + hiddenAgents: 2, + }); + + await expectError( + service.deleteSkill('skill_a', [PROJECT]), + ConflictError, + 'Skill "Brand voice" is used by Support bot (draft), 2 agents in other projects and cannot be deleted.', + ); + expect(skills.findUsage).toHaveBeenCalledWith('skill_a', [PROJECT], TX_CTX); + expect(skills.deleteSkill).not.toHaveBeenCalled(); + }); + + it('refuses when only agents the caller cannot read use the skill', async () => { + skills.findByIds.mockResolvedValue([skillRow('skill_a')]); + skills.findLatestSaved.mockResolvedValue( + new Map([['skill_a', versionRow(skillRow('skill_a'), 2)]]), + ); + skills.findUsage.mockResolvedValue({ drafts: [], pins: [], hiddenAgents: 1 }); + + await expectError( + service.deleteSkill('skill_a', []), + ConflictError, + 'Skill "Brand voice" is used by 1 agent in other projects and cannot be deleted.', + ); + expect(skills.deleteSkill).not.toHaveBeenCalled(); + }); + + it('throws NotFoundError for a missing skill', async () => { + await expect(service.deleteSkill('skill_gone')).rejects.toThrow(NotFoundError); + }); + }); +}); 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 new file mode 100644 index 00000000000..af6b7e5b409 --- /dev/null +++ b/packages/cli/src/modules/agents/__tests__/skills-api.service.test.ts @@ -0,0 +1,484 @@ +/* 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 { ForbiddenError, NotFoundError, UserError } from '@n8n/errors'; +import { hasGlobalScope } from '@n8n/permissions'; +import { mock } from 'vitest-mock-extended'; + +import { userHasScopes } from '@/permissions.ee/check-access'; + +import type { SkillFile } from '../entities/skill-file.entity'; +import type { SkillVersion } from '../entities/skill-version.entity'; +import type { Skill } from '../entities/skill.entity'; +import type { ResolvedSkillRow, SkillRepository } from '../repositories/skill.repository'; +import { SkillsApiService } from '../skills/skills-api.service'; +import type { SkillService } from '../skills/skill.service'; +import { getAgentSkillHash } from '../utils/agent-config-hash'; + +vi.mock('@/permissions.ee/check-access', () => ({ userHasScopes: vi.fn() })); +vi.mock('@n8n/permissions', async (importOriginal) => ({ + ...(await importOriginal()), + hasGlobalScope: vi.fn(), +})); + +const user = mock({ id: 'user-1' }); +const DATE = new Date('2026-10-01T00:00:00.000Z'); + +function skill(id: string, target: Partial> = {}): Skill { + return mock({ + id, + projectId: target.projectId ?? null, + agentId: target.agentId ?? null, + source: 'ui', + createdAt: DATE, + updatedAt: DATE, + }); +} + +function row( + owner: Skill, + version: number, + fields: { name?: string; instructions?: string } = {}, +): ResolvedSkillRow { + const name = fields.name ?? NAMES[owner.id] ?? 'Skill'; + return { + skill: owner, + version: mock({ + id: `${owner.id}-v${version}`, + skillId: owner.id, + version, + name, + description: `${name} desc`, + instructions: fields.instructions ?? 'Do it.', + frontmatter: null, + contentHash: 'hash', + }), + files: [] as SkillFile[], + }; +} + +const NAMES: Record = { + skill_a: 'Brand voice', + skill_b: 'Cite sources', + skill_c: 'Triage rules', +}; + +const query = (overrides: Partial = {}) => + ({ skip: 0, ...overrides }) as ListSkillsQueryDto; + +describe('SkillsApiService', () => { + const repository = mock(); + const skillService = mock(); + const projectScopeService = mock(); + const service = new SkillsApiService(skillService, repository, projectScopeService); + let visible: Skill[]; + + beforeEach(() => { + vi.resetAllMocks(); + visible = Object.keys(NAMES).map((id) => skill(id)); + projectScopeService.getProjectIds.mockResolvedValue(['project-1']); + vi.mocked(hasGlobalScope).mockReturnValue(false); + repository.findVisiblePage.mockImplementation(async () => ({ + skills: visible, + count: visible.length, + })); + repository.findByIds.mockImplementation(async (ids) => + visible.filter((s) => ids.includes(s.id)), + ); + repository.findLatestSummaries.mockImplementation( + async (ids) => + new Map( + visible + .filter((s) => ids.includes(s.id)) + .map(({ id }) => { + const { name, description } = row(skill(id), 2).version; + return [id, { name, description, version: 2 }] as const; + }), + ), + ); + repository.findLatestSaved.mockImplementation( + async (ids) => + 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.findUsage.mockResolvedValue({ drafts: [], pins: [], hiddenAgents: 0 }); + repository.findAgents.mockImplementation( + async (ids) => + new Map( + ids + .filter((id) => id === 'agent-1') + .map((id) => [id, { name: 'Support bot', projectId: 'project-1' }] as const), + ), + ); + skillService.canAccess.mockResolvedValue(true); + skillService.accessCheck.mockResolvedValue(() => true); + }); + + describe('list', () => { + it('passes the filters and the page to the repository', async () => { + const result = await service.list( + user, + query({ scope: 'project', projectId: 'project-1', search: 'rules', skip: 10, take: 5 }), + ); + + expect(repository.findVisiblePage).toHaveBeenCalledWith( + { projectIds: ['project-1'] }, + { scope: 'project', projectId: 'project-1', search: 'rules', skip: 10, take: 5 }, + ); + expect(result.count).toBe(3); + }); + + it('passes the agent that attaches skills, with its project', async () => { + await service.list(user, query({ attachableTo: 'agent-1', search: 'voice' })); + + expect(repository.findVisiblePage).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + attachableTo: { agentId: 'agent-1', projectId: 'project-1' }, + search: 'voice', + }), + ); + }); + + it('refuses attachableTo for an unknown agent', async () => { + await expect(service.list(user, query({ attachableTo: 'agent-gone' }))).rejects.toThrow( + NotFoundError, + ); + expect(repository.findVisiblePage).not.toHaveBeenCalled(); + }); + + it.each([{ scope: 'agent' as const }, { projectId: 'project-1' }])( + 'refuses attachableTo together with %o', + async (filter) => { + await expect( + service.list(user, query({ attachableTo: 'agent-1', ...filter })), + ).rejects.toThrow(UserError); + expect(repository.findVisiblePage).not.toHaveBeenCalled(); + }, + ); + + it('shapes only the page into list items, from summaries', async () => { + visible = [skill('skill_b')]; + repository.findVisiblePage.mockResolvedValue({ skills: visible, count: 3 }); + + const result = await service.list(user, query({ skip: 1, take: 1 })); + + expect(result).toMatchObject({ count: 3, data: [{ id: 'skill_b', name: 'Cite sources' }] }); + // Summaries carry no instructions or files. + expect(repository.findLatestSummaries).toHaveBeenCalledWith(['skill_b']); + expect(repository.findLatestSaved).not.toHaveBeenCalled(); + }); + + it('asks for the projects the user may list skills of', async () => { + await service.list(user, query()); + + expect(projectScopeService.getProjectIds).toHaveBeenCalledWith(user, ['projectSkill:list']); + }); + + it('widens the list for a user with global scopes', async () => { + projectScopeService.getProjectIds.mockResolvedValue(null); + + await service.list(user, query()); + + expect(repository.findVisiblePage).toHaveBeenCalledWith( + { projectIds: 'all' }, + expect.anything(), + ); + }); + + 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']])); + skillService.accessCheck.mockImplementation(async (_user, op) => () => op === 'update'); + + const [item] = (await service.list(user, query())).data; + + expect(item).toEqual({ + id: 'skill_a', + name: 'Brand voice', + description: 'Brand voice desc', + scope: 'project', + projectId: 'project-1', + projectName: 'Team', + agentId: null, + agentName: null, + source: 'ui', + latestVersion: 2, + usedByAgents: 2, + canEdit: true, + canDelete: false, + createdAt: DATE.toISOString(), + updatedAt: DATE.toISOString(), + }); + }); + + 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']])); + const check = vi.fn().mockReturnValue(true); + skillService.accessCheck.mockResolvedValue(check); + + const [item] = (await service.list(user, query())).data; + + expect(item).toMatchObject({ + scope: 'agent', + projectId: 'project-1', + projectName: 'Team', + agentId: 'agent-1', + agentName: 'Support bot', + }); + // Permissions come from the agent's project. + expect(check).toHaveBeenCalledWith(visible[0], 'project-1'); + }); + + it('checks edit and delete once for the page, not once per skill', async () => { + await service.list(user, query()); + + expect(skillService.accessCheck).toHaveBeenCalledTimes(2); + expect(skillService.accessCheck).toHaveBeenCalledWith(user, 'update'); + expect(skillService.accessCheck).toHaveBeenCalledWith(user, 'delete'); + expect(skillService.canAccess).not.toHaveBeenCalled(); + }); + }); + + describe('get', () => { + it('returns the latest version, its hash and the usage', async () => { + const usage = { + drafts: [{ agentId: 'agent-1', agentName: 'Support', projectId: 'project-1' }], + pins: [], + hiddenAgents: 1, + }; + repository.findUsage.mockResolvedValue(usage); + const latest = row(visible[0], 3, { instructions: 'saved' }); + repository.findLatestSaved.mockResolvedValue(new Map([['skill_a', latest]])); + repository.findLatestSummaries.mockResolvedValue( + new Map([ + [ + 'skill_a', + { name: latest.version.name, description: latest.version.description, version: 3 }, + ], + ]), + ); + + const detail = await service.get(user, 'skill_a'); + + expect(detail.skill.instructions).toBe('saved'); + expect(detail.latestVersion).toBe(3); + expect(detail.skillHash).toBe(getAgentSkillHash(detail.skill)); + expect(detail.usedBy).toEqual(usage); + }); + + it('limits the usage to the agents the user may read', async () => { + await service.get(user, 'skill_a'); + + expect(projectScopeService.getProjectIds).toHaveBeenCalledWith(user, ['agent:read']); + expect(repository.findUsage).toHaveBeenCalledWith('skill_a', ['project-1']); + }); + + it('shows the agents of every project to a user with the global agent scope', async () => { + projectScopeService.getProjectIds.mockResolvedValue(null); + + await service.get(user, 'skill_a'); + + expect(repository.findUsage).toHaveBeenCalledWith('skill_a', 'all'); + }); + + it('answers a skill the user cannot see like a missing one', async () => { + skillService.canAccess.mockResolvedValue(false); + + await expect(service.get(user, 'skill_a')).rejects.toThrow(NotFoundError); + await expect(service.get(user, 'skill_gone')).rejects.toThrow(NotFoundError); + expect(skillService.canAccess).toHaveBeenCalledWith(user, visible[0], 'read'); + }); + }); + + describe('create', () => { + const body = { name: 'Tone', description: 'How we sound', instructions: 'Be kind.' }; + const payload = (overrides: Partial) => + ({ scope: 'instance', skill: body, ...overrides }) as CreateSkillDto; + + beforeEach(() => { + skillService.create.mockImplementation(async () => { + visible.push(skill('skill_new')); + return 'skill_new'; + }); + }); + + // A custom role can hold projectSkill:create without projectSkill:read. + it('returns the new skill to a creator who may not read it afterwards', async () => { + vi.mocked(userHasScopes).mockResolvedValue(true); + skillService.canAccess.mockResolvedValue(false); + skillService.accessCheck.mockResolvedValue(() => false); + + const detail = await service.create( + user, + payload({ scope: 'project', projectId: 'project-1' }), + ); + + expect(detail).toMatchObject({ id: 'skill_new', name: 'Skill', canEdit: false }); + }); + + it('creates an agent skill with projectSkill:create on the project of the agent', async () => { + vi.mocked(userHasScopes).mockResolvedValue(true); + + const detail = await service.create(user, payload({ scope: 'agent', agentId: 'agent-1' })); + + expect(userHasScopes).toHaveBeenCalledWith(user, ['projectSkill:create'], false, { + projectId: 'project-1', + }); + expect(skillService.create).toHaveBeenCalledWith({ + target: { projectId: null, agentId: 'agent-1' }, + skill: body, + source: 'ui', + createdById: user.id, + }); + expect(detail.id).toBe('skill_new'); + }); + + it('refuses an agent skill for an agent that does not exist', async () => { + vi.mocked(userHasScopes).mockResolvedValue(true); + + await expect( + service.create(user, payload({ scope: 'agent', agentId: 'agent-gone' })), + ).rejects.toThrow(NotFoundError); + expect(skillService.create).not.toHaveBeenCalled(); + }); + + it('creates a project skill with projectSkill:create, also in a personal project', async () => { + vi.mocked(userHasScopes).mockResolvedValue(true); + + await service.create(user, payload({ scope: 'project', projectId: 'personal-1' })); + + expect(userHasScopes).toHaveBeenCalledWith(user, ['projectSkill:create'], false, { + projectId: 'personal-1', + }); + expect(skillService.create).toHaveBeenCalledWith( + expect.objectContaining({ target: { projectId: 'personal-1', agentId: null } }), + ); + }); + + it('creates an instance skill with skill:create', async () => { + vi.mocked(hasGlobalScope).mockReturnValue(true); + + await service.create(user, payload({ scope: 'instance' })); + + expect(hasGlobalScope).toHaveBeenCalledWith(user, 'skill:create'); + expect(skillService.create).toHaveBeenCalledWith( + expect.objectContaining({ target: { projectId: null, agentId: null } }), + ); + }); + + it.each([ + ['project', {}, UserError], + ['project', { projectId: 'project-1', agentId: 'agent-1' }, UserError], + ['agent', {}, UserError], + ['agent', { agentId: 'agent-1', projectId: 'project-1' }, UserError], + ['agent', { agentId: 'agent-1' }, ForbiddenError], + ['instance', { projectId: 'project-1' }, UserError], + ['instance', { agentId: 'agent-1' }, UserError], + ['instance', {}, ForbiddenError], + ['project', { projectId: 'project-1' }, ForbiddenError], + ] as const)('refuses scope %s with %o', async (scope, extra, errorClass) => { + vi.mocked(userHasScopes).mockResolvedValue(false); + + await expect(service.create(user, payload({ scope, ...extra }))).rejects.toThrow(errorClass); + expect(skillService.create).not.toHaveBeenCalled(); + }); + + // 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()); + + await expect( + service.create(user, payload({ scope: 'project', projectId: 'project-gone' })), + ).rejects.toThrow(NotFoundError); + expect(repository.findProjectNames).toHaveBeenCalledWith(['project-gone']); + expect(skillService.create).not.toHaveBeenCalled(); + }); + }); + + describe('update', () => { + const update = (fields: Partial) => fields as SaveSkillDto; + const saved = { + versionId: 'v3', + version: 3, + created: true, + skill: { name: 'Brand voice', description: 'd', instructions: 'New.' }, + }; + + it('saves the update for a user who may edit the skill', async () => { + skillService.save.mockResolvedValue(saved); + + const result = await service.update( + user, + 'skill_a', + update({ instructions: 'New.', baseSkillHash: 'base' }), + ); + + expect(skillService.save).toHaveBeenCalledWith( + 'skill_a', + { instructions: 'New.', baseSkillHash: 'base' }, + user.id, + ); + expect(skillService.canAccess).toHaveBeenCalledWith(user, visible[0], 'update'); + expect(result).toEqual({ + id: 'skill_a', + ...saved, + skillHash: getAgentSkillHash(saved.skill), + }); + }); + + it('refuses a user who may not edit the skill', async () => { + skillService.canAccess.mockImplementation(async (_user, _skill, op) => op === 'read'); + + await expect( + service.update(user, 'skill_a', update({ instructions: 'New.' })), + ).rejects.toThrow(ForbiddenError); + expect(skillService.save).not.toHaveBeenCalled(); + }); + + it('answers a skill the user cannot see like a missing one', async () => { + skillService.canAccess.mockResolvedValue(false); + + await expect( + service.update(user, 'skill_a', update({ instructions: 'New.' })), + ).rejects.toThrow(NotFoundError); + }); + }); + + describe('delete', () => { + it('deletes for a user with the delete permission', async () => { + await service.delete(user, 'skill_a'); + + expect(skillService.canAccess).toHaveBeenCalledWith(user, visible[0], 'delete'); + expect(skillService.deleteSkill).toHaveBeenCalledWith('skill_a', ['project-1']); + }); + + it('names the agents of the projects the user may read in a conflict', async () => { + await service.delete(user, 'skill_a'); + + expect(projectScopeService.getProjectIds).toHaveBeenCalledWith(user, ['agent:read']); + }); + + it('names every agent for a user with the global agent scope', async () => { + projectScopeService.getProjectIds.mockResolvedValue(null); + + await service.delete(user, 'skill_a'); + + expect(skillService.deleteSkill).toHaveBeenCalledWith('skill_a', 'all'); + }); + + it('refuses a user without it', async () => { + skillService.canAccess.mockImplementation(async (_user, _skill, op) => op === 'read'); + + await expect(service.delete(user, 'skill_a')).rejects.toThrow(ForbiddenError); + expect(skillService.deleteSkill).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/packages/cli/src/modules/agents/__tests__/skills.controller.test.ts b/packages/cli/src/modules/agents/__tests__/skills.controller.test.ts new file mode 100644 index 00000000000..26dac7815e0 --- /dev/null +++ b/packages/cli/src/modules/agents/__tests__/skills.controller.test.ts @@ -0,0 +1,28 @@ +import { ControllerRegistryMetadata } from '@n8n/decorators'; +import { Container } from '@n8n/di'; + +import { SkillsController } from '../skills.controller'; + +/** + * The skills routes carry no scope decorator: `SkillsApiService` checks each skill row. + * 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']; + +describe('SkillsController routes', () => { + const metadata = Container.get(ControllerRegistryMetadata).getControllerMetadata( + SkillsController as never, + ); + + it('has only the routes that check access for each skill', () => { + expect([...metadata.routes.keys()].sort()).toEqual(ROW_CHECKED_HANDLERS); + }); + + it.each(ROW_CHECKED_HANDLERS)('%s needs a login and carries no route scope', (handlerName) => { + const route = metadata.routes.get(handlerName); + + expect(route?.accessScope).toBeUndefined(); + expect(route?.skipAuth).toBe(false); + expect(route?.allowUnauthenticated).toBe(false); + }); +}); diff --git a/packages/cli/src/modules/agents/agents.module.ts b/packages/cli/src/modules/agents/agents.module.ts index 891c3d31f72..d998a5b9f95 100644 --- a/packages/cli/src/modules/agents/agents.module.ts +++ b/packages/cli/src/modules/agents/agents.module.ts @@ -21,6 +21,7 @@ export class AgentsModule implements ModuleInterface { await import('./agents-settings.controller.js'); await import('./agents-config.controller.js'); await import('./agents-skills.controller.js'); + await import('./skills.controller.js'); await import('./agent-knowledge.controller.js'); await import('./agent-publish.controller.js'); await import('./agent-chat.controller.js'); diff --git a/packages/cli/src/modules/agents/repositories/agent.repository.ts b/packages/cli/src/modules/agents/repositories/agent.repository.ts index c0b19f1f8cc..2ac23989b4b 100644 --- a/packages/cli/src/modules/agents/repositories/agent.repository.ts +++ b/packages/cli/src/modules/agents/repositories/agent.repository.ts @@ -3,6 +3,7 @@ import type { AgentIntegrationConfig, ListAgentsQueryDto } from '@n8n/api-types' import { BaseRepository, chunkIds, TransactionRunner, type OperationContext } from '@n8n/db'; import { Service } from '@n8n/di'; import { DataSource, In, IsNull, Not, type SelectQueryBuilder } from '@n8n/typeorm'; +import { randomUUID } from 'node:crypto'; import type { QueryDeepPartialEntity } from '@n8n/typeorm/query-builder/QueryPartialEntity'; import { Agent } from '../entities/agent.entity'; @@ -644,6 +645,44 @@ export class AgentRepository extends BaseRepository { return won; } + /** + * A skill that these agents follow got a new saved version. Give each agent that is in + * sync with its published version a new draft version id, so it shows unpublished + * changes. One conditional UPDATE per agent and no read first, so a concurrent draft + * edit cannot make the skill save fail. `revision` does not move: a draft edit also + * moves `versionId` away from `activeVersionId`, so neither write undoes the other. + * Returns the agents it changed. + */ + async markDraftChangedIfInSync( + agentIds: string[], + ctx: OperationContext = {}, + ): Promise { + const changed: string[] = []; + for (const id of agentIds) { + const result = await this.managerFor(ctx) + .createQueryBuilder() + .update(Agent) + .set({ versionId: randomUUID() }) + .where('id = :id', { id }) + .andWhere('"versionId" = "activeVersionId"') + .execute(); + if ((result.affected ?? 0) > 0) changed.push(id); + } + return changed; + } + + /** The project of each agent, to send an update push to the right project. */ + async findProjectIdsByIds( + ids: string[], + ctx: OperationContext = {}, + ): Promise>> { + if (ids.length === 0) return []; + return await this.managerFor(ctx).find(Agent, { + where: { id: In(ids) }, + select: ['id', 'projectId'], + }); + } + /** * Write the `schema` column of a draft. Write `updatedAt` too, with the value * from the fenced save: without it, TypeORM sets `CURRENT_TIMESTAMP`. This diff --git a/packages/cli/src/modules/agents/repositories/skill.repository.ts b/packages/cli/src/modules/agents/repositories/skill.repository.ts new file mode 100644 index 00000000000..3c008c954ee --- /dev/null +++ b/packages/cli/src/modules/agents/repositories/skill.repository.ts @@ -0,0 +1,666 @@ +import { + BaseRepository, + LIKE_ESCAPE_CLAUSE, + Project, + TransactionRunner, + chunkIds, + escapeLike, + type OperationContext, +} from '@n8n/db'; +import { Service } from '@n8n/di'; +import { Brackets, DataSource, In, IsNull, type EntityManager } from '@n8n/typeorm'; +import type { SkillUsage } from '@n8n/api-types'; +import { UnexpectedError } from 'n8n-workflow'; +import { randomUUID } from 'node:crypto'; + +import { AgentVersionSkillDependency } from '../entities/agent-version-skill-dependency.entity'; +import { AgentHistory } from '../entities/agent-history.entity'; +import { AgentSkillDependency } from '../entities/agent-skill-dependency.entity'; +import { Agent } from '../entities/agent.entity'; +import { SkillFile } from '../entities/skill-file.entity'; +import { SkillVersion } from '../entities/skill-version.entity'; +import { Skill, type SkillSource } from '../entities/skill.entity'; +import { + compareSkillFilePaths, + skillContentHash, + type SkillContent, +} from '../skills/skill-content-hash'; + +/** + * Where a skill lives: a project, one agent, or the instance (both null). An agent skill + * has no projectId: its project is the agent's. + */ +export type SkillTarget = { projectId: string | null; agentId: string | null }; + +/** One skill with one of its version rows and that row's files, sorted by path. */ +export type ResolvedSkillRow = { + skill: Skill; + version: SkillVersion; + files: SkillFile[]; +}; + +/** A skill ref of an agent draft. A ref with `versionId` is pinned to that saved version. */ +export type SkillDependencyRef = { skillId: string; versionId?: string | null }; + +/** Which skills one user may list: instance skills always, plus the skills of these projects and of their agents. */ +export type SkillVisibility = { + projectIds: string[] | 'all'; +}; + +/** Narrows the visible skills of `findVisiblePage` and cuts one page. */ +export type SkillListFilter = { + scope?: 'project' | 'agent' | 'instance'; + /** The skills of this project and of its agents. */ + projectId?: string; + /** What this agent may attach: instance skills, its project's skills and its own. */ + attachableTo?: { agentId: string; projectId: string }; + search?: string; + skip: number; + take?: number; +}; + +export type { SkillUsage } from '@n8n/api-types'; + +/** Latest version of a skill without its content. */ +export type SkillSummary = { name: string; description: string; version: number }; + +/** Persistence for skills, their versions and files, and the agent rows that use them. */ +@Service() +export class SkillRepository extends BaseRepository { + constructor(dataSource: DataSource, transactionRunner: TransactionRunner) { + super(Skill, dataSource.manager, transactionRunner); + } + + /** + * One page of the skills the user may see, newest change first, and the count of all + * matches. The filters, the search and the page cut run in the query, so the read grows + * with the page, not with the skills on the instance. The search matches the name or + * the description of the latest version. Without `take` every match is returned. + */ + async findVisiblePage( + visibility: SkillVisibility, + filter: SkillListFilter, + ctx: OperationContext = {}, + ): Promise<{ skills: Skill[]; count: number }> { + const manager = this.managerFor(ctx); + // An agent skill has no projectId: its project comes from its agent. + const agentsOf = (projectCondition: string) => + manager + .createQueryBuilder() + .subQuery() + .select('owner.id') + .from(Agent, 'owner') + .where(projectCondition) + .getQuery(); + const { projectIds } = visibility; + const branches: string[] = []; + if (!filter.scope || filter.scope === 'instance') { + branches.push('skill.projectId IS NULL AND skill.agentId IS NULL'); + } + if (filter.attachableTo !== undefined) { + if (projectIds === 'all' || projectIds.includes(filter.attachableTo.projectId)) { + branches.push('skill.projectId = :attachProjectId', 'skill.agentId = :attachAgentId'); + } + } else { + if (!filter.scope || filter.scope === 'project') { + if (projectIds === 'all') branches.push('skill.projectId IS NOT NULL'); + else if (projectIds.length > 0) branches.push('skill.projectId IN (:...projectIds)'); + } + if (!filter.scope || filter.scope === 'agent') { + if (projectIds === 'all') branches.push('skill.agentId IS NOT NULL'); + else if (projectIds.length > 0) { + branches.push(`skill.agentId IN ${agentsOf('owner.projectId IN (:...projectIds)')}`); + } + } + } + if (branches.length === 0) return { skills: [], count: 0 }; + + const query = manager + .createQueryBuilder(Skill, 'skill') + .where(new Brackets((qb) => branches.forEach((branch) => qb.orWhere(`(${branch})`)))) + .setParameters({ + projectIds, + ...(filter.attachableTo !== undefined && { + attachProjectId: filter.attachableTo.projectId, + attachAgentId: filter.attachableTo.agentId, + }), + }); + if (filter.projectId) { + query.andWhere( + new Brackets((qb) => + qb + .where('skill.projectId = :projectId') + .orWhere(`skill.agentId IN ${agentsOf('owner.projectId = :projectId')}`), + ), + { projectId: filter.projectId }, + ); + } + if (filter.search) { + query + .innerJoin(SkillVersion, 'latest', 'latest.skillId = skill.id') + .andWhere((qb) => { + const highest = qb + .subQuery() + .select('MAX(later.version)') + .from(SkillVersion, 'later') + .where('later.skillId = skill.id') + .getQuery(); + return `latest.version = ${highest}`; + }) + .andWhere( + new Brackets((qb) => + qb + .where(`LOWER(latest.name) LIKE :search ${LIKE_ESCAPE_CLAUSE}`) + .orWhere(`LOWER(latest.description) LIKE :search ${LIKE_ESCAPE_CLAUSE}`), + ), + { search: `%${escapeLike(filter.search.toLowerCase())}%` }, + ); + } + query.orderBy('skill.updatedAt', 'DESC').addOrderBy('skill.id', 'ASC').offset(filter.skip); + if (filter.take !== undefined) query.limit(filter.take); + const [skills, count] = await query.getManyAndCount(); + return { skills, count }; + } + + async findByIds(ids: string[], ctx: OperationContext = {}): Promise { + const manager = this.managerFor(ctx); + return await inChunks( + ids, + async (chunk) => await manager.find(Skill, { where: { id: In(chunk) } }), + ); + } + + /** The highest version of each skill, keyed by skill id. A following ref reads it. */ + async findLatestSaved( + skillIds: string[], + ctx: OperationContext = {}, + ): Promise> { + const manager = this.managerFor(ctx); + const versions = await inChunks( + skillIds, + async (chunk) => await latestVersionsQuery(manager, chunk).getMany(), + ); + return await this.resolveBySkill(versions, ctx); + } + + /** Name, description and number of the latest version, without its content. */ + async findLatestSummaries( + skillIds: string[], + ctx: OperationContext = {}, + ): Promise> { + const manager = this.managerFor(ctx); + const versions = await inChunks( + skillIds, + async (chunk) => + await latestVersionsQuery(manager, chunk) + .select(['version.skillId', 'version.name', 'version.description', 'version.version']) + .getMany(), + ); + return new Map( + versions.map((v) => [ + v.skillId, + { name: v.name, description: v.description, version: v.version }, + ]), + ); + } + + /** Version rows by id, keyed by version id. A pinned ref reads them. */ + async findVersionsByIds( + versionIds: string[], + ctx: OperationContext = {}, + ): Promise> { + if (versionIds.length === 0) return new Map(); + const versions = await this.managerFor(ctx).find(SkillVersion, { + where: { id: In(unique(versionIds)) }, + }); + const rows = await this.resolve(versions, ctx); + return new Map(rows.map((row) => [row.version.id, row])); + } + + /** The pinned version of each skill of one published agent version, keyed by skill id. */ + async findPinned( + agentVersionId: string, + ctx: OperationContext = {}, + ): Promise> { + const pins = await this.managerFor(ctx).find(AgentVersionSkillDependency, { + where: { agentVersionId }, + }); + const versions = await this.findVersionsByIds( + pins.map((pin) => pin.skillVersionId), + ctx, + ); + const result = new Map(); + for (const pin of pins) { + const row = versions.get(pin.skillVersionId); + if (row) result.set(pin.skillId, row); + } + return result; + } + + /** The next version number of a skill: the highest version plus one, or 1. */ + async nextVersionNumber(skillId: string, ctx: OperationContext = {}): Promise { + const latest = await this.managerFor(ctx).findOne(SkillVersion, { + select: ['version'], + where: { skillId }, + order: { version: 'DESC' }, + }); + return (latest?.version ?? 0) + 1; + } + + /** Inserts the skill with its v1. */ + async createSkill( + skill: { id: string; target: SkillTarget; source: SkillSource; createdById: string | null }, + content: SkillContent, + ctx: OperationContext = {}, + ): Promise { + await this.runInTransaction(ctx, async (manager) => { + await manager.insert(Skill, { + id: skill.id, + projectId: skill.target.projectId, + agentId: skill.target.agentId, + source: skill.source, + createdById: skill.createdById, + }); + await insertVersion(manager, skill.id, 1, content, skill.createdById); + }); + } + + /** Inserts the next version with its files and returns its id. The skill moves up the list. */ + async insertSavedVersion( + skillId: string, + version: number, + content: SkillContent, + createdById: string | null, + ctx: OperationContext = {}, + ): Promise { + return await this.runInTransaction(ctx, async (manager) => { + const id = await insertVersion(manager, skillId, version, content, createdById); + await manager.update(Skill, { id: skillId }, { updatedAt: new Date() }); + return id; + }); + } + + async insertPins( + pins: Array<{ agentVersionId: string; skillId: string; skillVersionId: string }>, + ctx: OperationContext = {}, + ): Promise { + if (pins.length === 0) return; + await this.managerFor(ctx).insert(AgentVersionSkillDependency, pins); + } + + /** + * Sets the dependency rows of one agent to these refs. Refs to unknown skills are + * skipped. A row that stays keeps its `createdAt`, which orders an agent's skills, and + * new rows sort after it in the given order. When one skill has a following ref and a + * pinned ref, the following ref wins, because the draft then runs the latest version. + */ + async replaceDependencies( + agentId: string, + refs: SkillDependencyRef[], + ctx: OperationContext = {}, + ): Promise { + const pinBySkill = new Map(); + for (const ref of refs) { + if (pinBySkill.get(ref.skillId) === null) continue; + pinBySkill.set(ref.skillId, ref.versionId ?? null); + } + await this.runInTransaction(ctx, async (manager, txCtx) => { + const known = new Set( + (await this.findByIds([...pinBySkill.keys()], txCtx)).map((skill) => skill.id), + ); + const current = await manager.find(AgentSkillDependency, { where: { agentId } }); + const removed = current.filter( + (row) => !known.has(row.skillId) || !pinBySkill.has(row.skillId), + ); + if (removed.length > 0) { + await manager.delete(AgentSkillDependency, { + agentId, + skillId: In(removed.map((row) => row.skillId)), + }); + } + const kept = new Map(current.map((row) => [row.skillId, row])); + for (const [skillId, skillVersionId] of pinBySkill) { + const row = kept.get(skillId); + if (row && row.skillVersionId !== skillVersionId) { + await manager.update(AgentSkillDependency, { agentId, skillId }, { skillVersionId }); + } + } + const added = [...pinBySkill].filter(([skillId]) => known.has(skillId) && !kept.has(skillId)); + if (added.length === 0) return; + // One insert gives every row the same timestamp, so each new row gets its own. + const newest = Math.max(Date.now(), ...current.map((row) => row.createdAt.getTime() + 1)); + await manager.insert( + AgentSkillDependency, + added.map(([skillId, skillVersionId], index) => ({ + agentId, + skillId, + skillVersionId, + createdAt: new Date(newest + index), + })), + ); + }); + } + + /** Agents whose draft follows the skill (no pin). */ + async findFollowingAgents( + skillId: string, + ctx: OperationContext = {}, + ): Promise>> { + const agentIds = await this.findFollowingAgentIds([skillId], ctx); + if (agentIds.length === 0) return []; + return await this.managerFor(ctx).find(Agent, { + where: { id: In(agentIds) }, + select: ['id', 'name'], + }); + } + + /** The skill refs of each agent's draft, in the order they were attached. */ + async findDependencies( + agentIds: string[], + ctx: OperationContext = {}, + ): Promise>> { + if (agentIds.length === 0) return new Map(); + const rows = await this.managerFor(ctx).find(AgentSkillDependency, { + where: { agentId: In(unique(agentIds)) }, + order: { createdAt: 'ASC' }, + }); + const result = new Map>(); + for (const row of rows) { + const refs = result.get(row.agentId) ?? []; + refs.push({ skillId: row.skillId, versionId: row.skillVersionId }); + result.set(row.agentId, refs); + } + return result; + } + + /** Ids of the agents whose draft follows any of these skills (no pin). */ + async findFollowingAgentIds(skillIds: string[], ctx: OperationContext = {}): Promise { + if (skillIds.length === 0) return []; + const rows = await this.managerFor(ctx).find(AgentSkillDependency, { + where: { skillId: In(unique(skillIds)), skillVersionId: IsNull() }, + }); + return unique(rows.map((row) => row.agentId)); + } + + /** + * "Used by": each agent that uses the skill once, as a draft ref, as a published pin, or + * both. A pin row shows the version the agent's current published version runs, or + * null when only older published versions use the skill. Agents outside `projectIds` + * are only counted. The read grows with the agents, not with their publishes. + */ + async findUsage( + skillId: string, + projectIds: string[] | 'all' = 'all', + ctx: OperationContext = {}, + ): Promise { + const manager = this.managerFor(ctx); + const drafted = await manager + .createQueryBuilder(Agent, 'agent') + .innerJoin(AgentSkillDependency, 'dep', 'dep.agentId = agent.id') + .where('dep.skillId = :skillId', { skillId }) + .select(['agent.id', 'agent.name', 'agent.projectId']) + .getMany(); + const published = await manager + .createQueryBuilder(Agent, 'agent') + .where((qb) => { + const pinned = qb + .subQuery() + .select('history.agentId') + .from(AgentHistory, 'history') + .innerJoin(AgentVersionSkillDependency, 'pin', 'pin.agentVersionId = history.versionId') + .where('pin.skillId = :skillId') + .getQuery(); + return `agent.id IN ${pinned}`; + }) + .setParameter('skillId', skillId) + .select(['agent.id', 'agent.name', 'agent.projectId', 'agent.activeVersionId']) + .getMany(); + + const isVisible = (agent: Pick) => + projectIds === 'all' || projectIds.includes(agent.projectId); + const hidden = new Set( + [...drafted, ...published].filter((agent) => !isVisible(agent)).map((agent) => agent.id), + ); + const visiblePublished = published.filter(isVisible); + const activeVersions = await this.findActivePinVersions( + skillId, + visiblePublished.flatMap((agent) => (agent.activeVersionId ? [agent.activeVersionId] : [])), + manager, + ); + return { + drafts: drafted.filter(isVisible).map((agent) => ({ + agentId: agent.id, + agentName: agent.name, + projectId: agent.projectId, + })), + pins: visiblePublished.map((agent) => ({ + agentId: agent.id, + agentName: agent.name, + projectId: agent.projectId, + version: activeVersions.get(agent.activeVersionId ?? '') ?? null, + })), + hiddenAgents: hidden.size, + }; + } + + /** Distinct agents per skill, drafts and pins together. Skills nobody uses are left out. */ + async countUsingAgents( + skillIds: string[], + ctx: OperationContext = {}, + ): Promise> { + const manager = this.managerFor(ctx); + const pairs = await inChunks(skillIds, async (chunk) => { + const drafts = await manager.find(AgentSkillDependency, { + select: ['skillId', 'agentId'], + where: { skillId: In(chunk) }, + }); + const pins = await manager + .createQueryBuilder(AgentVersionSkillDependency, 'pin') + .innerJoin(AgentHistory, 'history', 'history.versionId = pin.agentVersionId') + .where('pin.skillId IN (:...chunk)', { chunk }) + .select('pin.skillId', 'skillId') + .addSelect('history.agentId', 'agentId') + .distinct(true) + .getRawMany<{ skillId: string; agentId: string }>(); + return [...drafts, ...pins]; + }); + const agentsBySkill = new Map>(); + for (const { skillId, agentId } of pairs) { + agentsBySkill.set(skillId, (agentsBySkill.get(skillId) ?? new Set()).add(agentId)); + } + return new Map([...agentsBySkill].map(([skillId, agents]) => [skillId, agents.size])); + } + + async findProjectNames( + projectIds: string[], + ctx: OperationContext = {}, + ): Promise> { + const manager = this.managerFor(ctx); + const projects = await inChunks( + projectIds, + async (chunk) => + await manager.find(Project, { where: { id: In(chunk) }, select: ['id', 'name'] }), + ); + 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. */ + async findAgents( + agentIds: string[], + ctx: OperationContext = {}, + ): Promise>> { + const manager = this.managerFor(ctx); + const agents = await inChunks( + agentIds, + async (chunk) => + await manager.find(Agent, { + where: { id: In(chunk) }, + select: ['id', 'name', 'projectId'], + }), + ); + return new Map(agents.map(({ id, name, projectId }) => [id, { name, projectId }])); + } + + /** + * Deletes the skill. Its versions, files, dependency rows and pins go by cascade, + * so callers check usage first. + */ + async deleteSkill(skillId: string, ctx: OperationContext = {}): Promise { + await this.managerFor(ctx).delete(Skill, { id: skillId }); + } + + /** + * Holds the skill rows for writing until the transaction ends, so a save and a publish + * that pins the same skill run one after the other. See `lockForPublish`. + */ + async lockForEdit(skillIds: string[], ctx: OperationContext = {}): Promise { + await this.lock(skillIds, 'pessimistic_write', ctx); + } + + /** Shared lock: publishes run side by side, a save waits for them. */ + async lockForPublish(skillIds: string[], ctx: OperationContext = {}): Promise { + await this.lock(skillIds, 'pessimistic_read', ctx); + } + + private async lock( + skillIds: string[], + mode: 'pessimistic_write' | 'pessimistic_read', + ctx: OperationContext, + ): Promise { + if (!ctx.trx) throw new UnexpectedError('Skill row locks need a transaction'); + // SQLite allows one write transaction at a time, so the rows cannot change under us. + if (skillIds.length === 0 || this.manager.connection.options.type !== 'postgres') return; + await this.managerFor(ctx).find(Skill, { + select: ['id'], + where: { id: In(unique(skillIds)) }, + lock: { mode }, + }); + } + + /** The skill version number each of these published agent versions pins. */ + private async findActivePinVersions( + skillId: string, + agentVersionIds: string[], + manager: EntityManager, + ): Promise> { + const rows = await inChunks( + agentVersionIds, + async (chunk) => + await manager + .createQueryBuilder(AgentVersionSkillDependency, 'pin') + .innerJoin(SkillVersion, 'version', 'version.id = pin.skillVersionId') + .where('pin.skillId = :skillId', { skillId }) + .andWhere('pin.agentVersionId IN (:...chunk)', { chunk }) + .select('pin.agentVersionId', 'agentVersionId') + .addSelect('version.version', 'version') + .getRawMany<{ agentVersionId: string; version: number | string }>(), + ); + return new Map(rows.map((row) => [row.agentVersionId, Number(row.version)])); + } + + private async resolveBySkill( + versions: SkillVersion[], + ctx: OperationContext, + ): Promise> { + const rows = await this.resolve(versions, ctx); + return new Map(rows.map((row) => [row.skill.id, row])); + } + + /** Adds the skill and the files, sorted by path, to each version row. */ + private async resolve( + versions: SkillVersion[], + ctx: OperationContext, + ): Promise { + if (versions.length === 0) return []; + const manager = this.managerFor(ctx); + const skills = await this.findByIds( + versions.map((version) => version.skillId), + ctx, + ); + const files = await inChunks( + versions.map((version) => version.id), + async (chunk) => await manager.find(SkillFile, { where: { skillVersionId: In(chunk) } }), + ); + const skillById = new Map(skills.map((skill) => [skill.id, skill])); + const filesByVersion = new Map(); + for (const file of files.sort((a, b) => compareSkillFilePaths(a.path, b.path))) { + filesByVersion.set(file.skillVersionId, [ + ...(filesByVersion.get(file.skillVersionId) ?? []), + file, + ]); + } + return versions.flatMap((version) => { + const skill = skillById.get(version.skillId); + return skill ? [{ skill, version, files: filesByVersion.get(version.id) ?? [] }] : []; + }); + } +} + +function unique(values: string[]): string[] { + return [...new Set(values)]; +} + +/** Runs `read` over the distinct ids in chunks that stay under the bind limit. */ +async function inChunks(ids: string[], read: (chunk: string[]) => Promise): Promise { + const results: T[] = []; + for (const chunk of chunkIds(unique(ids))) results.push(...(await read(chunk))); + return results; +} + +/** 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}`; + }); +} + +async function insertVersion( + manager: EntityManager, + skillId: string, + version: number, + content: SkillContent, + createdById: string | null, +): Promise { + const id = randomUUID(); + // `save`, not `insert`: the partial-entity type of `insert` rejects free-form JSON. + await manager.save( + manager.create(SkillVersion, { + id, + skillId, + version, + name: content.name, + description: content.description, + instructions: content.instructions, + frontmatter: content.frontmatter, + contentHash: skillContentHash(content), + createdById, + }), + ); + await insertFiles(manager, id, content.files); + return id; +} + +async function insertFiles( + manager: EntityManager, + skillVersionId: string, + files: SkillContent['files'], +): Promise { + if (files.length === 0) return; + await manager.insert( + SkillFile, + files.map((file) => ({ + skillVersionId, + path: file.path, + content: file.content, + })), + ); +} diff --git a/packages/cli/src/modules/agents/skills.controller.ts b/packages/cli/src/modules/agents/skills.controller.ts new file mode 100644 index 00000000000..3a9e5b48ac6 --- /dev/null +++ b/packages/cli/src/modules/agents/skills.controller.ts @@ -0,0 +1,47 @@ +import { CreateSkillDto, ListSkillsQueryDto, SaveSkillDto } from '@n8n/api-types'; +import type { AuthenticatedRequest } from '@n8n/db'; +import { Body, Delete, Get, Param, Patch, Post, Query, RestController } from '@n8n/decorators'; +import type { Response } from 'express'; + +import { SkillsApiService } from './skills/skills-api.service'; + +/** + * Skills for the settings page. No route scope: the scope of each skill decides who may + * see or change it, so `SkillsApiService` checks every row. + */ +@RestController('/skills') +export class SkillsController { + constructor(private readonly skillsApi: SkillsApiService) {} + + @Get('/') + async list(req: AuthenticatedRequest, _res: Response, @Query query: ListSkillsQueryDto) { + return await this.skillsApi.list(req.user, query); + } + + @Get('/:skillId') + async get(req: AuthenticatedRequest, _res: Response, @Param('skillId') skillId: string) { + return await this.skillsApi.get(req.user, skillId); + } + + @Post('/') + async create(req: AuthenticatedRequest, _res: Response, @Body payload: CreateSkillDto) { + return await this.skillsApi.create(req.user, payload); + } + + /** Saves the changes as the next version. There is no draft: the editor keeps them until here. */ + @Patch('/:skillId') + async update( + req: AuthenticatedRequest, + _res: Response, + @Param('skillId') skillId: string, + @Body payload: SaveSkillDto, + ) { + return await this.skillsApi.update(req.user, skillId, payload); + } + + @Delete('/:skillId') + async delete(req: AuthenticatedRequest, _res: Response, @Param('skillId') skillId: string) { + await this.skillsApi.delete(req.user, skillId); + return { ok: true }; + } +} diff --git a/packages/cli/src/modules/agents/skills/skill-content-hash.ts b/packages/cli/src/modules/agents/skills/skill-content-hash.ts new file mode 100644 index 00000000000..cf6cb5c335d --- /dev/null +++ b/packages/cli/src/modules/agents/skills/skill-content-hash.ts @@ -0,0 +1,55 @@ +import { createHash } from 'node:crypto'; + +import type { SkillFrontmatter } from '../entities/skill-version.entity'; + +/** Everything one `skill_version` row holds, the name included. */ +export type SkillContent = { + name: string; + description: string; + instructions: string; + frontmatter: SkillFrontmatter | null; + files: Array<{ path: string; content: string }>; +}; + +/** Plain code-unit order, the same order every reader of `skill_file` uses. */ +export function compareSkillFilePaths(left: string, right: string): number { + return left < right ? -1 : left > right ? 1 : 0; +} + +/** + * Map keys in sorted order at every level, so two equal maps hash the same. List order + * stays. For a flat map of strings, the only shape the data migration writes, this is + * the same as its top-level sort. + */ +function sortKeys(value: unknown): unknown { + if (Array.isArray(value)) return value.map(sortKeys); + if (value === null || typeof value !== 'object') return value; + return Object.fromEntries( + Object.entries(value) + .sort(([left], [right]) => (left < right ? -1 : left > right ? 1 : 0)) + .map(([key, entry]) => [key, sortKeys(entry)]), + ); +} + +/** + * Identity of a version's content, name included. Save creates no version when the + * draft has the hash of the latest version. + * + * The data migration `MigrateAgentSkillsToHub` (CONTEXT-225) must compute the same value. + * Keep both in sync: the migration test must pin the literals of `skill-content-hash.test.ts`. + */ +export function skillContentHash(content: SkillContent): string { + return createHash('sha256') + .update( + JSON.stringify({ + name: content.name, + description: content.description, + instructions: content.instructions, + frontmatter: content.frontmatter ? sortKeys(content.frontmatter) : null, + files: content.files + .map((file) => [file.path, file.content]) + .sort(([left], [right]) => compareSkillFilePaths(left, right)), + }), + ) + .digest('hex'); +} diff --git a/packages/cli/src/modules/agents/skills/skill-names.ts b/packages/cli/src/modules/agents/skills/skill-names.ts new file mode 100644 index 00000000000..5a569890cbd --- /dev/null +++ b/packages/cli/src/modules/agents/skills/skill-names.ts @@ -0,0 +1,47 @@ +/** + * The rule for two skill names that are hard to tell apart inside one agent: case, + * surrounding spaces, and runs of spaces, hyphens and underscores do not count. + */ +export function skillNameKey(name: string): string { + return name + .trim() + .toLowerCase() + .replace(/[\s_-]+/g, ' ') + .trim(); +} + +/** + * Names in `added` that clash with a name in `existing` or with an earlier added name. + * Clashes among `existing` alone are allowed: agents keep the names they already had. + */ +export function findIntroducedNameClashes(existing: string[], added: string[]): string[] { + const taken = new Set(existing.map(skillNameKey)); + const clashes: string[] = []; + for (const name of added) { + const key = skillNameKey(name); + if (taken.has(key)) clashes.push(name); + taken.add(key); + } + return clashes; +} + +/** Whether renaming one skill of an agent makes it clash with the agent's other skills. */ +export function renameIntroducesClash( + otherNames: string[], + oldName: string, + newName: string, +): boolean { + const newKey = skillNameKey(newName); + if (newKey === skillNameKey(oldName)) return false; + return otherNames.some((name) => skillNameKey(name) === newKey); +} + +/** Groups of names that clash with each other, as written. */ +export function findNameClashGroups(names: string[]): string[][] { + const byKey = new Map(); + for (const name of names) { + const key = skillNameKey(name); + byKey.set(key, [...(byKey.get(key) ?? []), name]); + } + return [...byKey.values()].filter((group) => group.length > 1); +} diff --git a/packages/cli/src/modules/agents/skills/skill.service.ts b/packages/cli/src/modules/agents/skills/skill.service.ts new file mode 100644 index 00000000000..f9ca051c611 --- /dev/null +++ b/packages/cli/src/modules/agents/skills/skill.service.ts @@ -0,0 +1,502 @@ +import 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'; +import { Container, Service } from '@n8n/di'; +import { ConflictError, ForbiddenError, NotFoundError, UserError } from '@n8n/errors'; +import { hasGlobalScope } from '@n8n/permissions'; +import { generateNanoId } from '@n8n/utils/generate-nano-id'; + +import { userHasScopes } from '@/permissions.ee/check-access'; + +import { AgentUpdateBroadcaster } from '../agent-update-broadcaster'; +import type { SkillFrontmatter } from '../entities/skill-version.entity'; +import type { Skill, SkillSource } from '../entities/skill.entity'; +import { AgentRepository } from '../repositories/agent.repository'; +import { + SkillRepository, + type ResolvedSkillRow, + type SkillTarget, +} from '../repositories/skill.repository'; +import { getAgentSkillHash } from '../utils/agent-config-hash'; +import { skillContentHash, type SkillContent } from './skill-content-hash'; +import { findIntroducedNameClashes, renameIntroducesClash } from './skill-names'; + +/** A skill ref in an agent config. A ref with `versionId` runs that version. */ +export type SkillRef = { id: string; versionId?: string }; + +/** Fields to change, plus the hash of the version the editor started from. */ +export type SkillUpdate = Partial & { baseSkillHash: string }; + +export type SkillSaveResult = { + versionId: string; + version: number; + /** False when the content matched the latest version and nothing was saved. */ + created: boolean; + /** The content of the latest version after the save. */ + skill: AgentSkill; +}; + +/** + * The content of one version. Skills ignore `allowedTools`: the frontmatter, the + * `allowed-tools` key included, stays as given. + */ +export function toSkillContent(skill: AgentSkill, frontmatter: SkillFrontmatter | null = null) { + const content: SkillContent = { + name: skill.name, + description: skill.description, + instructions: skill.instructions, + frontmatter: frontmatter && Object.keys(frontmatter).length > 0 ? { ...frontmatter } : null, + files: (skill.references ?? []).map(({ path, content: text }) => ({ path, content: text })), + }; + return content; +} + +/** The `AgentSkill` shape the runtime and the API use, from one version row. */ +export function toAgentSkill(row: ResolvedSkillRow): AgentSkill { + return { + name: row.version.name, + description: row.version.description, + instructions: row.version.instructions, + ...(row.files.length + ? { references: row.files.map(({ path, content }) => ({ path, content })) } + : {}), + }; +} + +function assertHasInstructions(skill: AgentSkill): void { + if (!skill.instructions.trim()) throw new UserError('Skill instructions are required.'); +} + +function toRecord(rows: Map): Record { + return Object.fromEntries([...rows].map(([refId, row]) => [refId, toAgentSkill(row)])); +} + +/** + * The rules of skills: which version an agent runs, save, publish pins, revert pins, + * attach checks, edit permission and the delete guard. The runtime loads skills by id + * with no permission check. + */ +@Service() +export class SkillService { + constructor( + private readonly logger: Logger, + private readonly skillRepository: SkillRepository, + private readonly agentRepository: AgentRepository, + private readonly txRunner: TransactionRunner, + private readonly agentUpdateBroadcaster: AgentUpdateBroadcaster, + private readonly projectScopeService: ProjectScopeService, + ) {} + + // ---------------------------------------------------------------- reads + + /** + * What an agent draft runs, keyed by ref id: the latest version, or the pinned version + * of a ref with `versionId`. A pin to a version of another skill falls back to the latest. + */ + async resolveForAgentDraft( + refs: SkillRef[], + ctx: OperationContext = {}, + ): Promise> { + return toRecord(await this.resolveRows(refs, ctx)); + } + + /** The pinned versions of one published agent version, keyed by skill id. */ + async resolvePinned( + agentVersionId: string, + ctx: OperationContext = {}, + ): Promise> { + return toRecord(await this.skillRepository.findPinned(agentVersionId, ctx)); + } + + private async resolveRows( + refs: SkillRef[], + ctx: OperationContext, + ): Promise> { + const latest = await this.skillRepository.findLatestSaved( + refs.map((ref) => ref.id), + ctx, + ); + const pinnedIds = refs.flatMap((ref) => (ref.versionId ? [ref.versionId] : [])); + const pinned = await this.skillRepository.findVersionsByIds(pinnedIds, ctx); + const rows = new Map(); + for (const ref of refs) { + const pin = ref.versionId ? pinned.get(ref.versionId) : undefined; + const row = pin?.skill.id === ref.id ? pin : latest.get(ref.id); + if (row) rows.set(ref.id, row); + } + return rows; + } + + /** The names agents see: those of the latest versions. */ + private async latestNames(skillIds: string[], ctx: OperationContext) { + const rows = await this.skillRepository.findLatestSaved(skillIds, ctx); + return new Map([...rows].map(([id, row]) => [id, row.version.name])); + } + + // ---------------------------------------------------------------- writes + + /** Creates a skill with its v1. Returns the new id. */ + async create( + input: { target: SkillTarget; skill: AgentSkill; source: SkillSource; createdById: string }, + ctx: OperationContext = {}, + ): Promise { + assertHasInstructions(input.skill); + const id = `skill_${generateNanoId()}`; + await this.skillRepository.createSkill( + { id, target: input.target, source: input.source, createdById: input.createdById }, + toSkillContent(input.skill), + ctx, + ); + this.logger.debug('Created skill', { skillId: id }); + return id; + } + + /** + * Save: merges the update onto the latest version and stores the result as the next + * version, unless the content did not change. Every agent that follows the skill then + * runs the new version, and each one that was in sync with its published version + * shows unpublished changes. `baseSkillHash` is checked under the edit lock, so two + * saves from the same base cannot both pass. + */ + async save( + skillId: string, + update: SkillUpdate, + userId: string, + ctx: OperationContext = {}, + ): Promise { + const { saved, following } = await this.txRunner.run(ctx, async (txCtx) => { + await this.skillRepository.lockForEdit([skillId], txCtx); + const latest = (await this.skillRepository.findLatestSaved([skillId], txCtx)).get(skillId); + if (!latest) throw new NotFoundError('Skill not found'); + const current = toAgentSkill(latest); + const { baseSkillHash, allowedTools: _ignored, ...changes } = update; + if (baseSkillHash !== getAgentSkillHash(current)) { + throw new ConflictError( + 'The skill was changed somewhere else. Reload to get the latest version.', + ); + } + const skill: AgentSkill = { ...current, ...changes }; + if (!skill.references?.length) delete skill.references; + assertHasInstructions(skill); + const content = toSkillContent(skill, latest.version.frontmatter); + if (skillContentHash(content) === latest.version.contentHash) { + const unchanged = { versionId: latest.version.id, version: latest.version.version }; + return { saved: { ...unchanged, created: false, skill: current }, following: [] }; + } + if (skill.name !== latest.version.name) { + await this.assertRenameKeepsNamesApart(skillId, latest.version.name, skill.name, txCtx); + } + const version = await this.skillRepository.nextVersionNumber(skillId, txCtx); + const versionId = await this.skillRepository.insertSavedVersion( + skillId, + version, + content, + userId, + txCtx, + ); + const followingIds = await this.skillRepository.findFollowingAgentIds([skillId], txCtx); + await this.agentRepository.markDraftChangedIfInSync(followingIds, txCtx); + return { saved: { versionId, version, created: true, skill }, following: followingIds }; + }); + if (following.length > 0) await this.refreshAgents(following); + this.logger.debug('Saved skill', { + skillId, + version: saved.version, + created: saved.created, + following, + }); + return saved; + } + + /** After a save: following agents drop their cached runtime and their editors reload. */ + private async refreshAgents(agentIds: string[]): Promise { + // Imported on use: the runtime cache depends on services that will depend on this one. + const { AgentRuntimeCacheService } = await import('../agent-runtime-cache.service.js'); + const cache = Container.get(AgentRuntimeCacheService); + for (const agentId of agentIds) cache.clearRuntimes(agentId); + for (const agent of await this.agentRepository.findProjectIdsByIds(agentIds)) { + this.agentUpdateBroadcaster.notify({ + projectId: agent.projectId, + agentId: agent.id, + source: 'user', + }); + } + } + + private async assertRenameKeepsNamesApart( + skillId: string, + oldName: string, + newName: string, + ctx: OperationContext, + ): Promise { + const agents = await this.skillRepository.findFollowingAgents(skillId, ctx); + const refs = await this.skillRepository.findDependencies( + agents.map((agent) => agent.id), + ctx, + ); + const otherIds = (agent: (typeof agents)[number]) => + (refs.get(agent.id) ?? []).map((ref) => ref.skillId).filter((id) => id !== skillId); + const names = await this.latestNames(agents.flatMap(otherIds), ctx); + const clashing = agents.filter((agent) => + renameIntroducesClash( + otherIds(agent).flatMap((id) => names.get(id) ?? []), + oldName, + newName, + ), + ); + if (clashing.length > 0) { + throw new UserError( + `Cannot rename to "${newName}": ${clashing.map((agent) => agent.name).join(', ')} already ${ + clashing.length > 1 ? 'use' : 'uses' + } a skill with a name like that.`, + ); + } + } + + // ---------------------------------------------------------------- publish and revert + + /** + * Publish, step 1: the version each ref resolves to. Publish + * creates no version. Needs the caller's transaction for the publish lock. + */ + async snapshotForPublish( + refs: SkillRef[], + ctx: OperationContext, + ): Promise<{ skills: Record; versionByRef: Map }> { + const refIds = [...new Set(refs.map((ref) => ref.id))]; + await this.skillRepository.lockForPublish(refIds, ctx); + const rows = await this.resolveRows(refs, ctx); + const versionByRef = new Map([...rows].map(([refId, row]) => [refId, row.version.id])); + return { skills: toRecord(rows), versionByRef }; + } + + /** Publish, step 2: after the `agent_history` row exists. */ + async pinForPublish( + agentVersionId: string, + versionByRef: Map, + ctx: OperationContext, + ): Promise { + await this.skillRepository.insertPins( + [...versionByRef].map(([skillId, skillVersionId]) => ({ + agentVersionId, + skillId, + skillVersionId, + })), + ctx, + ); + } + + /** + * Revert: each ref gets a pin to the version that agent version used. No skill row is + * written, so other agents are not affected and no skill permission is needed. A ref + * with no pin follows the latest version. + */ + async restoreFromVersion( + agentVersionId: string, + refs: SkillRef[], + ctx: OperationContext = {}, + ): Promise { + const pinned = await this.skillRepository.findPinned(agentVersionId, ctx); + return refs.map(({ versionId: _dropped, ...ref }) => { + const row = pinned.get(ref.id); + return row ? { ...ref, versionId: row.version.id } : ref; + }); + } + + // ---------------------------------------------------------------- attach + + /** An agent can attach instance skills, the skills of its own project and its own agent skills. */ + isAttachable( + skill: Pick, + agent: { id: string; projectId: string }, + ): boolean { + if (skill.agentId !== null) return skill.agentId === agent.id; + return skill.projectId === null || skill.projectId === agent.projectId; + } + + /** + * The check an agent config save runs on the skill refs it gets. The agent's current + * refs come from its dependency rows. Unknown ids are dropped unless already + * attached. A ref keeps a pin only when the agent already had it. A new ref + * must be attachable and must not clash by name. + */ + async checkRefs( + agent: { id: string; projectId: string }, + refs: SkillRef[], + ctx: OperationContext = {}, + ): Promise { + const current = + (await this.skillRepository.findDependencies([agent.id], ctx)).get(agent.id) ?? []; + const existingIds = new Set(current.map((ref) => ref.skillId)); + const existingPins = new Map(current.map((ref) => [ref.skillId, ref.versionId])); + const found = new Map( + ( + await this.skillRepository.findByIds( + refs.map((ref) => ref.id), + ctx, + ) + ).map((skill) => [skill.id, skill]), + ); + const kept = refs + .filter((ref) => existingIds.has(ref.id) || found.has(ref.id)) + .map(({ versionId, ...ref }) => + versionId !== undefined && existingPins.get(ref.id) === versionId + ? { ...ref, versionId } + : ref, + ); + const newIds = [...new Set(kept.map((ref) => ref.id))].filter( + (id) => !existingIds.has(id) && found.has(id), + ); + if (newIds.length === 0) return kept; + + const keptExistingIds = [...new Set(kept.map((ref) => ref.id))].filter((id) => + existingIds.has(id), + ); + const names = await this.latestNames([...keptExistingIds, ...newIds], ctx); + for (const id of newIds) { + const skill = found.get(id); + if (skill && !this.isAttachable(skill, agent)) { + throw new UserError( + `Skill "${names.get(id) ?? id}" (${id}) is not available to this agent. An agent can use the skills of its own project, its own skills and instance skills.`, + ); + } + } + const clashes = findIntroducedNameClashes( + keptExistingIds.flatMap((id) => names.get(id) ?? []), + newIds.flatMap((id) => names.get(id) ?? []), + ); + if (clashes.length > 0) { + throw new UserError( + `This agent already has a skill with a name like "${clashes.join('", "')}". Rename one of them, or detach the other skill first.`, + ); + } + return kept; + } + + // ---------------------------------------------------------------- permission + + /** + * The project a skill's permissions come from: its own for a project skill, its + * agent's for an agent skill, none for an instance skill. A skill whose agent is gone + * has none either, and only the global scope can reach it. + */ + async projectOf( + skill: Pick, + ctx: OperationContext = {}, + ): Promise { + if (skill.projectId) return skill.projectId; + if (!skill.agentId) return null; + const agent = (await this.skillRepository.findAgents([skill.agentId], ctx)).get(skill.agentId); + return agent?.projectId ?? null; + } + + /** + * Whether the user may do `operation` on the skill, from the skill's own scope: + * `projectSkill:*` on the project of a project or agent skill, and the global + * `skill:*` for an instance skill. Every user may read an instance skill. + */ + async canAccess( + user: User, + skill: Pick, + operation: 'read' | 'update' | 'delete', + ): Promise { + const projectId = await this.projectOf(skill); + if (projectId) { + return await userHasScopes(user, [`projectSkill:${operation}`], false, { projectId }); + } + return this.canAccessInstanceSkill(user, skill, operation); + } + + /** + * `canAccess` for a whole list: one query for the user's projects, not one per skill. + * The caller passes each skill's project from `projectOf`, resolved in bulk. + */ + async accessCheck( + user: User, + operation: 'read' | 'update' | 'delete', + ): Promise<(skill: Pick, projectId: string | null) => boolean> { + const projectIds = await this.projectScopeService.getProjectIds(user, [ + `projectSkill:${operation}`, + ]); + const allowed = projectIds === null ? null : new Set(projectIds); + return (skill, projectId) => + projectId + ? allowed === null || allowed.has(projectId) + : this.canAccessInstanceSkill(user, skill, operation); + } + + private canAccessInstanceSkill( + user: User, + skill: Pick, + operation: 'read' | 'update' | 'delete', + ): boolean { + // An agent skill without a project lost its agent mid-request: only the global scope. + if (skill.agentId === null && operation === 'read') return true; + return hasGlobalScope(user, `skill:${operation}`); + } + + async canEdit(user: User, skill: Pick): Promise { + return await this.canAccess(user, skill, 'update'); + } + + async assertCanEdit(user: User, skillIds: string[], ctx: OperationContext = {}): Promise { + const skills = await this.skillRepository.findByIds(skillIds, ctx); + const names = await this.latestNames( + skills.map((skill) => skill.id), + ctx, + ); + const denied: string[] = []; + for (const skill of skills) { + if (!(await this.canEdit(user, skill))) { + denied.push(`"${names.get(skill.id) ?? skill.id}" (${skill.id})`); + } + } + if (denied.length > 0) { + throw new ForbiddenError( + `You can use but not edit ${denied.join(', ')}. Ask someone who can edit ${ + denied.length > 1 ? 'these skills' : 'this skill' + }, or detach it from this agent.`, + ); + } + } + + // ---------------------------------------------------------------- delete + + /** + * Deletes a skill that no agent draft and no published agent version uses. The conflict + * names the agents in `agentProjectIds` and only counts the others. + */ + async deleteSkill( + skillId: string, + agentProjectIds: string[] | 'all' = 'all', + ctx: OperationContext = {}, + ): Promise { + await this.txRunner.run(ctx, async (txCtx) => { + const [skill] = await this.skillRepository.findByIds([skillId], txCtx); + if (!skill) throw new NotFoundError('Skill not found'); + await this.skillRepository.lockForEdit([skillId], txCtx); + const usage = await this.skillRepository.findUsage(skillId, agentProjectIds, txCtx); + if (usage.drafts.length > 0 || usage.pins.length > 0 || usage.hiddenAgents > 0) { + const name = (await this.latestNames([skillId], txCtx)).get(skillId) ?? skillId; + const users = [ + ...usage.drafts.map((draft) => `${draft.agentName} (draft)`), + ...usage.pins.map( + (pin) => + `${pin.agentName} (${pin.version === null ? 'older published version' : `published v${pin.version}`})`, + ), + ]; + if (usage.hiddenAgents > 0) { + users.push( + `${usage.hiddenAgents} ${usage.hiddenAgents === 1 ? 'agent' : 'agents'} in other projects`, + ); + } + throw new ConflictError( + `Skill "${name}" is used by ${[...new Set(users)].join(', ')} and cannot be deleted.`, + ); + } + await this.skillRepository.deleteSkill(skillId, txCtx); + }); + this.logger.debug('Deleted skill', { skillId }); + } +} diff --git a/packages/cli/src/modules/agents/skills/skills-api.service.ts b/packages/cli/src/modules/agents/skills/skills-api.service.ts new file mode 100644 index 00000000000..60e07ccf33d --- /dev/null +++ b/packages/cli/src/modules/agents/skills/skills-api.service.ts @@ -0,0 +1,221 @@ +import type { + CreateSkillDto, + ListSkillsQueryDto, + SkillDetail, + SkillListItem, + SkillListResponse, + SkillSaveResponse, + SaveSkillDto, + SkillScope, +} from '@n8n/api-types'; +import { ProjectScopeService } from '@n8n/backend-services'; +import type { User } from '@n8n/db'; +import { Service } from '@n8n/di'; +import { ForbiddenError, NotFoundError, UserError } from '@n8n/errors'; +import { hasGlobalScope } from '@n8n/permissions'; + +import { userHasScopes } from '@/permissions.ee/check-access'; + +import type { Skill } from '../entities/skill.entity'; +import { SkillRepository, type SkillTarget } from '../repositories/skill.repository'; +import { getAgentSkillHash } from '../utils/agent-config-hash'; +import { SkillService, toAgentSkill } from './skill.service'; + +function scopeOf(skill: Pick): SkillScope { + if (skill.agentId) return 'agent'; + if (skill.projectId) return 'project'; + return 'instance'; +} + +/** + * Skills as the settings page sees them: list, read, create, save and delete. The + * permission comes from each skill's own scope (see `SkillService.canAccess`), so the + * routes carry no scope decorator. + */ +@Service() +export class SkillsApiService { + constructor( + private readonly skillService: SkillService, + private readonly skillRepository: SkillRepository, + private readonly projectScopeService: ProjectScopeService, + ) {} + + async list(user: User, query: ListSkillsQueryDto): Promise { + if (query.attachableTo !== undefined && (query.scope || query.projectId)) { + throw new UserError('Filter by attachableTo alone, without scope or projectId.'); + } + const projectIds = await this.projectScopeService.getProjectIds(user, ['projectSkill:list']); + const attachableTo = await this.attachTarget(query.attachableTo); + const { skills, count } = await this.skillRepository.findVisiblePage( + { projectIds: projectIds ?? 'all' }, + { + scope: query.scope, + projectId: query.projectId, + attachableTo, + search: query.search, + skip: query.skip, + take: query.take, + }, + ); + return { count, data: await this.toListItems(user, skills) }; + } + + async get(user: User, skillId: string): Promise { + return await this.toDetail(user, await this.requireAccess(user, skillId, 'read')); + } + + async create(user: User, payload: CreateSkillDto): Promise { + const target = await this.targetForCreate(user, payload); + const id = await this.skillService.create({ + target, + skill: payload.skill, + source: 'ui', + createdById: user.id, + }); + // No second access check: a custom role can create skills it cannot read later. + const [skill] = await this.skillRepository.findByIds([id]); + if (!skill) throw new NotFoundError('Skill not found'); + return await this.toDetail(user, skill); + } + + /** 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 saved = await this.skillService.save(skillId, { ...payload }, user.id); + return { id: skillId, ...saved, skillHash: getAgentSkillHash(saved.skill) }; + } + + async delete(user: User, skillId: string): Promise { + await this.requireAccess(user, skillId, 'delete'); + // The conflict message names only the agents the user may read, as the detail does. + const agentProjectIds = await this.projectScopeService.getProjectIds(user, ['agent:read']); + await this.skillService.deleteSkill(skillId, agentProjectIds ?? 'all'); + } + + private async toDetail(user: User, skill: Skill): Promise { + const [item] = await this.toListItems(user, [skill]); + const latest = (await this.skillRepository.findLatestSaved([skill.id])).get(skill.id); + if (!item || !latest) throw new NotFoundError('Skill not found'); + const content = toAgentSkill(latest); + // Any user can read an instance skill, so "used by" only names the agents they can read. + const agentProjectIds = await this.projectScopeService.getProjectIds(user, ['agent:read']); + return { + ...item, + skill: content, + skillHash: getAgentSkillHash(content), + usedBy: await this.skillRepository.findUsage(skill.id, agentProjectIds ?? 'all'), + }; + } + + /** A skill the user cannot see answers like a missing one. */ + private async requireAccess( + user: User, + skillId: string, + operation: 'read' | 'update' | 'delete', + ): Promise { + const [skill] = await this.skillRepository.findByIds([skillId]); + if (!skill || !(await this.skillService.canAccess(user, skill, 'read'))) { + throw new NotFoundError('Skill not found'); + } + if (operation !== 'read' && !(await this.skillService.canAccess(user, skill, operation))) { + throw new ForbiddenError( + operation === 'delete' + ? 'You are not allowed to delete this skill.' + : 'You can use but not edit this skill.', + ); + } + return skill; + } + + /** The agent of an `attachableTo` filter with its project. An unknown agent can attach nothing. */ + private async attachTarget( + agentId: string | undefined, + ): Promise<{ agentId: string; projectId: string } | undefined> { + if (agentId === undefined) return undefined; + const agent = (await this.skillRepository.findAgents([agentId])).get(agentId); + if (!agent) throw new NotFoundError('Agent not found'); + return { agentId, projectId: agent.projectId }; + } + + private async targetForCreate(user: User, payload: CreateSkillDto): Promise { + switch (payload.scope) { + case 'instance': + if (payload.projectId || payload.agentId) { + throw new UserError('An instance skill has no project and no agent.'); + } + if (!hasGlobalScope(user, 'skill:create')) { + throw new ForbiddenError('You are not allowed to create skills for the whole instance.'); + } + return { projectId: null, agentId: null }; + case 'project': { + const { projectId } = payload; + if (!projectId) throw new UserError('A project skill needs a project id.'); + 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)) { + throw new NotFoundError('Project not found'); + } + // A personal project keeps its own skills, as a team project does. + return { projectId, agentId: null }; + } + case 'agent': { + const { agentId } = payload; + if (!agentId) throw new UserError('An agent skill needs an agent id.'); + if (payload.projectId) + throw new UserError('An agent skill takes its project from the agent.'); + const agent = (await this.skillRepository.findAgents([agentId])).get(agentId); + if (!agent) throw new NotFoundError('Agent not found'); + await this.assertCanCreateIn(user, agent.projectId); + return { projectId: null, agentId }; + } + } + } + + private async assertCanCreateIn(user: User, projectId: string): Promise { + if (!(await userHasScopes(user, ['projectSkill:create'], false, { projectId }))) { + throw new ForbiddenError('You are not allowed to create skills for this project.'); + } + } + + private async toListItems(user: User, skills: Skill[]): Promise { + const ids = skills.map((skill) => skill.id); + // An agent skill has no projectId: resolve its project through its agent. + const agents = await this.skillRepository.findAgents( + skills.flatMap((skill) => (skill.agentId ? [skill.agentId] : [])), + ); + const projectOf = (skill: Skill) => + skill.projectId ?? (skill.agentId ? (agents.get(skill.agentId)?.projectId ?? null) : null); + const [latest, usage, projects, canEdit, canDelete] = await Promise.all([ + this.skillRepository.findLatestSummaries(ids), + this.skillRepository.countUsingAgents(ids), + this.skillRepository.findProjectNames(skills.flatMap((skill) => projectOf(skill) ?? [])), + this.skillService.accessCheck(user, 'update'), + this.skillService.accessCheck(user, 'delete'), + ]); + const items: SkillListItem[] = []; + for (const skill of skills) { + const summary = latest.get(skill.id); + if (!summary) continue; + const projectId = projectOf(skill); + items.push({ + id: skill.id, + name: summary.name, + description: summary.description, + scope: scopeOf(skill), + projectId, + projectName: projectId ? (projects.get(projectId) ?? null) : null, + agentId: skill.agentId, + agentName: skill.agentId ? (agents.get(skill.agentId)?.name ?? null) : null, + source: skill.source, + latestVersion: summary.version, + usedByAgents: usage.get(skill.id) ?? 0, + canEdit: canEdit(skill, projectId), + canDelete: canDelete(skill, projectId), + createdAt: skill.createdAt.toISOString(), + updatedAt: skill.updatedAt.toISOString(), + }); + } + return items; + } +} diff --git a/packages/cli/test/integration/agents/skill.repository.test.ts b/packages/cli/test/integration/agents/skill.repository.test.ts new file mode 100644 index 00000000000..fe6b2607bea --- /dev/null +++ b/packages/cli/test/integration/agents/skill.repository.test.ts @@ -0,0 +1,734 @@ +import { createTeamProject, 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'; + +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 { skillContentHash, type SkillContent } from '@/modules/agents/skills/skill-content-hash'; + +import { createUser } from '../shared/db/users'; + +/** More ids than SQLite binds in one query (32,766). */ +const manyUnknownIds = () => Array.from({ length: 33_000 }, (_, i) => `skill_unknown${i}`); + +const content = (overrides: Partial = {}): SkillContent => ({ + name: 'Brand voice', + description: 'Write in our voice', + instructions: 'Use short sentences.', + frontmatter: null, + files: [], + ...overrides, +}); + +describe('SkillRepository', () => { + let skills: SkillRepository; + let agents: AgentRepository; + let histories: AgentHistoryRepository; + let txRunner: TransactionRunner; + let user: User; + let project: Project; + let otherProject: Project; + + beforeAll(async () => { + await testModules.loadModules(['agents']); + await testDb.init(); + skills = Container.get(SkillRepository); + agents = Container.get(AgentRepository); + histories = Container.get(AgentHistoryRepository); + txRunner = Container.get(TransactionRunner); + user = await createUser(); + project = await createTeamProject('Team'); + otherProject = await createTeamProject('Other team'); + }); + + afterEach(async () => { + // Agent rows cascade to their history, pins and dependency rows. + await agents.delete({}); + await skills.delete({}); + }); + + afterAll(async () => { + await testDb.terminate(); + }); + + let counter = 0; + async function createSkill( + target: { projectId?: string; agentId?: string } = {}, + body: SkillContent = content(), + ): Promise { + const id = `skill_test${++counter}`; + await skills.createSkill( + { + id, + target: { projectId: target.projectId ?? null, agentId: target.agentId ?? null }, + source: 'ui', + createdById: null, + }, + body, + ); + return id; + } + + async function createAgent(name: string, options: { inSync?: boolean; projectId?: string } = {}) { + const versionId = randomUUID(); + const agent = await agents.save( + agents.create({ + id: randomUUID(), + name, + projectId: options.projectId ?? project.id, + integrations: [], + tools: {}, + skills: {}, + versionId, + }), + ); + if (options.inSync) { + await histories.save(histories.create({ versionId, agentId: agent.id, author: 'Test' })); + await agents.update({ id: agent.id }, { activeVersionId: versionId }); + } + return agent; + } + + async function publish(agentId: string): Promise { + const versionId = randomUUID(); + await histories.save(histories.create({ versionId, agentId, author: 'Test' })); + return versionId; + } + + async function latestVersionId(skillId: string): Promise { + const row = (await skills.findLatestSaved([skillId])).get(skillId); + if (!row) throw new Error(`no saved version for ${skillId}`); + return row.version.id; + } + + describe('create', () => { + it('writes the skill with v1 as its only version', async () => { + const id = await createSkill({ projectId: project.id }); + + const latest = (await skills.findLatestSaved([id])).get(id); + + expect(latest?.version.version).toBe(1); + expect(latest?.skill).toMatchObject({ + id, + projectId: project.id, + agentId: null, + source: 'ui', + }); + expect(latest?.version.contentHash).toBe(skillContentHash(content())); + expect(await skills.nextVersionNumber(id)).toBe(2); + }); + + it('stores the files of v1 sorted by path', async () => { + const id = await createSkill( + {}, + content({ + files: [ + { path: 'references/b.md', content: 'b' }, + { path: 'references/B.md', content: 'B' }, + { path: 'references/a.md', content: 'a' }, + ], + }), + ); + + const latest = (await skills.findLatestSaved([id])).get(id); + + const paths = ['references/B.md', 'references/a.md', 'references/b.md']; + expect(latest?.files.map((file) => file.path)).toEqual(paths); + expect(latest?.files[0]).toMatchObject({ content: 'B' }); + }); + + // SQLite returns rows in UTF-8 byte order and Postgres in its collation order. The + // content hash uses UTF-16 code units, so the repository sorts on its own. + it('sorts files by UTF-16 code unit, as the content hash does', async () => { + const id = await createSkill( + {}, + content({ + files: [ + { path: 'references/\uff5e.md', content: 'fullwidth tilde' }, + { path: 'references/\u{1f600}.md', content: 'emoji' }, + ], + }), + ); + + const latest = (await skills.findLatestSaved([id])).get(id); + + expect(latest?.files.map((file) => file.content)).toEqual(['emoji', 'fullwidth tilde']); + }); + + it('rolls back with the caller transaction', async () => { + await expect( + txRunner.run({}, async (ctx) => { + await skills.createSkill( + { + id: 'skill_rollback', + target: { projectId: null, agentId: null }, + source: 'ui', + createdById: null, + }, + content(), + ctx, + ); + throw new Error('abort'); + }), + ).rejects.toThrow('abort'); + + expect(await skills.findByIds(['skill_rollback'])).toEqual([]); + }); + }); + + describe('findVisiblePage', () => { + const firstPage = { skip: 0 }; + const ids = (page: { skills: Array<{ id: string }> }) => page.skills.map((skill) => skill.id); + + it('lists instance skills and the skills of the given projects and their agents, newest change first', async () => { + const instance = await createSkill(); + const team = await createSkill({ projectId: project.id }); + const otherTeam = await createSkill({ projectId: otherProject.id }); + const agentSkill = await createSkill({ agentId: (await createAgent('Support')).id }); + const otherAgentSkill = await createSkill({ + agentId: (await createAgent('Sales', { projectId: otherProject.id })).id, + }); + await skills.insertSavedVersion(instance, 2, content({ instructions: 'touched' }), null); + + const page = await skills.findVisiblePage({ projectIds: [project.id] }, firstPage); + + expect(ids(page)).toEqual(expect.arrayContaining([instance, team, agentSkill])); + expect(ids(page)).not.toContain(otherTeam); + expect(ids(page)).not.toContain(otherAgentSkill); + expect(ids(page)[0]).toBe(instance); + expect(page.count).toBe(3); + }); + + it("adds every project's skills and every agent's skills when allowed", async () => { + const otherTeam = await createSkill({ projectId: otherProject.id }); + const otherAgentSkill = await createSkill({ + agentId: (await createAgent('Sales', { projectId: otherProject.id })).id, + }); + + const page = await skills.findVisiblePage({ projectIds: 'all' }, firstPage); + + expect(ids(page)).toEqual(expect.arrayContaining([otherTeam, otherAgentSkill])); + }); + + it('lists no project or agent skill for an empty project list', async () => { + await createSkill({ projectId: project.id }); + await createSkill({ agentId: (await createAgent('Support')).id }); + + const visibility = { projectIds: [] }; + const none = { skills: [], count: 0 }; + expect(await skills.findVisiblePage(visibility, firstPage)).toEqual(none); + expect(await skills.findVisiblePage(visibility, { ...firstPage, scope: 'project' })).toEqual( + none, + ); + expect(await skills.findVisiblePage(visibility, { ...firstPage, scope: 'agent' })).toEqual( + none, + ); + }); + + it('filters by scope and project in the query, an agent skill by the project of its agent', async () => { + const instance = await createSkill(); + const team = await createSkill({ projectId: project.id }); + const otherTeam = await createSkill({ projectId: otherProject.id }); + const agentSkill = await createSkill({ agentId: (await createAgent('Support')).id }); + const otherAgentSkill = await createSkill({ + agentId: (await createAgent('Sales', { projectId: otherProject.id })).id, + }); + const everything = { projectIds: 'all' as const }; + + const page = async (filter: { + scope?: 'project' | 'agent' | 'instance'; + projectId?: string; + }) => ids(await skills.findVisiblePage(everything, { ...firstPage, ...filter })).sort(); + + expect(await page({ scope: 'instance' })).toEqual([instance]); + expect(await page({ scope: 'project' })).toEqual([team, otherTeam].sort()); + expect(await page({ scope: 'agent' })).toEqual([agentSkill, otherAgentSkill].sort()); + expect(await page({ projectId: project.id })).toEqual([team, agentSkill].sort()); + expect(await page({ scope: 'project', projectId: project.id })).toEqual([team]); + expect(await page({ scope: 'agent', projectId: project.id })).toEqual([agentSkill]); + expect(await page({ scope: 'instance', projectId: project.id })).toEqual([]); + }); + + it('lists what one agent may attach: its project skills, its own skills and instance skills', async () => { + const support = await createAgent('Support'); + const sales = await createAgent('Sales'); + const instance = await createSkill(); + const team = await createSkill({ projectId: project.id }); + await createSkill({ projectId: otherProject.id }); + const own = await createSkill({ agentId: support.id }); + await createSkill({ agentId: sales.id }); + + const attachable = async (projectIds: string[] | 'all') => + ids( + await skills.findVisiblePage( + { projectIds }, + { skip: 0, attachableTo: { agentId: support.id, projectId: project.id } }, + ), + ).sort(); + + expect(await attachable([project.id])).toEqual([instance, team, own].sort()); + expect(await attachable('all')).toEqual([instance, team, own].sort()); + // A user who may not list the project's skills gets only the instance skills. + expect(await attachable([otherProject.id])).toEqual([instance]); + }); + + it('searches the name and description of the latest version, before the page is cut', async () => { + const renamed = await createSkill({}, content({ name: 'Old name' })); + await skills.insertSavedVersion(renamed, 2, content({ name: 'Triage RULES' }), null); + const described = await createSkill( + {}, + content({ name: 'Other', description: 'rules for x' }), + ); + await createSkill({}, content({ name: 'Unrelated' })); + const visibility = { projectIds: [] }; + + const byName = await skills.findVisiblePage(visibility, { + skip: 0, + take: 1, + search: 'rules', + }); + const all = await skills.findVisiblePage(visibility, { skip: 0, search: 'RULES' }); + const oldName = await skills.findVisiblePage(visibility, { skip: 0, search: 'old name' }); + + expect(byName.count).toBe(2); + expect(byName.skills).toHaveLength(1); + expect(ids(all).sort()).toEqual([renamed, described].sort()); + expect(oldName).toEqual({ skills: [], count: 0 }); + }); + + it('treats LIKE wildcards in the search as plain text', async () => { + const percent = await createSkill({}, content({ name: '100% done' })); + await createSkill({}, content({ name: '100 done' })); + const visibility = { projectIds: [] }; + + const page = await skills.findVisiblePage(visibility, { skip: 0, search: '100%' }); + + expect(ids(page)).toEqual([percent]); + }); + + it('cuts the page with skip and take, and answers past the end with the full count', async () => { + for (let i = 0; i < 3; i++) await createSkill(); + const visibility = { projectIds: [] }; + + const all = await skills.findVisiblePage(visibility, { skip: 0 }); + const second = await skills.findVisiblePage(visibility, { skip: 1, take: 1 }); + const past = await skills.findVisiblePage(visibility, { skip: 10, take: 5 }); + + expect(all.skills).toHaveLength(3); + expect(ids(second)).toEqual([ids(all)[1]]); + expect(second.count).toBe(3); + expect(past).toEqual({ skills: [], count: 3 }); + }); + }); + + describe('findByIds', () => { + it('returns the existing skills and skips unknown ids', async () => { + const id = await createSkill(); + + const found = await skills.findByIds([id, 'skill_unknown']); + + expect(found.map((skill) => skill.id)).toEqual([id]); + }); + }); + + describe('saved versions', () => { + it('numbers the next version after the highest one', async () => { + const id = await createSkill(); + + expect(await skills.nextVersionNumber(id)).toBe(2); + await skills.insertSavedVersion(id, 2, content({ instructions: 'v2' }), user.id); + expect(await skills.nextVersionNumber(id)).toBe(3); + }); + + it('moves the skill to the top of the visible list', async () => { + const first = await createSkill(); + await createSkill(); + + await skills.insertSavedVersion(first, 2, content({ instructions: 'newer' }), null); + + const { skills: page } = await skills.findVisiblePage({ projectIds: [] }, { skip: 0 }); + expect(page[0].id).toBe(first); + }); + + it('returns 1 for a skill with no saved version', async () => { + expect(await skills.nextVersionNumber('skill_unknown')).toBe(1); + }); + + it('resolves the highest version as the latest, with its files and author', async () => { + const id = await createSkill(); + const versionId = await skills.insertSavedVersion( + id, + 2, + content({ instructions: 'v2', files: [{ path: 'references/x.md', content: 'x' }] }), + user.id, + ); + + const latest = (await skills.findLatestSaved([id])).get(id); + + expect(latest?.version).toMatchObject({ id: versionId, version: 2, instructions: 'v2' }); + expect(latest?.version.createdById).toBe(user.id); + expect(latest?.files.map((file) => file.content)).toEqual(['x']); + }); + + it('returns name and description of the latest version as summaries', async () => { + const id = await createSkill(); + await skills.insertSavedVersion( + id, + 2, + content({ name: 'Tone', description: 'How we sound' }), + null, + ); + + expect((await skills.findLatestSummaries([id, 'skill_unknown'])).get(id)).toEqual({ + name: 'Tone', + description: 'How we sound', + version: 2, + }); + }); + + it('resolves the latest of three versions with its own files', async () => { + const id = await createSkill(); + await skills.insertSavedVersion( + id, + 2, + content({ instructions: 'v2', files: [{ path: 'references/a.md', content: 'a2' }] }), + null, + ); + await skills.insertSavedVersion( + id, + 3, + content({ instructions: 'v3', files: [{ path: 'references/a.md', content: 'a3' }] }), + null, + ); + + const latest = (await skills.findLatestSaved([id])).get(id); + + expect(latest?.version).toMatchObject({ version: 3, instructions: 'v3' }); + expect(latest?.files.map((file) => file.content)).toEqual(['a3']); + }); + + it('reads more skill ids than one query can bind', async () => { + const id = await createSkill(); + const ids = [id, ...manyUnknownIds()]; + + expect([...(await skills.findLatestSaved(ids)).keys()]).toEqual([id]); + expect([...(await skills.findLatestSummaries(ids)).keys()]).toEqual([id]); + expect(await skills.findByIds(ids)).toHaveLength(1); + }); + + it('finds versions by id with their skill', async () => { + const id = await createSkill(); + const v1 = await latestVersionId(id); + + const found = await skills.findVersionsByIds([v1, randomUUID()]); + + expect([...found.keys()]).toEqual([v1]); + expect(found.get(v1)?.skill.id).toBe(id); + }); + }); + + describe('pins', () => { + it('finds the pinned version of each skill of one published agent version', async () => { + const id = await createSkill(); + const v1 = await latestVersionId(id); + await skills.insertSavedVersion(id, 2, content({ instructions: 'v2' }), null); + const agent = await createAgent('Support'); + const agentVersionId = await publish(agent.id); + + await skills.insertPins([{ agentVersionId, skillId: id, skillVersionId: v1 }]); + + const pinned = await skills.findPinned(agentVersionId); + expect([...pinned.keys()]).toEqual([id]); + expect(pinned.get(id)?.version).toMatchObject({ id: v1, version: 1 }); + }); + }); + + describe('dependencies', () => { + it('replaces the rows of one agent and skips unknown skills', async () => { + const a = await createSkill(); + const b = await createSkill(); + const agent = await createAgent('Support'); + await skills.replaceDependencies(agent.id, [{ skillId: a }]); + + await skills.replaceDependencies(agent.id, [{ skillId: b }, { skillId: 'skill_unknown' }]); + + expect(await skills.findFollowingAgentIds([a])).toEqual([]); + expect(await skills.findFollowingAgentIds([b])).toEqual([agent.id]); + }); + + it('leaves pinned refs out of the following agents', async () => { + const id = await createSkill(); + const v1 = await latestVersionId(id); + const following = await createAgent('Following'); + const pinned = await createAgent('Pinned'); + await skills.replaceDependencies(following.id, [{ skillId: id }]); + await skills.replaceDependencies(pinned.id, [{ skillId: id, versionId: v1 }]); + + expect(await skills.findFollowingAgentIds([id])).toEqual([following.id]); + expect(await skills.findFollowingAgents(id)).toEqual([ + { id: following.id, name: 'Following' }, + ]); + }); + + it('lets a following ref win over a pinned ref to the same skill', async () => { + const id = await createSkill(); + const v1 = await latestVersionId(id); + const agent = await createAgent('Both'); + + await skills.replaceDependencies(agent.id, [{ skillId: id }, { skillId: id, versionId: v1 }]); + expect(await skills.findFollowingAgentIds([id])).toEqual([agent.id]); + + await skills.replaceDependencies(agent.id, [{ skillId: id, versionId: v1 }, { skillId: id }]); + expect(await skills.findFollowingAgentIds([id])).toEqual([agent.id]); + }); + + it('returns the refs of each agent in the order they were attached', async () => { + const [a, b, c] = [await createSkill(), await createSkill(), await createSkill()]; + const v1 = await latestVersionId(a); + const agent = await createAgent('Support'); + const other = await createAgent('Sales'); + await skills.replaceDependencies(agent.id, [{ skillId: b }, { skillId: a, versionId: v1 }]); + await skills.replaceDependencies(other.id, [{ skillId: c }]); + + const byAgent = await skills.findDependencies([agent.id, other.id]); + + expect(byAgent).toEqual( + new Map([ + [ + agent.id, + [ + { skillId: b, versionId: null }, + { skillId: a, versionId: v1 }, + ], + ], + [other.id, [{ skillId: c, versionId: null }]], + ]), + ); + }); + + // The attach order of an agent's skills comes from `createdAt`, so a row that stays + // keeps its timestamp. + it('keeps the rows that stay, so a new skill sorts after them', async () => { + const [a, b, c] = [await createSkill(), await createSkill(), await createSkill()]; + const agent = await createAgent('Support'); + await skills.replaceDependencies(agent.id, [{ skillId: a }, { skillId: b }]); + + await skills.replaceDependencies(agent.id, [{ skillId: c }, { skillId: b }, { skillId: a }]); + + expect( + (await skills.findDependencies([agent.id])).get(agent.id)?.map((ref) => ref.skillId), + ).toEqual([a, b, c]); + }); + + it('updates the pin of a row that stays', async () => { + const id = await createSkill(); + const v1 = await latestVersionId(id); + const agent = await createAgent('Support'); + await skills.replaceDependencies(agent.id, [{ skillId: id }]); + + await skills.replaceDependencies(agent.id, [{ skillId: id, versionId: v1 }]); + expect(await skills.findFollowingAgentIds([id])).toEqual([]); + + await skills.replaceDependencies(agent.id, [{ skillId: id }]); + expect(await skills.findFollowingAgentIds([id])).toEqual([agent.id]); + }); + }); + + describe('usage', () => { + it('lists each agent once, with the version its current published version runs', async () => { + const id = await createSkill(); + const v1 = await latestVersionId(id); + const v2 = await skills.insertSavedVersion(id, 2, content({ instructions: 'v2' }), null); + const drafting = await createAgent('Drafting'); + await skills.replaceDependencies(drafting.id, [{ skillId: id }]); + const published = await createAgent('Published'); + const old = await publish(published.id); + const current = await publish(published.id); + await agents.update({ id: published.id }, { activeVersionId: current }); + const retired = await createAgent('Retired', { inSync: true }); + await skills.insertPins([ + { agentVersionId: old, skillId: id, skillVersionId: v1 }, + { agentVersionId: current, skillId: id, skillVersionId: v2 }, + { agentVersionId: await publish(retired.id), skillId: id, skillVersionId: v1 }, + ]); + + const usage = await skills.findUsage(id); + + expect(usage.drafts).toEqual([ + { agentId: drafting.id, agentName: 'Drafting', projectId: project.id }, + ]); + expect(usage.pins).toHaveLength(2); + expect(usage.pins).toEqual( + expect.arrayContaining([ + { agentId: published.id, agentName: 'Published', projectId: project.id, version: 2 }, + { agentId: retired.id, agentName: 'Retired', projectId: project.id, version: null }, + ]), + ); + expect(usage.hiddenAgents).toBe(0); + }); + + it('leaves out the agents of other projects and counts them', async () => { + const id = await createSkill(); + const v1 = await latestVersionId(id); + const visible = await createAgent('Visible'); + await skills.replaceDependencies(visible.id, [{ skillId: id }]); + const drafting = await createAgent('Elsewhere', { projectId: otherProject.id }); + await skills.replaceDependencies(drafting.id, [{ skillId: id }]); + const published = await createAgent('Published elsewhere', { projectId: otherProject.id }); + await skills.insertPins([ + { agentVersionId: await publish(published.id), skillId: id, skillVersionId: v1 }, + ]); + + const usage = await skills.findUsage(id, [project.id]); + + expect(usage).toEqual({ + drafts: [{ agentId: visible.id, agentName: 'Visible', projectId: project.id }], + pins: [], + hiddenAgents: 2, + }); + }); + + it('is empty for a skill nobody uses', async () => { + const id = await createSkill(); + + expect(await skills.findUsage(id)).toEqual({ drafts: [], pins: [], hiddenAgents: 0 }); + }); + + it('counts distinct agents per skill across drafts and pins', async () => { + const id = await createSkill(); + const unused = await createSkill(); + const v1 = await latestVersionId(id); + const agent = await createAgent('Support'); + await skills.replaceDependencies(agent.id, [{ skillId: id }]); + const other = await createAgent('Sales'); + for (const agentId of [agent.id, other.id]) { + for (let i = 0; i < 3; i++) { + await skills.insertPins([ + { agentVersionId: await publish(agentId), skillId: id, skillVersionId: v1 }, + ]); + } + } + + const counts = await skills.countUsingAgents([id, unused, ...manyUnknownIds()]); + + expect(counts.get(id)).toBe(2); + expect(counts.get(unused)).toBeUndefined(); + }); + }); + + describe('findProjectNames', () => { + it('maps project ids to names', async () => { + const names = await skills.findProjectNames([project.id, project.id, otherProject.id]); + + expect(names).toEqual( + new Map([ + [project.id, 'Team'], + [otherProject.id, 'Other team'], + ]), + ); + }); + }); + + describe('findAgents', () => { + it('maps agent ids to their name and project, and skips unknown ids', async () => { + const support = await createAgent('Support'); + const sales = await createAgent('Sales', { projectId: otherProject.id }); + + const agents = await skills.findAgents([support.id, support.id, sales.id, 'agent-gone']); + + expect(agents).toEqual( + new Map([ + [support.id, { name: 'Support', projectId: project.id }], + [sales.id, { name: 'Sales', projectId: otherProject.id }], + ]), + ); + }); + }); + + describe('deleteSkill', () => { + it('removes the skill with its versions, files and dependency rows', async () => { + const id = await createSkill( + {}, + content({ files: [{ path: 'references/a.md', content: 'a' }] }), + ); + const v1 = await latestVersionId(id); + const agent = await createAgent('Support'); + await skills.replaceDependencies(agent.id, [{ skillId: id }]); + + await skills.deleteSkill(id); + + expect(await skills.findByIds([id])).toEqual([]); + expect((await skills.findVersionsByIds([v1])).size).toBe(0); + expect(await skills.findFollowingAgentIds([id])).toEqual([]); + }); + + it('removes the pins of published agent versions with the skill', async () => { + const id = await createSkill(); + const agent = await createAgent('Support'); + const agentVersionId = await publish(agent.id); + await skills.insertPins([ + { agentVersionId, skillId: id, skillVersionId: await latestVersionId(id) }, + ]); + + await skills.deleteSkill(id); + + expect(await skills.findPinned(agentVersionId)).toEqual(new Map()); + }); + }); + + describe('row locks', () => { + it('refuse to run outside a transaction', async () => { + const id = await createSkill(); + + await expect(skills.lockForEdit([id])).rejects.toThrow('transaction'); + await expect(skills.lockForPublish([id])).rejects.toThrow('transaction'); + }); + + it('run inside a transaction', async () => { + const id = await createSkill(); + + await txRunner.run({}, async (ctx) => { + await skills.lockForEdit([id], ctx); + await skills.lockForPublish([id], ctx); + }); + }); + }); + + describe('AgentRepository.markDraftChangedIfInSync', () => { + it('gives an agent in sync with its published version a new draft version id', async () => { + const agent = await createAgent('Support', { inSync: true }); + + const changed = await agents.markDraftChangedIfInSync([agent.id]); + + const reloaded = await agents.findOneByOrFail({ id: agent.id }); + expect(changed).toEqual([agent.id]); + expect(reloaded.versionId).not.toBe(reloaded.activeVersionId); + expect(reloaded.revision).toBe(agent.revision); + }); + + it('leaves an agent that already has unpublished changes alone', async () => { + const agent = await createAgent('Support'); + + const changed = await agents.markDraftChangedIfInSync([agent.id]); + + const reloaded = await agents.findOneByOrFail({ id: agent.id }); + expect(changed).toEqual([]); + expect(reloaded.versionId).toBe(agent.versionId); + }); + + it('writes nothing for no agents', async () => { + expect(await agents.markDraftChangedIfInSync([])).toEqual([]); + expect(await agents.findProjectIdsByIds([])).toEqual([]); + }); + + it('returns the project of each agent', async () => { + const agent = await createAgent('Support'); + + expect(await agents.findProjectIdsByIds([agent.id])).toEqual([ + { id: agent.id, projectId: project.id }, + ]); + }); + }); +}); diff --git a/packages/cli/test/integration/agents/skills.controller.test.ts b/packages/cli/test/integration/agents/skills.controller.test.ts new file mode 100644 index 00000000000..395bebb4814 --- /dev/null +++ b/packages/cli/test/integration/agents/skills.controller.test.ts @@ -0,0 +1,400 @@ +import { createTeamProject, linkUserToProject, testDb, testModules } from '@n8n/backend-test-utils'; +import type { Project, User } from '@n8n/db'; +import { Container } from '@n8n/di'; +import { randomUUID } from 'node:crypto'; + +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 { createMember, createOwner } from '../shared/db/users'; +import type { SuperAgentTest } from '../shared/types'; +import { setupTestServer } from '../shared/utils'; + +const body = { name: 'Brand voice', description: 'How we write', instructions: 'Be brief.' }; + +beforeAll(async () => { + await testModules.loadModules(['agents']); +}); + +describe('SkillsController', () => { + const server = setupTestServer({ endpointGroups: ['ai'] }); + let ownerUser: User; + let memberUser: User; + let owner: SuperAgentTest; + let member: SuperAgentTest; + let editor: SuperAgentTest; + let viewer: SuperAgentTest; + let outsider: SuperAgentTest; + let project: Project; + + beforeAll(async () => { + ownerUser = await createOwner(); + memberUser = await createMember(); + const editorUser = await createMember(); + const viewerUser = await createMember(); + owner = server.authAgentFor(ownerUser); + member = server.authAgentFor(memberUser); + editor = server.authAgentFor(editorUser); + viewer = server.authAgentFor(viewerUser); + outsider = server.authAgentFor(await createMember()); + project = await createTeamProject('Team'); + await linkUserToProject(editorUser, project, 'project:editor'); + await linkUserToProject(viewerUser, project, 'project:viewer'); + }); + + afterEach(async () => { + await Container.get(AgentRepository).delete({}); + await Container.get(SkillRepository).delete({}); + }); + + async function newAgent(name: string, projectId: string) { + const agents = Container.get(AgentRepository); + return await agents.save( + agents.create({ + id: randomUUID(), + name, + projectId, + integrations: [], + tools: {}, + skills: {}, + versionId: randomUUID(), + }), + ); + } + + async function attachToNewAgent( + skillId: string, + name: string, + projectId: string, + options: { published?: boolean; pinnedVersionId?: string } = {}, + ) { + const agents = Container.get(AgentRepository); + const versionId = randomUUID(); + const agent = await agents.save( + agents.create({ + id: randomUUID(), + name, + projectId, + integrations: [], + tools: {}, + skills: {}, + versionId, + }), + ); + if (options.published) { + // Published and in sync: the draft is the published version. + const histories = Container.get(AgentHistoryRepository); + await histories.save(histories.create({ versionId, agentId: agent.id, author: 'Test' })); + await agents.update({ id: agent.id }, { activeVersionId: versionId }); + } + await Container.get(SkillRepository).replaceDependencies(agent.id, [ + { skillId, versionId: options.pinnedVersionId }, + ]); + return { ...agent, versionId }; + } + + const draftVersionOf = async (agentId: string) => + (await Container.get(AgentRepository).findOneByOrFail({ id: agentId })).versionId; + + async function create(agent: SuperAgentTest, payload: object, status = 200) { + const response = await agent.post('/skills').send(payload).expect(status); + return response.body.data as { id: string; skillHash: string }; + } + + describe('agent skills', () => { + it('lets a project editor create one for an agent and checks the project of the agent', async () => { + const agent = await newAgent('Support bot', project.id); + const { id } = await create(editor, { scope: 'agent', agentId: agent.id, skill: body }); + + const read = await editor.get(`/skills/${id}`).expect(200); + expect(read.body.data).toMatchObject({ + id, + scope: 'agent', + agentId: agent.id, + agentName: 'Support bot', + projectId: project.id, + projectName: 'Team', + latestVersion: 1, + canEdit: true, + canDelete: true, + }); + await outsider.get(`/skills/${id}`).expect(404); + await outsider + .patch(`/skills/${id}`) + .send({ instructions: 'x', baseSkillHash: 'x' }) + .expect(404); + await owner.get(`/skills/${id}`).expect(200); + }); + + it('refuses an agent skill to a user outside the project, and for an unknown agent', async () => { + const agent = await newAgent('Support bot', project.id); + + await create(outsider, { scope: 'agent', agentId: agent.id, skill: body }, 403); + await create(editor, { scope: 'agent', skill: body }, 400); + await create(owner, { scope: 'agent', agentId: 'agent-gone', skill: body }, 404); + }); + }); + + describe('instance skills', () => { + it('lets only owners and admins create and edit them, and everyone read them', async () => { + await create(member, { scope: 'instance', skill: body }, 403); + const { id } = await create(owner, { scope: 'instance', skill: body }); + + const read = await member.get(`/skills/${id}`).expect(200); + expect(read.body.data).toMatchObject({ scope: 'instance', canEdit: false, canDelete: false }); + await member + .patch(`/skills/${id}`) + .send({ instructions: 'x', baseSkillHash: 'x' }) + .expect(403); + await member.delete(`/skills/${id}`).expect(403); + }); + + it('names only the agents the reader may see, and counts the others', async () => { + const { id } = await create(owner, { scope: 'instance', skill: body }); + const agent = await attachToNewAgent(id, 'Support bot', project.id); + + const asOutsider = await outsider.get(`/skills/${id}`).expect(200); + const asEditor = await editor.get(`/skills/${id}`).expect(200); + + expect(asOutsider.body.data.usedBy).toEqual({ drafts: [], pins: [], hiddenAgents: 1 }); + expect(asEditor.body.data.usedBy).toEqual({ + drafts: [{ agentId: agent.id, agentName: 'Support bot', projectId: project.id }], + pins: [], + hiddenAgents: 0, + }); + }); + }); + + describe('project skills', () => { + it('lets editors write, viewers read, and hides them from outsiders', async () => { + const { id } = await create(editor, { scope: 'project', projectId: project.id, skill: body }); + + await viewer.get(`/skills/${id}`).expect(200); + await viewer + .patch(`/skills/${id}`) + .send({ instructions: 'x', baseSkillHash: 'x' }) + .expect(403); + await outsider.get(`/skills/${id}`).expect(404); + await create(viewer, { scope: 'project', projectId: project.id, skill: body }, 403); + await create(outsider, { scope: 'project', projectId: project.id, skill: body }, 403); + }); + + it('needs a project id', async () => { + await create(editor, { scope: 'project', skill: body }, 400); + }); + + // The owner's global projectSkill:create passes the scope check for any project id. + it('answers 404 for a project that does not exist', async () => { + await create(owner, { scope: 'project', projectId: 'project-gone', skill: body }, 404); + await create(member, { scope: 'project', projectId: 'project-gone', skill: body }, 403); + }); + + it('shows editors, viewers and owners what they may do, in the detail and the list', async () => { + const { id } = await create(editor, { scope: 'project', projectId: project.id, skill: body }); + + for (const [agent, may] of [ + [editor, true], + [viewer, false], + [owner, true], + ] as const) { + const detail = await agent.get(`/skills/${id}`).expect(200); + const list = await agent.get('/skills').query({ projectId: project.id }).expect(200); + + const expected = { id, canEdit: may, canDelete: may }; + expect(detail.body.data).toMatchObject(expected); + expect(list.body.data.data).toEqual([expect.objectContaining(expected)]); + } + }); + }); + + describe('list', () => { + 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' } }); + await create(owner, { scope: 'instance', skill: { ...body, name: 'Shared rules' } }); + await create(editor, { + scope: 'project', + projectId: project.id, + skill: { ...body, name: 'Team rules' }, + }); + + const all = await editor.get('/skills').expect(200); + const outside = await member.get('/skills').expect(200); + const search = await editor.get('/skills').query({ search: 'rules', take: 1 }).expect(200); + const ownerView = await owner.get('/skills').query({ scope: 'project' }).expect(200); + + expect(all.body.data.data.map((s: { name: string }) => s.name).sort()).toEqual([ + 'Mine', + 'Shared rules', + 'Team rules', + ]); + expect(outside.body.data.data.map((s: { name: string }) => s.name)).toEqual(['Shared rules']); + expect(search.body.data.count).toBe(2); + expect(search.body.data.data).toHaveLength(1); + expect(ownerView.body.data.data).toEqual([ + expect.objectContaining({ name: 'Team rules', projectName: 'Team' }), + ]); + }); + + it('lists the skills one agent may attach, never those of another agent', async () => { + const support = await newAgent('Support bot', project.id); + const sales = await newAgent('Sales bot', project.id); + await create(editor, { + scope: 'agent', + agentId: support.id, + skill: { ...body, name: 'Mine' }, + }); + await create(editor, { + scope: 'agent', + agentId: sales.id, + skill: { ...body, name: 'Theirs' }, + }); + await create(owner, { scope: 'instance', skill: { ...body, name: 'Shared rules' } }); + await create(editor, { + scope: 'project', + projectId: project.id, + skill: { ...body, name: 'Team rules' }, + }); + + const attachable = await editor + .get('/skills') + .query({ attachableTo: support.id }) + .expect(200); + + expect(attachable.body.data.data.map((s: { name: string }) => s.name).sort()).toEqual([ + 'Mine', + 'Shared rules', + 'Team rules', + ]); + await editor.get('/skills').query({ attachableTo: support.id, scope: 'agent' }).expect(400); + }); + }); + + describe('save', () => { + it('saves changed content as the next version and unchanged content as nothing', async () => { + const { id, skillHash } = await create(editor, { + scope: 'project', + projectId: project.id, + skill: body, + }); + + const saved = await editor + .patch(`/skills/${id}`) + .send({ instructions: 'Be very brief.', baseSkillHash: skillHash }) + .expect(200); + const again = await editor + .patch(`/skills/${id}`) + .send({ instructions: 'Be very brief.', baseSkillHash: saved.body.data.skillHash }) + .expect(200); + const read = await editor.get(`/skills/${id}`).expect(200); + + expect(saved.body.data).toMatchObject({ id, version: 2, created: true }); + expect(saved.body.data.skill.instructions).toBe('Be very brief.'); + expect(again.body.data).toMatchObject({ id, version: 2, created: false }); + expect(read.body.data).toMatchObject({ + latestVersion: 2, + skillHash: saved.body.data.skillHash, + }); + }); + + it('refuses a save based on a stale hash', async () => { + const { id, skillHash } = await create(editor, { + scope: 'project', + projectId: project.id, + skill: body, + }); + await editor + .patch(`/skills/${id}`) + .send({ instructions: 'First.', baseSkillHash: skillHash }); + + await editor + .patch(`/skills/${id}`) + .send({ instructions: 'Second.', baseSkillHash: skillHash }) + .expect(409); + }); + + it('refuses a save without a base hash', async () => { + const { id } = await create(editor, { scope: 'project', projectId: project.id, skill: body }); + + await editor.patch(`/skills/${id}`).send({ instructions: 'Be very brief.' }).expect(400); + }); + + it('refuses blank instructions', async () => { + await create( + editor, + { scope: 'project', projectId: project.id, skill: { ...body, instructions: ' ' } }, + 400, + ); + }); + + it('marks a following agent that was in sync as changed, and leaves a pinned agent alone', async () => { + const { id, skillHash } = await create(editor, { + scope: 'project', + projectId: project.id, + skill: body, + }); + const v1 = (await Container.get(SkillRepository).findLatestSaved([id])).get(id)?.version.id; + const following = await attachToNewAgent(id, 'Follower', project.id, { published: true }); + const pinned = await attachToNewAgent(id, 'Pinned', project.id, { + published: true, + pinnedVersionId: v1, + }); + + const saved = await editor + .patch(`/skills/${id}`) + .send({ instructions: 'Ship weekly.', baseSkillHash: skillHash }) + .expect(200); + + expect(saved.body.data).toMatchObject({ version: 2, created: true }); + expect(await draftVersionOf(following.id)).not.toBe(following.versionId); + expect(await draftVersionOf(pinned.id)).toBe(pinned.versionId); + }); + }); + + // The edit lock is a row lock on Postgres only. SQLite runs one write at a time. + describe.skipIf(process.env.DB_TYPE !== 'postgresdb')('parallel saves on Postgres', () => { + it('answers 409 to the second of two saves from the same base', async () => { + const { id, skillHash } = await create(editor, { + scope: 'project', + projectId: project.id, + skill: body, + }); + + const responses = await Promise.all( + ['First.', 'Second.'].map( + async (instructions) => + await editor.patch(`/skills/${id}`).send({ instructions, baseSkillHash: skillHash }), + ), + ); + const read = await editor.get(`/skills/${id}`).expect(200); + + expect(responses.map((response) => response.status).sort()).toEqual([200, 409]); + expect(read.body.data.latestVersion).toBe(2); + }); + }); + + describe('delete', () => { + it('deletes an unused skill', async () => { + const { id } = await create(editor, { scope: 'project', projectId: project.id, skill: body }); + + await editor.delete(`/skills/${id}`).expect(200); + await editor.get(`/skills/${id}`).expect(404); + }); + + it('answers 409 and names the agents that use the skill', async () => { + const { id } = await create(editor, { scope: 'project', projectId: project.id, skill: body }); + await attachToNewAgent(id, 'Support bot', project.id); + + const response = await editor.delete(`/skills/${id}`).expect(409); + + expect(response.body.message).toBe( + 'Skill "Brand voice" is used by Support bot (draft) and cannot be deleted.', + ); + }); + }); + + afterAll(async () => { + await testDb.truncate(['User']); + }); +}); diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index 2c9e124cfff..f9f152115a4 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -3083,6 +3083,14 @@ "projectRoles.projectAiPreference:create.tooltip": "Add an AI preference for everyone in the project", "projectRoles.projectAiPreference:delete": "Delete", "projectRoles.projectAiPreference:delete.tooltip": "Remove an AI preference from the project", + "projectRoles.projectSkill:read": "View", + "projectRoles.projectSkill:read.tooltip": "See the skills of the project", + "projectRoles.projectSkill:update": "Edit", + "projectRoles.projectSkill:update.tooltip": "Change and save the skills of the project", + "projectRoles.projectSkill:create": "Create", + "projectRoles.projectSkill:create.tooltip": "Add a skill to the project", + "projectRoles.projectSkill:delete": "Delete", + "projectRoles.projectSkill:delete.tooltip": "Remove a skill from the project", "projectRoles.sourceControl:pull": "Pull", "projectRoles.sourceControl:push": "Push", "projectRoles.sourceControl:push.tooltip": "Push changes (workflows, credentials, tags) to the remote repository", @@ -3119,6 +3127,7 @@ "projectRoles.type.dataTable": "Data tables", "projectRoles.type.projectVariable": "Project variables", "projectRoles.type.projectAiPreference": "AI preferences", + "projectRoles.type.projectSkill": "Skills", "projectRoles.type.externalSecretsProvider": "Secrets vaults", "projectRoles.type.externalSecret": "Secrets", "projectRoles.externalSecretsProvider:read": "View", diff --git a/packages/frontend/@n8n/stores/src/rbac.store.ts b/packages/frontend/@n8n/stores/src/rbac.store.ts index 29978075c85..87af2a30764 100644 --- a/packages/frontend/@n8n/stores/src/rbac.store.ts +++ b/packages/frontend/@n8n/stores/src/rbac.store.ts @@ -17,6 +17,8 @@ export const useRBACStore = defineStore(STORES.RBAC, () => { aiAssistant: {}, aiPreference: {}, projectAiPreference: {}, + skill: {}, + projectSkill: {}, workflow: {}, tag: {}, annotationTag: {}, diff --git a/packages/frontend/editor-ui/src/features/roles/project/projectRoleScopes.ts b/packages/frontend/editor-ui/src/features/roles/project/projectRoleScopes.ts index 9185dde47c5..5536eccdf8a 100644 --- a/packages/frontend/editor-ui/src/features/roles/project/projectRoleScopes.ts +++ b/packages/frontend/editor-ui/src/features/roles/project/projectRoleScopes.ts @@ -29,6 +29,7 @@ export const SCOPE_TYPES: ProjectResource[] = [ 'dataTable', 'projectVariable', 'projectAiPreference', + 'projectSkill', 'sourceControl', ];