From 963068b112f2dacfeedc6f370af28053bb99fbb2 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 11 Apr 2026 19:41:50 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=AC=20feat:=20Scaffold=20Skills=20CRUD?= =?UTF-8?q?=20with=20ACL=20Sharing=20and=20File=20Schema=20(#12613)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🧬 feat: Scaffold Skills CRUD with ACL Sharing and File Schema Adds Skills as a new first-class resource modeled on Anthropic's Agent Skills, reusing the existing Prompt ACL stack for sharing. Lays the groundwork for multi-file skills (SkillFile schema + metadata routes) without wiring upload processing β€” single-file skills (inline SKILL.md body) work end-to-end, multi-file uploads are stubbed for phase 2. * πŸ”¬ fix: Wire Skill Cleanup, AccessRole Enum, and Express 5 Path Params CI surfaced four follow-ups from the initial Skills scaffolding commit that local builds missed: - AccessRole's resourceType field had a hardcoded enum that didn't include `'skill'`, blocking SKILL_OWNER/EDITOR/VIEWER role creation in every test that hit the AccessRole model. - The seedDefaultRoles assertion in accessRole.spec.ts hard-listed the expected role IDs and needed the new SKILL_* entries. - deleteUserController had no cleanup for skills, and the deleteUserResourceCoverage guard test enforces every ResourceType has a documented handler β€” wired in db.deleteUserSkills(user._id) and added the entry to HANDLED_RESOURCE_TYPES. - Express 5's path-to-regexp v6 rejects the legacy `(*)` named-group glob syntax. The two skill file routes now use a plain `:relativePath` param; the client already encodeURIComponents the path, so a single param is sufficient and decoded server-side. * πŸͺ‘ fix: Make Skill Name Uniqueness Application-Level Resolve three more CI failures from the Skills scaffolding PR: - Mongoose creates indexes asynchronously and mongodb-memory-server tests can race ahead of the unique (name, author, tenantId) index being built, so the duplicate-name uniqueness test was flaky. Added an explicit findOne pre-check inside createSkill that throws with code 11000 (mimicking the index violation), giving deterministic behavior. The unique index stays as the persistent guarantee. - The deleteUser.spec.js and UserController.spec.js suites mock the ~/models module directly and were missing deleteUserSkills, causing deleteUserController to throw and return 500 instead of 200. - Removed two doc-comment claims that the SKILL_NAME_MAX_LENGTH and SKILL_DESCRIPTION_MAX_LENGTH constants "match Anthropic's API". The values themselves are reasonable but the comments were misleading about who enforces them. * πŸͺ’ fix: Address Code Review Findings on Skills Scaffolding Resolve all 15 findings from the comprehensive PR review: Critical: - Rollback the created skill when grantPermission throws so a transient ACL failure cannot leave an orphaned, inaccessible skill in the DB. - Fix infinite query cache corruption in useUpdateSkillMutation helpers. setQueriesData([QueryKeys.skills]) matches useSkillsInfiniteQuery's InfiniteData cache entries, which have { pages, pageParams } shape β€” spreading data.skills on those would throw. Added an isInfiniteSkillData guard and per-page transform so both flat and infinite caches update correctly. Major: - Fix TUpdateSkillContext type: the public type declared previousListData but onMutate actually returns previousListSnapshots (a [key, value] tuple array). Updated the type + added TSkillCacheEntry as a shared export from data-provider. - Add cancelQueries calls before optimistic update in onMutate so in-flight refetches cannot clobber the optimistic state. - Parallelize deleteUserSkills ACL removal via Promise.allSettled instead of a sequential await loop β€” O(1) round-trip vs O(n). - Stub mockDeleteUserSkills in stubDeletionMocks() and assert it's called with user.id in the deleteUser.spec.js happy-path test. - Add idResolver: getSkillById to the SKILL branch in accessPermissions.js so GET /api/permissions/skill/ returns 404 instead of 403. Minor: - Reuse resolved skill from req.resourceAccess.resourceInfo in getHandler to eliminate a redundant getSkillById call per GET /api/skills/:id. - Reject PATCH /api/skills/:id requests whose body contains only expectedVersion β€” previously they silently bumped version with no changes, triggering spurious 409s for collaborators. - Make TSkill.frontmatter optional (wire type) and add serializeFrontmatter / serializeSourceMetadata helpers that return undefined for empty objects instead of casting incomplete data to SkillFrontmatter. - Standardize deleteUserSkills to accept string | ObjectId and convert internally, matching deleteUserPrompts's signature; UserController now passes user.id consistently. - Replace bumpSkillVersionAndRecount (read-then-write, racy) with bumpSkillVersionAndAdjustFileCount using atomic $inc. upsertSkillFile pre-checks existence to distinguish insert (+1) from replace (0). - Add DELETE /api/skills/:id/files/:relativePath integration tests covering success, 404, and 403 paths. Nits: - Drop trivial resolveSkillId wrapper β€” pass getSkillById directly. - Remove dead staleTime: 1000 * 10 from useListSkillsQuery since all refetch triggers are already disabled. * 🧭 fix: Resolve Second Skills Review Pass β€” Cache, Gate, TOCTOU Address 13 of 14 findings from the second code review; reject #13 as misread of the AGENTS.md import-order rule (package types correctly precede local types regardless of length). Major: - Fix addSkillToCachedLists closure bug: a hoisted `prepended` flag was shared across every cache entry matched by setQueriesData, so concurrent flat + infinite caches would silently drop the prepend on whichever was processed second. Replaced the shared helper with three per-entry inline updaters that handle InfiniteData at the page level (page 0 only for prepend, all pages for replace/remove). - Tighten patchHandler's expectedVersion validation: NaN passes `typeof === 'number'` and would previously leak current skill state via a misleading 409. Now requires finite positive integer and returns 400 otherwise. - Guard decodeURIComponent in deleteFileHandler with try/catch β€” malformed percent encoding now returns 400 instead of 500. - Add PermissionTypes.SKILLS + skillPermissionsSchema + TSkillPermissions in data-provider; seed default SKILLS permissions for ADMIN (all true) and USER (use + create only); wire checkSkillAccess / checkSkillCreate via generateCheckAccess onto the skills router mirroring the prompts pattern. Skills route now enforces role-based capability gates alongside per-resource ACLs. Test suite adds a mocked getRoleByName returning permissive SKILLS. - Fix upsertSkillFile TOCTOU: replaced the pre-check + upsert pair with a single `findOneAndUpdate({ new: false, upsert: true })` call that atomically returns the pre-update doc (null β‡’ insert) so fileCount delta can't double-count on concurrent same-path uploads. Minor: - Add `sourceMetadata` to listSkillsByAccess .select() so summaries no longer silently drop the field for GitHub/Notion-synced skills. - Include `cursor` in useListSkillsQuery's query key so manual pagination doesn't alias across pages. - Clean up TSkillSummary to `Omit` matching what serializeSkillSummary actually emits; drop the Omit-then-re-add noise. - Skip getPublicSkillIdSet in createHandler; a newly-created skill cannot have a PUBLIC ACL entry, so pass an empty set directly instead of paying a DB round-trip. - Trim SkillMethods public surface: drop internal helpers countSkillFiles / deleteSkillFilesBySkillId / getSkillFile from the return object; inline the file cascade into deleteSkill. - Use TSkillConflictResponse at the PATCH 409 call site instead of an inline ad-hoc object literal. - Drop the now-unused EXPECTED_VERSION_ERROR module constant. * 🧩 fix: Extend Role Schema + Types with SKILLS PermissionType CI type-check and unit test failures from the PermissionTypes.SKILLS addition surfaced three unrelated places that all hardcode the permission-type set: - IRole.permissions in data-schemas/types/role.ts enumerates every PermissionTypes key as an optional field. Adding SKILLS to the enum without updating the interface caused TS7053 'expression of type PermissionTypes can't be used to index type' errors in role.methods.spec.ts (lines 407-408, 477-478) because Object.values(PermissionTypes) now yielded a value the interface didn't cover. - schema/role.ts rolePermissionsSchema mirrors the interface at the Mongoose layer; also needed SKILLS added so the persisted role document can actually store skill permissions. - data-provider/roles.spec.ts has a guard test that every permission type carrying CREATE/SHARE/SHARE_PUBLIC must be explicitly "tracked" either in RESOURCE_PERMISSION_TYPES or in the PROMPTS/AGENTS/MEMORIES exemption list. Added SKILLS to the exemption list since skills follow the same default model as prompts/agents (USE + CREATE on for USER, SHARE / SHARE_PUBLIC off). All three are additive pass-throughs with no behavior change. * 🏷️ refactor: Introduce ISkillSummary for Narrow List Projection Follow-up NITs from the second review pass on the Skills PR: - Define ISkillSummary = Omit and use it as the element type in ListSkillsByAccessResult. The list query's .select() intentionally omits body and frontmatter for payload size, but the previous type claimed both fields were present β€” a type lie that would mislead future readers even though serializeSkillSummary never touches those fields at runtime. handlers.ts's signature for serializeSkillSummary now accepts ISkillSummary too. - Document the intentional second-round-trip `findOne` in upsertSkillFile. Switching to `findOneAndUpdate({ new: false })` was required for TOCTOU-safe insert-vs-replace detection, which means the handler needs a follow-up query to return the post-upsert document. A comment now explains the tradeoff so future readers don't silently "optimize" it away. No behavior change. * 🌐 fix: Wire SKILL into SHARE_PUBLIC Resource Maps Address codex comment #1 β€” making a skill public was blocked on two hardcoded resourceβ†’permission-type maps that didn't know about SKILL: - api/server/middleware/checkSharePublicAccess.js's resourceToPermissionType map was missing ResourceType.SKILL, so PUT /api/permissions/skill/:id with { public: true } would fall through to the 400 "Unsupported resource type for public sharing" path even though PermissionTypes.SKILLS exists and ADMIN has SHARE_PUBLIC configured. Added the mapping. - client/src/hooks/Sharing/useCanSharePublic.ts has an identical client-side map used to gate the "Make Public" UI toggle. Without the SKILL mapping the hook returned false for everyone, so the toggle wouldn't render for skills once the sharing UI lands in phase 2. Added the mapping. Codex comment #2 (create/update cache writes inject skills into unrelated filtered lists) is invalid β€” it flags a pattern that mirrors useUpdatePromptGroup (which the PR description explicitly cites as the model) and is a deliberate optimistic-update tradeoff. Trying to match each cache key's embedded filter would couple the mutation callback to query-key internals, which is exactly what setQueriesData is designed to avoid. No change there. * πŸ§ͺ feat: Frontmatter Validation, Reserved-Name Fixes, Coaching Warnings Address the follow-up review notes on the Skills PR. This commit closes the gap between the wire-type promise and what the backend actually enforces, tightens the reserved-name rules, and adds a non-blocking coaching tier for validators. Frontmatter validation (new): - Add `validateSkillFrontmatter` in data-schemas/methods/skill.ts with strict mode β€” unknown keys are rejected so expanding the allowed set is an intentional code change. Known keys are type-checked against a `FrontmatterKind` table derived from Anthropic's Agent Skills spec (name, description, when-to-use, allowed-tools, arguments, argument-hint, user-invocable, disable-model-invocation, model, effort, context, agent, paths, shell, hooks, version, metadata). - `hooks` and `metadata` get a shallow JSON-safety check (max depth 4, max string 2000, max array 100) instead of a full schema, since their full shapes live outside this module. - Wired into BOTH createSkill AND updateSkill so the PATCH path can't smuggle invalid frontmatter past the validator. Validation warning tier (new): - Add optional `severity: 'error' | 'warning'` to `ValidationIssue` (defaults to error). `partitionIssues` splits an issue list into blocking errors and non-blocking warnings. - `createSkill` / `updateSkill` filter on errors for the throw check and return warnings in a new `warnings: ValidationIssue[]` field on their result objects (`CreateSkillResult` / `UpdateSkillResult`). - `validateSkillDescription` now emits a `TOO_SHORT` warning for descriptions under 20 chars β€” the primary triggering field, so a little coaching goes a long way. - `createHandler` / `patchHandler` in packages/api surface the warnings via a new `attachWarnings` helper that decorates the serialized response with a `warnings?: TSkillWarning[]` field. - `TSkill` gains an optional `warnings?: TSkillWarning[]` field documented as "present on POST/PATCH, never on GET". Reserved-name filter (tightened): - Replace the substring match (`.includes('anthropic')`) with prefix matching on `anthropic-` and `claude-` plus exact-match rejection of CLI slash-command collisions (help, clear, compact, model, exit, quit, settings, plus the bare `anthropic` / `claude` words). Both the pure validator (`methods/skill.ts`) and the Mongoose schema validator (`schema/skill.ts`) updated in lockstep; comments on each reference the other to prevent drift. - `research-anthropic-helper` and `about-claude` are now allowed; `anthropic-helper`, `claude-bot`, and `settings` are still rejected. Documentation: - Add docstrings on `ISkill`, `schema/skill.ts`, and `TSkill` explaining the semantics of `name` (Claude-visible identifier, kebab-case, stable), `displayTitle` (UI-only cosmetic label, NOT sent to Claude), `description` (highest-leverage trigger field), and `source` / `sourceMetadata` (reserved for phase 2+ external sync). - Add a detailed consistency comment on `bumpSkillVersionAndAdjustFileCount` explaining that it runs as a separate MongoDB operation from upsertSkillFile/deleteSkillFile, so `fileCount` can drift if the second op fails β€” options listed, tradeoff documented, phase 1 risk window noted as closed because upload is still stubbed. Tests: - data-schemas skill.spec.ts: destructure `{ skill, warnings }` from createSkill at every call site; add a TOO_SHORT warning test, a frontmatter strict-mode test, reserved-prefix tests (including positive cases for substring names that should pass), CLI reserved word tests, and a full `validateSkillFrontmatter` describe block covering unknown keys, type mismatches, and deep-nesting rejection. - api/server/routes/skills.test.js: bump default test description above the 20-char threshold, add a warning-emission test, add reserved-prefix + reserved-CLI-word tests, add an unknown-frontmatter- key test asserting the 400 response carries `issues` with `UNKNOWN_KEY`. * πŸ“¦ fix: Export CreateSkillResult from data-schemas Methods Index `CreateSkillResult` was defined in `methods/skill.ts` and consumed by `packages/api/src/skills/handlers.ts` but never re-exported from the methods barrel, so the type-check job failed with TS2724 "'@librechat/data-schemas' has no exported member named 'CreateSkillResult'". Rollup's bundle-mode build picked up the type via its internal resolver, but the standalone `tsc --noEmit` type-check ran against the package's public entrypoint and couldn't see it. Added the type import + export alongside the existing `UpdateSkillResult` export, which fixes the CI type-check without any runtime change. --- api/server/controllers/UserController.js | 1 + api/server/controllers/UserController.spec.js | 1 + .../controllers/__tests__/deleteUser.spec.js | 6 + .../deleteUserResourceCoverage.spec.js | 1 + api/server/experimental.js | 1 + api/server/index.js | 1 + .../accessResources/canAccessSkillResource.js | 32 + .../middleware/accessResources/index.js | 2 + .../middleware/checkSharePublicAccess.js | 1 + api/server/routes/accessPermissions.js | 9 +- api/server/routes/index.js | 2 + api/server/routes/skills.js | 102 ++ api/server/routes/skills.test.js | 480 ++++++++++ client/src/data-provider/Skills/index.ts | 2 + client/src/data-provider/Skills/mutations.ts | 264 ++++++ client/src/data-provider/Skills/queries.ts | 117 +++ client/src/data-provider/index.ts | 1 + client/src/hooks/Sharing/useCanSharePublic.ts | 1 + packages/api/src/index.ts | 2 + packages/api/src/skills/handlers.ts | 572 ++++++++++++ packages/api/src/skills/index.ts | 1 + .../data-provider/src/accessPermissions.ts | 7 + packages/data-provider/src/api-endpoints.ts | 27 + packages/data-provider/src/data-service.ts | 41 + packages/data-provider/src/index.ts | 1 + packages/data-provider/src/keys.ts | 4 + packages/data-provider/src/permissions.ts | 14 + packages/data-provider/src/roles.spec.ts | 13 +- packages/data-provider/src/roles.ts | 19 + packages/data-provider/src/types/mutations.ts | 48 + packages/data-provider/src/types/skills.ts | 219 +++++ .../data-schemas/src/admin/capabilities.ts | 6 + .../src/methods/accessRole.spec.ts | 3 + .../data-schemas/src/methods/accessRole.ts | 21 + packages/data-schemas/src/methods/index.ts | 31 + .../data-schemas/src/methods/skill.spec.ts | 617 ++++++++++++ packages/data-schemas/src/methods/skill.ts | 883 ++++++++++++++++++ packages/data-schemas/src/models/index.ts | 4 + packages/data-schemas/src/models/skill.ts | 8 + packages/data-schemas/src/models/skillFile.ts | 10 + .../data-schemas/src/schema/accessRole.ts | 2 +- packages/data-schemas/src/schema/role.ts | 6 + packages/data-schemas/src/schema/skill.ts | 183 ++++ packages/data-schemas/src/schema/skillFile.ts | 102 ++ packages/data-schemas/src/types/index.ts | 2 + packages/data-schemas/src/types/role.ts | 6 + packages/data-schemas/src/types/skill.ts | 97 ++ 47 files changed, 3965 insertions(+), 8 deletions(-) create mode 100644 api/server/middleware/accessResources/canAccessSkillResource.js create mode 100644 api/server/routes/skills.js create mode 100644 api/server/routes/skills.test.js create mode 100644 client/src/data-provider/Skills/index.ts create mode 100644 client/src/data-provider/Skills/mutations.ts create mode 100644 client/src/data-provider/Skills/queries.ts create mode 100644 packages/api/src/skills/handlers.ts create mode 100644 packages/api/src/skills/index.ts create mode 100644 packages/data-provider/src/types/skills.ts create mode 100644 packages/data-schemas/src/methods/skill.spec.ts create mode 100644 packages/data-schemas/src/methods/skill.ts create mode 100644 packages/data-schemas/src/models/skill.ts create mode 100644 packages/data-schemas/src/models/skillFile.ts create mode 100644 packages/data-schemas/src/schema/skill.ts create mode 100644 packages/data-schemas/src/schema/skillFile.ts create mode 100644 packages/data-schemas/src/types/skill.ts diff --git a/api/server/controllers/UserController.js b/api/server/controllers/UserController.js index 16b68968d9..939a637563 100644 --- a/api/server/controllers/UserController.js +++ b/api/server/controllers/UserController.js @@ -333,6 +333,7 @@ const deleteUserController = async (req, res) => { await db.deleteConversationTags({ user: user.id }); await db.deleteAllUserMemories(user.id); await db.deleteUserPrompts(user.id); + await db.deleteUserSkills(user.id); await deleteUserMcpServers(user.id); await db.deleteActions({ user: user.id }); await db.deleteTokens({ userId: user.id }); diff --git a/api/server/controllers/UserController.spec.js b/api/server/controllers/UserController.spec.js index 4a96072062..30e6190e28 100644 --- a/api/server/controllers/UserController.spec.js +++ b/api/server/controllers/UserController.spec.js @@ -28,6 +28,7 @@ jest.mock('~/models', () => { deleteAssistants: jest.fn().mockResolvedValue(undefined), deleteUserById: jest.fn().mockResolvedValue(undefined), deleteUserPrompts: jest.fn().mockResolvedValue(undefined), + deleteUserSkills: jest.fn().mockResolvedValue(undefined), deleteMessages: jest.fn().mockResolvedValue(undefined), deleteBalances: jest.fn().mockResolvedValue(undefined), deleteActions: jest.fn().mockResolvedValue(undefined), diff --git a/api/server/controllers/__tests__/deleteUser.spec.js b/api/server/controllers/__tests__/deleteUser.spec.js index a382a6cdc7..bc6acde53d 100644 --- a/api/server/controllers/__tests__/deleteUser.spec.js +++ b/api/server/controllers/__tests__/deleteUser.spec.js @@ -17,6 +17,7 @@ const mockProcessDeleteRequest = jest.fn(); const mockDeleteToolCalls = jest.fn(); const mockDeleteUserAgents = jest.fn(); const mockDeleteUserPrompts = jest.fn(); +const mockDeleteUserSkills = jest.fn(); jest.mock('@librechat/data-schemas', () => ({ logger: { error: jest.fn(), info: jest.fn() }, @@ -56,6 +57,7 @@ jest.mock('~/models', () => ({ deleteToolCalls: (...args) => mockDeleteToolCalls(...args), deleteUserAgents: (...args) => mockDeleteUserAgents(...args), deleteUserPrompts: (...args) => mockDeleteUserPrompts(...args), + deleteUserSkills: (...args) => mockDeleteUserSkills(...args), deleteTransactions: jest.fn(), deleteBalances: jest.fn(), deleteAllAgentApiKeys: jest.fn(), @@ -130,6 +132,7 @@ function stubDeletionMocks() { mockDeleteToolCalls.mockResolvedValue(); mockDeleteUserAgents.mockResolvedValue(); mockDeleteUserPrompts.mockResolvedValue(); + mockDeleteUserSkills.mockResolvedValue(0); } beforeEach(() => { @@ -148,6 +151,9 @@ describe('deleteUserController - 2FA enforcement', () => { expect(res.status).toHaveBeenCalledWith(200); expect(res.send).toHaveBeenCalledWith({ message: 'User deleted' }); expect(mockDeleteMessages).toHaveBeenCalled(); + expect(mockDeleteUserAgents).toHaveBeenCalledWith('user1'); + expect(mockDeleteUserPrompts).toHaveBeenCalledWith('user1'); + expect(mockDeleteUserSkills).toHaveBeenCalledWith('user1'); expect(mockVerifyOTPOrBackupCode).not.toHaveBeenCalled(); }); diff --git a/api/server/controllers/__tests__/deleteUserResourceCoverage.spec.js b/api/server/controllers/__tests__/deleteUserResourceCoverage.spec.js index b08e502800..78fcfa16b0 100644 --- a/api/server/controllers/__tests__/deleteUserResourceCoverage.spec.js +++ b/api/server/controllers/__tests__/deleteUserResourceCoverage.spec.js @@ -15,6 +15,7 @@ const HANDLED_RESOURCE_TYPES = { [ResourceType.REMOTE_AGENT]: 'deleteUserAgents', [ResourceType.PROMPTGROUP]: 'deleteUserPrompts', [ResourceType.MCPSERVER]: 'deleteUserMcpServers', + [ResourceType.SKILL]: 'deleteUserSkills', }; /** diff --git a/api/server/experimental.js b/api/server/experimental.js index 3dfd5aee3f..cd594c1696 100644 --- a/api/server/experimental.js +++ b/api/server/experimental.js @@ -310,6 +310,7 @@ if (cluster.isMaster) { app.use('/api/convos', routes.convos); app.use('/api/presets', routes.presets); app.use('/api/prompts', routes.prompts); + app.use('/api/skills', routes.skills); app.use('/api/categories', routes.categories); app.use('/api/endpoints', routes.endpoints); app.use('/api/balance', routes.balance); diff --git a/api/server/index.js b/api/server/index.js index adeaacdfcc..d798f1a166 100644 --- a/api/server/index.js +++ b/api/server/index.js @@ -168,6 +168,7 @@ const startServer = async () => { app.use('/api/convos', routes.convos); app.use('/api/presets', routes.presets); app.use('/api/prompts', routes.prompts); + app.use('/api/skills', routes.skills); app.use('/api/categories', routes.categories); app.use('/api/endpoints', routes.endpoints); app.use('/api/balance', routes.balance); diff --git a/api/server/middleware/accessResources/canAccessSkillResource.js b/api/server/middleware/accessResources/canAccessSkillResource.js new file mode 100644 index 0000000000..1010ca8998 --- /dev/null +++ b/api/server/middleware/accessResources/canAccessSkillResource.js @@ -0,0 +1,32 @@ +const { ResourceType } = require('librechat-data-provider'); +const { canAccessResource } = require('./canAccessResource'); +const { getSkillById } = require('~/models'); + +/** + * Skill-specific middleware factory that checks skill access permissions. + * Wraps the generic `canAccessResource` with the SKILL resource type and + * `getSkillById` as the ID resolver. + * + * @param {Object} options + * @param {number} options.requiredPermission - Permission bit required (1=view, 2=edit, 4=delete, 8=share) + * @param {string} [options.resourceIdParam='id'] - Route parameter name holding the skill id + * @returns {Function} Express middleware + */ +const canAccessSkillResource = (options) => { + const { requiredPermission, resourceIdParam = 'id' } = options || {}; + + if (!requiredPermission || typeof requiredPermission !== 'number') { + throw new Error('canAccessSkillResource: requiredPermission is required and must be a number'); + } + + return canAccessResource({ + resourceType: ResourceType.SKILL, + requiredPermission, + resourceIdParam, + idResolver: getSkillById, + }); +}; + +module.exports = { + canAccessSkillResource, +}; diff --git a/api/server/middleware/accessResources/index.js b/api/server/middleware/accessResources/index.js index 838834919a..31548411ba 100644 --- a/api/server/middleware/accessResources/index.js +++ b/api/server/middleware/accessResources/index.js @@ -4,6 +4,7 @@ const { canAccessAgentFromBody } = require('./canAccessAgentFromBody'); const { canAccessPromptViaGroup } = require('./canAccessPromptViaGroup'); const { canAccessPromptGroupResource } = require('./canAccessPromptGroupResource'); const { canAccessMCPServerResource } = require('./canAccessMCPServerResource'); +const { canAccessSkillResource } = require('./canAccessSkillResource'); module.exports = { canAccessResource, @@ -12,4 +13,5 @@ module.exports = { canAccessPromptViaGroup, canAccessPromptGroupResource, canAccessMCPServerResource, + canAccessSkillResource, }; diff --git a/api/server/middleware/checkSharePublicAccess.js b/api/server/middleware/checkSharePublicAccess.js index c7b65a077e..945d5521b7 100644 --- a/api/server/middleware/checkSharePublicAccess.js +++ b/api/server/middleware/checkSharePublicAccess.js @@ -10,6 +10,7 @@ const resourceToPermissionType = { [ResourceType.PROMPTGROUP]: PermissionTypes.PROMPTS, [ResourceType.MCPSERVER]: PermissionTypes.MCP_SERVERS, [ResourceType.REMOTE_AGENT]: PermissionTypes.REMOTE_AGENTS, + [ResourceType.SKILL]: PermissionTypes.SKILLS, }; /** diff --git a/api/server/routes/accessPermissions.js b/api/server/routes/accessPermissions.js index 45afec133b..01e3196b8f 100644 --- a/api/server/routes/accessPermissions.js +++ b/api/server/routes/accessPermissions.js @@ -11,7 +11,7 @@ const { const { requireJwtAuth, checkBan, uaParser, canAccessResource } = require('~/server/middleware'); const { checkPeoplePickerAccess } = require('~/server/middleware/checkPeoplePickerAccess'); const { checkSharePublicAccess } = require('~/server/middleware/checkSharePublicAccess'); -const { findMCPServerByObjectId } = require('~/models'); +const { findMCPServerByObjectId, getSkillById } = require('~/models'); const router = express.Router(); @@ -72,6 +72,13 @@ const checkResourcePermissionAccess = (requiredPermission) => (req, res, next) = resourceIdParam: 'resourceId', idResolver: findMCPServerByObjectId, }); + } else if (resourceType === ResourceType.SKILL) { + middleware = canAccessResource({ + resourceType: ResourceType.SKILL, + requiredPermission, + resourceIdParam: 'resourceId', + idResolver: getSkillById, + }); } else { return res.status(400).json({ error: 'Bad Request', diff --git a/api/server/routes/index.js b/api/server/routes/index.js index 1feaf63fdb..cab2f92ed1 100644 --- a/api/server/routes/index.js +++ b/api/server/routes/index.js @@ -13,6 +13,7 @@ const messages = require('./messages'); const memories = require('./memories'); const presets = require('./presets'); const prompts = require('./prompts'); +const skills = require('./skills'); const balance = require('./balance'); const actions = require('./actions'); const apiKeys = require('./apiKeys'); @@ -56,6 +57,7 @@ module.exports = { config, models, prompts, + skills, actions, presets, balance, diff --git a/api/server/routes/skills.js b/api/server/routes/skills.js new file mode 100644 index 0000000000..38d4b38ca5 --- /dev/null +++ b/api/server/routes/skills.js @@ -0,0 +1,102 @@ +const express = require('express'); +const { createSkillsHandlers, generateCheckAccess } = require('@librechat/api'); +const { isValidObjectIdString } = require('@librechat/data-schemas'); +const { PermissionBits, PermissionTypes, Permissions } = require('librechat-data-provider'); +const { + createSkill, + getSkillById, + listSkillsByAccess, + updateSkill, + deleteSkill, + listSkillFiles, + deleteSkillFile, + getRoleByName, +} = require('~/models'); +const { requireJwtAuth, canAccessSkillResource } = require('~/server/middleware'); +const { + findAccessibleResources, + findPubliclyAccessibleResources, + grantPermission, +} = require('~/server/services/PermissionService'); + +const router = express.Router(); + +// Role-based capability gates. Mirrors prompts.js β€” the ACL middleware on each +// route handles per-resource permissions; these check that the caller's role +// is even allowed to use / create skills at all. +const checkSkillAccess = generateCheckAccess({ + permissionType: PermissionTypes.SKILLS, + permissions: [Permissions.USE], + getRoleByName, +}); +const checkSkillCreate = generateCheckAccess({ + permissionType: PermissionTypes.SKILLS, + permissions: [Permissions.USE, Permissions.CREATE], + getRoleByName, +}); + +router.use(requireJwtAuth); +router.use(checkSkillAccess); + +const handlers = createSkillsHandlers({ + createSkill, + getSkillById, + listSkillsByAccess, + updateSkill, + deleteSkill, + listSkillFiles, + deleteSkillFile, + findAccessibleResources, + findPubliclyAccessibleResources, + grantPermission, + isValidObjectIdString, +}); + +router.get('/', handlers.list); +router.post('/', checkSkillCreate, handlers.create); + +router.get( + '/:id', + canAccessSkillResource({ requiredPermission: PermissionBits.VIEW }), + handlers.get, +); + +router.patch( + '/:id', + checkSkillCreate, + canAccessSkillResource({ requiredPermission: PermissionBits.EDIT }), + handlers.patch, +); + +router.delete( + '/:id', + checkSkillCreate, + canAccessSkillResource({ requiredPermission: PermissionBits.DELETE }), + handlers.delete, +); + +router.get( + '/:id/files', + canAccessSkillResource({ requiredPermission: PermissionBits.VIEW }), + handlers.listFiles, +); + +router.post( + '/:id/files', + canAccessSkillResource({ requiredPermission: PermissionBits.EDIT }), + handlers.uploadFileStub, +); + +router.get( + '/:id/files/:relativePath', + canAccessSkillResource({ requiredPermission: PermissionBits.VIEW }), + handlers.downloadFileStub, +); + +router.delete( + '/:id/files/:relativePath', + canAccessSkillResource({ requiredPermission: PermissionBits.EDIT }), + handlers.deleteFile, +); + +module.exports = router; diff --git a/api/server/routes/skills.test.js b/api/server/routes/skills.test.js new file mode 100644 index 0000000000..c4b0c6b8c9 --- /dev/null +++ b/api/server/routes/skills.test.js @@ -0,0 +1,480 @@ +const express = require('express'); +const request = require('supertest'); +const mongoose = require('mongoose'); +const { MongoMemoryServer } = require('mongodb-memory-server'); +const { + SystemRoles, + ResourceType, + AccessRoleIds, + PrincipalType, + PermissionBits, +} = require('librechat-data-provider'); + +jest.mock('~/server/services/Config', () => ({ + getCachedTools: jest.fn().mockResolvedValue({}), +})); + +jest.mock('~/models', () => { + const mongoose = require('mongoose'); + const { createMethods } = require('@librechat/data-schemas'); + const methods = createMethods(mongoose, { + removeAllPermissions: async ({ resourceType, resourceId }) => { + const AclEntry = mongoose.models.AclEntry; + if (AclEntry) { + await AclEntry.deleteMany({ resourceType, resourceId }); + } + }, + }); + // Override getRoleByName to return a permissive SKILLS capability block for all + // test users. The real role seeding relies on `initializeRoles` which this + // suite intentionally skips to keep setup minimal. + return { + ...methods, + getRoleByName: jest.fn(), + }; +}); + +jest.mock('~/server/middleware', () => ({ + requireJwtAuth: (req, res, next) => next(), + canAccessSkillResource: jest.requireActual('~/server/middleware').canAccessSkillResource, +})); + +let app; +let mongoServer; +let skillRoutes; +let Skill; +let SkillFile; +let AclEntry; +let AccessRole; +let User; +let testUsers; +let testRoles; +let grantPermission; +let currentTestUser; + +function setTestUser(user) { + currentTestUser = user; +} + +beforeAll(async () => { + mongoServer = await MongoMemoryServer.create(); + await mongoose.connect(mongoServer.getUri()); + + const dbModels = require('~/db/models'); + Skill = dbModels.Skill; + SkillFile = dbModels.SkillFile; + AclEntry = dbModels.AclEntry; + AccessRole = dbModels.AccessRole; + User = dbModels.User; + + const permissionService = require('~/server/services/PermissionService'); + grantPermission = permissionService.grantPermission; + + await setupTestData(); + + app = express(); + app.use(express.json()); + app.use((req, res, next) => { + if (currentTestUser) { + req.user = { + ...(currentTestUser.toObject ? currentTestUser.toObject() : currentTestUser), + id: currentTestUser._id.toString(), + _id: currentTestUser._id, + name: currentTestUser.name, + role: currentTestUser.role, + }; + } + next(); + }); + + currentTestUser = testUsers.owner; + skillRoutes = require('./skills'); + app.use('/api/skills', skillRoutes); +}); + +afterEach(async () => { + await Skill.deleteMany({}); + await SkillFile.deleteMany({}); + await AclEntry.deleteMany({}); + currentTestUser = testUsers.owner; +}); + +afterAll(async () => { + await mongoose.disconnect(); + await mongoServer.stop(); + jest.clearAllMocks(); +}); + +async function setupTestData() { + testRoles = { + viewer: await AccessRole.create({ + accessRoleId: AccessRoleIds.SKILL_VIEWER, + name: 'Viewer', + resourceType: ResourceType.SKILL, + permBits: PermissionBits.VIEW, + }), + editor: await AccessRole.create({ + accessRoleId: AccessRoleIds.SKILL_EDITOR, + name: 'Editor', + resourceType: ResourceType.SKILL, + permBits: PermissionBits.VIEW | PermissionBits.EDIT, + }), + owner: await AccessRole.create({ + accessRoleId: AccessRoleIds.SKILL_OWNER, + name: 'Owner', + resourceType: ResourceType.SKILL, + permBits: + PermissionBits.VIEW | PermissionBits.EDIT | PermissionBits.DELETE | PermissionBits.SHARE, + }), + }; + + testUsers = { + owner: await User.create({ + name: 'Skill Owner', + email: 'skill-owner@test.com', + role: SystemRoles.USER, + }), + editor: await User.create({ + name: 'Skill Editor', + email: 'skill-editor@test.com', + role: SystemRoles.USER, + }), + noAccess: await User.create({ + name: 'No Access', + email: 'no-access@test.com', + role: SystemRoles.USER, + }), + }; + + const { getRoleByName } = require('~/models'); + getRoleByName.mockImplementation((roleName) => { + if (roleName === SystemRoles.USER || roleName === SystemRoles.ADMIN) { + return { + permissions: { + SKILLS: { + USE: true, + CREATE: true, + SHARE: true, + SHARE_PUBLIC: true, + }, + }, + }; + } + return null; + }); +} + +async function createSkillAsOwner(overrides = {}) { + // Description is deliberately kept above the 20-char short-description + // warning threshold so existing tests don't trip the coaching warning. + const res = await request(app) + .post('/api/skills') + .send({ + name: 'demo-skill', + description: 'A small demo skill used in routing integration tests.', + body: '# Demo', + ...overrides, + }); + return res; +} + +describe('Skill routes', () => { + let errSpy; + beforeEach(() => { + errSpy = jest.spyOn(console, 'error').mockImplementation(); + }); + afterEach(() => errSpy.mockRestore()); + + describe('POST /api/skills', () => { + it('creates a skill and grants SKILL_OWNER ACL', async () => { + const res = await createSkillAsOwner(); + expect(res.status).toBe(201); + expect(res.body._id).toBeDefined(); + expect(res.body.version).toBe(1); + expect(res.body.name).toBe('demo-skill'); + // No warnings on a description that comfortably clears the threshold. + expect(res.body.warnings).toBeUndefined(); + + const acl = await AclEntry.findOne({ + resourceType: ResourceType.SKILL, + resourceId: res.body._id, + principalType: PrincipalType.USER, + principalId: testUsers.owner._id, + }); + expect(acl).toBeTruthy(); + expect(acl.roleId.toString()).toBe(testRoles.owner._id.toString()); + }); + + it('attaches a TOO_SHORT warning on create when description is under 20 chars', async () => { + const res = await createSkillAsOwner({ + name: 'short-desc-skill', + description: 'Too short.', + }); + expect(res.status).toBe(201); + expect(res.body._id).toBeDefined(); + expect(Array.isArray(res.body.warnings)).toBe(true); + expect(res.body.warnings).toEqual([ + expect.objectContaining({ + field: 'description', + code: 'TOO_SHORT', + severity: 'warning', + }), + ]); + }); + + it('rejects names starting with reserved brand prefixes', async () => { + const anthropic = await createSkillAsOwner({ name: 'anthropic-helper' }); + expect(anthropic.status).toBe(400); + const claude = await createSkillAsOwner({ name: 'claude-helper' }); + expect(claude.status).toBe(400); + }); + + it('allows names that merely contain reserved brand words as substrings', async () => { + const res = await createSkillAsOwner({ name: 'research-anthropic-helper' }); + expect(res.status).toBe(201); + }); + + it('rejects reserved CLI command names', async () => { + const res = await createSkillAsOwner({ name: 'settings' }); + expect(res.status).toBe(400); + }); + + it('rejects frontmatter with unknown keys', async () => { + const res = await createSkillAsOwner({ + name: 'bad-frontmatter-skill', + frontmatter: { 'not-a-real-key': 'value' }, + }); + expect(res.status).toBe(400); + expect(res.body.issues).toEqual( + expect.arrayContaining([expect.objectContaining({ code: 'UNKNOWN_KEY' })]), + ); + }); + + it('rejects missing description with 400', async () => { + const res = await request(app).post('/api/skills').send({ name: 'x-skill', body: '' }); + expect(res.status).toBe(400); + }); + + it('rejects invalid name with 400 validation failure', async () => { + const res = await createSkillAsOwner({ name: 'BAD NAME' }); + expect(res.status).toBe(400); + expect(res.body.issues).toBeDefined(); + }); + + it('rejects duplicate names with 409', async () => { + const a = await createSkillAsOwner(); + expect(a.status).toBe(201); + const b = await createSkillAsOwner(); + expect(b.status).toBe(409); + }); + }); + + describe('GET /api/skills', () => { + it('returns only skills the caller can access', async () => { + const mine = await createSkillAsOwner({ name: 'mine-skill' }); + expect(mine.status).toBe(201); + + setTestUser(testUsers.noAccess); + const other = await createSkillAsOwner({ name: 'other-skill' }); + expect(other.status).toBe(201); + // Note: the user middleware grants owner perms to whichever user created, so both + // users see their own skill only. + + setTestUser(testUsers.owner); + const res = await request(app).get('/api/skills'); + expect(res.status).toBe(200); + expect(res.body.skills.length).toBe(1); + expect(res.body.skills[0].name).toBe('mine-skill'); + }); + }); + + describe('GET /api/skills/:id', () => { + it('returns 403 when the user has no access', async () => { + const created = await createSkillAsOwner(); + expect(created.status).toBe(201); + setTestUser(testUsers.noAccess); + const res = await request(app).get(`/api/skills/${created.body._id}`); + expect(res.status).toBe(403); + }); + + it('returns the skill to the owner with isPublic flag', async () => { + const created = await createSkillAsOwner(); + const res = await request(app).get(`/api/skills/${created.body._id}`); + expect(res.status).toBe(200); + expect(res.body.name).toBe('demo-skill'); + expect(res.body.isPublic).toBe(false); + }); + }); + + describe('PATCH /api/skills/:id (optimistic concurrency)', () => { + it('updates with correct expectedVersion and bumps version', async () => { + const created = await createSkillAsOwner(); + const res = await request(app) + .patch(`/api/skills/${created.body._id}`) + .send({ expectedVersion: 1, description: 'Updated description' }); + expect(res.status).toBe(200); + expect(res.body.version).toBe(2); + expect(res.body.description).toBe('Updated description'); + }); + + it('returns 409 on stale expectedVersion', async () => { + const created = await createSkillAsOwner(); + const first = await request(app) + .patch(`/api/skills/${created.body._id}`) + .send({ expectedVersion: 1, description: 'First' }); + expect(first.status).toBe(200); + + const stale = await request(app) + .patch(`/api/skills/${created.body._id}`) + .send({ expectedVersion: 1, description: 'Stale' }); + expect(stale.status).toBe(409); + expect(stale.body.error).toBe('skill_version_conflict'); + expect(stale.body.current.version).toBe(2); + }); + + it('rejects updates without expectedVersion', async () => { + const created = await createSkillAsOwner(); + const res = await request(app) + .patch(`/api/skills/${created.body._id}`) + .send({ description: 'no version' }); + expect(res.status).toBe(400); + }); + + it('returns 403 for a user without EDIT permission', async () => { + const created = await createSkillAsOwner(); + setTestUser(testUsers.noAccess); + const res = await request(app) + .patch(`/api/skills/${created.body._id}`) + .send({ expectedVersion: 1, description: 'nope' }); + expect(res.status).toBe(403); + }); + }); + + describe('DELETE /api/skills/:id', () => { + it('deletes and cascades ACL entries', async () => { + const created = await createSkillAsOwner(); + const res = await request(app).delete(`/api/skills/${created.body._id}`); + expect(res.status).toBe(200); + expect(res.body.deleted).toBe(true); + + const remainingAcl = await AclEntry.countDocuments({ + resourceType: ResourceType.SKILL, + resourceId: created.body._id, + }); + expect(remainingAcl).toBe(0); + }); + + it('returns 403 for a non-owner', async () => { + const created = await createSkillAsOwner(); + setTestUser(testUsers.noAccess); + const res = await request(app).delete(`/api/skills/${created.body._id}`); + expect(res.status).toBe(403); + }); + }); + + describe('GET /api/skills/:id/files', () => { + it('returns an empty list for a skill with no files', async () => { + const created = await createSkillAsOwner(); + const res = await request(app).get(`/api/skills/${created.body._id}/files`); + expect(res.status).toBe(200); + expect(res.body.files).toEqual([]); + }); + }); + + describe('POST /api/skills/:id/files (stub)', () => { + it('returns 501 with phase marker', async () => { + const created = await createSkillAsOwner(); + const res = await request(app).post(`/api/skills/${created.body._id}/files`); + expect(res.status).toBe(501); + expect(res.body.phase).toBe(2); + }); + }); + + describe('GET /api/skills/:id/files/:relativePath (stub)', () => { + it('returns 501 stub', async () => { + const created = await createSkillAsOwner(); + const res = await request(app).get( + `/api/skills/${created.body._id}/files/scripts%2Fparse.sh`, + ); + expect(res.status).toBe(501); + }); + }); + + describe('DELETE /api/skills/:id/files/:relativePath', () => { + const { upsertSkillFile } = require('~/models'); + + it('deletes an existing skill file, bumps skill version, and returns 200', async () => { + const created = await createSkillAsOwner(); + await upsertSkillFile({ + skillId: created.body._id, + relativePath: 'scripts/parse.sh', + file_id: 'file-1', + filename: 'parse.sh', + filepath: '/tmp/parse.sh', + source: 'local', + mimeType: 'text/x-shellscript', + bytes: 42, + author: testUsers.owner._id, + }); + + const beforeSkill = await request(app).get(`/api/skills/${created.body._id}`); + expect(beforeSkill.body.fileCount).toBe(1); + expect(beforeSkill.body.version).toBe(2); + + const res = await request(app).delete( + `/api/skills/${created.body._id}/files/scripts%2Fparse.sh`, + ); + expect(res.status).toBe(200); + expect(res.body).toEqual({ + skillId: created.body._id, + relativePath: 'scripts/parse.sh', + deleted: true, + }); + + const afterSkill = await request(app).get(`/api/skills/${created.body._id}`); + expect(afterSkill.body.fileCount).toBe(0); + expect(afterSkill.body.version).toBe(3); + }); + + it('returns 404 when the file does not exist', async () => { + const created = await createSkillAsOwner(); + const res = await request(app).delete( + `/api/skills/${created.body._id}/files/scripts%2Fmissing.sh`, + ); + expect(res.status).toBe(404); + }); + + it('returns 403 for a non-owner', async () => { + const created = await createSkillAsOwner(); + setTestUser(testUsers.noAccess); + const res = await request(app).delete( + `/api/skills/${created.body._id}/files/scripts%2Fparse.sh`, + ); + expect(res.status).toBe(403); + }); + }); + + describe('Sharing via ACL (editor grant)', () => { + it('allows an editor to patch a shared skill', async () => { + const created = await createSkillAsOwner(); + await grantPermission({ + principalType: PrincipalType.USER, + principalId: testUsers.editor._id, + resourceType: ResourceType.SKILL, + resourceId: created.body._id, + accessRoleId: AccessRoleIds.SKILL_EDITOR, + grantedBy: testUsers.owner._id, + }); + + setTestUser(testUsers.editor); + const res = await request(app) + .patch(`/api/skills/${created.body._id}`) + .send({ expectedVersion: 1, description: 'Edited by editor' }); + expect(res.status).toBe(200); + + // Editor should NOT be able to delete + const del = await request(app).delete(`/api/skills/${created.body._id}`); + expect(del.status).toBe(403); + }); + }); +}); diff --git a/client/src/data-provider/Skills/index.ts b/client/src/data-provider/Skills/index.ts new file mode 100644 index 0000000000..d0720956a0 --- /dev/null +++ b/client/src/data-provider/Skills/index.ts @@ -0,0 +1,2 @@ +export * from './queries'; +export * from './mutations'; diff --git a/client/src/data-provider/Skills/mutations.ts b/client/src/data-provider/Skills/mutations.ts new file mode 100644 index 0000000000..473d13ff54 --- /dev/null +++ b/client/src/data-provider/Skills/mutations.ts @@ -0,0 +1,264 @@ +import { useMutation, useQueryClient } from '@tanstack/react-query'; +import { QueryKeys, dataService } from 'librechat-data-provider'; +import type { InfiniteData, QueryKey, UseMutationResult } from '@tanstack/react-query'; +import type { + TSkill, + TSkillFile, + TCreateSkill, + TUpdateSkillVariables, + TUpdateSkillResponse, + TDeleteSkillResponse, + TSkillListResponse, + TSkillCacheEntry, + TUploadSkillFileVariables, + TDeleteSkillFileVariables, + TDeleteSkillFileResponse, + TListSkillFilesResponse, + CreateSkillOptions, + UpdateSkillOptions, + DeleteSkillOptions, + UploadSkillFileOptions, + DeleteSkillFileOptions, +} from 'librechat-data-provider'; + +function isInfiniteSkillData( + data: TSkillListResponse | InfiniteData, +): data is InfiniteData { + return Array.isArray((data as InfiniteData).pages); +} + +/** + * Prepend a newly-created skill into every cached skill list. For infinite + * queries the new skill is inserted at the top of page 0 only (it belongs + * strictly "before" every other page by the cursor contract). For flat + * queries it's prepended to the single page. + * + * This mirrors the established prompts pattern (`useUpdatePromptGroup`): we + * intentionally write across every `[QueryKeys.skills]` entry regardless of + * each entry's embedded filter. In exchange for instant visual feedback on + * every open list view, a filtered view may briefly show a non-matching skill + * until the next explicit refetch. Trying to match filters from a mutation + * callback would couple the hook to every query-key variant's internals, + * which is exactly what React Query's `setQueriesData` is designed to avoid. + */ +function addSkillToCachedLists( + queryClient: ReturnType, + skill: TSkill, +): void { + queryClient.setQueriesData([QueryKeys.skills], (data) => { + if (!data) return data; + if (isInfiniteSkillData(data)) { + return { + ...data, + pages: data.pages.map((page, i) => + i === 0 ? { ...page, skills: [skill, ...page.skills] } : page, + ), + }; + } + return { ...data, skills: [skill, ...data.skills] }; + }); +} + +/** Replace a skill by id in every cached page that contains it. */ +function replaceSkillInCachedLists( + queryClient: ReturnType, + skill: TSkill, +): void { + queryClient.setQueriesData([QueryKeys.skills], (data) => { + if (!data) return data; + if (isInfiniteSkillData(data)) { + return { + ...data, + pages: data.pages.map((page) => ({ + ...page, + skills: page.skills.map((existing) => (existing._id === skill._id ? skill : existing)), + })), + }; + } + return { + ...data, + skills: data.skills.map((existing) => (existing._id === skill._id ? skill : existing)), + }; + }); +} + +/** + * Remove a deleted skill id from every cached page that contains it. Id-based + * removal is always safe regardless of filter β€” a deleted skill never belongs + * in any list. + */ +function removeSkillFromCachedLists( + queryClient: ReturnType, + id: string, +): void { + queryClient.setQueriesData([QueryKeys.skills], (data) => { + if (!data) return data; + if (isInfiniteSkillData(data)) { + return { + ...data, + pages: data.pages.map((page) => ({ + ...page, + skills: page.skills.filter((s) => s._id !== id), + })), + }; + } + return { ...data, skills: data.skills.filter((s) => s._id !== id) }; + }); +} + +/** + * Create a new skill. On success, writes the new skill into both the detail and + * list caches so any open listing UI updates immediately. + */ +export const useCreateSkillMutation = ( + options?: CreateSkillOptions, +): UseMutationResult => { + const queryClient = useQueryClient(); + const { onSuccess, ...rest } = options ?? {}; + return useMutation({ + mutationFn: (payload: TCreateSkill) => dataService.createSkill(payload), + ...rest, + onSuccess: (skill, variables, context) => { + queryClient.setQueryData([QueryKeys.skill, skill._id], skill); + addSkillToCachedLists(queryClient, skill); + if (onSuccess) onSuccess(skill, variables, context); + }, + }); +}; + +/** + * Update a skill. Uses optimistic updates mirroring `useUpdatePromptGroup`: + * - cancel in-flight queries so a late refetch can't clobber the optimistic state + * - snapshot the previous skill + list data in `onMutate` + * - apply the patch locally + * - roll back on error + * - replace with server state on success (captures the new version) + */ +export const useUpdateSkillMutation = ( + options?: UpdateSkillOptions, +): UseMutationResult => { + const queryClient = useQueryClient(); + const { onMutate, onError, onSuccess, ...rest } = options ?? {}; + return useMutation({ + mutationFn: (vars: TUpdateSkillVariables) => dataService.updateSkill(vars), + ...rest, + onMutate: async (variables) => { + // Prevent in-flight refetches from overwriting the optimistic update. + await queryClient.cancelQueries([QueryKeys.skill, variables.id]); + await queryClient.cancelQueries([QueryKeys.skills]); + + const previousSkill = queryClient.getQueryData([QueryKeys.skill, variables.id]); + const previousListSnapshots = queryClient.getQueriesData([ + QueryKeys.skills, + ]) as Array<[QueryKey, TSkillCacheEntry]>; + + if (previousSkill) { + const optimistic: TSkill = { + ...previousSkill, + ...variables.payload, + frontmatter: { + ...(previousSkill.frontmatter ?? {}), + ...(variables.payload.frontmatter ?? {}), + } as TSkill['frontmatter'], + }; + queryClient.setQueryData([QueryKeys.skill, variables.id], optimistic); + replaceSkillInCachedLists(queryClient, optimistic); + } + + const userContext = await onMutate?.(variables); + return { previousSkill, previousListSnapshots, userContext }; + }, + onError: (error, variables, context) => { + if (context?.previousSkill) { + queryClient.setQueryData([QueryKeys.skill, variables.id], context.previousSkill); + } + if (context?.previousListSnapshots) { + for (const [key, value] of context.previousListSnapshots) { + queryClient.setQueryData(key, value); + } + } + if (onError) onError(error, variables, context); + }, + onSuccess: (skill, variables, context) => { + queryClient.setQueryData([QueryKeys.skill, skill._id], skill); + replaceSkillInCachedLists(queryClient, skill); + if (onSuccess) onSuccess(skill, variables, context); + }, + }); +}; + +/** + * Delete a skill. Removes it from caches and invalidates its file list cache. + */ +export const useDeleteSkillMutation = ( + options?: DeleteSkillOptions, +): UseMutationResult => { + const queryClient = useQueryClient(); + const { onSuccess, ...rest } = options ?? {}; + return useMutation({ + mutationFn: ({ id }: { id: string }) => dataService.deleteSkill(id), + ...rest, + onSuccess: (response, variables, context) => { + queryClient.removeQueries([QueryKeys.skill, variables.id]); + queryClient.removeQueries([QueryKeys.skillFiles, variables.id]); + removeSkillFromCachedLists(queryClient, variables.id); + if (onSuccess) onSuccess(response, variables, context); + }, + }); +}; + +/** + * Upload a file into a skill. Stubbed in phase 1 β€” the backend responds 501. + * The hook is wired now so the frontend can call it once the backend is ready. + */ +export const useUploadSkillFileMutation = ( + options?: UploadSkillFileOptions, +): UseMutationResult => { + const queryClient = useQueryClient(); + const { onSuccess, ...rest } = options ?? {}; + return useMutation({ + mutationFn: ({ skillId, formData }: TUploadSkillFileVariables) => + dataService.uploadSkillFile(skillId, formData), + ...rest, + onSuccess: (skillFile, variables, context) => { + queryClient.setQueryData( + [QueryKeys.skillFiles, variables.skillId], + (prev) => { + if (!prev) return { files: [skillFile] }; + const filtered = prev.files.filter((f) => f.relativePath !== skillFile.relativePath); + return { files: [...filtered, skillFile] }; + }, + ); + queryClient.invalidateQueries([QueryKeys.skill, variables.skillId]); + if (onSuccess) onSuccess(skillFile, variables, context); + }, + }); +}; + +/** + * Delete a file from a skill. Works in phase 1 β€” only the SkillFile row is removed. + */ +export const useDeleteSkillFileMutation = ( + options?: DeleteSkillFileOptions, +): UseMutationResult => { + const queryClient = useQueryClient(); + const { onSuccess, ...rest } = options ?? {}; + return useMutation({ + mutationFn: ({ skillId, relativePath }: TDeleteSkillFileVariables) => + dataService.deleteSkillFile(skillId, relativePath), + ...rest, + onSuccess: (response, variables, context) => { + queryClient.setQueryData( + [QueryKeys.skillFiles, variables.skillId], + (prev) => { + if (!prev) return prev; + return { + files: prev.files.filter((f) => f.relativePath !== variables.relativePath), + }; + }, + ); + queryClient.invalidateQueries([QueryKeys.skill, variables.skillId]); + if (onSuccess) onSuccess(response, variables, context); + }, + }); +}; diff --git a/client/src/data-provider/Skills/queries.ts b/client/src/data-provider/Skills/queries.ts new file mode 100644 index 0000000000..ecbad1a342 --- /dev/null +++ b/client/src/data-provider/Skills/queries.ts @@ -0,0 +1,117 @@ +import { useQuery, useInfiniteQuery } from '@tanstack/react-query'; +import { QueryKeys, dataService } from 'librechat-data-provider'; +import type { + QueryObserverResult, + UseQueryOptions, + UseInfiniteQueryOptions, +} from '@tanstack/react-query'; +import type { + TSkill, + TSkillListRequest, + TSkillListResponse, + TListSkillFilesResponse, +} from 'librechat-data-provider'; + +/** + * Paginated skill list (single page) β€” use this for small lists or when you want to + * control pagination manually. + */ +export const useListSkillsQuery = ( + params?: TSkillListRequest, + config?: UseQueryOptions, +): QueryObserverResult => { + return useQuery( + [ + QueryKeys.skills, + params?.category ?? '', + params?.search ?? '', + params?.limit ?? 20, + params?.cursor ?? '', + ], + () => dataService.listSkills(params), + { + refetchOnWindowFocus: false, + refetchOnReconnect: false, + refetchOnMount: false, + ...config, + }, + ); +}; + +/** + * Cursor-paginated infinite query for skills. Use this for the main skills listing UI. + */ +export const useSkillsInfiniteQuery = ( + params?: Omit, + config?: UseInfiniteQueryOptions, +) => { + return useInfiniteQuery( + [ + QueryKeys.skills, + 'infinite', + params?.category ?? '', + params?.search ?? '', + params?.limit ?? 20, + ], + ({ pageParam }) => { + const request: TSkillListRequest = { + category: params?.category, + search: params?.search, + limit: params?.limit, + }; + if (typeof pageParam === 'string' && pageParam.length > 0) { + request.cursor = pageParam; + } + return dataService.listSkills(request); + }, + { + getNextPageParam: (lastPage) => + lastPage.has_more && lastPage.after ? lastPage.after : undefined, + refetchOnWindowFocus: false, + refetchOnReconnect: false, + refetchOnMount: false, + ...config, + }, + ); +}; + +/** + * Fetch a single skill by id (includes full body + frontmatter). + */ +export const useGetSkillQuery = ( + id: string | null | undefined, + config?: UseQueryOptions, +): QueryObserverResult => { + const enabled = !!id; + return useQuery([QueryKeys.skill, id], () => dataService.getSkill(id as string), { + refetchOnWindowFocus: false, + refetchOnReconnect: false, + refetchOnMount: false, + retry: false, + ...config, + enabled: enabled && (config?.enabled ?? true), + }); +}; + +/** + * List file metadata for a single skill. In phase 1 this returns an empty array for + * skills that have only an inline `SKILL.md`; multi-file skills arrive in phase 2. + */ +export const useListSkillFilesQuery = ( + skillId: string | null | undefined, + config?: UseQueryOptions, +): QueryObserverResult => { + const enabled = !!skillId; + return useQuery( + [QueryKeys.skillFiles, skillId], + () => dataService.listSkillFiles(skillId as string), + { + refetchOnWindowFocus: false, + refetchOnReconnect: false, + refetchOnMount: false, + retry: false, + ...config, + enabled: enabled && (config?.enabled ?? true), + }, + ); +}; diff --git a/client/src/data-provider/index.ts b/client/src/data-provider/index.ts index bfc87bb232..8d1e38f3a5 100644 --- a/client/src/data-provider/index.ts +++ b/client/src/data-provider/index.ts @@ -12,6 +12,7 @@ export * from './Favorites'; export * from './mutations'; export * from './prompts'; export * from './queries'; +export * from './Skills'; export * from './roles'; export * from './tags'; export * from './MCP'; diff --git a/client/src/hooks/Sharing/useCanSharePublic.ts b/client/src/hooks/Sharing/useCanSharePublic.ts index 54355dd9a8..e93a3dcc82 100644 --- a/client/src/hooks/Sharing/useCanSharePublic.ts +++ b/client/src/hooks/Sharing/useCanSharePublic.ts @@ -6,6 +6,7 @@ const resourceToPermissionMap: Partial> = [ResourceType.PROMPTGROUP]: PermissionTypes.PROMPTS, [ResourceType.MCPSERVER]: PermissionTypes.MCP_SERVERS, [ResourceType.REMOTE_AGENT]: PermissionTypes.REMOTE_AGENTS, + [ResourceType.SKILL]: PermissionTypes.SKILLS, }; /** diff --git a/packages/api/src/index.ts b/packages/api/src/index.ts index d4b6ac9542..6369ce6ed4 100644 --- a/packages/api/src/index.ts +++ b/packages/api/src/index.ts @@ -37,6 +37,8 @@ export * from './memory'; export * from './agents'; /* Prompts */ export * from './prompts'; +/* Skills */ +export * from './skills'; /* Endpoints */ export * from './endpoints'; /* Files */ diff --git a/packages/api/src/skills/handlers.ts b/packages/api/src/skills/handlers.ts new file mode 100644 index 0000000000..3b044a8a9d --- /dev/null +++ b/packages/api/src/skills/handlers.ts @@ -0,0 +1,572 @@ +import { logger } from '@librechat/data-schemas'; +import { + ResourceType, + AccessRoleIds, + PrincipalType, + PermissionBits, +} from 'librechat-data-provider'; +import type { Response } from 'express'; +import type { Types } from 'mongoose'; +import type { + ISkill, + ISkillFile, + ISkillSummary, + CreateSkillInput, + CreateSkillResult, + UpdateSkillInput, + ListSkillsByAccessResult, + UpdateSkillResult, + ValidationIssue, +} from '@librechat/data-schemas'; +import type { + TSkill, + TSkillFile, + TSkillSummary, + TSkillWarning, + TCreateSkill, + TUpdateSkillPayload, + TListSkillFilesResponse, + TDeleteSkillResponse, + TDeleteSkillFileResponse, + TSkillConflictResponse, +} from 'librechat-data-provider'; +import type { ServerRequest } from '~/types/http'; + +/** Thin error shape the skill methods throw on validation failure. */ +type SkillValidationError = Error & { code?: string; issues?: ValidationIssue[] }; + +/** Mongo duplicate-key shape. */ +type DuplicateKeyError = Error & { code?: number | string }; + +/** + * All dependencies required to serve skill HTTP requests. Every dep is resolved + * from the legacy api layer (`~/models`, `PermissionService`) so the TS handlers + * stay pure β€” no direct imports of mongoose, no direct filesystem I/O. + */ +export interface SkillsHandlersDeps { + /** Skill CRUD β€” from `@librechat/data-schemas` `createMethods` output. */ + createSkill: (data: CreateSkillInput) => Promise; + getSkillById: (id: string | Types.ObjectId) => Promise<(ISkill & { _id: Types.ObjectId }) | null>; + listSkillsByAccess: (params: { + accessibleIds: Types.ObjectId[]; + category?: string; + search?: string; + limit: number; + cursor?: string | null; + }) => Promise; + updateSkill: (params: { + id: string; + expectedVersion: number; + update: UpdateSkillInput; + }) => Promise; + deleteSkill: (id: string) => Promise<{ deleted: boolean }>; + listSkillFiles: ( + skillId: string | Types.ObjectId, + ) => Promise>; + deleteSkillFile: ( + skillId: string | Types.ObjectId, + relativePath: string, + ) => Promise<{ deleted: boolean }>; + + /** Access-control primitives from PermissionService. */ + findAccessibleResources: (params: { + userId: string; + role?: string | null; + resourceType: string; + requiredPermissions: number; + }) => Promise; + findPubliclyAccessibleResources: (params: { + resourceType: string; + requiredPermissions: number; + }) => Promise; + grantPermission: (params: { + principalType: string; + principalId: string | Types.ObjectId; + resourceType: string; + resourceId: string | Types.ObjectId; + accessRoleId: string; + grantedBy: string | Types.ObjectId; + }) => Promise; + + /** ObjectId validation helper from data-schemas. */ + isValidObjectIdString: (value: unknown) => boolean; +} + +/** + * Narrow an opaque `Record` frontmatter (as stored in Mongoose + * `Mixed`) to the wire type. Returns `undefined` for empty/missing frontmatter + * so clients see a clear absence instead of an empty `{}`. + */ +function serializeFrontmatter( + frontmatter: Record | undefined, +): TSkill['frontmatter'] { + if (!frontmatter || typeof frontmatter !== 'object' || Object.keys(frontmatter).length === 0) { + return undefined; + } + return frontmatter as TSkill['frontmatter']; +} + +function serializeSourceMetadata( + metadata: Record | undefined, +): TSkill['sourceMetadata'] { + if (!metadata || typeof metadata !== 'object' || Object.keys(metadata).length === 0) { + return undefined; + } + return metadata as TSkill['sourceMetadata']; +} + +/** Converts a skill document to the wire format returned by the API. */ +function serializeSkill(skill: ISkill & { _id: Types.ObjectId }, publicSet: Set): TSkill { + return { + _id: skill._id.toString(), + name: skill.name, + displayTitle: skill.displayTitle, + description: skill.description, + body: skill.body, + frontmatter: serializeFrontmatter(skill.frontmatter), + category: skill.category, + author: skill.author.toString(), + authorName: skill.authorName, + version: skill.version, + source: skill.source, + sourceMetadata: serializeSourceMetadata(skill.sourceMetadata), + fileCount: skill.fileCount, + isPublic: publicSet.has(skill._id.toString()), + tenantId: skill.tenantId, + createdAt: (skill.createdAt ?? new Date()).toISOString(), + updatedAt: (skill.updatedAt ?? new Date()).toISOString(), + }; +} + +function serializeSkillSummary( + skill: ISkillSummary & { _id: Types.ObjectId }, + publicSet: Set, +): TSkillSummary { + return { + _id: skill._id.toString(), + name: skill.name, + displayTitle: skill.displayTitle, + description: skill.description, + category: skill.category, + author: skill.author.toString(), + authorName: skill.authorName, + version: skill.version, + source: skill.source, + sourceMetadata: serializeSourceMetadata(skill.sourceMetadata), + fileCount: skill.fileCount, + isPublic: publicSet.has(skill._id.toString()), + tenantId: skill.tenantId, + createdAt: (skill.createdAt ?? new Date()).toISOString(), + updatedAt: (skill.updatedAt ?? new Date()).toISOString(), + }; +} + +function serializeSkillFile(file: ISkillFile & { _id: Types.ObjectId }): TSkillFile { + return { + _id: file._id.toString(), + skillId: file.skillId.toString(), + relativePath: file.relativePath, + file_id: file.file_id, + filename: file.filename, + filepath: file.filepath, + source: file.source as TSkillFile['source'], + mimeType: file.mimeType, + bytes: file.bytes, + category: file.category, + isExecutable: file.isExecutable, + author: file.author.toString(), + tenantId: file.tenantId, + createdAt: (file.createdAt ?? new Date()).toISOString(), + updatedAt: (file.updatedAt ?? new Date()).toISOString(), + }; +} + +/** + * Attach non-blocking coaching warnings to a serialized skill response. + * Called from `createHandler` and `patchHandler` so clients can show inline + * feedback (e.g. "description too short") without the write being rejected. + * Only warning-severity issues come through here β€” errors are thrown by + * `createSkill`/`updateSkill` before we reach this point. + */ +function attachWarnings(skill: TSkill, warnings: ValidationIssue[]): TSkill { + if (!warnings || warnings.length === 0) { + return skill; + } + return { + ...skill, + warnings: warnings.map((w) => ({ + field: w.field, + code: w.code, + message: w.message, + severity: 'warning' as const, + })) satisfies TSkillWarning[], + }; +} + +function isValidationError(error: unknown): error is SkillValidationError { + if (!error || typeof error !== 'object') { + return false; + } + const code = (error as { code?: unknown }).code; + return code === 'SKILL_VALIDATION_FAILED' || code === 'SKILL_FILE_VALIDATION_FAILED'; +} + +function isDuplicateKeyError(error: unknown): error is DuplicateKeyError { + if (!error || typeof error !== 'object') { + return false; + } + return (error as { code?: unknown }).code === 11000; +} + +function parseLimit(raw: unknown): number { + const parsed = parseInt(String(raw ?? '20'), 10); + if (Number.isNaN(parsed)) { + return 20; + } + return Math.min(Math.max(1, parsed), 100); +} + +/** + * Factory for the typed Express handlers served at `/api/skills`. + * The legacy `api/server/routes/skills.js` imports this, passes in concrete + * deps from `~/models` + `PermissionService`, and wires the returned handlers + * onto the Express router. + */ +export function createSkillsHandlers(deps: SkillsHandlersDeps) { + const { + createSkill, + getSkillById, + listSkillsByAccess, + updateSkill, + deleteSkill, + listSkillFiles, + deleteSkillFile, + findAccessibleResources, + findPubliclyAccessibleResources, + grantPermission, + isValidObjectIdString, + } = deps; + + async function getPublicSkillIdSet(): Promise> { + try { + const publicIds = await findPubliclyAccessibleResources({ + resourceType: ResourceType.SKILL, + requiredPermissions: PermissionBits.VIEW, + }); + return new Set(publicIds.map((id) => id.toString())); + } catch (error) { + logger.error('[skills] Failed to fetch public skill IDs', error); + return new Set(); + } + } + + async function listHandler(req: ServerRequest, res: Response) { + try { + const user = req.user; + if (!user || !user.id) { + return res.status(401).json({ error: 'Authentication required' }); + } + const { category, search, limit, cursor } = req.query as { + category?: string; + search?: string; + limit?: string; + cursor?: string; + }; + const parsedLimit = parseLimit(limit); + + const [accessibleIds, publicIds] = await Promise.all([ + findAccessibleResources({ + userId: user.id, + role: user.role, + resourceType: ResourceType.SKILL, + requiredPermissions: PermissionBits.VIEW, + }), + findPubliclyAccessibleResources({ + resourceType: ResourceType.SKILL, + requiredPermissions: PermissionBits.VIEW, + }), + ]); + + const mergedIds = Array.from( + new Map([...accessibleIds, ...publicIds].map((id) => [id.toString(), id])).values(), + ); + + const result = await listSkillsByAccess({ + accessibleIds: mergedIds, + category: typeof category === 'string' && category.length > 0 ? category : undefined, + search: typeof search === 'string' && search.length > 0 ? search : undefined, + limit: parsedLimit, + cursor: typeof cursor === 'string' && cursor.length > 0 ? cursor : null, + }); + + const publicSet = new Set(publicIds.map((id) => id.toString())); + const skills = result.skills.map((s) => serializeSkillSummary(s, publicSet)); + + return res.status(200).json({ + skills, + has_more: result.has_more, + after: result.after, + }); + } catch (error) { + logger.error('[GET /skills] Error listing skills', error); + return res.status(500).json({ error: 'Error listing skills' }); + } + } + + async function createHandler(req: ServerRequest, res: Response) { + try { + const user = req.user; + if (!user || !user.id) { + return res.status(401).json({ error: 'Authentication required' }); + } + + const body = (req.body ?? {}) as TCreateSkill; + + if (!body.name || typeof body.name !== 'string') { + return res.status(400).json({ error: 'Skill name is required' }); + } + if (!body.description || typeof body.description !== 'string') { + return res.status(400).json({ error: 'Skill description is required' }); + } + + const authorId = (user._id ?? user.id) as unknown as Types.ObjectId; + const authorName = user.name ?? user.username ?? 'Unknown'; + + let createResult: CreateSkillResult; + try { + createResult = await createSkill({ + name: body.name, + displayTitle: body.displayTitle, + description: body.description, + body: body.body, + frontmatter: body.frontmatter as Record | undefined, + category: body.category, + author: authorId, + authorName, + tenantId: user.tenantId, + }); + } catch (error) { + if (isValidationError(error)) { + return res.status(400).json({ error: 'Validation failed', issues: error.issues }); + } + if (isDuplicateKeyError(error)) { + return res.status(409).json({ error: 'A skill with this name already exists' }); + } + throw error; + } + + const { skill, warnings } = createResult; + + try { + await grantPermission({ + principalType: PrincipalType.USER, + principalId: user.id, + resourceType: ResourceType.SKILL, + resourceId: skill._id, + accessRoleId: AccessRoleIds.SKILL_OWNER, + grantedBy: user.id, + }); + } catch (permissionError) { + logger.error( + `[POST /skills] Failed to grant owner permission for skill ${skill._id.toString()}, rolling back:`, + permissionError, + ); + try { + await deleteSkill(skill._id.toString()); + } catch (rollbackError) { + logger.error( + `[POST /skills] Compensating delete failed for orphaned skill ${skill._id.toString()}:`, + rollbackError, + ); + } + return res.status(500).json({ error: 'Failed to initialize skill permissions' }); + } + + // A freshly created skill has no PUBLIC ACL entry, so `isPublic` is + // always false. Skip the DB round-trip. + return res + .status(201) + .json(attachWarnings(serializeSkill(skill, new Set()), warnings)); + } catch (error) { + logger.error('[POST /skills] Error creating skill', error); + return res.status(500).json({ error: 'Error creating skill' }); + } + } + + async function getHandler(req: ServerRequest, res: Response) { + try { + const { id } = req.params as { id: string }; + // The canAccessSkillResource middleware already resolved the skill via + // getSkillById as its idResolver and stashed it on req.resourceAccess.resourceInfo. + // Reuse it to avoid a second DB round-trip. + const resolved = ( + req as ServerRequest & { + resourceAccess?: { resourceInfo?: ISkill & { _id: Types.ObjectId } }; + } + ).resourceAccess?.resourceInfo; + const skill = resolved ?? (await getSkillById(id)); + if (!skill) { + return res.status(404).json({ error: 'Skill not found' }); + } + const publicSet = await getPublicSkillIdSet(); + return res.status(200).json(serializeSkill(skill, publicSet)); + } catch (error) { + logger.error('[GET /skills/:id] Error fetching skill', error); + return res.status(500).json({ error: 'Error fetching skill' }); + } + } + + async function patchHandler(req: ServerRequest, res: Response) { + try { + const { id } = req.params as { id: string }; + const body = (req.body ?? {}) as TUpdateSkillPayload & { expectedVersion?: number }; + const { expectedVersion, ...rest } = body; + // `typeof NaN === 'number'` is true, so we need the stricter isFinite/isInteger + // checks below to avoid NaN passing through and triggering a misleading 409 + // (MongoDB's `{ version: NaN }` never matches, so the handler would fall + // through to the conflict branch and leak the current skill state). + if ( + typeof expectedVersion !== 'number' || + !Number.isFinite(expectedVersion) || + !Number.isInteger(expectedVersion) || + expectedVersion < 1 + ) { + return res + .status(400) + .json({ error: 'expectedVersion is required and must be a positive integer' }); + } + + const update: UpdateSkillInput = {}; + if (rest.name !== undefined) update.name = rest.name; + if (rest.displayTitle !== undefined) update.displayTitle = rest.displayTitle; + if (rest.description !== undefined) update.description = rest.description; + if (rest.body !== undefined) update.body = rest.body; + if (rest.frontmatter !== undefined) { + update.frontmatter = rest.frontmatter as Record; + } + if (rest.category !== undefined) update.category = rest.category; + + if (Object.keys(update).length === 0) { + return res.status(400).json({ error: 'At least one field must be provided for update' }); + } + + let result: UpdateSkillResult; + try { + result = await updateSkill({ id, expectedVersion, update }); + } catch (error) { + if (isValidationError(error)) { + return res.status(400).json({ error: 'Validation failed', issues: error.issues }); + } + if (isDuplicateKeyError(error)) { + return res.status(409).json({ error: 'A skill with this name already exists' }); + } + throw error; + } + + if (result.status === 'not_found') { + return res.status(404).json({ error: 'Skill not found' }); + } + const publicSet = await getPublicSkillIdSet(); + if (result.status === 'conflict') { + const conflict: TSkillConflictResponse = { + error: 'skill_version_conflict', + current: serializeSkill(result.current, publicSet), + }; + return res.status(409).json(conflict); + } + return res + .status(200) + .json(attachWarnings(serializeSkill(result.skill, publicSet), result.warnings)); + } catch (error) { + logger.error('[PATCH /skills/:id] Error updating skill', error); + return res.status(500).json({ error: 'Error updating skill' }); + } + } + + async function deleteHandler(req: ServerRequest, res: Response) { + try { + const { id } = req.params as { id: string }; + if (!isValidObjectIdString(id)) { + return res.status(400).json({ error: 'Invalid skill id' }); + } + const result = await deleteSkill(id); + if (!result.deleted) { + return res.status(404).json({ error: 'Skill not found' }); + } + const response: TDeleteSkillResponse = { id, deleted: true }; + return res.status(200).json(response); + } catch (error) { + logger.error('[DELETE /skills/:id] Error deleting skill', error); + return res.status(500).json({ error: 'Error deleting skill' }); + } + } + + async function listFilesHandler(req: ServerRequest, res: Response) { + try { + const { id } = req.params as { id: string }; + const rows = await listSkillFiles(id); + const response: TListSkillFilesResponse = { files: rows.map(serializeSkillFile) }; + return res.status(200).json(response); + } catch (error) { + logger.error('[GET /skills/:id/files] Error listing skill files', error); + return res.status(500).json({ error: 'Error listing skill files' }); + } + } + + function uploadFileStubHandler(_req: ServerRequest, res: Response) { + return res.status(501).json({ + error: 'skill_file_upload_not_implemented', + phase: 2, + message: + 'Skill file upload is not yet wired up. This endpoint is a stub reserved for phase 2.', + }); + } + + function downloadFileStubHandler(_req: ServerRequest, res: Response) { + return res.status(501).json({ + error: 'skill_file_download_not_implemented', + phase: 2, + message: + 'Skill file download is not yet wired up. This endpoint is a stub reserved for phase 2.', + }); + } + + async function deleteFileHandler(req: ServerRequest, res: Response) { + try { + const { id, relativePath } = req.params as { id: string; relativePath: string }; + let decodedPath: string; + try { + decodedPath = decodeURIComponent(relativePath); + } catch { + return res.status(400).json({ error: 'Invalid file path encoding' }); + } + const result = await deleteSkillFile(id, decodedPath); + if (!result.deleted) { + return res.status(404).json({ error: 'Skill file not found' }); + } + const response: TDeleteSkillFileResponse = { + skillId: id, + relativePath: decodedPath, + deleted: true, + }; + return res.status(200).json(response); + } catch (error) { + logger.error('[DELETE /skills/:id/files/:relativePath] Error', error); + return res.status(500).json({ error: 'Error deleting skill file' }); + } + } + + return { + list: listHandler, + create: createHandler, + get: getHandler, + patch: patchHandler, + delete: deleteHandler, + listFiles: listFilesHandler, + uploadFileStub: uploadFileStubHandler, + downloadFileStub: downloadFileStubHandler, + deleteFile: deleteFileHandler, + }; +} + +export type SkillsHandlers = ReturnType; diff --git a/packages/api/src/skills/index.ts b/packages/api/src/skills/index.ts new file mode 100644 index 0000000000..6c6f862d01 --- /dev/null +++ b/packages/api/src/skills/index.ts @@ -0,0 +1 @@ +export * from './handlers'; diff --git a/packages/data-provider/src/accessPermissions.ts b/packages/data-provider/src/accessPermissions.ts index bc97458076..8047f14b79 100644 --- a/packages/data-provider/src/accessPermissions.ts +++ b/packages/data-provider/src/accessPermissions.ts @@ -47,6 +47,7 @@ export enum ResourceType { PROMPTGROUP = 'promptGroup', MCPSERVER = 'mcpServer', REMOTE_AGENT = 'remoteAgent', + SKILL = 'skill', } /** @@ -79,6 +80,9 @@ export enum AccessRoleIds { REMOTE_AGENT_VIEWER = 'remoteAgent_viewer', REMOTE_AGENT_EDITOR = 'remoteAgent_editor', REMOTE_AGENT_OWNER = 'remoteAgent_owner', + SKILL_VIEWER = 'skill_viewer', + SKILL_EDITOR = 'skill_editor', + SKILL_OWNER = 'skill_owner', } // ===== ZOD SCHEMAS ===== @@ -317,16 +321,19 @@ export function accessRoleToPermBits(accessRoleId: string): number { case AccessRoleIds.PROMPTGROUP_VIEWER: case AccessRoleIds.MCPSERVER_VIEWER: case AccessRoleIds.REMOTE_AGENT_VIEWER: + case AccessRoleIds.SKILL_VIEWER: return PermissionBits.VIEW; case AccessRoleIds.AGENT_EDITOR: case AccessRoleIds.PROMPTGROUP_EDITOR: case AccessRoleIds.MCPSERVER_EDITOR: case AccessRoleIds.REMOTE_AGENT_EDITOR: + case AccessRoleIds.SKILL_EDITOR: return PermissionBits.VIEW | PermissionBits.EDIT; case AccessRoleIds.AGENT_OWNER: case AccessRoleIds.PROMPTGROUP_OWNER: case AccessRoleIds.MCPSERVER_OWNER: case AccessRoleIds.REMOTE_AGENT_OWNER: + case AccessRoleIds.SKILL_OWNER: return ( PermissionBits.VIEW | PermissionBits.EDIT | PermissionBits.DELETE | PermissionBits.SHARE ); diff --git a/packages/data-provider/src/api-endpoints.ts b/packages/data-provider/src/api-endpoints.ts index 21f9cdc6a3..69867a14ef 100644 --- a/packages/data-provider/src/api-endpoints.ts +++ b/packages/data-provider/src/api-endpoints.ts @@ -359,6 +359,33 @@ export const getCategories = () => `${BASE_URL}/api/categories`; export const getAllPromptGroups = () => `${prompts()}/all`; +/* Skills */ +export const skills = () => `${BASE_URL}/api/skills`; + +export const getSkill = (id: string) => `${skills()}/${encodeURIComponent(id)}`; + +export const listSkillsWithFilters = ( + filter: Record, +) => { + const cleaned = Object.entries(filter).reduce( + (acc, [key, value]) => { + if (value !== undefined && value !== null && value !== '') { + acc[key] = String(value); + } + return acc; + }, + {} as Record, + ); + const query = + Object.keys(cleaned).length > 0 ? `?${new URLSearchParams(cleaned).toString()}` : ''; + return `${skills()}${query}`; +}; + +export const skillFiles = (id: string) => `${getSkill(id)}/files`; + +export const skillFile = (id: string, relativePath: string) => + `${skillFiles(id)}/${encodeURIComponent(relativePath)}`; + /* Roles */ export const roles = () => `${BASE_URL}/api/roles`; export const adminRoles = () => `${BASE_URL}/api/admin/roles`; diff --git a/packages/data-provider/src/data-service.ts b/packages/data-provider/src/data-service.ts index d21089db82..e67fbedbf2 100644 --- a/packages/data-provider/src/data-service.ts +++ b/packages/data-provider/src/data-service.ts @@ -6,6 +6,7 @@ import * as ag from './types/agents'; import * as m from './types/mutations'; import * as q from './types/queries'; import * as f from './types/files'; +import * as sk from './types/skills'; import * as mcp from './types/mcpServers'; import * as config from './config'; import request from './request'; @@ -859,6 +860,46 @@ export function getRandomPrompts( return request.get(endpoints.getRandomPrompts(variables.limit, variables.skip)); } +/* Skills */ + +export function listSkills(params?: sk.TSkillListRequest): Promise { + return request.get(endpoints.listSkillsWithFilters(params ?? {})); +} + +export function getSkill(id: string): Promise { + return request.get(endpoints.getSkill(id)); +} + +export function createSkill(payload: sk.TCreateSkill): Promise { + return request.post(endpoints.skills(), payload); +} + +export function updateSkill(variables: sk.TUpdateSkillVariables): Promise { + return request.patch(endpoints.getSkill(variables.id), { + expectedVersion: variables.expectedVersion, + ...variables.payload, + }); +} + +export function deleteSkill(id: string): Promise { + return request.delete(endpoints.getSkill(id)); +} + +export function listSkillFiles(skillId: string): Promise { + return request.get(endpoints.skillFiles(skillId)); +} + +export function uploadSkillFile(skillId: string, formData: FormData): Promise { + return request.postMultiPart(endpoints.skillFiles(skillId), formData); +} + +export function deleteSkillFile( + skillId: string, + relativePath: string, +): Promise { + return request.delete(endpoints.skillFile(skillId, relativePath)); +} + /* Roles */ export function listRoles(): Promise { return request.get(`${endpoints.adminRoles()}?limit=200`); diff --git a/packages/data-provider/src/index.ts b/packages/data-provider/src/index.ts index 2867bd1ec5..08ab674849 100644 --- a/packages/data-provider/src/index.ts +++ b/packages/data-provider/src/index.ts @@ -25,6 +25,7 @@ export * from './types/files'; export * from './types/mcpServers'; export * from './types/mutations'; export * from './types/queries'; +export * from './types/skills'; export * from './types/runs'; export * from './types/web'; export * from './types/graph'; diff --git a/packages/data-provider/src/keys.ts b/packages/data-provider/src/keys.ts index 52208d3717..28a568b87e 100644 --- a/packages/data-provider/src/keys.ts +++ b/packages/data-provider/src/keys.ts @@ -65,6 +65,10 @@ export enum QueryKeys { activeJobs = 'activeJobs', /* Agent API Keys */ agentApiKeys = 'agentApiKeys', + /* Skills */ + skills = 'skills', + skill = 'skill', + skillFiles = 'skillFiles', } // Dynamic query keys that require parameters diff --git a/packages/data-provider/src/permissions.ts b/packages/data-provider/src/permissions.ts index 58cf0df15d..3f4590af3a 100644 --- a/packages/data-provider/src/permissions.ts +++ b/packages/data-provider/src/permissions.ts @@ -60,6 +60,10 @@ export enum PermissionTypes { * Type for Remote Agent (API) Permissions */ REMOTE_AGENTS = 'REMOTE_AGENTS', + /** + * Type for Skill Permissions + */ + SKILLS = 'SKILLS', } /** @@ -82,6 +86,7 @@ export const PERMISSION_TYPE_INTERFACE_FIELDS: Record = [PermissionTypes.MARKETPLACE]: 'marketplace', [PermissionTypes.MCP_SERVERS]: 'mcpServers', [PermissionTypes.REMOTE_AGENTS]: 'remoteAgents', + [PermissionTypes.SKILLS]: 'skills', }; /** Set of interface config field names that correspond to role permissions. */ @@ -218,6 +223,14 @@ export const remoteAgentsPermissionsSchema = z.object({ }); export type TRemoteAgentsPermissions = z.infer; +export const skillPermissionsSchema = z.object({ + [Permissions.USE]: z.boolean().default(true), + [Permissions.CREATE]: z.boolean().default(true), + [Permissions.SHARE]: z.boolean().default(false), + [Permissions.SHARE_PUBLIC]: z.boolean().default(false), +}); +export type TSkillPermissions = z.infer; + // Define a single permissions schema that holds all permission types. export const permissionsSchema = z.object({ [PermissionTypes.PROMPTS]: promptPermissionsSchema, @@ -234,4 +247,5 @@ export const permissionsSchema = z.object({ [PermissionTypes.FILE_CITATIONS]: fileCitationsPermissionsSchema, [PermissionTypes.MCP_SERVERS]: mcpServersPermissionsSchema, [PermissionTypes.REMOTE_AGENTS]: remoteAgentsPermissionsSchema, + [PermissionTypes.SKILLS]: skillPermissionsSchema, }); diff --git a/packages/data-provider/src/roles.spec.ts b/packages/data-provider/src/roles.spec.ts index 60dac5ab50..503bb7cb1e 100644 --- a/packages/data-provider/src/roles.spec.ts +++ b/packages/data-provider/src/roles.spec.ts @@ -31,8 +31,7 @@ describe('roleDefaults', () => { continue; } - const userValues = - userPerms[permType as PermissionTypes] as Record; + const userValues = userPerms[permType as PermissionTypes] as Record; for (const field of fieldNames) { expect({ @@ -78,7 +77,9 @@ describe('roleDefaults', () => { for (const [permType, subSchema] of Object.entries(schemaShape)) { const fieldNames = Object.keys(subSchema.shape); - const hasResourceFields = fieldNames.some((f) => RESOURCE_MANAGEMENT_FIELDS.includes(f as Permissions)); + const hasResourceFields = fieldNames.some((f) => + RESOURCE_MANAGEMENT_FIELDS.includes(f as Permissions), + ); if (!hasResourceFields) { continue; } @@ -87,7 +88,8 @@ describe('roleDefaults', () => { restrictedSet.has(permType) || permType === PermissionTypes.MEMORIES || permType === PermissionTypes.PROMPTS || - permType === PermissionTypes.AGENTS; + permType === PermissionTypes.AGENTS || + permType === PermissionTypes.SKILLS; expect({ permType, @@ -110,8 +112,7 @@ describe('roleDefaults', () => { for (const [permType, subSchema] of Object.entries(schemaShape)) { const fieldNames = Object.keys(subSchema.shape); - const adminValues = - adminPerms[permType as PermissionTypes] as Record; + const adminValues = adminPerms[permType as PermissionTypes] as Record; for (const field of fieldNames) { expect({ diff --git a/packages/data-provider/src/roles.ts b/packages/data-provider/src/roles.ts index 0d81c55304..600c017402 100644 --- a/packages/data-provider/src/roles.ts +++ b/packages/data-provider/src/roles.ts @@ -5,6 +5,7 @@ import { permissionsSchema, agentPermissionsSchema, promptPermissionsSchema, + skillPermissionsSchema, memoryPermissionsSchema, runCodePermissionsSchema, bookmarkPermissionsSchema, @@ -103,6 +104,12 @@ const defaultRolesSchema = z.object({ [Permissions.SHARE]: z.boolean().default(true), [Permissions.SHARE_PUBLIC]: z.boolean().default(true), }), + [PermissionTypes.SKILLS]: skillPermissionsSchema.extend({ + [Permissions.USE]: z.boolean().default(true), + [Permissions.CREATE]: z.boolean().default(true), + [Permissions.SHARE]: z.boolean().default(true), + [Permissions.SHARE_PUBLIC]: z.boolean().default(true), + }), }), }), [SystemRoles.USER]: roleSchema.extend({ @@ -185,6 +192,12 @@ export const roleDefaults = defaultRolesSchema.parse({ [Permissions.SHARE]: true, [Permissions.SHARE_PUBLIC]: true, }, + [PermissionTypes.SKILLS]: { + [Permissions.USE]: true, + [Permissions.CREATE]: true, + [Permissions.SHARE]: true, + [Permissions.SHARE_PUBLIC]: true, + }, }, }, [SystemRoles.USER]: { @@ -230,6 +243,12 @@ export const roleDefaults = defaultRolesSchema.parse({ [Permissions.SHARE]: false, [Permissions.SHARE_PUBLIC]: false, }, + [PermissionTypes.SKILLS]: { + [Permissions.USE]: true, + [Permissions.CREATE]: true, + [Permissions.SHARE]: false, + [Permissions.SHARE_PUBLIC]: false, + }, }, }, }); diff --git a/packages/data-provider/src/types/mutations.ts b/packages/data-provider/src/types/mutations.ts index 15f0cd2329..4b2c470821 100644 --- a/packages/data-provider/src/types/mutations.ts +++ b/packages/data-provider/src/types/mutations.ts @@ -13,6 +13,19 @@ import { AgentUpdateParams, } from './assistants'; import { Action, ActionMetadata } from './agents'; +import type { InfiniteData, QueryKey } from '@tanstack/react-query'; +import type { + TSkill, + TSkillFile, + TCreateSkill, + TUpdateSkillVariables, + TUpdateSkillResponse, + TDeleteSkillResponse, + TUploadSkillFileVariables, + TDeleteSkillFileVariables, + TDeleteSkillFileResponse, + TSkillListResponse, +} from './skills'; export type MutationOptions< Response, @@ -275,9 +288,44 @@ export type UpdateMemoryPermVars = UpdatePermVars; export type UpdateAgentPermVars = UpdatePermVars; export type UpdatePeoplePickerPermVars = UpdatePermVars; export type UpdateMCPServersPermVars = UpdatePermVars; +export type UpdateSkillPermVars = UpdatePermVars; export type UpdatePermResponse = r.TRole; +/* Skill mutations */ + +/** + * Cache entries that can appear under the `[QueryKeys.skills, ...]` key prefix. + * Flat responses come from `useListSkillsQuery`; infinite responses come from + * `useSkillsInfiniteQuery`. The context carries both shapes for rollback. + */ +export type TSkillCacheEntry = TSkillListResponse | InfiniteData | undefined; + +export type TUpdateSkillContext = + | { + previousSkill?: TSkill; + previousListSnapshots?: Array<[QueryKey, TSkillCacheEntry]>; + userContext?: unknown; + } + | undefined; + +export type CreateSkillOptions = MutationOptions; + +export type UpdateSkillOptions = MutationOptions< + TUpdateSkillResponse, + TUpdateSkillVariables, + TUpdateSkillContext +>; + +export type DeleteSkillOptions = MutationOptions; + +export type UploadSkillFileOptions = MutationOptions; + +export type DeleteSkillFileOptions = MutationOptions< + TDeleteSkillFileResponse, + TDeleteSkillFileVariables +>; + export type UpdatePromptPermOptions = MutationOptions< UpdatePermResponse, UpdatePromptPermVars, diff --git a/packages/data-provider/src/types/skills.ts b/packages/data-provider/src/types/skills.ts new file mode 100644 index 0000000000..8e445d34b3 --- /dev/null +++ b/packages/data-provider/src/types/skills.ts @@ -0,0 +1,219 @@ +import type { FileSources } from './files'; + +/** + * Source of a skill β€” where its canonical definition came from. + * `inline` means the skill was authored directly in LibreChat. + * `github` / `notion` are reserved for future sync integrations. + */ +export type SkillSource = 'inline' | 'github' | 'notion'; + +/** + * Category inferred from a skill file's top-level directory prefix. + * `script` for `scripts/...`, `reference` for `references/...`, `asset` for `assets/...`, + * everything else (including root-level files) is `other`. + */ +export type SkillFileCategory = 'script' | 'reference' | 'asset' | 'other'; + +/** + * Allowed value types inside a skill's YAML frontmatter. + * Kept strict so callers cannot slip arbitrary `unknown` payloads through the API. + */ +export type SkillFrontmatterValue = string | number | boolean | string[] | null; + +/** + * Structured YAML frontmatter for a skill. All keys are optional on the wire + * because not every skill document carries a complete frontmatter block β€” + * `name` and `description` live as first-class columns on `TSkill` itself, + * and frontmatter is an extension bag for additional fields like `when-to-use`, + * `allowed-tools`, etc. + */ +export type SkillFrontmatter = { + name?: string; + description?: string; +} & Record; + +/** + * Provenance metadata for skills that originated from an external source + * (e.g. a GitHub commit SHA or a Notion page id). + * + * Reserved for phase 2+ external sync β€” no code path currently populates this + * in phase 1, but the column exists so a future sync worker can use it + * without a schema migration. + */ +export type SkillSourceMetadata = Record; + +/** + * A non-blocking coaching hint surfaced alongside a successful create/update + * response. Unlike validation errors (which return 400 and block the write), + * warnings ride on the 2xx response so the UI can show inline feedback + * without rejecting the user's input. Example: "description is too short, + * Claude may undertrigger this skill". + */ +export type TSkillWarning = { + field: string; + code: string; + message: string; + severity: 'warning'; +}; + +/** + * API shape for a full skill (returned by GET `/api/skills/:id`). + * + * Field semantics: + * - `name` is the machine-readable kebab-case identifier Claude sees in its + * skill manifest. It's what drives triggering and must be stable across + * edits. Unique per author+tenant. + * - `displayTitle` is the human-readable UI label only. NOT sent to Claude, + * NOT part of the trigger path β€” purely cosmetic. + * - `description` is the "when to use this skill" sentence. Highest-leverage + * field for trigger accuracy; a short/vague one causes undertriggering. + * - `frontmatter` is the structured YAML bag minus `name`/`description` + * (those live as top-level columns). Validated strictly against a known + * key set server-side. + * - `source`/`sourceMetadata` are reserved for phase 2+ external sync and + * always `'inline'` / absent in phase 1. + */ +export type TSkill = { + _id: string; + name: string; + displayTitle?: string; + description: string; + body: string; + frontmatter?: SkillFrontmatter; + category?: string; + author: string; + authorName: string; + version: number; + source: SkillSource; + sourceMetadata?: SkillSourceMetadata; + fileCount: number; + isPublic?: boolean; + tenantId?: string; + createdAt: string; + updatedAt: string; + /** + * Present on POST/PATCH responses when the server emitted non-blocking + * coaching warnings (e.g. description too short). Never present on GET + * responses. + */ + warnings?: TSkillWarning[]; +}; + +/** + * Summary shape used in list endpoints β€” omits `body` and `frontmatter` to keep + * list payloads small. Callers that need the full body/frontmatter must fetch + * the detail via `GET /api/skills/:id`. + */ +export type TSkillSummary = Omit; + +/** + * Metadata for a single file bundled inside a skill. + * File content itself is fetched separately via the file download endpoint. + */ +export type TSkillFile = { + _id: string; + skillId: string; + relativePath: string; + file_id: string; + filename: string; + filepath: string; + source: FileSources; + mimeType: string; + bytes: number; + category: SkillFileCategory; + isExecutable: boolean; + author: string; + tenantId?: string; + createdAt: string; + updatedAt: string; +}; + +/** Request body for POST `/api/skills`. */ +export type TCreateSkill = { + name: string; + displayTitle?: string; + description: string; + body: string; + frontmatter?: Partial; + category?: string; +}; + +/** Partial payload for PATCH `/api/skills/:id` β€” all fields optional. */ +export type TUpdateSkillPayload = { + name?: string; + displayTitle?: string; + description?: string; + body?: string; + frontmatter?: Partial; + category?: string; +}; + +/** Variables passed into the update mutation: id + expectedVersion + partial payload. */ +export type TUpdateSkillVariables = { + id: string; + expectedVersion: number; + payload: TUpdateSkillPayload; +}; + +/** Response from a successful PATCH β€” includes the bumped version. */ +export type TUpdateSkillResponse = TSkill; + +/** Response from a 409 concurrency conflict β€” includes the current authoritative state. */ +export type TSkillConflictResponse = { + error: 'skill_version_conflict'; + current: TSkill; +}; + +/** Query params for GET `/api/skills` (list). */ +export type TSkillListRequest = { + category?: string; + search?: string; + limit?: number; + cursor?: string; +}; + +/** Paginated list response. `after` is the cursor to pass for the next page. */ +export type TSkillListResponse = { + skills: TSkillSummary[]; + has_more: boolean; + after: string | null; +}; + +/** Response from DELETE `/api/skills/:id`. */ +export type TDeleteSkillResponse = { + id: string; + deleted: true; +}; + +/** Response from GET `/api/skills/:id/files`. */ +export type TListSkillFilesResponse = { + files: TSkillFile[]; +}; + +/** + * Upload body for POST `/api/skills/:id/files`. + * In phase 1 the backend responds with 501; the client contract is still defined here + * so hooks are stable when the upload pipeline is wired up in phase 2. + */ +export type TUploadSkillFilePayload = { + relativePath: string; +}; + +/** Response from DELETE `/api/skills/:id/files/:relativePath`. */ +export type TDeleteSkillFileResponse = { + skillId: string; + relativePath: string; + deleted: true; +}; + +/** Variables passed into the skill file upload mutation. */ +export type TUploadSkillFileVariables = { + skillId: string; + formData: FormData; +}; + +/** Variables passed into the skill file delete mutation. */ +export type TDeleteSkillFileVariables = { + skillId: string; + relativePath: string; +}; diff --git a/packages/data-schemas/src/admin/capabilities.ts b/packages/data-schemas/src/admin/capabilities.ts index 44b9eaab4c..0284fb5586 100644 --- a/packages/data-schemas/src/admin/capabilities.ts +++ b/packages/data-schemas/src/admin/capabilities.ts @@ -35,6 +35,8 @@ export const SystemCapabilities = { MANAGE_MCP_SERVERS: 'manage:mcpservers', READ_PROMPTS: 'read:prompts', MANAGE_PROMPTS: 'manage:prompts', + READ_SKILLS: 'read:skills', + MANAGE_SKILLS: 'manage:skills', /** Reserved β€” not yet enforced by any middleware. */ READ_ASSISTANTS: 'read:assistants', MANAGE_ASSISTANTS: 'manage:assistants', @@ -52,6 +54,7 @@ export const CapabilityImplications: Partial = { [ResourceType.PROMPTGROUP]: SystemCapabilities.MANAGE_PROMPTS, [ResourceType.MCPSERVER]: SystemCapabilities.MANAGE_MCP_SERVERS, [ResourceType.REMOTE_AGENT]: SystemCapabilities.MANAGE_AGENTS, + [ResourceType.SKILL]: SystemCapabilities.MANAGE_SKILLS, }; /** @@ -204,6 +208,8 @@ export const CAPABILITY_CATEGORIES: CapabilityCategory[] = [ SystemCapabilities.READ_AGENTS, SystemCapabilities.MANAGE_PROMPTS, SystemCapabilities.READ_PROMPTS, + SystemCapabilities.MANAGE_SKILLS, + SystemCapabilities.READ_SKILLS, SystemCapabilities.MANAGE_ASSISTANTS, SystemCapabilities.READ_ASSISTANTS, SystemCapabilities.MANAGE_MCP_SERVERS, diff --git a/packages/data-schemas/src/methods/accessRole.spec.ts b/packages/data-schemas/src/methods/accessRole.spec.ts index 6919e6c7df..7f62c2f8cf 100644 --- a/packages/data-schemas/src/methods/accessRole.spec.ts +++ b/packages/data-schemas/src/methods/accessRole.spec.ts @@ -206,6 +206,9 @@ describe('AccessRole Model Tests', () => { AccessRoleIds.REMOTE_AGENT_EDITOR, AccessRoleIds.REMOTE_AGENT_OWNER, AccessRoleIds.REMOTE_AGENT_VIEWER, + AccessRoleIds.SKILL_EDITOR, + AccessRoleIds.SKILL_OWNER, + AccessRoleIds.SKILL_VIEWER, ].sort(), ); diff --git a/packages/data-schemas/src/methods/accessRole.ts b/packages/data-schemas/src/methods/accessRole.ts index ab2cffbad8..71cbba0c76 100644 --- a/packages/data-schemas/src/methods/accessRole.ts +++ b/packages/data-schemas/src/methods/accessRole.ts @@ -188,6 +188,27 @@ export function createAccessRoleMethods(mongoose: typeof import('mongoose')) { resourceType: ResourceType.REMOTE_AGENT, permBits: RoleBits.OWNER, }, + { + accessRoleId: AccessRoleIds.SKILL_VIEWER, + name: 'com_ui_role_viewer', + description: 'com_ui_role_viewer_desc', + resourceType: ResourceType.SKILL, + permBits: RoleBits.VIEWER, + }, + { + accessRoleId: AccessRoleIds.SKILL_EDITOR, + name: 'com_ui_role_editor', + description: 'com_ui_role_editor_desc', + resourceType: ResourceType.SKILL, + permBits: RoleBits.EDITOR, + }, + { + accessRoleId: AccessRoleIds.SKILL_OWNER, + name: 'com_ui_role_owner', + description: 'com_ui_role_owner_desc', + resourceType: ResourceType.SKILL, + permBits: RoleBits.OWNER, + }, ]; const result: Record = {}; diff --git a/packages/data-schemas/src/methods/index.ts b/packages/data-schemas/src/methods/index.ts index c2f6f7cb82..28d3effab7 100644 --- a/packages/data-schemas/src/methods/index.ts +++ b/packages/data-schemas/src/methods/index.ts @@ -45,6 +45,19 @@ import { import { createTransactionMethods, type TransactionMethods } from './transaction'; import { createSpendTokensMethods, type SpendTokensMethods } from './spendTokens'; import { createPromptMethods, type PromptMethods, type PromptDeps } from './prompt'; +import { + createSkillMethods, + type SkillMethods, + type SkillDeps, + type CreateSkillInput, + type CreateSkillResult, + type UpdateSkillInput, + type UpsertSkillFileInput, + type ListSkillsByAccessParams, + type ListSkillsByAccessResult, + type UpdateSkillResult, + type ValidationIssue, +} from './skill'; /* Tier 5 β€” Agent */ import { createAgentMethods, type AgentMethods, type AgentDeps } from './agent'; /* Config */ @@ -83,6 +96,7 @@ export type AllMethods = UserMethods & TransactionMethods & SpendTokensMethods & PromptMethods & + SkillMethods & AgentMethods & ConfigMethods; @@ -155,6 +169,12 @@ export function createMethods( }; const promptMethods = createPromptMethods(mongoose, promptDeps); + const skillDeps: SkillDeps = { + removeAllPermissions, + getSoleOwnedResourceIds: aclEntryMethods.getSoleOwnedResourceIds, + }; + const skillMethods = createSkillMethods(mongoose, skillDeps); + // Role methods with optional cache injection const roleDeps: RoleDeps = { getCache: deps.getCache }; const roleMethods = createRoleMethods(mongoose, roleDeps); @@ -203,6 +223,7 @@ export function createMethods( ...transactionMethods, ...spendTokensMethods, ...promptMethods, + ...skillMethods, /* Tier 5 */ ...agentMethods, /* Config */ @@ -240,6 +261,16 @@ export type { TransactionMethods, SpendTokensMethods, PromptMethods, + SkillMethods, + SkillDeps, + CreateSkillInput, + CreateSkillResult, + UpdateSkillInput, + UpsertSkillFileInput, + ListSkillsByAccessParams, + ListSkillsByAccessResult, + UpdateSkillResult, + ValidationIssue, AgentMethods, ConfigMethods, }; diff --git a/packages/data-schemas/src/methods/skill.spec.ts b/packages/data-schemas/src/methods/skill.spec.ts new file mode 100644 index 0000000000..332a7ecd5e --- /dev/null +++ b/packages/data-schemas/src/methods/skill.spec.ts @@ -0,0 +1,617 @@ +import mongoose from 'mongoose'; +import { MongoMemoryServer } from 'mongodb-memory-server'; +import { + SystemRoles, + ResourceType, + AccessRoleIds, + PrincipalType, + PermissionBits, +} from 'librechat-data-provider'; +import { createAclEntryMethods } from './aclEntry'; +import { + validateSkillName, + validateSkillDescription, + validateSkillFrontmatter, + validateRelativePath, + inferSkillFileCategory, +} from './skill'; +import { logger, createModels } from '..'; +import { createMethods } from './index'; + +logger.silent = true; + +type LeanUser = { + _id: mongoose.Types.ObjectId; + name?: string; + email: string; + role?: string; +}; + +let Skill: mongoose.Model; +let SkillFile: mongoose.Model; +let AclEntry: mongoose.Model; +let AccessRole: mongoose.Model; +let User: mongoose.Model; +let methods: ReturnType; +let aclMethods: ReturnType; +let owner: LeanUser; +let other: LeanUser; + +let mongoServer: MongoMemoryServer; + +beforeAll(async () => { + mongoServer = await MongoMemoryServer.create(); + await mongoose.connect(mongoServer.getUri()); + + createModels(mongoose); + Skill = mongoose.models.Skill; + SkillFile = mongoose.models.SkillFile; + AclEntry = mongoose.models.AclEntry; + AccessRole = mongoose.models.AccessRole; + User = mongoose.models.User; + + methods = createMethods(mongoose, { + removeAllPermissions: async ({ resourceType, resourceId }) => { + await AclEntry.deleteMany({ resourceType, resourceId }); + }, + }); + aclMethods = createAclEntryMethods(mongoose); + + await AccessRole.create({ + accessRoleId: AccessRoleIds.SKILL_OWNER, + name: 'Owner', + description: 'Full control over skills', + resourceType: ResourceType.SKILL, + permBits: + PermissionBits.VIEW | PermissionBits.EDIT | PermissionBits.DELETE | PermissionBits.SHARE, + }); + await AccessRole.create({ + accessRoleId: AccessRoleIds.SKILL_EDITOR, + name: 'Editor', + description: 'Can view and edit skills', + resourceType: ResourceType.SKILL, + permBits: PermissionBits.VIEW | PermissionBits.EDIT, + }); + await AccessRole.create({ + accessRoleId: AccessRoleIds.SKILL_VIEWER, + name: 'Viewer', + description: 'Can view skills', + resourceType: ResourceType.SKILL, + permBits: PermissionBits.VIEW, + }); + + owner = ( + await User.create({ + name: 'Skill Owner', + email: 'skill-owner@example.com', + role: SystemRoles.USER, + }) + ).toObject() as unknown as LeanUser; + other = ( + await User.create({ + name: 'Other User', + email: 'skill-other@example.com', + role: SystemRoles.USER, + }) + ).toObject() as unknown as LeanUser; +}); + +afterAll(async () => { + await mongoose.disconnect(); + await mongoServer.stop(); +}); + +afterEach(async () => { + await Skill.deleteMany({}); + await SkillFile.deleteMany({}); + await AclEntry.deleteMany({}); +}); + +async function grantOwner(resourceId: mongoose.Types.ObjectId | string) { + const role = (await AccessRole.findOne({ accessRoleId: AccessRoleIds.SKILL_OWNER }).lean()) as { + _id: mongoose.Types.ObjectId; + permBits: number; + } | null; + if (!role) throw new Error('SKILL_OWNER role not seeded'); + return aclMethods.grantPermission( + PrincipalType.USER, + owner._id, + ResourceType.SKILL, + resourceId, + role.permBits, + owner._id, + undefined, + role._id, + ); +} + +function makeSkillInput(overrides: Record = {}) { + return { + name: 'demo-skill', + description: 'A small demo skill used in tests.', + body: '# Demo\n\nBody content', + frontmatter: { name: 'demo-skill', description: 'A small demo skill used in tests.' }, + author: owner._id, + authorName: owner.name ?? 'Skill Owner', + ...overrides, + }; +} + +describe('skill validation helpers', () => { + it('rejects names starting with reserved brand prefixes', () => { + expect(validateSkillName('anthropic-helper').some((i) => i.code === 'RESERVED_PREFIX')).toBe( + true, + ); + expect(validateSkillName('claude-helper').some((i) => i.code === 'RESERVED_PREFIX')).toBe(true); + }); + + it('allows names that merely contain reserved words as substrings', () => { + expect(validateSkillName('research-anthropic-helper')).toEqual([]); + expect(validateSkillName('about-claude')).toEqual([]); + }); + + it('rejects reserved CLI command names', () => { + for (const word of ['help', 'clear', 'compact', 'model', 'exit', 'quit', 'settings']) { + expect(validateSkillName(word).some((i) => i.code === 'RESERVED_WORD')).toBe(true); + } + }); + + it('rejects non-kebab-case names', () => { + expect(validateSkillName('My Skill').some((i) => i.code === 'INVALID_FORMAT')).toBe(true); + expect(validateSkillName('UPPER').some((i) => i.code === 'INVALID_FORMAT')).toBe(true); + }); + + it('rejects names longer than 64 chars', () => { + const long = 'a'.repeat(65); + expect(validateSkillName(long).some((i) => i.code === 'TOO_LONG')).toBe(true); + }); + + it('accepts valid kebab-case names', () => { + expect(validateSkillName('my-skill-1')).toEqual([]); + }); + + it('requires description', () => { + expect(validateSkillDescription('').some((i) => i.code === 'REQUIRED')).toBe(true); + expect(validateSkillDescription(' ').some((i) => i.code === 'REQUIRED')).toBe(true); + }); + + it('rejects description over 1024 chars', () => { + expect(validateSkillDescription('x'.repeat(1025)).some((i) => i.code === 'TOO_LONG')).toBe( + true, + ); + }); + + it('emits a warning (not an error) for short descriptions', () => { + const issues = validateSkillDescription('too short'); + const warnings = issues.filter((i) => i.severity === 'warning' && i.code === 'TOO_SHORT'); + const errors = issues.filter((i) => i.severity !== 'warning'); + expect(warnings.length).toBe(1); + expect(errors.length).toBe(0); + }); + + it('does not warn for descriptions at or above the threshold', () => { + expect( + validateSkillDescription('This description is comfortably over the threshold length.'), + ).toEqual([]); + }); + + it('rejects path traversal', () => { + expect(validateRelativePath('../evil').some((i) => i.code === 'TRAVERSAL')).toBe(true); + expect(validateRelativePath('scripts/../../etc').some((i) => i.code === 'TRAVERSAL')).toBe( + true, + ); + }); + + it('rejects absolute paths', () => { + expect(validateRelativePath('/abs/path').some((i) => i.code === 'ABSOLUTE_PATH')).toBe(true); + }); + + it('rejects SKILL.md explicitly', () => { + expect(validateRelativePath('SKILL.md').some((i) => i.code === 'RESERVED')).toBe(true); + }); + + it('accepts well-formed relative paths', () => { + expect(validateRelativePath('scripts/parse.sh')).toEqual([]); + expect(validateRelativePath('references/schema.md')).toEqual([]); + }); + + it('infers category from top-level prefix', () => { + expect(inferSkillFileCategory('scripts/parse.sh')).toBe('script'); + expect(inferSkillFileCategory('references/notes.md')).toBe('reference'); + expect(inferSkillFileCategory('assets/image.png')).toBe('asset'); + expect(inferSkillFileCategory('readme.txt')).toBe('other'); + }); + + describe('validateSkillFrontmatter', () => { + it('accepts an undefined or empty frontmatter', () => { + expect(validateSkillFrontmatter(undefined)).toEqual([]); + expect(validateSkillFrontmatter(null)).toEqual([]); + expect(validateSkillFrontmatter({})).toEqual([]); + }); + + it('rejects non-object frontmatter', () => { + expect(validateSkillFrontmatter('not an object').some((i) => i.code === 'INVALID_TYPE')).toBe( + true, + ); + expect(validateSkillFrontmatter([]).some((i) => i.code === 'INVALID_TYPE')).toBe(true); + }); + + it('rejects unknown keys in strict mode', () => { + const issues = validateSkillFrontmatter({ 'not-a-real-key': 'value' }); + expect(issues.some((i) => i.code === 'UNKNOWN_KEY')).toBe(true); + }); + + it('accepts known keys with correct types', () => { + expect( + validateSkillFrontmatter({ + name: 'demo-skill', + description: 'A demo skill', + 'when-to-use': 'When the user needs a demo.', + 'allowed-tools': ['read', 'write'], + 'user-invocable': true, + effort: 5, + version: '1.0.0', + hooks: { 'pre-run': 'echo hi' }, + }), + ).toEqual([]); + }); + + it('rejects known keys with wrong types', () => { + expect( + validateSkillFrontmatter({ 'user-invocable': 'yes' }).some( + (i) => i.code === 'INVALID_TYPE', + ), + ).toBe(true); + expect( + validateSkillFrontmatter({ 'allowed-tools': [1, 2, 3] }).some( + (i) => i.code === 'INVALID_TYPE', + ), + ).toBe(true); + }); + + it('rejects hooks object with excessive nesting', () => { + const deep = { a: { b: { c: { d: { e: { f: 'too deep' } } } } } }; + expect( + validateSkillFrontmatter({ hooks: deep }).some((i) => i.code === 'INVALID_SHAPE'), + ).toBe(true); + }); + }); +}); + +describe('Skill CRUD methods', () => { + it('creates a skill with version 1 and default fileCount 0', async () => { + const { skill, warnings } = await methods.createSkill(makeSkillInput()); + expect(skill.name).toBe('demo-skill'); + expect(skill.version).toBe(1); + expect(skill.fileCount).toBe(0); + expect(skill.source).toBe('inline'); + expect(skill.author.toString()).toBe(owner._id.toString()); + expect(warnings).toEqual([]); + }); + + it('emits a too-short warning when the description is under 20 chars', async () => { + const { skill, warnings } = await methods.createSkill( + makeSkillInput({ name: 'short-desc-skill', description: 'Too short.' }), + ); + expect(skill._id).toBeDefined(); + expect(warnings).toEqual([ + expect.objectContaining({ + field: 'description', + code: 'TOO_SHORT', + severity: 'warning', + }), + ]); + }); + + it('rejects frontmatter with unknown keys (strict mode)', async () => { + await expect( + methods.createSkill( + makeSkillInput({ + name: 'strict-frontmatter', + frontmatter: { 'bogus-key': 'nope' }, + }), + ), + ).rejects.toMatchObject({ code: 'SKILL_VALIDATION_FAILED' }); + }); + + it('rejects names that start with reserved brand prefixes', async () => { + await expect( + methods.createSkill(makeSkillInput({ name: 'anthropic-helper' })), + ).rejects.toMatchObject({ code: 'SKILL_VALIDATION_FAILED' }); + await expect( + methods.createSkill(makeSkillInput({ name: 'claude-helper' })), + ).rejects.toMatchObject({ code: 'SKILL_VALIDATION_FAILED' }); + }); + + it('allows names that merely contain the reserved words as substrings', async () => { + const { skill } = await methods.createSkill( + makeSkillInput({ name: 'research-anthropic-helper' }), + ); + expect(skill.name).toBe('research-anthropic-helper'); + }); + + it('rejects reserved CLI command names', async () => { + await expect(methods.createSkill(makeSkillInput({ name: 'help' }))).rejects.toMatchObject({ + code: 'SKILL_VALIDATION_FAILED', + }); + await expect(methods.createSkill(makeSkillInput({ name: 'settings' }))).rejects.toMatchObject({ + code: 'SKILL_VALIDATION_FAILED', + }); + }); + + it('enforces name uniqueness per author', async () => { + await methods.createSkill(makeSkillInput()); + await expect(methods.createSkill(makeSkillInput())).rejects.toBeDefined(); + }); + + it('throws with validation issues on bad input', async () => { + await expect(methods.createSkill(makeSkillInput({ name: 'BAD NAME' }))).rejects.toMatchObject({ + code: 'SKILL_VALIDATION_FAILED', + }); + }); + + it('optimistic concurrency β€” stale version returns conflict with current state', async () => { + const { skill } = await methods.createSkill(makeSkillInput()); + const id = skill._id.toString(); + const first = await methods.updateSkill({ + id, + expectedVersion: 1, + update: { description: 'First update description (long enough).' }, + }); + expect(first.status).toBe('updated'); + if (first.status === 'updated') { + expect(first.skill.version).toBe(2); + } + + const stale = await methods.updateSkill({ + id, + expectedVersion: 1, + update: { description: 'Second update description (long enough).' }, + }); + expect(stale.status).toBe('conflict'); + if (stale.status === 'conflict') { + expect(stale.current.version).toBe(2); + expect(stale.current.description).toBe('First update description (long enough).'); + } + }); + + it('deleteSkill cascades ACL entries and skill files', async () => { + const { skill } = await methods.createSkill(makeSkillInput()); + await grantOwner(skill._id); + await methods.upsertSkillFile({ + skillId: skill._id, + relativePath: 'scripts/parse.sh', + file_id: 'file-123', + filename: 'parse.sh', + filepath: '/tmp/parse.sh', + source: 'local', + mimeType: 'text/x-shellscript', + bytes: 42, + author: owner._id, + }); + + expect(await AclEntry.countDocuments({ resourceId: skill._id })).toBe(1); + expect(await SkillFile.countDocuments({ skillId: skill._id })).toBe(1); + + const res = await methods.deleteSkill(skill._id.toString()); + expect(res.deleted).toBe(true); + expect(await AclEntry.countDocuments({ resourceId: skill._id })).toBe(0); + expect(await SkillFile.countDocuments({ skillId: skill._id })).toBe(0); + }); + + it('listSkillsByAccess returns only accessible skills and paginates by cursor', async () => { + const ids: mongoose.Types.ObjectId[] = []; + for (let i = 0; i < 3; i++) { + const { skill } = await methods.createSkill( + makeSkillInput({ + name: `demo-skill-${i}`, + description: `A demo skill used to test pagination (${i}).`, + }), + ); + ids.push(skill._id); + await new Promise((r) => setTimeout(r, 5)); + } + + const page1 = await methods.listSkillsByAccess({ accessibleIds: ids, limit: 2 }); + expect(page1.skills.length).toBe(2); + expect(page1.has_more).toBe(true); + expect(page1.after).not.toBeNull(); + + const page2 = await methods.listSkillsByAccess({ + accessibleIds: ids, + limit: 2, + cursor: page1.after, + }); + expect(page2.skills.length).toBe(1); + expect(page2.has_more).toBe(false); + }); + + it('listSkillsByAccess filters by category and search', async () => { + const { skill: a } = await methods.createSkill( + makeSkillInput({ name: 'alpha-skill', category: 'tools' }), + ); + await methods.createSkill( + makeSkillInput({ + name: 'beta-skill', + category: 'other', + description: 'A beta description that is long enough.', + }), + ); + const ids = await Skill.find().distinct('_id'); + + const byCat = await methods.listSkillsByAccess({ + accessibleIds: ids as unknown as mongoose.Types.ObjectId[], + limit: 10, + category: 'tools', + }); + expect(byCat.skills.length).toBe(1); + expect(byCat.skills[0].name).toBe(a.name); + + const bySearch = await methods.listSkillsByAccess({ + accessibleIds: ids as unknown as mongoose.Types.ObjectId[], + limit: 10, + search: 'beta', + }); + expect(bySearch.skills.length).toBe(1); + expect(bySearch.skills[0].name).toBe('beta-skill'); + }); +}); + +describe('SkillFile methods', () => { + it('upsertSkillFile bumps parent skill version and updates fileCount', async () => { + const { skill } = await methods.createSkill(makeSkillInput()); + expect(skill.version).toBe(1); + expect(skill.fileCount).toBe(0); + + await methods.upsertSkillFile({ + skillId: skill._id, + relativePath: 'scripts/parse.sh', + file_id: 'file-abc', + filename: 'parse.sh', + filepath: '/tmp/parse.sh', + source: 'local', + mimeType: 'text/plain', + bytes: 10, + author: owner._id, + }); + + const updated = await methods.getSkillById(skill._id); + expect(updated?.version).toBe(2); + expect(updated?.fileCount).toBe(1); + }); + + it('upsertSkillFile replaces an existing row and bumps version again', async () => { + const { skill } = await methods.createSkill(makeSkillInput()); + await methods.upsertSkillFile({ + skillId: skill._id, + relativePath: 'scripts/parse.sh', + file_id: 'file-1', + filename: 'parse.sh', + filepath: '/tmp/v1', + source: 'local', + mimeType: 'text/plain', + bytes: 10, + author: owner._id, + }); + await methods.upsertSkillFile({ + skillId: skill._id, + relativePath: 'scripts/parse.sh', + file_id: 'file-2', + filename: 'parse.sh', + filepath: '/tmp/v2', + source: 'local', + mimeType: 'text/plain', + bytes: 20, + author: owner._id, + }); + const after = await methods.getSkillById(skill._id); + expect(after?.fileCount).toBe(1); + expect(after?.version).toBe(3); + const files = await methods.listSkillFiles(skill._id); + expect(files.length).toBe(1); + expect(files[0].file_id).toBe('file-2'); + }); + + it('deleteSkillFile recounts and bumps version', async () => { + const { skill } = await methods.createSkill(makeSkillInput()); + await methods.upsertSkillFile({ + skillId: skill._id, + relativePath: 'scripts/a.sh', + file_id: 'f1', + filename: 'a.sh', + filepath: '/a', + source: 'local', + mimeType: 'text/plain', + bytes: 1, + author: owner._id, + }); + await methods.upsertSkillFile({ + skillId: skill._id, + relativePath: 'scripts/b.sh', + file_id: 'f2', + filename: 'b.sh', + filepath: '/b', + source: 'local', + mimeType: 'text/plain', + bytes: 1, + author: owner._id, + }); + const res = await methods.deleteSkillFile(skill._id, 'scripts/a.sh'); + expect(res.deleted).toBe(true); + const after = await methods.getSkillById(skill._id); + expect(after?.fileCount).toBe(1); + expect(after?.version).toBe(4); + }); + + it('rejects invalid paths via upsertSkillFile', async () => { + const { skill } = await methods.createSkill(makeSkillInput()); + await expect( + methods.upsertSkillFile({ + skillId: skill._id, + relativePath: '../evil', + file_id: 'f1', + filename: 'evil', + filepath: '/x', + source: 'local', + mimeType: 'text/plain', + bytes: 1, + author: owner._id, + }), + ).rejects.toMatchObject({ code: 'SKILL_FILE_VALIDATION_FAILED' }); + }); +}); + +describe('deleteUserSkills', () => { + it('removes sole-owned skills only', async () => { + const { skill: mine } = await methods.createSkill(makeSkillInput({ name: 'mine' })); + await grantOwner(mine._id); + + // Create a second skill owned by someone else + const shared = await Skill.create({ + name: 'shared', + description: 'shared skill', + body: '', + frontmatter: {}, + category: '', + author: other._id, + authorName: other.name ?? 'Other', + version: 1, + source: 'inline', + fileCount: 0, + }); + const sharedId = (shared.toObject() as { _id: mongoose.Types.ObjectId })._id; + // Owner user has viewer access to shared β€” so it's NOT sole-owned + const viewerRole = (await AccessRole.findOne({ + accessRoleId: AccessRoleIds.SKILL_VIEWER, + }).lean()) as { _id: mongoose.Types.ObjectId; permBits: number } | null; + await aclMethods.grantPermission( + PrincipalType.USER, + owner._id, + ResourceType.SKILL, + sharedId, + viewerRole!.permBits, + other._id, + undefined, + viewerRole!._id, + ); + // And other is owner of the shared skill + const ownerRole = (await AccessRole.findOne({ + accessRoleId: AccessRoleIds.SKILL_OWNER, + }).lean()) as { _id: mongoose.Types.ObjectId; permBits: number } | null; + await aclMethods.grantPermission( + PrincipalType.USER, + other._id, + ResourceType.SKILL, + sharedId, + ownerRole!.permBits, + other._id, + undefined, + ownerRole!._id, + ); + + const deleted = await methods.deleteUserSkills(owner._id as mongoose.Types.ObjectId); + expect(deleted).toBe(1); + expect(await Skill.countDocuments()).toBe(1); + expect(await Skill.countDocuments({ _id: sharedId })).toBe(1); + }); +}); diff --git a/packages/data-schemas/src/methods/skill.ts b/packages/data-schemas/src/methods/skill.ts new file mode 100644 index 0000000000..9a30ec5fc7 --- /dev/null +++ b/packages/data-schemas/src/methods/skill.ts @@ -0,0 +1,883 @@ +import { ResourceType } from 'librechat-data-provider'; +import type { Model, Types, FilterQuery } from 'mongoose'; +import type { + ISkill, + ISkillDocument, + ISkillFile, + ISkillFileDocument, + ISkillSummary, +} from '~/types/skill'; +import { isValidObjectIdString } from '~/utils/objectId'; +import { escapeRegExp } from '~/utils/string'; +import logger from '~/config/winston'; + +/** ---------- Validation helpers (pure) ---------- */ + +/** + * A single validation issue emitted by a skill validator. Most issues are + * errors and block the mutation; some are warnings (e.g. "description is + * awfully short, Claude may undertrigger the skill") that surface inline + * coaching without rejecting the request. + */ +export type ValidationIssue = { + field: string; + code: string; + message: string; + /** + * Defaults to `'error'` when omitted. Errors cause `createSkill` / + * `updateSkill` to throw with code `SKILL_VALIDATION_FAILED`; warnings + * are surfaced on successful responses so the UI can show inline feedback. + */ + severity?: 'error' | 'warning'; +}; + +/** Partition an issue list into blocking errors and non-blocking warnings. */ +export function partitionIssues(issues: ValidationIssue[]): { + errors: ValidationIssue[]; + warnings: ValidationIssue[]; +} { + const errors: ValidationIssue[] = []; + const warnings: ValidationIssue[] = []; + for (const issue of issues) { + if (issue.severity === 'warning') { + warnings.push(issue); + } else { + errors.push(issue); + } + } + return { errors, warnings }; +} + +const SKILL_NAME_MAX = 64; +const SKILL_DESCRIPTION_MAX = 1024; +const SKILL_DESCRIPTION_SHORT_THRESHOLD = 20; +const SKILL_DISPLAY_TITLE_MAX = 128; +const SKILL_BODY_MAX = 100_000; +const SKILL_FILE_PATH_MAX = 500; +const SKILL_NAME_PATTERN = /^[a-z0-9][a-z0-9-]*$/; +const RELATIVE_PATH_CHARS = /^[a-zA-Z0-9._\-/]+$/; + +/** + * Brand namespaces reserved for Anthropic-published skills and first-party + * bundles. Matched as prefixes, so `anthropic-helper` is rejected but + * `research-anthropic-helper` is fine. + */ +const RESERVED_NAME_PREFIXES = ['anthropic-', 'claude-']; + +/** + * Slash-command names that collide with LibreChat / Claude Code CLI commands. + * A skill with one of these names would shadow a real command in any + * slash-command UI. Matched exactly (not as prefix). + */ +const RESERVED_NAME_WORDS = new Set([ + 'help', + 'clear', + 'compact', + 'model', + 'exit', + 'quit', + 'settings', + 'anthropic', + 'claude', +]); + +export function validateSkillName(name: unknown): ValidationIssue[] { + const issues: ValidationIssue[] = []; + if (typeof name !== 'string' || name.length === 0) { + issues.push({ field: 'name', code: 'REQUIRED', message: 'Name is required' }); + return issues; + } + if (name.length > SKILL_NAME_MAX) { + issues.push({ + field: 'name', + code: 'TOO_LONG', + message: `Name must be ${SKILL_NAME_MAX} characters or less`, + }); + } + if (!SKILL_NAME_PATTERN.test(name)) { + issues.push({ + field: 'name', + code: 'INVALID_FORMAT', + message: + 'Name must be kebab-case: start with a lowercase letter or digit and contain only lowercase letters, digits, and hyphens', + }); + } + const lowered = name.toLowerCase(); + if (RESERVED_NAME_PREFIXES.some((prefix) => lowered.startsWith(prefix))) { + issues.push({ + field: 'name', + code: 'RESERVED_PREFIX', + message: `Name cannot start with ${RESERVED_NAME_PREFIXES.map((p) => `"${p}"`).join(' or ')}`, + }); + } + if (RESERVED_NAME_WORDS.has(lowered)) { + issues.push({ + field: 'name', + code: 'RESERVED_WORD', + message: `"${name}" is a reserved name`, + }); + } + return issues; +} + +export function validateSkillDescription(description: unknown): ValidationIssue[] { + const issues: ValidationIssue[] = []; + if (typeof description !== 'string' || description.trim().length === 0) { + issues.push({ + field: 'description', + code: 'REQUIRED', + message: 'Description is required', + }); + return issues; + } + if (description.length > SKILL_DESCRIPTION_MAX) { + issues.push({ + field: 'description', + code: 'TOO_LONG', + message: `Description must be ${SKILL_DESCRIPTION_MAX} characters or less`, + }); + } + if (description.trim().length < SKILL_DESCRIPTION_SHORT_THRESHOLD) { + issues.push({ + field: 'description', + code: 'TOO_SHORT', + severity: 'warning', + message: + 'Short descriptions may cause Claude to miss triggering opportunities β€” aim for a concrete "when to use this skill" sentence.', + }); + } + return issues; +} + +export function validateSkillBody(body: unknown): ValidationIssue[] { + const issues: ValidationIssue[] = []; + if (body !== undefined && typeof body !== 'string') { + issues.push({ field: 'body', code: 'INVALID_TYPE', message: 'Body must be a string' }); + return issues; + } + if (typeof body === 'string' && body.length > SKILL_BODY_MAX) { + issues.push({ + field: 'body', + code: 'TOO_LONG', + message: `Body must be ${SKILL_BODY_MAX} characters or less`, + }); + } + return issues; +} + +export function validateSkillDisplayTitle(displayTitle: unknown): ValidationIssue[] { + if (displayTitle === undefined || displayTitle === null) { + return []; + } + if (typeof displayTitle !== 'string') { + return [ + { field: 'displayTitle', code: 'INVALID_TYPE', message: 'Display title must be a string' }, + ]; + } + if (displayTitle.length > SKILL_DISPLAY_TITLE_MAX) { + return [ + { + field: 'displayTitle', + code: 'TOO_LONG', + message: `Display title must be ${SKILL_DISPLAY_TITLE_MAX} characters or less`, + }, + ]; + } + return []; +} + +/** + * Known fields allowed inside a skill's YAML frontmatter. Anything else is + * rejected in strict mode. The list is derived from Anthropic's Agent Skills + * spec plus the fields LibreChat needs to pass through (`name`/`description` + * are duplicated from the top-level columns because real `SKILL.md` files + * include them in their frontmatter block). + */ +const ALLOWED_FRONTMATTER_KEYS = new Set([ + 'name', + 'description', + 'when-to-use', + 'allowed-tools', + 'arguments', + 'argument-hint', + 'user-invocable', + 'disable-model-invocation', + 'model', + 'effort', + 'context', + 'agent', + 'paths', + 'shell', + 'hooks', + 'version', + 'metadata', +]); + +const FRONTMATTER_MAX_STRING = 2000; +const FRONTMATTER_MAX_ARRAY = 100; +const FRONTMATTER_MAX_DEPTH = 4; + +type FrontmatterKind = 'string' | 'number' | 'boolean' | 'stringArray'; + +const FRONTMATTER_KIND: Record = { + name: 'string', + description: 'string', + 'when-to-use': 'string', + 'allowed-tools': ['string', 'stringArray'], + arguments: ['string', 'stringArray'], + 'argument-hint': 'string', + 'user-invocable': 'boolean', + 'disable-model-invocation': 'boolean', + model: 'string', + effort: ['string', 'number'], + context: 'string', + agent: 'string', + paths: ['string', 'stringArray'], + shell: 'string', + version: 'string', +}; + +function isPlainObject(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} + +function isStringArray(value: unknown): value is string[] { + return ( + Array.isArray(value) && + value.length <= FRONTMATTER_MAX_ARRAY && + value.every((v) => typeof v === 'string' && v.length <= FRONTMATTER_MAX_STRING) + ); +} + +function matchesKind(value: unknown, kind: FrontmatterKind): boolean { + if (kind === 'string') { + return typeof value === 'string' && value.length <= FRONTMATTER_MAX_STRING; + } + if (kind === 'number') { + return typeof value === 'number' && Number.isFinite(value); + } + if (kind === 'boolean') { + return typeof value === 'boolean'; + } + return isStringArray(value); +} + +/** + * Shallow structural sanity check for `hooks`/`metadata` objects. We don't + * know their full schema yet, so we just verify they are plain objects with + * JSON-serializable leaf values up to a max depth β€” enough to block pathological + * payloads without constraining legitimate frontmatter extensions. + */ +function isJsonSafe(value: unknown, depth: number): boolean { + if (depth > FRONTMATTER_MAX_DEPTH) { + return false; + } + if (value === null) return true; + const t = typeof value; + if (t === 'string') return (value as string).length <= FRONTMATTER_MAX_STRING; + if (t === 'number') return Number.isFinite(value); + if (t === 'boolean') return true; + if (Array.isArray(value)) { + if (value.length > FRONTMATTER_MAX_ARRAY) return false; + return value.every((v) => isJsonSafe(v, depth + 1)); + } + if (isPlainObject(value)) { + return Object.values(value).every((v) => isJsonSafe(v, depth + 1)); + } + return false; +} + +/** + * Validate a skill's structured YAML frontmatter. Strict mode: unknown keys + * are rejected so any expansion of the allowed set is an intentional code + * change. Known keys are type-checked against `FRONTMATTER_KIND`; `hooks` and + * `metadata` fall back to a shallow JSON-safety check because their full + * schemas live outside this module. + */ +export function validateSkillFrontmatter(frontmatter: unknown): ValidationIssue[] { + if (frontmatter === undefined || frontmatter === null) { + return []; + } + if (!isPlainObject(frontmatter)) { + return [ + { + field: 'frontmatter', + code: 'INVALID_TYPE', + message: 'Frontmatter must be a plain object', + }, + ]; + } + + const issues: ValidationIssue[] = []; + for (const [key, value] of Object.entries(frontmatter)) { + if (!ALLOWED_FRONTMATTER_KEYS.has(key)) { + issues.push({ + field: `frontmatter.${key}`, + code: 'UNKNOWN_KEY', + message: `"${key}" is not a recognized frontmatter key`, + }); + continue; + } + + if (key === 'hooks' || key === 'metadata') { + if (!isPlainObject(value) || !isJsonSafe(value, 0)) { + issues.push({ + field: `frontmatter.${key}`, + code: 'INVALID_SHAPE', + message: `"${key}" must be a plain JSON-safe object (max depth ${FRONTMATTER_MAX_DEPTH}, max string ${FRONTMATTER_MAX_STRING})`, + }); + } + continue; + } + + const expected = FRONTMATTER_KIND[key]; + if (!expected) { + continue; + } + const kinds = Array.isArray(expected) ? expected : [expected]; + if (!kinds.some((kind) => matchesKind(value, kind))) { + issues.push({ + field: `frontmatter.${key}`, + code: 'INVALID_TYPE', + message: `"${key}" must be ${kinds.join(' or ')}`, + }); + } + } + return issues; +} + +export function validateRelativePath(relativePath: unknown): ValidationIssue[] { + const issues: ValidationIssue[] = []; + if (typeof relativePath !== 'string' || relativePath.length === 0) { + issues.push({ + field: 'relativePath', + code: 'REQUIRED', + message: 'Relative path is required', + }); + return issues; + } + if (relativePath.length > SKILL_FILE_PATH_MAX) { + issues.push({ + field: 'relativePath', + code: 'TOO_LONG', + message: `Relative path must be ${SKILL_FILE_PATH_MAX} characters or less`, + }); + } + if (relativePath.startsWith('/') || relativePath.startsWith('\\')) { + issues.push({ + field: 'relativePath', + code: 'ABSOLUTE_PATH', + message: 'Relative path must not start with a slash', + }); + } + if (!RELATIVE_PATH_CHARS.test(relativePath)) { + issues.push({ + field: 'relativePath', + code: 'INVALID_CHARS', + message: 'Relative path contains invalid characters', + }); + } + const segments = relativePath.split('/'); + if (segments.some((s) => s === '' || s === '.' || s === '..')) { + issues.push({ + field: 'relativePath', + code: 'TRAVERSAL', + message: 'Relative path cannot contain empty segments or "." / ".."', + }); + } + if (relativePath === 'SKILL.md' || segments[0] === 'SKILL.md') { + issues.push({ + field: 'relativePath', + code: 'RESERVED', + message: 'SKILL.md is managed via the skill body, not file uploads', + }); + } + return issues; +} + +export function inferSkillFileCategory( + relativePath: string, +): 'script' | 'reference' | 'asset' | 'other' { + const [top] = relativePath.split('/'); + if (top === 'scripts') return 'script'; + if (top === 'references') return 'reference'; + if (top === 'assets') return 'asset'; + return 'other'; +} + +/** ---------- Method factory ---------- */ + +export interface SkillDeps { + /** Removes all ACL entries for a resource. Injected from PermissionService. */ + removeAllPermissions: (params: { resourceType: string; resourceId: unknown }) => Promise; + /** Returns resource IDs solely owned by the given user. From createAclEntryMethods. */ + getSoleOwnedResourceIds: ( + userObjectId: Types.ObjectId, + resourceTypes: string | string[], + ) => Promise; +} + +export type CreateSkillInput = { + name: string; + displayTitle?: string; + description: string; + body?: string; + frontmatter?: Record; + category?: string; + author: Types.ObjectId; + authorName: string; + source?: 'inline' | 'github' | 'notion'; + sourceMetadata?: Record; + tenantId?: string; +}; + +export type UpdateSkillInput = { + name?: string; + displayTitle?: string; + description?: string; + body?: string; + frontmatter?: Record; + category?: string; +}; + +export type UpsertSkillFileInput = { + skillId: Types.ObjectId | string; + relativePath: string; + file_id: string; + filename: string; + filepath: string; + source: string; + mimeType: string; + bytes: number; + isExecutable?: boolean; + author: Types.ObjectId; + tenantId?: string; +}; + +export type ListSkillsByAccessParams = { + accessibleIds: Types.ObjectId[]; + category?: string; + search?: string; + limit: number; + cursor?: string | null; +}; + +export type ListSkillsByAccessResult = { + /** + * Summary rows β€” `body` and `frontmatter` are intentionally omitted at the + * query projection layer to keep list payloads small. Callers that need the + * full document must fetch the detail via `getSkillById`. + */ + skills: Array; + has_more: boolean; + after: string | null; +}; + +export type UpdateSkillResult = + | { + status: 'updated'; + skill: ISkill & { _id: Types.ObjectId }; + warnings: ValidationIssue[]; + } + | { status: 'conflict'; current: ISkill & { _id: Types.ObjectId } } + | { status: 'not_found' }; + +export type CreateSkillResult = { + skill: ISkill & { _id: Types.ObjectId }; + warnings: ValidationIssue[]; +}; + +export function createSkillMethods(mongoose: typeof import('mongoose'), deps: SkillDeps) { + const { ObjectId } = mongoose.Types; + + function buildSkillFilter( + params: Pick, + ): FilterQuery { + const filter: FilterQuery = { + _id: { $in: params.accessibleIds }, + }; + if (params.category && params.category.length > 0) { + filter.category = params.category; + } + if (params.search && params.search.length > 0) { + const rx = new RegExp(escapeRegExp(params.search), 'i'); + filter.$or = [{ name: rx }, { description: rx }, { displayTitle: rx }]; + } + return filter; + } + + function decodeCursor( + cursor: string | null | undefined, + ): { updatedAt: Date; _id: Types.ObjectId } | null { + if (!cursor || cursor === 'undefined' || cursor === 'null') { + return null; + } + try { + const decoded = JSON.parse(Buffer.from(cursor, 'base64').toString('utf8')) as { + updatedAt?: string; + _id?: string; + }; + if ( + !decoded.updatedAt || + !decoded._id || + Number.isNaN(new Date(decoded.updatedAt).getTime()) || + !isValidObjectIdString(decoded._id) + ) { + return null; + } + return { updatedAt: new Date(decoded.updatedAt), _id: new ObjectId(decoded._id) }; + } catch (error) { + logger.warn(`[skill.decodeCursor] Invalid cursor: ${(error as Error).message}`); + return null; + } + } + + function encodeCursor(row: { updatedAt: Date; _id: Types.ObjectId }): string { + return Buffer.from( + JSON.stringify({ updatedAt: row.updatedAt.toISOString(), _id: row._id.toString() }), + ).toString('base64'); + } + + async function createSkill(data: CreateSkillInput): Promise { + const issues: ValidationIssue[] = [ + ...validateSkillName(data.name), + ...validateSkillDescription(data.description), + ...validateSkillBody(data.body), + ...validateSkillDisplayTitle(data.displayTitle), + ...validateSkillFrontmatter(data.frontmatter), + ]; + const { errors, warnings } = partitionIssues(issues); + if (errors.length > 0) { + const error = new Error('Skill validation failed'); + (error as Error & { issues?: ValidationIssue[]; code?: string }).issues = errors; + (error as Error & { code?: string }).code = 'SKILL_VALIDATION_FAILED'; + throw error; + } + + const Skill = mongoose.models.Skill as Model; + + // Application-level uniqueness check on (name, author, tenantId). + // The unique index in the schema is the persistent guarantee, but Mongoose + // creates indexes asynchronously and tests can race ahead of index creation, + // so we also enforce it here for deterministic behavior and a clean error. + const existing = await Skill.findOne({ + name: data.name, + author: data.author, + tenantId: data.tenantId ?? null, + }) + .select('_id') + .lean(); + if (existing) { + const error = new Error(`A skill with name "${data.name}" already exists for this author`); + (error as Error & { code?: string | number }).code = 11000; + throw error; + } + + const doc = await Skill.create({ + name: data.name, + displayTitle: data.displayTitle, + description: data.description, + body: data.body ?? '', + frontmatter: data.frontmatter ?? {}, + category: data.category ?? '', + author: data.author, + authorName: data.authorName, + version: 1, + source: data.source ?? 'inline', + sourceMetadata: data.sourceMetadata, + fileCount: 0, + tenantId: data.tenantId, + }); + return { + skill: doc.toObject() as unknown as ISkill & { _id: Types.ObjectId }, + warnings, + }; + } + + async function getSkillById( + id: string | Types.ObjectId, + ): Promise<(ISkill & { _id: Types.ObjectId }) | null> { + if (typeof id === 'string' && !isValidObjectIdString(id)) { + return null; + } + const Skill = mongoose.models.Skill as Model; + const doc = await Skill.findById(id).lean(); + return (doc as unknown as (ISkill & { _id: Types.ObjectId }) | null) ?? null; + } + + async function listSkillsByAccess( + params: ListSkillsByAccessParams, + ): Promise { + const Skill = mongoose.models.Skill as Model; + const limit = Math.min(Math.max(1, params.limit || 20), 100); + + const baseFilter = buildSkillFilter(params); + const cursor = decodeCursor(params.cursor); + + let filter: FilterQuery = baseFilter; + if (cursor) { + const cursorCondition: FilterQuery = { + $or: [ + { updatedAt: { $lt: cursor.updatedAt } }, + { updatedAt: cursor.updatedAt, _id: { $gt: cursor._id } }, + ], + }; + filter = { $and: [baseFilter, cursorCondition] }; + } + + const rows = await Skill.find(filter) + .sort({ updatedAt: -1, _id: 1 }) + .limit(limit + 1) + .select( + 'name displayTitle description category author authorName version source sourceMetadata fileCount tenantId createdAt updatedAt', + ) + .lean(); + + const has_more = rows.length > limit; + const sliced = has_more ? rows.slice(0, limit) : rows; + const last = sliced[sliced.length - 1]; + const after = + has_more && last + ? encodeCursor({ + updatedAt: last.updatedAt as Date, + _id: last._id as Types.ObjectId, + }) + : null; + + return { + skills: sliced as unknown as Array, + has_more, + after, + }; + } + + async function updateSkill(params: { + id: string; + expectedVersion: number; + update: UpdateSkillInput; + }): Promise { + const { id, expectedVersion, update } = params; + if (!isValidObjectIdString(id)) { + return { status: 'not_found' }; + } + + const issues: ValidationIssue[] = []; + if (update.name !== undefined) issues.push(...validateSkillName(update.name)); + if (update.description !== undefined) + issues.push(...validateSkillDescription(update.description)); + if (update.body !== undefined) issues.push(...validateSkillBody(update.body)); + if (update.displayTitle !== undefined) + issues.push(...validateSkillDisplayTitle(update.displayTitle)); + if (update.frontmatter !== undefined) + issues.push(...validateSkillFrontmatter(update.frontmatter)); + const { errors, warnings } = partitionIssues(issues); + if (errors.length > 0) { + const error = new Error('Skill validation failed'); + (error as Error & { issues?: ValidationIssue[]; code?: string }).issues = errors; + (error as Error & { code?: string }).code = 'SKILL_VALIDATION_FAILED'; + throw error; + } + + const Skill = mongoose.models.Skill as Model; + const setPayload: Record = {}; + if (update.name !== undefined) setPayload.name = update.name; + if (update.displayTitle !== undefined) setPayload.displayTitle = update.displayTitle; + if (update.description !== undefined) setPayload.description = update.description; + if (update.body !== undefined) setPayload.body = update.body; + if (update.frontmatter !== undefined) setPayload.frontmatter = update.frontmatter; + if (update.category !== undefined) setPayload.category = update.category; + + const result = await Skill.findOneAndUpdate( + { _id: new ObjectId(id), version: expectedVersion }, + { $set: setPayload, $inc: { version: 1 } }, + { new: true }, + ).lean(); + + if (result) { + return { + status: 'updated', + skill: result as unknown as ISkill & { _id: Types.ObjectId }, + warnings, + }; + } + + const current = await Skill.findById(id).lean(); + if (!current) { + return { status: 'not_found' }; + } + return { + status: 'conflict', + current: current as unknown as ISkill & { _id: Types.ObjectId }, + }; + } + + async function deleteSkill(id: string): Promise<{ deleted: boolean }> { + if (!isValidObjectIdString(id)) { + return { deleted: false }; + } + const Skill = mongoose.models.Skill as Model; + const SkillFile = mongoose.models.SkillFile as Model; + const objectId = new ObjectId(id); + const res = await Skill.deleteOne({ _id: objectId }); + if (!res.deletedCount) { + return { deleted: false }; + } + await SkillFile.deleteMany({ skillId: objectId }); + try { + await deps.removeAllPermissions({ resourceType: ResourceType.SKILL, resourceId: id }); + } catch (error) { + logger.error(`[deleteSkill] Error removing permissions for ${id}:`, error); + } + return { deleted: true }; + } + + async function deleteUserSkills(userId: Types.ObjectId | string): Promise { + const userObjectId = typeof userId === 'string' ? new ObjectId(userId) : userId; + const Skill = mongoose.models.Skill as Model; + const soleOwned = await deps.getSoleOwnedResourceIds(userObjectId, ResourceType.SKILL); + if (soleOwned.length === 0) { + return 0; + } + const SkillFile = mongoose.models.SkillFile as Model; + await SkillFile.deleteMany({ skillId: { $in: soleOwned } }); + const res = await Skill.deleteMany({ _id: { $in: soleOwned } }); + await Promise.allSettled( + soleOwned.map((rid) => + deps + .removeAllPermissions({ + resourceType: ResourceType.SKILL, + resourceId: rid.toString(), + }) + .catch((error) => + logger.error(`[deleteUserSkills] Error removing permissions for ${rid}:`, error), + ), + ), + ); + return res.deletedCount ?? 0; + } + + /** + * Atomically bumps `Skill.version` and adjusts `fileCount` by `delta`. + * `delta` is `+1` when a new file is inserted, `-1` when one is deleted, and + * `0` when an existing file is replaced in place. + * + * NOTE on consistency: this runs as a **separate** MongoDB operation from + * the `upsertSkillFile` / `deleteSkillFile` that triggers it. MongoDB only + * provides multi-document ACID via transactions (which require a replica + * set), and LibreChat does not currently require that deployment shape. In + * the rare case where a SkillFile write succeeds but the subsequent + * `findByIdAndUpdate` here fails (connection drop, primary failover mid- + * request), the `fileCount` on the parent Skill will drift from the true + * row count until the next successful upsert/delete corrects it. Options if + * this ever shows up in practice: + * - wrap both ops in a transaction (requires a replica set) + * - periodic reconciliation: `fileCount = count(skill_files where skillId = ?)` + * - treat `fileCount` as advisory and recompute on read when accuracy + * matters + * For phase 1, skill files are stubbed at the upload boundary, so the risk + * window doesn't open in practice. + */ + async function bumpSkillVersionAndAdjustFileCount( + skillId: Types.ObjectId | string, + delta: number, + ): Promise { + const Skill = mongoose.models.Skill as Model; + const updateOps: Record> = { $inc: { version: 1 } }; + if (delta !== 0) { + updateOps.$inc.fileCount = delta; + } + await Skill.findByIdAndUpdate(skillId, updateOps); + } + + async function listSkillFiles( + skillId: Types.ObjectId | string, + ): Promise> { + const SkillFile = mongoose.models.SkillFile as Model; + const rows = await SkillFile.find({ skillId }).sort({ relativePath: 1 }).lean(); + return rows as unknown as Array; + } + + async function upsertSkillFile( + row: UpsertSkillFileInput, + ): Promise { + const issues = validateRelativePath(row.relativePath); + if (issues.length > 0) { + const error = new Error('Skill file validation failed'); + (error as Error & { issues?: ValidationIssue[]; code?: string }).issues = issues; + (error as Error & { code?: string }).code = 'SKILL_FILE_VALIDATION_FAILED'; + throw error; + } + const SkillFile = mongoose.models.SkillFile as Model; + const category = inferSkillFileCategory(row.relativePath); + // Atomic new-vs-replace detection: with `new: false, upsert: true`, + // `findOneAndUpdate` returns the pre-update document (or null if the doc + // did not exist and was just inserted). Checking the return value replaces + // a non-atomic `findOne` + `upsert` pair that could double-count on + // concurrent uploads of the same (skillId, relativePath). + const previous = await SkillFile.findOneAndUpdate( + { skillId: row.skillId, relativePath: row.relativePath }, + { + $set: { + skillId: row.skillId, + relativePath: row.relativePath, + file_id: row.file_id, + filename: row.filename, + filepath: row.filepath, + source: row.source, + mimeType: row.mimeType, + bytes: row.bytes, + category, + isExecutable: row.isExecutable ?? false, + author: row.author, + tenantId: row.tenantId, + }, + }, + { new: false, upsert: true }, + ).lean(); + const delta = previous ? 0 : 1; + await bumpSkillVersionAndAdjustFileCount(row.skillId, delta); + + // Fetch the current (post-upsert) document for the caller. This second + // round-trip is an intentional tradeoff for the TOCTOU-safe detection + // above: `new: false` is required to distinguish insert from replace + // atomically, which means `findOneAndUpdate` returns the pre-update + // document (null on insert). A separate `findOne` is the simplest way + // to return the authoritative post-upsert state. Performance impact is + // negligible compared to the file upload I/O this sits behind. + const current = await SkillFile.findOne({ + skillId: row.skillId, + relativePath: row.relativePath, + }).lean(); + return current as unknown as ISkillFile & { _id: Types.ObjectId }; + } + + async function deleteSkillFile( + skillId: Types.ObjectId | string, + relativePath: string, + ): Promise<{ deleted: boolean }> { + const SkillFile = mongoose.models.SkillFile as Model; + const res = await SkillFile.deleteOne({ skillId, relativePath }); + if (!res.deletedCount) { + return { deleted: false }; + } + await bumpSkillVersionAndAdjustFileCount(skillId, -1); + return { deleted: true }; + } + + // The public surface is scoped to methods that handlers and the user + // deletion controller actually call. The per-skill file cascade on + // `deleteSkill` is inlined; there's no need for a separate export. + return { + createSkill, + getSkillById, + listSkillsByAccess, + updateSkill, + deleteSkill, + deleteUserSkills, + listSkillFiles, + upsertSkillFile, + deleteSkillFile, + }; +} + +export type SkillMethods = ReturnType; diff --git a/packages/data-schemas/src/models/index.ts b/packages/data-schemas/src/models/index.ts index 5a8e8f1c2c..dcd88f58d2 100644 --- a/packages/data-schemas/src/models/index.ts +++ b/packages/data-schemas/src/models/index.ts @@ -19,6 +19,8 @@ import { createTransactionModel } from './transaction'; import { createPresetModel } from './preset'; import { createPromptModel } from './prompt'; import { createPromptGroupModel } from './promptGroup'; +import { createSkillModel } from './skill'; +import { createSkillFileModel } from './skillFile'; import { createConversationTagModel } from './conversationTag'; import { createSharedLinkModel } from './sharedLink'; import { createToolCallModel } from './toolCall'; @@ -55,6 +57,8 @@ export function createModels(mongoose: typeof import('mongoose')) { Preset: createPresetModel(mongoose), Prompt: createPromptModel(mongoose), PromptGroup: createPromptGroupModel(mongoose), + Skill: createSkillModel(mongoose), + SkillFile: createSkillFileModel(mongoose), ConversationTag: createConversationTagModel(mongoose), SharedLink: createSharedLinkModel(mongoose), ToolCall: createToolCallModel(mongoose), diff --git a/packages/data-schemas/src/models/skill.ts b/packages/data-schemas/src/models/skill.ts new file mode 100644 index 0000000000..588523f3de --- /dev/null +++ b/packages/data-schemas/src/models/skill.ts @@ -0,0 +1,8 @@ +import skillSchema from '~/schema/skill'; +import { applyTenantIsolation } from '~/models/plugins/tenantIsolation'; +import type { ISkillDocument } from '~/types/skill'; + +export function createSkillModel(mongoose: typeof import('mongoose')) { + applyTenantIsolation(skillSchema); + return mongoose.models.Skill || mongoose.model('Skill', skillSchema); +} diff --git a/packages/data-schemas/src/models/skillFile.ts b/packages/data-schemas/src/models/skillFile.ts new file mode 100644 index 0000000000..230a244910 --- /dev/null +++ b/packages/data-schemas/src/models/skillFile.ts @@ -0,0 +1,10 @@ +import skillFileSchema from '~/schema/skillFile'; +import { applyTenantIsolation } from '~/models/plugins/tenantIsolation'; +import type { ISkillFileDocument } from '~/types/skill'; + +export function createSkillFileModel(mongoose: typeof import('mongoose')) { + applyTenantIsolation(skillFileSchema); + return ( + mongoose.models.SkillFile || mongoose.model('SkillFile', skillFileSchema) + ); +} diff --git a/packages/data-schemas/src/schema/accessRole.ts b/packages/data-schemas/src/schema/accessRole.ts index dbcaf83ddb..9b5bd49010 100644 --- a/packages/data-schemas/src/schema/accessRole.ts +++ b/packages/data-schemas/src/schema/accessRole.ts @@ -15,7 +15,7 @@ const accessRoleSchema = new Schema( description: String, resourceType: { type: String, - enum: ['agent', 'project', 'file', 'promptGroup', 'mcpServer', 'remoteAgent'], + enum: ['agent', 'project', 'file', 'promptGroup', 'mcpServer', 'remoteAgent', 'skill'], required: true, default: 'agent', }, diff --git a/packages/data-schemas/src/schema/role.ts b/packages/data-schemas/src/schema/role.ts index ac478c2a83..ab1fc582e9 100644 --- a/packages/data-schemas/src/schema/role.ts +++ b/packages/data-schemas/src/schema/role.ts @@ -67,6 +67,12 @@ const rolePermissionsSchema = new Schema( [Permissions.SHARE]: { type: Boolean }, [Permissions.SHARE_PUBLIC]: { type: Boolean }, }, + [PermissionTypes.SKILLS]: { + [Permissions.USE]: { type: Boolean }, + [Permissions.CREATE]: { type: Boolean }, + [Permissions.SHARE]: { type: Boolean }, + [Permissions.SHARE_PUBLIC]: { type: Boolean }, + }, }, { _id: false }, ); diff --git a/packages/data-schemas/src/schema/skill.ts b/packages/data-schemas/src/schema/skill.ts new file mode 100644 index 0000000000..219bd291e4 --- /dev/null +++ b/packages/data-schemas/src/schema/skill.ts @@ -0,0 +1,183 @@ +import { Schema } from 'mongoose'; +import type { ISkillDocument } from '~/types/skill'; + +/** Max length for a skill `name` (kebab-case identifier). */ +const SKILL_NAME_MAX_LENGTH = 64; + +/** Max length for a skill `description`. */ +const SKILL_DESCRIPTION_MAX_LENGTH = 1024; + +/** Max length for the skill `body` (the SKILL.md content). */ +const SKILL_BODY_MAX_LENGTH = 100_000; + +/** Max length for the human-friendly `displayTitle`. */ +const SKILL_DISPLAY_TITLE_MAX_LENGTH = 128; + +const skillNamePattern = /^[a-z0-9][a-z0-9-]*$/; + +/** + * Brand namespaces reserved for first-party skills. Matched as prefixes β€” + * `anthropic-helper` is rejected but `my-helper` is fine. Keep this in sync + * with `RESERVED_NAME_PREFIXES` in `methods/skill.ts`. + */ +const RESERVED_NAME_PREFIXES = ['anthropic-', 'claude-']; + +/** + * Slash-command names that collide with LibreChat / Claude Code CLI commands. + * Matched exactly against the full skill name. Keep this in sync with + * `RESERVED_NAME_WORDS` in `methods/skill.ts`. + */ +const RESERVED_NAME_WORDS = new Set([ + 'help', + 'clear', + 'compact', + 'model', + 'exit', + 'quit', + 'settings', + 'anthropic', + 'claude', +]); + +const skillSchema: Schema = new Schema( + { + /** + * Machine-readable identifier. Kebab-case, max 64 chars, unique per + * (author, tenantId). This is what Claude sees in its system prompt when + * deciding to trigger the skill, and what slash-command integrations key + * off β€” so it must be stable, concise, and ASCII-friendly. User-facing + * labels live on `displayTitle` instead. + */ + name: { + type: String, + required: true, + index: true, + maxlength: [SKILL_NAME_MAX_LENGTH, `Name cannot exceed ${SKILL_NAME_MAX_LENGTH} characters`], + validate: { + validator: function (value: string): boolean { + if (!skillNamePattern.test(value)) { + return false; + } + const lowered = value.toLowerCase(); + if (RESERVED_NAME_PREFIXES.some((prefix) => lowered.startsWith(prefix))) { + return false; + } + if (RESERVED_NAME_WORDS.has(lowered)) { + return false; + } + return true; + }, + message: + 'Name must be kebab-case (lowercase letters, digits, hyphens), cannot start with "anthropic-" or "claude-", and cannot be a reserved CLI word.', + }, + }, + /** + * Human-readable label shown in the LibreChat UI (skill list, detail + * header, sharing dialogs). NOT sent to Claude and NOT used to trigger + * the skill β€” `name` + `description` drive triggering. Purely cosmetic: + * useful when the author wants a prettier label than the kebab-case + * identifier while keeping the identifier stable. + */ + displayTitle: { + type: String, + maxlength: [ + SKILL_DISPLAY_TITLE_MAX_LENGTH, + `Display title cannot exceed ${SKILL_DISPLAY_TITLE_MAX_LENGTH} characters`, + ], + }, + /** + * "When to use this skill" sentence that Claude reads from the system + * prompt to decide whether to invoke the skill. This is the highest- + * leverage field for triggering accuracy β€” a vague description causes + * undertriggering. Denormalized from the YAML frontmatter for indexed + * querying. + */ + description: { + type: String, + required: true, + maxlength: [ + SKILL_DESCRIPTION_MAX_LENGTH, + `Description cannot exceed ${SKILL_DESCRIPTION_MAX_LENGTH} characters`, + ], + }, + /** The SKILL.md body (markdown after the YAML frontmatter). */ + body: { + type: String, + default: '', + maxlength: [SKILL_BODY_MAX_LENGTH, `Body cannot exceed ${SKILL_BODY_MAX_LENGTH} characters`], + }, + /** + * Structured YAML frontmatter bag (everything except `name`/`description`, + * which live as first-class columns). Validated in strict mode against + * `validateSkillFrontmatter` before write β€” unknown keys are rejected + * so any expansion of the allowed set is an explicit code change. + */ + frontmatter: { + type: Schema.Types.Mixed, + default: {}, + }, + category: { + type: String, + default: '', + index: true, + }, + author: { + type: Schema.Types.ObjectId, + ref: 'User', + required: true, + index: true, + }, + authorName: { + type: String, + required: true, + }, + version: { + type: Number, + default: 1, + min: 1, + }, + /** + * Provenance of this skill's canonical definition. + * + * - `inline` β€” authored directly inside LibreChat (the only path wired + * up in phase 1). + * - `github` / `notion` β€” **reserved for phase 2+ external sync**. No + * code path currently produces these values. The column exists now so + * a future sync worker can populate `source` + `sourceMetadata` without + * a schema migration. + */ + source: { + type: String, + enum: ['inline', 'github', 'notion'], + default: 'inline', + }, + /** + * Arbitrary JSON provenance payload keyed by `source`. Phase 2+ sync + * workers will use this to store the upstream commit SHA (github), + * page id (notion), etc. Unused in phase 1 β€” kept `Mixed` to avoid + * committing to a shape before the sync paths exist. + */ + sourceMetadata: { + type: Schema.Types.Mixed, + }, + fileCount: { + type: Number, + default: 0, + min: 0, + }, + tenantId: { + type: String, + index: true, + }, + }, + { + timestamps: true, + }, +); + +skillSchema.index({ author: 1, tenantId: 1 }); +skillSchema.index({ category: 1, updatedAt: -1 }); +skillSchema.index({ updatedAt: -1, _id: 1 }); +skillSchema.index({ name: 1, author: 1, tenantId: 1 }, { unique: true }); + +export default skillSchema; diff --git a/packages/data-schemas/src/schema/skillFile.ts b/packages/data-schemas/src/schema/skillFile.ts new file mode 100644 index 0000000000..af2371894f --- /dev/null +++ b/packages/data-schemas/src/schema/skillFile.ts @@ -0,0 +1,102 @@ +import { Schema } from 'mongoose'; +import type { ISkillFileDocument } from '~/types/skill'; + +/** Max length for a skill file's relative path (e.g. "scripts/parse.sh"). */ +const SKILL_FILE_PATH_MAX_LENGTH = 500; + +/** Pattern for valid path characters β€” conservative allowlist. */ +const relativePathCharPattern = /^[a-zA-Z0-9._\-/]+$/; + +const skillFileSchema: Schema = new Schema( + { + skillId: { + type: Schema.Types.ObjectId, + ref: 'Skill', + required: true, + index: true, + }, + relativePath: { + type: String, + required: true, + maxlength: [ + SKILL_FILE_PATH_MAX_LENGTH, + `Relative path cannot exceed ${SKILL_FILE_PATH_MAX_LENGTH} characters`, + ], + validate: { + validator: function (value: string): boolean { + if (!value || value.length === 0) { + return false; + } + if (value.startsWith('/') || value.startsWith('\\')) { + return false; + } + if (!relativePathCharPattern.test(value)) { + return false; + } + const segments = value.split('/'); + if (segments.some((s) => s === '' || s === '.' || s === '..')) { + return false; + } + if (value === 'SKILL.md' || segments[0] === 'SKILL.md') { + return false; + } + return true; + }, + message: + 'Relative path must be relative, cannot contain ".." or absolute paths, and cannot be SKILL.md.', + }, + }, + file_id: { + type: String, + required: true, + index: true, + }, + filename: { + type: String, + required: true, + }, + filepath: { + type: String, + required: true, + }, + source: { + type: String, + required: true, + }, + mimeType: { + type: String, + required: true, + }, + bytes: { + type: Number, + required: true, + min: 0, + }, + category: { + type: String, + enum: ['script', 'reference', 'asset', 'other'], + default: 'other', + }, + isExecutable: { + type: Boolean, + default: false, + }, + author: { + type: Schema.Types.ObjectId, + ref: 'User', + required: true, + }, + tenantId: { + type: String, + index: true, + }, + }, + { + timestamps: true, + }, +); + +skillFileSchema.index({ skillId: 1, relativePath: 1 }, { unique: true }); +skillFileSchema.index({ skillId: 1, category: 1 }); + +export default skillFileSchema; diff --git a/packages/data-schemas/src/types/index.ts b/packages/data-schemas/src/types/index.ts index 748ea5d77d..f0b1fee5ec 100644 --- a/packages/data-schemas/src/types/index.ts +++ b/packages/data-schemas/src/types/index.ts @@ -23,6 +23,8 @@ export * from './pluginAuth'; export * from './memory'; /* Prompts */ export * from './prompts'; +/* Skills */ +export * from './skill'; /* Access Control */ export * from './accessRole'; export * from './aclEntry'; diff --git a/packages/data-schemas/src/types/role.ts b/packages/data-schemas/src/types/role.ts index bc85284c34..7ffa6e8849 100644 --- a/packages/data-schemas/src/types/role.ts +++ b/packages/data-schemas/src/types/role.ts @@ -66,6 +66,12 @@ export interface IRole extends Document { [Permissions.SHARE]?: boolean; [Permissions.SHARE_PUBLIC]?: boolean; }; + [PermissionTypes.SKILLS]?: { + [Permissions.USE]?: boolean; + [Permissions.CREATE]?: boolean; + [Permissions.SHARE]?: boolean; + [Permissions.SHARE_PUBLIC]?: boolean; + }; }; tenantId?: string; } diff --git a/packages/data-schemas/src/types/skill.ts b/packages/data-schemas/src/types/skill.ts new file mode 100644 index 0000000000..afaa1b5c95 --- /dev/null +++ b/packages/data-schemas/src/types/skill.ts @@ -0,0 +1,97 @@ +import type { Document, Types } from 'mongoose'; + +/** + * Skill β€” the single source of truth for a LibreChat skill. + * Each document is the full SKILL.md body plus structured frontmatter and metadata. + * The `version` field is an integer monotonic counter used for optimistic concurrency. + */ +export interface ISkill { + /** + * Machine-readable kebab-case identifier. This is what Claude sees in the + * system-prompt skill manifest and what slash-command integrations key off. + * Unique per `(author, tenantId)`. Must be stable across edits β€” a rename + * invalidates any external reference to the skill. + */ + name: string; + /** + * Human-readable label shown only in the LibreChat UI (skill list, detail + * header, sharing dialogs). NOT sent to Claude and NOT part of the trigger + * path β€” `name` + `description` drive triggering. Purely cosmetic: lets an + * author keep a stable kebab-case `name` while showing something prettier + * in the UI. + */ + displayTitle?: string; + /** + * "When to use this skill" sentence. This is the highest-leverage field for + * Claude's triggering decision β€” vague or missing descriptions cause + * undertriggering. Denormalized from the YAML frontmatter onto its own + * column so listings can filter/sort on it without loading `body`. + */ + description: string; + /** The SKILL.md body (markdown after the YAML frontmatter). */ + body: string; + /** + * Structured YAML frontmatter (excluding `name` and `description`, which live as + * top-level columns). Stored as Mongoose Mixed so callers can extend without schema + * churn; validated in strict mode via `validateSkillFrontmatter` β€” unknown keys + * are rejected so expanding the allowed set is an intentional code change. + */ + frontmatter: Record; + category?: string; + author: Types.ObjectId; + authorName: string; + version: number; + /** + * Provenance of this skill's canonical definition. + * - `inline` β€” authored inside LibreChat (the only value phase 1 produces). + * - `github` / `notion` β€” reserved for phase 2+ external sync. Kept in the + * enum so a future sync worker can populate it without a migration. + */ + source: 'inline' | 'github' | 'notion'; + /** + * Provenance payload keyed by `source`. Phase 2+ sync workers will store + * upstream identifiers (commit SHA, page id, etc.) here. Unused in phase 1. + */ + sourceMetadata?: Record; + /** Denormalized count of associated `SkillFile` rows. Kept in sync by skill methods. */ + fileCount: number; + tenantId?: string; + createdAt?: Date; + updatedAt?: Date; + /** Computed from ACL at read time, never persisted. */ + isPublic?: boolean; +} + +export interface ISkillDocument extends ISkill, Document {} + +/** + * Lean summary projection returned by `listSkillsByAccess`. The list query + * uses a narrow `.select()` that omits `body` and `frontmatter` to keep + * payloads small, so those fields are truthfully absent on summary rows. + */ +export type ISkillSummary = Omit; + +/** + * SkillFile β€” metadata for a file bundled inside a skill. + * Blob content lives in the existing file storage layer (local/S3/etc.) and is + * addressed via `source` + `filepath` + `file_id` (mirroring the File schema). + * `(skillId, relativePath)` is unique per skill. + */ +export interface ISkillFile { + skillId: Types.ObjectId; + relativePath: string; + file_id: string; + filename: string; + filepath: string; + source: string; + mimeType: string; + bytes: number; + category: 'script' | 'reference' | 'asset' | 'other'; + isExecutable: boolean; + author: Types.ObjectId; + tenantId?: string; + createdAt?: Date; + updatedAt?: Date; +} + +export interface ISkillFileDocument extends ISkillFile, Document {}