From 3e9f07976a53b2a458dd2246d8aa44465ba47fe5 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 21 Jul 2026 19:44:27 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=A9=20fix:=20Preserve=20Deployment=20S?= =?UTF-8?q?kill=20IDs=20on=20Agents=20(#14368)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: preserve deployment skills on agents * fix: expose deployment skills to agent viewers * refactor: centralize deployment skill ID merging --------- Co-authored-by: Dennis Schenk --- api/models/index.js | 3 +- api/server/controllers/agents/v1.js | 5 +- api/server/controllers/agents/v1.spec.js | 30 ++++++- .../data-schemas/src/methods/agent.spec.ts | 79 +++++++++++++++++++ packages/data-schemas/src/methods/agent.ts | 15 +++- packages/data-schemas/src/methods/index.ts | 3 + packages/data-schemas/src/methods/skill.ts | 6 +- 7 files changed, 133 insertions(+), 8 deletions(-) diff --git a/api/models/index.js b/api/models/index.js index 2a1cb222f9..07504cf8b8 100644 --- a/api/models/index.js +++ b/api/models/index.js @@ -1,11 +1,12 @@ const mongoose = require('mongoose'); const { createMethods } = require('@librechat/data-schemas'); -const { matchModelName, findMatchingPattern } = require('@librechat/api'); +const { matchModelName, findMatchingPattern, isDeploymentSkillId } = require('@librechat/api'); const getLogStores = require('~/cache/getLogStores'); const methods = createMethods(mongoose, { matchModelName, findMatchingPattern, + isExternalSkillId: isDeploymentSkillId, getCache: getLogStores, }); diff --git a/api/server/controllers/agents/v1.js b/api/server/controllers/agents/v1.js index ec5b81a1a8..32de58835d 100644 --- a/api/server/controllers/agents/v1.js +++ b/api/server/controllers/agents/v1.js @@ -8,6 +8,7 @@ const { agentUpdateSchema, refreshListAvatars, collectEdgeAgentIds, + mergeDeploymentSkillIds, mergeAgentOcrConversion, sanitizeModelParameters, MAX_AVATAR_REFRESH_AGENTS, @@ -1098,7 +1099,9 @@ const getListAgentsHandler = async (req, res) => { resourceType: ResourceType.SKILL, requiredPermissions: PermissionBits.VIEW, }); - accessibleSkillSet = new Set(accessibleSkillIds.map((oid) => oid.toString())); + accessibleSkillSet = new Set( + mergeDeploymentSkillIds(accessibleSkillIds).map((oid) => oid.toString()), + ); } const publicSet = new Set(publiclyAccessibleIds.map((oid) => oid.toString())); diff --git a/api/server/controllers/agents/v1.spec.js b/api/server/controllers/agents/v1.spec.js index 998156b9e2..9c4187b6ab 100644 --- a/api/server/controllers/agents/v1.spec.js +++ b/api/server/controllers/agents/v1.spec.js @@ -38,6 +38,7 @@ jest.mock('sharp', () => jest.mock('@librechat/api', () => ({ ...jest.requireActual('@librechat/api'), + mergeDeploymentSkillIds: jest.fn((ids) => ids), refreshS3Url: jest.fn(), })); @@ -92,7 +93,7 @@ const { getResourcePermissionsMap, } = require('~/server/services/PermissionService'); -const { refreshS3Url } = require('@librechat/api'); +const { mergeDeploymentSkillIds, refreshS3Url } = require('@librechat/api'); /** * @type {import('mongoose').Model} @@ -155,6 +156,7 @@ describe('Agent Controllers - Mass Assignment Protection', () => { // Reset all mocks jest.clearAllMocks(); + mergeDeploymentSkillIds.mockImplementation((ids) => ids); // Setup mock request and response objects mockReq = { @@ -1850,6 +1852,32 @@ describe('Agent Controllers - Mass Assignment Protection', () => { expect(response.data[0].skills_enabled).toBeUndefined(); }); + test('should preserve deployment skill scope for VIEW list callers', async () => { + const deploymentSkillId = new mongoose.Types.ObjectId(); + await Agent.findByIdAndUpdate(agentA1._id, { + skills_enabled: true, + skills: [deploymentSkillId.toString()], + }); + + mockReq.user.id = userB.toString(); + mockReq.query.requiredPermission = String(PermissionBits.VIEW); + findAccessibleResources.mockImplementation(({ resourceType }) => { + if (resourceType === ResourceType.AGENT) { + return Promise.resolve([agentA1._id]); + } + return Promise.resolve([]); + }); + findPubliclyAccessibleResources.mockResolvedValue([]); + mergeDeploymentSkillIds.mockImplementation((ids) => [...ids, deploymentSkillId]); + + await getListAgentsHandler(mockReq, mockRes); + + const response = mockRes.json.mock.calls[0][0]; + expect(response.data).toHaveLength(1); + expect(response.data[0].skills_enabled).toBe(true); + expect(response.data[0].skills).toEqual([deploymentSkillId.toString()]); + }); + test('should preserve enabled skill scope for VIEW list callers with an empty allowlist', async () => { await Agent.findByIdAndUpdate(agentA1._id, { skills_enabled: true, diff --git a/packages/data-schemas/src/methods/agent.spec.ts b/packages/data-schemas/src/methods/agent.spec.ts index 5e93ed4c29..ec55d0fbab 100644 --- a/packages/data-schemas/src/methods/agent.spec.ts +++ b/packages/data-schemas/src/methods/agent.spec.ts @@ -60,6 +60,7 @@ let getListAgentsByAccess: AgentMethods['getListAgentsByAccess']; let generateActionMetadataHash: AgentMethods['generateActionMetadataHash']; const getActions = jest.fn().mockResolvedValue([]); +const externalSkillIds = new Set(); beforeAll(async () => { mongoServer = await MongoMemoryServer.create(); @@ -89,6 +90,7 @@ beforeAll(async () => { removeAllPermissions, getActions, getSoleOwnedResourceIds, + isExternalSkillId: (id) => externalSkillIds.has(id), }); createAgent = methods.createAgent; getAgent = methods.getAgent; @@ -130,6 +132,10 @@ afterAll(async () => { }); describe('Agent Methods', () => { + beforeEach(() => { + externalSkillIds.clear(); + }); + describe('Agent Resource File Operations', () => { beforeEach(async () => { await Agent.deleteMany({}); @@ -585,6 +591,32 @@ describe('Agent Methods', () => { expect(newAgent.skills).toEqual([realSkill._id.toString()]); }); + test('should preserve external skill ids on create', async () => { + const { agentId, authorId } = createTestIds(); + const realSkill = await mongoose.models.Skill.create({ + name: 'create-external-skill', + description: 'Skill backing the external create-time allowlist test.', + author: authorId, + authorName: 'Test Author', + }); + const externalSkillId = new mongoose.Types.ObjectId().toString(); + const danglingId = new mongoose.Types.ObjectId().toString(); + externalSkillIds.add(externalSkillId); + + const newAgent = await createAgent({ + id: agentId, + name: 'External Skill Agent', + provider: 'test', + model: 'test-model', + author: authorId, + skills: [externalSkillId, realSkill._id.toString(), danglingId, externalSkillId], + skills_enabled: true, + }); + + expect(newAgent.skills).toEqual([externalSkillId, realSkill._id.toString()]); + expect(newAgent.skills_enabled).toBe(true); + }); + test('should prune nonexistent skill ids from the allowlist on update', async () => { const { agentId, authorId } = createTestIds(); const realSkill = await mongoose.models.Skill.create({ @@ -612,6 +644,29 @@ describe('Agent Methods', () => { expect(updatedAgent!.skills_enabled).toBe(true); }); + test('should preserve external skill ids on update', async () => { + const { agentId, authorId } = createTestIds(); + const externalSkillId = new mongoose.Types.ObjectId().toString(); + const danglingId = new mongoose.Types.ObjectId().toString(); + externalSkillIds.add(externalSkillId); + + await createAgent({ + id: agentId, + name: 'External Skill Agent', + provider: 'test', + model: 'test-model', + author: authorId, + }); + + const updatedAgent = await updateAgent( + { id: agentId }, + { skills: [danglingId, externalSkillId], skills_enabled: true }, + ); + + expect(updatedAgent!.skills).toEqual([externalSkillId]); + expect(updatedAgent!.skills_enabled).toBe(true); + }); + test('should fail closed when pruning empties the allowlist on update', async () => { const { agentId, authorId } = createTestIds(); const danglingId = new mongoose.Types.ObjectId().toString(); @@ -1914,6 +1969,30 @@ describe('Agent Methods', () => { expect(revertedAgent.skills_enabled).toBe(false); }); + test('should preserve external skill ids when reverting to an older version', async () => { + const agentId = `agent_${uuidv4()}`; + const authorId = new mongoose.Types.ObjectId(); + const externalSkillId = new mongoose.Types.ObjectId().toString(); + externalSkillIds.add(externalSkillId); + + await createAgent({ + id: agentId, + name: 'Revert External Skill Agent', + provider: 'test', + model: 'test-model', + author: authorId, + skills: [externalSkillId], + skills_enabled: true, + }); + + await updateAgent({ id: agentId }, { skills: [], name: 'No Skills Anymore' }); + const revertedAgent = await revertAgentVersion({ id: agentId }, 0); + + expect(revertedAgent.name).toBe('Revert External Skill Agent'); + expect(revertedAgent.skills).toEqual([externalSkillId]); + expect(revertedAgent.skills_enabled).toBe(true); + }); + test('should detect action metadata changes and force version update', async () => { const agentId = `agent_${uuidv4()}`; const authorId = new mongoose.Types.ObjectId(); diff --git a/packages/data-schemas/src/methods/agent.ts b/packages/data-schemas/src/methods/agent.ts index eb725007e5..509e13f811 100644 --- a/packages/data-schemas/src/methods/agent.ts +++ b/packages/data-schemas/src/methods/agent.ts @@ -41,6 +41,8 @@ export interface AgentDeps { userObjectId: Types.ObjectId, resourceTypes: string | string[], ) => Promise; + /** Recognizes skill IDs supplied by an external, non-database registry. */ + isExternalSkillId?: (id: string) => boolean; } /** @@ -328,7 +330,7 @@ export function createAgentMethods( file_ids: string[]; }) => Promise<{ matchedCount: number; modifiedCount: number }>; } { - const { removeAllPermissions, getActions, getSoleOwnedResourceIds } = deps; + const { removeAllPermissions, getActions, getSoleOwnedResourceIds, isExternalSkillId } = deps; /** * Create an agent with the provided data. @@ -336,7 +338,11 @@ export function createAgentMethods( async function createAgent(agentData: Record): Promise { const Agent = mongoose.models.Agent as Model; if (Array.isArray(agentData.skills) && agentData.skills.length > 0) { - const prunedSkills = await filterExistingSkillIds(mongoose, agentData.skills as string[]); + const prunedSkills = await filterExistingSkillIds( + mongoose, + agentData.skills as string[], + isExternalSkillId, + ); agentData.skills = prunedSkills; /** Fail closed when pruning empties a non-empty allowlist — empty + * enabled means the full catalog, and hygiene must never widen scope. */ @@ -486,7 +492,8 @@ export function createAgentMethods( } = currentAgent.toObject() as unknown as Record; const { $push, $pull, $addToSet, ...directUpdates } = updateData; - /** Self-heal: drop allowlist ids whose skill doc no longer exists. + /** Self-heal: drop allowlist ids whose skill no longer exists in the + * database or the external registry. * A dangling id keeps the allowlist non-empty while scoping the * runtime catalog to an empty intersection — silently disabling * skills for the agent. When pruning empties a non-empty allowlist, @@ -498,6 +505,7 @@ export function createAgentMethods( const prunedSkills = await filterExistingSkillIds( mongoose, directUpdates.skills as string[], + isExternalSkillId, ); directUpdates.skills = prunedSkills; updateData.skills = prunedSkills; @@ -988,6 +996,7 @@ export function createAgentMethods( const prunedSkills = await filterExistingSkillIds( mongoose, revertToVersion.skills as string[], + isExternalSkillId, ); revertToVersion.skills = prunedSkills; if (prunedSkills.length === 0) { diff --git a/packages/data-schemas/src/methods/index.ts b/packages/data-schemas/src/methods/index.ts index 18448cccc7..e21115b8c0 100644 --- a/packages/data-schemas/src/methods/index.ts +++ b/packages/data-schemas/src/methods/index.ts @@ -168,6 +168,8 @@ export interface CreateMethodsDeps { removeAllPermissions?: (params: { resourceType: string; resourceId: unknown }) => Promise; /** Returns a cache store for the given key. From getLogStores. */ getCache?: RoleDeps['getCache']; + /** Recognizes agent skill IDs supplied by an external, non-database registry. */ + isExternalSkillId?: AgentDeps['isExternalSkillId']; } /** @@ -243,6 +245,7 @@ export function createMethods( removeAllPermissions, getActions: actionMethods.getActions, getSoleOwnedResourceIds: aclEntryMethods.getSoleOwnedResourceIds, + isExternalSkillId: deps.isExternalSkillId, }; const agentMethods = createAgentMethods(mongoose, agentDeps); diff --git a/packages/data-schemas/src/methods/skill.ts b/packages/data-schemas/src/methods/skill.ts index 57fd3ac319..4b0632a336 100644 --- a/packages/data-schemas/src/methods/skill.ts +++ b/packages/data-schemas/src/methods/skill.ts @@ -838,7 +838,8 @@ function resolveAlwaysApplyFromInput( } /** - * Narrows candidate skill ids to those backed by an existing Skill doc. + * Narrows candidate skill ids to those backed by an existing Skill doc or + * recognized by an injected external skill registry. * Existence-only check (no ACL) so pruning an agent allowlist never drops * skills the saving user merely can't view. Preserves input order, dedupes, * and drops malformed ids — they can't reference anything. Candidates are @@ -850,6 +851,7 @@ function resolveAlwaysApplyFromInput( export async function filterExistingSkillIds( mongoose: typeof import('mongoose'), skillIds: string[], + isExternalSkillId?: (id: string) => boolean, ): Promise { const candidates = [ ...new Set(skillIds.filter(isValidObjectIdString).map((id) => id.toLowerCase())), @@ -863,7 +865,7 @@ export async function filterExistingSkillIds( { _id: 1 }, ).lean>(); const existing = new Set(docs.map((doc) => doc._id.toString())); - return candidates.filter((id) => existing.has(id)); + return candidates.filter((id) => existing.has(id) || isExternalSkillId?.(id) === true); } /**