From 596f806f60264c816b5a1bc358593aeb6d004741 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 25 Apr 2026 00:34:12 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20fix:=20Strict=20Opt-In?= =?UTF-8?q?=20Skills=20Activation=20per=20Agent=20(#12823)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🛡️ fix: Strict opt-in skills activation per agent Skills were activating on every agent run that had the capability + RBAC enabled, regardless of whether the user (ephemeral) or author (persisted) had opted in. `scopeSkillIds(undefined)` fell through to "full accessible catalog" whenever `agent.skills` was unset, which is the default state for any agent created before skills existed and for every ephemeral agent. Activation now requires an explicit signal: - Ephemeral agent → per-conversation skills badge toggle. - Persisted agent → new `skills_enabled` master switch on the agent doc, surfaced as a toggle in the Agent Builder skills section. Enabled + empty/undefined allowlist = full accessible catalog; enabled + non-empty allowlist = narrow to those ids; disabled (or undefined) = no skills available, even if an allowlist is set. Centralised the predicate in `resolveAgentScopedSkillIds` so the primary-agent path, handoff/discovery, the subagent loop, and both OpenAI controllers all share one source of truth. Frontend `$` popover scope mirrors the same logic so the UI never offers skills the backend would refuse to activate. * test: mock resolveAgentScopedSkillIds in agent controller specs * refactor: address review findings on skills opt-in PR - AgentConfig: associate skills label with toggle via htmlFor for click/keyboard affordance; simplify Switch handler to Boolean(value). - skills: mark scopeSkillIds as @internal so runtime callers continue to route through resolveAgentScopedSkillIds and inherit the activation predicate (ephemeral toggle, persisted skills_enabled). * fix(agents): include skills_enabled in agent list projection Without this field, agents loaded via the list endpoint hydrate into the client agentsMap with skills_enabled === undefined, causing the `$` skill popover to hide every skill on a fresh page load even when the agent was saved with skills_enabled: true. * fix(skills): fail closed for persisted agents during agentsMap hydration Returning undefined while the agents map loads let the popover render the full catalog for a persisted agent before we could read its skills_enabled flag, so the user could pick a skill the backend would then refuse for the turn. Match the strict opt-in contract by returning [] until the map is authoritative. * refactor(skills): extract skillsHintKey for readability Replaces the nested ternary in the skills section JSX with a pre-computed constant so the activation -> hint key mapping reads top-down. * refactor(skills): unflatten skillsHintKey to remove nested ternary --- .../agents/__tests__/openai.spec.js | 3 + .../agents/__tests__/responses.unit.spec.js | 3 + api/server/controllers/agents/openai.js | 16 +- api/server/controllers/agents/responses.js | 10 +- .../services/Endpoints/agents/initialize.js | 38 ++-- client/src/common/agents-types.ts | 1 + .../components/Chat/Input/SkillsCommand.tsx | 32 ++-- .../Input/__tests__/SkillsCommand.spec.tsx | 55 ++++-- .../SidePanel/Agents/AgentConfig.tsx | 46 ++++- .../SidePanel/Agents/AgentPanel.tsx | 2 + .../SidePanel/Agents/AgentSelect.tsx | 5 + client/src/locales/en/translation.json | 4 + .../api/src/agents/__tests__/skills.test.ts | 171 ++++++++++++++++++ packages/api/src/agents/skills.ts | 52 ++++++ packages/api/src/agents/validation.ts | 1 + packages/data-provider/src/schemas.ts | 9 +- .../data-provider/src/types/assistants.ts | 7 +- packages/data-schemas/src/methods/agent.ts | 5 +- packages/data-schemas/src/schema/agent.ts | 4 + packages/data-schemas/src/types/agent.ts | 1 + 20 files changed, 404 insertions(+), 61 deletions(-) diff --git a/api/server/controllers/agents/__tests__/openai.spec.js b/api/server/controllers/agents/__tests__/openai.spec.js index 7fb41697e9..7638fc2e35 100644 --- a/api/server/controllers/agents/__tests__/openai.spec.js +++ b/api/server/controllers/agents/__tests__/openai.spec.js @@ -41,6 +41,9 @@ jest.mock('@librechat/api', () => ({ createChunk: jest.fn().mockReturnValue({}), buildToolSet: jest.fn().mockReturnValue(new Set()), scopeSkillIds: jest.fn().mockImplementation((ids) => ids), + resolveAgentScopedSkillIds: jest + .fn() + .mockImplementation(({ accessibleSkillIds }) => accessibleSkillIds), loadSkillStates: jest.fn().mockResolvedValue({ skillStates: {}, defaultActiveOnShare: false }), sendFinalChunk: jest.fn(), createSafeUser: jest.fn().mockReturnValue({ id: 'user-123' }), diff --git a/api/server/controllers/agents/__tests__/responses.unit.spec.js b/api/server/controllers/agents/__tests__/responses.unit.spec.js index 9b760b7076..e5569fbbf5 100644 --- a/api/server/controllers/agents/__tests__/responses.unit.spec.js +++ b/api/server/controllers/agents/__tests__/responses.unit.spec.js @@ -42,6 +42,9 @@ jest.mock('@librechat/api', () => ({ }), buildToolSet: jest.fn().mockReturnValue(new Set()), scopeSkillIds: jest.fn().mockImplementation((ids) => ids), + resolveAgentScopedSkillIds: jest + .fn() + .mockImplementation(({ accessibleSkillIds }) => accessibleSkillIds), loadSkillStates: jest.fn().mockResolvedValue({ skillStates: {}, defaultActiveOnShare: false }), createSafeUser: jest.fn().mockReturnValue({ id: 'user-123' }), initializeAgent: jest.fn().mockResolvedValue({ diff --git a/api/server/controllers/agents/openai.js b/api/server/controllers/agents/openai.js index 9d98600ef7..7d2395b09f 100644 --- a/api/server/controllers/agents/openai.js +++ b/api/server/controllers/agents/openai.js @@ -13,26 +13,26 @@ const { createRun, createChunk, buildToolSet, - scopeSkillIds, loadSkillStates, sendFinalChunk, createSafeUser, validateRequest, initializeAgent, getBalanceConfig, + injectSkillPrimes, extractManualSkills, createErrorResponse, recordCollectedUsage, getTransactionsConfig, resolveRecursionLimit, + discoverConnectedAgents, + getRemoteAgentPermissions, createToolExecuteHandler, buildNonStreamingResponse, createOpenAIStreamTracker, + resolveAgentScopedSkillIds, createOpenAIContentAggregator, - injectSkillPrimes, isChatCompletionValidationFailure, - discoverConnectedAgents, - getRemoteAgentPermissions, } = require('@librechat/api'); const { buildSummarizationHandlers, @@ -283,10 +283,12 @@ const OpenAIChatCompletionController = async (req, res) => { endpointOption, allowedProviders, isInitialAgent: true, - accessibleSkillIds: scopeSkillIds( + accessibleSkillIds: resolveAgentScopedSkillIds({ + agent, accessibleSkillIds, - ephemeralSkillsToggle ? undefined : agent.skills, - ), + skillsCapabilityEnabled, + ephemeralSkillsToggle, + }), codeEnvAvailable: enabledCapabilities.has(AgentCapabilities.execute_code), skillStates, defaultActiveOnShare, diff --git a/api/server/controllers/agents/responses.js b/api/server/controllers/agents/responses.js index 08ca17bde5..b2805fc19f 100644 --- a/api/server/controllers/agents/responses.js +++ b/api/server/controllers/agents/responses.js @@ -12,8 +12,8 @@ const { const { createRun, buildToolSet, - scopeSkillIds, loadSkillStates, + resolveAgentScopedSkillIds, createSafeUser, initializeAgent, getBalanceConfig, @@ -413,10 +413,12 @@ const createResponse = async (req, res) => { endpointOption, allowedProviders, isInitialAgent: true, - accessibleSkillIds: scopeSkillIds( + accessibleSkillIds: resolveAgentScopedSkillIds({ + agent, accessibleSkillIds, - ephemeralSkillsToggle ? undefined : agent.skills, - ), + skillsCapabilityEnabled, + ephemeralSkillsToggle, + }), codeEnvAvailable: enabledCapabilities.has(AgentCapabilities.execute_code), skillStates, defaultActiveOnShare, diff --git a/api/server/services/Endpoints/agents/initialize.js b/api/server/services/Endpoints/agents/initialize.js index 367a8adb45..939a0a7ff6 100644 --- a/api/server/services/Endpoints/agents/initialize.js +++ b/api/server/services/Endpoints/agents/initialize.js @@ -1,7 +1,6 @@ const { logger } = require('@librechat/data-schemas'); const { createContentAggregator } = require('@librechat/agents'); const { - scopeSkillIds, loadSkillStates, initializeAgent, primeInvokedSkills, @@ -10,6 +9,7 @@ const { GenerationJobManager, getCustomEndpointConfig, discoverConnectedAgents, + resolveAgentScopedSkillIds, } = require('@librechat/api'); const { ResourceType, @@ -115,9 +115,12 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { const toolEndCallback = createToolEndCallback({ req, res, artifactPromises, streamId }); /** Query accessible skill IDs once per run (shared across all agents). - * Skills activate when the admin capability is enabled AND either: - * - the per-conversation toggle is on (ephemeral), OR - * - the agent has stored skills (scoped by scopeSkillIds later). */ + * Skills activate under strict opt-in semantics — see + * `resolveAgentScopedSkillIds` for the per-agent activation predicate: + * - Ephemeral agent → per-conversation skills badge toggle (full catalog). + * - Persisted agent → `agent.skills_enabled === true`. Optional + * `agent.skills` allowlist narrows the catalog; empty/undefined + * allowlist with the toggle on = full accessible catalog. */ const enabledCapabilities = new Set(appConfig?.endpoints?.[EModelEndpoint.agents]?.capabilities); const skillsCapabilityEnabled = enabledCapabilities.has(AgentCapabilities.skills); const codeEnvAvailable = enabledCapabilities.has(AgentCapabilities.execute_code); @@ -259,6 +262,13 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { */ const manualSkills = extractManualSkills(req.body); + const primaryScopedSkillIds = resolveAgentScopedSkillIds({ + agent: primaryAgent, + accessibleSkillIds, + skillsCapabilityEnabled, + ephemeralSkillsToggle, + }); + const primaryConfig = await initializeAgent( { req, @@ -271,10 +281,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { endpointOption, allowedProviders, isInitialAgent: true, - accessibleSkillIds: scopeSkillIds( - accessibleSkillIds, - ephemeralSkillsToggle ? undefined : primaryAgent.skills, - ), + accessibleSkillIds: primaryScopedSkillIds, codeEnvAvailable, skillStates, defaultActiveOnShare, @@ -340,7 +347,12 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { conversationId, parentMessageId, computeAccessibleSkillIds: (agent) => - scopeSkillIds(accessibleSkillIds, ephemeralSkillsToggle ? undefined : agent.skills), + resolveAgentScopedSkillIds({ + agent, + accessibleSkillIds, + skillsCapabilityEnabled, + ephemeralSkillsToggle, + }), skillStates, defaultActiveOnShare, codeEnvAvailable, @@ -544,10 +556,12 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { parentMessageId, endpointOption: { ...endpointOption, endpoint: EModelEndpoint.agents }, allowedProviders, - accessibleSkillIds: scopeSkillIds( + accessibleSkillIds: resolveAgentScopedSkillIds({ + agent, accessibleSkillIds, - ephemeralSkillsToggle ? undefined : agent.skills, - ), + skillsCapabilityEnabled, + ephemeralSkillsToggle, + }), skillStates, defaultActiveOnShare, }, diff --git a/client/src/common/agents-types.ts b/client/src/common/agents-types.ts index 8a018e8fcb..7313812ec5 100644 --- a/client/src/common/agents-types.ts +++ b/client/src/common/agents-types.ts @@ -40,6 +40,7 @@ export type AgentForm = { /** Per-tool configuration options (deferred loading, allowed callers, etc.) */ tool_options?: AgentToolOptions; skills?: string[]; + skills_enabled?: boolean; provider?: AgentProvider | OptionWithIcon; /** @deprecated Use edges instead */ agent_ids?: string[]; diff --git a/client/src/components/Chat/Input/SkillsCommand.tsx b/client/src/components/Chat/Input/SkillsCommand.tsx index aec1d1b80c..9105c780a3 100644 --- a/client/src/components/Chat/Input/SkillsCommand.tsx +++ b/client/src/components/Chat/Input/SkillsCommand.tsx @@ -96,31 +96,33 @@ function SkillsCommandContent({ 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. */ + /* Resolve the per-agent skill scope. Mirrors backend + `resolveAgentScopedSkillIds`: ephemeral agents always see the full + catalog (picking any skill flips `ephemeralAgent.skills = true` and + activates for the turn); persisted agents gate on the builder's + `skills_enabled` master toggle — off or unset means opt-out, on with + an empty allowlist means full catalog, on with a non-empty allowlist + means narrow to those ids. Persisted agents fail closed during the + `agentsMap` hydration window so the user cannot pick a skill the + backend will then refuse, and again when the map is authoritative + but the agent isn't in it (deleted, or VIEW revoked mid-session). + `agentId` is threaded in as a prop so this component stays + memoizable. */ const agentSkillIds = useMemo(() => { if (!agentId || isEphemeralAgent(agentId)) { return undefined; } if (!agentsMap) { - return undefined; + return []; } const agent = agentsMap[agentId]; if (!agent) { return []; } - return agent.skills; + if (agent.skills_enabled !== true) { + return []; + } + return Array.isArray(agent.skills) && agent.skills.length > 0 ? agent.skills : undefined; }, [agentId, agentsMap]); const { data, isLoading, isError, fetchNextPage, 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 064688552e..53ec6388f7 100644 --- a/client/src/components/Chat/Input/__tests__/SkillsCommand.spec.tsx +++ b/client/src/components/Chat/Input/__tests__/SkillsCommand.spec.tsx @@ -225,7 +225,7 @@ describe('SkillsCommand', () => { expect(mockSetShowSkillsPopover).toHaveBeenCalledWith(false); }); - it('narrows the list to the agent-configured scope when agent.skills is set', async () => { + it('narrows the list to the agent-configured scope when agent.skills is set and skills_enabled is true', async () => { mockUseSkillsInfiniteQuery.mockReturnValue({ data: twoSkillsResponse, isLoading: false, @@ -235,7 +235,7 @@ describe('SkillsCommand', () => { isFetchingNextPage: false, }); mockUseAgentsMapContext.mockReturnValue({ - agent_1: { id: 'agent_1', skills: ['2'] }, + agent_1: { id: 'agent_1', skills: ['2'], skills_enabled: true }, }); const textAreaRef = makeTextarea('$'); @@ -253,7 +253,7 @@ describe('SkillsCommand', () => { expect(await screen.findByRole('button', { name: /Style Guide/i })).toBeInTheDocument(); }); - it('shows nothing when the agent has an empty skills array (explicit opt-out)', () => { + it('shows nothing when the agent has skills_enabled:false, regardless of allowlist', () => { mockUseSkillsInfiniteQuery.mockReturnValue({ data: twoSkillsResponse, isLoading: false, @@ -263,7 +263,7 @@ describe('SkillsCommand', () => { isFetchingNextPage: false, }); mockUseAgentsMapContext.mockReturnValue({ - agent_1: { id: 'agent_1', skills: [] }, + agent_1: { id: 'agent_1', skills: ['1', '2'], skills_enabled: false }, }); const textAreaRef = makeTextarea('$'); @@ -280,7 +280,7 @@ describe('SkillsCommand', () => { expect(screen.queryByRole('button', { name: /Style Guide/i })).toBeNull(); }); - it('shows the full ACL catalog when the agent has no skills field configured', async () => { + it('hides all skills for a persisted agent with skills_enabled undefined (default off)', async () => { mockUseSkillsInfiniteQuery.mockReturnValue({ data: twoSkillsResponse, isLoading: false, @@ -303,8 +303,38 @@ describe('SkillsCommand', () => { />, ); + /* Mirrors backend `resolveAgentScopedSkillIds`: persisted agents are + off by default; the builder's `skills_enabled` master toggle is + the only signal that activates skills for the agent. */ + expect(screen.queryByRole('button', { name: /Brand Guidelines/i })).toBeNull(); + expect(screen.queryByRole('button', { name: /Style Guide/i })).toBeNull(); + }); + + it('shows the full catalog for a persisted agent with skills_enabled:true and empty allowlist', async () => { + mockUseSkillsInfiniteQuery.mockReturnValue({ + data: twoSkillsResponse, + isLoading: false, + isError: false, + fetchNextPage: jest.fn(), + hasNextPage: false, + isFetchingNextPage: false, + }); + mockUseAgentsMapContext.mockReturnValue({ + agent_1: { id: 'agent_1', skills_enabled: true }, + }); + + const textAreaRef = makeTextarea('$'); + render( + , + ); + expect(await screen.findByRole('button', { name: /Brand Guidelines/i })).toBeInTheDocument(); - expect(await screen.findByRole('button', { name: /Style Guide/i })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: /Style Guide/i })).toBeInTheDocument(); }); it('treats an ephemeral agent id as unscoped and shows the full ACL catalog', async () => { @@ -334,7 +364,7 @@ describe('SkillsCommand', () => { 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 () => { + it('fails closed for persisted agents while the agents map is hydrating', async () => { mockUseSkillsInfiniteQuery.mockReturnValue({ data: twoSkillsResponse, isLoading: false, @@ -355,11 +385,12 @@ describe('SkillsCommand', () => { />, ); - /* 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(); + /* Hydration race: agents map not yet loaded. Without `skills_enabled` + visibility we cannot prove the persisted agent opted in, so fail + closed; otherwise the user can pick a skill the backend will then + refuse for the turn. */ + expect(screen.queryByRole('button', { name: /Brand Guidelines/i })).toBeNull(); + expect(screen.queryByRole('button', { name: /Style Guide/i })).toBeNull(); }); it('fails closed when the agent id is set but missing from the agents map', () => { diff --git a/client/src/components/SidePanel/Agents/AgentConfig.tsx b/client/src/components/SidePanel/Agents/AgentConfig.tsx index 8495da8027..2ee88a59c8 100644 --- a/client/src/components/SidePanel/Agents/AgentConfig.tsx +++ b/client/src/components/SidePanel/Agents/AgentConfig.tsx @@ -1,6 +1,6 @@ import React, { useState, useMemo, useCallback } from 'react'; import { X } from 'lucide-react'; -import { useToastContext } from '@librechat/client'; +import { Switch, useToastContext } from '@librechat/client'; import { Controller, useWatch, useFormContext } from 'react-hook-form'; import { EModelEndpoint, @@ -72,8 +72,20 @@ export default function AgentConfig() { const agent = useWatch({ control, name: 'agent' }); const tools = useWatch({ control, name: 'tools' }); const skills = useWatch({ control, name: 'skills' }); + const skillsActive = useWatch({ control, name: 'skills_enabled' }); const agent_id = useWatch({ control, name: 'id' }); + let skillsHintKey: + | 'com_ui_skills_disabled_hint' + | 'com_ui_skills_enabled_allowlist_hint' + | 'com_ui_skills_enabled_all_hint' = 'com_ui_skills_disabled_hint'; + if (skillsActive === true) { + skillsHintKey = + (skills ?? []).length > 0 + ? 'com_ui_skills_enabled_allowlist_hint' + : 'com_ui_skills_enabled_all_hint'; + } + const { codeEnabled, toolsEnabled, @@ -340,10 +352,32 @@ export default function AgentConfig() { {showSkills && (
- -
+
+ + ( + field.onChange(Boolean(value))} + data-testid="skills_enabled" + aria-label={localize('com_ui_skills_enable_toggle')} + /> + )} + /> +
+

{localize(skillsHintKey)}

+
{(skills ?? []).map((skillId) => { const skillName = skillsMap.get(skillId); @@ -368,6 +402,7 @@ export default function AgentConfig() { }} className="ml-2 flex-shrink-0 text-text-secondary transition-colors hover:text-text-primary" aria-label={localize('com_ui_remove_skill_var', { 0: skillName })} + disabled={skillsActive !== true} >