diff --git a/client/src/components/Chat/Input/ChatForm.tsx b/client/src/components/Chat/Input/ChatForm.tsx index 50f2076dd0..3f62b716b4 100644 --- a/client/src/components/Chat/Input/ChatForm.tsx +++ b/client/src/components/Chat/Input/ChatForm.tsx @@ -255,7 +255,12 @@ const ChatForm = memo(function ChatForm({ textAreaRef={textAreaRef} /> - +
) => boolean; + }, +): TSkillSummary[] { + const { agentSkillIds, isActive } = ctx; + if (agentSkillIds != null && agentSkillIds.length === 0) { + return []; + } + const agentSet = + agentSkillIds != null && agentSkillIds.length > 0 ? new Set(agentSkillIds) : null; + const result: TSkillSummary[] = []; + for (const skill of skills) { + if (agentSet && !agentSet.has(skill._id)) { + continue; + } + if (!isActive(skill)) { + continue; + } + if (!isUserInvocable(skill)) { + continue; + } + result.push(skill); + } + return result; +} + function SkillsCommandContent({ index, textAreaRef, conversationId, + agentId, }: { index: number; textAreaRef: React.MutableRefObject; conversationId: string; + agentId?: string | null; }) { const localize = useLocalize(); const setShowSkillsPopover = useSetRecoilState(store.showSkillsPopoverFamily(index)); @@ -48,6 +97,36 @@ function SkillsCommandContent({ store.pendingManualSkillsByConvoId(conversationId), ); + const agentsMap = useAgentsMapContext(); + const { isActive } = useSkillActiveState(); + + /* Resolve the per-agent skill scope. Mirrors backend `scopeSkillIds` for + the happy path: no `skills` field → no scope, `[]` → opt-out, non-empty + → intersection. Ephemeral agent ids (null/undefined/placeholder strings + that don't begin with `agent_`) are unscoped — they correspond to + conversations without a persisted agent and are intentionally absent + from the agents map. While the map is still hydrating we pass through + (undefined → full catalog): the backend enforces scope at turn time, so + there's no security benefit to flashing an empty popover, and the map + typically lands well before the first open. Once the map is authoritative + but the agent isn't in it (deleted, or VIEW revoked mid-session), we fail + closed — scope is unresolvable and the full catalog would be misleading. + `agentId` is threaded in as a prop so this component stays memoizable + and skips re-renders on unrelated conversation-shape changes. */ + const agentSkillIds = useMemo(() => { + if (!agentId || isEphemeralAgent(agentId)) { + return undefined; + } + if (!agentsMap) { + return undefined; + } + const agent = agentsMap[agentId]; + if (!agent) { + return []; + } + return agent.skills; + }, [agentId, agentsMap]); + const { data, isLoading, isError, fetchNextPage, hasNextPage, isFetchingNextPage } = useSkillsInfiniteQuery({ limit: 50 }); @@ -77,21 +156,25 @@ function SkillsCommandContent({ if (!data?.pages) { return []; } - return data.pages.reduce((acc, page) => { + const allSkills: TSkillSummary[] = []; + for (const page of data.pages) { for (const skill of page.skills) { - if (isUserInvocable(skill)) { - acc.push({ - label: skill.displayTitle ?? skill.name, - value: skill.name, - description: skill.description, - type: 'skill', - icon: skillIcon, - }); - } + allSkills.push(skill); } - return acc; - }, []); - }, [data?.pages]); + } + const filtered = filterSkillsForPopover(allSkills, { agentSkillIds, isActive }); + const options: MentionOption[] = []; + for (const skill of filtered) { + options.push({ + label: skill.displayTitle ?? skill.name, + value: skill.name, + description: skill.description, + type: 'skill', + icon: skillIcon, + }); + } + return options; + }, [data?.pages, agentSkillIds, isActive]); const [activeIndex, setActiveIndex] = useState(0); const timeoutRef = useRef(null); @@ -314,17 +397,24 @@ const SkillsCommand = memo(function SkillsCommand({ index, textAreaRef, conversationId, + agentId, }: { index: number; textAreaRef: React.MutableRefObject; conversationId: string; + agentId?: string | null; }) { const show = useRecoilValue(store.showSkillsPopoverFamily(index)); if (!show) { return null; } return ( - + ); }); diff --git a/client/src/components/Chat/Input/__tests__/SkillsCommand.spec.tsx b/client/src/components/Chat/Input/__tests__/SkillsCommand.spec.tsx index 27d665a674..3366779cb8 100644 --- a/client/src/components/Chat/Input/__tests__/SkillsCommand.spec.tsx +++ b/client/src/components/Chat/Input/__tests__/SkillsCommand.spec.tsx @@ -4,11 +4,18 @@ * component must (a) push the skill name onto the per-conversation * `pendingManualSkillsByConvoId` atom, (b) flip `ephemeralAgent.skills` * to true, and (c) insert `$skill-name ` into the textarea. + * + * Also covers the Phase 2 filter composition: per-agent skill scope + * intersects with the ACL catalog, and per-user active-state toggles + * hide inactive entries from the popover (backend still enforces both + * at runtime regardless). */ import React from 'react'; import { act } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { render, screen } from '@testing-library/react'; +import { InvocationMode } from 'librechat-data-provider'; +import type { TSkillSummary } from 'librechat-data-provider'; const CONVO_ID = 'convo-1'; @@ -58,8 +65,20 @@ jest.mock('~/data-provider', () => ({ useSkillsInfiniteQuery: () => mockUseSkillsInfiniteQuery(), })); +/* Phase 2: the popover reads agent skill config via useAgentsMapContext + and the agent id is threaded down as a prop from ChatForm (so the + component stays memoizable across unrelated convo-shape changes). The + test harness swaps the agents map in so cases can configure ephemeral + vs. agent-scoped behavior without standing up a real provider. */ +const mockUseAgentsMapContext = jest.fn(); +jest.mock('~/Providers', () => ({ + useAgentsMapContext: () => mockUseAgentsMapContext(), +})); + +const mockIsActive = jest.fn(); jest.mock('~/hooks', () => ({ useLocalize: () => (key: string) => key, + useSkillActiveState: () => ({ isActive: mockIsActive }), })); jest.mock('@librechat/client', () => { @@ -95,7 +114,7 @@ jest.mock('react-virtualized', () => ({ }, })); -import SkillsCommand from '../SkillsCommand'; +import SkillsCommand, { filterSkillsForPopover } from '../SkillsCommand'; const makeTextarea = (initial = '$') => { const textarea = document.createElement('textarea'); @@ -104,23 +123,37 @@ const makeTextarea = (initial = '$') => { return { current: textarea } as React.MutableRefObject; }; +const makeSkill = (overrides: Partial): TSkillSummary => ({ + _id: '1', + name: 'brand-guidelines', + displayTitle: 'Brand Guidelines', + description: 'Apply brand styling', + author: 'u', + authorName: 'U', + version: 1, + source: 'inline', + fileCount: 0, + createdAt: '', + updatedAt: '', + ...overrides, +}); + const skillsResponse = { + pages: [ + { + skills: [makeSkill({})], + has_more: false, + after: null, + }, + ], +}; + +const twoSkillsResponse = { pages: [ { skills: [ - { - _id: '1', - name: 'brand-guidelines', - displayTitle: 'Brand Guidelines', - description: 'Apply brand styling', - author: 'u', - authorName: 'U', - version: 1, - source: 'inline', - fileCount: 0, - createdAt: '', - updatedAt: '', - }, + makeSkill({ _id: '1', name: 'brand-guidelines', displayTitle: 'Brand Guidelines' }), + makeSkill({ _id: '2', name: 'style-guide', displayTitle: 'Style Guide' }), ], has_more: false, after: null, @@ -140,6 +173,11 @@ beforeEach(() => { hasNextPage: false, isFetchingNextPage: false, }); + /* Defaults: empty agents map and every skill active. Individual tests + override these and pass `agentId` as a prop to exercise the Phase 2 + filter composition. */ + mockUseAgentsMapContext.mockReturnValue({}); + mockIsActive.mockReturnValue(true); }); describe('SkillsCommand', () => { @@ -184,4 +222,244 @@ describe('SkillsCommand', () => { /* Popover dismisses on selection. */ expect(mockSetShowSkillsPopover).toHaveBeenCalledWith(false); }); + + it('narrows the list to the agent-configured scope when agent.skills is set', async () => { + mockUseSkillsInfiniteQuery.mockReturnValue({ + data: twoSkillsResponse, + isLoading: false, + isError: false, + fetchNextPage: jest.fn(), + hasNextPage: false, + isFetchingNextPage: false, + }); + mockUseAgentsMapContext.mockReturnValue({ + agent_1: { id: 'agent_1', skills: ['2'] }, + }); + + const textAreaRef = makeTextarea('$'); + render( + , + ); + + /* Only the skill whose _id is in agent.skills should appear. */ + expect(screen.queryByRole('button', { name: /Brand Guidelines/i })).toBeNull(); + expect(await screen.findByRole('button', { name: /Style Guide/i })).toBeInTheDocument(); + }); + + it('shows nothing when the agent has an empty skills array (explicit opt-out)', () => { + mockUseSkillsInfiniteQuery.mockReturnValue({ + data: twoSkillsResponse, + isLoading: false, + isError: false, + fetchNextPage: jest.fn(), + hasNextPage: false, + isFetchingNextPage: false, + }); + mockUseAgentsMapContext.mockReturnValue({ + agent_1: { id: 'agent_1', skills: [] }, + }); + + const textAreaRef = makeTextarea('$'); + render( + , + ); + + expect(screen.queryByRole('button', { name: /Brand Guidelines/i })).toBeNull(); + expect(screen.queryByRole('button', { name: /Style Guide/i })).toBeNull(); + }); + + it('shows the full ACL catalog when the agent has no skills field configured', async () => { + mockUseSkillsInfiniteQuery.mockReturnValue({ + data: twoSkillsResponse, + isLoading: false, + isError: false, + fetchNextPage: jest.fn(), + hasNextPage: false, + isFetchingNextPage: false, + }); + mockUseAgentsMapContext.mockReturnValue({ + agent_1: { id: 'agent_1' }, + }); + + const textAreaRef = makeTextarea('$'); + render( + , + ); + + expect(await screen.findByRole('button', { name: /Brand Guidelines/i })).toBeInTheDocument(); + expect(await screen.findByRole('button', { name: /Style Guide/i })).toBeInTheDocument(); + }); + + it('treats an ephemeral agent id as unscoped and shows the full ACL catalog', async () => { + mockUseSkillsInfiniteQuery.mockReturnValue({ + data: twoSkillsResponse, + isLoading: false, + isError: false, + fetchNextPage: jest.fn(), + hasNextPage: false, + isFetchingNextPage: false, + }); + mockUseAgentsMapContext.mockReturnValue({}); + + const textAreaRef = makeTextarea('$'); + render( + , + ); + + /* `ephemeral` doesn't start with `agent_`, so it's an ephemeral id; + scope filter should be skipped and the full catalog displayed. */ + expect(await screen.findByRole('button', { name: /Brand Guidelines/i })).toBeInTheDocument(); + expect(await screen.findByRole('button', { name: /Style Guide/i })).toBeInTheDocument(); + }); + + it('shows the full ACL catalog while the agents map is hydrating (backend still gates the turn)', async () => { + mockUseSkillsInfiniteQuery.mockReturnValue({ + data: twoSkillsResponse, + isLoading: false, + isError: false, + fetchNextPage: jest.fn(), + hasNextPage: false, + isFetchingNextPage: false, + }); + mockUseAgentsMapContext.mockReturnValue(undefined); + + const textAreaRef = makeTextarea('$'); + render( + , + ); + + /* Hydration race: map not yet loaded. Pass through to full catalog — + the backend scopes at turn time and blanking the popover during + sub-second hydration is worse UX for no security benefit. */ + expect(await screen.findByRole('button', { name: /Brand Guidelines/i })).toBeInTheDocument(); + expect(await screen.findByRole('button', { name: /Style Guide/i })).toBeInTheDocument(); + }); + + it('fails closed when the agent id is set but missing from the agents map', () => { + mockUseSkillsInfiniteQuery.mockReturnValue({ + data: twoSkillsResponse, + isLoading: false, + isError: false, + fetchNextPage: jest.fn(), + hasNextPage: false, + isFetchingNextPage: false, + }); + mockUseAgentsMapContext.mockReturnValue({}); + + const textAreaRef = makeTextarea('$'); + render( + , + ); + + expect(screen.queryByRole('button', { name: /Brand Guidelines/i })).toBeNull(); + expect(screen.queryByRole('button', { name: /Style Guide/i })).toBeNull(); + }); + + it('hides inactive skills from the popover', () => { + mockUseSkillsInfiniteQuery.mockReturnValue({ + data: twoSkillsResponse, + isLoading: false, + isError: false, + fetchNextPage: jest.fn(), + hasNextPage: false, + isFetchingNextPage: false, + }); + mockIsActive.mockImplementation((skill: { _id: string }) => skill._id !== '1'); + + const textAreaRef = makeTextarea('$'); + render(); + + expect(screen.queryByRole('button', { name: /Brand Guidelines/i })).toBeNull(); + expect(screen.getByRole('button', { name: /Style Guide/i })).toBeInTheDocument(); + }); +}); + +describe('filterSkillsForPopover', () => { + const active = () => true; + const inactive = () => false; + const s1 = makeSkill({ _id: '1', name: 'a' }); + const s2 = makeSkill({ _id: '2', name: 'b' }); + const s3 = makeSkill({ _id: '3', name: 'c', invocationMode: InvocationMode.auto }); + + it('passes everything through when agentSkillIds is undefined', () => { + const out = filterSkillsForPopover([s1, s2], { agentSkillIds: undefined, isActive: active }); + expect(out.map((s) => s._id)).toEqual(['1', '2']); + }); + + it('passes everything through when agentSkillIds is null', () => { + const out = filterSkillsForPopover([s1, s2], { agentSkillIds: null, isActive: active }); + expect(out.map((s) => s._id)).toEqual(['1', '2']); + }); + + it('returns empty when agentSkillIds is []', () => { + const out = filterSkillsForPopover([s1, s2], { agentSkillIds: [], isActive: active }); + expect(out).toEqual([]); + }); + + it('intersects with a non-empty agentSkillIds', () => { + const out = filterSkillsForPopover([s1, s2], { agentSkillIds: ['2'], isActive: active }); + expect(out.map((s) => s._id)).toEqual(['2']); + }); + + it('excludes inactive skills', () => { + const isActive = (skill: { _id: string }) => skill._id !== '1'; + const out = filterSkillsForPopover([s1, s2], { agentSkillIds: null, isActive }); + expect(out.map((s) => s._id)).toEqual(['2']); + }); + + it('excludes auto-only skills via isUserInvocable', () => { + const out = filterSkillsForPopover([s1, s3], { agentSkillIds: null, isActive: active }); + expect(out.map((s) => s._id)).toEqual(['1']); + }); + + it('still empty when agent scope is [] even if everything is active and invocable', () => { + const out = filterSkillsForPopover([s1, s2, s3], { agentSkillIds: [], isActive: active }); + expect(out).toEqual([]); + }); + + it('layers all three filters (agent scope ∩ active ∩ invocable)', () => { + const isActive = (skill: { _id: string }) => skill._id !== '2'; + const out = filterSkillsForPopover([s1, s2, s3], { + agentSkillIds: ['1', '2', '3'], + isActive, + }); + /* s1 passes (active, manual-by-default, scoped), s2 drops (inactive), + s3 drops (auto-only invocation mode). */ + expect(out.map((s) => s._id)).toEqual(['1']); + }); + + it('drops everything when isActive returns false for all inputs', () => { + const out = filterSkillsForPopover([s1, s2], { agentSkillIds: null, isActive: inactive }); + expect(out).toEqual([]); + }); }); diff --git a/packages/data-schemas/src/methods/agent.spec.ts b/packages/data-schemas/src/methods/agent.spec.ts index 0c7042cde1..2d5a6666bb 100644 --- a/packages/data-schemas/src/methods/agent.spec.ts +++ b/packages/data-schemas/src/methods/agent.spec.ts @@ -3334,6 +3334,33 @@ describe('Support Contact Field', () => { expect(result.data[0].name).toBe('Agent A1'); }); + test('should include the skills field in the list projection', async () => { + // Frontend popover scoping relies on `agent.skills` being present in + // list results so it can narrow the `$` catalog without refetching the + // full agent document. Locks in that projection contract. + const targetSkillIds = [ + new mongoose.Types.ObjectId().toString(), + new mongoose.Types.ObjectId().toString(), + ]; + const scopedAgent = await createAgent({ + id: `agent_${uuidv4().slice(0, 12)}`, + name: 'Scoped Agent', + description: 'Agent with configured skill scope', + provider: 'openai', + model: 'gpt-4', + author: userA, + skills: targetSkillIds, + }); + + const result = await getListAgentsByAccess({ + accessibleIds: [scopedAgent._id] as mongoose.Types.ObjectId[], + otherParams: {}, + }); + + expect(result.data).toHaveLength(1); + expect(result.data[0].skills).toEqual(targetSkillIds); + }); + test('should return multiple accessible agents when provided', async () => { // Give User B access to two of User A's agents const accessibleIds = [agentA1._id, agentA3._id] as mongoose.Types.ObjectId[]; diff --git a/packages/data-schemas/src/methods/agent.ts b/packages/data-schemas/src/methods/agent.ts index 7cdfd68f52..f09bf8460b 100644 --- a/packages/data-schemas/src/methods/agent.ts +++ b/packages/data-schemas/src/methods/agent.ts @@ -711,6 +711,9 @@ export function createAgentMethods(mongoose: typeof import('mongoose'), deps: Ag category: 1, support_contact: 1, is_promoted: 1, + /* Needed so the client can scope the `$` skill popover to each agent's + configured catalog without refetching the full agent document. */ + skills: 1, }).sort({ updatedAt: -1, _id: 1 }); if (isPaginated && normalizedLimit) {