mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 06:52:47 +00:00
🧩 fix: Preserve Deployment Skill IDs on Agents (#14368)
* fix: preserve deployment skills on agents * fix: expose deployment skills to agent viewers * refactor: centralize deployment skill ID merging --------- Co-authored-by: Dennis Schenk <dennis@gridonic.ch>
This commit is contained in:
parent
ade02054c8
commit
3e9f07976a
7 changed files with 133 additions and 8 deletions
|
|
@ -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,
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -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()));
|
||||
|
|
|
|||
|
|
@ -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<import('@librechat/data-schemas').IAgent>}
|
||||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -60,6 +60,7 @@ let getListAgentsByAccess: AgentMethods['getListAgentsByAccess'];
|
|||
let generateActionMetadataHash: AgentMethods['generateActionMetadataHash'];
|
||||
|
||||
const getActions = jest.fn().mockResolvedValue([]);
|
||||
const externalSkillIds = new Set<string>();
|
||||
|
||||
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();
|
||||
|
|
|
|||
|
|
@ -41,6 +41,8 @@ export interface AgentDeps {
|
|||
userObjectId: Types.ObjectId,
|
||||
resourceTypes: string | string[],
|
||||
) => Promise<Types.ObjectId[]>;
|
||||
/** 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<string, unknown>): Promise<IAgent> {
|
||||
const Agent = mongoose.models.Agent as Model<IAgent>;
|
||||
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<string, unknown>;
|
||||
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) {
|
||||
|
|
|
|||
|
|
@ -168,6 +168,8 @@ export interface CreateMethodsDeps {
|
|||
removeAllPermissions?: (params: { resourceType: string; resourceId: unknown }) => Promise<void>;
|
||||
/** 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);
|
||||
|
||||
|
|
|
|||
|
|
@ -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<string[]> {
|
||||
const candidates = [
|
||||
...new Set(skillIds.filter(isValidObjectIdString).map((id) => id.toLowerCase())),
|
||||
|
|
@ -863,7 +865,7 @@ export async function filterExistingSkillIds(
|
|||
{ _id: 1 },
|
||||
).lean<Array<{ _id: Types.ObjectId }>>();
|
||||
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);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue