CERBERUS/LibreChat
1
0
Fork 0
mirror of https://github.com/danny-avila/LibreChat.git synced 2026-08-31 17:03:24 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 9

🛡️ fix: Strict Opt-In Skills Activation per Agent (#12823)

Browse source 
* 🛡️ 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
This commit is contained in:
Danny Avila 2026-04-25 00:34:12 -07:00
parent 7f3d41024a
commit 596f806f60
20 changed files with 404 additions and 61 deletions
Download patch file Download diff file Expand all files Collapse all files

1
client/src/common/agents-types.ts
View file

@ -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[];

32
client/src/components/Chat/Input/SkillsCommand.tsx
View file

@ -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<string[] | null | undefined>(() => {
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 } =

55
client/src/components/Chat/Input/__tests__/SkillsCommand.spec.tsx
View file

@ -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(
<SkillsCommand
index={0}
textAreaRef={textAreaRef}
conversationId={CONVO_ID}
agentId="agent_1"
/>,
);
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', () => {

46
client/src/components/SidePanel/Agents/AgentConfig.tsx
View file

@ -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 && (
<div className="mb-4">
<label className="text-token-text-primary mb-2 block text-sm font-medium">
{localize('com_ui_skills')}
</label>
<div>
<div className="mb-2 flex items-center justify-between">
<label
htmlFor="skills_enabled"
className="text-token-text-primary block text-sm font-medium"
>
{localize('com_ui_skills')}
</label>
<Controller
name="skills_enabled"
control={control}
render={({ field }) => (
<Switch
id="skills_enabled"
checked={field.value === true}
onCheckedChange={(value: boolean) => field.onChange(Boolean(value))}
data-testid="skills_enabled"
aria-label={localize('com_ui_skills_enable_toggle')}
/>
)}
/>
</div>
<p className="mb-2 text-xs text-text-secondary">{localize(skillsHintKey)}</p>
<div
className={skillsActive === true ? undefined : 'pointer-events-none opacity-50'}
aria-disabled={skillsActive !== true}
>
<div className="mb-1">
{(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}
>
<X className="h-4 w-4" aria-hidden="true" />
</button>
@ -381,6 +416,7 @@ export default function AgentConfig() {
onClick={() => setShowSkillDialog(true)}
className="btn btn-neutral border-token-border-light relative h-9 w-full rounded-lg font-medium"
aria-haspopup="dialog"
disabled={skillsActive !== true}
>
<div className="flex w-full items-center justify-center gap-2">
{localize('com_ui_add_skills')}

2
client/src/components/SidePanel/Agents/AgentPanel.tsx
View file

@ -77,6 +77,7 @@ export function composeAgentUpdatePayload(data: AgentForm, agent_id?: string | n
support_contact,
tool_options,
skills,
skills_enabled,
avatar_action: avatarActionState,
} = data;
@ -105,6 +106,7 @@ export function composeAgentUpdatePayload(data: AgentForm, agent_id?: string | n
support_contact,
tool_options,
skills,
skills_enabled,
...(shouldResetAvatar ? { avatar: null } : {}),
},
provider,

5
client/src/components/SidePanel/Agents/AgentSelect.tsx
View file

@ -115,6 +115,11 @@ function AgentSelect({
return;
}
if (name === 'skills_enabled' && typeof value === 'boolean') {
formValues[name] = value;
return;
}
if (name === 'edges' && Array.isArray(value)) {
formValues[name] = value;
return;

4
client/src/locales/en/translation.json
View file

@ -1546,7 +1546,11 @@
"com_ui_skills_always_apply_invoked": "Auto-applied skills",
"com_ui_skills_always_apply_pin_title": "Always-applied skill (auto-primed on every turn)",
"com_ui_skills_command_placeholder": "Select a Skill by name",
"com_ui_skills_disabled_hint": "Skills are off for this agent. Enable to allow the model to use your skills catalog.",
"com_ui_skills_empty": "No skills yet",
"com_ui_skills_enable_toggle": "Enable skills for this agent",
"com_ui_skills_enabled_all_hint": "Enabled. The full accessible catalog is available; narrow it below if you want to limit which skills this agent can use.",
"com_ui_skills_enabled_allowlist_hint": "Enabled. Limited to the skills selected below.",
"com_ui_skills_load_error": "Failed to load skills",
"com_ui_skills_manual_invoked": "Manually invoked skills",
"com_ui_skills_queued": "Skills queued for next submission",