diff --git a/api/server/routes/__tests__/config.spec.js b/api/server/routes/__tests__/config.spec.js index 606b4ae8a1..e0fc200486 100644 --- a/api/server/routes/__tests__/config.spec.js +++ b/api/server/routes/__tests__/config.spec.js @@ -327,6 +327,7 @@ describe('GET /api/config', () => { { name: 'guarded-spec', label: 'Guarded Spec', + skills: ['private-skill'], preset: { endpoint: 'openAI', model: 'gpt-4o', @@ -352,6 +353,7 @@ describe('GET /api/config', () => { model: 'gpt-4o', greeting: 'Hello', }); + expect(response.body.modelSpecs.list[0]).not.toHaveProperty('skills'); }); it('should include full interface config', async () => { diff --git a/api/server/services/Endpoints/agents/addedConvo.js b/api/server/services/Endpoints/agents/addedConvo.js index 2a2cd9ca30..847c6c01af 100644 --- a/api/server/services/Endpoints/agents/addedConvo.js +++ b/api/server/services/Endpoints/agents/addedConvo.js @@ -3,10 +3,14 @@ const { ADDED_AGENT_ID, initializeAgent, validateAgentModel, + resolveAgentScopedSkillIds, + resolveModelSpecSkillIds, loadAddedAgent: loadAddedAgentFn, } = require('@librechat/api'); +const { isEphemeralAgentId } = require('librechat-data-provider'); const { filterFilesByAgentAccess } = require('~/server/services/Files/permissions'); const { getMCPServerTools } = require('~/server/services/Config'); +const { canAuthorSkillFiles } = require('./skillDeps'); const db = require('~/models'); const loadAddedAgent = (params) => @@ -40,6 +44,13 @@ const loadAddedAgent = (params) => * @param {Map} params.agentConfigs - Map of agent configs to add to * @param {string} params.primaryAgentId - The primary agent ID * @param {Object|undefined} params.userMCPAuthMap - User MCP auth map to merge into + * @param {Array} [params.accessibleSkillIds] - Full VIEW-accessible skill IDs for the user + * @param {Array} [params.editableSkillIds] - Full EDIT-accessible skill IDs for the user + * @param {boolean} [params.skillsCapabilityEnabled] - Whether endpoint Skills are enabled + * @param {boolean} [params.ephemeralSkillsToggle] - Per-request ephemeral Skills badge state + * @param {boolean} [params.skillCreateAllowed] - Whether the user can create Skills + * @param {Record} [params.skillStates] - Per-user Skill active overrides + * @param {boolean} [params.defaultActiveOnShare] - Default active state for shared Skills * @param {boolean} [params.codeEnvAvailable] - `execute_code` capability flag; * forwarded verbatim to the added agent's `initializeAgent`. @see * InitializeAgentParams.codeEnvAvailable for full semantics. @@ -60,6 +71,13 @@ const processAddedConvo = async ({ primaryAgentId, primaryAgent, userMCPAuthMap, + accessibleSkillIds = [], + editableSkillIds = [], + skillsCapabilityEnabled = false, + ephemeralSkillsToggle = false, + skillCreateAllowed = false, + skillStates, + defaultActiveOnShare, codeEnvAvailable, }) => { const addedConvo = endpointOption.addedConvo; @@ -94,6 +112,47 @@ const processAddedConvo = async ({ return { userMCPAuthMap }; } + const selectedModelSpec = + addedConvo.spec && Array.isArray(req.config?.modelSpecs?.list) + ? req.config.modelSpecs.list.find((modelSpec) => modelSpec.name === addedConvo.spec) + : null; + + if ( + addedAgent && + isEphemeralAgentId(addedAgent.id) && + selectedModelSpec && + Object.hasOwn(selectedModelSpec, 'skills') + ) { + if (selectedModelSpec.skills === true) { + addedAgent.skills_enabled = true; + delete addedAgent.skills; + } else if (selectedModelSpec.skills === false) { + addedAgent.skills_enabled = false; + addedAgent.skills = []; + } else if (Array.isArray(selectedModelSpec.skills)) { + const resolvedSkillIds = await resolveModelSpecSkillIds({ + names: selectedModelSpec.skills, + accessibleSkillIds, + getSkillByName: db.getSkillByName, + }); + addedAgent.skills_enabled = true; + addedAgent.skills = resolvedSkillIds.map((id) => id.toString()); + } + } + + const scopedSkillIds = resolveAgentScopedSkillIds({ + agent: addedAgent, + accessibleSkillIds, + skillsCapabilityEnabled, + ephemeralSkillsToggle, + }); + const scopedEditableSkillIds = resolveAgentScopedSkillIds({ + agent: addedAgent, + accessibleSkillIds: editableSkillIds, + skillsCapabilityEnabled, + ephemeralSkillsToggle, + }); + const addedConfig = await initializeAgent( { req, @@ -105,7 +164,17 @@ const processAddedConvo = async ({ agent: addedAgent, endpointOption, allowedProviders, + accessibleSkillIds: scopedSkillIds, + skillAuthoringAvailable: canAuthorSkillFiles({ + agent: addedAgent, + scopedEditableSkillIds, + skillCreateAllowed, + skillsCapabilityEnabled, + ephemeralSkillsToggle, + }), codeEnvAvailable, + skillStates, + defaultActiveOnShare, }, { getFiles: db.getFiles, @@ -118,6 +187,9 @@ const processAddedConvo = async ({ getToolFilesByIds: db.getToolFilesByIds, getCodeGeneratedFiles: db.getCodeGeneratedFiles, filterFilesByAgentAccess, + listSkillsByAccess: db.listSkillsByAccess, + listAlwaysApplySkills: db.listAlwaysApplySkills, + getSkillByName: db.getSkillByName, }, ); diff --git a/api/server/services/Endpoints/agents/addedConvo.spec.js b/api/server/services/Endpoints/agents/addedConvo.spec.js index b5c9427690..cca3372bbd 100644 --- a/api/server/services/Endpoints/agents/addedConvo.spec.js +++ b/api/server/services/Endpoints/agents/addedConvo.spec.js @@ -1,6 +1,9 @@ const mockInitializeAgent = jest.fn(); const mockValidateAgentModel = jest.fn(); const mockLoadAddedAgent = jest.fn(); +const mockResolveAgentScopedSkillIds = jest.fn(); +const mockResolveModelSpecSkillIds = jest.fn(); +const mockCanAuthorSkillFiles = jest.fn(); const mockGetAgent = jest.fn(); const mockGetMCPServerTools = jest.fn(); @@ -18,6 +21,8 @@ jest.mock('@librechat/api', () => ({ initializeAgent: (...args) => mockInitializeAgent(...args), validateAgentModel: (...args) => mockValidateAgentModel(...args), loadAddedAgent: (params) => mockLoadAddedAgent(params), + resolveAgentScopedSkillIds: (...args) => mockResolveAgentScopedSkillIds(...args), + resolveModelSpecSkillIds: (...args) => mockResolveModelSpecSkillIds(...args), })); jest.mock('~/server/services/Files/permissions', () => ({ @@ -28,11 +33,20 @@ jest.mock('~/server/services/Config', () => ({ getMCPServerTools: (...args) => mockGetMCPServerTools(...args), })); +jest.mock('./skillDeps', () => ({ + canAuthorSkillFiles: (...args) => mockCanAuthorSkillFiles(...args), +})); + jest.mock('~/models', () => ({ getAgent: (...args) => mockGetAgent(...args), + getSkillByName: jest.fn(), + listSkillsByAccess: jest.fn(), + listAlwaysApplySkills: jest.fn(), })); const { processAddedConvo } = require('./addedConvo'); +const db = require('~/models'); +const { Constants } = require('librechat-data-provider'); const makeReq = () => ({ user: { id: 'u1', role: 'USER' } }); @@ -44,7 +58,7 @@ const makeReq = () => ({ user: { id: 'u1', role: 'USER' } }); * `CodeExecutionToolDefinition` landed in their `toolDefinitions` via the * registry regardless of any explicit flag. */ -describe('processAddedConvo — codeEnvAvailable passthrough', () => { +describe('processAddedConvo', () => { beforeEach(() => { jest.clearAllMocks(); mockValidateAgentModel.mockResolvedValue({ isValid: true }); @@ -53,6 +67,11 @@ describe('processAddedConvo — codeEnvAvailable passthrough', () => { userMCPAuthMap: undefined, }); mockLoadAddedAgent.mockResolvedValue({ id: 'added-agent', provider: 'openai' }); + mockResolveAgentScopedSkillIds.mockImplementation( + ({ accessibleSkillIds }) => accessibleSkillIds, + ); + mockResolveModelSpecSkillIds.mockResolvedValue([]); + mockCanAuthorSkillFiles.mockReturnValue(false); }); const baseParams = (overrides = {}) => ({ @@ -105,4 +124,108 @@ describe('processAddedConvo — codeEnvAvailable passthrough', () => { expect.anything(), ); }); + + it('resolves and forwards model-spec skill scope for added ephemeral agents', async () => { + const accessibleSkillId = { toString: () => 'accessible-skill' }; + const editableSkillId = { toString: () => 'editable-skill' }; + const resolvedSkillId = { toString: () => 'resolved-skill' }; + const scopedSkillId = { toString: () => 'scoped-skill' }; + const scopedEditableSkillId = { toString: () => 'scoped-editable-skill' }; + const skillStates = { 'scoped-skill': true }; + + mockLoadAddedAgent.mockResolvedValue({ + id: Constants.EPHEMERAL_AGENT_ID, + provider: 'openai', + skills_enabled: true, + skills: [], + }); + mockResolveModelSpecSkillIds.mockResolvedValue([resolvedSkillId]); + mockResolveAgentScopedSkillIds + .mockReturnValueOnce([scopedSkillId]) + .mockReturnValueOnce([scopedEditableSkillId]); + mockCanAuthorSkillFiles.mockReturnValue(true); + + await processAddedConvo( + baseParams({ + req: { + user: { id: 'u1', role: 'USER' }, + config: { + modelSpecs: { + list: [ + { + name: 'added-spec', + skills: ['finance-analyst'], + }, + ], + }, + }, + }, + endpointOption: { + spec: 'primary-spec', + addedConvo: { + endpoint: 'openai', + model: 'gpt-4o', + spec: 'added-spec', + }, + }, + accessibleSkillIds: [accessibleSkillId], + editableSkillIds: [editableSkillId], + skillsCapabilityEnabled: true, + ephemeralSkillsToggle: false, + skillCreateAllowed: true, + skillStates, + defaultActiveOnShare: true, + }), + ); + + expect(mockResolveModelSpecSkillIds).toHaveBeenCalledWith({ + names: ['finance-analyst'], + accessibleSkillIds: [accessibleSkillId], + getSkillByName: db.getSkillByName, + }); + expect(mockResolveAgentScopedSkillIds).toHaveBeenNthCalledWith(1, { + agent: expect.objectContaining({ + id: Constants.EPHEMERAL_AGENT_ID, + skills_enabled: true, + skills: ['resolved-skill'], + }), + accessibleSkillIds: [accessibleSkillId], + skillsCapabilityEnabled: true, + ephemeralSkillsToggle: false, + }); + expect(mockResolveAgentScopedSkillIds).toHaveBeenNthCalledWith(2, { + agent: expect.objectContaining({ + id: Constants.EPHEMERAL_AGENT_ID, + skills_enabled: true, + skills: ['resolved-skill'], + }), + accessibleSkillIds: [editableSkillId], + skillsCapabilityEnabled: true, + ephemeralSkillsToggle: false, + }); + expect(mockCanAuthorSkillFiles).toHaveBeenCalledWith({ + agent: expect.objectContaining({ + id: Constants.EPHEMERAL_AGENT_ID, + skills_enabled: true, + skills: ['resolved-skill'], + }), + scopedEditableSkillIds: [scopedEditableSkillId], + skillCreateAllowed: true, + skillsCapabilityEnabled: true, + ephemeralSkillsToggle: false, + }); + expect(mockInitializeAgent).toHaveBeenCalledWith( + expect.objectContaining({ + accessibleSkillIds: [scopedSkillId], + skillAuthoringAvailable: true, + skillStates, + defaultActiveOnShare: true, + }), + expect.objectContaining({ + listSkillsByAccess: db.listSkillsByAccess, + listAlwaysApplySkills: db.listAlwaysApplySkills, + getSkillByName: db.getSkillByName, + }), + ); + }); }); diff --git a/api/server/services/Endpoints/agents/initialize.js b/api/server/services/Endpoints/agents/initialize.js index c7e62bd9e3..25b00d05a2 100644 --- a/api/server/services/Endpoints/agents/initialize.js +++ b/api/server/services/Endpoints/agents/initialize.js @@ -10,6 +10,7 @@ const { getCustomEndpointConfig, discoverConnectedAgents, resolveAgentScopedSkillIds, + resolveModelSpecSkillIds, buildAgentContextAttachmentsByAgentId, } = require('@librechat/api'); const { @@ -133,7 +134,8 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { /** Query accessible skill IDs once per run (shared across all agents). * Skills activate under strict opt-in semantics — see * `resolveAgentScopedSkillIds` for the per-agent activation predicate: - * - Ephemeral agent → per-conversation skills badge toggle (full catalog). + * - Ephemeral agent → model-spec `skills` config first, otherwise the + * per-conversation skills badge toggle (full catalog). * - Persisted agent → `agent.skills_enabled === true`. Optional * `agent.skills` allowlist narrows the catalog; empty/undefined * allowlist with the toggle on = full accessible catalog. */ @@ -288,6 +290,34 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { */ const manualSkills = extractManualSkills(req.body); + const selectedModelSpec = + endpointOption.spec && Array.isArray(appConfig?.modelSpecs?.list) + ? appConfig.modelSpecs.list.find((modelSpec) => modelSpec.name === endpointOption.spec) + : null; + + if ( + primaryAgent && + isEphemeralAgentId(primaryAgent.id) && + selectedModelSpec && + Object.hasOwn(selectedModelSpec, 'skills') + ) { + if (selectedModelSpec.skills === true) { + primaryAgent.skills_enabled = true; + delete primaryAgent.skills; + } else if (selectedModelSpec.skills === false) { + primaryAgent.skills_enabled = false; + primaryAgent.skills = []; + } else if (Array.isArray(selectedModelSpec.skills)) { + const resolvedSkillIds = await resolveModelSpecSkillIds({ + names: selectedModelSpec.skills, + accessibleSkillIds, + getSkillByName: db.getSkillByName, + }); + primaryAgent.skills_enabled = true; + primaryAgent.skills = resolvedSkillIds.map((id) => id.toString()); + } + } + const primaryScopedSkillIds = resolveAgentScopedSkillIds({ agent: primaryAgent, accessibleSkillIds, @@ -456,6 +486,13 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { parentMessageId, allowedProviders, primaryAgentId: primaryConfig.id, + accessibleSkillIds, + editableSkillIds, + skillsCapabilityEnabled, + ephemeralSkillsToggle, + skillCreateAllowed, + skillStates, + defaultActiveOnShare, codeEnvAvailable, }); diff --git a/api/server/services/Endpoints/agents/initialize.spec.js b/api/server/services/Endpoints/agents/initialize.spec.js index 846e5adb1f..c2ca150228 100644 --- a/api/server/services/Endpoints/agents/initialize.spec.js +++ b/api/server/services/Endpoints/agents/initialize.spec.js @@ -6,6 +6,7 @@ const { PrincipalModel, MAX_SUBAGENT_DEPTH, MAX_SUBAGENT_GRAPH_NODES, + Constants, } = require('librechat-data-provider'); const { MongoMemoryServer } = require('mongodb-memory-server'); @@ -277,6 +278,42 @@ describe('initializeClient — processAgent ACL gate', () => { expect(initializeParams.accessibleSkillIds.map(String)).toContain(skill._id.toString()); expect(initializeParams.skillAuthoringAvailable).toBe(false); }); + + it('enables skill authoring when model specs enable skills for an ephemeral agent', async () => { + const endpointOption = makeEndpointOption(); + endpointOption.spec = 'spec-skills'; + endpointOption.agent = Promise.resolve({ + id: Constants.EPHEMERAL_AGENT_ID, + name: 'Ephemeral Primary', + provider: 'openai', + model: 'gpt-4', + tools: [], + }); + mockInitializeAgent.mockResolvedValue(makePrimaryConfig([])); + const req = makeReq(); + req.config.endpoints.agents = { capabilities: ['skills'] }; + req.config.modelSpecs = { + list: [{ name: 'spec-skills', skills: true }], + }; + const canCreateSkillSpy = jest + .spyOn(getSkillToolDeps(), 'canCreateSkill') + .mockResolvedValue(true); + + try { + await initializeClient({ + req, + res: {}, + signal: new AbortController().signal, + endpointOption, + }); + } finally { + canCreateSkillSpy.mockRestore(); + } + + const initializeParams = mockInitializeAgent.mock.calls[0][0]; + expect(initializeParams.agent.skills_enabled).toBe(true); + expect(initializeParams.skillAuthoringAvailable).toBe(true); + }); }); describe('initializeClient — subagent loading', () => { diff --git a/api/server/services/Endpoints/agents/skillDeps.js b/api/server/services/Endpoints/agents/skillDeps.js index 39cbc7e858..27b8eeff66 100644 --- a/api/server/services/Endpoints/agents/skillDeps.js +++ b/api/server/services/Endpoints/agents/skillDeps.js @@ -128,6 +128,12 @@ function isAgentSkillsEnabledForRun({ agent, skillsCapabilityEnabled, ephemeralS return false; } if (isEphemeralAgentId(agent.id)) { + if (agent.skills_enabled === false) { + return false; + } + if (agent.skills_enabled === true) { + return true; + } return ephemeralSkillsToggle === true; } return agent.skills_enabled === true; diff --git a/e2e/config/librechat.e2e.yaml b/e2e/config/librechat.e2e.yaml index 8f73e53e8c..7da6aa4b10 100644 --- a/e2e/config/librechat.e2e.yaml +++ b/e2e/config/librechat.e2e.yaml @@ -25,3 +25,27 @@ endpoints: fetch: false titleConvo: false modelDisplayLabel: 'Mock Provider B' + +modelSpecs: + list: + - name: 'e2e-mock-provider-a' + label: 'Mock Provider A' + preset: + endpoint: 'Mock Provider A' + model: 'mock-model-a' + + - name: 'e2e-mock-provider-b' + label: 'Mock Provider B' + preset: + endpoint: 'Mock Provider B' + model: 'mock-model-b' + + - name: 'e2e-skill-scope' + label: 'E2E Skill Scope' + preset: + endpoint: 'Mock Provider A' + model: 'mock-model-a' + skills: + - 'e2e-model-spec-allowed' + - 'e2e-model-spec-missing' + - 'e2e-model-spec-inaccessible' diff --git a/e2e/setup/fake-model.js b/e2e/setup/fake-model.js index 04a2f9b47b..032399ece1 100644 --- a/e2e/setup/fake-model.js +++ b/e2e/setup/fake-model.js @@ -14,13 +14,20 @@ const CHUNK_DELAY_MS = Number(process.env.MOCK_LLM_CHUNK_DELAY_MS) || 10; const CREATE_SKILL_MARKER = 'E2E_CREATE_SKILL:'; const EDIT_SKILL_MARKER = 'E2E_EDIT_SKILL:'; +const ASSERT_MODEL_SPEC_SKILLS_MARKER = 'E2E_ASSERT_MODEL_SPEC_SKILLS'; const CREATE_FILE_AUTHORING_FINAL_TEXT = 'E2E file authoring complete'; const EDIT_FILE_AUTHORING_FINAL_TEXT = 'E2E file edit complete'; +const MODEL_SPEC_SKILL_ASSERTION_FINAL_TEXT = 'E2E model spec skill assertion passed'; const CREATE_FILE_TOOL_NAME = 'create_file'; const EDIT_FILE_TOOL_NAME = 'edit_file'; const BASH_TOOL_NAME = 'bash_tool'; +const SKILL_TOOL_NAME = 'skill'; const CREATE_SKILL_TOOL_CALL_ID = 'call_e2e_create_skill'; const EDIT_SKILL_TOOL_CALL_ID = 'call_e2e_edit_skill'; +const MODEL_SPEC_ACCESSIBLE_SKILL = 'e2e-model-spec-allowed'; +const MODEL_SPEC_MISSING_SKILL = 'e2e-model-spec-missing'; +const MODEL_SPEC_INACCESSIBLE_SKILL = 'e2e-model-spec-inaccessible'; +const ALWAYS_APPLY_BODY_MARKER = 'E2E_ALWAYS_APPLY_BODY_MARKER'; const SKILL_DESCRIPTION = 'Use this skill to verify LibreChat skill file authoring in mock end-to-end tests.'; const EDITED_SKILL_DESCRIPTION = @@ -108,6 +115,67 @@ function collectToolNames(agents) { return names; } +function collectAdditionalInstructions(agents) { + return (agents ?? []) + .map((agent) => + typeof agent?.additional_instructions === 'string' ? agent.additional_instructions : '', + ) + .filter(Boolean) + .join('\n'); +} + +function collectSkillPrimeMessages(messages) { + return (messages ?? []) + .filter((message) => message?.additional_kwargs?.source === 'skill') + .map((message) => ({ + name: message.additional_kwargs.skillName, + trigger: message.additional_kwargs.trigger, + content: getContentText(message.content), + })); +} + +function modelSpecSkillAssertionResponses({ agents, messages, toolNames }) { + const failures = []; + const additionalInstructions = collectAdditionalInstructions(agents); + const skillPrimeMessages = collectSkillPrimeMessages(messages); + const alwaysApplyPrime = skillPrimeMessages.find( + (message) => message.name === MODEL_SPEC_ACCESSIBLE_SKILL && message.trigger === 'always-apply', + ); + + if (!toolNames.has(SKILL_TOOL_NAME)) { + failures.push(`${SKILL_TOOL_NAME} tool was not advertised`); + } + if (!additionalInstructions.includes(MODEL_SPEC_ACCESSIBLE_SKILL)) { + failures.push(`${MODEL_SPEC_ACCESSIBLE_SKILL} was not present in the model-visible catalog`); + } + if (additionalInstructions.includes(MODEL_SPEC_MISSING_SKILL)) { + failures.push(`${MODEL_SPEC_MISSING_SKILL} leaked into the model-visible catalog`); + } + if (additionalInstructions.includes(MODEL_SPEC_INACCESSIBLE_SKILL)) { + failures.push(`${MODEL_SPEC_INACCESSIBLE_SKILL} leaked into the model-visible catalog`); + } + if (!alwaysApplyPrime) { + failures.push(`${MODEL_SPEC_ACCESSIBLE_SKILL} was not always-apply primed`); + } else if (!alwaysApplyPrime.content.includes(ALWAYS_APPLY_BODY_MARKER)) { + failures.push(`${MODEL_SPEC_ACCESSIBLE_SKILL} always-apply body was missing its marker`); + } + if (skillPrimeMessages.some((message) => message.name === MODEL_SPEC_MISSING_SKILL)) { + failures.push(`${MODEL_SPEC_MISSING_SKILL} was unexpectedly primed`); + } + if (skillPrimeMessages.some((message) => message.name === MODEL_SPEC_INACCESSIBLE_SKILL)) { + failures.push(`${MODEL_SPEC_INACCESSIBLE_SKILL} was unexpectedly primed`); + } + + if (failures.length > 0) { + return { + responses: [`E2E model spec skill assertion failed: ${failures.join('; ')}`], + }; + } + return { + responses: [`${MODEL_SPEC_SKILL_ASSERTION_FINAL_TEXT}: ${MODEL_SPEC_ACCESSIBLE_SKILL}`], + }; +} + function buildSkillBody(skillName) { return `--- name: ${skillName} @@ -166,7 +234,11 @@ function fileAuthoringResponses(operation, toolNames) { }; } -function resolveResponses(text, toolNames) { +function resolveResponses({ agents, messages, text, toolNames }) { + if (text.includes(ASSERT_MODEL_SPEC_SKILLS_MARKER)) { + return modelSpecSkillAssertionResponses({ agents, messages, toolNames }); + } + const createSkillName = getRequestedSkillName(text, CREATE_SKILL_MARKER); if (createSkillName) { return fileAuthoringResponses( @@ -208,6 +280,11 @@ module.exports = function fakeModelHook(run, context) { const text = getLatestUserText(context?.messages); const toolNames = collectToolNames(context?.agents); - const { responses, toolCalls } = resolveResponses(text, toolNames); + const { responses, toolCalls } = resolveResponses({ + agents: context?.agents, + messages: context?.messages, + text, + toolNames, + }); graph.overrideTestModel(responses, CHUNK_DELAY_MS, toolCalls); }; diff --git a/e2e/specs/mock/helpers.ts b/e2e/specs/mock/helpers.ts index 7ad75687fd..0ec03b16b0 100644 --- a/e2e/specs/mock/helpers.ts +++ b/e2e/specs/mock/helpers.ts @@ -14,6 +14,10 @@ export type MockEndpoint = (typeof MOCK_ENDPOINTS)[number]; export const NEW_CHAT_PATH = '/c/new'; +type RefreshTokenBody = { + token?: string; +}; + export function isAgentsStream(response: Response) { return response.url().includes('/api/agents') && response.status() === 200; } @@ -21,12 +25,30 @@ export function isAgentsStream(response: Response) { const modelSelectorTrigger = (page: Page) => page.getByRole('button', { name: 'Select a model' }).first(); +const escapeRegExp = (value: string) => value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + /** Open the model selector, choose an endpoint, then its model (committed on the model click). */ export async function selectMockEndpoint(page: Page, endpoint: MockEndpoint) { - await modelSelectorTrigger(page).click(); + const trigger = modelSelectorTrigger(page); + await trigger.click(); await page.getByRole('option', { name: endpoint.label }).click(); - await page.getByRole('option', { name: endpoint.model, exact: true }).click(); - await expect(modelSelectorTrigger(page)).not.toHaveText('Select a model'); + const modelOption = page.getByRole('option', { name: endpoint.model, exact: true }); + if (await modelOption.isVisible({ timeout: 1000 }).catch(() => false)) { + await modelOption.click(); + } + await expect(trigger).not.toHaveText('Select a model'); +} + +/** Open the model selector and choose a configured model spec by label. */ +export async function selectModelSpec(page: Page, label: string) { + const trigger = modelSelectorTrigger(page); + await expect(trigger).toBeVisible(); + if ((await trigger.textContent())?.includes(label)) { + return; + } + await trigger.click(); + await page.getByRole('option', { name: new RegExp(`^${escapeRegExp(label)}\\b`) }).click(); + await expect(trigger).toContainText(label); } /** Enable the ephemeral Skills capability from the composer tool menu. */ @@ -53,3 +75,88 @@ export async function sendMessage(page: Page, text: string): Promise { ]); return response; } + +export async function getAccessToken(page: Page): Promise { + const result = await page.evaluate(async () => { + const response = await fetch('/api/auth/refresh', { + method: 'POST', + credentials: 'include', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({}), + }); + const text = await response.text(); + let json: unknown = null; + try { + json = text ? JSON.parse(text) : null; + } catch { + json = null; + } + return { ok: response.ok, status: response.status, text, json }; + }); + + if (!result.ok) { + throw new Error( + `Expected /api/auth/refresh to return 2xx, got ${result.status}: ${result.text}`, + ); + } + + const body = result.json as RefreshTokenBody | null; + if (!body?.token) { + throw new Error(`Expected /api/auth/refresh to return a token, got: ${result.text}`); + } + + return body.token; +} + +export async function requestJson( + page: Page, + params: { + path: string; + token: string; + method?: string; + body?: unknown; + }, +): Promise { + const result = await page.evaluate( + async ({ accessToken, body, method, urlPath }) => { + const headers: Record = { + Authorization: `Bearer ${accessToken}`, + }; + const init: RequestInit = { + method, + credentials: 'include', + headers, + }; + if (body !== undefined) { + headers['Content-Type'] = 'application/json'; + init.body = JSON.stringify(body); + } + const response = await fetch(urlPath, init); + const text = await response.text(); + let json: unknown = null; + try { + json = text ? JSON.parse(text) : null; + } catch { + json = null; + } + return { ok: response.ok, status: response.status, text, json }; + }, + { + accessToken: params.token, + body: params.body, + method: params.method ?? 'GET', + urlPath: params.path, + }, + ); + + if (!result.ok) { + throw new Error( + `Expected ${params.method ?? 'GET'} ${params.path} to return 2xx, got ${result.status}: ${result.text}`, + ); + } + return result.json as T; +} + +export async function fetchJson(page: Page, path: string, token: string): Promise { + return requestJson(page, { path, token }); +} diff --git a/e2e/specs/mock/model-spec-skills.spec.ts b/e2e/specs/mock/model-spec-skills.spec.ts new file mode 100644 index 0000000000..b0c9394959 --- /dev/null +++ b/e2e/specs/mock/model-spec-skills.spec.ts @@ -0,0 +1,175 @@ +import { expect, test } from '@playwright/test'; +import { MongoClient, ObjectId } from 'mongodb'; +import type { Page } from '@playwright/test'; +import { applyRuntimeEnv } from '../../setup/runtimeEnv'; +import { + NEW_CHAT_PATH, + fetchJson, + getAccessToken, + requestJson, + selectModelSpec, + sendMessage, +} from './helpers'; + +const MODEL_SPEC_LABEL = 'E2E Skill Scope'; +const ASSERTION_MARKER = 'E2E_ASSERT_MODEL_SPEC_SKILLS'; +const ASSERTION_FINAL_TEXT = 'E2E model spec skill assertion passed'; +const ACCESSIBLE_SKILL_NAME = 'e2e-model-spec-allowed'; +const INACCESSIBLE_SKILL_NAME = 'e2e-model-spec-inaccessible'; +const ALWAYS_APPLY_BODY_MARKER = 'E2E_ALWAYS_APPLY_BODY_MARKER'; +const INACCESSIBLE_AUTHOR_ID = new ObjectId('64f000000000000000000001'); +const SKILL_DESCRIPTION = + 'Use this skill to verify model-spec skill scoping and always-apply priming in mock e2e tests.'; + +type SkillSummary = { + _id: string; + name: string; + description: string; + version: number; + alwaysApply?: boolean; +}; + +type SkillDetail = SkillSummary & { + body: string; +}; + +function buildSkillBody(name: string) { + return `--- +name: ${name} +description: ${SKILL_DESCRIPTION} +alwaysApply: true +--- + +# ${name} + +${ALWAYS_APPLY_BODY_MARKER} + +This body should be injected as an always-apply skill prime.`; +} + +async function findSkill( + page: Page, + skillName: string, + token: string, +): Promise { + const body = await fetchJson<{ skills?: SkillSummary[] }>( + page, + `/api/skills?search=${encodeURIComponent(skillName)}&limit=10`, + token, + ); + return body.skills?.find((skill) => skill.name === skillName) ?? null; +} + +async function seedAccessibleSkill(page: Page, token: string): Promise { + const body = buildSkillBody(ACCESSIBLE_SKILL_NAME); + const payload = { + name: ACCESSIBLE_SKILL_NAME, + description: SKILL_DESCRIPTION, + body, + }; + const existing = await findSkill(page, ACCESSIBLE_SKILL_NAME, token); + if (!existing) { + return requestJson(page, { + path: '/api/skills', + token, + method: 'POST', + body: payload, + }); + } + + const detail = await fetchJson( + page, + `/api/skills/${encodeURIComponent(existing._id)}`, + token, + ); + return requestJson(page, { + path: `/api/skills/${encodeURIComponent(existing._id)}`, + token, + method: 'PATCH', + body: { + ...payload, + expectedVersion: detail.version, + }, + }); +} + +async function seedInaccessibleSkill() { + applyRuntimeEnv(); + if (!process.env.MONGO_URI) { + throw new Error('MONGO_URI must be available for model-spec skill mock e2e tests'); + } + + const client = new MongoClient(process.env.MONGO_URI); + await client.connect(); + try { + const db = client.db(); + const skills = db.collection('skills'); + const aclEntries = db.collection('aclentries'); + const now = new Date(); + await skills.updateOne( + { + name: INACCESSIBLE_SKILL_NAME, + author: INACCESSIBLE_AUTHOR_ID, + tenantId: null, + }, + { + $set: { + description: SKILL_DESCRIPTION, + body: buildSkillBody(INACCESSIBLE_SKILL_NAME), + authorName: 'Inaccessible E2E User', + updatedAt: now, + }, + $setOnInsert: { + displayTitle: INACCESSIBLE_SKILL_NAME, + frontmatter: {}, + disableModelInvocation: false, + userInvocable: true, + allowedTools: [], + alwaysApply: true, + source: 'inline', + fileCount: 0, + version: 1, + createdAt: now, + }, + }, + { upsert: true }, + ); + const skill = await skills.findOne({ + name: INACCESSIBLE_SKILL_NAME, + author: INACCESSIBLE_AUTHOR_ID, + tenantId: null, + }); + if (skill?._id) { + await aclEntries.deleteMany({ resourceType: 'skill', resourceId: skill._id }); + } + } finally { + await client.close(); + } +} + +test.describe('model spec skills', () => { + test('loads accessible configured skills and skips missing or inaccessible names', async ({ + page, + }) => { + test.setTimeout(120000); + + await page.goto(NEW_CHAT_PATH, { timeout: 10000 }); + const token = await getAccessToken(page); + const skill = await seedAccessibleSkill(page, token); + expect(skill.alwaysApply).toBe(true); + await seedInaccessibleSkill(); + + await selectModelSpec(page, MODEL_SPEC_LABEL); + const response = await sendMessage( + page, + `${ASSERTION_MARKER}\nVerify model-spec skill scope and always-apply frontmatter.`, + ); + expect(response.ok()).toBeTruthy(); + + await expect( + page + .getByTestId('messages-view') + .getByText(`${ASSERTION_FINAL_TEXT}: ${ACCESSIBLE_SKILL_NAME}`), + ).toBeVisible({ timeout: 30000 }); + }); +}); diff --git a/e2e/specs/mock/skill-file-authoring.spec.ts b/e2e/specs/mock/skill-file-authoring.spec.ts index 5e9bd1f25f..c3b9b20c4d 100644 --- a/e2e/specs/mock/skill-file-authoring.spec.ts +++ b/e2e/specs/mock/skill-file-authoring.spec.ts @@ -4,6 +4,8 @@ import { MOCK_ENDPOINTS, NEW_CHAT_PATH, enableSkills, + fetchJson, + getAccessToken, selectMockEndpoint, sendMessage, } from './helpers'; @@ -25,69 +27,8 @@ type SkillDetail = SkillSummary & { body: string; }; -type RefreshTokenBody = { - token?: string; -}; - const uniqueSkillName = () => `e2e-file-authoring-${Date.now()}-${Math.floor(Math.random() * 1e4)}`; -async function getAccessToken(page: Page): Promise { - const result = await page.evaluate(async () => { - const response = await fetch('/api/auth/refresh', { - method: 'POST', - credentials: 'include', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({}), - }); - const text = await response.text(); - let json: unknown = null; - try { - json = text ? JSON.parse(text) : null; - } catch { - json = null; - } - return { ok: response.ok, status: response.status, text, json }; - }); - - if (!result.ok) { - throw new Error( - `Expected /api/auth/refresh to return 2xx, got ${result.status}: ${result.text}`, - ); - } - - const body = result.json as RefreshTokenBody | null; - if (!body?.token) { - throw new Error(`Expected /api/auth/refresh to return a token, got: ${result.text}`); - } - - return body.token; -} - -async function fetchJson(page: Page, path: string, token: string): Promise { - const result = await page.evaluate( - async ({ accessToken, urlPath }) => { - const response = await fetch(urlPath, { - credentials: 'include', - headers: { Authorization: `Bearer ${accessToken}` }, - }); - const text = await response.text(); - let json: unknown = null; - try { - json = text ? JSON.parse(text) : null; - } catch { - json = null; - } - return { ok: response.ok, status: response.status, text, json }; - }, - { accessToken: token, urlPath: path }, - ); - - if (!result.ok) { - throw new Error(`Expected ${path} to return 2xx, got ${result.status}: ${result.text}`); - } - return result.json as T; -} - async function findSkill( page: Page, skillName: string, diff --git a/librechat.example.yaml b/librechat.example.yaml index 77c9c4c479..2f94889239 100644 --- a/librechat.example.yaml +++ b/librechat.example.yaml @@ -381,6 +381,10 @@ endpoints: # # (optional) Minimum relevance score for sources to be included in responses, defaults to 0.45 (45% relevance threshold) # # Set to 0.0 to show all sources (no filtering), or higher like 0.7 for stricter filtering # minRelevanceScore: 0.45 + # # (optional) Cap the number of active accessible skills shown in the model-visible catalog. + # # Useful for large organizations where many department-specific skills may be available. + # skills: + # maxCatalogSkills: 20 # # (optional) Agent Capabilities available to all users. Omit the ones you wish to exclude. Defaults to list below. # capabilities: ["deferred_tools", "execute_code", "file_search", "actions", "tools"] @@ -606,6 +610,10 @@ endpoints: # preset: # endpoint: "openAI" # model: "gpt-4o" +# # Skills can be enabled per model spec. Use true for the user's active +# # accessible skill catalog, false to force skills off, or a name list as +# # a strict allowlist for catalog, manual, and always-apply resolution. +# # skills: ["brand-guidelines", "code-review"] # # # Example 2: Nested under a custom endpoint (grouped with groq endpoint) # - name: "llama3-70b-8192" diff --git a/packages/api/src/agents/__tests__/load.spec.ts b/packages/api/src/agents/__tests__/load.spec.ts index 2419310b83..fb8c98039d 100644 --- a/packages/api/src/agents/__tests__/load.spec.ts +++ b/packages/api/src/agents/__tests__/load.spec.ts @@ -1,11 +1,16 @@ import mongoose from 'mongoose'; import { v4 as uuidv4 } from 'uuid'; -import { Constants } from 'librechat-data-provider'; +import { Constants, FileSources } from 'librechat-data-provider'; import { MongoMemoryServer } from 'mongodb-memory-server'; import { agentSchema, createMethods } from '@librechat/data-schemas'; -import type { AgentModelParameters } from 'librechat-data-provider'; +import type { + Agent as LibreChatAgent, + AgentModelParameters, + TConversation, +} from 'librechat-data-provider'; import type { LoadAgentParams, LoadAgentDeps } from '../load'; import { loadAgent } from '../load'; +import { loadAddedAgent } from '../added'; let Agent: mongoose.Model; let createAgent: ReturnType['createAgent']; @@ -253,6 +258,184 @@ describe('loadAgent', () => { expect(result?.model_parameters).not.toHaveProperty('promptPrefix'); }); + test('should enable full skill scope for ephemeral model spec with skills true', async () => { + const { EPHEMERAL_AGENT_ID } = Constants; + + const result = await loadAgent( + { + req: { + user: { id: 'user123' }, + body: {}, + config: { + config: {}, + fileStrategy: FileSources.local, + imageOutputType: 'png', + modelSpecs: { + list: [ + { + name: 'skills-on', + label: 'Skills On', + preset: { endpoint: 'openai', model: 'gpt-4' }, + skills: true, + }, + ], + }, + }, + }, + spec: 'skills-on', + agent_id: EPHEMERAL_AGENT_ID as string, + endpoint: 'openai', + model_parameters: { model: 'gpt-4' } as unknown as AgentModelParameters, + }, + deps, + ); + + expect(result?.skills_enabled).toBe(true); + expect(result?.skills).toBeUndefined(); + }); + + test('should initialize an empty allowlist for ephemeral model spec skill names', async () => { + const { EPHEMERAL_AGENT_ID } = Constants; + + const result = await loadAgent( + { + req: { + user: { id: 'user123' }, + body: {}, + config: { + config: {}, + fileStrategy: FileSources.local, + imageOutputType: 'png', + modelSpecs: { + list: [ + { + name: 'scoped-skills', + label: 'Scoped Skills', + preset: { endpoint: 'openai', model: 'gpt-4' }, + skills: ['finance-analyst', 'brand-writer'], + }, + ], + }, + }, + }, + spec: 'scoped-skills', + agent_id: EPHEMERAL_AGENT_ID as string, + endpoint: 'openai', + model_parameters: { model: 'gpt-4' } as unknown as AgentModelParameters, + }, + deps, + ); + + expect(result?.skills_enabled).toBe(true); + expect(result?.skills).toEqual([]); + }); + + test('should enable full skill scope for added ephemeral model spec with skills true', async () => { + const result = await loadAddedAgent( + { + req: { + user: { id: 'user123' }, + config: { + config: {}, + fileStrategy: FileSources.local, + imageOutputType: 'png', + modelSpecs: { + list: [ + { + name: 'added-skills-on', + label: 'Added Skills On', + preset: { endpoint: 'openai', model: 'gpt-4' }, + skills: true, + }, + ], + }, + }, + }, + conversation: { + endpoint: 'openai', + model: 'gpt-4', + spec: 'added-skills-on', + } as unknown as TConversation, + }, + deps, + ); + + expect(result?.skills_enabled).toBe(true); + expect(result?.skills).toBeUndefined(); + }); + + test('should initialize an empty allowlist for added ephemeral model spec skill names', async () => { + const result = await loadAddedAgent( + { + req: { + user: { id: 'user123' }, + config: { + config: {}, + fileStrategy: FileSources.local, + imageOutputType: 'png', + modelSpecs: { + list: [ + { + name: 'added-scoped-skills', + label: 'Added Scoped Skills', + preset: { endpoint: 'openai', model: 'gpt-4' }, + skills: ['finance-analyst', 'brand-writer'], + }, + ], + }, + }, + }, + conversation: { + endpoint: 'openai', + model: 'gpt-4', + spec: 'added-scoped-skills', + } as unknown as TConversation, + }, + deps, + ); + + expect(result?.skills_enabled).toBe(true); + expect(result?.skills).toEqual([]); + }); + + test('should apply model spec skills when added agent mirrors ephemeral primary tools', async () => { + const { EPHEMERAL_AGENT_ID } = Constants; + + const result = await loadAddedAgent( + { + req: { + user: { id: 'user123' }, + config: { + config: {}, + fileStrategy: FileSources.local, + imageOutputType: 'png', + modelSpecs: { + list: [ + { + name: 'mirrored-scoped-skills', + label: 'Mirrored Scoped Skills', + preset: { endpoint: 'openai', model: 'gpt-4' }, + skills: ['brand-writer'], + }, + ], + }, + }, + }, + conversation: { + endpoint: 'openai', + model: 'gpt-4', + spec: 'mirrored-scoped-skills', + } as unknown as TConversation, + primaryAgent: { id: EPHEMERAL_AGENT_ID as string, tools: ['web_search'] } as LibreChatAgent, + }, + deps, + ); + + expect(result?.tools).toEqual(['web_search']); + expect(result?.skills_enabled).toBe(true); + expect(result?.skills).toEqual([]); + }); + test('should handle ephemeral agent with undefined ephemeralAgent in body', async () => { const { EPHEMERAL_AGENT_ID } = Constants; diff --git a/packages/api/src/agents/__tests__/skills.test.ts b/packages/api/src/agents/__tests__/skills.test.ts index 3142273f7c..b09b39685b 100644 --- a/packages/api/src/agents/__tests__/skills.test.ts +++ b/packages/api/src/agents/__tests__/skills.test.ts @@ -38,6 +38,7 @@ import { scopeSkillIds, resolveSkillActive, resolveAgentScopedSkillIds, + resolveModelSpecSkillIds, injectSkillCatalog, buildSkillPrimeMessage, resolveManualSkills, @@ -312,9 +313,11 @@ describe('resolveAgentScopedSkillIds', () => { }); const ephemeralAgent = ( skills?: string[], + skills_enabled?: boolean, ): { id: string; skills?: string[]; skills_enabled?: boolean } => ({ id: 'ephemeral_convo_xyz', skills, + skills_enabled, }); it('returns [] when the skills capability is disabled, even with every other signal on', () => { @@ -366,14 +369,50 @@ describe('resolveAgentScopedSkillIds', () => { expect(scoped.map((o) => o.toString()).sort()).toEqual([a.toString(), b.toString()].sort()); }); - it('ignores any `skills` field on an ephemeral agent (toggle is the only signal)', () => { + it('returns the full accessible catalog when a model spec enables skills', () => { + const a = makeId(); + const b = makeId(); + const scoped = resolveAgentScopedSkillIds({ + agent: ephemeralAgent(undefined, true), + accessibleSkillIds: [a, b], + skillsCapabilityEnabled: true, + ephemeralSkillsToggle: false, + }); + expect(scoped.map((o) => o.toString()).sort()).toEqual([a.toString(), b.toString()].sort()); + }); + + it('returns the model-spec allowlist intersection when configured', () => { + const a = makeId(); + const b = makeId(); + const scoped = resolveAgentScopedSkillIds({ + agent: ephemeralAgent([a.toString()], true), + accessibleSkillIds: [a, b], + skillsCapabilityEnabled: true, + ephemeralSkillsToggle: false, + }); + expect(scoped.map((o) => o.toString())).toEqual([a.toString()]); + }); + + it('treats an empty model-spec allowlist as explicit none', () => { const a = makeId(); expect( resolveAgentScopedSkillIds({ - agent: ephemeralAgent([a.toString()]), + agent: ephemeralAgent([], true), accessibleSkillIds: [a], skillsCapabilityEnabled: true, - ephemeralSkillsToggle: false, + ephemeralSkillsToggle: true, + }), + ).toEqual([]); + }); + + it('lets an explicit model-spec skills=false override the badge toggle', () => { + const a = makeId(); + expect( + resolveAgentScopedSkillIds({ + agent: ephemeralAgent(undefined, false), + accessibleSkillIds: [a], + skillsCapabilityEnabled: true, + ephemeralSkillsToggle: true, }), ).toEqual([]); }); @@ -470,6 +509,95 @@ describe('resolveAgentScopedSkillIds', () => { }); }); +describe('resolveModelSpecSkillIds', () => { + const userObjectId = new Types.ObjectId(); + + it('resolves configured names against accessible skills and skips misses without failing', async () => { + const knownId = new Types.ObjectId(); + const getSkillByName = jest.fn(async (name: string) => { + if (name === 'known-skill') { + return { + _id: knownId, + name, + body: 'body', + author: userObjectId, + }; + } + if (name === 'throws') { + throw new Error('lookup failed'); + } + return null; + }); + + const result = await resolveModelSpecSkillIds({ + names: [' known-skill ', 'missing-skill', 'throws', 'known-skill'], + accessibleSkillIds: [knownId], + getSkillByName, + }); + + expect(result.map((id) => id.toString())).toEqual([knownId.toString()]); + expect(getSkillByName).toHaveBeenCalledTimes(3); + expect(getSkillByName).toHaveBeenCalledWith('known-skill', [knownId], { + preferModelInvocable: true, + }); + }); + + it('resolves configured names sequentially to avoid query bursts', async () => { + const firstId = new Types.ObjectId(); + const secondId = new Types.ObjectId(); + const order: string[] = []; + let releaseFirst!: () => void; + const firstLookup = new Promise((resolve) => { + releaseFirst = resolve; + }); + const getSkillByName = jest.fn(async (name: string) => { + order.push(`start:${name}`); + if (name === 'first') { + await firstLookup; + order.push(`end:${name}`); + return { + _id: firstId, + name, + body: 'body', + author: userObjectId, + }; + } + order.push(`end:${name}`); + return { + _id: secondId, + name, + body: 'body', + author: userObjectId, + }; + }); + + const promise = resolveModelSpecSkillIds({ + names: ['first', 'second'], + accessibleSkillIds: [firstId, secondId], + getSkillByName, + }); + + await Promise.resolve(); + expect(order).toEqual(['start:first']); + + releaseFirst(); + const result = await promise; + + expect(result.map((id) => id.toString())).toEqual([firstId.toString(), secondId.toString()]); + expect(order).toEqual(['start:first', 'end:first', 'start:second', 'end:second']); + }); + + it('returns [] when no skill lookup is available', async () => { + const result = await resolveModelSpecSkillIds({ + names: ['known-skill'], + accessibleSkillIds: [new Types.ObjectId()], + getSkillByName: undefined, + }); + + expect(result).toEqual([]); + }); +}); + describe('resolveSkillActive', () => { const makeSkill = (author: Types.ObjectId) => ({ _id: new Types.ObjectId(), author }); @@ -744,6 +872,26 @@ describe('injectSkillCatalog', () => { expect(agent.additional_instructions).toContain('desc-my-skill'); }); + it('honors a configured maxCatalogSkills below the default hard limit', async () => { + const first = makeSkill('first-skill', userObjectId); + const second = makeSkill('second-skill', userObjectId); + const third = makeSkill('third-skill', userObjectId); + const listSkillsByAccess = buildPager([[first, second, third]]); + const agent = makeAgent(); + const result = await injectSkillCatalog( + baseParams({ listSkillsByAccess, agent, maxCatalogSkills: 2 }), + ); + + expect(result.skillCount).toBe(2); + expect(result.activeSkillIds.map((id) => id.toString())).toEqual([ + first._id.toString(), + second._id.toString(), + ]); + expect(agent.additional_instructions).toContain('first-skill'); + expect(agent.additional_instructions).toContain('second-skill'); + expect(agent.additional_instructions).not.toContain('third-skill'); + }); + it('fails closed when userId is absent (shared skills drop, owned would need override)', async () => { const owned = makeSkill('my-skill', userObjectId); const shared = makeSkill('shared-skill', new Types.ObjectId()); diff --git a/packages/api/src/agents/added.ts b/packages/api/src/agents/added.ts index 587f3bc437..64b92938ca 100644 --- a/packages/api/src/agents/added.ts +++ b/packages/api/src/agents/added.ts @@ -8,13 +8,32 @@ import { appendAgentIdSuffix, encodeEphemeralAgentId, } from 'librechat-data-provider'; -import type { Agent, TConversation } from 'librechat-data-provider'; +import type { Agent, TConversation, TModelSpec } from 'librechat-data-provider'; import { getCustomEndpointConfig } from '~/app/config'; const { mcp_all, mcp_delimiter } = Constants; export const ADDED_AGENT_ID = 'added_agent'; +function applyModelSpecSkills( + result: Record, + modelSpec: Pick | null | undefined, +): void { + if (!modelSpec || !Object.prototype.hasOwnProperty.call(modelSpec, 'skills')) { + return; + } + if (modelSpec.skills === true) { + result.skills_enabled = true; + delete result.skills; + } else if (modelSpec.skills === false) { + result.skills_enabled = false; + result.skills = []; + } else if (Array.isArray(modelSpec.skills)) { + result.skills_enabled = true; + result.skills = []; + } +} + export interface LoadAddedAgentDeps { getAgent: (searchParameter: { id: string }) => Promise; getMCPServerTools: ( @@ -95,8 +114,7 @@ export async function loadAddedAgent( } } - const modelSpecs = (appConfig?.modelSpecs as { list?: Array<{ name: string; label?: string }> }) - ?.list; + const modelSpecs = (appConfig?.modelSpecs as { list?: TModelSpec[] })?.list; const modelSpec = spec != null && spec !== '' ? modelSpecs?.find((s) => s.name === spec) : null; const sender = rest.modelLabel ?? @@ -105,14 +123,16 @@ export async function loadAddedAgent( ''; const ephemeralId = encodeEphemeralAgentId({ endpoint, model, sender, index: 1 }); - return { + const result: Record = { id: ephemeralId, instructions: promptPrefix || '', provider: endpoint, model_parameters: {}, model, tools: [...primaryAgent.tools], - } as unknown as Agent; + }; + applyModelSpecSkills(result, modelSpec); + return result as unknown as Agent; } const ephemeralAgent = rest.ephemeralAgent as @@ -127,18 +147,7 @@ export async function loadAddedAgent( const mcpServers = new Set(ephemeralAgent?.mcp); const userId = req.user?.id ?? ''; - const modelSpecs = ( - appConfig?.modelSpecs as { - list?: Array<{ - name: string; - label?: string; - mcpServers?: string[]; - executeCode?: boolean; - fileSearch?: boolean; - webSearch?: boolean; - }>; - } - )?.list; + const modelSpecs = (appConfig?.modelSpecs as { list?: TModelSpec[] })?.list; let modelSpec: (typeof modelSpecs extends Array | undefined ? T : never) | null = null; if (spec != null && spec !== '') { modelSpec = modelSpecs?.find((s) => s.name === spec) ?? null; @@ -225,6 +234,7 @@ export async function loadAddedAgent( if (ephemeralAgent?.artifacts != null && ephemeralAgent.artifacts) { result.artifacts = ephemeralAgent.artifacts; } + applyModelSpecSkills(result, modelSpec); return result as unknown as Agent; } diff --git a/packages/api/src/agents/initialize.ts b/packages/api/src/agents/initialize.ts index a7b1edbce0..ddcc528d12 100644 --- a/packages/api/src/agents/initialize.ts +++ b/packages/api/src/agents/initialize.ts @@ -70,6 +70,13 @@ function appendAdditionalInstructions(agent: Agent, text?: string | null): void .join('\n\n'); } +function getMaxCatalogSkills(req: ServerRequest): number | undefined { + const endpoints = req.config?.endpoints as + | Record + | undefined; + return endpoints?.[EModelEndpoint.agents]?.skills?.maxCatalogSkills; +} + function getToolName(tool: unknown): string | undefined { if (tool == null || typeof tool !== 'object') { return undefined; @@ -1022,6 +1029,7 @@ export async function initializeAgent( userId: req.user?.id, skillStates: params.skillStates, defaultActiveOnShare: params.defaultActiveOnShare, + maxCatalogSkills: getMaxCatalogSkills(req), }); toolDefinitions = skillResult.toolDefinitions; skillCount = skillResult.skillCount; diff --git a/packages/api/src/agents/load.ts b/packages/api/src/agents/load.ts index 66f01d9440..ec23b8e62c 100644 --- a/packages/api/src/agents/load.ts +++ b/packages/api/src/agents/load.ts @@ -135,6 +135,17 @@ export async function loadEphemeralAgent( if (ephemeralAgent?.artifacts) { result.artifacts = ephemeralAgent.artifacts; } + if (modelSpec && Object.prototype.hasOwnProperty.call(modelSpec, 'skills')) { + if (modelSpec.skills === true) { + result.skills_enabled = true; + } else if (modelSpec.skills === false) { + result.skills_enabled = false; + result.skills = []; + } else if (Array.isArray(modelSpec.skills)) { + result.skills_enabled = true; + result.skills = []; + } + } return result as Agent; } diff --git a/packages/api/src/agents/skills.ts b/packages/api/src/agents/skills.ts index 7112ee9592..188bcf4af5 100644 --- a/packages/api/src/agents/skills.ts +++ b/packages/api/src/agents/skills.ts @@ -11,10 +11,13 @@ import { registerCodeExecutionTools } from './tools'; import { logAxiosError } from '~/utils'; const SKILL_CATALOG_LIMIT = 100; +const MIN_SKILL_CATALOG_LIMIT = 1; /** Max pages scanned per run when filtering out inactive skills. */ const MAX_CATALOG_PAGES = 10; /** Page size used when paginating to fill the active-skill quota. */ const CATALOG_PAGE_SIZE = 100; +/** Hard ceiling on skill names a model spec can request by config. */ +const MAX_MODEL_SPEC_SKILLS = SKILL_CATALOG_LIMIT; /** * Hard ceiling on skill names resolved per request via `$` popover or * `always-apply`. The popover realistically surfaces only a few per turn; @@ -123,6 +126,96 @@ export function scopeSkillIds( return accessibleSkillIds.filter((oid) => agentSet.has(oid.toString())); } +function normalizeSkillCatalogLimit(limit: number | undefined): number { + if (typeof limit !== 'number' || !Number.isFinite(limit)) { + return SKILL_CATALOG_LIMIT; + } + return Math.min(SKILL_CATALOG_LIMIT, Math.max(MIN_SKILL_CATALOG_LIMIT, Math.floor(limit))); +} + +export interface ResolveModelSpecSkillIdsParams { + /** Skill names configured on a model spec. */ + names: string[]; + /** Full VIEW-accessible skill IDs for this user before model-spec scoping. */ + accessibleSkillIds: Types.ObjectId[]; + /** DB lookup: name → skill doc constrained to the user's accessible IDs. */ + getSkillByName?: InitializeAgentDbMethods['getSkillByName']; +} + +/** + * Resolves model-spec skill names against the current user's accessible skill + * set. Config is advisory: unrecognized, inaccessible, malformed, or errored + * names are skipped with a warning so a stale model-spec entry never blocks + * the actual chat request. + */ +export async function resolveModelSpecSkillIds({ + names, + accessibleSkillIds, + getSkillByName, +}: ResolveModelSpecSkillIdsParams): Promise { + if (!names.length || accessibleSkillIds.length === 0 || !getSkillByName) { + return []; + } + + const seenNames = new Set(); + const uniqueNames: string[] = []; + for (const name of names) { + if (typeof name !== 'string') { + continue; + } + const trimmed = name.trim(); + if (!trimmed || trimmed.length > MAX_SKILL_NAME_LENGTH || seenNames.has(trimmed)) { + continue; + } + seenNames.add(trimmed); + uniqueNames.push(trimmed); + } + + let boundedNames = uniqueNames; + if (uniqueNames.length > MAX_MODEL_SPEC_SKILLS) { + logger.warn( + `[resolveModelSpecSkillIds] Truncating model spec skill list from ${uniqueNames.length} to ${MAX_MODEL_SPEC_SKILLS}.`, + ); + boundedNames = uniqueNames.slice(0, MAX_MODEL_SPEC_SKILLS); + } + + const resolved: Array = []; + for (const name of boundedNames) { + try { + const skill = await getSkillByName(name, accessibleSkillIds, { + preferModelInvocable: true, + }); + if (!skill) { + logger.warn( + `[resolveModelSpecSkillIds] Skill "${name}" not found or not accessible for this user`, + ); + resolved.push(null); + continue; + } + resolved.push(skill._id); + } catch (err) { + logger.warn( + `[resolveModelSpecSkillIds] Failed to resolve skill "${name}":`, + err instanceof Error ? err.message : err, + ); + resolved.push(null); + } + } + + const seenIds = new Set(); + return resolved.filter((id): id is Types.ObjectId => { + if (!id) { + return false; + } + const key = id.toString(); + if (seenIds.has(key)) { + return false; + } + seenIds.add(key); + return true; + }); +} + export interface ResolveAgentScopedSkillIdsParams { /** Agent being initialized. Reads `id`, `skills`, and `skills_enabled`. */ agent: Pick; @@ -137,8 +230,10 @@ export interface ResolveAgentScopedSkillIdsParams { /** * Strict opt-in resolver for per-agent skill scope. Activation requires an * explicit signal from the user or the agent author: - * - Ephemeral agent → the skills badge toggle for this conversation. - * Toggle ON = full accessible catalog; OFF = no skills. + * - Ephemeral agent → model-spec `skills` wins when configured: + * `true` = full accessible catalog, string list = scoped allowlist, + * `false` = no skills. Otherwise the skills badge toggle controls + * the full accessible catalog. * - Persisted agent → the builder's `skills_enabled` master switch. * Enabled + empty allowlist = full catalog; enabled + non-empty * allowlist = narrow to those ids; disabled (or undefined) = no skills. @@ -158,6 +253,14 @@ export function resolveAgentScopedSkillIds( return []; } if (isEphemeralAgentId(agent.id)) { + if (agent.skills_enabled === false) { + return []; + } + if (agent.skills_enabled === true) { + return Array.isArray(agent.skills) + ? scopeSkillIds(accessibleSkillIds, agent.skills) + : scopeSkillIds(accessibleSkillIds, undefined); + } return ephemeralSkillsToggle ? scopeSkillIds(accessibleSkillIds, undefined) : []; } if (agent.skills_enabled !== true) { @@ -217,6 +320,8 @@ export interface InjectSkillCatalogParams { skillStates?: Record; /** Admin-configured default for shared skills. `true` = shared skills auto-activate. */ defaultActiveOnShare?: boolean; + /** Admin-configured cap on the model-visible catalog. Defaults to 100. */ + maxCatalogSkills?: number; } export interface InjectSkillCatalogResult { @@ -268,7 +373,9 @@ export async function injectSkillCatalog( userId, skillStates, defaultActiveOnShare = false, + maxCatalogSkills, } = params; + const catalogLimit = normalizeSkillCatalogLimit(maxCatalogSkills); if (!listSkillsByAccess || accessibleSkillIds.length === 0) { return { @@ -299,7 +406,7 @@ export async function injectSkillCatalog( let pages = 0; let reachedEnd = false; - while (visibleCount < SKILL_CATALOG_LIMIT && pages < MAX_CATALOG_PAGES) { + while (visibleCount < catalogLimit && pages < MAX_CATALOG_PAGES) { const page = await listSkillsByAccess({ accessibleIds: accessibleSkillIds, limit: CATALOG_PAGE_SIZE, @@ -307,7 +414,7 @@ export async function injectSkillCatalog( }); for (const skill of page.skills) { - if (visibleCount >= SKILL_CATALOG_LIMIT) { + if (visibleCount >= catalogLimit) { break; } /** @@ -344,9 +451,9 @@ export async function injectSkillCatalog( }; } - if (!reachedEnd && visibleCount < SKILL_CATALOG_LIMIT) { + if (!reachedEnd && visibleCount < catalogLimit) { logger.warn( - `[injectSkillCatalog] Scanned ${MAX_CATALOG_PAGES} pages without filling the ${SKILL_CATALOG_LIMIT}-skill catalog. Some active skills may be excluded.`, + `[injectSkillCatalog] Scanned ${MAX_CATALOG_PAGES} pages without filling the ${catalogLimit}-skill catalog. Some active skills may be excluded.`, ); } diff --git a/packages/api/src/modelSpecs/index.ts b/packages/api/src/modelSpecs/index.ts index 4e34dd0bac..6762721967 100644 --- a/packages/api/src/modelSpecs/index.ts +++ b/packages/api/src/modelSpecs/index.ts @@ -170,18 +170,24 @@ export function sanitizeModelSpecs | null | unde return { ...modelSpecs, list: modelSpecs.list.map((modelSpec) => { - const preset = modelSpec?.preset; - if (!preset || typeof preset !== 'object') { + if (!modelSpec || typeof modelSpec !== 'object') { return modelSpec; } + const preset = modelSpec?.preset; + const sanitizedModelSpec = { ...modelSpec }; + delete (sanitizedModelSpec as { skills?: unknown }).skills; + if (!preset || typeof preset !== 'object') { + return sanitizedModelSpec; + } + const sanitizedPreset = { ...preset }; for (const field of PRIVATE_MODEL_SPEC_PRESET_FIELDS) { delete sanitizedPreset[field]; } return { - ...modelSpec, + ...sanitizedModelSpec, preset: sanitizedPreset, }; }), diff --git a/packages/api/src/modelSpecs/modelSpecs.test.ts b/packages/api/src/modelSpecs/modelSpecs.test.ts index a617cd9442..8b721bb4be 100644 --- a/packages/api/src/modelSpecs/modelSpecs.test.ts +++ b/packages/api/src/modelSpecs/modelSpecs.test.ts @@ -17,6 +17,7 @@ describe('modelSpecs helpers', () => { { name: 'guarded-spec', label: 'Guarded Spec', + skills: ['private-skill'], preset: { endpoint: EModelEndpoint.openAI, model: 'gpt-4o', @@ -32,11 +33,13 @@ describe('modelSpecs helpers', () => { ], }; - expect(sanitizeModelSpecs(modelSpecs).list[0].preset).toEqual({ + const sanitizedModelSpecs = sanitizeModelSpecs(modelSpecs); + expect(sanitizedModelSpecs.list[0].preset).toEqual({ endpoint: EModelEndpoint.openAI, model: 'gpt-4o', greeting: 'Hello', }); + expect(sanitizedModelSpecs.list[0]).not.toHaveProperty('skills'); }); it('should restore only private fields for non-enforced model specs', () => { diff --git a/packages/api/src/skills/__tests__/import.test.ts b/packages/api/src/skills/__tests__/import.test.ts index 626edbdb80..391c164d73 100644 --- a/packages/api/src/skills/__tests__/import.test.ts +++ b/packages/api/src/skills/__tests__/import.test.ts @@ -121,6 +121,37 @@ describe('parseFrontmatter', () => { }); }); + it('extracts alwaysApply: true', () => { + const raw = `---\nname: legal\ndescription: Legal rules.\nalwaysApply: true\n---\n\n# Legal body`; + expect(parseFrontmatter(raw)).toEqual({ + name: 'legal', + description: 'Legal rules.', + alwaysApply: true, + invalidBooleans: [], + }); + }); + + it('lets always-apply win when both spellings are present', () => { + const raw = `---\nname: legal\ndescription: Legal rules.\nalways-apply: false\nalwaysApply: true\n---\n\n# Legal body`; + expect(parseFrontmatter(raw).alwaysApply).toBe(false); + }); + + it('ignores invalid alwaysApply alias when canonical always-apply is valid', () => { + const raw = `---\nname: legal\ndescription: Legal rules.\nalways-apply: true\nalwaysApply: yes\n---\n\n# Legal body`; + const result = parseFrontmatter(raw); + + expect(result.alwaysApply).toBe(true); + expect(result.invalidBooleans).toEqual([]); + }); + + it('ignores invalid alwaysApply alias before a valid canonical always-apply', () => { + const raw = `---\nname: legal\ndescription: Legal rules.\nalwaysApply: yes\nalways-apply: false\n---\n\n# Legal body`; + const result = parseFrontmatter(raw); + + expect(result.alwaysApply).toBe(false); + expect(result.invalidBooleans).toEqual([]); + }); + it('flags non-boolean always-apply values as invalid (no silent drop)', () => { const raw = `---\nname: n\ndescription: d\nalways-apply: yes\n---\n\nbody`; const result = parseFrontmatter(raw); @@ -128,6 +159,13 @@ describe('parseFrontmatter', () => { expect(result.invalidBooleans).toEqual(['always-apply']); }); + it('flags non-boolean alwaysApply values as invalid (no silent drop)', () => { + const raw = `---\nname: n\ndescription: d\nalwaysApply: yes\n---\n\nbody`; + const result = parseFrontmatter(raw); + expect(result.alwaysApply).toBeUndefined(); + expect(result.invalidBooleans).toEqual(['alwaysApply']); + }); + it('does not flag always-apply when the key is absent', () => { const raw = `---\nname: n\ndescription: d\n---\n\nbody`; expect(parseFrontmatter(raw).invalidBooleans).toEqual([]); diff --git a/packages/api/src/skills/import.ts b/packages/api/src/skills/import.ts index 013a021933..c5d7493277 100644 --- a/packages/api/src/skills/import.ts +++ b/packages/api/src/skills/import.ts @@ -64,11 +64,12 @@ function parseBooleanScalar(value: string): boolean | undefined { * `packages/data-schemas/src/methods/skill.ts` covers the wire contract; * this parser only needs to hand `createSkill` the columns it populates. * - * When a known boolean field (currently just `always-apply`) is present + * When a known boolean field (currently `always-apply` plus the accepted + * `alwaysApply` alias) is present * with a value that isn't recognizable as `true`/`false`, the parser * records it on `invalidBooleans[]` so the import handler can surface * a 400 instead of silently dropping the flag. Without this signal, - * authoring mistakes like `always-apply: yes` would be lossy-converted + * authoring mistakes like `alwaysApply: yes` would be lossy-converted * to "not always-applied" and the user would never learn their * frontmatter was malformed. * @@ -94,19 +95,23 @@ export function parseFrontmatter(raw: string): { let name = ''; let description = ''; let alwaysApply: boolean | undefined; - const invalidBooleans: string[] = []; + let hasValidCanonicalAlwaysApply = false; + const invalidCanonicalAlwaysApply: string[] = []; + const invalidAliasAlwaysApply: string[] = []; for (const line of block.split('\n')) { const colon = line.indexOf(':'); if (colon === -1) { continue; } - const key = line.slice(0, colon).trim().toLowerCase(); + const rawKey = line.slice(0, colon).trim(); + const key = rawKey.toLowerCase(); const rawValue = line.slice(colon + 1).trim(); if (key === 'name') { name = unquoteYaml(rawValue); } else if (key === 'description') { description = unquoteYaml(rawValue); - } else if (key === 'always-apply') { + } else if (key === 'always-apply' || key === 'alwaysapply') { + const isCanonical = key === 'always-apply'; // Operate on the raw post-colon text (no outer `unquoteYaml`): a // line like `always-apply: "true" # note` must have its comment // stripped BEFORE unquoting. Running `unquoteYaml` on the whole @@ -122,12 +127,24 @@ export function parseFrontmatter(raw: string): { } const parsed = parseBooleanScalar(unquoteYaml(stripped)); if (parsed === undefined) { - invalidBooleans.push(key); + if (isCanonical) { + invalidCanonicalAlwaysApply.push(rawKey); + } else { + invalidAliasAlwaysApply.push(rawKey); + } } else { - alwaysApply = parsed; + if (isCanonical) { + hasValidCanonicalAlwaysApply = true; + alwaysApply = parsed; + } else if (!hasValidCanonicalAlwaysApply && invalidCanonicalAlwaysApply.length === 0) { + alwaysApply = parsed; + } } } } + const invalidBooleans = hasValidCanonicalAlwaysApply + ? invalidCanonicalAlwaysApply + : [...invalidCanonicalAlwaysApply, ...invalidAliasAlwaysApply]; return { name, description, alwaysApply, invalidBooleans }; } diff --git a/packages/data-provider/src/config.ts b/packages/data-provider/src/config.ts index 5d3404e348..238a5bde6c 100644 --- a/packages/data-provider/src/config.ts +++ b/packages/data-provider/src/config.ts @@ -572,6 +572,11 @@ export const agentsEndpointSchema = baseEndpointSchema .array(z.nativeEnum(AgentCapabilities)) .optional() .default(defaultAgentCapabilities), + skills: z + .object({ + maxCatalogSkills: z.number().int().min(1).max(100).optional(), + }) + .optional(), remoteApi: remoteApiSchema.optional(), }), ) diff --git a/packages/data-provider/src/models.ts b/packages/data-provider/src/models.ts index 85f455fe17..5b7274401c 100644 --- a/packages/data-provider/src/models.ts +++ b/packages/data-provider/src/models.ts @@ -39,6 +39,7 @@ export type TModelSpec = { executeCode?: boolean; artifacts?: string | boolean; mcpServers?: string[]; + skills?: boolean | string[]; }; export const tModelSpecSchema = z.object({ @@ -60,6 +61,7 @@ export const tModelSpecSchema = z.object({ executeCode: z.boolean().optional(), artifacts: z.union([z.string(), z.boolean()]).optional(), mcpServers: z.array(z.string()).optional(), + skills: z.union([z.boolean(), z.array(z.string())]).optional(), }); export const specsConfigSchema = z.object({ diff --git a/packages/data-provider/src/types/skills.ts b/packages/data-provider/src/types/skills.ts index 3552629a80..5d6e86a041 100644 --- a/packages/data-provider/src/types/skills.ts +++ b/packages/data-provider/src/types/skills.ts @@ -197,7 +197,7 @@ export type TCreateSkill = { body: string; frontmatter?: Partial; category?: string; - /** When `true`, the skill auto-primes into every turn (mirrors `always-apply` frontmatter). */ + /** When `true`, the skill auto-primes into every turn (mirrors always-apply frontmatter). */ alwaysApply?: boolean; }; diff --git a/packages/data-schemas/src/methods/skill.spec.ts b/packages/data-schemas/src/methods/skill.spec.ts index 9d27105c0e..0db2e69ce5 100644 --- a/packages/data-schemas/src/methods/skill.spec.ts +++ b/packages/data-schemas/src/methods/skill.spec.ts @@ -283,6 +283,8 @@ describe('skill validation helpers', () => { it('accepts always-apply as a boolean', () => { expect(validateSkillFrontmatter({ 'always-apply': true })).toEqual([]); expect(validateSkillFrontmatter({ 'always-apply': false })).toEqual([]); + expect(validateSkillFrontmatter({ alwaysApply: true })).toEqual([]); + expect(validateSkillFrontmatter({ alwaysApply: false })).toEqual([]); }); it('rejects always-apply with non-boolean values', () => { @@ -294,6 +296,11 @@ describe('skill validation helpers', () => { expect( validateSkillFrontmatter({ 'always-apply': 1 }).some((i) => i.code === 'INVALID_TYPE'), ).toBe(true); + expect( + validateSkillFrontmatter({ alwaysApply: 'yes' }).some( + (i) => i.code === 'INVALID_TYPE' && i.field === 'frontmatter.alwaysApply', + ), + ).toBe(true); }); }); @@ -1076,6 +1083,35 @@ describe('Skill CRUD methods', () => { expect(skill.alwaysApply).toBe(true); }); + it('createSkill derives alwaysApply from the camel-case frontmatter alias', async () => { + const { skill } = await methods.createSkill( + makeSkillInput({ + name: 'frontmatter-alias-on', + frontmatter: { + name: 'frontmatter-alias-on', + description: 'A small demo skill used in tests.', + alwaysApply: true, + }, + }), + ); + expect(skill.alwaysApply).toBe(true); + }); + + it('createSkill lets always-apply win when both frontmatter spellings are present', async () => { + const { skill } = await methods.createSkill( + makeSkillInput({ + name: 'frontmatter-canonical-wins', + frontmatter: { + name: 'frontmatter-canonical-wins', + description: 'A small demo skill used in tests.', + 'always-apply': false, + alwaysApply: true, + }, + }), + ); + expect(skill.alwaysApply).toBe(false); + }); + it('createSkill prefers explicit top-level alwaysApply over frontmatter', async () => { const { skill } = await methods.createSkill( makeSkillInput({ @@ -1111,6 +1147,26 @@ describe('Skill CRUD methods', () => { } }); + it('updateSkill syncs alwaysApply column from the camel-case frontmatter alias', async () => { + const { skill } = await methods.createSkill(makeSkillInput({ name: 'sync-alias-test' })); + expect(skill.alwaysApply).toBe(false); + const result = await methods.updateSkill({ + id: skill._id.toString(), + expectedVersion: skill.version, + update: { + frontmatter: { + name: 'sync-alias-test', + description: 'A small demo skill used in tests.', + alwaysApply: true, + }, + }, + }); + expect(result.status).toBe('updated'); + if (result.status === 'updated') { + expect(result.skill.alwaysApply).toBe(true); + } + }); + it('updateSkill keeps alwaysApply column untouched when the frontmatter update omits always-apply', async () => { const { skill } = await methods.createSkill( makeSkillInput({ name: 'no-flip', alwaysApply: true }), @@ -1180,6 +1236,21 @@ describe('Skill CRUD methods', () => { } }); + it('updateSkill derives alwaysApply=true from a body edit that adds `alwaysApply: true` inline', async () => { + const { skill } = await methods.createSkill(makeSkillInput({ name: 'body-enable-alias' })); + expect(skill.alwaysApply).toBe(false); + const newBody = `---\nname: body-enable-alias\ndescription: now opting in.\nalwaysApply: true\n---\n\n# Body`; + const result = await methods.updateSkill({ + id: skill._id.toString(), + expectedVersion: skill.version, + update: { body: newBody }, + }); + expect(result.status).toBe('updated'); + if (result.status === 'updated') { + expect(result.skill.alwaysApply).toBe(true); + } + }); + it('updateSkill flips alwaysApply to false when the body removes the `always-apply:` line', async () => { /* Regression for the “durable mismatch” case: a previously- always-apply skill whose SKILL.md body no longer declares the diff --git a/packages/data-schemas/src/methods/skill.ts b/packages/data-schemas/src/methods/skill.ts index 512ce90c13..22eb60fa3f 100644 --- a/packages/data-schemas/src/methods/skill.ts +++ b/packages/data-schemas/src/methods/skill.ts @@ -250,6 +250,7 @@ const ALLOWED_FRONTMATTER_KEYS = new Set([ 'user-invocable', 'disable-model-invocation', 'always-apply', + 'alwaysApply', 'model', 'effort', 'context', @@ -278,6 +279,7 @@ const FRONTMATTER_KIND: Record = { 'user-invocable': 'boolean', 'disable-model-invocation': 'boolean', 'always-apply': 'boolean', + alwaysApply: 'boolean', model: 'string', effort: ['string', 'number'], context: 'string', @@ -597,6 +599,20 @@ export function backfillDerivedFromFrontmatter< return skill; } +function getAlwaysApplyFrontmatterValue( + frontmatter: Record | undefined, +): boolean | undefined { + const canonical = frontmatter?.['always-apply']; + if (typeof canonical === 'boolean') { + return canonical; + } + const camelAlias = frontmatter?.alwaysApply; + if (typeof camelAlias === 'boolean') { + return camelAlias; + } + return undefined; +} + export type UpsertSkillFileInput = { skillId: Types.ObjectId | string; relativePath: string; @@ -679,14 +695,14 @@ type BodyAlwaysApplyResult = | { status: 'invalid' }; /** - * Extractor for the `always-apply` flag sitting inside a SKILL.md body's + * Extractor for the `always-apply` / `alwaysApply` flag sitting inside a SKILL.md body's * YAML frontmatter block. The REST edit flow lets users rewrite the full * SKILL.md text via `update.body` without a structured `frontmatter` * object, so this is the only signal we have for "user flipped - * `always-apply:` inline in their editor". + * `always-apply:` or `alwaysApply:` inline in their editor". * * Returns a discriminated union so callers can tell: - * - `absent` — no `always-apply:` key (leave column alone; could be + * - `absent` — no always-apply key (leave column alone; could be * "user removed the flag" or "user hasn't written it yet" — both * resolve to no-op). An empty value (`always-apply:` with nothing * after the colon) is also treated as absent to allow mid-edit @@ -697,7 +713,8 @@ type BodyAlwaysApplyResult = * recognizable boolean (e.g. `tru`, `yes`, `1`). Validation rejects * this rather than silently ignoring so `always-apply: tru` typos * surface as 400s instead of drifting the column from what the - * saved SKILL.md text says. + * saved SKILL.md text says. When both forms are present, the canonical + * `always-apply` form wins because existing files may already rely on it. */ function extractAlwaysApplyFromBody(body: string | undefined): BodyAlwaysApplyResult { if (typeof body !== 'string' || body.length === 0) { @@ -713,13 +730,15 @@ function extractAlwaysApplyFromBody(body: string | undefined): BodyAlwaysApplyRe return { status: 'absent' }; } const block = after.slice(0, closingIdx); + let aliasResult: BodyAlwaysApplyResult | undefined; for (const line of block.split('\n')) { const colon = line.indexOf(':'); if (colon === -1) { continue; } - const key = line.slice(0, colon).trim().toLowerCase(); - if (key !== 'always-apply') { + const key = line.slice(0, colon).trim(); + const normalizedKey = key.toLowerCase(); + if (normalizedKey !== 'always-apply' && normalizedKey !== 'alwaysapply') { continue; } // Strip the YAML inline comment BEFORE unquoting — a line like @@ -728,7 +747,12 @@ function extractAlwaysApplyFromBody(body: string | undefined): BodyAlwaysApplyRe // the comment-strip would leave `"true"` which parses as invalid. let value = stripYamlTrailingComment(line.slice(colon + 1).trim()).trim(); if (value === '') { - return { status: 'absent' }; + const result: BodyAlwaysApplyResult = { status: 'absent' }; + if (normalizedKey === 'always-apply') { + return result; + } + aliasResult = result; + continue; } if ( value.length >= 2 && @@ -739,14 +763,28 @@ function extractAlwaysApplyFromBody(body: string | undefined): BodyAlwaysApplyRe } value = value.trim(); if (value === '') { - return { status: 'absent' }; + const result: BodyAlwaysApplyResult = { status: 'absent' }; + if (normalizedKey === 'always-apply') { + return result; + } + aliasResult = result; + continue; } const lowered = value.toLowerCase(); - if (lowered === 'true') return { status: 'valid', value: true }; - if (lowered === 'false') return { status: 'valid', value: false }; - return { status: 'invalid' }; + let result: BodyAlwaysApplyResult; + if (lowered === 'true') { + result = { status: 'valid', value: true }; + } else if (lowered === 'false') { + result = { status: 'valid', value: false }; + } else { + result = { status: 'invalid' }; + } + if (normalizedKey === 'always-apply') { + return result; + } + aliasResult = result; } - return { status: 'absent' }; + return aliasResult ?? { status: 'absent' }; } /** @@ -760,9 +798,9 @@ function extractAlwaysApplyFromBody(body: string | undefined): BodyAlwaysApplyRe * * Precedence: * 1. An explicit top-level `alwaysApply` wins (caller overrides). - * 2. Otherwise, derive from `frontmatter['always-apply']` when it is - * a strict boolean. - * 3. Otherwise, parse `always-apply:` out of the SKILL.md body + * 2. Otherwise, derive from `frontmatter['always-apply']` or the accepted + * alias `frontmatter.alwaysApply` when either is a strict boolean. + * 3. Otherwise, parse `always-apply:` / `alwaysApply:` out of the SKILL.md body * frontmatter block (covers the UI edit flow that sends only * `body` without a structured `frontmatter` object). * 4. Otherwise, return `fallback` (typically `false` on create, or the @@ -782,7 +820,7 @@ function resolveAlwaysApplyFromInput( if (typeof explicit === 'boolean') { return explicit; } - const fromFrontmatter = frontmatter?.['always-apply']; + const fromFrontmatter = getAlwaysApplyFrontmatterValue(frontmatter); if (typeof fromFrontmatter === 'boolean') { return fromFrontmatter; } @@ -807,9 +845,10 @@ export function validateAlwaysApplyInBody(body: string | undefined): ValidationI if (result.status === 'invalid') { return [ { - field: 'body.frontmatter.always-apply', + field: 'body.frontmatter.alwaysApply', code: 'INVALID_TYPE', - message: '"always-apply" in SKILL.md frontmatter must be a boolean (true or false)', + message: + '"always-apply" or "alwaysApply" in SKILL.md frontmatter must be a boolean (true or false)', }, ]; } @@ -885,18 +924,19 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk higher-precedence source won't override it (see `resolveAlwaysApplyFromInput` for the cascade). A caller sending an explicit top-level `alwaysApply` or a structured - `frontmatter['always-apply']` has the body value overridden at + an always-apply value in structured `frontmatter` has the body value overridden at derivation time, so rejecting them for a typo they aren't relying on would be user-hostile. */ if ( bodyAlwaysApply?.status === 'invalid' && typeof data.alwaysApply !== 'boolean' && - typeof data.frontmatter?.['always-apply'] !== 'boolean' + getAlwaysApplyFrontmatterValue(data.frontmatter) === undefined ) { issues.push({ - field: 'body.frontmatter.always-apply', + field: 'body.frontmatter.alwaysApply', code: 'INVALID_TYPE', - message: '"always-apply" in SKILL.md frontmatter must be a boolean (true or false)', + message: + '"always-apply" or "alwaysApply" in SKILL.md frontmatter must be a boolean (true or false)', }); } const { errors, warnings } = partitionIssues(issues); @@ -1214,12 +1254,13 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk if ( bodyAlwaysApply?.status === 'invalid' && update.alwaysApply === undefined && - typeof update.frontmatter?.['always-apply'] !== 'boolean' + getAlwaysApplyFrontmatterValue(update.frontmatter) === undefined ) { issues.push({ - field: 'body.frontmatter.always-apply', + field: 'body.frontmatter.alwaysApply', code: 'INVALID_TYPE', - message: '"always-apply" in SKILL.md frontmatter must be a boolean (true or false)', + message: + '"always-apply" or "alwaysApply" in SKILL.md frontmatter must be a boolean (true or false)', }); } const { errors, warnings } = partitionIssues(issues); @@ -1258,11 +1299,11 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk /** * Keep the indexed `alwaysApply` column in sync with whatever the update * is carrying: an explicit top-level `alwaysApply` always wins; a - * structured `frontmatter` with `always-apply: true/false` is next; and - * a `body` update is scanned last for an inline `always-apply:` line + * structured `frontmatter` with `always-apply: true/false` or the + * accepted `alwaysApply` alias is next; and a `body` update is scanned last for an inline `always-apply:` line * inside the SKILL.md frontmatter block. The body path is load-bearing * for the REST edit flow — the current UI sends `body` without a - * parallel `frontmatter` object, so inline edits to `always-apply:` + * parallel `frontmatter` object, so inline edits to `always-apply:` / `alwaysApply:` * would otherwise leave the column stale and auto-priming / pin * badges would keep using the old value. * @@ -1277,7 +1318,7 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk * at each level, not the presence of the parent field. An API caller * that sends both `body` and an unrelated `frontmatter` bag (e.g. * editing category + rewriting SKILL.md in one PATCH) still gets the - * body-inline flag respected because `frontmatter['always-apply']` + * body-inline flag respected because no structured always-apply key * is absent in that payload. */ let derivedAlwaysApply: boolean | undefined; @@ -1285,7 +1326,7 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk derivedAlwaysApply = update.alwaysApply; } if (derivedAlwaysApply === undefined && update.frontmatter !== undefined) { - const fromFrontmatter = update.frontmatter['always-apply']; + const fromFrontmatter = getAlwaysApplyFrontmatterValue(update.frontmatter); if (typeof fromFrontmatter === 'boolean') { derivedAlwaysApply = fromFrontmatter; } diff --git a/packages/data-schemas/src/schema/skill.ts b/packages/data-schemas/src/schema/skill.ts index c3d30e981a..d21839a2db 100644 --- a/packages/data-schemas/src/schema/skill.ts +++ b/packages/data-schemas/src/schema/skill.ts @@ -211,7 +211,7 @@ const skillSchema: Schema = new Schema( /** * When `true`, the skill's SKILL.md body is auto-primed into every turn * without user `$` invocation or model discretion. Mirrors the - * `always-apply` YAML frontmatter field and is kept as a first-class + * always-apply YAML frontmatter field and is kept as a first-class * column so the `listAlwaysApplySkills` query at the top of every * request is an indexed lookup, not a frontmatter scan. */