🛡️ feat: Persist & enforce disable-model-invocation / user-invocable / allowed-tools

client/src/components/Chat/Input/SkillsCommand.tsx

@@ -2,7 +2,6 @@ import { memo, useState, useRef, useEffect, useMemo, useCallback } from 'react';
 import { ScrollText } from 'lucide-react';
 import { AutoSizer, List } from 'react-virtualized';
 import { Spinner, useCombobox } from '@librechat/client';
-import { InvocationMode } from 'librechat-data-provider';
 import { useRecoilValue, useSetRecoilState } from 'recoil';
 import type { TSkillSummary } from 'librechat-data-provider';
 import type { MentionOption } from '~/common';
@@ -22,16 +21,13 @@ const skillIcon = <ScrollText className="icon-md text-cyan-500" />;
 
 /**
  * Determines whether a skill should appear in the `$` command popover.
- * `manual` and `both` are user-invocable. `auto` is model-only and hidden.
- * Skills without an explicit mode (undefined) default to visible for
- * backward compatibility until the backend persists `invocationMode`.
+ * Reads the persisted `userInvocable` field (mirrors the `user-invocable`
+ * frontmatter). Defaults to visible when the field is absent so older
+ * skills authored before Phase 6 stay user-invocable without a migration;
+ * only an explicit `false` hides them.
  */
 export function isUserInvocable(skill: TSkillSummary): boolean {
-  const mode = skill.invocationMode;
-  if (mode == null || mode === InvocationMode.both) {
-    return true;
-  }
-  return mode === InvocationMode.manual;
+  return skill.userInvocable !== false;
 }
 
 /**

client/src/components/Chat/Input/__tests__/SkillsCommand.spec.tsx

@@ -14,7 +14,6 @@ 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';
@@ -412,7 +411,7 @@ describe('filterSkillsForPopover', () => {
   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 });
+  const s3 = makeSkill({ _id: '3', name: 'c', userInvocable: false });
 
   it('passes everything through when agentSkillIds is undefined', () => {
     const out = filterSkillsForPopover([s1, s2], { agentSkillIds: undefined, isActive: active });
@@ -440,7 +439,7 @@ describe('filterSkillsForPopover', () => {
     expect(out.map((s) => s._id)).toEqual(['2']);
   });
 
-  it('excludes auto-only skills via isUserInvocable', () => {
+  it('excludes skills with userInvocable: false via isUserInvocable', () => {
     const out = filterSkillsForPopover([s1, s3], { agentSkillIds: null, isActive: active });
     expect(out.map((s) => s._id)).toEqual(['1']);
   });
@@ -456,8 +455,8 @@ describe('filterSkillsForPopover', () => {
       agentSkillIds: ['1', '2', '3'],
       isActive,
     });
-    /* s1 passes (active, manual-by-default, scoped), s2 drops (inactive),
-       s3 drops (auto-only invocation mode). */
+    /* s1 passes (active, user-invocable by default, scoped), s2 drops (inactive),
+       s3 drops (userInvocable: false). */
     expect(out.map((s) => s._id)).toEqual(['1']);
   });
 

packages/api/src/agents/__tests__/initialize.test.ts

@@ -577,3 +577,256 @@ describe('initializeAgent — manual skill priming (Phase 3)', () => {
     expect(result.manualSkillPrimes).toBeUndefined();
   });
 });
+
+describe('initializeAgent — skill `allowed-tools` union (Phase 6)', () => {
+  beforeEach(() => {
+    jest.clearAllMocks();
+  });
+
+  /**
+   * Same minimal pager used in the Phase 3 suite — the catalog isn't what
+   * we're exercising; we just need accessibleSkillIds to be non-empty so the
+   * resolver path runs.
+   */
+  const emptyListSkillsByAccess: InitializeAgentDbMethods['listSkillsByAccess'] = async () => ({
+    skills: [],
+    has_more: false,
+    after: null,
+  });
+
+  /** Helper: build a getSkillByName that returns a single skill with allowedTools. */
+  const buildGetSkillByName = (
+    name: string,
+    allowedTools: string[] | undefined,
+    skillId: import('mongoose').Types.ObjectId,
+    userId: string,
+  ): InitializeAgentDbMethods['getSkillByName'] =>
+    jest.fn().mockResolvedValue({
+      _id: skillId,
+      name,
+      body: `body of ${name}`,
+      author: { toString: () => userId } as unknown as import('mongoose').Types.ObjectId,
+      ...(allowedTools !== undefined ? { allowedTools } : {}),
+    });
+
+  it('passes the union of agent.tools + allowed-tools to loadTools and merges resulting toolDefinitions', async () => {
+    const { agent, req, res, loadTools, db } = createMocks();
+    agent.tools = ['web_search'];
+    const { Types } = await import('mongoose');
+    const skillId = new Types.ObjectId();
+
+    /* Mock loadTools to echo back what was requested as toolDefinitions —
+       lets the test assert both the input list and the output merge. */
+    loadTools.mockImplementation(async ({ tools }: { tools: string[] }) => ({
+      tools: [],
+      toolContextMap: {},
+      userMCPAuthMap: undefined,
+      toolRegistry: undefined,
+      toolDefinitions: tools.map((name: string) => ({ name, description: '', parameters: {} })),
+      hasDeferredTools: false,
+      actionsEnabled: undefined,
+    }));
+
+    const getSkillByName = buildGetSkillByName(
+      'tool-skill',
+      ['execute_code', 'read_file'],
+      skillId,
+      req.user!.id,
+    );
+
+    const result = await initializeAgent(
+      {
+        req,
+        res,
+        agent,
+        loadTools,
+        endpointOption: { endpoint: EModelEndpoint.agents },
+        allowedProviders: new Set([Providers.OPENAI]),
+        isInitialAgent: true,
+        accessibleSkillIds: [skillId],
+        manualSkills: ['tool-skill'],
+      },
+      { ...db, listSkillsByAccess: emptyListSkillsByAccess, getSkillByName },
+    );
+
+    /* Single loadTools call with the union — agent.tools + extras, dedup
+       not needed because unionPrimeAllowedTools already excluded
+       agent-baseline names. Order: agent first, then extras. */
+    expect(loadTools).toHaveBeenCalledTimes(1);
+    expect(loadTools.mock.calls[0][0].tools).toEqual(['web_search', 'execute_code', 'read_file']);
+
+    /* All three tools should appear in the merged toolDefinitions. */
+    const definedNames = result.toolDefinitions?.map((d) => d.name) ?? [];
+    expect(definedNames).toEqual(
+      expect.arrayContaining(['web_search', 'execute_code', 'read_file']),
+    );
+  });
+
+  it('does not call loadTools twice when the skill declares no allowed-tools', async () => {
+    const { agent, req, res, loadTools, db } = createMocks();
+    agent.tools = ['web_search'];
+    const { Types } = await import('mongoose');
+    const skillId = new Types.ObjectId();
+
+    const getSkillByName = buildGetSkillByName('plain', undefined, skillId, req.user!.id);
+
+    await initializeAgent(
+      {
+        req,
+        res,
+        agent,
+        loadTools,
+        endpointOption: { endpoint: EModelEndpoint.agents },
+        allowedProviders: new Set([Providers.OPENAI]),
+        isInitialAgent: true,
+        accessibleSkillIds: [skillId],
+        manualSkills: ['plain'],
+      },
+      { ...db, listSkillsByAccess: emptyListSkillsByAccess, getSkillByName },
+    );
+
+    expect(loadTools).toHaveBeenCalledTimes(1);
+    expect(loadTools.mock.calls[0][0].tools).toEqual(['web_search']);
+  });
+
+  it('skips extras already on the agent (agent baseline wins; no double-loading)', async () => {
+    const { agent, req, res, loadTools, db } = createMocks();
+    agent.tools = ['web_search', 'execute_code'];
+    const { Types } = await import('mongoose');
+    const skillId = new Types.ObjectId();
+
+    const getSkillByName = buildGetSkillByName(
+      'overlap',
+      ['web_search', 'read_file'], // web_search overlaps; read_file is new
+      skillId,
+      req.user!.id,
+    );
+
+    await initializeAgent(
+      {
+        req,
+        res,
+        agent,
+        loadTools,
+        endpointOption: { endpoint: EModelEndpoint.agents },
+        allowedProviders: new Set([Providers.OPENAI]),
+        isInitialAgent: true,
+        accessibleSkillIds: [skillId],
+        manualSkills: ['overlap'],
+      },
+      { ...db, listSkillsByAccess: emptyListSkillsByAccess, getSkillByName },
+    );
+
+    /* web_search is on the agent — not duplicated; only read_file is "extra". */
+    expect(loadTools.mock.calls[0][0].tools).toEqual(['web_search', 'execute_code', 'read_file']);
+  });
+
+  it('retries loadTools without extras when the union call throws (agent tools must still load)', async () => {
+    const { agent, req, res, loadTools, db } = createMocks();
+    agent.tools = ['web_search'];
+    const { Types } = await import('mongoose');
+    const skillId = new Types.ObjectId();
+
+    /* First call (with extras) fails; second call (without extras) succeeds. */
+    let call = 0;
+    loadTools.mockImplementation(async ({ tools }: { tools: string[] }) => {
+      call += 1;
+      if (call === 1) {
+        throw new Error('MCP connection failed for skill-added tool');
+      }
+      return {
+        tools: [],
+        toolContextMap: {},
+        userMCPAuthMap: undefined,
+        toolRegistry: undefined,
+        toolDefinitions: tools.map((name) => ({ name, description: '', parameters: {} })),
+        hasDeferredTools: false,
+        actionsEnabled: undefined,
+      };
+    });
+
+    const getSkillByName = buildGetSkillByName(
+      'bad-tool-skill',
+      ['mcp__broken__tool'],
+      skillId,
+      req.user!.id,
+    );
+
+    const result = await initializeAgent(
+      {
+        req,
+        res,
+        agent,
+        loadTools,
+        endpointOption: { endpoint: EModelEndpoint.agents },
+        allowedProviders: new Set([Providers.OPENAI]),
+        isInitialAgent: true,
+        accessibleSkillIds: [skillId],
+        manualSkills: ['bad-tool-skill'],
+      },
+      { ...db, listSkillsByAccess: emptyListSkillsByAccess, getSkillByName },
+    );
+
+    /* Two calls: union first (threw), then base-only retry (succeeded). */
+    expect(loadTools).toHaveBeenCalledTimes(2);
+    expect(loadTools.mock.calls[0][0].tools).toEqual(['web_search', 'mcp__broken__tool']);
+    expect(loadTools.mock.calls[1][0].tools).toEqual(['web_search']);
+
+    /* Agent's own tool survives; the broken extra is silently dropped. */
+    const definedNames = result.toolDefinitions?.map((d) => d.name) ?? [];
+    expect(definedNames).toContain('web_search');
+    expect(definedNames).not.toContain('mcp__broken__tool');
+  });
+
+  it('propagates the error when loadTools fails AND there are no skill-added extras to drop', async () => {
+    const { agent, req, res, loadTools, db } = createMocks();
+    agent.tools = ['web_search'];
+    /* No skills, no extras — a thrown loadTools is the agent's own problem,
+       not ours to absorb. */
+    loadTools.mockRejectedValueOnce(new Error('agent tool registry corrupted'));
+
+    await expect(
+      initializeAgent(
+        {
+          req,
+          res,
+          agent,
+          loadTools,
+          endpointOption: { endpoint: EModelEndpoint.agents },
+          allowedProviders: new Set([Providers.OPENAI]),
+          isInitialAgent: true,
+          accessibleSkillIds: undefined,
+        },
+        db,
+      ),
+    ).rejects.toThrow('agent tool registry corrupted');
+    expect(loadTools).toHaveBeenCalledTimes(1);
+  });
+
+  it('does not invoke loadTools twice when the agent has no tools and the skill adds none', async () => {
+    const { agent, req, res, loadTools, db } = createMocks();
+    agent.tools = [];
+    const { Types } = await import('mongoose');
+    const skillId = new Types.ObjectId();
+
+    const getSkillByName = buildGetSkillByName('plain', [], skillId, req.user!.id);
+
+    await initializeAgent(
+      {
+        req,
+        res,
+        agent,
+        loadTools,
+        endpointOption: { endpoint: EModelEndpoint.agents },
+        allowedProviders: new Set([Providers.OPENAI]),
+        isInitialAgent: true,
+        accessibleSkillIds: [skillId],
+        manualSkills: ['plain'],
+      },
+      { ...db, listSkillsByAccess: emptyListSkillsByAccess, getSkillByName },
+    );
+
+    expect(loadTools).toHaveBeenCalledTimes(1);
+    expect(loadTools.mock.calls[0][0].tools).toEqual([]);
+  });
+});