From f3159f98913bd5faf2c78f6650683fd6946b627d Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 25 Jul 2026 08:19:12 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=A9=20fix:=20Harden=20Agent=20Skill=20?= =?UTF-8?q?Lifecycles=20End=20to=20End=20(#14429)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test: cover agent skill lifecycles end to end * style: sort agent skill imports --- .../services/Endpoints/agents/addedConvo.js | 11 +- .../Endpoints/agents/addedConvo.spec.js | 89 +- .../services/Endpoints/agents/initialize.js | 2 +- .../Endpoints/agents/initialize.spec.js | 59 +- .../components/Chat/Input/SkillsCommand.tsx | 19 +- .../Input/__tests__/SkillsCommand.spec.tsx | 30 + .../SidePanel/Agents/Tools/SkillsDialog.tsx | 65 +- .../Tools/__tests__/SkillsDialog.spec.tsx | 249 ++++++ .../Skills/dialogs/CreateSkillDialog.tsx | 13 +- .../__tests__/CreateSkillDialog.spec.tsx | 111 +++ .../Skills/__tests__/mutations.spec.tsx | 142 +++ client/src/data-provider/Skills/mutations.ts | 28 +- .../__tests__/useSkillActiveState.test.ts | 25 + .../src/hooks/Skills/useSkillActiveState.ts | 30 +- e2e/config/librechat.e2e.yaml | 1 + e2e/setup/fake-model.js | 312 ++++++- e2e/specs/mock/agent-skills-added.spec.ts | 174 ++++ e2e/specs/mock/agent-skills.spec.ts | 810 ++++++++++++++++++ e2e/specs/mock/model-spec-skills.spec.ts | 106 ++- 19 files changed, 2160 insertions(+), 116 deletions(-) create mode 100644 client/src/components/SidePanel/Agents/Tools/__tests__/SkillsDialog.spec.tsx create mode 100644 client/src/components/Skills/dialogs/__tests__/CreateSkillDialog.spec.tsx create mode 100644 client/src/data-provider/Skills/__tests__/mutations.spec.tsx create mode 100644 client/src/hooks/Skills/__tests__/useSkillActiveState.test.ts create mode 100644 e2e/specs/mock/agent-skills-added.spec.ts create mode 100644 e2e/specs/mock/agent-skills.spec.ts diff --git a/api/server/services/Endpoints/agents/addedConvo.js b/api/server/services/Endpoints/agents/addedConvo.js index 09fad7a67f..837b1840f5 100644 --- a/api/server/services/Endpoints/agents/addedConvo.js +++ b/api/server/services/Endpoints/agents/addedConvo.js @@ -10,7 +10,7 @@ const { const { isEphemeralAgentId } = require('librechat-data-provider'); const { filterFilesByAgentAccess } = require('~/server/services/Files/permissions'); const { getMCPServerTools } = require('~/server/services/Config'); -const { canAuthorSkillFiles } = require('./skillDeps'); +const { getSkillDbMethods, canAuthorSkillFiles } = require('./skillDeps'); const db = require('~/models'); const loadAddedAgent = (params) => @@ -97,6 +97,7 @@ const processAddedConvo = async ({ }); try { + const skillDbMethods = getSkillDbMethods(); const addedAgent = await loadAddedAgent({ req, conversation: addedConvo, primaryAgent }); if (!addedAgent) { return { userMCPAuthMap }; @@ -138,7 +139,7 @@ const processAddedConvo = async ({ const resolvedSkillIds = await resolveModelSpecSkillIds({ names: selectedModelSpec.skills, accessibleSkillIds, - getSkillByName: db.getSkillByName, + getSkillByName: skillDbMethods.getSkillByName, }); addedAgent.skills_enabled = true; addedAgent.skills = resolvedSkillIds.map((id) => id.toString()); @@ -195,9 +196,9 @@ const processAddedConvo = async ({ getToolFilesByIds: db.getToolFilesByIds, getCodeGeneratedFiles: db.getCodeGeneratedFiles, filterFilesByAgentAccess, - listSkillsByAccess: db.listSkillsByAccess, - listAlwaysApplySkills: db.listAlwaysApplySkills, - getSkillByName: db.getSkillByName, + listSkillsByAccess: skillDbMethods.listSkillsByAccess, + listAlwaysApplySkills: skillDbMethods.listAlwaysApplySkills, + getSkillByName: skillDbMethods.getSkillByName, }, ); diff --git a/api/server/services/Endpoints/agents/addedConvo.spec.js b/api/server/services/Endpoints/agents/addedConvo.spec.js index cca3372bbd..89e8b0af0a 100644 --- a/api/server/services/Endpoints/agents/addedConvo.spec.js +++ b/api/server/services/Endpoints/agents/addedConvo.spec.js @@ -4,8 +4,12 @@ const mockLoadAddedAgent = jest.fn(); const mockResolveAgentScopedSkillIds = jest.fn(); const mockResolveModelSpecSkillIds = jest.fn(); const mockCanAuthorSkillFiles = jest.fn(); +const mockGetSkillDbMethods = jest.fn(); const mockGetAgent = jest.fn(); const mockGetMCPServerTools = jest.fn(); +const mockRegistryGetSkillByName = jest.fn(); +const mockRegistryListSkillsByAccess = jest.fn(); +const mockRegistryListAlwaysApplySkills = jest.fn(); jest.mock('@librechat/data-schemas', () => ({ logger: { @@ -35,6 +39,7 @@ jest.mock('~/server/services/Config', () => ({ jest.mock('./skillDeps', () => ({ canAuthorSkillFiles: (...args) => mockCanAuthorSkillFiles(...args), + getSkillDbMethods: () => mockGetSkillDbMethods(), })); jest.mock('~/models', () => ({ @@ -45,7 +50,6 @@ jest.mock('~/models', () => ({ })); const { processAddedConvo } = require('./addedConvo'); -const db = require('~/models'); const { Constants } = require('librechat-data-provider'); const makeReq = () => ({ user: { id: 'u1', role: 'USER' } }); @@ -72,6 +76,11 @@ describe('processAddedConvo', () => { ); mockResolveModelSpecSkillIds.mockResolvedValue([]); mockCanAuthorSkillFiles.mockReturnValue(false); + mockGetSkillDbMethods.mockReturnValue({ + getSkillByName: mockRegistryGetSkillByName, + listSkillsByAccess: mockRegistryListSkillsByAccess, + listAlwaysApplySkills: mockRegistryListAlwaysApplySkills, + }); }); const baseParams = (overrides = {}) => ({ @@ -125,6 +134,75 @@ describe('processAddedConvo', () => { ); }); + it('keeps deployment-aware skill metadata on a persisted added-agent config', async () => { + const deploymentSkillId = { toString: () => 'deployment-skill' }; + const agentConfigs = new Map(); + const initializedConfig = { + id: 'persisted-added-agent', + additional_instructions: 'deployment-skill', + manualSkillPrimes: [], + alwaysApplySkillPrimes: [ + { + _id: 'deployment-skill', + name: 'deployment-skill', + body: 'deployment skill body', + }, + ], + toolDefinitions: [{ name: 'skill' }], + userMCPAuthMap: undefined, + }; + + mockLoadAddedAgent.mockResolvedValue({ + id: 'persisted-added-agent', + provider: 'openai', + skills_enabled: true, + skills: ['deployment-skill'], + }); + mockResolveAgentScopedSkillIds.mockReturnValue([deploymentSkillId]); + mockInitializeAgent.mockResolvedValue(initializedConfig); + + await processAddedConvo( + baseParams({ + accessibleSkillIds: [deploymentSkillId], + editableSkillIds: [deploymentSkillId], + skillsCapabilityEnabled: true, + agentConfigs, + }), + ); + + expect(mockResolveModelSpecSkillIds).not.toHaveBeenCalled(); + expect(mockInitializeAgent).toHaveBeenCalledWith( + expect.objectContaining({ + agent: expect.objectContaining({ + id: 'persisted-added-agent', + skills_enabled: true, + skills: ['deployment-skill'], + }), + accessibleSkillIds: [deploymentSkillId], + }), + expect.objectContaining({ + listSkillsByAccess: mockRegistryListSkillsByAccess, + listAlwaysApplySkills: mockRegistryListAlwaysApplySkills, + getSkillByName: mockRegistryGetSkillByName, + }), + ); + expect(agentConfigs.get('persisted-added-agent')).toBe(initializedConfig); + expect(agentConfigs.get('persisted-added-agent')).toEqual( + expect.objectContaining({ + additional_instructions: 'deployment-skill', + manualSkillPrimes: [], + alwaysApplySkillPrimes: [ + expect.objectContaining({ + name: 'deployment-skill', + body: 'deployment skill body', + }), + ], + toolDefinitions: [expect.objectContaining({ name: 'skill' })], + }), + ); + expect(mockGetSkillDbMethods).toHaveBeenCalledTimes(1); + }); + it('resolves and forwards model-spec skill scope for added ephemeral agents', async () => { const accessibleSkillId = { toString: () => 'accessible-skill' }; const editableSkillId = { toString: () => 'editable-skill' }; @@ -181,7 +259,7 @@ describe('processAddedConvo', () => { expect(mockResolveModelSpecSkillIds).toHaveBeenCalledWith({ names: ['finance-analyst'], accessibleSkillIds: [accessibleSkillId], - getSkillByName: db.getSkillByName, + getSkillByName: mockRegistryGetSkillByName, }); expect(mockResolveAgentScopedSkillIds).toHaveBeenNthCalledWith(1, { agent: expect.objectContaining({ @@ -222,10 +300,11 @@ describe('processAddedConvo', () => { defaultActiveOnShare: true, }), expect.objectContaining({ - listSkillsByAccess: db.listSkillsByAccess, - listAlwaysApplySkills: db.listAlwaysApplySkills, - getSkillByName: db.getSkillByName, + listSkillsByAccess: mockRegistryListSkillsByAccess, + listAlwaysApplySkills: mockRegistryListAlwaysApplySkills, + getSkillByName: mockRegistryGetSkillByName, }), ); + expect(mockGetSkillDbMethods).toHaveBeenCalledTimes(1); }); }); diff --git a/api/server/services/Endpoints/agents/initialize.js b/api/server/services/Endpoints/agents/initialize.js index 50ab851aee..9cc8cba7d1 100644 --- a/api/server/services/Endpoints/agents/initialize.js +++ b/api/server/services/Endpoints/agents/initialize.js @@ -402,7 +402,7 @@ const initializeClient = async ({ req, res, signal, endpointOption, jobCreatedAt const resolvedSkillIds = await resolveModelSpecSkillIds({ names: selectedModelSpec.skills, accessibleSkillIds, - getSkillByName: db.getSkillByName, + getSkillByName: skillDbMethods.getSkillByName, }); primaryAgent.skills_enabled = true; primaryAgent.skills = resolvedSkillIds.map((id) => id.toString()); diff --git a/api/server/services/Endpoints/agents/initialize.spec.js b/api/server/services/Endpoints/agents/initialize.spec.js index c9f4cd4da9..dd3a4a86ca 100644 --- a/api/server/services/Endpoints/agents/initialize.spec.js +++ b/api/server/services/Endpoints/agents/initialize.spec.js @@ -79,7 +79,7 @@ jest.mock('~/cache', () => ({ })); const { initializeClient } = require('./initialize'); -const { getSkillToolDeps } = require('./skillDeps'); +const { getSkillDbMethods, getSkillToolDeps } = require('./skillDeps'); const { getModelsConfig } = require('~/server/controllers/ModelController'); const { logger } = require('@librechat/data-schemas'); const { User, AclEntry } = require('~/db/models'); @@ -360,6 +360,63 @@ describe('initializeClient — processAgent ACL gate', () => { canCreateSkillSpy.mockRestore(); } }); + + it('resolves model-spec skill names through deployment-aware skill methods', async () => { + const deploymentSkillId = new mongoose.Types.ObjectId(); + await AclEntry.create({ + principalType: PrincipalType.USER, + principalId: testUser._id, + principalModel: PrincipalModel.USER, + resourceType: ResourceType.SKILL, + resourceId: new mongoose.Types.ObjectId(), + permBits: PermissionBits.VIEW, + grantedBy: testUser._id, + }); + const endpointOption = makeEndpointOption(); + endpointOption.spec = 'spec-deployment-skill'; + endpointOption.agent = Promise.resolve({ + id: Constants.EPHEMERAL_AGENT_ID, + name: 'Ephemeral Primary', + provider: 'openai', + model: 'gpt-4', + tools: [], + }); + mockInitializeAgent.mockResolvedValue(makePrimaryConfig([])); + const req = makeReq(); + req.config.endpoints.agents = { capabilities: ['skills'] }; + req.config.modelSpecs = { + list: [{ name: 'spec-deployment-skill', skills: ['deployment-skill'] }], + }; + const getSkillByNameSpy = jest.spyOn(getSkillDbMethods(), 'getSkillByName').mockResolvedValue({ + _id: deploymentSkillId, + name: 'deployment-skill', + source: 'deployment', + }); + const canCreateSkillSpy = jest + .spyOn(getSkillToolDeps(), 'canCreateSkill') + .mockResolvedValue(false); + + try { + await initializeClient({ + req, + res: {}, + signal: new AbortController().signal, + endpointOption, + }); + + expect(getSkillByNameSpy).toHaveBeenCalledWith( + 'deployment-skill', + expect.any(Array), + expect.any(Object), + ); + const initializeParams = mockInitializeAgent.mock.calls[0][0]; + expect(initializeParams.agent.skills_enabled).toBe(true); + expect(initializeParams.agent.skills).toEqual([deploymentSkillId.toString()]); + } finally { + getSkillByNameSpy.mockRestore(); + canCreateSkillSpy.mockRestore(); + } + }); }); describe('initializeClient — subagent loading', () => { diff --git a/client/src/components/Chat/Input/SkillsCommand.tsx b/client/src/components/Chat/Input/SkillsCommand.tsx index 9105c780a3..f204acf714 100644 --- a/client/src/components/Chat/Input/SkillsCommand.tsx +++ b/client/src/components/Chat/Input/SkillsCommand.tsx @@ -7,10 +7,10 @@ import type { TSkillSummary } from 'librechat-data-provider'; import type { MentionOption } from '~/common'; import useInitPopoverInput from '~/hooks/Input/useInitPopoverInput'; import { useLocalize, useSkillActiveState } from '~/hooks'; -import { useAgentsMapContext } from '~/Providers'; import { useSkillsInfiniteQuery } from '~/data-provider'; -import { isEphemeralAgent } from '~/common'; +import { useAgentsMapContext } from '~/Providers'; import { ephemeralAgentByConvoId } from '~/store'; +import { isEphemeralAgent } from '~/common'; import { removeCharIfLast } from '~/utils'; import MentionItem from './MentionItem'; import store from '~/store'; @@ -50,7 +50,7 @@ export function filterSkillsForPopover( skills: TSkillSummary[], ctx: { agentSkillIds: string[] | null | undefined; - isActive: (skill: Pick) => boolean; + isActive: (skill: Pick) => boolean; }, ): TSkillSummary[] { const { agentSkillIds, isActive } = ctx; @@ -128,21 +128,10 @@ function SkillsCommandContent({ const { data, isLoading, isError, fetchNextPage, hasNextPage, isFetchingNextPage } = useSkillsInfiniteQuery({ limit: 50 }); - /* Sticky circuit breaker: once any page request fails, stop auto-fetching - for the lifetime of the popover so a transient API error does not turn - into an unbounded retry loop (isError can flip back to false on the - next attempt, which would otherwise re-arm the auto-fetch effect). */ - const paginationBlockedRef = useRef(false); - useEffect(() => { - if (isError) { - paginationBlockedRef.current = true; - } - }, [isError]); - /* Auto-fetch all pages so client-side search covers the full catalog, not just the first page. The skills API is server-side capped. */ useEffect(() => { - if (paginationBlockedRef.current || isError) { + if (isError) { return; } if (hasNextPage && !isFetchingNextPage) { diff --git a/client/src/components/Chat/Input/__tests__/SkillsCommand.spec.tsx b/client/src/components/Chat/Input/__tests__/SkillsCommand.spec.tsx index ecafb84586..a4951798dd 100644 --- a/client/src/components/Chat/Input/__tests__/SkillsCommand.spec.tsx +++ b/client/src/components/Chat/Input/__tests__/SkillsCommand.spec.tsx @@ -435,6 +435,36 @@ describe('SkillsCommand', () => { expect(screen.queryByRole('button', { name: /Brand Guidelines/i })).toBeNull(); expect(screen.getByRole('button', { name: /Style Guide/i })).toBeInTheDocument(); }); + + it('resumes catalog pagination after an external refetch clears an error', () => { + const fetchNextPage = jest.fn(); + mockUseSkillsInfiniteQuery.mockReturnValue({ + data: skillsResponse, + isLoading: false, + isError: true, + fetchNextPage, + hasNextPage: true, + isFetchingNextPage: false, + }); + + const textAreaRef = makeTextarea('$'); + const { rerender } = render( + , + ); + expect(fetchNextPage).not.toHaveBeenCalled(); + + mockUseSkillsInfiniteQuery.mockReturnValue({ + data: skillsResponse, + isLoading: false, + isError: false, + fetchNextPage, + hasNextPage: true, + isFetchingNextPage: false, + }); + rerender(); + + expect(fetchNextPage).toHaveBeenCalledTimes(1); + }); }); describe('filterSkillsForPopover', () => { diff --git a/client/src/components/SidePanel/Agents/Tools/SkillsDialog.tsx b/client/src/components/SidePanel/Agents/Tools/SkillsDialog.tsx index 08b934f580..29833e515f 100644 --- a/client/src/components/SidePanel/Agents/Tools/SkillsDialog.tsx +++ b/client/src/components/SidePanel/Agents/Tools/SkillsDialog.tsx @@ -1,4 +1,4 @@ -import { useMemo, useState, useCallback } from 'react'; +import { useMemo, useState, useCallback, useEffect } from 'react'; import { Plus, Search } from 'lucide-react'; import { useFormContext, useWatch } from 'react-hook-form'; import { PermissionTypes, Permissions } from 'librechat-data-provider'; @@ -11,7 +11,7 @@ import { OGDialogContent, OGDialogDescription, } from '@librechat/client'; -import type { TSkill } from 'librechat-data-provider'; +import type { TSkill, TSkillSummary } from 'librechat-data-provider'; import type { TranslationKeys } from '~/hooks/useLocalize'; import type { CategoryOption } from './CategoryFilter'; import type { AgentItem } from './items/types'; @@ -19,8 +19,8 @@ import type { AgentForm } from '~/common'; import { useLocalize, useHasAccess, useAuthContext, useToolFavorites } from '~/hooks'; import { CreateSkillDialog } from '~/components/Skills/dialogs'; import { skillsEnabledTransition } from './items/mutations'; +import { useSkillsInfiniteQuery } from '~/data-provider'; import MarketplaceCatalog from './MarketplaceCatalog'; -import { useListSkillsQuery } from '~/data-provider'; import { CategoryIcon } from '~/components/Prompts'; import { buildSkillItems } from './items/catalog'; import ItemDialog from './ItemDialog/ItemDialog'; @@ -55,12 +55,30 @@ export default function SkillsDialog({ open, onOpenChange, agentId }: SkillsDial permissionType: PermissionTypes.SKILLS, permission: Permissions.CREATE, }); - const { data: skillsData, isLoading: isLoadingSkills } = useListSkillsQuery( - { limit: 100 }, - { enabled: hasSkillsAccess }, - ); + const { + data: skillsData, + isLoading: isLoadingSkills, + isError: isSkillsError, + fetchNextPage, + refetch: refetchSkills, + hasNextPage, + isFetchingNextPage, + } = useSkillsInfiniteQuery({ limit: 100 }, { enabled: hasSkillsAccess }); const { favoriteKeys, toggle: toggleFavorite } = useToolFavorites(); + useEffect(() => { + if (isSkillsError) { + return; + } + if (hasNextPage && !isFetchingNextPage) { + fetchNextPage(); + } + }, [hasNextPage, isFetchingNextPage, isSkillsError, fetchNextPage]); + + const handleRetrySkills = useCallback(() => { + void refetchSkills(); + }, [refetchSkills]); + const skillsField = useWatch({ control, name: 'skills' }); const selectedIds = useMemo( () => new Set(((skillsField ?? []) as string[]).map((id) => itemKey({ kind: 'skill', id }))), @@ -73,10 +91,22 @@ export default function SkillsDialog({ open, onOpenChange, agentId }: SkillsDial const [createOpen, setCreateOpen] = useState(false); const [detailItem, setDetailItem] = useState(null); - const catalog = useMemo( - () => buildSkillItems(skillsData?.skills ?? [], user?.id), - [skillsData, user?.id], - ); + const skills = useMemo(() => { + const allSkills: TSkillSummary[] = []; + const seen = new Set(); + for (const page of skillsData?.pages ?? []) { + for (const skill of page.skills) { + if (seen.has(skill._id)) { + continue; + } + seen.add(skill._id); + allSkills.push(skill); + } + } + return allSkills; + }, [skillsData?.pages]); + + const catalog = useMemo(() => buildSkillItems(skills, user?.id), [skills, user?.id]); const categoryOptions = useMemo(() => { const seen = new Set(); @@ -210,13 +240,24 @@ export default function SkillsDialog({ open, onOpenChange, agentId }: SkillsDial
+ {isSkillsError && ( +
+ {localize('com_ui_skills_load_error')} + +
+ )} }; + isLoading: boolean; + isError: boolean; + fetchNextPage: jest.Mock; + refetch: jest.Mock; + hasNextPage: boolean; + isFetchingNextPage: boolean; +}; + +jest.mock('react-hook-form', () => ({ + useFormContext: () => ({ + control: {}, + getValues: (name: string) => (name === 'skills' ? [] : false), + setValue: jest.fn(), + }), + useWatch: () => [], +})); + +jest.mock('~/hooks', () => ({ + useLocalize: () => (key: string) => { + const values: Record = { + com_ui_skills: 'Skills', + com_ui_search_skills: 'Search skills...', + com_ui_skills_dialog_description: 'Browse and add skills to your agent.', + com_ui_skills_load_error: 'Failed to load skills', + com_ui_retry: 'Retry', + }; + return values[key] ?? key; + }, + useHasAccess: () => true, + useAuthContext: () => ({ user: { id: 'user-1' } }), + useToolFavorites: () => ({ + favoriteKeys: new Set(), + toggle: jest.fn(), + }), +})); + +jest.mock('~/data-provider', () => ({ + useListSkillsQuery: (...args: unknown[]) => mockUseListSkillsQuery(...args), + useSkillsInfiniteQuery: (...args: unknown[]) => mockUseSkillsInfiniteQuery(...args), +})); + +jest.mock('~/components/Prompts', () => ({ + CategoryIcon: () =>