diff --git a/api/server/controllers/agents/openai.js b/api/server/controllers/agents/openai.js index a5553af5a937..93e2b2fb1be2 100644 --- a/api/server/controllers/agents/openai.js +++ b/api/server/controllers/agents/openai.js @@ -42,6 +42,7 @@ const { findAccessibleResources } = require('~/server/services/PermissionService const { getSkillToolDeps, enrichWithSkillConfigurable, + buildManualSkillPrimedIdsByName, } = require('~/server/services/Endpoints/agents/skillDeps'); const db = require('~/models'); @@ -326,7 +327,13 @@ const OpenAIChatCompletionController = async (req, res) => { tool_resources: primaryConfig.tool_resources, actionsEnabled: primaryConfig.actionsEnabled, }); - return enrichWithSkillConfigurable(result, req, primaryConfig.accessibleSkillIds); + return enrichWithSkillConfigurable( + result, + req, + primaryConfig.accessibleSkillIds, + undefined, + buildManualSkillPrimedIdsByName(primaryConfig.manualSkillPrimes), + ); }, toolEndCallback, ...getSkillToolDeps(), diff --git a/api/server/controllers/agents/responses.js b/api/server/controllers/agents/responses.js index 5703a5817625..2ece214aa943 100644 --- a/api/server/controllers/agents/responses.js +++ b/api/server/controllers/agents/responses.js @@ -51,6 +51,7 @@ const { findAccessibleResources } = require('~/server/services/PermissionService const { getSkillToolDeps, enrichWithSkillConfigurable, + buildManualSkillPrimedIdsByName, } = require('~/server/services/Endpoints/agents/skillDeps'); const db = require('~/models'); @@ -509,7 +510,13 @@ const createResponse = async (req, res) => { tool_resources: primaryConfig.tool_resources, actionsEnabled: primaryConfig.actionsEnabled, }); - return enrichWithSkillConfigurable(result, req, primaryConfig.accessibleSkillIds); + return enrichWithSkillConfigurable( + result, + req, + primaryConfig.accessibleSkillIds, + undefined, + buildManualSkillPrimedIdsByName(primaryConfig.manualSkillPrimes), + ); }, toolEndCallback, ...getSkillToolDeps(), @@ -676,7 +683,13 @@ const createResponse = async (req, res) => { tool_resources: primaryConfig.tool_resources, actionsEnabled: primaryConfig.actionsEnabled, }); - return enrichWithSkillConfigurable(result, req, primaryConfig.accessibleSkillIds); + return enrichWithSkillConfigurable( + result, + req, + primaryConfig.accessibleSkillIds, + undefined, + buildManualSkillPrimedIdsByName(primaryConfig.manualSkillPrimes), + ); }, toolEndCallback, ...getSkillToolDeps(), diff --git a/api/server/services/Endpoints/agents/initialize.js b/api/server/services/Endpoints/agents/initialize.js index 49913f98e3dd..b5a251c1ebc2 100644 --- a/api/server/services/Endpoints/agents/initialize.js +++ b/api/server/services/Endpoints/agents/initialize.js @@ -29,7 +29,11 @@ const { const { loadAgentTools, loadToolsForExecution } = require('~/server/services/ToolService'); const { loadAuthValues } = require('~/server/services/Tools/credentials'); const { filterFilesByAgentAccess } = require('~/server/services/Files/permissions'); -const { getSkillToolDeps, enrichWithSkillConfigurable } = require('./skillDeps'); +const { + getSkillToolDeps, + enrichWithSkillConfigurable, + buildManualSkillPrimedIdsByName, +} = require('./skillDeps'); const { getModelsConfig } = require('~/server/controllers/ModelController'); const { checkPermission, findAccessibleResources } = require('~/server/services/PermissionService'); const AgentClient = require('~/server/controllers/agents/client'); @@ -185,7 +189,13 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { }); logger.debug(`[ON_TOOL_EXECUTE] loaded ${result.loadedTools?.length ?? 0} tools`); - return enrichWithSkillConfigurable(result, req, ctx.accessibleSkillIds, codeApiKey); + return enrichWithSkillConfigurable( + result, + req, + ctx.accessibleSkillIds, + codeApiKey, + ctx.manualSkillPrimedIdsByName, + ); }, toolEndCallback, ...getSkillToolDeps(), @@ -288,6 +298,13 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { logger.debug( `[initializeClient] Storing tool context for ${primaryConfig.id}: ${primaryConfig.toolDefinitions?.length ?? 0} tools, registry size: ${primaryConfig.toolRegistry?.size ?? '0'}`, ); + /** Maps each manually-primed skill name to the `_id` of the exact doc + * that was primed. Plumbed to `enrichWithSkillConfigurable` so the + * read_file handler can pin same-name collision lookups to the + * resolver's chosen doc. */ + const manualSkillPrimedIdsByName = buildManualSkillPrimedIdsByName( + primaryConfig.manualSkillPrimes, + ); agentToolContexts.set(primaryConfig.id, { agent: primaryAgent, toolRegistry: primaryConfig.toolRegistry, @@ -295,6 +312,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { tool_resources: primaryConfig.tool_resources, actionsEnabled: primaryConfig.actionsEnabled, accessibleSkillIds: primaryConfig.accessibleSkillIds, + manualSkillPrimedIdsByName, }); const agent_ids = primaryConfig.agent_ids; diff --git a/api/server/services/Endpoints/agents/skillDeps.js b/api/server/services/Endpoints/agents/skillDeps.js index 32a7995a49a8..ba55a58b3600 100644 --- a/api/server/services/Endpoints/agents/skillDeps.js +++ b/api/server/services/Endpoints/agents/skillDeps.js @@ -5,6 +5,27 @@ const { loadAuthValues } = require('~/server/services/Tools/credentials'); const { enrichWithSkillConfigurable } = require('@librechat/api'); const db = require('~/models'); +/** + * Builds the `manualSkillPrimedIdsByName` map passed through to + * `enrichWithSkillConfigurable`. Centralized here so the four CJS call + * sites (`initialize.js`, `responses.js` x2, `openai.js`) share one + * source of truth — if `ResolvedManualSkill` ever renames `_id` or + * gains new identifying fields, only this helper changes. + * + * Returns `undefined` (not `{}`) when there are no primes, so the + * downstream `enrichWithSkillConfigurable` cleanly omits the field + * from `mergedConfigurable` rather than threading an empty object. + * + * @param {Array<{ name: string, _id: { toString(): string } }> | undefined} manualSkillPrimes + * @returns {Record | undefined} + */ +function buildManualSkillPrimedIdsByName(manualSkillPrimes) { + if (!manualSkillPrimes?.length) { + return undefined; + } + return Object.fromEntries(manualSkillPrimes.map((p) => [p.name, p._id.toString()])); +} + /** Skill-related properties for ToolExecuteOptions (stable references, allocated once). */ const skillToolDeps = { getSkillByName: db.getSkillByName, @@ -28,16 +49,28 @@ function getSkillToolDeps() { * @param {object} req - The Express request object * @param {Array} accessibleSkillIds - Pre-computed accessible skill IDs * @param {string} [preResolvedCodeApiKey] - Pre-resolved code API key (skips redundant lookup) + * @param {Record} [manualSkillPrimedIdsByName] - Map of name → skill id for skills manually invoked this turn via the `$` popover. Pins same-name collision lookups in `read_file`. * @returns {Promise} Augmented result with skill configurable */ -function enrichConfigurable(result, req, accessibleSkillIds, preResolvedCodeApiKey) { +function enrichConfigurable( + result, + req, + accessibleSkillIds, + preResolvedCodeApiKey, + manualSkillPrimedIdsByName, +) { return enrichWithSkillConfigurable( result, req, accessibleSkillIds, loadAuthValues, preResolvedCodeApiKey, + manualSkillPrimedIdsByName, ); } -module.exports = { getSkillToolDeps, enrichWithSkillConfigurable: enrichConfigurable }; +module.exports = { + getSkillToolDeps, + enrichWithSkillConfigurable: enrichConfigurable, + buildManualSkillPrimedIdsByName, +}; diff --git a/client/src/components/Chat/Input/SkillsCommand.tsx b/client/src/components/Chat/Input/SkillsCommand.tsx index 88b94edb51a0..364a0ff32ebf 100644 --- a/client/src/components/Chat/Input/SkillsCommand.tsx +++ b/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 = ; /** * 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; } /** diff --git a/client/src/components/Chat/Input/__tests__/SkillsCommand.spec.tsx b/client/src/components/Chat/Input/__tests__/SkillsCommand.spec.tsx index 18ac124f7733..74ef28d99654 100644 --- a/client/src/components/Chat/Input/__tests__/SkillsCommand.spec.tsx +++ b/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']); }); diff --git a/packages/api/src/agents/__tests__/initialize.test.ts b/packages/api/src/agents/__tests__/initialize.test.ts index f73e8953f288..51fcc6d5ba82 100644 --- a/packages/api/src/agents/__tests__/initialize.test.ts +++ b/packages/api/src/agents/__tests__/initialize.test.ts @@ -477,9 +477,14 @@ describe('initializeAgent — manual skill priming (Phase 3)', () => { ); expect(result.manualSkillPrimes).toEqual([ - { name: 'brand-guidelines', body: '# Brand guidelines\nUse blue.' }, + { _id: skillId, name: 'brand-guidelines', body: '# Brand guidelines\nUse blue.' }, ]); - expect(getSkillByName).toHaveBeenCalledWith('brand-guidelines', [skillId]); + /* `preferUserInvocable` keeps name-collision lookups consistent with + the popover for manual paths — model-only (`userInvocable: false`) + duplicates can't shadow the user-invocable doc the user picked. */ + expect(getSkillByName).toHaveBeenCalledWith('brand-guidelines', [skillId], { + preferUserInvocable: true, + }); }); it('leaves manualSkillPrimes undefined when no manualSkills are provided', async () => { @@ -577,3 +582,361 @@ 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 returns undefined (production loaders swallow errors)', async () => { + /* Production loaders (`createToolLoader` in `initialize.js`, + `openai.js`, `responses.js`) wrap `loadAgentTools` in try/catch + and return `undefined` on failure. Without explicit handling we'd + fall through to the empty fallback and silently drop the agent's + baseline tools. This test pins the retry-on-undefined behavior. */ + const { agent, req, res, loadTools, db } = createMocks(); + agent.tools = ['web_search']; + const { Types } = await import('mongoose'); + const skillId = new Types.ObjectId(); + + let call = 0; + loadTools.mockImplementation(async ({ tools }: { tools: string[] }) => { + call += 1; + if (call === 1) { + return undefined; // simulate swallowed error in createToolLoader + } + return { + tools: [], + toolContextMap: {}, + userMCPAuthMap: undefined, + toolRegistry: undefined, + toolDefinitions: tools.map((name) => ({ name, description: '', parameters: {} })), + hasDeferredTools: false, + actionsEnabled: undefined, + }; + }); + + const getSkillByName = buildGetSkillByName( + 'silent-fail-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: ['silent-fail-skill'], + }, + { ...db, listSkillsByAccess: emptyListSkillsByAccess, getSkillByName }, + ); + + /* Two calls: union first (returned undefined → silent fail), then + base-only retry (succeeded). Agent's web_search survives. */ + 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']); + + const definedNames = result.toolDefinitions?.map((d) => d.name) ?? []; + expect(definedNames).toContain('web_search'); + expect(definedNames).not.toContain('mcp__broken__tool'); + }); + + 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('falls through to empty toolDefinitions when BOTH the union and base-only loadTools calls return undefined', async () => { + /* Worst-case silent-failure path: production loaders catch errors + and return undefined. If the agent's own tools fail to load AND + the retry without extras also fails, we have nothing to give the + LLM. The current behavior is to fall through to the `?? {}` + fallback rather than throw — pinning that contract here so the + turn doesn't crash hard but the agent simply has no tools. */ + const { agent, req, res, loadTools, db } = createMocks(); + agent.tools = ['web_search']; + const { Types } = await import('mongoose'); + const skillId = new Types.ObjectId(); + + /* Both calls (with extras + without extras) silently return undefined. */ + loadTools.mockResolvedValue(undefined); + + const getSkillByName = buildGetSkillByName( + 'broken-skill', + ['some-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: ['broken-skill'], + }, + { ...db, listSkillsByAccess: emptyListSkillsByAccess, getSkillByName }, + ); + + /* Two attempts (initial + retry), both undefined → empty fallback. + The agent gets no tool definitions for the turn but does NOT + crash; downstream code handles the empty case. */ + expect(loadTools).toHaveBeenCalledTimes(2); + expect(result.toolDefinitions).toEqual([]); + }); + + 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([]); + }); +}); diff --git a/packages/api/src/agents/__tests__/skills.test.ts b/packages/api/src/agents/__tests__/skills.test.ts index df91bf8eccfe..aa9fbbd4f6ce 100644 --- a/packages/api/src/agents/__tests__/skills.test.ts +++ b/packages/api/src/agents/__tests__/skills.test.ts @@ -38,6 +38,7 @@ import { extractManualSkills, isSkillPrimeMessage, buildSkillPrimeContentParts, + unionPrimeAllowedTools, MAX_MANUAL_SKILLS, } from '../skills'; import { extractInvokedSkillsFromPayload } from '../run'; @@ -47,6 +48,8 @@ type PageSkill = { name: string; description: string; author: Types.ObjectId; + disableModelInvocation?: boolean; + userInvocable?: boolean; }; describe('extractInvokedSkillsFromPayload', () => { @@ -570,6 +573,126 @@ describe('injectSkillCatalog', () => { expect(result.skillCount).toBe(0); expect(result.activeSkillIds).toEqual([]); }); + + it('excludes disableModelInvocation=true skills from catalog text but keeps them resolvable', async () => { + const open = makeSkill('open-skill', userObjectId); + const modelHidden: PageSkill = { + ...makeSkill('hidden-skill', userObjectId), + disableModelInvocation: true, + }; + const listSkillsByAccess = buildPager([[open, modelHidden]]); + const agent = makeAgent(); + const result = await injectSkillCatalog(baseParams({ listSkillsByAccess, agent })); + /* Catalog text only mentions the open skill — the disabled one costs + zero context tokens. */ + expect(result.skillCount).toBe(1); + expect(agent.additional_instructions).toContain('open-skill'); + expect(agent.additional_instructions).not.toContain('hidden-skill'); + /* But activeSkillIds keeps both — the runtime needs to resolve a + hallucinated/stale call to the disabled skill so handleSkillToolCall + can fire its explicit "cannot be invoked by the model" rejection + instead of a misleading generic "not found". */ + expect(result.activeSkillIds.map((id) => id.toString()).sort()).toEqual( + [open._id.toString(), modelHidden._id.toString()].sort(), + ); + }); + + it('catalog quota counts only model-visible skills (disabled rows do not consume slots)', async () => { + /* Build 3 disabled skills + 1 invocable skill on a single page. With a + hypothetical cap of 100 the disabled rows shouldn't crowd out the + invocable one — but the bug we're guarding against is the cap getting + consumed by disabled rows so the invocable one never lands in the + catalog. Verify the invocable skill makes it in regardless of how + many disabled rows precede it. */ + const disabled = (i: number): PageSkill => ({ + ...makeSkill(`disabled-${i}`, userObjectId), + disableModelInvocation: true, + }); + const visible = makeSkill('visible-skill', userObjectId); + const listSkillsByAccess = buildPager([[disabled(1), disabled(2), disabled(3), visible]]); + const agent = makeAgent(); + const result = await injectSkillCatalog(baseParams({ listSkillsByAccess, agent })); + expect(result.skillCount).toBe(1); + expect(agent.additional_instructions).toContain('visible-skill'); + /* All four are accessible at runtime — disabled ones for the explicit + rejection error, the visible one for normal execution. */ + expect(result.activeSkillIds).toHaveLength(4); + }); + + it('drops disabled docs from activeSkillIds when an invocable doc with the same name is in scope', async () => { + /* Same-name collision: invocable + disabled. Without dedup, getSkillByName + (sort by updatedAt desc) might pick the disabled doc and every model + call to that name would fail with "cannot be invoked". The invocable + doc must win the runtime ACL slot. */ + const invocable = makeSkill('shared-name', userObjectId); + const disabledDup: PageSkill = { + ...makeSkill('shared-name', userObjectId), + disableModelInvocation: true, + }; + const listSkillsByAccess = buildPager([[invocable, disabledDup]]); + const result = await injectSkillCatalog(baseParams({ listSkillsByAccess })); + expect(result.activeSkillIds.map((id) => id.toString())).toEqual([invocable._id.toString()]); + expect(result.skillCount).toBe(1); + }); + + it('keeps the disabled doc in activeSkillIds when no invocable doc with the same name exists', async () => { + /* Sole-disabled-name case: the disabled doc must stay so a hallucinated + model invocation fires the explicit error path. */ + const onlyDisabled: PageSkill = { + ...makeSkill('disabled-only', userObjectId), + disableModelInvocation: true, + }; + const otherInvocable = makeSkill('other-name', userObjectId); + const listSkillsByAccess = buildPager([[onlyDisabled, otherInvocable]]); + const result = await injectSkillCatalog(baseParams({ listSkillsByAccess })); + expect(result.activeSkillIds.map((id) => id.toString()).sort()).toEqual( + [onlyDisabled._id.toString(), otherInvocable._id.toString()].sort(), + ); + /* Only otherInvocable counts toward the catalog — the disabled one stays + runtime-resolvable for explicit error, but not in the model's prompt. */ + expect(result.skillCount).toBe(1); + }); + + it('omits catalog text + skill tool when every active skill is model-disabled, but keeps read_file', async () => { + /* Edge case: only `disable-model-invocation` skills accessible. The + model can't see any catalog and the `skill` tool isn't registered + (no targets to invoke) — but `read_file` MUST stay registered. + Manually-primed disabled skills still get their SKILL.md body in + context, and the body may reference `references/*` files that + require read_file to load. */ + const ownedHidden: PageSkill = { + ...makeSkill('owned-hidden', userObjectId), + disableModelInvocation: true, + }; + const listSkillsByAccess = buildPager([[ownedHidden]]); + const agent = makeAgent(); + const result = await injectSkillCatalog(baseParams({ listSkillsByAccess, agent })); + expect(result.skillCount).toBe(0); + expect(agent.additional_instructions).toBeUndefined(); + /* read_file is registered; the `skill` tool is NOT (would burn tokens + advertising a tool with no model-reachable targets). */ + const definedNames = (result.toolDefinitions ?? []).map((d) => d.name); + expect(definedNames).toContain('read_file'); + expect(definedNames).not.toContain('skill'); + expect(result.activeSkillIds.map((id) => id.toString())).toEqual([ownedHidden._id.toString()]); + }); + + it('registers bash_tool alongside read_file when codeEnvAvailable, even with no catalog', async () => { + /* Same edge case as above but with code env on — manually-primed + skills can use bash_tool too. */ + const ownedHidden: PageSkill = { + ...makeSkill('owned-hidden-code', userObjectId), + disableModelInvocation: true, + }; + const listSkillsByAccess = buildPager([[ownedHidden]]); + const result = await injectSkillCatalog( + baseParams({ listSkillsByAccess, codeEnvAvailable: true }), + ); + const definedNames = (result.toolDefinitions ?? []).map((d) => d.name); + expect(definedNames).toContain('read_file'); + expect(definedNames).toContain('bash_tool'); + expect(definedNames).not.toContain('skill'); + }); }); describe('buildSkillPrimeMessage', () => { @@ -605,6 +728,7 @@ describe('resolveManualSkills', () => { body: string; author: Types.ObjectId; allowedTools?: string[]; + userInvocable?: boolean; }; const buildGetSkillByName = @@ -647,7 +771,7 @@ describe('resolveManualSkills', () => { accessibleSkillIds: [owned._id], userId, }); - expect(result).toEqual([{ name: 'my-skill', body: 'MY SKILL BODY' }]); + expect(result).toEqual([{ _id: owned._id, name: 'my-skill', body: 'MY SKILL BODY' }]); }); it('passes allowedTools through when the skill doc carries the field', async () => { @@ -662,7 +786,12 @@ describe('resolveManualSkills', () => { userId, }); expect(result).toEqual([ - { name: 'with-tools', body: 'body', allowedTools: ['execute_code', 'read_file'] }, + { + _id: owned._id, + name: 'with-tools', + body: 'body', + allowedTools: ['execute_code', 'read_file'], + }, ]); }); @@ -674,7 +803,7 @@ describe('resolveManualSkills', () => { accessibleSkillIds: [owned._id], userId, }); - expect(resolved).toEqual({ name: 'no-tools', body: 'body' }); + expect(resolved).toEqual({ _id: owned._id, name: 'no-tools', body: 'body' }); expect(resolved).not.toHaveProperty('allowedTools'); }); @@ -686,7 +815,12 @@ describe('resolveManualSkills', () => { accessibleSkillIds: [owned._id], userId, }); - expect(resolved).toEqual({ name: 'empty-tools', body: 'body', allowedTools: [] }); + expect(resolved).toEqual({ + _id: owned._id, + name: 'empty-tools', + body: 'body', + allowedTools: [], + }); }); it('silently skips names with no backing skill (typo / ACL miss) without failing the batch', async () => { @@ -697,7 +831,75 @@ describe('resolveManualSkills', () => { accessibleSkillIds: [real._id], userId, }); - expect(result).toEqual([{ name: 'real', body: 'body of real' }]); + expect(result).toEqual([{ _id: real._id, name: 'real', body: 'body of real' }]); + }); + + it('silently skips skills with userInvocable: false, preserving the rest of the batch', async () => { + const open = mkSkill('open', userOid); + const modelOnly: SkillDoc = { ...mkSkill('model-only', userOid), userInvocable: false }; + const result = await resolveManualSkills({ + names: ['open', 'model-only'], + getSkillByName: buildGetSkillByName({ open, 'model-only': modelOnly }), + accessibleSkillIds: [open._id, modelOnly._id], + userId, + }); + /* Defense-in-depth: even if a popover-bypassing API caller names a + userInvocable:false skill, the resolver drops it with a warn log + rather than priming SKILL.md. The rest of the batch survives. */ + expect(result).toEqual([{ _id: open._id, name: 'open', body: 'body of open' }]); + }); + + it('passes preferUserInvocable to getSkillByName so name-collision picks the user-invocable doc, but does NOT pass preferModelInvocable (manual primes for disabled skills are supported)', async () => { + /* Same-name collision scenario: the popover surfaced the older + user-invocable doc; the resolver must look it up with + preferUserInvocable so a newer model-only (`userInvocable: false`) + duplicate doesn't silently shadow the user's selection. We + deliberately do NOT pass preferModelInvocable — manually invoking + a `disable-model-invocation: true` skill is the supported path + (iter 4), and adding the model-invocable filter would skip those + docs and break manual invocation of disabled skills. */ + const invocableDoc = mkSkill('collide', userOid, 'invocable body'); + const getSkillByName = jest.fn( + async ( + _name: string, + _ids: Types.ObjectId[], + options?: { preferUserInvocable?: boolean; preferModelInvocable?: boolean }, + ) => { + /* Return the invocable doc only when called with preferUserInvocable. + Without the flag, this fake returns null (simulating the + newer non-user-invocable duplicate scenario). */ + return options?.preferUserInvocable ? invocableDoc : null; + }, + ); + const result = await resolveManualSkills({ + names: ['collide'], + getSkillByName, + accessibleSkillIds: [invocableDoc._id], + userId, + }); + expect(result).toEqual([{ _id: invocableDoc._id, name: 'collide', body: 'invocable body' }]); + expect(getSkillByName).toHaveBeenCalledWith('collide', [invocableDoc._id], { + preferUserInvocable: true, + }); + /* Crucial: no preferModelInvocable — would skip disabled skills and + break the iter 4 manual-prime exception for disabled skills. */ + const callOptions = getSkillByName.mock.calls[0][2]; + expect(callOptions).not.toHaveProperty('preferModelInvocable', true); + }); + + it('treats userInvocable: true (or absent) as user-invocable', async () => { + const explicit: SkillDoc = { ...mkSkill('explicit', userOid), userInvocable: true }; + const implicit = mkSkill('implicit', userOid); + const result = await resolveManualSkills({ + names: ['explicit', 'implicit'], + getSkillByName: buildGetSkillByName({ explicit, implicit }), + accessibleSkillIds: [explicit._id, implicit._id], + userId, + }); + expect(result).toEqual([ + { _id: explicit._id, name: 'explicit', body: 'body of explicit' }, + { _id: implicit._id, name: 'implicit', body: 'body of implicit' }, + ]); }); it('dedupes repeated names, preserving first occurrence order', async () => { @@ -733,7 +935,7 @@ describe('resolveManualSkills', () => { userId, defaultActiveOnShare: true, }); - expect(result).toEqual([{ name: 'shared', body: 'shared-body' }]); + expect(result).toEqual([{ _id: shared._id, name: 'shared', body: 'shared-body' }]); }); it('drops explicitly-deactivated skills (skillStates override wins over ownership default)', async () => { @@ -758,7 +960,7 @@ describe('resolveManualSkills', () => { defaultActiveOnShare: false, skillStates: { [shared._id.toString()]: true }, }); - expect(result).toEqual([{ name: 'shared-on', body: 'on-body' }]); + expect(result).toEqual([{ _id: shared._id, name: 'shared-on', body: 'on-body' }]); }); it('skips skills with empty bodies (priming nothing adds no value)', async () => { @@ -786,7 +988,7 @@ describe('resolveManualSkills', () => { accessibleSkillIds: [good._id], userId, }); - expect(result).toEqual([{ name: 'good', body: 'good-body' }]); + expect(result).toEqual([{ _id: good._id, name: 'good', body: 'good-body' }]); }); it('truncates manual skill lists above MAX_MANUAL_SKILLS to bound concurrent DB lookups', async () => { @@ -1083,3 +1285,77 @@ describe('buildSkillPrimeContentParts', () => { expect(first[0].tool_call.id).not.toBe(second[0].tool_call.id); }); }); + +describe('unionPrimeAllowedTools', () => { + it('returns no extras when no primes carry allowedTools', () => { + const result = unionPrimeAllowedTools({ + primes: [{ name: 'a' }, { name: 'b', allowedTools: [] }], + agentToolNames: ['web_search'], + }); + expect(result.extraToolNames).toEqual([]); + expect(result.perSkillExtras.size).toBe(0); + }); + + it('skips tools already configured on the agent (agent baseline wins)', () => { + const result = unionPrimeAllowedTools({ + primes: [{ name: 'skill-a', allowedTools: ['web_search', 'execute_code'] }], + agentToolNames: ['web_search'], + }); + /* web_search is already on the agent — only execute_code is "extra". */ + expect(result.extraToolNames).toEqual(['execute_code']); + expect(result.perSkillExtras.get('skill-a')).toEqual(['execute_code']); + }); + + it('dedupes across skills (same tool requested by two primes counts once)', () => { + const result = unionPrimeAllowedTools({ + primes: [ + { name: 'skill-a', allowedTools: ['execute_code', 'read_file'] }, + { name: 'skill-b', allowedTools: ['execute_code', 'web_search'] }, + ], + agentToolNames: [], + }); + expect(result.extraToolNames.sort()).toEqual(['execute_code', 'read_file', 'web_search']); + /* Per-skill attribution credits the FIRST skill that contributed + a tool name — keeps debug logs stable instead of duplicating. */ + expect(result.perSkillExtras.get('skill-a')).toEqual(['execute_code', 'read_file']); + expect(result.perSkillExtras.get('skill-b')).toEqual(['web_search']); + }); + + it('filters out empty / non-string entries (defensive — frontmatter parser should already)', () => { + const result = unionPrimeAllowedTools({ + primes: [ + { + name: 'skill-a', + allowedTools: ['execute_code', '', 42 as unknown as string, 'web_search'], + }, + ], + agentToolNames: [], + }); + expect(result.extraToolNames).toEqual(['execute_code', 'web_search']); + }); + + it('omits skills with no contribution from the per-skill map', () => { + const result = unionPrimeAllowedTools({ + primes: [ + { name: 'skill-on-agent', allowedTools: ['web_search'] }, + { name: 'skill-extra', allowedTools: ['execute_code'] }, + ], + agentToolNames: ['web_search'], + }); + expect(result.extraToolNames).toEqual(['execute_code']); + expect(result.perSkillExtras.get('skill-on-agent')).toBeUndefined(); + expect(result.perSkillExtras.get('skill-extra')).toEqual(['execute_code']); + }); + + it('preserves first-occurrence order across skills (deterministic for log readability)', () => { + const result = unionPrimeAllowedTools({ + primes: [ + { name: 'a', allowedTools: ['z-tool'] }, + { name: 'b', allowedTools: ['m-tool', 'a-tool'] }, + { name: 'c', allowedTools: ['z-tool', 'b-tool'] }, + ], + agentToolNames: [], + }); + expect(result.extraToolNames).toEqual(['z-tool', 'm-tool', 'a-tool', 'b-tool']); + }); +}); diff --git a/packages/api/src/agents/handlers.spec.ts b/packages/api/src/agents/handlers.spec.ts index 5b8072f74308..77fb42274923 100644 --- a/packages/api/src/agents/handlers.spec.ts +++ b/packages/api/src/agents/handlers.spec.ts @@ -175,4 +175,316 @@ describe('createToolExecuteHandler', () => { expect(capturedConfigs[0]._injected_files).toBeUndefined(); }); }); + + describe('skill tool model-invocation gate', () => { + function createSkillHandler(getSkillByName: ToolExecuteOptions['getSkillByName']) { + const loadTools: ToolExecuteOptions['loadTools'] = jest.fn(async () => ({ + loadedTools: [], + })); + return createToolExecuteHandler({ loadTools, getSkillByName }); + } + + it('rejects with a clear error when the named skill has disableModelInvocation=true', async () => { + const getSkillByName = jest.fn(async () => ({ + _id: 'skill-id' as unknown as never, + name: 'pii-redactor', + body: 'restricted body', + fileCount: 0, + disableModelInvocation: true, + })); + const handler = createSkillHandler(getSkillByName); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_skill_1', + name: Constants.SKILL_TOOL, + args: { skillName: 'pii-redactor' }, + }, + ]); + + expect(result.status).toBe('error'); + expect(result.errorMessage).toContain('cannot be invoked by the model'); + expect(result.errorMessage).toContain('pii-redactor'); + }); + + it('returns the regular not-accessible error when the skill itself is missing (gate runs after lookup)', async () => { + const getSkillByName = jest.fn(async () => null); + const handler = createSkillHandler(getSkillByName); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_skill_2', + name: Constants.SKILL_TOOL, + args: { skillName: 'ghost' }, + }, + ]); + + expect(result.status).toBe('error'); + /* Distinct error message — operators can tell "not in catalog" apart + from "exists but model-blocked". */ + expect(result.errorMessage).toContain('not found or not accessible'); + expect(result.errorMessage).not.toContain('cannot be invoked'); + }); + + it('lets through skills without disableModelInvocation set (default behavior)', async () => { + const getSkillByName = jest.fn(async () => ({ + _id: 'skill-id' as unknown as never, + name: 'normal-skill', + body: 'body', + fileCount: 0, + })); + const handler = createSkillHandler(getSkillByName); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_skill_3', + name: Constants.SKILL_TOOL, + args: { skillName: 'normal-skill' }, + }, + ]); + + expect(result.status).toBe('success'); + expect(result.content).toContain('normal-skill'); + }); + + it('skill tool calls getSkillByName with preferModelInvocable (and NOT preferUserInvocable, so model-only skills still resolve)', async () => { + /* The skill tool should resolve to the cataloged model-invocable doc + when a same-name disabled duplicate exists — passing + preferModelInvocable keeps the resolution consistent with the + catalog. We do NOT pass preferUserInvocable: model-only skills + (`userInvocable: false`) are valid model-invocation targets, and + filtering them out would let an older user-invocable duplicate + shadow the cataloged model-only skill. Falls back to newest when + only a disabled doc exists so the gate fires its explicit error. */ + const getSkillByName = jest.fn(async () => ({ + _id: 'skill-id' as unknown as never, + name: 'maybe-disabled', + body: 'body', + fileCount: 0, + })); + const handler = createSkillHandler(getSkillByName); + + await invokeHandler(handler, [ + { + id: 'call_skill_4', + name: Constants.SKILL_TOOL, + args: { skillName: 'maybe-disabled' }, + }, + ]); + + expect(getSkillByName).toHaveBeenCalledWith('maybe-disabled', expect.any(Array), { + preferModelInvocable: true, + }); + const callOptions = (getSkillByName.mock.calls[0] as unknown[])[2] as + | { preferUserInvocable?: boolean } + | undefined; + expect(callOptions).not.toHaveProperty('preferUserInvocable', true); + }); + + it('read_file uses preferModelInvocable for AUTONOMOUS probes (skill not in manualSkillPrimedIdsByName)', async () => { + const getSkillByName = jest.fn(async () => ({ + _id: 'skill-id' as unknown as never, + name: 'maybe-disabled-read', + body: '# Body', + fileCount: 0, + })); + const handler = createToolExecuteHandler({ + loadTools: jest.fn(async () => ({ loadedTools: [] })), + getSkillByName, + }); + + await invokeHandler(handler, [ + { + id: 'call_read_5', + name: Constants.READ_FILE, + args: { file_path: 'maybe-disabled-read/SKILL.md' }, + }, + ]); + + expect(getSkillByName).toHaveBeenCalledWith('maybe-disabled-read', expect.any(Array), { + preferModelInvocable: true, + }); + const callOptions = (getSkillByName.mock.calls[0] as unknown[])[2] as + | { preferUserInvocable?: boolean } + | undefined; + expect(callOptions).not.toHaveProperty('preferUserInvocable', true); + }); + + it("read_file pins lookup to the primed skill's _id when manually invoked this turn (no shadowing on collision)", async () => { + /* Same-name collision corner: the resolver primed a specific doc + (its `_id` is in `manualSkillPrimedIdsByName`). If read_file used + the full ACL set + a `prefer*` flag, a same-name duplicate could + shadow the resolver's pick and the model would read files from + the WRONG skill. The handler now constrains accessibleIds to + the primed `_id`, so the lookup returns the EXACT same doc. */ + const { Types } = jest.requireActual('mongoose') as typeof import('mongoose'); + const primedHex = '507f1f77bcf86cd799439011'; + const getSkillByName = jest.fn(async () => ({ + _id: new Types.ObjectId(primedHex) as unknown as never, + name: 'manually-primed', + body: '# Body', + fileCount: 0, + })); + const handler = createToolExecuteHandler({ + loadTools: jest.fn(async () => ({ + loadedTools: [], + configurable: { + manualSkillPrimedIdsByName: { 'manually-primed': primedHex }, + }, + })), + getSkillByName, + }); + + await invokeHandler(handler, [ + { + id: 'call_read_6', + name: Constants.READ_FILE, + args: { file_path: 'manually-primed/references/foo.md' }, + }, + ]); + + /* Lookup is pinned to the primed _id (single-element array) and + carries no preference flags — the constrained accessibleIds set + already disambiguates which doc to return. */ + expect(getSkillByName).toHaveBeenCalledTimes(1); + const [calledName, calledIds, calledOpts] = getSkillByName.mock.calls[0] as unknown as [ + string, + Array<{ toString(): string }>, + object, + ]; + expect(calledName).toBe('manually-primed'); + expect(calledOpts).toEqual({}); + expect(calledIds).toHaveLength(1); + /* Constructed `ObjectId` with the primed hex — `.toString()` + produces the same hex back. Compare via the canonical form to + avoid coupling to the runtime ObjectId class. */ + expect(calledIds[0].toString()).toBe(primedHex); + }); + + it('rejects read_file tool calls for disableModelInvocation skills (file ACL parity)', async () => { + /* The `read_file` handler shares `accessibleSkillIds` with the skill + tool. Without the disableModelInvocation gate there too, a model + that learned a hidden skill's name (stale catalog, hallucination) + could read its SKILL.md body or bundled files via `read_file`, + defeating the contract. */ + const getSkillByName = jest.fn(async () => ({ + _id: 'skill-id' as unknown as never, + name: 'pii-redactor', + body: 'restricted body', + fileCount: 0, + disableModelInvocation: true, + })); + const handler = createToolExecuteHandler({ + loadTools: jest.fn(async () => ({ loadedTools: [] })), + getSkillByName, + getSkillFileByPath: jest.fn(), + }); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_read_1', + name: Constants.READ_FILE, + args: { file_path: 'pii-redactor/SKILL.md' }, + }, + ]); + + expect(result.status).toBe('error'); + expect(result.errorMessage).toContain('cannot be invoked by the model'); + expect(result.errorMessage).toContain('pii-redactor'); + }); + + it('lets read_file calls through for normal skills (regression: gate is not over-broad)', async () => { + const getSkillByName = jest.fn(async () => ({ + _id: 'skill-id' as unknown as never, + name: 'normal-skill', + body: '# Body', + fileCount: 0, + })); + const handler = createToolExecuteHandler({ + loadTools: jest.fn(async () => ({ loadedTools: [] })), + getSkillByName, + }); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_read_2', + name: Constants.READ_FILE, + args: { file_path: 'normal-skill/SKILL.md' }, + }, + ]); + + expect(result.status).toBe('success'); + expect(result.content).toContain('Body'); + }); + + it('allows read_file for a manually-primed disabled skill (manual `$` invocation must stay usable)', async () => { + /* Disabled skill that the user manually invoked this turn. The body + is already primed into context via `manualSkillPrimes`; if read_file + were also blocked here, any skill referencing `references/foo.md` + in its body would be non-functional under manual invocation. The + autonomous-block contract is preserved because the bypass is + scoped to the per-turn `manualSkillPrimedIdsByName` allowlist. */ + const getSkillByName = jest.fn(async () => ({ + _id: '507f1f77bcf86cd799439020' as unknown as never, + name: 'manual-only-skill', + body: '# Use references/docs.md for details', + fileCount: 0, + disableModelInvocation: true, + })); + const handler = createToolExecuteHandler({ + loadTools: jest.fn(async () => ({ + loadedTools: [], + configurable: { + manualSkillPrimedIdsByName: { 'manual-only-skill': '507f1f77bcf86cd799439020' }, + }, + })), + getSkillByName, + }); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_read_3', + name: Constants.READ_FILE, + args: { file_path: 'manual-only-skill/SKILL.md' }, + }, + ]); + + expect(result.status).toBe('success'); + expect(result.content).toContain('references/docs.md'); + }); + + it('still blocks read_file for a disabled skill the user did NOT manually prime this turn', async () => { + /* Defense-in-depth: the manual-prime exception is scoped to the + specific names in `manualSkillPrimedIdsByName`. A model trying + to read a different disabled skill (one the user never manually + invoked) is still rejected. */ + const getSkillByName = jest.fn(async () => ({ + _id: 'other-id' as unknown as never, + name: 'other-disabled-skill', + body: 'restricted', + fileCount: 0, + disableModelInvocation: true, + })); + const handler = createToolExecuteHandler({ + loadTools: jest.fn(async () => ({ + loadedTools: [], + configurable: { + manualSkillPrimedIdsByName: { 'something-else': '507f1f77bcf86cd799439030' }, + }, + })), + getSkillByName, + }); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_read_4', + name: Constants.READ_FILE, + args: { file_path: 'other-disabled-skill/SKILL.md' }, + }, + ]); + + expect(result.status).toBe('error'); + expect(result.errorMessage).toContain('cannot be invoked by the model'); + }); + }); }); diff --git a/packages/api/src/agents/handlers.ts b/packages/api/src/agents/handlers.ts index 778540d38426..8ea44cdc046a 100644 --- a/packages/api/src/agents/handlers.ts +++ b/packages/api/src/agents/handlers.ts @@ -9,7 +9,7 @@ import type { ToolExecuteResult, ToolExecuteBatchRequest, } from '@librechat/agents'; -import type { Types } from 'mongoose'; +import { Types } from 'mongoose'; import type { StructuredToolInterface } from '@langchain/core/tools'; import type { ServerRequest } from '~/types'; import { primeSkillFiles } from './skillFiles'; @@ -49,15 +49,31 @@ export interface ToolExecuteOptions { }>; /** Callback to process tool artifacts (code output files, file citations, etc.) */ toolEndCallback?: ToolEndCallback; - /** Loads a skill by name with ACL constraint (returns full body for injection) */ + /** + * Loads a skill by name with ACL constraint (returns full body for injection). + * + * `options.preferModelInvocable` (Phase 6): on a same-name collision, + * prefer the newest `disableModelInvocation !== true` doc. Avoids a + * newer disabled duplicate shadowing the cataloged model-invocable + * skill the model actually targeted; falls back to newest match so + * the explicit-rejection gate can still fire in the disabled-only case. + */ getSkillByName?: ( name: string, accessibleIds: Types.ObjectId[], + options?: { preferUserInvocable?: boolean; preferModelInvocable?: boolean }, ) => Promise<{ body: string; name: string; _id: Types.ObjectId; fileCount: number; + /** + * Set when the skill author opted out of model invocation. The handler + * rejects the call and returns an instructive error so the model knows + * it can't reach the skill via the `skill` tool — manual `$` invocation + * is still allowed and goes through `resolveManualSkills` instead. + */ + disableModelInvocation?: boolean; } | null>; /** Lists files bundled with a skill (for code env priming) */ listSkillFiles?: (skillId: Types.ObjectId | string) => Promise; @@ -167,7 +183,27 @@ async function handleReadFileCall( } const accessibleIds = (mergedConfigurable?.accessibleSkillIds as Types.ObjectId[]) ?? []; - const skill = await getSkillByName(skillName, accessibleIds); + const manualSkillPrimedIdsByName = + (mergedConfigurable?.manualSkillPrimedIdsByName as Record | undefined) ?? {}; + const primedIdString = manualSkillPrimedIdsByName[skillName]; + const isManuallyPrimedThisTurn = primedIdString != null; + /* On a manually-primed lookup, pin the accessible set to ONLY the + primed `_id`. This guarantees the doc whose body got primed is the + SAME doc whose files we read, even when same-name duplicates exist + and `activeSkillIds` had to drop some via the disable-model dedup. + For autonomous probes we keep the full ACL set + `preferModelInvocable` + so the lookup matches the catalog the model saw (and falls back to + newest so the disabled-only case still fires the explicit rejection + gate below). Constructing a real `ObjectId` (rather than relying on + mongoose's string auto-cast in `$in` queries) keeps the value + correct for any future consumer that compares with `.equals()` or + `===`. */ + const lookupAccessibleIds = isManuallyPrimedThisTurn + ? [new Types.ObjectId(primedIdString)] + : accessibleIds; + const lookupOptions: { preferUserInvocable?: boolean; preferModelInvocable?: boolean } = + isManuallyPrimedThisTurn ? {} : { preferModelInvocable: true }; + const skill = await getSkillByName(skillName, lookupAccessibleIds, lookupOptions); if (!skill) { return { toolCallId: tc.id, @@ -177,6 +213,29 @@ async function handleReadFileCall( }; } + /** + * `disable-model-invocation: true` blocks AUTONOMOUS read_file probes: + * a model that learned a hidden skill's name (stale catalog, hallucination) + * shouldn't be able to read its SKILL.md body or bundled files. But when + * the user explicitly invoked the skill manually this turn, the body is + * already primed into context — and a manually-primed skill that depends + * on `references/foo.md` would be non-functional if read_file were + * blocked. Bypass the gate for manually-primed skill names so manual `$` + * invocation of disabled skills stays usable end-to-end. + * + * Sticky-primed skills (manually or model-invoked in prior turns) are not + * yet in this exception list — that's a known limitation tracked for + * a follow-up. Same-turn manual invocation is the load-bearing path. + */ + if (skill.disableModelInvocation === true && !isManuallyPrimedThisTurn) { + return { + toolCallId: tc.id, + status: 'error', + content: '', + errorMessage: `Skill "${skillName}" cannot be invoked by the model`, + }; + } + // SKILL.md special case: read from skill.body directly if (relativePath === 'SKILL.md') { if (!skill.body) { @@ -422,7 +481,16 @@ async function handleSkillToolCall( } const accessibleIds = (mergedConfigurable?.accessibleSkillIds as Types.ObjectId[]) ?? []; - const skill = await getSkillByName(args.skillName, accessibleIds); + /* `preferModelInvocable` keeps name-collision resolution aligned with + the catalog: a newer `disable-model-invocation: true` duplicate + can't shadow the cataloged invocable doc. Model-only + (`userInvocable: false`) skills are intentionally still resolvable + here — they're valid model-invocation targets. Falls back to the + newest match so the disabled-only case still resolves and the gate + below fires its explicit error. */ + const skill = await getSkillByName(args.skillName, accessibleIds, { + preferModelInvocable: true, + }); if (!skill) { return { @@ -433,6 +501,23 @@ async function handleSkillToolCall( }; } + /** + * `disable-model-invocation: true` skills are excluded from the catalog + * the model sees, but a model that learned the name elsewhere (stale + * cache, hallucinated guess) could still try to invoke it. Reject + * explicitly so the error message tells the model exactly why and it + * doesn't loop retrying. Manual `$` invocation goes through + * `resolveManualSkills`, which is unaffected by this flag. + */ + if (skill.disableModelInvocation === true) { + return { + toolCallId: tc.id, + status: 'error', + content: '', + errorMessage: `Skill "${args.skillName}" cannot be invoked by the model`, + }; + } + let body = skill.body; if (args.args) { body = body.replace(/\$ARGUMENTS/g, args.args); diff --git a/packages/api/src/agents/initialize.ts b/packages/api/src/agents/initialize.ts index ec4e4539fe2b..4ef4432ffa12 100644 --- a/packages/api/src/agents/initialize.ts +++ b/packages/api/src/agents/initialize.ts @@ -1,4 +1,5 @@ import { Providers } from '@librechat/agents'; +import { logger } from '@librechat/data-schemas'; import { Constants, ErrorTypes, @@ -30,9 +31,9 @@ import { import { filterFilesByEndpointConfig } from '~/files'; import { generateArtifactsPrompt } from '~/prompts'; import { getProviderConfig } from '~/endpoints'; -import { injectSkillCatalog, resolveManualSkills } from './skills'; -import type { ResolvedManualSkill } from './skills'; +import { injectSkillCatalog, resolveManualSkills, unionPrimeAllowedTools } from './skills'; import { primeResources } from './resources'; +import type { ResolvedManualSkill } from './skills'; import type { TFilterFilesByAgentAccess } from './resources'; /** @@ -184,6 +185,17 @@ export interface InitializeAgentDbMethods extends EndpointDbMethods { name: string; description: string; author: import('mongoose').Types.ObjectId; + /** + * When `true`, the skill is excluded from the catalog injected into + * the agent's additional_instructions and the model cannot invoke it + * via the `skill` tool. Manual `$` invocation is unaffected. + */ + disableModelInvocation?: boolean; + /** + * When `false`, the skill is hidden from the `$` popover and rejected + * by the manual-invocation resolver. Defaults to `true`. + */ + userInvocable?: boolean; }>; has_more?: boolean; after?: string | null; @@ -192,10 +204,19 @@ export interface InitializeAgentDbMethods extends EndpointDbMethods { * Load a single skill by name, constrained to an ACL-accessible ID set. * Returns the full document (including `body`) so manual invocation can * prime SKILL.md without a second DB round-trip. + * + * `preferUserInvocable` (manual paths): on a same-name collision, + * prefer the newest doc with `userInvocable !== false`. + * `preferModelInvocable` (model paths — `skill` / `read_file`): on a + * same-name collision, prefer the newest doc with + * `disableModelInvocation !== true`. Both fall back to the newest match + * so the explicit-rejection error paths still fire when only the + * non-preferred variant exists. */ getSkillByName?: ( name: string, accessibleIds: import('mongoose').Types.ObjectId[], + options?: { preferUserInvocable?: boolean; preferModelInvocable?: boolean }, ) => Promise<{ _id: import('mongoose').Types.ObjectId; name: string; @@ -207,6 +228,19 @@ export interface InitializeAgentDbMethods extends EndpointDbMethods { * future runtime enforcement without a second round-trip. */ allowedTools?: string[]; + /** + * Set when the skill was authored with `disable-model-invocation: true`. + * The skill tool handler short-circuits on this so a model that names + * such a skill (e.g. via hallucination or stale catalog) gets a clear + * rejection instead of silently executing. + */ + disableModelInvocation?: boolean; + /** + * Set when the skill was authored with `user-invocable: false`. The + * manual-invocation resolver skips with a warn log so an API-direct + * caller can't bypass the popover-side filter. + */ + userInvocable?: boolean; } | null>; } @@ -360,6 +394,106 @@ export async function initializeAgent( requestFileSet: new Set(requestFiles?.map((file) => file.file_id)), }); + /** + * Pre-resolve manually-invoked skill primes so their `allowed-tools` can + * be unioned into the agent's effective tool set BEFORE `loadTools` runs. + * Single load is correctness-critical: a second `loadTools` pass would + * compute its own `userMCPAuthMap` / `toolContextMap` / OAuth flow state + * that the InitializedAgent never sees, so an MCP tool added via + * `allowed-tools` would be visible to the model but fail at execution + * time without its per-user auth context. + * + * Resolution uses `params.accessibleSkillIds` (not the active-filtered + * subset that `injectSkillCatalog` will produce later) — see + * `resolveManualSkills` doc for why a skill outside the catalog cap can + * still be authorizable for direct manual invocation. + */ + let manualSkillPrimes: ResolvedManualSkill[] | undefined; + let extraAllowedToolNames: string[] = []; + let perSkillExtras: Map = new Map(); + if ( + params.manualSkills?.length && + db.getSkillByName && + params.accessibleSkillIds && + params.accessibleSkillIds.length > 0 + ) { + manualSkillPrimes = await resolveManualSkills({ + names: params.manualSkills, + getSkillByName: db.getSkillByName, + accessibleSkillIds: params.accessibleSkillIds, + userId: req.user?.id, + skillStates: params.skillStates, + defaultActiveOnShare: params.defaultActiveOnShare, + }); + if (manualSkillPrimes.length > 0) { + const union = unionPrimeAllowedTools({ + primes: manualSkillPrimes, + agentToolNames: agent.tools ?? [], + }); + extraAllowedToolNames = union.extraToolNames; + perSkillExtras = union.perSkillExtras; + } + } + + const baseToolNames = agent.tools ?? []; + const requestedToolNames = + extraAllowedToolNames.length > 0 ? [...baseToolNames, ...extraAllowedToolNames] : baseToolNames; + + /** + * `loadTools` failures take two forms: + * 1. The wrapper throws — rare; only when something around the + * try/catch in `createToolLoader` itself fails. + * 2. The wrapper returns `undefined` — the typical CJS path: every + * production loader (`createToolLoader` in `initialize.js`, + * `openai.js`, `responses.js`) catches `loadAgentTools` errors and + * returns `undefined`. Without explicit handling, the empty + * fallback object below would silently drop the agent's baseline + * tools for the turn (not just the skill-added extras). + * + * If a skill-contributed `allowed-tools` entry is the culprit, retry + * with just `agent.tools` so the agent's own tools still load (the + * dropped-tools debug log below picks up which extras vanished). If + * the retry-without-extras also fails, propagate / fall through with + * the empty fallback — the agent's own tools are the problem. + */ + const callLoadTools = async (tools: string[]) => + loadTools?.({ + req, + res, + provider, + agentId: agent.id, + tools, + model: agent.model, + tool_options: agent.tool_options, + tool_resources, + }); + + let loadToolsResult; + const initialFailedSilently = (result: unknown) => + result == null && extraAllowedToolNames.length > 0; + try { + loadToolsResult = await callLoadTools(requestedToolNames); + } catch (err) { + if (extraAllowedToolNames.length > 0) { + logger.warn( + `[allowedTools] loadTools threw with skill-added extras [${extraAllowedToolNames.join(', ')}]; retrying without them:`, + err instanceof Error ? err.message : err, + ); + loadToolsResult = await callLoadTools(baseToolNames); + } else { + throw err; + } + } + if (initialFailedSilently(loadToolsResult)) { + /* Production loaders swallow errors and return undefined. Treat that + the same as a throw when extras were requested — the agent's own + tools must still load. */ + logger.warn( + `[allowedTools] loadTools returned no result with skill-added extras [${extraAllowedToolNames.join(', ')}]; retrying without them.`, + ); + loadToolsResult = await callLoadTools(baseToolNames); + } + const { toolRegistry, toolContextMap, @@ -368,16 +502,7 @@ export async function initializeAgent( hasDeferredTools, actionsEnabled, tools: structuredTools, - } = (await loadTools?.({ - req, - res, - provider, - agentId: agent.id, - tools: agent.tools ?? [], - model: agent.model, - tool_options: agent.tool_options, - tool_resources, - })) ?? { + } = loadToolsResult ?? { tools: [], toolContextMap: {}, userMCPAuthMap: undefined, @@ -389,6 +514,33 @@ export async function initializeAgent( let toolDefinitions = loadedToolDefinitions; + /** + * Tolerant filter: anything `loadTools` couldn't resolve (capability + * disabled, plugin missing, name unknown to the registry) is silently + * dropped with an attributed debug log. Cross-ecosystem skills authored + * against tools LibreChat hasn't shipped yet (Claude Code's `edit_file`, + * etc.) import without breaking — they light up automatically once + * support lands. Skips when there were no extras to begin with. + */ + if (extraAllowedToolNames.length > 0) { + const loadedNames = new Set((toolDefinitions ?? []).map((d) => d.name)); + const dropped = extraAllowedToolNames.filter((n) => !loadedNames.has(n)); + if (dropped.length > 0) { + const sources: string[] = []; + for (const [skillName, names] of perSkillExtras) { + const droppedFromSkill = names.filter((n) => !loadedNames.has(n)); + if (droppedFromSkill.length > 0) { + sources.push(`"${skillName}" → [${droppedFromSkill.join(', ')}]`); + } + } + logger.debug( + `[allowedTools] Dropped unrecognized tool names: ${ + sources.length > 0 ? sources.join('; ') : dropped.join(', ') + }`, + ); + } + } + const { getOptions, overrideProvider, customEndpointConfig } = getProviderConfig({ provider, appConfig: req.config, @@ -489,7 +641,6 @@ export async function initializeAgent( * LLM (or a direct-invocation path) names one. */ let executableSkillIds = params.accessibleSkillIds; - let manualSkillPrimes: ResolvedManualSkill[] | undefined; const { accessibleSkillIds } = params; if (accessibleSkillIds && accessibleSkillIds.length > 0) { const skillResult = await injectSkillCatalog({ @@ -507,25 +658,6 @@ export async function initializeAgent( toolDefinitions = skillResult.toolDefinitions; skillCount = skillResult.skillCount; executableSkillIds = skillResult.activeSkillIds; - - /** - * Resolve manually-invoked skills against the same ACL set used for the - * catalog. We use `accessibleSkillIds` (not `activeSkillIds`) because the - * catalog is capped at a token budget — a skill that fell outside that cap - * can still be authorized for direct manual invocation. The active-state - * filter is re-applied inside `resolveManualSkills` so deactivated skills - * never leak through this path either. - */ - if (params.manualSkills?.length && db.getSkillByName) { - manualSkillPrimes = await resolveManualSkills({ - names: params.manualSkills, - getSkillByName: db.getSkillByName, - accessibleSkillIds, - userId: req.user?.id, - skillStates: params.skillStates, - defaultActiveOnShare: params.defaultActiveOnShare, - }); - } } const agentMaxContextNum = Number(agentMaxContextTokens) || DEFAULT_MAX_CONTEXT_TOKENS; diff --git a/packages/api/src/agents/skillConfigurable.ts b/packages/api/src/agents/skillConfigurable.ts index e41012820a7b..de7e79008dd3 100644 --- a/packages/api/src/agents/skillConfigurable.ts +++ b/packages/api/src/agents/skillConfigurable.ts @@ -4,6 +4,21 @@ import { logger } from '@librechat/data-schemas'; /** * Augments a loadTools result with skill-specific configurable properties. * Loads the code API key and merges it with accessibleSkillIds and the request object. + * + * `manualSkillPrimedIdsByName` maps each manually-invoked skill name to + * the `_id` of the exact doc that was primed. Skill-tool handlers consult + * it to: + * 1. Relax the `disable-model-invocation` gate on `read_file` for + * manually-primed skills (so a `disable-model-invocation: true` + * skill the user invoked manually can still load its + * `references/*` / `scripts/*` files). + * 2. Constrain the lookup to the primed `_id` on same-name collisions, + * so `read_file` reads from the same doc whose body got primed + * (otherwise a newer same-name duplicate could shadow the + * resolver's pick and cause body/file mismatch within a single + * turn). + * Empty/missing → no exception, the gate applies as normal and the + * lookup uses the full ACL set. */ export async function enrichWithSkillConfigurable( result: { loadedTools: unknown[]; configurable?: Record }, @@ -15,6 +30,12 @@ export async function enrichWithSkillConfigurable( }) => Promise>, /** Pre-resolved code API key. When provided, loadAuthValues is skipped. */ preResolvedCodeApiKey?: string, + /** + * `{ [skillName]: skillIdString }` for skills the user manually invoked + * this turn (`$` popover). The id pins same-name collision lookups to + * the exact doc the resolver primed. + */ + manualSkillPrimedIdsByName?: Record, ): Promise<{ loadedTools: unknown[]; configurable: Record }> { let codeApiKey: string | undefined = preResolvedCodeApiKey; if (!codeApiKey) { @@ -38,6 +59,7 @@ export async function enrichWithSkillConfigurable( req, codeApiKey, accessibleSkillIds, + manualSkillPrimedIdsByName, }, }; } diff --git a/packages/api/src/agents/skills.ts b/packages/api/src/agents/skills.ts index 84d019e3a54c..1a6c642471e0 100644 --- a/packages/api/src/agents/skills.ts +++ b/packages/api/src/agents/skills.ts @@ -136,10 +136,13 @@ export interface InjectSkillCatalogResult { toolDefinitions: LCTool[] | undefined; skillCount: number; /** - * IDs of skills that passed the active-state filter and appear in the - * injected catalog. Runtime tool execution must authorize against this set, - * not the full `accessibleSkillIds`, so deactivated skills cannot be - * invoked by name even if the LLM hallucinates them. + * IDs of skills the runtime is authorized to resolve via `getSkillByName`. + * Includes `disable-model-invocation: true` skills even though they're + * absent from the catalog text — the skill-tool handler needs to be able + * to fetch the doc to fire the explicit "cannot be invoked by the model" + * rejection (instead of a generic "not found"). Per-user deactivated + * skills are still excluded — explicit user opt-out shouldn't be + * resolvable. */ activeSkillIds: Types.ObjectId[]; } @@ -181,11 +184,21 @@ export async function injectSkillCatalog( resolveSkillActive({ skill: s, skillStates, userId, defaultActiveOnShare }); const activeSkills: SkillSummary[] = []; + /** + * Catalog cap counts only model-visible (non-`disable-model-invocation`) + * skills. Counting against the merged active set would let a tenant + * with many disabled skills near the top of the cursor exhaust the + * 100-slot quota before any invocable skills got scanned — the catalog + * could end up empty even though invocable skills exist further down + * the paginated results. Tracking the visible count separately also + * keeps `MAX_CATALOG_PAGES` honest as a true scan-budget ceiling. + */ + let visibleCount = 0; let cursor: string | null = null; let pages = 0; let reachedEnd = false; - while (activeSkills.length < SKILL_CATALOG_LIMIT && pages < MAX_CATALOG_PAGES) { + while (visibleCount < SKILL_CATALOG_LIMIT && pages < MAX_CATALOG_PAGES) { const page = await listSkillsByAccess({ accessibleIds: accessibleSkillIds, limit: CATALOG_PAGE_SIZE, @@ -193,11 +206,23 @@ export async function injectSkillCatalog( }); for (const skill of page.skills) { - if (activeSkills.length >= SKILL_CATALOG_LIMIT) { + if (visibleCount >= SKILL_CATALOG_LIMIT) { break; } + /** + * Active set keeps `disable-model-invocation` skills so the runtime + * can still resolve them by name and the skill-tool handler can fire + * its explicit "cannot be invoked by the model" rejection (instead + * of a misleading "not found"). Catalog text formatting filters + * them back out below — they cost zero context tokens. Per-user + * deactivation (`isActive` false) is still a hard exclusion: the + * user explicitly opted out, so we honor it everywhere. + */ if (isActive(skill)) { activeSkills.push(skill); + if (skill.disableModelInvocation !== true) { + visibleCount += 1; + } } } @@ -213,15 +238,47 @@ export async function injectSkillCatalog( return { toolDefinitions: inputDefs, skillCount: 0, activeSkillIds: [] }; } - if (!reachedEnd && activeSkills.length < SKILL_CATALOG_LIMIT) { + if (!reachedEnd && visibleCount < SKILL_CATALOG_LIMIT) { logger.warn( `[injectSkillCatalog] Scanned ${MAX_CATALOG_PAGES} pages without filling the ${SKILL_CATALOG_LIMIT}-skill catalog. Some active skills may be excluded.`, ); } - // Warn on duplicate names — model may invoke the wrong skill + /** + * Catalog text and tool registration are gated on the *visible* subset. + * If every active skill is `disable-model-invocation: true`, the model + * can't reach any of them anyway — registering the skill tool would + * burn context tokens for nothing. + */ + const catalogVisibleSkills = activeSkills.filter((s) => s.disableModelInvocation !== true); + + /** + * Resolve same-name collisions in the runtime ACL set: when an invocable + * skill and a `disable-model-invocation: true` skill share a name, + * `getSkillByName` would pick whichever is newer (it sorts by + * `updatedAt` desc) — and if the disabled one is newer, EVERY model + * call to that name would fail with "cannot be invoked by the model" + * even though the cataloged invocable skill exists. Drop the disabled + * doc(s) from `activeSkillIds` whenever an invocable doc with the same + * name is also in scope. When ONLY a disabled doc exists for a name, + * keep it so the explicit-rejection error path still fires. + * + * Note: we never drop two invocable docs that share a name — the + * existing duplicate-name warn log below covers that ambiguity, and + * `getSkillByName` picks the newest (deterministic). + */ + const invocableNames = new Set(); + for (const s of catalogVisibleSkills) { + invocableNames.add(s.name); + } + const executableSkills = activeSkills.filter( + (s) => s.disableModelInvocation !== true || !invocableNames.has(s.name), + ); + + // Warn on duplicate names within the catalog-visible set — those are the + // ones the model can actually invoke ambiguously. const nameCount = new Map(); - for (const s of activeSkills) { + for (const s of catalogVisibleSkills) { nameCount.set(s.name, (nameCount.get(s.name) ?? 0) + 1); } for (const [dupName, count] of nameCount) { @@ -232,15 +289,26 @@ export async function injectSkillCatalog( } } - const catalog = formatSkillCatalog( - activeSkills.map((s) => ({ name: s.name, description: s.description })), - { contextWindowTokens: contextWindowTokens || 200_000 }, - ); - - if (catalog) { - agent.additional_instructions = agent.additional_instructions - ? `${agent.additional_instructions}\n\n${catalog}` - : catalog; + /** + * Catalog text is gated on the visible subset — `disable-model-invocation` + * skills cost zero context tokens. When no visible skills exist, the + * model gets no catalog and the `skill` tool is omitted from the + * registry (registering it would burn description tokens for a tool + * the model has no targets for). `read_file` and `bash_tool` are still + * registered though: manually-primed disabled skills can have their + * SKILL.md body in context referring to `references/*` and `scripts/*`, + * and those reads would otherwise be impossible. + */ + if (catalogVisibleSkills.length > 0) { + const catalog = formatSkillCatalog( + catalogVisibleSkills.map((s) => ({ name: s.name, description: s.description })), + { contextWindowTokens: contextWindowTokens || 200_000 }, + ); + if (catalog) { + agent.additional_instructions = agent.additional_instructions + ? `${agent.additional_instructions}\n\n${catalog}` + : catalog; + } } const skillToolDef: LCTool = { @@ -262,8 +330,17 @@ export async function injectSkillCatalog( parameters: BashExecutionToolDefinition.schema as unknown as LCTool['parameters'], }; - // Always register skill + read_file; only register bash_tool when code env is available - const defs: LCTool[] = [skillToolDef, readFileDef]; + /** + * `skill` tool is conditional on having anything for the model to invoke; + * `read_file` is always registered when any active skill is in scope + * (manually-primed disabled skills still need it); `bash_tool` follows + * code-env availability as before. + */ + const defs: LCTool[] = []; + if (catalogVisibleSkills.length > 0) { + defs.push(skillToolDef); + } + defs.push(readFileDef); if (codeEnvAvailable) { defs.push(bashToolDef); } @@ -277,8 +354,8 @@ export async function injectSkillCatalog( return { toolDefinitions, - skillCount: activeSkills.length, - activeSkillIds: activeSkills.map((s) => s._id), + skillCount: catalogVisibleSkills.length, + activeSkillIds: executableSkills.map((s) => s._id), }; } @@ -307,10 +384,17 @@ export function buildSkillPrimeMessage(skill: { name: string; body: string }): I export interface ResolveManualSkillsParams { /** Skill names the user invoked (via `$` popover or `always-apply`). */ names: string[]; - /** DB lookup: name → skill doc, constrained to ACL-accessible IDs. */ + /** DB lookup: name → skill doc, constrained to ACL-accessible IDs. + * + * Resolver always passes `options.preferUserInvocable: true` so a + * same-name newer `userInvocable: false` (model-only) duplicate can't + * shadow the older user-invocable doc the popover surfaced. Disable- + * model-invocation status is irrelevant for the manual path. + */ getSkillByName: ( name: string, accessibleIds: Types.ObjectId[], + options?: { preferUserInvocable?: boolean; preferModelInvocable?: boolean }, ) => Promise<{ _id: Types.ObjectId; name: string; @@ -323,6 +407,12 @@ export interface ResolveManualSkillsParams { * re-fetching the document. Populated by the DB method when available. */ allowedTools?: string[]; + /** + * When `false`, the skill author opted out of manual invocation. The + * resolver skips with a warn log rather than priming SKILL.md. + * Defaults to `true` when omitted. + */ + userInvocable?: boolean; } | null>; /** ACL-accessible skill IDs for this user (already scoped by `scopeSkillIds`). */ accessibleSkillIds: Types.ObjectId[]; @@ -335,6 +425,14 @@ export interface ResolveManualSkillsParams { } export interface ResolvedManualSkill { + /** + * `_id` of the exact doc that was primed. Plumbed to the runtime so the + * `read_file` handler can constrain its name lookup to this id and avoid + * resolving to a different same-name doc on collisions (which would + * cause the model's reads to hit files from a different skill than the + * body it sees). + */ + _id: Types.ObjectId; name: string; body: string; /** @@ -409,11 +507,41 @@ export async function resolveManualSkills( const resolved = await Promise.all( boundedNames.map(async (name) => { try { - const skill = await getSkillByName(name, accessibleSkillIds); + /* `preferUserInvocable` lets the lookup return the older + user-invocable variant when a newer same-name duplicate has + `userInvocable: false` (model-only). Without this, the + popover-visible skill the user picked would silently no-op + because `getSkillByName`'s `updatedAt desc` tiebreak returns + the model-only newer doc and the resolver skips it on the + userInvocable check below. We deliberately do NOT also pass + `preferModelInvocable` — manually invoking a `disable-model- + invocation: true` skill is the supported path (iter 4) and + the model-only filter would interfere with that. */ + const skill = await getSkillByName(name, accessibleSkillIds, { + preferUserInvocable: true, + }); if (!skill) { logger.warn(`[resolveManualSkills] Skill "${name}" not found or not accessible`); return null; } + /** + * `user-invocable: false` skills are model-only by author intent; + * the popover already hides them on the UI side, but an API-direct + * caller could still name one in `manualSkills`. Defense-in-depth: + * skip with a warn log so the contract holds at the runtime + * boundary too. Silent skip (not error) matches the established + * "not found / inactive / empty body" pattern — a single misfiring + * skill must never block the user's actual message from going out. + * + * Checked before the body check because `userInvocable` is an + * authoritative author decision, while empty body could be a + * transient state — surfacing the more specific cause helps + * operators triage faster. + */ + if (skill.userInvocable === false) { + logger.warn(`[resolveManualSkills] Skill "${name}" is not user-invocable — skipping`); + return null; + } if (!skill.body) { logger.warn(`[resolveManualSkills] Skill "${name}" has empty body — skipping`); return null; @@ -428,7 +556,11 @@ export async function resolveManualSkills( logger.warn(`[resolveManualSkills] Skill "${name}" is inactive for this user — skipping`); return null; } - const resolved: ResolvedManualSkill = { name: skill.name, body: skill.body }; + const resolved: ResolvedManualSkill = { + _id: skill._id, + name: skill.name, + body: skill.body, + }; if (skill.allowedTools !== undefined) { resolved.allowedTools = skill.allowedTools; } @@ -451,8 +583,13 @@ export interface InjectManualSkillPrimesParams { initialMessages: BaseMessage[]; /** Per-index token count map returned by `formatAgentMessages`. */ indexTokenCountMap: Record | undefined; - /** Resolved skill primes to splice in. */ - manualSkillPrimes: ResolvedManualSkill[]; + /** + * Resolved skill primes to splice in. Only `name` and `body` are used + * to construct the meta `HumanMessage`; widening the type to `Pick<...>` + * lets tests pass minimal `{ name, body }` literals without inventing + * `_id`s. The resolver always returns full primes in production. + */ + manualSkillPrimes: Pick[]; } export interface InjectManualSkillPrimesResult { @@ -574,7 +711,13 @@ export interface BuildSkillPrimeContentPartsParams { * scanner could skip it. */ export function buildSkillPrimeContentParts( - primes: ResolvedManualSkill[], + /** + * Only `name` is read here; widening the param type to `Pick<...>` lets + * callers (and tests) pass either the full `ResolvedManualSkill` or a + * minimal `{ name, body }` literal without needing to invent a + * placeholder `_id`. The resolver always returns full primes. + */ + primes: Pick[], { runId, startOffset = 0 }: BuildSkillPrimeContentPartsParams, ): SkillPrimeContentPart[] { return primes.map((prime, i) => { @@ -593,6 +736,93 @@ export function buildSkillPrimeContentParts( }); } +export interface SkillPrimeWithTools { + /** Skill name — used for log attribution only. */ + name: string; + /** Author-declared `allowed-tools` from frontmatter, if any. */ + allowedTools?: string[]; +} + +export interface UnionPrimeAllowedToolsParams { + /** + * Resolved skill primes (manual + always-apply once Phase 5 lands) that + * may carry an `allowedTools` allowlist. Order is irrelevant — the union + * is set-based — but stable iteration helps debug logs read consistently. + */ + primes: SkillPrimeWithTools[]; + /** Tool names already configured on the agent. Skipped when computing extras. */ + agentToolNames: string[]; +} + +export interface UnionPrimeAllowedToolsResult { + /** + * Tool names contributed by skill primes that aren't already on the + * agent. Caller is responsible for actually loading these and merging + * the resulting tool definitions into the agent's effective set; the + * helper itself stays pure so it can be unit-tested in isolation and + * reused for the always-apply path. + */ + extraToolNames: string[]; + /** + * Per-skill breakdown of which tool names that skill contributed to the + * final union. Useful for the debug log when the runtime drops names + * that the registry doesn't recognize — operators can see which skill + * asked for the missing tool. + */ + perSkillExtras: Map; +} + +/** + * Computes the union of `allowed-tools` declared by a turn's resolved skill + * primes, minus tools already configured on the agent. The agent-provided + * tools are the authoritative baseline; allowed-tools only ever ADDS to + * the surface, never replaces or removes. + * + * Tolerant of unknown tool names: validation against the runtime registry + * happens at the caller (in `initialize.ts`) so we can support skills + * authored against tools LibreChat hasn't implemented yet — the registry + * intersection silently drops them with a debug log, but the import path + * never rejects them. + * + * Pure function; returns set-style data so callers can dedupe across + * concurrent always-apply + manual paths without re-implementing the + * fold themselves. + */ +export function unionPrimeAllowedTools( + params: UnionPrimeAllowedToolsParams, +): UnionPrimeAllowedToolsResult { + const { primes, agentToolNames } = params; + const onAgent = new Set(agentToolNames); + const extras = new Set(); + const perSkill = new Map(); + + for (const prime of primes) { + const requested = prime.allowedTools; + if (!requested || requested.length === 0) { + continue; + } + const contributed: string[] = []; + for (const name of requested) { + if (typeof name !== 'string' || name.length === 0) { + continue; + } + if (onAgent.has(name) || extras.has(name)) { + continue; + } + extras.add(name); + contributed.push(name); + } + if (contributed.length > 0) { + perSkill.set(prime.name, contributed); + } + } + + return { + extraToolNames: Array.from(extras), + perSkillExtras: perSkill, + }; +} + /** * Safely pulls `manualSkills` from an untyped request body. * diff --git a/packages/api/src/skills/handlers.ts b/packages/api/src/skills/handlers.ts index ef7c1356db99..1f02e54ed41a 100644 --- a/packages/api/src/skills/handlers.ts +++ b/packages/api/src/skills/handlers.ts @@ -153,6 +153,9 @@ function serializeSkill( body: skill.body, frontmatter: serializeFrontmatter(skill.frontmatter), category: skill.category, + disableModelInvocation: skill.disableModelInvocation, + userInvocable: skill.userInvocable, + allowedTools: skill.allowedTools, author: skill.author.toString(), authorName: skill.authorName, version: skill.version, @@ -177,6 +180,9 @@ function serializeSkillSummary( displayTitle: skill.displayTitle, description: skill.description, category: skill.category, + disableModelInvocation: skill.disableModelInvocation, + userInvocable: skill.userInvocable, + allowedTools: skill.allowedTools, author: skill.author.toString(), authorName: skill.authorName, version: skill.version, diff --git a/packages/data-provider/src/types.ts b/packages/data-provider/src/types.ts index a142f720a216..72b17f0dcdcd 100644 --- a/packages/data-provider/src/types.ts +++ b/packages/data-provider/src/types.ts @@ -693,10 +693,10 @@ export type TBalanceResponse = { /* -------------------------------------------------------------------------- */ /** - * Trigger mode controlling whether an agent auto-invokes a skill, requires - * manual selection, or supports both. UI-only for phase 1 — the backend - * `TSkill` shape doesn't have `invocationMode` yet. Forms default to `auto` - * and the value is discarded on save until the backend lands support. + * @deprecated Superseded by the persisted `userInvocable` / + * `disableModelInvocation` pair derived from frontmatter. Retained for the + * transition window so older UI forms and tests still type-check; the + * backend no longer reads or writes it. */ export enum InvocationMode { auto = 'auto', diff --git a/packages/data-provider/src/types/skills.ts b/packages/data-provider/src/types/skills.ts index 12fdeacfe9b1..1721224e9648 100644 --- a/packages/data-provider/src/types/skills.ts +++ b/packages/data-provider/src/types/skills.ts @@ -104,11 +104,32 @@ export type TSkill = { frontmatter?: SkillFrontmatter; category?: string; /** - * UI-only phase 1. The backend doesn't persist `invocationMode` yet — - * forms default to `auto` and discard the value on save. Phase 2 will - * move this to a first-class column. + * @deprecated Replaced by the persisted `userInvocable` / + * `disableModelInvocation` pair derived from frontmatter. Retained + * temporarily so older form code that hasn't migrated still type-checks; + * the backend no longer reads or writes it. */ invocationMode?: import('../types').InvocationMode; + /** + * Mirrors the `disable-model-invocation` frontmatter field. `true` means + * the model can no longer invoke this skill via the `skill` tool and the + * skill is excluded from the catalog injected into the system prompt. + * Manual `$` invocation is unaffected. + */ + disableModelInvocation?: boolean; + /** + * Mirrors the `user-invocable` frontmatter field. `false` hides the skill + * from the `$` popover and rejects manual invocation. Defaults to `true`. + */ + userInvocable?: boolean; + /** + * Skill-declared tool allowlist (mirrors the `allowed-tools` frontmatter + * field). When the skill is invoked, these tools are unioned into the + * agent's effective tool set for the turn. Tolerant of unknown names — + * the runtime intersects against the loaded tool registry, so skills + * referencing yet-to-be-implemented tools import without breaking. + */ + allowedTools?: string[]; author: string; authorName: string; version: number; diff --git a/packages/data-schemas/src/methods/skill.spec.ts b/packages/data-schemas/src/methods/skill.spec.ts index 332a7ecd5e56..7fb8a65f07fa 100644 --- a/packages/data-schemas/src/methods/skill.spec.ts +++ b/packages/data-schemas/src/methods/skill.spec.ts @@ -14,6 +14,7 @@ import { validateSkillFrontmatter, validateRelativePath, inferSkillFileCategory, + deriveStructuredFrontmatterFields, } from './skill'; import { logger, createModels } from '..'; import { createMethods } from './index'; @@ -276,6 +277,73 @@ describe('skill validation helpers', () => { ).toBe(true); }); }); + + describe('deriveStructuredFrontmatterFields', () => { + it('returns empty object for missing or non-object frontmatter', () => { + expect(deriveStructuredFrontmatterFields(undefined)).toEqual({}); + expect(deriveStructuredFrontmatterFields(null as unknown as Record)).toEqual( + {}, + ); + }); + + it('extracts disable-model-invocation only when explicitly boolean', () => { + expect(deriveStructuredFrontmatterFields({ 'disable-model-invocation': true })).toEqual({ + disableModelInvocation: true, + }); + expect(deriveStructuredFrontmatterFields({ 'disable-model-invocation': false })).toEqual({ + disableModelInvocation: false, + }); + /* Non-boolean values are silently ignored — the validator already + rejects them upstream with INVALID_TYPE, so deriving here would + double-fire the failure. */ + expect(deriveStructuredFrontmatterFields({ 'disable-model-invocation': 'yes' })).toEqual({}); + }); + + it('extracts user-invocable only when explicitly boolean', () => { + expect(deriveStructuredFrontmatterFields({ 'user-invocable': true })).toEqual({ + userInvocable: true, + }); + expect(deriveStructuredFrontmatterFields({ 'user-invocable': false })).toEqual({ + userInvocable: false, + }); + /* Field absent → no override; downstream uses schema default of true. */ + expect(deriveStructuredFrontmatterFields({})).toEqual({}); + }); + + it('normalizes a string allowed-tools to a one-element array', () => { + expect(deriveStructuredFrontmatterFields({ 'allowed-tools': 'web_search' })).toEqual({ + allowedTools: ['web_search'], + }); + /* Empty string → not extracted; an explicit empty array is the + author's way to say "no extras". */ + expect(deriveStructuredFrontmatterFields({ 'allowed-tools': '' })).toEqual({}); + }); + + it('passes through array allowed-tools, dropping non-string entries', () => { + expect( + deriveStructuredFrontmatterFields({ + 'allowed-tools': ['execute_code', 'read_file', '', 42 as unknown as string, null], + }), + ).toEqual({ allowedTools: ['execute_code', 'read_file'] }); + expect(deriveStructuredFrontmatterFields({ 'allowed-tools': [] })).toEqual({ + allowedTools: [], + }); + }); + + it('combines all three fields when present together', () => { + expect( + deriveStructuredFrontmatterFields({ + 'disable-model-invocation': true, + 'user-invocable': false, + 'allowed-tools': ['execute_code'], + }), + ).toEqual({ + disableModelInvocation: true, + userInvocable: false, + allowedTools: ['execute_code'], + }); + }); + }); }); describe('Skill CRUD methods', () => { @@ -426,6 +494,365 @@ describe('Skill CRUD methods', () => { expect(page2.has_more).toBe(false); }); + it('persists disableModelInvocation / userInvocable / allowedTools derived from frontmatter', async () => { + const { skill } = await methods.createSkill( + makeSkillInput({ + name: 'frontmatter-derived', + frontmatter: { + name: 'frontmatter-derived', + description: 'A demo skill used in tests.', + 'disable-model-invocation': true, + 'user-invocable': false, + 'allowed-tools': ['execute_code', 'read_file'], + }, + }), + ); + expect(skill.disableModelInvocation).toBe(true); + expect(skill.userInvocable).toBe(false); + expect(skill.allowedTools).toEqual(['execute_code', 'read_file']); + + /* Re-fetch via getSkillById to confirm the columns survive the round-trip + (proving they're persisted, not just echoed in the create response). */ + const reloaded = await methods.getSkillById(skill._id); + expect(reloaded?.disableModelInvocation).toBe(true); + expect(reloaded?.userInvocable).toBe(false); + expect(reloaded?.allowedTools).toEqual(['execute_code', 'read_file']); + }); + + it('defaults disableModelInvocation=false / userInvocable=true / allowedTools=undefined when frontmatter omits them', async () => { + const { skill } = await methods.createSkill(makeSkillInput({ name: 'defaults' })); + expect(skill.disableModelInvocation).toBe(false); + expect(skill.userInvocable).toBe(true); + expect(skill.allowedTools).toBeUndefined(); + }); + + it('updateSkill re-derives the structured columns when frontmatter changes', async () => { + const { skill } = await methods.createSkill( + makeSkillInput({ + name: 'mutating', + frontmatter: { + name: 'mutating', + description: 'A demo skill used in tests.', + 'disable-model-invocation': true, + 'user-invocable': false, + 'allowed-tools': ['execute_code'], + }, + }), + ); + expect(skill.disableModelInvocation).toBe(true); + + const updated = await methods.updateSkill({ + id: skill._id.toString(), + expectedVersion: 1, + update: { + frontmatter: { + name: 'mutating', + description: 'A demo skill used in tests.', + /* Drops disable-model-invocation, allowed-tools; flips user-invocable. */ + 'user-invocable': true, + }, + }, + }); + expect(updated.status).toBe('updated'); + if (updated.status !== 'updated') return; + /* Fields the new frontmatter omits are `$unset` so a re-fetch returns + undefined. Functionally equivalent to the schema default for all three: + runtime catalog filter checks `disableModelInvocation === true` (so + undefined passes through), `resolveManualSkills` checks + `userInvocable === false` (undefined passes through), and the tool- + union helper treats undefined `allowedTools` as "no extras". */ + expect(updated.skill.disableModelInvocation).toBeUndefined(); + expect(updated.skill.userInvocable).toBe(true); + expect(updated.skill.allowedTools).toBeUndefined(); + }); + + it('backfills legacy skills from frontmatter when columns are unset (getSkillByName)', async () => { + /* Simulate a pre-Phase-6 skill: it has `user-invocable` / + `disable-model-invocation` / `allowed-tools` set in frontmatter + but the structured columns were never populated (no migration). */ + const legacy = await Skill.create({ + name: 'legacy-skill', + description: 'A legacy skill from before Phase 6.', + body: 'body', + frontmatter: { + name: 'legacy-skill', + description: 'A legacy skill from before Phase 6.', + 'disable-model-invocation': true, + 'user-invocable': false, + 'allowed-tools': ['execute_code'], + }, + author: owner._id, + authorName: owner.name ?? 'Skill Owner', + version: 1, + source: 'inline', + fileCount: 0, + }); + /* Strip the columns to simulate pre-Phase-6 state — `Skill.create` + above would have run the new derive helper, so we explicitly unset + to recreate the legacy shape. */ + await Skill.collection.updateOne( + { _id: legacy._id }, + { $unset: { disableModelInvocation: '', userInvocable: '', allowedTools: '' } }, + ); + + const fetched = await methods.getSkillByName('legacy-skill', [ + legacy._id as mongoose.Types.ObjectId, + ]); + /* Backfill must populate the columns from frontmatter so runtime + gates fire correctly without a write migration. */ + expect(fetched?.disableModelInvocation).toBe(true); + expect(fetched?.userInvocable).toBe(false); + expect(fetched?.allowedTools).toEqual(['execute_code']); + }); + + it('backfills legacy skills from frontmatter on listSkillsByAccess summaries', async () => { + /* Same legacy-shape simulation; the summary projection now includes + frontmatter so the backfill helper has data to read. */ + const legacy = await Skill.create({ + name: 'legacy-summary', + description: 'A legacy skill listed in summaries.', + body: 'body', + frontmatter: { + name: 'legacy-summary', + description: 'A legacy skill listed in summaries.', + 'disable-model-invocation': true, + 'user-invocable': false, + }, + author: owner._id, + authorName: owner.name ?? 'Skill Owner', + version: 1, + source: 'inline', + fileCount: 0, + }); + await Skill.collection.updateOne( + { _id: legacy._id }, + { $unset: { disableModelInvocation: '', userInvocable: '' } }, + ); + + const result = await methods.listSkillsByAccess({ + accessibleIds: [legacy._id as mongoose.Types.ObjectId], + limit: 10, + }); + expect(result.skills.length).toBe(1); + expect(result.skills[0].disableModelInvocation).toBe(true); + expect(result.skills[0].userInvocable).toBe(false); + }); + + it('preferModelInvocable picks the model-invocable doc on same-name collision (does NOT filter on userInvocable)', async () => { + /* Same-name collision scenario the model paths must handle: an older + user-invocable variant and a newer model-only variant + (`userInvocable: false`) — both are model-invocable. The model + path uses preferModelInvocable, which deliberately does NOT + filter on userInvocable; otherwise the older doc would shadow the + cataloged model-only doc the model targeted. */ + const olderUserInvocable = await Skill.create({ + name: 'model-collision', + description: 'Older user-invocable variant of the colliding name.', + body: 'user-invocable body', + frontmatter: {}, + author: owner._id, + authorName: owner.name ?? 'Skill Owner', + version: 1, + source: 'inline', + fileCount: 0, + userInvocable: true, + }); + const newerModelOnly = await Skill.create({ + name: 'model-collision', + description: 'Newer model-only variant of the colliding name.', + body: 'model-only body', + frontmatter: {}, + author: other._id, + authorName: other.name ?? 'Other', + version: 1, + source: 'inline', + fileCount: 0, + userInvocable: false, + }); + /* Set updatedAt explicitly so the "older / newer" assertion is + deterministic across CI runners — relying on wall-clock spacing + between two `Skill.create` calls would race on a fast machine. */ + await Skill.collection.updateOne( + { _id: olderUserInvocable._id }, + { $set: { updatedAt: new Date(Date.now() - 1000) } }, + ); + + const accessibleIds = [ + olderUserInvocable._id as mongoose.Types.ObjectId, + newerModelOnly._id as mongoose.Types.ObjectId, + ]; + + /* preferModelInvocable: both are model-invocable (neither has + disableModelInvocation: true), so newest wins → model-only doc. */ + const modelLookup = await methods.getSkillByName('model-collision', accessibleIds, { + preferModelInvocable: true, + }); + expect(modelLookup?._id.toString()).toBe(newerModelOnly._id.toString()); + }); + + it('preferUserInvocable picks the user-invocable doc on same-name collision (does NOT filter on disableModelInvocation)', async () => { + /* Manual path scenario: older user-invocable variant + newer + model-only variant. The popover surfaced the older doc; the + resolver must look it up with preferUserInvocable so the + newer model-only doc doesn't shadow the user's selection. */ + const olderUserInvocable = await Skill.create({ + name: 'user-collision', + description: 'Older user-invocable variant.', + body: 'user-invocable body', + frontmatter: {}, + author: owner._id, + authorName: owner.name ?? 'Skill Owner', + version: 1, + source: 'inline', + fileCount: 0, + userInvocable: true, + }); + const newerModelOnly = await Skill.create({ + name: 'user-collision', + description: 'Newer model-only variant.', + body: 'model-only body', + frontmatter: {}, + author: other._id, + authorName: other.name ?? 'Other', + version: 1, + source: 'inline', + fileCount: 0, + userInvocable: false, + }); + /* Deterministic ordering — see the model-collision test above. */ + await Skill.collection.updateOne( + { _id: olderUserInvocable._id }, + { $set: { updatedAt: new Date(Date.now() - 1000) } }, + ); + + const accessibleIds = [ + olderUserInvocable._id as mongoose.Types.ObjectId, + newerModelOnly._id as mongoose.Types.ObjectId, + ]; + + /* Default behavior: newest wins → model-only doc returned. */ + const defaultLookup = await methods.getSkillByName('user-collision', accessibleIds); + expect(defaultLookup?._id.toString()).toBe(newerModelOnly._id.toString()); + + /* preferUserInvocable: user-invocable doc wins → older doc returned. + Disabled-model status is irrelevant here. */ + const preferred = await methods.getSkillByName('user-collision', accessibleIds, { + preferUserInvocable: true, + }); + expect(preferred?._id.toString()).toBe(olderUserInvocable._id.toString()); + }); + + it('preferUserInvocable still returns disabled docs (manual prime of disabled is supported)', async () => { + /* Manual path with a disabled-but-user-invocable skill: the user + can still pick it from the popover; resolveManualSkills uses + preferUserInvocable, which doesn't filter on + disableModelInvocation. */ + const disabled = await Skill.create({ + name: 'disabled-user-invocable', + description: 'User-invocable but model-disabled.', + body: 'body', + frontmatter: {}, + author: owner._id, + authorName: owner.name ?? 'Skill Owner', + version: 1, + source: 'inline', + fileCount: 0, + userInvocable: true, + disableModelInvocation: true, + }); + + const result = await methods.getSkillByName( + 'disabled-user-invocable', + [disabled._id as mongoose.Types.ObjectId], + { preferUserInvocable: true }, + ); + expect(result?._id.toString()).toBe(disabled._id.toString()); + expect(result?.disableModelInvocation).toBe(true); + }); + + it('preferred lookups fall back to the newest match when no preferred doc exists', async () => { + /* Sole-disabled name: only a disable-model-invocation doc exists. + preferModelInvocable must still return it so handleSkillToolCall + can fire the explicit-rejection error path. */ + const disabledOnly = await Skill.create({ + name: 'sole-disabled-fallback', + description: 'Only a model-disabled variant of this name exists.', + body: 'disabled body', + frontmatter: {}, + author: owner._id, + authorName: owner.name ?? 'Skill Owner', + version: 1, + source: 'inline', + fileCount: 0, + disableModelInvocation: true, + }); + + const result = await methods.getSkillByName( + 'sole-disabled-fallback', + [disabledOnly._id as mongoose.Types.ObjectId], + { preferModelInvocable: true }, + ); + expect(result?._id.toString()).toBe(disabledOnly._id.toString()); + expect(result?.disableModelInvocation).toBe(true); + }); + + it('does not overwrite explicit columns with frontmatter values (column wins)', async () => { + /* If both column and frontmatter are set (e.g. an admin edited the + column directly via the API but the frontmatter still says + otherwise), the column is authoritative — backfill only fills + undefined fields. */ + const skill = await Skill.create({ + name: 'column-wins', + description: 'A demo skill used in tests.', + body: 'body', + frontmatter: { + name: 'column-wins', + description: 'A demo skill used in tests.', + 'disable-model-invocation': true, + 'user-invocable': false, + }, + author: owner._id, + authorName: owner.name ?? 'Skill Owner', + version: 1, + source: 'inline', + fileCount: 0, + disableModelInvocation: false, + userInvocable: true, + }); + + const fetched = await methods.getSkillByName('column-wins', [ + skill._id as mongoose.Types.ObjectId, + ]); + expect(fetched?.disableModelInvocation).toBe(false); + expect(fetched?.userInvocable).toBe(true); + }); + + it('listSkillsByAccess returns disableModelInvocation / userInvocable / allowedTools on summary rows', async () => { + const { skill } = await methods.createSkill( + makeSkillInput({ + name: 'list-summary', + frontmatter: { + name: 'list-summary', + description: 'A demo skill used in tests.', + 'disable-model-invocation': true, + 'user-invocable': false, + 'allowed-tools': ['web_search'], + }, + }), + ); + const result = await methods.listSkillsByAccess({ + accessibleIds: [skill._id], + limit: 10, + }); + expect(result.skills.length).toBe(1); + /* These are the fields injectSkillCatalog filters on; if they're + missing from the projection, the catalog can't enforce + disableModelInvocation. */ + expect(result.skills[0].disableModelInvocation).toBe(true); + expect(result.skills[0].userInvocable).toBe(false); + expect(result.skills[0].allowedTools).toEqual(['web_search']); + }); + it('listSkillsByAccess filters by category and search', async () => { const { skill: a } = await methods.createSkill( makeSkillInput({ name: 'alpha-skill', category: 'tools' }), diff --git a/packages/data-schemas/src/methods/skill.ts b/packages/data-schemas/src/methods/skill.ts index a857933f4300..2b425fe2b295 100644 --- a/packages/data-schemas/src/methods/skill.ts +++ b/packages/data-schemas/src/methods/skill.ts @@ -449,6 +449,99 @@ export type UpdateSkillInput = { category?: string; }; +/** + * Maps the runtime-enforced frontmatter fields onto their first-class + * column equivalents. Returns only the keys that were explicitly set on the + * frontmatter so callers can decide whether to write `undefined` (skip the + * `$set`) versus a concrete value. + * + * `allowed-tools` accepts string or string[] per the validator; both are + * normalized to an array. Empty strings are filtered out so a stray comma + * in YAML doesn't leak through as `''`. + */ +export function deriveStructuredFrontmatterFields( + frontmatter: Record | undefined, +): { + disableModelInvocation?: boolean; + userInvocable?: boolean; + allowedTools?: string[]; +} { + if (!frontmatter || typeof frontmatter !== 'object') { + return {}; + } + const derived: { + disableModelInvocation?: boolean; + userInvocable?: boolean; + allowedTools?: string[]; + } = {}; + const disableModelInvocationRaw = frontmatter['disable-model-invocation']; + if (typeof disableModelInvocationRaw === 'boolean') { + derived.disableModelInvocation = disableModelInvocationRaw; + } + const userInvocableRaw = frontmatter['user-invocable']; + if (typeof userInvocableRaw === 'boolean') { + derived.userInvocable = userInvocableRaw; + } + const allowedToolsRaw = frontmatter['allowed-tools']; + if (typeof allowedToolsRaw === 'string') { + /** + * YAML scalars like `allowed-tools: web_search` are parsed as a single + * string. Wrap into a one-element array; we deliberately do NOT split + * on commas — the validator already accepts string-array form and + * trying to "be helpful" by splitting `"web_search, file_search"` + * would silently invent semantics the spec doesn't promise. + */ + if (allowedToolsRaw.length > 0) { + derived.allowedTools = [allowedToolsRaw]; + } + } else if (Array.isArray(allowedToolsRaw)) { + derived.allowedTools = allowedToolsRaw.filter( + (entry): entry is string => typeof entry === 'string' && entry.length > 0, + ); + } + return derived; +} + +/** + * Read-time fallback for skills authored before the structured columns + * existed: if the column is unset but the matching key is present in + * `frontmatter`, fill the column in on the returned object so downstream + * runtime checks (`skill.userInvocable === false`, + * `skill.disableModelInvocation === true`, `skill.allowedTools`) behave + * the same way they would for a freshly-created skill. + * + * Side-effect-free w.r.t. the DB (no writes), but mutates its argument + * in place and returns the same reference. Callers passing a `lean()` + * doc this is fine — the doc is a fresh JS object owned by the caller. + * When the skill is next updated, `updateSkill` re-derives and persists + * the columns naturally, so this fallback gradually becomes a no-op. + * + * Skills with the columns already populated short-circuit to no-op. + */ +export function backfillDerivedFromFrontmatter< + T extends { + frontmatter?: Record; + disableModelInvocation?: boolean; + userInvocable?: boolean; + allowedTools?: string[]; + }, +>(skill: T | null): T | null { + if (!skill || !skill.frontmatter) { + return skill; + } + const derived = deriveStructuredFrontmatterFields(skill.frontmatter); + if (skill.disableModelInvocation === undefined && derived.disableModelInvocation !== undefined) { + skill.disableModelInvocation = derived.disableModelInvocation; + } + if (skill.userInvocable === undefined && derived.userInvocable !== undefined) { + skill.userInvocable = derived.userInvocable; + } + if (skill.allowedTools === undefined && derived.allowedTools !== undefined) { + skill.allowedTools = derived.allowedTools; + } + return skill; +} + export type UpsertSkillFileInput = { skillId: Types.ObjectId | string; relativePath: string; @@ -582,6 +675,7 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk throw error; } + const derived = deriveStructuredFrontmatterFields(data.frontmatter); const doc = await Skill.create({ name: data.name, displayTitle: data.displayTitle, @@ -596,6 +690,7 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk sourceMetadata: data.sourceMetadata, fileCount: 0, tenantId: data.tenantId, + ...derived, }); return { skill: doc.toObject() as unknown as ISkill & { _id: Types.ObjectId }, @@ -617,13 +712,66 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk async function getSkillByName( name: string, accessibleIds: Types.ObjectId[], + options?: { + /** + * Manual paths (`$` popover, always-apply once Phase 5 lands) set + * this so a same-name newer `userInvocable: false` duplicate can't + * shadow the older user-invocable doc the popover surfaced. + * Disable-model-invocation status is irrelevant here — manually- + * primed disabled skills are explicitly supported (iter 4). + */ + preferUserInvocable?: boolean; + /** + * Model paths (`skill` / `read_file` tool handlers) set this so a + * same-name newer `disable-model-invocation: true` duplicate can't + * shadow the cataloged model-invocable doc. User-invocability is + * irrelevant here — `userInvocable: false` skills are model-only + * and remain valid model-invocation targets. + * + * Both flags fall back to the newest match when no preferred doc + * exists, so handlers can still fire their explicit-rejection + * error paths (e.g. "cannot be invoked by the model" in the + * disabled-only case). + */ + preferModelInvocable?: boolean; + }, ): Promise<(ISkill & { _id: Types.ObjectId }) | null> { const Skill = mongoose.models.Skill as Model; - // sort by updatedAt desc for deterministic result when multiple skills share a name - const doc = await Skill.findOne({ name, _id: { $in: accessibleIds } }) + const preferUserInvocable = options?.preferUserInvocable === true; + const preferModelInvocable = options?.preferModelInvocable === true; + /* Single-doc fast path when no preference is requested — preserves + the previous performance characteristics for callers that just + want "newest match". */ + if (!preferUserInvocable && !preferModelInvocable) { + const doc = await Skill.findOne({ name, _id: { $in: accessibleIds } }) + .sort({ updatedAt: -1 }) + .lean(); + return backfillDerivedFromFrontmatter( + (doc as unknown as (ISkill & { _id: Types.ObjectId }) | null) ?? null, + ); + } + /* Multi-doc path: fetch all matching docs (typically 1, rarely 2+ + across same-name duplicates) and pick the first satisfying the + caller's preference; fall back to newest. */ + const docs = (await Skill.find({ name, _id: { $in: accessibleIds } }) .sort({ updatedAt: -1 }) - .lean(); - return (doc as unknown as (ISkill & { _id: Types.ObjectId }) | null) ?? null; + .lean()) as unknown as Array; + if (docs.length === 0) { + return null; + } + for (const doc of docs) { + backfillDerivedFromFrontmatter(doc); + } + const preferred = docs.find((d) => { + if (preferUserInvocable && d.userInvocable === false) { + return false; + } + if (preferModelInvocable && d.disableModelInvocation === true) { + return false; + } + return true; + }); + return preferred ?? docs[0]; } async function listSkillsByAccess( @@ -649,11 +797,28 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk const rows = await Skill.find(filter) .sort({ updatedAt: -1, _id: 1 }) .limit(limit + 1) + /* `frontmatter` is included so `backfillDerivedFromFrontmatter` can + restore the runtime fields for skills authored before Phase 6 + landed the columns. Body is still excluded — the size win was + body, not frontmatter (which is bounded by the validator). + TODO(post-backfill): once a write migration backfills all + pre-Phase-6 skills' columns from frontmatter (or after a + deployment window long enough that any active skill has been + re-saved), drop `frontmatter` from this projection. ~2KB/skill + × 100/page is wasted bandwidth on every list call once backfill + is no longer needed. */ .select( - 'name displayTitle description category author authorName version source sourceMetadata fileCount tenantId createdAt updatedAt', + 'name displayTitle description category author authorName version source sourceMetadata fileCount tenantId disableModelInvocation userInvocable allowedTools frontmatter createdAt updatedAt', ) .lean(); + /* Read-time fallback: pre-Phase-6 skills with `user-invocable` / + `disable-model-invocation` only in frontmatter (no derived column) + must still be filtered correctly by the catalog and the popover. */ + for (const row of rows) { + backfillDerivedFromFrontmatter(row as unknown as ISkill); + } + const has_more = rows.length > limit; const sliced = has_more ? rows.slice(0, limit) : rows; const last = sliced[sliced.length - 1]; @@ -701,16 +866,40 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk const Skill = mongoose.models.Skill as Model; const setPayload: Record = {}; + const unsetPayload: Record = {}; if (update.name !== undefined) setPayload.name = update.name; if (update.displayTitle !== undefined) setPayload.displayTitle = update.displayTitle; if (update.description !== undefined) setPayload.description = update.description; if (update.body !== undefined) setPayload.body = update.body; - if (update.frontmatter !== undefined) setPayload.frontmatter = update.frontmatter; + if (update.frontmatter !== undefined) { + setPayload.frontmatter = update.frontmatter; + /** + * Derived columns track frontmatter — when frontmatter changes, the + * derived view must follow. Fields the new frontmatter omits are + * unset (back to schema default) so removing `disable-model-invocation` + * from a SKILL.md re-enables model invocation on the next save. + */ + const derived = deriveStructuredFrontmatterFields(update.frontmatter); + for (const key of ['disableModelInvocation', 'userInvocable', 'allowedTools'] as const) { + if (derived[key] !== undefined) { + setPayload[key] = derived[key]; + } else { + unsetPayload[key] = ''; + } + } + } if (update.category !== undefined) setPayload.category = update.category; + const updateOps: Record = { + $set: setPayload, + $inc: { version: 1 }, + }; + if (Object.keys(unsetPayload).length > 0) { + updateOps.$unset = unsetPayload; + } const result = await Skill.findOneAndUpdate( { _id: new ObjectId(id), version: expectedVersion }, - { $set: setPayload, $inc: { version: 1 } }, + updateOps, { new: true }, ).lean(); diff --git a/packages/data-schemas/src/schema/skill.ts b/packages/data-schemas/src/schema/skill.ts index 219bd291e48f..31a39a59b678 100644 --- a/packages/data-schemas/src/schema/skill.ts +++ b/packages/data-schemas/src/schema/skill.ts @@ -116,6 +116,49 @@ const skillSchema: Schema = new Schema( type: Schema.Types.Mixed, default: {}, }, + /** + * When `true`, the model cannot invoke this skill via the `skill` tool + * and the skill is excluded from the catalog injected into the agent's + * additional_instructions. Manual `$` invocation is unaffected. Mirrors + * the `disable-model-invocation` frontmatter field. Filtering currently + * happens application-side after `listSkillsByAccess` returns, so no + * index — add one if/when a query starts filtering by this column at + * the DB level. + */ + disableModelInvocation: { + type: Boolean, + default: false, + }, + /** + * When `false`, the skill is hidden from the `$` popover and rejected + * by the manual-invocation resolver. Defaults to `true` so existing + * skills remain user-invocable without a migration. Mirrors the + * `user-invocable` frontmatter field. + */ + userInvocable: { + type: Boolean, + default: true, + }, + /** + * Skill-declared tool allowlist forwarded verbatim from frontmatter. + * Surfaced on resolved skill primes so the agent's effective tool set + * for the turn can union these in alongside the agent-configured tools. + * `default: undefined` (not `[]`) preserves the distinction between + * "author declared no extras" and "author explicitly declared none". + * + * Phase 6 wires this in for **manually-primed** skills (Phase 5's + * always-apply primes will pass through the same union once that + * resolver lands). Model-invoked skills (via the `skill` tool + * mid-turn) do NOT trigger tool union at execution time — adding a + * tool partway through a turn would require a graph rebuild that + * isn't worth the complexity for v1. If the author wants the model + * to have access to extra tools when the model picks the skill, they + * should add those tools to the agent directly. + */ + allowedTools: { + type: [String], + default: undefined, + }, category: { type: String, default: '', diff --git a/packages/data-schemas/src/types/skill.ts b/packages/data-schemas/src/types/skill.ts index d1582e5314ee..c240abdb3664 100644 --- a/packages/data-schemas/src/types/skill.ts +++ b/packages/data-schemas/src/types/skill.ts @@ -37,6 +37,32 @@ export interface ISkill { * are rejected so expanding the allowed set is an intentional code change. */ frontmatter: Record; + /** + * Mirrors the `disable-model-invocation` frontmatter field. `true` removes + * the skill from the model's catalog and rejects model-side `skill` tool + * calls; manual `$` invocation is unaffected. Defaults to `false`. + */ + disableModelInvocation?: boolean; + /** + * Mirrors the `user-invocable` frontmatter field. `false` hides the skill + * from the `$` popover and rejects manual invocation. Defaults to `true`. + */ + userInvocable?: boolean; + /** + * Skill-declared tool allowlist (mirrors the `allowed-tools` frontmatter + * field). When the skill is invoked **manually** (via `$` popover, or + * always-apply once Phase 5 lands), these tools are unioned into the + * agent's effective tool set for the turn. Tolerant of unknown names — + * the runtime intersects against the loaded tool registry and silently + * drops anything missing, so cross-ecosystem skills authored against + * unimplemented tools import without breaking. + * + * Note: model-invoked skills (via the `skill` tool mid-turn) do NOT + * trigger tool union at execution time — adding tools after the graph + * has started would require a rebuild. Agents that need a tool when + * the model picks a skill should add it to `agent.tools` directly. + */ + allowedTools?: string[]; category?: string; author: Types.ObjectId; authorName: string;