diff --git a/api/app/clients/BaseClient.js b/api/app/clients/BaseClient.js index ab79b3bb53b1..1a77c8cf8245 100644 --- a/api/app/clients/BaseClient.js +++ b/api/app/clients/BaseClient.js @@ -508,6 +508,23 @@ class BaseClient { userMessage.manualSkills = skills; } } + /** + * Persist the names of skills auto-primed this turn via `always-apply` + * frontmatter so `ManualSkillPills` can render pinned-variant badges + * on the user bubble that survive reload and history render. Frozen + * at turn time (not reconstructed from `Skill.alwaysApply` at render + * time) because the flag is mutable — historical turns must keep + * their audit trail even if an admin flips `alwaysApply` off later. + */ + const alwaysApplySkillPrimes = this.options.agent?.alwaysApplySkillPrimes; + if (Array.isArray(alwaysApplySkillPrimes) && alwaysApplySkillPrimes.length > 0) { + const names = alwaysApplySkillPrimes + .map((p) => p?.name) + .filter((n) => typeof n === 'string' && n.length > 0); + if (names.length > 0) { + userMessage.alwaysAppliedSkills = names; + } + } userMessagePromise = this.saveMessageToDatabase(userMessage, saveOptions, user).catch( (err) => { logger.error('[BaseClient] Failed to save user message:', err); diff --git a/api/server/controllers/agents/__tests__/openai.spec.js b/api/server/controllers/agents/__tests__/openai.spec.js index 89aad741434d..c2b9ab993c22 100644 --- a/api/server/controllers/agents/__tests__/openai.spec.js +++ b/api/server/controllers/agents/__tests__/openai.spec.js @@ -57,6 +57,14 @@ jest.mock('@librechat/api', () => ({ getTransactionsConfig: mockGetTransactionsConfig, recordCollectedUsage: mockRecordCollectedUsage, extractManualSkills: jest.fn().mockReturnValue(undefined), + injectSkillPrimes: jest.fn().mockReturnValue({ + initialMessages: [], + indexTokenCountMap: {}, + inserted: 0, + insertIdx: -1, + alwaysApplyDropped: 0, + alwaysApplyDedupedFromManual: 0, + }), buildNonStreamingResponse: jest.fn().mockReturnValue({ id: 'resp-123' }), createOpenAIStreamTracker: jest.fn().mockReturnValue({ addText: jest.fn(), diff --git a/api/server/controllers/agents/__tests__/responses.unit.spec.js b/api/server/controllers/agents/__tests__/responses.unit.spec.js index 04c3d1e4e7a5..0dc07572aefd 100644 --- a/api/server/controllers/agents/__tests__/responses.unit.spec.js +++ b/api/server/controllers/agents/__tests__/responses.unit.spec.js @@ -53,6 +53,14 @@ jest.mock('@librechat/api', () => ({ getTransactionsConfig: mockGetTransactionsConfig, recordCollectedUsage: mockRecordCollectedUsage, extractManualSkills: jest.fn().mockReturnValue(undefined), + injectSkillPrimes: jest.fn().mockReturnValue({ + initialMessages: [], + indexTokenCountMap: {}, + inserted: 0, + insertIdx: -1, + alwaysApplyDropped: 0, + alwaysApplyDedupedFromManual: 0, + }), createToolExecuteHandler: jest.fn().mockReturnValue({ handle: jest.fn() }), // Responses API writeDone: jest.fn(), diff --git a/api/server/controllers/agents/client.js b/api/server/controllers/agents/client.js index 40673e163c05..f8e109ae73cf 100644 --- a/api/server/controllers/agents/client.js +++ b/api/server/controllers/agents/client.js @@ -28,7 +28,7 @@ const { filterMalformedContentParts, countFormattedMessageTokens, hydrateMissingIndexTokenCounts, - injectManualSkillPrimes, + injectSkillPrimes, isSkillPrimeMessage, buildSkillPrimeContentParts, } = require('@librechat/api'); @@ -773,26 +773,41 @@ class AgentClient extends BaseClient { } /** - * Phase 3 manual skill priming — injected by user via `$` popover. + * Skill priming — both manual ($ popover) and always-apply (frontmatter). * - * Splice + index-shift logic lives in `injectManualSkillPrimes` + * Splice + index-shift logic lives in `injectSkillPrimes` * (packages/api/src/agents/skills.ts) so the delicate position math - * can be unit-tested in TS without standing up AgentClient. Runs for - * both single-agent and multi-agent runs; how primes interact with - * handoff / added-convo agents' per-agent state is an agents-SDK - * concern, not this layer's to gate. + * can be unit-tested in TS without standing up AgentClient. The + * resolver enforces a combined ceiling (manual-first, always-apply + * truncated first when over cap) before reaching here; the splice + * re-applies the cap as defense-in-depth. Runs for both single- + * agent and multi-agent runs; how primes interact with handoff / + * added-convo agents' per-agent state is an agents-SDK concern, + * not this layer's to gate. */ const manualSkillPrimes = this.options.agent?.manualSkillPrimes; - if (manualSkillPrimes && manualSkillPrimes.length > 0) { - const primeResult = injectManualSkillPrimes({ + const alwaysApplySkillPrimes = this.options.agent?.alwaysApplySkillPrimes; + if ( + (manualSkillPrimes && manualSkillPrimes.length > 0) || + (alwaysApplySkillPrimes && alwaysApplySkillPrimes.length > 0) + ) { + const primeResult = injectSkillPrimes({ initialMessages, indexTokenCountMap, manualSkillPrimes, + alwaysApplySkillPrimes, }); indexTokenCountMap = primeResult.indexTokenCountMap; if (primeResult.inserted > 0) { + const manualNames = (manualSkillPrimes ?? []).map((p) => p.name); + const alwaysApplyNames = (alwaysApplySkillPrimes ?? []).map((p) => p.name); logger.debug( - `[AgentClient] Primed ${primeResult.inserted} manual skill(s) at message index ${primeResult.insertIdx}: ${manualSkillPrimes.map((p) => p.name).join(', ')}`, + `[AgentClient] Primed ${primeResult.inserted} skill(s) at message index ${primeResult.insertIdx} — manual: [${manualNames.join(', ')}], always-apply: [${alwaysApplyNames.join(', ')}]`, + ); + } + if (primeResult.alwaysApplyDropped > 0) { + logger.warn( + `[AgentClient] Dropped ${primeResult.alwaysApplyDropped} always-apply prime(s) to stay within MAX_PRIMED_SKILLS_PER_TURN.`, ); } } @@ -914,28 +929,40 @@ class AgentClient extends BaseClient { await runAgents(initialMessages); /** - * Surface a completed `skill` tool_call content part per manually- - * invoked skill so the existing `SkillCall` frontend renderer shows + * Surface a completed `skill` tool_call content part per *manually*- + * primed skill so the existing `SkillCall` frontend renderer shows * a "Skill X loaded" card on the assistant response. Applied after * the graph finishes to avoid clashing with the aggregator's own * per-step content indexing. Prepended (not appended) so cards sit * above the model's output — priming ran before the turn, the * reply follows. * - * Live streaming display of cards is handled on the user side via - * `ManualSkillPills` reading the message's `manualSkills` field; - * no separate SSE emit is needed here, and trying to stream a - * mid-run tool_call at index 0 collided with the LLM's first text - * content, while emitting at a sparse offset pushed the card below - * the reply on finalize. Post-run unshift keeps the final + * Always-apply primes intentionally do NOT emit assistant-side + * cards. `extractInvokedSkillsFromPayload` scans history for + * `skill` tool_calls and feeds `primeInvokedSkills`, which is + * Phase 3's sticky-re-prime path — that's the right behavior for + * manual (user picked `$skill` once; re-prime on every subsequent + * turn from history). For always-apply, `resolveAlwaysApplySkills` + * already re-primes every turn from fresh DB state, so persisting + * the card would cause the skill body to get primed twice per + * turn starting on turn 2. The user-facing acknowledgement for + * always-apply lives on the user bubble as the pinned + * `ManualSkillPills` row (`message.alwaysAppliedSkills`), which + * is the durable signal the user wants: "this skill auto-primes". + * + * Live streaming display of manual user-bubble pills is handled + * by `ManualSkillPills` reading `message.manualSkills`. No + * separate SSE emit is needed here; trying to stream a mid-run + * tool_call at index 0 collided with the LLM's first text + * content, while emitting at a sparse offset pushed the card + * below the reply on finalize. Post-run unshift keeps the final * responseMessage.content in the right order. */ - const primedSkills = this.options.agent?.manualSkillPrimes; - if (primedSkills && primedSkills.length > 0) { - const primeParts = buildSkillPrimeContentParts(primedSkills, { - runId: this.responseMessageId ?? 'manual-skill', - }); - this.contentParts.unshift(...primeParts); + const manualPrimed = this.options.agent?.manualSkillPrimes ?? []; + if (manualPrimed.length > 0) { + const runId = this.responseMessageId ?? 'skill-prime'; + const manualParts = buildSkillPrimeContentParts(manualPrimed, { runId }); + this.contentParts.unshift(...manualParts); } /** @deprecated Agent Chain */ diff --git a/api/server/controllers/agents/openai.js b/api/server/controllers/agents/openai.js index 93e2b2fb1be2..adb80da9af37 100644 --- a/api/server/controllers/agents/openai.js +++ b/api/server/controllers/agents/openai.js @@ -28,7 +28,7 @@ const { buildNonStreamingResponse, createOpenAIStreamTracker, createOpenAIContentAggregator, - injectManualSkillPrimes, + injectSkillPrimes, isChatCompletionValidationFailure, } = require('@librechat/api'); const { @@ -42,7 +42,7 @@ const { findAccessibleResources } = require('~/server/services/PermissionService const { getSkillToolDeps, enrichWithSkillConfigurable, - buildManualSkillPrimedIdsByName, + buildSkillPrimedIdsByName, } = require('~/server/services/Endpoints/agents/skillDeps'); const db = require('~/models'); @@ -274,6 +274,7 @@ const OpenAIChatCompletionController = async (req, res) => { getToolFilesByIds: db.getToolFilesByIds, getCodeGeneratedFiles: db.getCodeGeneratedFiles, listSkillsByAccess: db.listSkillsByAccess, + listAlwaysApplySkills: db.listAlwaysApplySkills, getSkillByName: db.getSkillByName, }, ); @@ -332,7 +333,10 @@ const OpenAIChatCompletionController = async (req, res) => { req, primaryConfig.accessibleSkillIds, undefined, - buildManualSkillPrimedIdsByName(primaryConfig.manualSkillPrimes), + buildSkillPrimedIdsByName( + primaryConfig.manualSkillPrimes, + primaryConfig.alwaysApplySkillPrimes, + ), ); }, toolEndCallback, @@ -350,17 +354,23 @@ const OpenAIChatCompletionController = async (req, res) => { let indexTokenCountMap = formatted.indexTokenCountMap; /** - * Inject manual skill primes so the model sees SKILL.md bodies for this - * turn — parity with AgentClient's chat path. OpenAI-compatible streaming - * uses its own tracker/aggregator shape, so the LibreChat-style card SSE - * events don't apply here; only the message-context part carries over. + * Inject manual + always-apply skill primes so the model sees SKILL.md + * bodies for this turn — parity with AgentClient's chat path. OpenAI- + * compatible streaming uses its own tracker/aggregator shape, so the + * LibreChat-style card SSE events don't apply here; only the + * message-context part carries over. */ const manualSkillPrimes = primaryConfig.manualSkillPrimes; - if (manualSkillPrimes && manualSkillPrimes.length > 0) { - const primeResult = injectManualSkillPrimes({ + const alwaysApplySkillPrimes = primaryConfig.alwaysApplySkillPrimes; + if ( + (manualSkillPrimes && manualSkillPrimes.length > 0) || + (alwaysApplySkillPrimes && alwaysApplySkillPrimes.length > 0) + ) { + const primeResult = injectSkillPrimes({ initialMessages: formattedMessages, indexTokenCountMap, manualSkillPrimes, + alwaysApplySkillPrimes, }); indexTokenCountMap = primeResult.indexTokenCountMap; } diff --git a/api/server/controllers/agents/responses.js b/api/server/controllers/agents/responses.js index 2ece214aa943..0727cdc69193 100644 --- a/api/server/controllers/agents/responses.js +++ b/api/server/controllers/agents/responses.js @@ -19,7 +19,7 @@ const { recordCollectedUsage, getTransactionsConfig, extractManualSkills, - injectManualSkillPrimes, + injectSkillPrimes, createToolExecuteHandler, // Responses API writeDone, @@ -51,7 +51,7 @@ const { findAccessibleResources } = require('~/server/services/PermissionService const { getSkillToolDeps, enrichWithSkillConfigurable, - buildManualSkillPrimedIdsByName, + buildSkillPrimedIdsByName, } = require('~/server/services/Endpoints/agents/skillDeps'); const db = require('~/models'); @@ -414,6 +414,7 @@ const createResponse = async (req, res) => { getToolFilesByIds: db.getToolFilesByIds, getCodeGeneratedFiles: db.getCodeGeneratedFiles, listSkillsByAccess: db.listSkillsByAccess, + listAlwaysApplySkills: db.listAlwaysApplySkills, getSkillByName: db.getSkillByName, }, ); @@ -444,17 +445,23 @@ const createResponse = async (req, res) => { let indexTokenCountMap = formatted.indexTokenCountMap; /** - * Inject manual skill primes so the model sees SKILL.md bodies for this - * turn — parity with AgentClient's chat path. The Responses API uses its - * own response-builder shape, so LibreChat-style card SSE events don't - * apply; only the message-context part carries over. + * Inject manual + always-apply skill primes so the model sees SKILL.md + * bodies for this turn — parity with AgentClient's chat path. The + * Responses API uses its own response-builder shape, so LibreChat- + * style card SSE events don't apply; only the message-context part + * carries over. */ const manualSkillPrimes = primaryConfig.manualSkillPrimes; - if (manualSkillPrimes && manualSkillPrimes.length > 0) { - const primeResult = injectManualSkillPrimes({ + const alwaysApplySkillPrimes = primaryConfig.alwaysApplySkillPrimes; + if ( + (manualSkillPrimes && manualSkillPrimes.length > 0) || + (alwaysApplySkillPrimes && alwaysApplySkillPrimes.length > 0) + ) { + const primeResult = injectSkillPrimes({ initialMessages: formattedMessages, indexTokenCountMap, manualSkillPrimes, + alwaysApplySkillPrimes, }); indexTokenCountMap = primeResult.indexTokenCountMap; } @@ -515,7 +522,10 @@ const createResponse = async (req, res) => { req, primaryConfig.accessibleSkillIds, undefined, - buildManualSkillPrimedIdsByName(primaryConfig.manualSkillPrimes), + buildSkillPrimedIdsByName( + primaryConfig.manualSkillPrimes, + primaryConfig.alwaysApplySkillPrimes, + ), ); }, toolEndCallback, @@ -688,7 +698,10 @@ const createResponse = async (req, res) => { req, primaryConfig.accessibleSkillIds, undefined, - buildManualSkillPrimedIdsByName(primaryConfig.manualSkillPrimes), + buildSkillPrimedIdsByName( + primaryConfig.manualSkillPrimes, + primaryConfig.alwaysApplySkillPrimes, + ), ); }, toolEndCallback, diff --git a/api/server/services/Endpoints/agents/initialize.js b/api/server/services/Endpoints/agents/initialize.js index b5a251c1ebc2..f15c965cf891 100644 --- a/api/server/services/Endpoints/agents/initialize.js +++ b/api/server/services/Endpoints/agents/initialize.js @@ -32,7 +32,7 @@ const { filterFilesByAgentAccess } = require('~/server/services/Files/permission const { getSkillToolDeps, enrichWithSkillConfigurable, - buildManualSkillPrimedIdsByName, + buildSkillPrimedIdsByName, } = require('./skillDeps'); const { getModelsConfig } = require('~/server/controllers/ModelController'); const { checkPermission, findAccessibleResources } = require('~/server/services/PermissionService'); @@ -194,7 +194,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { req, ctx.accessibleSkillIds, codeApiKey, - ctx.manualSkillPrimedIdsByName, + ctx.skillPrimedIdsByName, ); }, toolEndCallback, @@ -291,6 +291,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { getCodeGeneratedFiles: db.getCodeGeneratedFiles, filterFilesByAgentAccess, listSkillsByAccess: db.listSkillsByAccess, + listAlwaysApplySkills: db.listAlwaysApplySkills, getSkillByName: db.getSkillByName, }, ); @@ -298,12 +299,15 @@ 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( + /** Maps each primed skill name (manual `$` or always-apply) 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 AND relax + * the disable-model-invocation gate for skills whose body is already + * in this turn's context. */ + const skillPrimedIdsByName = buildSkillPrimedIdsByName( primaryConfig.manualSkillPrimes, + primaryConfig.alwaysApplySkillPrimes, ); agentToolContexts.set(primaryConfig.id, { agent: primaryAgent, @@ -312,7 +316,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { tool_resources: primaryConfig.tool_resources, actionsEnabled: primaryConfig.actionsEnabled, accessibleSkillIds: primaryConfig.accessibleSkillIds, - manualSkillPrimedIdsByName, + skillPrimedIdsByName, }); const agent_ids = primaryConfig.agent_ids; @@ -389,6 +393,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { getCodeGeneratedFiles: db.getCodeGeneratedFiles, filterFilesByAgentAccess, listSkillsByAccess: db.listSkillsByAccess, + listAlwaysApplySkills: db.listAlwaysApplySkills, getSkillByName: db.getSkillByName, }, ); @@ -399,7 +404,13 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { userMCPAuthMap = config.userMCPAuthMap; } - /** Store handoff agent's tool context for ON_TOOL_EXECUTE callback */ + /** Store handoff agent's tool context for ON_TOOL_EXECUTE callback. + * Handoff agents get the same `skillPrimedIdsByName` plumbing as the + * primary so `read_file` can pin same-name collisions to the exact + * primed doc AND relax the `disable-model-invocation: true` gate for + * skills whose body is already in this turn's context — matters for + * handoff agents that have their own always-apply skills bound or + * that the user `$`-invokes within the handoff flow. */ agentToolContexts.set(agentId, { agent, toolRegistry: config.toolRegistry, @@ -407,6 +418,10 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { tool_resources: config.tool_resources, actionsEnabled: config.actionsEnabled, accessibleSkillIds: config.accessibleSkillIds, + skillPrimedIdsByName: buildSkillPrimedIdsByName( + config.manualSkillPrimes, + config.alwaysApplySkillPrimes, + ), }); agentConfigs.set(agentId, config); diff --git a/api/server/services/Endpoints/agents/skillDeps.js b/api/server/services/Endpoints/agents/skillDeps.js index ba55a58b3600..1cd1352dc04a 100644 --- a/api/server/services/Endpoints/agents/skillDeps.js +++ b/api/server/services/Endpoints/agents/skillDeps.js @@ -6,24 +6,54 @@ const { enrichWithSkillConfigurable } = require('@librechat/api'); const db = require('~/models'); /** - * Builds the `manualSkillPrimedIdsByName` map passed through to + * Builds the `skillPrimedIdsByName` 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. + * Combines both manual (`$`-popover) primes AND always-apply primes so + * `read_file` can: + * - Relax the `disable-model-invocation: true` gate for either source + * (the body is already in context; blocking its own files would be + * nonsensical). + * - Pin same-name collision lookups to the exact `_id` the resolver + * primed (otherwise a newer same-name duplicate could shadow the + * body/file pair within a single turn). + * + * On the rare overlap (a name appears in both arrays because upstream + * dedup was skipped), manual wins — manual invocation is explicit user + * intent and carries the authoritative `_id` for this turn. + * + * Returns `undefined` (not `{}`) when both arrays are empty, 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 + * @param {Array<{ name: string, _id: { toString(): string } }> | undefined} alwaysApplySkillPrimes * @returns {Record | undefined} */ -function buildManualSkillPrimedIdsByName(manualSkillPrimes) { - if (!manualSkillPrimes?.length) { +function buildSkillPrimedIdsByName(manualSkillPrimes, alwaysApplySkillPrimes) { + const manualCount = manualSkillPrimes?.length ?? 0; + const alwaysApplyCount = alwaysApplySkillPrimes?.length ?? 0; + if (manualCount === 0 && alwaysApplyCount === 0) { return undefined; } - return Object.fromEntries(manualSkillPrimes.map((p) => [p.name, p._id.toString()])); + const out = {}; + /* Order matters on the edge case where the same name appears in both + lists: always-apply goes in first, then manual overwrites — manual + wins because it's explicit user intent for this turn. */ + if (alwaysApplyCount > 0) { + for (const p of alwaysApplySkillPrimes) { + out[p.name] = p._id.toString(); + } + } + if (manualCount > 0) { + for (const p of manualSkillPrimes) { + out[p.name] = p._id.toString(); + } + } + return out; } /** Skill-related properties for ToolExecuteOptions (stable references, allocated once). */ @@ -49,7 +79,7 @@ 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`. + * @param {Record} [skillPrimedIdsByName] - Map of name → skill id for skills primed this turn (manual `$`-popover invocation OR always-apply). Pins same-name collision lookups in `read_file` and relaxes the disable-model-invocation gate for the primed doc. * @returns {Promise} Augmented result with skill configurable */ function enrichConfigurable( @@ -57,7 +87,7 @@ function enrichConfigurable( req, accessibleSkillIds, preResolvedCodeApiKey, - manualSkillPrimedIdsByName, + skillPrimedIdsByName, ) { return enrichWithSkillConfigurable( result, @@ -65,12 +95,12 @@ function enrichConfigurable( accessibleSkillIds, loadAuthValues, preResolvedCodeApiKey, - manualSkillPrimedIdsByName, + skillPrimedIdsByName, ); } module.exports = { getSkillToolDeps, enrichWithSkillConfigurable: enrichConfigurable, - buildManualSkillPrimedIdsByName, + buildSkillPrimedIdsByName, }; diff --git a/client/src/components/Chat/Messages/Content/Container.tsx b/client/src/components/Chat/Messages/Content/Container.tsx index 6a34016c48da..00d28b053646 100644 --- a/client/src/components/Chat/Messages/Content/Container.tsx +++ b/client/src/components/Chat/Messages/Content/Container.tsx @@ -7,8 +7,13 @@ const Container = ({ children, message }: { children: React.ReactNode; message?: className="text-message flex min-h-[20px] flex-col items-start gap-3 overflow-visible [.text-message+&]:mt-5" dir="auto" > - {message?.isCreatedByUser === true && } - {message?.isCreatedByUser === true && } + {message?.isCreatedByUser === true && ( + <> + + + + + )} {children} ); diff --git a/client/src/components/Chat/Messages/Content/ManualSkillPills.tsx b/client/src/components/Chat/Messages/Content/ManualSkillPills.tsx index de5fcd6322e1..7e56b048c4c0 100644 --- a/client/src/components/Chat/Messages/Content/ManualSkillPills.tsx +++ b/client/src/components/Chat/Messages/Content/ManualSkillPills.tsx @@ -1,37 +1,62 @@ import { memo } from 'react'; -import { ScrollText } from 'lucide-react'; +import { Pin, ScrollText } from 'lucide-react'; import { useLocalize } from '~/hooks'; +/** + * Origin tag driving the pill variant: + * - `'manual'` — the user invoked this skill via the `$` popover. + * - `'always-apply'` — the skill auto-primed because its frontmatter flag + * is set. Adds a pin icon so the ambient priming is visible to the user. + * + * Exported so message-render code can import and pass the appropriate + * value without hardcoding string literals. + */ +export type ManualSkillPillsSource = 'manual' | 'always-apply'; + /** * Compact pill row rendered on a submitted user message, one chip per skill - * the user invoked via the `$` popover. Presentational component — takes - * only the scalar `skills` array, no full message object (keeps - * `React.memo` comparisons on parent wrappers shallow and cheap). + * primed into the turn. Presentational component — takes only the scalar + * `skills` array (no full message object) so `React.memo` comparisons on + * parent wrappers stay shallow and cheap. * - * Backend persists the source field (`message.manualSkills`), so callers - * reading from the message pass `skills={message.manualSkills}` and pills - * survive page reloads / history renders. + * Backend persists the source field (`message.manualSkills` / + * `message.alwaysAppliedSkills`), so callers reading from the message pass + * `skills={message.manualSkills}` / `skills={message.alwaysAppliedSkills}` + * and pills survive page reloads / history renders. The `source` prop picks + * the icon variant so both flavors render from the same component. */ -function ManualSkillPills({ skills }: { skills?: string[] }) { +function ManualSkillPills({ + skills, + source = 'manual', +}: { + skills?: string[]; + source?: ManualSkillPillsSource; +}) { const localize = useLocalize(); if (!skills || skills.length === 0) { return null; } + const ariaLabelKey = + source === 'always-apply' + ? 'com_ui_skills_always_apply_invoked' + : 'com_ui_skills_manual_invoked'; + return ( -
+
{skills.map((name) => ( - ))} diff --git a/client/src/components/Chat/Messages/Content/__tests__/ManualSkillPills.test.tsx b/client/src/components/Chat/Messages/Content/__tests__/ManualSkillPills.test.tsx index 8066c507f3f9..a39e95e310b4 100644 --- a/client/src/components/Chat/Messages/Content/__tests__/ManualSkillPills.test.tsx +++ b/client/src/components/Chat/Messages/Content/__tests__/ManualSkillPills.test.tsx @@ -27,8 +27,43 @@ describe('ManualSkillPills', () => { expect(items[1]).toHaveTextContent('pptx'); }); - it('localizes the list aria-label', () => { + it('localizes the list aria-label (manual default)', () => { render(); expect(screen.getByRole('list')).toHaveAttribute('aria-label', 'com_ui_skills_manual_invoked:'); }); + + it('tags each pill with data-skill-source="manual" by default', () => { + render(); + const items = screen.getAllByRole('listitem'); + expect(items[0]).toHaveAttribute('data-skill-source', 'manual'); + }); + + it('switches aria-label and data attribute for source="always-apply"', () => { + render(); + expect(screen.getByRole('list')).toHaveAttribute( + 'aria-label', + 'com_ui_skills_always_apply_invoked:', + ); + const items = screen.getAllByRole('listitem'); + expect(items[0]).toHaveAttribute('data-skill-source', 'always-apply'); + }); + + it('renders the pin-icon variant for always-apply (no ScrollText)', () => { + const { container: alwaysApply } = render( + , + ); + const { container: manual } = render(); + // lucide-react icons render as SVGs with distinguishing class names. We + // assert on class token presence rather than the full SVG markup so a + // lucide internal change to path data doesn't break this contract. + const alwaysApplySvg = alwaysApply.querySelector('svg'); + const manualSvg = manual.querySelector('svg'); + expect(alwaysApplySvg).toBeTruthy(); + expect(manualSvg).toBeTruthy(); + // Pin vs. ScrollText have distinct lucide class names; both carry the + // cyan-500 accent the component applies, but only one carries the + // lucide-pin class. + expect(alwaysApplySvg?.getAttribute('class')).toContain('lucide-pin'); + expect(manualSvg?.getAttribute('class')).not.toContain('lucide-pin'); + }); }); diff --git a/client/src/components/Messages/ContentRender.tsx b/client/src/components/Messages/ContentRender.tsx index bb7925cf1a1f..da40bec10476 100644 --- a/client/src/components/Messages/ContentRender.tsx +++ b/client/src/components/Messages/ContentRender.tsx @@ -79,7 +79,8 @@ function areContentRenderPropsEqual(prev: ContentRenderProps, next: ContentRende prevMsg.iconURL === nextMsg.iconURL && prevMsg.feedback?.rating === nextMsg.feedback?.rating && (prevMsg.attachments?.length ?? 0) === (nextMsg.attachments?.length ?? 0) && - (prevMsg.manualSkills?.length ?? 0) === (nextMsg.manualSkills?.length ?? 0) + (prevMsg.manualSkills?.length ?? 0) === (nextMsg.manualSkills?.length ?? 0) && + (prevMsg.alwaysAppliedSkills?.length ?? 0) === (nextMsg.alwaysAppliedSkills?.length ?? 0) ); } diff --git a/client/src/components/Skills/display/SkillDetailHeader.tsx b/client/src/components/Skills/display/SkillDetailHeader.tsx index 405e4ce54a57..91de3ec6fb83 100644 --- a/client/src/components/Skills/display/SkillDetailHeader.tsx +++ b/client/src/components/Skills/display/SkillDetailHeader.tsx @@ -1,7 +1,7 @@ import { format } from 'date-fns'; import { useNavigate } from 'react-router-dom'; import { Button, TooltipAnchor } from '@librechat/client'; -import { Pencil, User, Calendar, EarthIcon, Sparkles } from 'lucide-react'; +import { Pencil, Pin, User, Calendar, EarthIcon, Sparkles } from 'lucide-react'; import { InvocationMode } from 'librechat-data-provider'; import type { TSkill } from 'librechat-data-provider'; import type { TranslationKeys } from '~/hooks'; @@ -48,6 +48,18 @@ const SkillDetailHeader = ({ skill, showActions = true }: SkillDetailHeaderProps } /> )} + {skill.alwaysApply === true && ( + + } + /> + )}
{skill.description && (

{skill.description}

diff --git a/client/src/components/Skills/lists/SkillListItem.tsx b/client/src/components/Skills/lists/SkillListItem.tsx index 923f2a1243b3..9976082dfbbf 100644 --- a/client/src/components/Skills/lists/SkillListItem.tsx +++ b/client/src/components/Skills/lists/SkillListItem.tsx @@ -1,10 +1,11 @@ import { memo, useState, useMemo, useCallback } from 'react'; -import { ScrollText, ChevronDown, ChevronRight, Folder } from 'lucide-react'; +import { ScrollText, ChevronDown, ChevronRight, Folder, Pin } from 'lucide-react'; import { useNavigate } from 'react-router-dom'; import { FixedSizeTree } from 'react-vtree'; import type { FixedSizeNodeData, TreeWalkerValue, TreeWalker } from 'react-vtree'; import type { TSkill, TSkillFile } from 'librechat-data-provider'; import { useListSkillFilesQuery } from '~/data-provider'; +import { useLocalize } from '~/hooks'; import { cn } from '~/utils'; interface SkillListItemProps { @@ -261,6 +262,7 @@ function SkillListItem({ onToggleExpand, }: SkillListItemProps) { const navigate = useNavigate(); + const localize = useLocalize(); // Fetch files for active skill (always, since cached fileCount may be stale) // or expanded skills. The response is small (metadata only, no content). @@ -320,8 +322,14 @@ function SkillListItem({ - + {skill.name} + {skill.alwaysApply === true && ( + + )} {hasFiles && ( diff --git a/client/src/locales/en/translation.json b/client/src/locales/en/translation.json index 63924100cd65..b1abf8ce8e87 100644 --- a/client/src/locales/en/translation.json +++ b/client/src/locales/en/translation.json @@ -1520,6 +1520,8 @@ "com_ui_skills_allow_share": "Allow sharing Skills", "com_ui_skills_allow_share_public": "Allow sharing Skills publicly", "com_ui_skills_allow_use": "Allow using Skills", + "com_ui_skills_always_apply_invoked": "Auto-applied skills", + "com_ui_skills_always_apply_pin_title": "Always-applied skill (auto-primed on every turn)", "com_ui_skills_command_placeholder": "Select a Skill by name", "com_ui_skills_empty": "No skills yet", "com_ui_skills_load_error": "Failed to load skills", diff --git a/packages/api/src/agents/__tests__/skills.test.ts b/packages/api/src/agents/__tests__/skills.test.ts index aa9fbbd4f6ce..85fedf2eec11 100644 --- a/packages/api/src/agents/__tests__/skills.test.ts +++ b/packages/api/src/agents/__tests__/skills.test.ts @@ -34,12 +34,16 @@ import { injectSkillCatalog, buildSkillPrimeMessage, resolveManualSkills, + resolveAlwaysApplySkills, injectManualSkillPrimes, + injectSkillPrimes, extractManualSkills, isSkillPrimeMessage, buildSkillPrimeContentParts, unionPrimeAllowedTools, MAX_MANUAL_SKILLS, + MAX_ALWAYS_APPLY_SKILLS, + MAX_PRIMED_SKILLS_PER_TURN, } from '../skills'; import { extractInvokedSkillsFromPayload } from '../run'; @@ -1116,6 +1120,7 @@ describe('injectManualSkillPrimes', () => { expect(primed.additional_kwargs).toEqual({ isMeta: true, source: 'skill', + trigger: 'manual', skillName: 'brand', }); }); @@ -1359,3 +1364,424 @@ describe('unionPrimeAllowedTools', () => { expect(result.extraToolNames).toEqual(['z-tool', 'm-tool', 'a-tool', 'b-tool']); }); }); + +describe('resolveAlwaysApplySkills', () => { + const userId = new Types.ObjectId().toString(); + const userOid = new Types.ObjectId(userId); + const otherAuthor = new Types.ObjectId(); + + type AlwaysApplyRow = { + _id: Types.ObjectId; + name: string; + body: string; + author: Types.ObjectId | string; + allowedTools?: string[]; + }; + + const mkRow = ( + name: string, + author: Types.ObjectId, + body = `body of ${name}`, + ): AlwaysApplyRow => ({ + _id: new Types.ObjectId(), + name, + body, + author, + }); + + /** Single-page lister — emits every row on page 1, then signals `has_more: false`. */ + const buildLister = (rows: AlwaysApplyRow[]) => + jest.fn().mockResolvedValue({ skills: rows, has_more: false, after: null }); + + /** + * Multi-page lister driven by a cursor string. Each `page` is a + * slice of rows emitted on the matching cursor call, with `has_more` + * / `after` set automatically so the resolver advances through the + * pages in order. + */ + const buildPagedLister = (pages: AlwaysApplyRow[][]) => + jest.fn().mockImplementation(async (args: { cursor?: string | null }) => { + const idx = args.cursor ? Number(args.cursor) : 0; + const skills = pages[idx] ?? []; + const has_more = idx < pages.length - 1; + return { skills, has_more, after: has_more ? String(idx + 1) : null }; + }); + + it('returns empty when accessibleSkillIds is empty', async () => { + const result = await resolveAlwaysApplySkills({ + listAlwaysApplySkills: buildLister([mkRow('foo', userOid)]), + accessibleSkillIds: [], + userId, + }); + expect(result).toEqual([]); + }); + + it('resolves owned always-apply skills into the prime shape by default', async () => { + const row = mkRow('my-always', userOid, 'BODY'); + const result = await resolveAlwaysApplySkills({ + listAlwaysApplySkills: buildLister([row]), + accessibleSkillIds: [row._id], + userId, + }); + expect(result).toEqual([{ _id: row._id, name: 'my-always', body: 'BODY' }]); + }); + + it('passes allowedTools through when the row declares them', async () => { + const row: AlwaysApplyRow = { + ...mkRow('with-tools', userOid, 'body'), + allowedTools: ['execute_code'], + }; + const result = await resolveAlwaysApplySkills({ + listAlwaysApplySkills: buildLister([row]), + accessibleSkillIds: [row._id], + userId, + }); + expect(result).toEqual([ + { _id: row._id, name: 'with-tools', body: 'body', allowedTools: ['execute_code'] }, + ]); + }); + + it('filters shared always-apply skills when defaultActiveOnShare is false', async () => { + const shared = mkRow('shared', otherAuthor); + const result = await resolveAlwaysApplySkills({ + listAlwaysApplySkills: buildLister([shared]), + accessibleSkillIds: [shared._id], + userId, + defaultActiveOnShare: false, + }); + expect(result).toEqual([]); + }); + + it('allows shared always-apply skills when defaultActiveOnShare is true', async () => { + const shared = mkRow('shared-on', otherAuthor, 'shared-body'); + const result = await resolveAlwaysApplySkills({ + listAlwaysApplySkills: buildLister([shared]), + accessibleSkillIds: [shared._id], + userId, + defaultActiveOnShare: true, + }); + expect(result).toEqual([{ _id: shared._id, name: 'shared-on', body: 'shared-body' }]); + }); + + it('honors explicit deactivation override even for owned skills', async () => { + const owned = mkRow('owned-off', userOid); + const result = await resolveAlwaysApplySkills({ + listAlwaysApplySkills: buildLister([owned]), + accessibleSkillIds: [owned._id], + userId, + skillStates: { [owned._id.toString()]: false }, + }); + expect(result).toEqual([]); + }); + + it('skips rows with empty bodies (priming nothing adds no value)', async () => { + const empty = mkRow('empty', userOid, ''); + const result = await resolveAlwaysApplySkills({ + listAlwaysApplySkills: buildLister([empty]), + accessibleSkillIds: [empty._id], + userId, + }); + expect(result).toEqual([]); + }); + + it('issues a paginated fetch with a roomy page size (not the active budget as the DB cap)', async () => { + const row = mkRow('n1', userOid); + const lister = buildLister([row]); + await resolveAlwaysApplySkills({ + listAlwaysApplySkills: lister, + accessibleSkillIds: [row._id], + userId, + }); + // Page size must be larger than MAX_ALWAYS_APPLY_SKILLS so a single + // page normally suffices; using the budget itself as the DB cap would + // starve the active-state filter when early rows are inactive. + const call = lister.mock.calls[0][0] as { limit: number }; + expect(call.limit).toBeGreaterThan(MAX_ALWAYS_APPLY_SKILLS); + }); + + it('respects a caller-supplied maxAlwaysApplySkills cap on the number of active primes', async () => { + const rows = Array.from({ length: 8 }, (_, i) => mkRow(`n${i}`, userOid)); + const lister = buildLister(rows); + const result = await resolveAlwaysApplySkills({ + listAlwaysApplySkills: lister, + accessibleSkillIds: rows.map((r) => r._id), + userId, + maxAlwaysApplySkills: 3, + }); + expect(result).toHaveLength(3); + expect(result.map((r) => r.name)).toEqual(['n0', 'n1', 'n2']); + }); + + it('stops paginating once maxAlwaysApplySkills active primes are collected', async () => { + const page1 = Array.from({ length: 5 }, (_, i) => mkRow(`p1-${i}`, userOid)); + const page2 = Array.from({ length: 5 }, (_, i) => mkRow(`p2-${i}`, userOid)); + const lister = buildPagedLister([page1, page2]); + const result = await resolveAlwaysApplySkills({ + listAlwaysApplySkills: lister, + accessibleSkillIds: [...page1, ...page2].map((r) => r._id), + userId, + maxAlwaysApplySkills: 3, + }); + expect(result).toHaveLength(3); + // Only needed the first page's first 3 rows — never asked for page 2. + expect(lister).toHaveBeenCalledTimes(1); + }); + + it('returns [] when maxAlwaysApplySkills is 0 (no-op short-circuit before DB)', async () => { + const lister = buildLister([mkRow('n1', userOid)]); + const result = await resolveAlwaysApplySkills({ + listAlwaysApplySkills: lister, + accessibleSkillIds: [new Types.ObjectId()], + userId, + maxAlwaysApplySkills: 0, + }); + expect(result).toEqual([]); + expect(lister).not.toHaveBeenCalled(); + }); + + it('paginates across inactive-for-user rows to fill the active budget (no silent starvation)', async () => { + // Page 1 is all shared-inactive (defaultActiveOnShare: false). + const inactivePage = Array.from({ length: 5 }, (_, i) => mkRow(`shared-${i}`, otherAuthor)); + // Page 2 has owned (always-active) rows that should backfill the budget. + const ownedPage = Array.from({ length: 3 }, (_, i) => mkRow(`owned-${i}`, userOid)); + const lister = buildPagedLister([inactivePage, ownedPage]); + const result = await resolveAlwaysApplySkills({ + listAlwaysApplySkills: lister, + accessibleSkillIds: [...inactivePage, ...ownedPage].map((r) => r._id), + userId, + defaultActiveOnShare: false, + maxAlwaysApplySkills: 5, + }); + expect(result.map((r) => r.name)).toEqual(['owned-0', 'owned-1', 'owned-2']); + expect(lister).toHaveBeenCalledTimes(2); + }); + + it('dedupes duplicate-named always-apply skills (keeps first/freshest by DB sort)', async () => { + // Same `name` but distinct `_id` — mimics two authors in the same + // tenant shipping skills with matching names that both ended up in + // the user's accessible set. DB sort puts the row returned first + // as the "fresher" one (updatedAt desc). + const fresh = mkRow('shared-name', userOid, 'FRESH BODY'); + const stale = mkRow('shared-name', otherAuthor, 'STALE BODY'); + const lister = buildLister([fresh, stale]); + const result = await resolveAlwaysApplySkills({ + listAlwaysApplySkills: lister, + accessibleSkillIds: [fresh._id, stale._id], + userId, + defaultActiveOnShare: true, + }); + expect(result).toEqual([{ _id: fresh._id, name: 'shared-name', body: 'FRESH BODY' }]); + }); + + it('dedupes across page boundaries, not just within a single page', async () => { + const page1 = [mkRow('dup', userOid, 'FIRST')]; + const page2 = [mkRow('dup', userOid, 'SECOND')]; + const lister = buildPagedLister([page1, page2]); + const result = await resolveAlwaysApplySkills({ + listAlwaysApplySkills: lister, + accessibleSkillIds: [page1[0]._id, page2[0]._id], + userId, + maxAlwaysApplySkills: 5, + }); + expect(result).toEqual([{ _id: page1[0]._id, name: 'dup', body: 'FIRST' }]); + }); + + it('stops after MAX_ALWAYS_APPLY_PAGES even when no active row is found', async () => { + const inactivePage = Array.from({ length: 20 }, (_, i) => mkRow(`shared-${i}`, otherAuthor)); + // 12 inactive pages — loop must cap out at MAX_ALWAYS_APPLY_PAGES (10) + // rather than scanning indefinitely. + const pages = Array.from({ length: 12 }, () => inactivePage); + const lister = buildPagedLister(pages); + const result = await resolveAlwaysApplySkills({ + listAlwaysApplySkills: lister, + accessibleSkillIds: inactivePage.map((r) => r._id), + userId, + defaultActiveOnShare: false, + }); + expect(result).toEqual([]); + expect(lister).toHaveBeenCalledTimes(10); + }); + + it('terminates early when has_more is false even below the active budget', async () => { + const page = [mkRow('solo', userOid)]; + const lister = buildPagedLister([page]); + await resolveAlwaysApplySkills({ + listAlwaysApplySkills: lister, + accessibleSkillIds: [page[0]._id], + userId, + }); + expect(lister).toHaveBeenCalledTimes(1); + }); + + it('swallows lister errors and returns [] so a DB blip does not block the turn', async () => { + const lister = jest.fn().mockRejectedValue(new Error('db down')); + const result = await resolveAlwaysApplySkills({ + listAlwaysApplySkills: lister, + accessibleSkillIds: [new Types.ObjectId()], + userId, + }); + expect(result).toEqual([]); + }); +}); + +describe('injectSkillPrimes', () => { + const manual = (name: string, body: string) => ({ name, body }); + const always = (name: string, body: string) => ({ name, body }); + + it('splices both lists with always-apply first, manual last (closer to user msg)', () => { + const userMsg = new HumanMessage('what next?'); + const messages = [userMsg]; + const result = injectSkillPrimes({ + initialMessages: messages, + indexTokenCountMap: undefined, + manualSkillPrimes: [manual('brand', 'brand-body')], + alwaysApplySkillPrimes: [always('legal', 'legal-body')], + }); + expect(result.inserted).toBe(2); + expect(messages).toHaveLength(3); + expect(messages[0].content).toBe('legal-body'); + expect(messages[1].content).toBe('brand-body'); + expect(messages[2]).toBe(userMsg); + }); + + it('tags triggers distinctly on each primed HumanMessage', () => { + const messages = [new HumanMessage('hello')]; + injectSkillPrimes({ + initialMessages: messages, + indexTokenCountMap: undefined, + manualSkillPrimes: [manual('m', 'm-body')], + alwaysApplySkillPrimes: [always('a', 'a-body')], + }); + const alwaysPrime = messages[0] as HumanMessage; + const manualPrime = messages[1] as HumanMessage; + expect(alwaysPrime.additional_kwargs.trigger).toBe('always-apply'); + expect(manualPrime.additional_kwargs.trigger).toBe('manual'); + }); + + it('is a no-op when both lists are empty/undefined', () => { + const messages = [new HumanMessage('hi')]; + const map = { 0: 7 }; + const result = injectSkillPrimes({ + initialMessages: messages, + indexTokenCountMap: map, + }); + expect(result.inserted).toBe(0); + expect(result.insertIdx).toBe(-1); + expect(messages).toHaveLength(1); + expect(result.indexTokenCountMap).toBe(map); + }); + + it('truncates always-apply first when combined total exceeds maxPrimesPerTurn', () => { + const messages = [new HumanMessage('user')]; + const manualSkillPrimes = [manual('m1', 'm1'), manual('m2', 'm2')]; + const alwaysApplySkillPrimes = [always('a1', 'a1'), always('a2', 'a2'), always('a3', 'a3')]; + const result = injectSkillPrimes({ + initialMessages: messages, + indexTokenCountMap: undefined, + manualSkillPrimes, + alwaysApplySkillPrimes, + maxPrimesPerTurn: 3, // total 5, cap 3 → 1 always-apply survives + }); + expect(result.inserted).toBe(3); + expect(result.alwaysApplyDropped).toBe(2); + // Ordering: [a1, m1, m2, user] + expect(messages.map((m) => (m as HumanMessage).content)).toEqual(['a1', 'm1', 'm2', 'user']); + }); + + it('preserves all manual primes when the cap is below their count (budget clamped to 0)', () => { + const messages = [new HumanMessage('user')]; + const manualSkillPrimes = Array.from({ length: 5 }, (_, i) => manual(`m${i}`, `m${i}`)); + const alwaysApplySkillPrimes = [always('a1', 'a1'), always('a2', 'a2')]; + const result = injectSkillPrimes({ + initialMessages: messages, + indexTokenCountMap: undefined, + manualSkillPrimes, + alwaysApplySkillPrimes, + maxPrimesPerTurn: 3, + }); + // Manual (5) already exceeds cap (3); budget for always-apply is 0. + expect(result.alwaysApplyDropped).toBe(2); + // All 5 manual primes still land (cap defense is manual-preserving; upstream + // resolver is responsible for capping manual to MAX_MANUAL_SKILLS before here). + expect(result.inserted).toBe(5); + const contents = messages.map((m) => (m as HumanMessage).content); + expect(contents).toEqual(['m0', 'm1', 'm2', 'm3', 'm4', 'user']); + }); + + it('uses MAX_PRIMED_SKILLS_PER_TURN as the default combined cap', () => { + const messages = [new HumanMessage('user')]; + // Land exactly 1 over the cap so we can observe the drop + const overCapCount = MAX_PRIMED_SKILLS_PER_TURN + 1; + const alwaysApplySkillPrimes = Array.from({ length: overCapCount }, (_, i) => + always(`a${i}`, `a${i}`), + ); + const result = injectSkillPrimes({ + initialMessages: messages, + indexTokenCountMap: undefined, + alwaysApplySkillPrimes, + }); + expect(result.alwaysApplyDropped).toBe(1); + expect(result.inserted).toBe(MAX_PRIMED_SKILLS_PER_TURN); + }); + + it('shifts indexTokenCountMap for combined splices', () => { + const messages = [new HumanMessage('user-0'), new HumanMessage('user-1')]; + const result = injectSkillPrimes({ + initialMessages: messages, + indexTokenCountMap: { 0: 3, 1: 5 }, + manualSkillPrimes: [manual('m', 'm-body')], + alwaysApplySkillPrimes: [always('a', 'a-body')], + }); + // insertIdx = 1, numPrimes = 2 → entry at idx 1 moves to idx 3 + expect(result.indexTokenCountMap).toEqual({ 0: 3, 3: 5 }); + }); + + it('drops an always-apply prime whose name is already in the manual list (no double-prime)', () => { + const messages = [new HumanMessage('user')]; + const result = injectSkillPrimes({ + initialMessages: messages, + indexTokenCountMap: undefined, + manualSkillPrimes: [manual('legal', 'manual-body')], + alwaysApplySkillPrimes: [always('legal', 'always-body')], + }); + // Only the manual variant survives; same SKILL.md body never lands twice. + expect(result.inserted).toBe(1); + expect(result.alwaysApplyDedupedFromManual).toBe(1); + expect(messages).toHaveLength(2); + expect((messages[0] as HumanMessage).content).toBe('manual-body'); + expect((messages[0] as HumanMessage).additional_kwargs.trigger).toBe('manual'); + }); + + it('dedups only the overlapping name and keeps disjoint always-apply primes', () => { + const messages = [new HumanMessage('user')]; + const result = injectSkillPrimes({ + initialMessages: messages, + indexTokenCountMap: undefined, + manualSkillPrimes: [manual('shared', 'shared-manual')], + alwaysApplySkillPrimes: [always('shared', 'shared-always'), always('distinct', 'dist-body')], + }); + expect(result.inserted).toBe(2); + expect(result.alwaysApplyDedupedFromManual).toBe(1); + // Order: always-apply first (distinct), then manual (shared), then user. + const contents = messages.map((m) => (m as HumanMessage).content); + expect(contents).toEqual(['dist-body', 'shared-manual', 'user']); + }); + + it('dedups before applying the combined cap so the cap reflects real primes', () => { + const messages = [new HumanMessage('user')]; + const manualSkillPrimes = [manual('shared', 'mb')]; + // Two always-apply entries, one of which overlaps manual. After dedup: + // manual(1) + always-apply(1) = 2, well under the cap — no warn-level drop. + const alwaysApplySkillPrimes = [always('shared', 'ab'), always('ambient', 'amb-body')]; + const result = injectSkillPrimes({ + initialMessages: messages, + indexTokenCountMap: undefined, + manualSkillPrimes, + alwaysApplySkillPrimes, + maxPrimesPerTurn: 3, + }); + expect(result.alwaysApplyDedupedFromManual).toBe(1); + expect(result.alwaysApplyDropped).toBe(0); + expect(result.inserted).toBe(2); + }); +}); diff --git a/packages/api/src/agents/handlers.spec.ts b/packages/api/src/agents/handlers.spec.ts index 77fb42274923..06069b4e458d 100644 --- a/packages/api/src/agents/handlers.spec.ts +++ b/packages/api/src/agents/handlers.spec.ts @@ -281,7 +281,7 @@ describe('createToolExecuteHandler', () => { expect(callOptions).not.toHaveProperty('preferUserInvocable', true); }); - it('read_file uses preferModelInvocable for AUTONOMOUS probes (skill not in manualSkillPrimedIdsByName)', async () => { + it('read_file uses preferModelInvocable for AUTONOMOUS probes (skill not in skillPrimedIdsByName)', async () => { const getSkillByName = jest.fn(async () => ({ _id: 'skill-id' as unknown as never, name: 'maybe-disabled-read', @@ -312,7 +312,7 @@ describe('createToolExecuteHandler', () => { 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 + (its `_id` is in `skillPrimedIdsByName`). 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 @@ -329,7 +329,7 @@ describe('createToolExecuteHandler', () => { loadTools: jest.fn(async () => ({ loadedTools: [], configurable: { - manualSkillPrimedIdsByName: { 'manually-primed': primedHex }, + skillPrimedIdsByName: { 'manually-primed': primedHex }, }, })), getSkillByName, @@ -423,7 +423,7 @@ describe('createToolExecuteHandler', () => { 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. */ + scoped to the per-turn `skillPrimedIdsByName` allowlist. */ const getSkillByName = jest.fn(async () => ({ _id: '507f1f77bcf86cd799439020' as unknown as never, name: 'manual-only-skill', @@ -435,7 +435,7 @@ describe('createToolExecuteHandler', () => { loadTools: jest.fn(async () => ({ loadedTools: [], configurable: { - manualSkillPrimedIdsByName: { 'manual-only-skill': '507f1f77bcf86cd799439020' }, + skillPrimedIdsByName: { 'manual-only-skill': '507f1f77bcf86cd799439020' }, }, })), getSkillByName, @@ -455,7 +455,7 @@ describe('createToolExecuteHandler', () => { 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 + specific names in `skillPrimedIdsByName`. A model trying to read a different disabled skill (one the user never manually invoked) is still rejected. */ const getSkillByName = jest.fn(async () => ({ @@ -469,7 +469,7 @@ describe('createToolExecuteHandler', () => { loadTools: jest.fn(async () => ({ loadedTools: [], configurable: { - manualSkillPrimedIdsByName: { 'something-else': '507f1f77bcf86cd799439030' }, + skillPrimedIdsByName: { 'something-else': '507f1f77bcf86cd799439030' }, }, })), getSkillByName, @@ -486,5 +486,89 @@ describe('createToolExecuteHandler', () => { expect(result.status).toBe('error'); expect(result.errorMessage).toContain('cannot be invoked by the model'); }); + + it('relaxes the disable-model gate for always-apply primes the same way it does for manual', async () => { + /* Regression: always-apply skills landed in `skillPrimedIdsByName` + alongside manual primes, so a `disable-model-invocation: true` + skill that auto-primes via always-apply must be able to read + its own bundled files. Without this, a team's auto-primed + "model-only" skill (e.g. legal boilerplate) would silently + degrade the first time it referenced `references/foo.md`. */ + const getSkillByName = jest.fn(async () => ({ + _id: '507f1f77bcf86cd799439040' as unknown as never, + name: 'always-applied-legal', + body: '# Cite references/policy.md when advising', + fileCount: 0, + disableModelInvocation: true, + })); + const handler = createToolExecuteHandler({ + loadTools: jest.fn(async () => ({ + loadedTools: [], + configurable: { + /* Map includes the always-apply skill because `buildSkillPrimedIdsByName` + now combines both prime sources. */ + skillPrimedIdsByName: { + 'always-applied-legal': '507f1f77bcf86cd799439040', + }, + }, + })), + getSkillByName, + }); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_read_always', + name: Constants.READ_FILE, + args: { file_path: 'always-applied-legal/SKILL.md' }, + }, + ]); + + expect(result.status).toBe('success'); + expect(result.content).toContain('references/policy.md'); + }); + + it('pins accessibleIds to the primed _id for an always-apply skill (no same-name shadowing)', async () => { + /* Same-name collision: two skills share a name, one got primed via + always-apply. read_file must resolve to the exact primed doc so + the body and file lookup stay consistent within a turn. */ + const { Types } = jest.requireActual('mongoose') as typeof import('mongoose'); + const primedHex = '507f1f77bcf86cd799439050'; + const getSkillByName = jest.fn(async () => ({ + _id: new Types.ObjectId(primedHex) as unknown as never, + name: 'collides', + body: '# primed body', + fileCount: 0, + })); + const handler = createToolExecuteHandler({ + loadTools: jest.fn(async () => ({ + loadedTools: [], + configurable: { + skillPrimedIdsByName: { collides: primedHex }, + }, + })), + getSkillByName, + }); + + await invokeHandler(handler, [ + { + id: 'call_read_pin', + name: Constants.READ_FILE, + args: { file_path: 'collides/SKILL.md' }, + }, + ]); + + const firstCall = getSkillByName.mock.calls[0] as unknown as [ + string, + Array<{ toString(): string }>, + Record, + ]; + const accessibleIdsArg = firstCall[1]; + const lookupOptions = firstCall[2]; + expect(accessibleIdsArg).toHaveLength(1); + expect(accessibleIdsArg[0].toString()).toBe(primedHex); + // Primed lookups do NOT pass preferModelInvocable — the _id pin is + // authoritative. + expect(lookupOptions).toEqual({}); + }); }); }); diff --git a/packages/api/src/agents/handlers.ts b/packages/api/src/agents/handlers.ts index 8ea44cdc046a..97d951666d81 100644 --- a/packages/api/src/agents/handlers.ts +++ b/packages/api/src/agents/handlers.ts @@ -183,26 +183,26 @@ async function handleReadFileCall( } const accessibleIds = (mergedConfigurable?.accessibleSkillIds as Types.ObjectId[]) ?? []; - 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 + const skillPrimedIdsByName = + (mergedConfigurable?.skillPrimedIdsByName as Record | undefined) ?? {}; + const primedIdString = skillPrimedIdsByName[skillName]; + const isPrimedThisTurn = primedIdString != null; + /* On a primed lookup (manual `$` OR always-apply), 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 = isPrimedThisTurn ? [new Types.ObjectId(primedIdString)] : accessibleIds; const lookupOptions: { preferUserInvocable?: boolean; preferModelInvocable?: boolean } = - isManuallyPrimedThisTurn ? {} : { preferModelInvocable: true }; + isPrimedThisTurn ? {} : { preferModelInvocable: true }; const skill = await getSkillByName(skillName, lookupAccessibleIds, lookupOptions); if (!skill) { return { @@ -217,17 +217,17 @@ 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. + * the skill was primed this turn (manual `$` invocation OR always-apply), + * the body is already in context — and a primed skill that depends on + * `references/foo.md` would be non-functional if read_file were blocked. + * Bypass the gate for primed names so this stays usable end-to-end for + * both prime sources. * * 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. + * a follow-up. Same-turn priming is the load-bearing path. */ - if (skill.disableModelInvocation === true && !isManuallyPrimedThisTurn) { + if (skill.disableModelInvocation === true && !isPrimedThisTurn) { return { toolCallId: tc.id, status: 'error', diff --git a/packages/api/src/agents/initialize.ts b/packages/api/src/agents/initialize.ts index 4ef4432ffa12..0d0b9aa95376 100644 --- a/packages/api/src/agents/initialize.ts +++ b/packages/api/src/agents/initialize.ts @@ -31,9 +31,15 @@ import { import { filterFilesByEndpointConfig } from '~/files'; import { generateArtifactsPrompt } from '~/prompts'; import { getProviderConfig } from '~/endpoints'; -import { injectSkillCatalog, resolveManualSkills, unionPrimeAllowedTools } from './skills'; +import { + injectSkillCatalog, + resolveManualSkills, + resolveAlwaysApplySkills, + unionPrimeAllowedTools, + MAX_PRIMED_SKILLS_PER_TURN, +} from './skills'; import { primeResources } from './resources'; -import type { ResolvedManualSkill } from './skills'; +import type { ResolvedManualSkill, ResolvedAlwaysApplySkill } from './skills'; import type { TFilterFilesByAgentAccess } from './resources'; /** @@ -80,6 +86,15 @@ export type InitializedAgent = Agent & { * message array — deterministic priming without a tool roundtrip. */ manualSkillPrimes?: ResolvedManualSkill[]; + /** + * Skills auto-primed this turn because their `always-apply` frontmatter + * flag is set. Resolved against the same `accessibleSkillIds` set and + * subjected to the same active-state / ACL filters as the catalog, then + * handed to the AgentClient for splicing alongside manual primes. Their + * `allowedTools` entries also union into the agent's effective tool set + * via `unionPrimeAllowedTools` (same pipeline as manual primes). + */ + alwaysApplySkillPrimes?: ResolvedAlwaysApplySkill[]; }; export const DEFAULT_MAX_CONTEXT_TOKENS = 32000; @@ -242,6 +257,28 @@ export interface InitializeAgentDbMethods extends EndpointDbMethods { */ userInvocable?: boolean; } | null>; + /** + * Load accessible skills with `alwaysApply: true`, eagerly including + * `body` so the priming pipeline can splice at turn start without a + * per-skill round-trip. Cursor-paginated so the resolver can fill its + * active-state budget even when early-sorted rows are inactive for + * the current user. + */ + listAlwaysApplySkills?: (params: { + accessibleIds: import('mongoose').Types.ObjectId[]; + limit: number; + cursor?: string | null; + }) => Promise<{ + skills: Array<{ + _id: import('mongoose').Types.ObjectId; + name: string; + body: string; + author: import('mongoose').Types.ObjectId; + allowedTools?: string[]; + }>; + has_more?: boolean; + after?: string | null; + }>; } /** @@ -395,39 +432,108 @@ export async function initializeAgent( }); /** - * 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. + * Pre-resolve manually-invoked + always-apply 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. + * + * Manual + always-apply primes feed the same `unionPrimeAllowedTools` + * call — the helper is pure / set-based, so concatenating the two + * lists gives the right union with no double-counting. Manual primes + * go first so their names win on dedup (primes earlier in the list + * contribute before the same name gets deduped on a later prime). */ + const hasSkillAccess = params.accessibleSkillIds && params.accessibleSkillIds.length > 0; let manualSkillPrimes: ResolvedManualSkill[] | undefined; + let alwaysApplySkillPrimes: ResolvedAlwaysApplySkill[] | 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) { + if (hasSkillAccess) { + const [manualPrimesResult, alwaysApplyPrimesResult] = await Promise.all([ + params.manualSkills?.length && db.getSkillByName + ? resolveManualSkills({ + names: params.manualSkills, + getSkillByName: db.getSkillByName, + accessibleSkillIds: params.accessibleSkillIds!, + userId: req.user?.id, + skillStates: params.skillStates, + defaultActiveOnShare: params.defaultActiveOnShare, + }) + : Promise.resolve(undefined), + db.listAlwaysApplySkills + ? resolveAlwaysApplySkills({ + listAlwaysApplySkills: db.listAlwaysApplySkills, + accessibleSkillIds: params.accessibleSkillIds!, + userId: req.user?.id, + skillStates: params.skillStates, + defaultActiveOnShare: params.defaultActiveOnShare, + }) + : Promise.resolve(undefined), + ]); + + manualSkillPrimes = manualPrimesResult; + alwaysApplySkillPrimes = alwaysApplyPrimesResult; + + /** + * Cross-list dedup: when a user `$`-invokes a skill that is also + * marked `always-apply`, the always-apply copy is dropped here so + * the same SKILL.md body isn't primed twice in the same turn. + * Manual wins because it sits closer to the user message and + * carries explicit intent. Done at the initializer (not just at + * splice time in `injectSkillPrimes`) so persisted user-bubble + * `alwaysAppliedSkills` pills reflect the post-dedup set and the + * tool-union step below doesn't bill allowed-tools to the dropped + * always-apply entry. + */ + if ( + alwaysApplySkillPrimes && + alwaysApplySkillPrimes.length > 0 && + manualSkillPrimes && + manualSkillPrimes.length > 0 + ) { + const manualNames = new Set(manualSkillPrimes.map((p) => p.name)); + const deduped = alwaysApplySkillPrimes.filter((p) => !manualNames.has(p.name)); + const removed = alwaysApplySkillPrimes.length - deduped.length; + if (removed > 0) { + logger.info( + `[initializeAgent] Dropped ${removed} always-apply prime(s) already present in the manual list; same-named skills prime only once per turn.`, + ); + alwaysApplySkillPrimes = deduped; + } + } + + /** + * Enforce the combined `MAX_PRIMED_SKILLS_PER_TURN` ceiling up-front + * so persisted user-bubble `alwaysAppliedSkills` pills stay in sync + * with what actually gets primed. `injectSkillPrimes` re-applies the + * cap as defense-in-depth at splice time. Always-apply primes are + * truncated first — manual invocation is explicit user intent and + * should never be silently dropped. + */ + const manualCount = manualSkillPrimes?.length ?? 0; + const alwaysApplyCount = alwaysApplySkillPrimes?.length ?? 0; + if (alwaysApplySkillPrimes && manualCount + alwaysApplyCount > MAX_PRIMED_SKILLS_PER_TURN) { + const budgetForAlwaysApply = Math.max(0, MAX_PRIMED_SKILLS_PER_TURN - manualCount); + const dropped = alwaysApplyCount - budgetForAlwaysApply; + logger.warn( + `[initializeAgent] Combined primes (${manualCount} manual + ${alwaysApplyCount} always-apply) exceeds MAX_PRIMED_SKILLS_PER_TURN (${MAX_PRIMED_SKILLS_PER_TURN}); truncating ${dropped} always-apply prime(s) so persisted user-message pills stay in sync with what got primed.`, + ); + alwaysApplySkillPrimes = alwaysApplySkillPrimes.slice(0, budgetForAlwaysApply); + } + + const primesForUnion = [...(manualSkillPrimes ?? []), ...(alwaysApplySkillPrimes ?? [])]; + if (primesForUnion.length > 0) { const union = unionPrimeAllowedTools({ - primes: manualSkillPrimes, + primes: primesForUnion, agentToolNames: agent.tools ?? [], }); extraAllowedToolNames = union.extraToolNames; @@ -694,6 +800,7 @@ export async function initializeAgent( skillCount, accessibleSkillIds: executableSkillIds, manualSkillPrimes, + alwaysApplySkillPrimes, attachments: finalAttachments, toolContextMap: toolContextMap ?? {}, useLegacyContent: !!options.useLegacyContent, diff --git a/packages/api/src/agents/skillConfigurable.ts b/packages/api/src/agents/skillConfigurable.ts index de7e79008dd3..960d390a52f2 100644 --- a/packages/api/src/agents/skillConfigurable.ts +++ b/packages/api/src/agents/skillConfigurable.ts @@ -5,13 +5,15 @@ 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). + * `skillPrimedIdsByName` maps each primed skill name (manual `$` or + * always-apply) to the `_id` of the exact doc whose body was primed into + * the turn. Skill-tool handlers consult it to: + * 1. Relax the `disable-model-invocation` gate on `read_file` for any + * primed skill (so a `disable-model-invocation: true` skill whose + * body is in context can still load its `references/*` / `scripts/*` + * files). Without this, a user manually invoking — or auto-priming + * — a disabled skill would get a body that references files the + * model is forbidden to open. * 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 @@ -31,11 +33,11 @@ export async function enrichWithSkillConfigurable( /** 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. + * `{ [skillName]: skillIdString }` for every skill primed this turn + * (manual or always-apply). The id pins same-name collision lookups to + * the exact doc the resolver primed and relaxes the disable-model gate. */ - manualSkillPrimedIdsByName?: Record, + skillPrimedIdsByName?: Record, ): Promise<{ loadedTools: unknown[]; configurable: Record }> { let codeApiKey: string | undefined = preResolvedCodeApiKey; if (!codeApiKey) { @@ -59,7 +61,7 @@ export async function enrichWithSkillConfigurable( req, codeApiKey, accessibleSkillIds, - manualSkillPrimedIdsByName, + skillPrimedIdsByName, }, }; } diff --git a/packages/api/src/agents/skills.ts b/packages/api/src/agents/skills.ts index 1a6c642471e0..9a8745befda6 100644 --- a/packages/api/src/agents/skills.ts +++ b/packages/api/src/agents/skills.ts @@ -25,6 +25,23 @@ const CATALOG_PAGE_SIZE = 100; */ export const MAX_MANUAL_SKILLS = 10; +/** + * Hard ceiling on `always-apply` skills primed per turn. Larger than + * `MAX_MANUAL_SKILLS` because these are admin / author curated (not + * per-request user input), so the defensive-payload concern is weaker — + * but still bounded so a pathological team config can't push dozens of + * skill bodies into every turn. + */ +export const MAX_ALWAYS_APPLY_SKILLS = 20; + +/** + * Combined hard ceiling applied in `injectSkillPrimes`. When the total + * (manual + always-apply) exceeds this, always-apply gets truncated + * first — manual invocation is explicit user intent and should never + * be silently dropped. + */ +export const MAX_PRIMED_SKILLS_PER_TURN = 30; + /** * Hard ceiling on individual skill name length. Real skill names are * short slugs (e.g. `pptx`, `brand-guidelines`); anything beyond this is @@ -42,6 +59,27 @@ export const MAX_SKILL_NAME_LENGTH = 200; */ export const SKILL_MESSAGE_SOURCE = 'skill'; +/** + * Discriminator tag on a skill prime message that records *why* the skill + * was primed into the turn. Stored on `additional_kwargs.trigger` of the + * `HumanMessage` produced by `injectSkillPrimes`; surfaced on UI + * (pill variants) and accessible to downstream filtering / telemetry. + */ +export const SKILL_TRIGGER_MANUAL = 'manual'; +/** + * Reserved for the model-invoked path (runtime catalog → tool-call + * resolution). Declared here so the `SkillTrigger` union and the pill + * UI already speak in terms of three sources rather than growing a + * fourth value later. + */ +export const SKILL_TRIGGER_MODEL = 'model'; +export const SKILL_TRIGGER_ALWAYS_APPLY = 'always-apply'; + +export type SkillTrigger = + | typeof SKILL_TRIGGER_MANUAL + | typeof SKILL_TRIGGER_MODEL + | typeof SKILL_TRIGGER_ALWAYS_APPLY; + /** * Predicate that identifies a LangChain message as one we spliced in via * `injectManualSkillPrimes` (or the equivalent model-invoked path). Callers @@ -424,7 +462,16 @@ export interface ResolveManualSkillsParams { defaultActiveOnShare?: boolean; } -export interface ResolvedManualSkill { +/** + * Canonical shape of a skill resolved into prime-ready form. Both + * manual-invocation (`$` popover) and `always-apply` resolvers emit this + * shape so downstream pipeline stages (`injectSkillPrimes`, + * `unionPrimeAllowedTools`, `buildSkillPrimedIdsByName`) can treat either + * source uniformly. The per-prime distinction lives on + * `additional_kwargs.trigger` of the spliced `HumanMessage` (see + * `SkillTrigger`), not on this resolver output. + */ +export interface ResolvedSkillPrime { /** * `_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 @@ -444,6 +491,20 @@ export interface ResolvedManualSkill { allowedTools?: string[]; } +/** + * Back-compat alias for manual-invocation primes (`$` popover). Semantic + * aliases over `ResolvedSkillPrime` keep the per-source naming at call + * sites (so `manualPrimes: ResolvedManualSkill[]` stays readable) without + * maintaining parallel interfaces. + */ +export type ResolvedManualSkill = ResolvedSkillPrime; + +/** + * Back-compat alias for always-apply primes (auto-applied every turn). + * See `ResolvedSkillPrime` for the canonical definition. + */ +export type ResolvedAlwaysApplySkill = ResolvedSkillPrime; + /** * Resolves user-provided skill names to `{ name, body }` pairs ready for * priming. Filters out: @@ -578,6 +639,182 @@ export async function resolveManualSkills( return resolved.filter((r): r is ResolvedManualSkill => r !== null); } +export interface ResolveAlwaysApplySkillsParams { + /** + * Paginated DB lookup for accessible skills with `alwaysApply: true`, + * eagerly loaded with `body` and optional `allowedTools`. Scoped to + * `accessibleIds` (post-`scopeSkillIds`). The resolver pages until the + * active-state budget is filled so inactive early rows cannot starve + * the prime catalog. + */ + listAlwaysApplySkills: (params: { + accessibleIds: Types.ObjectId[]; + limit: number; + cursor?: string | null; + }) => Promise<{ + skills: Array<{ + _id: Types.ObjectId; + name: string; + body: string; + author: Types.ObjectId | string; + allowedTools?: string[]; + }>; + has_more?: boolean; + after?: string | null; + }>; + /** ACL-accessible skill IDs for this user (already scoped by `scopeSkillIds`). */ + accessibleSkillIds: Types.ObjectId[]; + /** Current user ID — required for ownership-based active-state defaults. */ + userId?: string; + /** Per-user skill active/inactive overrides. */ + skillStates?: Record; + /** Admin-configured default for shared skills. */ + defaultActiveOnShare?: boolean; + /** Override cap on the number of always-apply primes to resolve. Defaults to `MAX_ALWAYS_APPLY_SKILLS`. */ + maxAlwaysApplySkills?: number; +} + +/** + * Page size used by `resolveAlwaysApplySkills` when paginating the DB + * listing. Sized comfortably above `MAX_ALWAYS_APPLY_SKILLS` so a single + * page is enough in the overwhelmingly common case, but the resolver + * still pages when the catalog is dominated by rows that are inactive + * for this user. + */ +const ALWAYS_APPLY_PAGE_SIZE = 50; + +/** + * Max pages scanned when filling the active always-apply budget. Bounds + * worst-case DB load for pathological configs (tens of thousands of + * shared-inactive always-apply skills) without silently truncating the + * active budget when the first page happens to land on inactive rows. + */ +const MAX_ALWAYS_APPLY_PAGES = 10; + +/** + * Resolves accessible skills with `alwaysApply: true` into prime-ready + * form. Mirrors `resolveManualSkills`' contract on purpose so the two feed + * the same `injectSkillPrimes` + `unionPrimeAllowedTools` pipeline. + * + * Paginates the DB listing until the active budget is filled (or we hit + * `MAX_ALWAYS_APPLY_PAGES`). Applying the budget pre-filter would silently + * starve the prime catalog whenever early-sorted rows happen to be + * inactive for this user (e.g. shared skills with + * `defaultActiveOnShare: false`, or explicit `skillStates` overrides). + * + * Name-level dedup across pages — the schema's uniqueness index is + * `(name, author, tenantId)`, so two authors in the same tenant can + * ship skills with the same `name` shared with a third user. First + * occurrence wins (DB sort is `updatedAt` desc, so freshest definition). + */ +export async function resolveAlwaysApplySkills( + params: ResolveAlwaysApplySkillsParams, +): Promise { + const { + listAlwaysApplySkills, + accessibleSkillIds, + userId, + skillStates, + defaultActiveOnShare, + maxAlwaysApplySkills = MAX_ALWAYS_APPLY_SKILLS, + } = params; + + if (accessibleSkillIds.length === 0 || maxAlwaysApplySkills <= 0) { + return []; + } + + const resolved: ResolvedAlwaysApplySkill[] = []; + const seenNames = new Set(); + let cursor: string | null = null; + let pages = 0; + let reachedEnd = false; + let inactiveSkipped = 0; + let duplicateNameSkipped = 0; + + while (resolved.length < maxAlwaysApplySkills && pages < MAX_ALWAYS_APPLY_PAGES) { + let page: Awaited>; + try { + page = await listAlwaysApplySkills({ + accessibleIds: accessibleSkillIds, + limit: ALWAYS_APPLY_PAGE_SIZE, + cursor, + }); + } catch (err) { + logger.warn( + '[resolveAlwaysApplySkills] listAlwaysApplySkills failed:', + err instanceof Error ? err.message : err, + ); + return resolved; + } + + for (const skill of page.skills) { + if (resolved.length >= maxAlwaysApplySkills) { + break; + } + if (!skill.body) { + logger.warn(`[resolveAlwaysApplySkills] Skill "${skill.name}" has empty body — skipping`); + continue; + } + const active = resolveSkillActive({ + skill: { _id: skill._id, author: skill.author }, + skillStates, + userId, + defaultActiveOnShare, + }); + if (!active) { + /** + * Intentionally no per-skill log: `defaultActiveOnShare: false` + * makes inactive-for-user rows an *expected* outcome on every + * turn, and with pagination (up to MAX_ALWAYS_APPLY_PAGES × page + * size rows) logging each one floods the telemetry and buries + * real issues. Aggregated count surfaces once below when non-zero. + */ + inactiveSkipped += 1; + continue; + } + if (seenNames.has(skill.name)) { + duplicateNameSkipped += 1; + continue; + } + seenNames.add(skill.name); + const prime: ResolvedAlwaysApplySkill = { + _id: skill._id, + name: skill.name, + body: skill.body, + }; + if (skill.allowedTools !== undefined) { + prime.allowedTools = skill.allowedTools; + } + resolved.push(prime); + } + + if (!page.has_more || !page.after) { + reachedEnd = true; + break; + } + cursor = page.after; + pages += 1; + } + + if (inactiveSkipped > 0) { + logger.debug( + `[resolveAlwaysApplySkills] Skipped ${inactiveSkipped} always-apply skill(s) inactive for this user.`, + ); + } + if (duplicateNameSkipped > 0) { + logger.warn( + `[resolveAlwaysApplySkills] Skipped ${duplicateNameSkipped} duplicate-named always-apply skill(s); kept the most-recently-updated copy of each name.`, + ); + } + if (!reachedEnd && resolved.length < maxAlwaysApplySkills) { + logger.warn( + `[resolveAlwaysApplySkills] Scanned ${MAX_ALWAYS_APPLY_PAGES} page(s) without filling the ${maxAlwaysApplySkills}-prime budget. Some active always-apply skills may be excluded.`, + ); + } + + return resolved; +} + export interface InjectManualSkillPrimesParams { /** Formatted LangChain messages produced by `formatAgentMessages`. Mutated in place. */ initialMessages: BaseMessage[]; @@ -619,6 +856,12 @@ export interface InjectManualSkillPrimesResult { * * Callers are responsible for scoping (e.g. single-agent runs only — see * `AgentClient.chatCompletion`). This helper is agent-agnostic. + * + * @deprecated Use {@link injectSkillPrimes} instead. That function accepts + * both manual and always-apply primes, applies cross-list dedup, and + * enforces the combined `MAX_PRIMED_SKILLS_PER_TURN` ceiling. Retained here + * for backward compatibility with external consumers of + * `@librechat/api` that import the manual-only splicer directly. */ export function injectManualSkillPrimes( params: InjectManualSkillPrimesParams, @@ -646,7 +889,12 @@ export function injectManualSkillPrimes( (p) => new HumanMessage({ content: p.body, - additional_kwargs: { isMeta: true, source: SKILL_MESSAGE_SOURCE, skillName: p.name }, + additional_kwargs: { + isMeta: true, + source: SKILL_MESSAGE_SOURCE, + trigger: SKILL_TRIGGER_MANUAL, + skillName: p.name, + }, }), ); initialMessages.splice(insertIdx, 0, ...primeMessages); @@ -654,6 +902,143 @@ export function injectManualSkillPrimes( return { initialMessages, indexTokenCountMap, inserted: numPrimes, insertIdx }; } +export interface InjectSkillPrimesParams { + /** Formatted LangChain messages produced by `formatAgentMessages`. Mutated in place. */ + initialMessages: BaseMessage[]; + /** Per-index token count map returned by `formatAgentMessages`. */ + indexTokenCountMap: Record | undefined; + /** Resolved manual-invocation primes ($-popover). */ + manualSkillPrimes?: Pick[]; + /** Resolved `always-apply` primes (frontmatter-driven, auto-applied every turn). */ + alwaysApplySkillPrimes?: Pick[]; + /** + * Combined ceiling on primes per turn. Defaults to + * `MAX_PRIMED_SKILLS_PER_TURN`. When the sum of `manualSkillPrimes` + + * `alwaysApplySkillPrimes` exceeds the cap, always-apply primes are + * truncated first — manual invocation is explicit user intent and must + * never be silently dropped. + */ + maxPrimesPerTurn?: number; +} + +export interface InjectSkillPrimesResult { + initialMessages: BaseMessage[]; + indexTokenCountMap: Record | undefined; + inserted: number; + insertIdx: number; + alwaysApplyDropped: number; + /** + * Count of always-apply primes dropped because the same skill name already + * appears in the manual list — dedup prevents the same SKILL.md body from + * being spliced in twice in one turn. + */ + alwaysApplyDedupedFromManual: number; +} + +/** + * Splices manual + always-apply skill prime messages into a formatted + * message array just before the latest user message. Ordering: always-apply + * primes first (further from the user message, ambient context), then + * manual primes (closer to the user message, explicit user intent). More + * recent context gets more attention in most LLMs, so we want explicit `$` + * picks landing closest to the latest user turn and ambient priming + * sitting further back. Shifts `indexTokenCountMap` for the combined + * splice. + * + * Cross-list dedup: if a user `$`-invokes a skill that is also marked + * `always-apply`, the always-apply copy is dropped so the SKILL.md body + * is primed only once. Manual wins (drops the always-apply side) because + * manual primes sit closer to the user message and carry explicit intent. + * + * Enforces a combined ceiling (`maxPrimesPerTurn`, default + * `MAX_PRIMED_SKILLS_PER_TURN`) by truncating always-apply first so + * manual is never silently dropped. Dedup runs before the cap so the + * cap reflects the real prime count, not the pre-dedup total. + */ +export function injectSkillPrimes(params: InjectSkillPrimesParams): InjectSkillPrimesResult { + const { + initialMessages, + manualSkillPrimes = [], + alwaysApplySkillPrimes = [], + maxPrimesPerTurn = MAX_PRIMED_SKILLS_PER_TURN, + } = params; + let { indexTokenCountMap } = params; + + let alwaysApply = alwaysApplySkillPrimes; + let alwaysApplyDedupedFromManual = 0; + if (alwaysApply.length > 0 && manualSkillPrimes.length > 0) { + const manualNames = new Set(manualSkillPrimes.map((p) => p.name)); + const deduped = alwaysApply.filter((p) => !manualNames.has(p.name)); + alwaysApplyDedupedFromManual = alwaysApply.length - deduped.length; + if (alwaysApplyDedupedFromManual > 0) { + logger.info( + `[injectSkillPrimes] Dropped ${alwaysApplyDedupedFromManual} always-apply prime(s) already present in the manual list; same-named skills are primed only once per turn.`, + ); + alwaysApply = deduped; + } + } + + let alwaysApplyDropped = 0; + const total = manualSkillPrimes.length + alwaysApply.length; + if (total > maxPrimesPerTurn) { + const budgetForAlwaysApply = Math.max(0, maxPrimesPerTurn - manualSkillPrimes.length); + alwaysApplyDropped = alwaysApply.length - budgetForAlwaysApply; + alwaysApply = alwaysApply.slice(0, budgetForAlwaysApply); + logger.warn( + `[injectSkillPrimes] Combined primes ${total} exceeds cap ${maxPrimesPerTurn}; dropping ${alwaysApplyDropped} always-apply prime(s) to preserve manual invocations.`, + ); + } + + const numPrimes = manualSkillPrimes.length + alwaysApply.length; + if (numPrimes === 0 || initialMessages.length === 0) { + return { + initialMessages, + indexTokenCountMap, + inserted: 0, + insertIdx: -1, + alwaysApplyDropped, + alwaysApplyDedupedFromManual, + }; + } + + const insertIdx = initialMessages.length - 1; + + if (indexTokenCountMap) { + const shifted: Record = {}; + for (const [idxStr, count] of Object.entries(indexTokenCountMap)) { + const idx = Number(idxStr); + shifted[idx >= insertIdx ? idx + numPrimes : idx] = count; + } + indexTokenCountMap = shifted; + } + + const buildPrime = (p: { name: string; body: string }, trigger: SkillTrigger): HumanMessage => + new HumanMessage({ + content: p.body, + additional_kwargs: { + isMeta: true, + source: SKILL_MESSAGE_SOURCE, + trigger, + skillName: p.name, + }, + }); + + const primeMessages: HumanMessage[] = [ + ...alwaysApply.map((p) => buildPrime(p, SKILL_TRIGGER_ALWAYS_APPLY)), + ...manualSkillPrimes.map((p) => buildPrime(p, SKILL_TRIGGER_MANUAL)), + ]; + initialMessages.splice(insertIdx, 0, ...primeMessages); + + return { + initialMessages, + indexTokenCountMap, + inserted: numPrimes, + insertIdx, + alwaysApplyDropped, + alwaysApplyDedupedFromManual, + }; +} + export interface SkillPrimeContentPart { type: 'tool_call'; tool_call: { diff --git a/packages/api/src/skills/__tests__/import.test.ts b/packages/api/src/skills/__tests__/import.test.ts new file mode 100644 index 000000000000..271da6d81c71 --- /dev/null +++ b/packages/api/src/skills/__tests__/import.test.ts @@ -0,0 +1,127 @@ +import { parseFrontmatter } from '../import'; + +describe('parseFrontmatter', () => { + it('extracts name + description from a minimal frontmatter block', () => { + const raw = `---\nname: demo\ndescription: A demo skill.\n---\n\n# Body`; + expect(parseFrontmatter(raw)).toEqual({ + name: 'demo', + description: 'A demo skill.', + alwaysApply: undefined, + invalidBooleans: [], + }); + }); + + it('extracts always-apply: true', () => { + const raw = `---\nname: legal\ndescription: Legal rules.\nalways-apply: true\n---\n\n# Legal body`; + expect(parseFrontmatter(raw)).toEqual({ + name: 'legal', + description: 'Legal rules.', + alwaysApply: true, + invalidBooleans: [], + }); + }); + + it('extracts always-apply: false', () => { + const raw = `---\nname: optional\ndescription: Optional rules.\nalways-apply: false\n---\n\nOptional body`; + expect(parseFrontmatter(raw)).toEqual({ + name: 'optional', + description: 'Optional rules.', + alwaysApply: false, + invalidBooleans: [], + }); + }); + + 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); + expect(result.alwaysApply).toBeUndefined(); + expect(result.invalidBooleans).toEqual(['always-apply']); + }); + + 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([]); + }); + + it('does not flag always-apply when the value is an empty string (treated as absent)', () => { + const raw = `---\nname: n\ndescription: d\nalways-apply:\n---\n\nbody`; + const result = parseFrontmatter(raw); + expect(result.alwaysApply).toBeUndefined(); + expect(result.invalidBooleans).toEqual([]); + }); + + it('is case-insensitive on the key but strict on the value', () => { + const raw = `---\nname: n\ndescription: d\nALWAYS-APPLY: TRUE\n---\n\nbody`; + expect(parseFrontmatter(raw).alwaysApply).toBe(true); + }); + + it('handles quoted values correctly', () => { + const raw = `---\nname: "quoted-name"\ndescription: 'quoted desc'\nalways-apply: "true"\n---\n\nbody`; + expect(parseFrontmatter(raw)).toEqual({ + name: 'quoted-name', + description: 'quoted desc', + alwaysApply: true, + invalidBooleans: [], + }); + }); + + it('returns empty fields when no frontmatter block is present', () => { + const raw = '# Just a body with no frontmatter'; + expect(parseFrontmatter(raw)).toEqual({ + name: '', + description: '', + invalidBooleans: [], + }); + }); + + it('returns empty fields when frontmatter is unterminated', () => { + const raw = `---\nname: incomplete\n`; + expect(parseFrontmatter(raw)).toEqual({ + name: '', + description: '', + invalidBooleans: [], + }); + }); + + it('ignores always-apply appearing outside the frontmatter block', () => { + const raw = `---\nname: n\ndescription: d\n---\n\nalways-apply: true (but this is in the body)`; + const result = parseFrontmatter(raw); + expect(result.alwaysApply).toBeUndefined(); + expect(result.invalidBooleans).toEqual([]); + }); + + it('tolerates a YAML inline comment after the boolean value', () => { + const raw = `---\nname: commented\ndescription: demo.\nalways-apply: true # auto-prime every turn\n---\n\nbody`; + const result = parseFrontmatter(raw); + expect(result.alwaysApply).toBe(true); + expect(result.invalidBooleans).toEqual([]); + }); + + it('treats a comment-only always-apply value as absent (mid-edit placeholder)', () => { + const raw = `---\nname: only-comment\ndescription: demo.\nalways-apply: # nothing here yet\n---\n\nbody`; + const result = parseFrontmatter(raw); + expect(result.alwaysApply).toBeUndefined(); + expect(result.invalidBooleans).toEqual([]); + }); + + it('flags a typo value as invalid even when followed by a comment', () => { + const raw = `---\nname: typo\ndescription: demo.\nalways-apply: tru # typo\n---\n\nbody`; + const result = parseFrontmatter(raw); + expect(result.alwaysApply).toBeUndefined(); + expect(result.invalidBooleans).toEqual(['always-apply']); + }); + + it('handles a quoted boolean value followed by an inline comment', () => { + const raw = `---\nname: quoted-comment\ndescription: demo.\nalways-apply: "true" # note\n---\n\nbody`; + const result = parseFrontmatter(raw); + expect(result.alwaysApply).toBe(true); + expect(result.invalidBooleans).toEqual([]); + }); + + it('handles a single-quoted false with an inline comment', () => { + const raw = `---\nname: single-quote\ndescription: demo.\nalways-apply: 'false' # off\n---\n\nbody`; + const result = parseFrontmatter(raw); + expect(result.alwaysApply).toBe(false); + expect(result.invalidBooleans).toEqual([]); + }); +}); diff --git a/packages/api/src/skills/handlers.ts b/packages/api/src/skills/handlers.ts index 1f02e54ed41a..5b713d459659 100644 --- a/packages/api/src/skills/handlers.ts +++ b/packages/api/src/skills/handlers.ts @@ -162,6 +162,7 @@ function serializeSkill( source: skill.source, sourceMetadata: serializeSourceMetadata(skill.sourceMetadata), fileCount: skill.fileCount, + alwaysApply: skill.alwaysApply, isPublic: pub, tenantId: skill.tenantId, createdAt: (skill.createdAt ?? new Date()).toISOString(), @@ -189,6 +190,7 @@ function serializeSkillSummary( source: skill.source, sourceMetadata: serializeSourceMetadata(skill.sourceMetadata), fileCount: skill.fileCount, + alwaysApply: skill.alwaysApply, isPublic: pub, tenantId: skill.tenantId, createdAt: (skill.createdAt ?? new Date()).toISOString(), @@ -393,6 +395,7 @@ export function createSkillsHandlers(deps: SkillsHandlersDeps) { body: body.body, frontmatter: body.frontmatter as Record | undefined, category: body.category, + alwaysApply: body.alwaysApply, author: authorId, authorName, tenantId: user.tenantId, @@ -497,6 +500,7 @@ export function createSkillsHandlers(deps: SkillsHandlersDeps) { update.frontmatter = rest.frontmatter as Record; } if (rest.category !== undefined) update.category = rest.category; + if (rest.alwaysApply !== undefined) update.alwaysApply = rest.alwaysApply; if (Object.keys(update).length === 0) { return res.status(400).json({ error: 'At least one field must be provided for update' }); diff --git a/packages/api/src/skills/import.ts b/packages/api/src/skills/import.ts index ab8ba53420d3..f0223b8700ce 100644 --- a/packages/api/src/skills/import.ts +++ b/packages/api/src/skills/import.ts @@ -2,7 +2,7 @@ import crypto from 'crypto'; import path from 'path'; import JSZip from 'jszip'; import { ResourceType, AccessRoleIds, PrincipalType } from 'librechat-data-provider'; -import { logger } from '@librechat/data-schemas'; +import { logger, stripYamlTrailingComment } from '@librechat/data-schemas'; import type { Request, Response } from 'express'; import type { Types } from 'mongoose'; import type { @@ -32,34 +32,95 @@ function unquoteYaml(value: string): string { return value; } -/** YAML frontmatter parser — extracts name + description from SKILL.md. */ -function parseFrontmatter(raw: string): { name: string; description: string } { +/** + * Parse a YAML scalar as a strict boolean. Returns `undefined` when the + * value is neither `true` nor `false`. Callers should pre-strip inline + * comments with `stripYamlTrailingComment` when needed; this helper only + * normalizes case / whitespace so the call site stays one-purpose. + */ +function parseBooleanScalar(value: string): boolean | undefined { + const lowered = value.trim().toLowerCase(); + if (lowered === 'true') { + return true; + } + if (lowered === 'false') { + return false; + } + return undefined; +} + +/** + * YAML frontmatter parser — extracts the first-class fields LibreChat + * persists as columns (`name`, `description`, `alwaysApply`) out of a + * SKILL.md file. Intentionally narrow: the full frontmatter validator in + * `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 + * 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 + * to "not always-applied" and the user would never learn their + * frontmatter was malformed. + * + * Exported for unit testing only — prefer `createImportHandler` at runtime. + */ +export function parseFrontmatter(raw: string): { + name: string; + description: string; + alwaysApply?: boolean; + /** Keys that carried non-boolean values for fields that must be boolean. */ + invalidBooleans: string[]; +} { const trimmed = raw.trim(); if (!trimmed.startsWith('---')) { - return { name: '', description: '' }; + return { name: '', description: '', invalidBooleans: [] }; } const after = trimmed.slice(3); const closingIdx = after.indexOf('\n---'); if (closingIdx === -1) { - return { name: '', description: '' }; + return { name: '', description: '', invalidBooleans: [] }; } const block = after.slice(0, closingIdx); let name = ''; let description = ''; + let alwaysApply: boolean | undefined; + const invalidBooleans: 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 value = unquoteYaml(line.slice(colon + 1).trim()); + const rawValue = line.slice(colon + 1).trim(); if (key === 'name') { - name = value; + name = unquoteYaml(rawValue); } else if (key === 'description') { - description = value; + description = unquoteYaml(rawValue); + } else if (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 + // line first would miss the quoted branch (the line doesn't end + // in a quote once the comment is attached), and unquoting twice + // would be fragile if `unquoteYaml` ever gains richer YAML-escape + // handling. + const stripped = stripYamlTrailingComment(rawValue).trim(); + if (stripped === '') { + // Empty value or comment-only (`always-apply: # TBD`) — treat as + // absent so mid-edit placeholder states don't reject the save. + continue; + } + const parsed = parseBooleanScalar(unquoteYaml(stripped)); + if (parsed === undefined) { + invalidBooleans.push(key); + } else { + alwaysApply = parsed; + } } } - return { name, description }; + return { name, description, alwaysApply, invalidBooleans }; } /** Validates a relative path is safe (no traversal, no absolute paths). */ @@ -220,7 +281,17 @@ async function handleMarkdown( ) { const content = file.buffer.toString('utf-8'); - const { name, description } = parseFrontmatter(content); + const { name, description, alwaysApply, invalidBooleans } = parseFrontmatter(content); + if (invalidBooleans.length > 0) { + return res.status(400).json({ + error: 'Validation failed', + issues: invalidBooleans.map((key) => ({ + field: `frontmatter.${key}`, + code: 'INVALID_TYPE', + message: `"${key}" must be a boolean (true or false)`, + })), + }); + } const inferredName = name || file.originalname @@ -242,6 +313,7 @@ async function handleMarkdown( body: content, author: authorId, authorName, + alwaysApply, tenantId, }); @@ -317,7 +389,17 @@ async function handleZip( return res.status(400).json({ error: 'SKILL.md exceeds maximum file size' }); } - const { name, description } = parseFrontmatter(skillMdContent); + const { name, description, alwaysApply, invalidBooleans } = parseFrontmatter(skillMdContent); + if (invalidBooleans.length > 0) { + return res.status(400).json({ + error: 'Validation failed', + issues: invalidBooleans.map((key) => ({ + field: `frontmatter.${key}`, + code: 'INVALID_TYPE', + message: `"${key}" must be a boolean (true or false)`, + })), + }); + } const inferredName = name || file.originalname @@ -339,6 +421,7 @@ async function handleZip( body: skillMdContent, author: authorId, authorName, + alwaysApply, tenantId, }); diff --git a/packages/data-provider/src/schemas.ts b/packages/data-provider/src/schemas.ts index 529278b9c2ba..b8700286ed5a 100644 --- a/packages/data-provider/src/schemas.ts +++ b/packages/data-provider/src/schemas.ts @@ -689,6 +689,14 @@ export const tMessageSchema = z.object({ * content parts on the assistant message instead). */ manualSkills: z.array(z.string()).optional(), + /** + * Skill names auto-primed on this turn because their `always-apply` + * frontmatter flag is set. Persisted at turn time so the pinned-variant + * pills on the user bubble survive reload and stay stable across later + * edits to the skill's `alwaysApply` flag (the user bubble reflects + * what actually ran, not the current catalog). + */ + alwaysAppliedSkills: z.array(z.string()).optional(), }); export type MemoryArtifact = { diff --git a/packages/data-provider/src/types/skills.ts b/packages/data-provider/src/types/skills.ts index 1721224e9648..b8dd22a7f3dd 100644 --- a/packages/data-provider/src/types/skills.ts +++ b/packages/data-provider/src/types/skills.ts @@ -136,6 +136,12 @@ export type TSkill = { source: SkillSource; sourceMetadata?: SkillSourceMetadata; fileCount: number; + /** + * When `true`, the skill auto-primes into every turn — no user `$` picks + * or model discretion required. Surfaced on the list view so the UI can + * show a pin badge on rows that apply ambiently. + */ + alwaysApply?: boolean; isPublic?: boolean; tenantId?: string; createdAt: string; @@ -189,6 +195,8 @@ export type TCreateSkill = { body: string; frontmatter?: Partial; category?: string; + /** When `true`, the skill auto-primes into every turn (mirrors `always-apply` frontmatter). */ + alwaysApply?: boolean; }; /** Partial payload for PATCH `/api/skills/:id` — all fields optional. */ @@ -199,6 +207,7 @@ export type TUpdateSkillPayload = { body?: string; frontmatter?: Partial; category?: string; + alwaysApply?: boolean; }; /** Variables passed into the update mutation: id + expectedVersion + partial payload. */ diff --git a/packages/data-schemas/src/methods/skill.spec.ts b/packages/data-schemas/src/methods/skill.spec.ts index 7fb8a65f07fa..4d8ea062a9ee 100644 --- a/packages/data-schemas/src/methods/skill.spec.ts +++ b/packages/data-schemas/src/methods/skill.spec.ts @@ -12,6 +12,7 @@ import { validateSkillName, validateSkillDescription, validateSkillFrontmatter, + validateAlwaysApply, validateRelativePath, inferSkillFileCategory, deriveStructuredFrontmatterFields, @@ -276,6 +277,45 @@ describe('skill validation helpers', () => { validateSkillFrontmatter({ hooks: deep }).some((i) => i.code === 'INVALID_SHAPE'), ).toBe(true); }); + + it('accepts always-apply as a boolean', () => { + expect(validateSkillFrontmatter({ 'always-apply': true })).toEqual([]); + expect(validateSkillFrontmatter({ 'always-apply': false })).toEqual([]); + }); + + it('rejects always-apply with non-boolean values', () => { + expect( + validateSkillFrontmatter({ 'always-apply': 'yes' }).some( + (i) => i.code === 'INVALID_TYPE' && i.field === 'frontmatter.always-apply', + ), + ).toBe(true); + expect( + validateSkillFrontmatter({ 'always-apply': 1 }).some((i) => i.code === 'INVALID_TYPE'), + ).toBe(true); + }); + }); + + describe('validateAlwaysApply', () => { + it('accepts undefined and booleans (undefined = no change)', () => { + expect(validateAlwaysApply(undefined)).toEqual([]); + expect(validateAlwaysApply(true)).toEqual([]); + expect(validateAlwaysApply(false)).toEqual([]); + }); + + it('rejects null (PATCH forwards any non-undefined value to $set, and null in a boolean column is an ambiguous state)', () => { + expect(validateAlwaysApply(null).some((i) => i.code === 'INVALID_TYPE')).toBe(true); + }); + + it('rejects string payloads (defends against loosely-typed clients)', () => { + expect(validateAlwaysApply('true').some((i) => i.code === 'INVALID_TYPE')).toBe(true); + expect(validateAlwaysApply('false').some((i) => i.code === 'INVALID_TYPE')).toBe(true); + }); + + it('rejects numeric / object / array payloads', () => { + expect(validateAlwaysApply(1).some((i) => i.code === 'INVALID_TYPE')).toBe(true); + expect(validateAlwaysApply({}).some((i) => i.code === 'INVALID_TYPE')).toBe(true); + expect(validateAlwaysApply([]).some((i) => i.code === 'INVALID_TYPE')).toBe(true); + }); }); describe('deriveStructuredFrontmatterFields', () => { @@ -605,39 +645,6 @@ describe('Skill CRUD methods', () => { 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 @@ -882,6 +889,304 @@ describe('Skill CRUD methods', () => { expect(bySearch.skills.length).toBe(1); expect(bySearch.skills[0].name).toBe('beta-skill'); }); + + it('listAlwaysApplySkills returns only alwaysApply:true rows within accessibleIds', async () => { + const { skill: a } = await methods.createSkill( + makeSkillInput({ name: 'always-a', alwaysApply: true }), + ); + const { skill: b } = await methods.createSkill( + makeSkillInput({ name: 'always-b', alwaysApply: true }), + ); + const { skill: c } = await methods.createSkill( + makeSkillInput({ name: 'not-always-c' /* alwaysApply defaults false */ }), + ); + const ids = [a._id, b._id, c._id]; + const result = await methods.listAlwaysApplySkills({ + accessibleIds: ids as unknown as mongoose.Types.ObjectId[], + limit: 10, + }); + const names = result.skills.map((s) => s.name).sort(); + expect(names).toEqual(['always-a', 'always-b']); + }); + + it('listAlwaysApplySkills excludes rows outside accessibleIds', async () => { + const { skill: mine } = await methods.createSkill( + makeSkillInput({ name: 'mine-always', alwaysApply: true }), + ); + await methods.createSkill(makeSkillInput({ name: 'other-always', alwaysApply: true })); + const result = await methods.listAlwaysApplySkills({ + accessibleIds: [mine._id] as unknown as mongoose.Types.ObjectId[], + limit: 10, + }); + expect(result.skills.map((s) => s.name)).toEqual(['mine-always']); + }); + + it('listAlwaysApplySkills respects the limit and reports has_more for pagination', async () => { + const created = []; + for (let i = 0; i < 3; i++) { + const { skill } = await methods.createSkill( + makeSkillInput({ name: `always-${i}`, alwaysApply: true }), + ); + created.push(skill); + } + const result = await methods.listAlwaysApplySkills({ + accessibleIds: created.map((s) => s._id) as unknown as mongoose.Types.ObjectId[], + limit: 2, + }); + expect(result.skills).toHaveLength(2); + expect(result.has_more).toBe(true); + expect(result.after).not.toBeNull(); + }); + + it('listAlwaysApplySkills paginates via cursor to return subsequent rows without duplicates', async () => { + const created = []; + for (let i = 0; i < 5; i++) { + const { skill } = await methods.createSkill( + makeSkillInput({ name: `paged-${i}`, alwaysApply: true }), + ); + created.push(skill); + } + const ids = created.map((s) => s._id) as unknown as mongoose.Types.ObjectId[]; + const first = await methods.listAlwaysApplySkills({ accessibleIds: ids, limit: 2 }); + expect(first.skills).toHaveLength(2); + expect(first.has_more).toBe(true); + const second = await methods.listAlwaysApplySkills({ + accessibleIds: ids, + limit: 2, + cursor: first.after, + }); + expect(second.skills).toHaveLength(2); + const third = await methods.listAlwaysApplySkills({ + accessibleIds: ids, + limit: 2, + cursor: second.after, + }); + expect(third.has_more).toBe(false); + expect(third.after).toBeNull(); + const seenIds = [...first.skills, ...second.skills, ...third.skills].map((s) => + s._id.toString(), + ); + expect(new Set(seenIds).size).toBe(5); + }); + + it('createSkill persists alwaysApply first-class column', async () => { + const { skill } = await methods.createSkill( + makeSkillInput({ name: 'flagged', alwaysApply: true }), + ); + expect(skill.alwaysApply).toBe(true); + + const { skill: defaulted } = await methods.createSkill(makeSkillInput({ name: 'defaulted' })); + expect(defaulted.alwaysApply).toBe(false); + }); + + it('createSkill rejects a non-boolean top-level alwaysApply', async () => { + await expect( + methods.createSkill( + makeSkillInput({ name: 'bad-always', alwaysApply: 'false' as unknown as boolean }), + ), + ).rejects.toMatchObject({ code: 'SKILL_VALIDATION_FAILED' }); + }); + + it('updateSkill rejects a non-boolean top-level alwaysApply', async () => { + const { skill } = await methods.createSkill(makeSkillInput({ name: 'type-guard' })); + await expect( + methods.updateSkill({ + id: skill._id.toString(), + expectedVersion: skill.version, + update: { alwaysApply: 'true' as unknown as boolean }, + }), + ).rejects.toMatchObject({ code: 'SKILL_VALIDATION_FAILED' }); + }); + + it('updateSkill rejects explicit null for alwaysApply (cannot persist ambiguous state)', async () => { + const { skill } = await methods.createSkill( + makeSkillInput({ name: 'null-guard', alwaysApply: true }), + ); + await expect( + methods.updateSkill({ + id: skill._id.toString(), + expectedVersion: skill.version, + update: { alwaysApply: null as unknown as boolean }, + }), + ).rejects.toMatchObject({ code: 'SKILL_VALIDATION_FAILED' }); + }); + + it('createSkill derives alwaysApply from frontmatter when the top-level flag is absent', async () => { + const { skill } = await methods.createSkill( + makeSkillInput({ + name: 'frontmatter-on', + frontmatter: { + name: 'frontmatter-on', + description: 'A small demo skill used in tests.', + 'always-apply': true, + }, + }), + ); + expect(skill.alwaysApply).toBe(true); + }); + + it('createSkill prefers explicit top-level alwaysApply over frontmatter', async () => { + const { skill } = await methods.createSkill( + makeSkillInput({ + name: 'explicit-wins', + alwaysApply: false, + frontmatter: { + name: 'explicit-wins', + description: 'A small demo skill used in tests.', + 'always-apply': true, + }, + }), + ); + expect(skill.alwaysApply).toBe(false); + }); + + it('updateSkill syncs alwaysApply column from a frontmatter-only update', async () => { + const { skill } = await methods.createSkill(makeSkillInput({ name: 'sync-test' })); + expect(skill.alwaysApply).toBe(false); + const result = await methods.updateSkill({ + id: skill._id.toString(), + expectedVersion: skill.version, + update: { + frontmatter: { + name: 'sync-test', + description: 'A small demo skill used in tests.', + 'always-apply': 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 }), + ); + const result = await methods.updateSkill({ + id: skill._id.toString(), + expectedVersion: skill.version, + update: { + frontmatter: { name: 'no-flip', description: 'Updated desc without touching the flag.' }, + }, + }); + expect(result.status).toBe('updated'); + if (result.status === 'updated') { + expect(result.skill.alwaysApply).toBe(true); + } + }); + + it('updateSkill top-level alwaysApply wins over a frontmatter flag in the same update', async () => { + const { skill } = await methods.createSkill(makeSkillInput({ name: 'explicit-update-wins' })); + const result = await methods.updateSkill({ + id: skill._id.toString(), + expectedVersion: skill.version, + update: { + alwaysApply: false, + frontmatter: { + name: 'explicit-update-wins', + description: 'A small demo skill used in tests.', + 'always-apply': true, + }, + }, + }); + expect(result.status).toBe('updated'); + if (result.status === 'updated') { + expect(result.skill.alwaysApply).toBe(false); + } + }); + + it('updateSkill derives alwaysApply from a body edit that flips `always-apply:` inline', async () => { + const { skill } = await methods.createSkill( + makeSkillInput({ name: 'body-flip', alwaysApply: true }), + ); + const newBody = `---\nname: body-flip\ndescription: still a demo skill.\nalways-apply: false\n---\n\n# Edited 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(false); + expect(result.skill.body).toBe(newBody); + } + }); + + it('updateSkill derives alwaysApply=true from a body edit that adds `always-apply: true` inline', async () => { + const { skill } = await methods.createSkill(makeSkillInput({ name: 'body-enable' })); + expect(skill.alwaysApply).toBe(false); + const newBody = `---\nname: body-enable\ndescription: now opting in.\nalways-apply: 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 + flag must stop auto-priming. Without this, the column would + stick at `true` and the UI pin badge would persist even though + the file itself no longer opts in. */ + const { skill } = await methods.createSkill( + makeSkillInput({ name: 'body-remove', alwaysApply: true }), + ); + const bodyWithoutKey = `---\nname: body-remove\ndescription: opting out by removing the line.\n---\n\n# Body`; + const result = await methods.updateSkill({ + id: skill._id.toString(), + expectedVersion: skill.version, + update: { body: bodyWithoutKey }, + }); + expect(result.status).toBe('updated'); + if (result.status === 'updated') { + expect(result.skill.alwaysApply).toBe(false); + expect(result.skill.body).toBe(bodyWithoutKey); + } + }); + + it('updateSkill flips alwaysApply to false when the body update carries no frontmatter block at all', async () => { + /* An author rewriting SKILL.md without any YAML frontmatter is an + implicit opt-out — there is no declaration anywhere, so the + column should reflect that. */ + const { skill } = await methods.createSkill( + makeSkillInput({ name: 'body-strip-fm', alwaysApply: true }), + ); + const plainBody = `# Just the body now — no frontmatter.`; + const result = await methods.updateSkill({ + id: skill._id.toString(), + expectedVersion: skill.version, + update: { body: plainBody }, + }); + expect(result.status).toBe('updated'); + if (result.status === 'updated') { + expect(result.skill.alwaysApply).toBe(false); + } + }); + + it('updateSkill explicit alwaysApply still wins when body would otherwise flip it to false', async () => { + /* Higher-precedence sources still override: an API caller sending + both `alwaysApply: true` and a body without the key keeps the + column `true`, because explicit top-level is the authoritative + source for programmatic callers. */ + const { skill } = await methods.createSkill(makeSkillInput({ name: 'explicit-trumps-body' })); + const bodyWithoutKey = `---\nname: explicit-trumps-body\ndescription: frontmatter block without the flag.\n---\n\n# Body`; + const result = await methods.updateSkill({ + id: skill._id.toString(), + expectedVersion: skill.version, + update: { alwaysApply: true, body: bodyWithoutKey }, + }); + expect(result.status).toBe('updated'); + if (result.status === 'updated') { + expect(result.skill.alwaysApply).toBe(true); + } + }); }); describe('SkillFile methods', () => { diff --git a/packages/data-schemas/src/methods/skill.ts b/packages/data-schemas/src/methods/skill.ts index 2b425fe2b295..55607b26ad12 100644 --- a/packages/data-schemas/src/methods/skill.ts +++ b/packages/data-schemas/src/methods/skill.ts @@ -17,6 +17,7 @@ import type { } from '~/types/skill'; import { isValidObjectIdString } from '~/utils/objectId'; import { tenantSafeBulkWrite } from '~/utils/tenantBulkWrite'; +import { stripYamlTrailingComment } from '~/utils/yaml'; import { escapeRegExp } from '~/utils/string'; import logger from '~/config/winston'; @@ -195,6 +196,35 @@ export function validateSkillDisplayTitle(displayTitle: unknown): ValidationIssu return []; } +/** + * Validate the top-level `alwaysApply` column input. Mirrors the boolean + * check on `frontmatter['always-apply']` so a loosely-typed API caller + * sending `{"alwaysApply": "false"}` (string) gets a clean 400 at the + * validation boundary instead of relying on Mongoose casting quirks to + * coerce the value. + * + * `undefined` is the only pass-through value (meaning "don't touch this + * field"). `null` is rejected: PATCH forwards any non-`undefined` value + * straight into `$set`, so a `null` payload would persist `null` in a + * boolean column, leaving the skill in a state that is neither "on" nor + * "off" while `listAlwaysApplySkills` only matches `true`. + */ +export function validateAlwaysApply(alwaysApply: unknown): ValidationIssue[] { + if (alwaysApply === undefined) { + return []; + } + if (typeof alwaysApply !== 'boolean') { + return [ + { + field: 'alwaysApply', + code: 'INVALID_TYPE', + message: 'alwaysApply must be a boolean', + }, + ]; + } + return []; +} + /** * Known fields allowed inside a skill's YAML frontmatter. Anything else is * rejected in strict mode. The list is derived from Anthropic's Agent Skills @@ -211,6 +241,7 @@ const ALLOWED_FRONTMATTER_KEYS = new Set([ 'argument-hint', 'user-invocable', 'disable-model-invocation', + 'always-apply', 'model', 'effort', 'context', @@ -237,6 +268,7 @@ const FRONTMATTER_KIND: Record = { 'argument-hint': 'string', 'user-invocable': 'boolean', 'disable-model-invocation': 'boolean', + 'always-apply': 'boolean', model: 'string', effort: ['string', 'number'], context: 'string', @@ -437,6 +469,12 @@ export type CreateSkillInput = { authorName: string; source?: 'inline' | 'github' | 'notion'; sourceMetadata?: Record; + /** + * When `true`, the skill is auto-primed into every turn. Callers pass this + * through alongside `frontmatter` so the boolean lands on both the indexed + * first-class column (queryable) and the raw frontmatter bag (inspectable). + */ + alwaysApply?: boolean; tenantId?: string; }; @@ -447,6 +485,7 @@ export type UpdateSkillInput = { body?: string; frontmatter?: Record; category?: string; + alwaysApply?: boolean; }; /** @@ -575,6 +614,33 @@ export type ListSkillsByAccessResult = { after: string | null; }; +export type ListAlwaysApplySkillsParams = { + accessibleIds: Types.ObjectId[]; + /** Max rows to return per page. The caller paginates to fill an active-state budget. */ + limit: number; + /** Opaque cursor from a prior page. `null` / absent = first page. */ + cursor?: string | null; +}; + +export type ListAlwaysApplySkillsResult = { + /** + * Rows for `alwaysApply: true` skills within `accessibleIds` on this page. + * Returns `body` eagerly — callers prime the full SKILL.md on every turn, + * so round-tripping through `getSkillById` per skill would double DB ops. + */ + skills: Array<{ + _id: Types.ObjectId; + name: string; + body: string; + author: Types.ObjectId; + allowedTools?: string[]; + }>; + /** `true` when another page exists beyond this one. */ + has_more: boolean; + /** Cursor for the next page, or `null` when `has_more` is `false`. */ + after: string | null; +}; + export type UpdateSkillResult = | { status: 'updated'; @@ -589,6 +655,145 @@ export type CreateSkillResult = { warnings: ValidationIssue[]; }; +type BodyAlwaysApplyResult = + | { status: 'absent' } + | { status: 'valid'; value: boolean } + | { status: 'invalid' }; + +/** + * Extractor for the `always-apply` 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". + * + * Returns a discriminated union so callers can tell: + * - `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 + * placeholder states without rejecting a save. + * - `valid` — parsed cleanly as `true` / `false` (case-insensitive, + * quote-tolerant, YAML inline-comment-tolerant). + * - `invalid` — key is present with a non-empty value that isn't a + * 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. + */ +function extractAlwaysApplyFromBody(body: string | undefined): BodyAlwaysApplyResult { + if (typeof body !== 'string' || body.length === 0) { + return { status: 'absent' }; + } + const trimmed = body.trim(); + if (!trimmed.startsWith('---')) { + return { status: 'absent' }; + } + const after = trimmed.slice(3); + const closingIdx = after.indexOf('\n---'); + if (closingIdx === -1) { + return { status: 'absent' }; + } + const block = after.slice(0, closingIdx); + 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') { + continue; + } + // Strip the YAML inline comment BEFORE unquoting — a line like + // `always-apply: "true" # note` has both, and if we only handled + // whole-line quoting first, the quoted branch wouldn't match and + // the comment-strip would leave `"true"` which parses as invalid. + let value = stripYamlTrailingComment(line.slice(colon + 1).trim()).trim(); + if (value === '') { + return { status: 'absent' }; + } + if ( + value.length >= 2 && + ((value[0] === '"' && value[value.length - 1] === '"') || + (value[0] === "'" && value[value.length - 1] === "'")) + ) { + value = value.slice(1, -1); + } + value = value.trim(); + if (value === '') { + return { status: 'absent' }; + } + const lowered = value.toLowerCase(); + if (lowered === 'true') return { status: 'valid', value: true }; + if (lowered === 'false') return { status: 'valid', value: false }; + return { status: 'invalid' }; + } + return { status: 'absent' }; +} + +/** + * Resolve the effective `alwaysApply` boolean for a create/update call. + * + * The indexed `alwaysApply` column is the source of truth for auto-priming + * queries; it can also be carried inline inside the SKILL.md `body` or in + * the structured `frontmatter` bag. All three surfaces must stay in sync + * or a skill edit that flips `always-apply:` in the body would leave the + * column stale and the UI / auto-priming query would use the old value. + * + * 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 + * 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 + * current column value on update so an update that doesn't touch + * any of the three sources leaves the column alone). + */ +function resolveAlwaysApplyFromInput( + explicit: boolean | undefined, + frontmatter: Record | undefined, + body: string | undefined, + fallback: boolean, +): boolean { + if (typeof explicit === 'boolean') { + return explicit; + } + const fromFrontmatter = frontmatter?.['always-apply']; + if (typeof fromFrontmatter === 'boolean') { + return fromFrontmatter; + } + const fromBody = extractAlwaysApplyFromBody(body); + if (fromBody.status === 'valid') { + return fromBody.value; + } + return fallback; +} + +/** + * Validate the `always-apply` value that would be derived from the + * SKILL.md body's inline frontmatter. Only reports an issue when the + * key is present with an unparseable value — absent / valid / empty + * all pass silently so mid-edit saves that haven't touched the flag + * yet don't get rejected. Wired into both `createSkill` and + * `updateSkill` so a body PATCH carrying `always-apply: tru` (typo) + * surfaces as 400 instead of drifting the indexed column. + */ +export function validateAlwaysApplyInBody(body: string | undefined): ValidationIssue[] { + const result = extractAlwaysApplyFromBody(body); + if (result.status === 'invalid') { + return [ + { + field: 'body.frontmatter.always-apply', + code: 'INVALID_TYPE', + message: '"always-apply" in SKILL.md frontmatter must be a boolean (true or false)', + }, + ]; + } + return []; +} + export function createSkillMethods(mongoose: typeof import('mongoose'), deps: SkillDeps) { const { ObjectId } = mongoose.Types; @@ -647,6 +852,8 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk ...validateSkillBody(data.body), ...validateSkillDisplayTitle(data.displayTitle), ...validateSkillFrontmatter(data.frontmatter), + ...validateAlwaysApply(data.alwaysApply), + ...validateAlwaysApplyInBody(data.body), ]; const { errors, warnings } = partitionIssues(issues); if (errors.length > 0) { @@ -689,6 +896,12 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk source: data.source ?? 'inline', sourceMetadata: data.sourceMetadata, fileCount: 0, + alwaysApply: resolveAlwaysApplyFromInput( + data.alwaysApply, + data.frontmatter, + data.body, + false, + ), tenantId: data.tenantId, ...derived, }); @@ -797,24 +1010,24 @@ 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. */ + /* `frontmatter` is deliberately NOT projected: the structured + columns (disableModelInvocation / userInvocable / allowedTools / + alwaysApply) are always populated by `createSkill` / `updateSkill` + going forward, and the branch this code ships on never shipped + to main — so no legacy rows exist that would need a frontmatter + read-time backfill on summaries. Skipping it saves ~2KB/skill × + 100/page of wire traffic. `backfillDerivedFromFrontmatter` is + still called below as defensive code; it short-circuits when + `frontmatter` is undefined. */ .select( - 'name displayTitle description category author authorName version source sourceMetadata fileCount tenantId disableModelInvocation userInvocable allowedTools frontmatter createdAt updatedAt', + 'name displayTitle description category author authorName version source sourceMetadata fileCount alwaysApply tenantId disableModelInvocation userInvocable allowedTools 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. */ + /* Defensive read-time fallback. With `frontmatter` excluded from the + projection, the helper short-circuits immediately; kept in the loop + so a future projection change (or legacy rows appearing via a + migration) continues to get runtime-column restoration for free. */ for (const row of rows) { backfillDerivedFromFrontmatter(row as unknown as ISkill); } @@ -837,6 +1050,74 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk }; } + async function listAlwaysApplySkills( + params: ListAlwaysApplySkillsParams, + ): Promise { + const Skill = mongoose.models.Skill as Model; + if (!params.accessibleIds.length) { + return { skills: [], has_more: false, after: null }; + } + const limit = Math.min(Math.max(1, params.limit || 20), 100); + + const baseFilter: FilterQuery = { + _id: { $in: params.accessibleIds }, + alwaysApply: true, + }; + const cursor = decodeCursor(params.cursor); + + let filter: FilterQuery = baseFilter; + if (cursor) { + const cursorCondition: FilterQuery = { + $or: [ + { updatedAt: { $lt: cursor.updatedAt } }, + { updatedAt: cursor.updatedAt, _id: { $gt: cursor._id } }, + ], + }; + filter = { $and: [baseFilter, cursorCondition] }; + } + + const rows = await Skill.find(filter) + .sort({ updatedAt: -1, _id: 1 }) + .limit(limit + 1) + .select('name body author updatedAt allowedTools') + .lean(); + + const has_more = rows.length > limit; + const sliced = has_more ? rows.slice(0, limit) : rows; + const last = sliced[sliced.length - 1]; + const after = + has_more && last + ? encodeCursor({ + updatedAt: last.updatedAt as Date, + _id: last._id as Types.ObjectId, + }) + : null; + + /** + * `allowedTools` is projected alongside `name`/`body`/`author` so the + * always-apply prime pipeline (post-Phase 6) can union skill-declared + * tool allowlists into the agent's effective tool set for the turn — + * same symmetry as the manual-prime path, which reads the column off + * `getSkillByName`. Older rows predating the column show up with + * `allowedTools === undefined` (the backfill helper runs on those at + * read time elsewhere; per-turn priming is fine with undefined). + */ + const skills = sliced.map((row) => { + const result: ListAlwaysApplySkillsResult['skills'][number] = { + _id: row._id as Types.ObjectId, + name: row.name, + body: row.body ?? '', + author: row.author as Types.ObjectId, + }; + if (row.allowedTools !== undefined) { + result.allowedTools = row.allowedTools; + } + return result; + }); + + return { skills, has_more, after }; + } + async function updateSkill(params: { id: string; expectedVersion: number; @@ -856,6 +1137,8 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk issues.push(...validateSkillDisplayTitle(update.displayTitle)); if (update.frontmatter !== undefined) issues.push(...validateSkillFrontmatter(update.frontmatter)); + if (update.alwaysApply !== undefined) issues.push(...validateAlwaysApply(update.alwaysApply)); + if (update.body !== undefined) issues.push(...validateAlwaysApplyInBody(update.body)); const { errors, warnings } = partitionIssues(issues); if (errors.length > 0) { const error = new Error('Skill validation failed'); @@ -889,6 +1172,63 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk } } if (update.category !== undefined) setPayload.category = update.category; + /** + * 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 + * 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:` + * would otherwise leave the column stale and auto-priming / pin + * badges would keep using the old value. + * + * When a `body` is submitted with NO `always-apply:` line (e.g. the + * user removed the line from SKILL.md), that counts as a positive + * declaration of "not always-apply" — the column flips to `false`. + * Leaving it untouched would leave a skill that was once always-apply + * silently auto-priming even after its own SKILL.md no longer + * declares the flag. + * + * Important: the gates key off the *presence of an always-apply value* + * 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']` + * is absent in that payload. + */ + let derivedAlwaysApply: boolean | undefined; + if (update.alwaysApply !== undefined) { + derivedAlwaysApply = update.alwaysApply; + } + if (derivedAlwaysApply === undefined && update.frontmatter !== undefined) { + const fromFrontmatter = update.frontmatter['always-apply']; + if (typeof fromFrontmatter === 'boolean') { + derivedAlwaysApply = fromFrontmatter; + } + } + if (derivedAlwaysApply === undefined && update.body !== undefined) { + const fromBody = extractAlwaysApplyFromBody(update.body); + if (fromBody.status === 'valid') { + derivedAlwaysApply = fromBody.value; + } else if (fromBody.status === 'absent') { + /* An `absent` result means the user submitted a new body that + declares no `always-apply:` key (either the key was removed or + no frontmatter block was ever there). The body is the + authoritative source for this skill's declared state: editing + it to drop the flag intends to turn auto-priming off, so flip + the column to `false`. Without this, a skill that was once + `alwaysApply: true` would keep auto-priming after the user + removed the declaration from SKILL.md — a persistent, + invisible mismatch between the file and runtime behavior. + `invalid` is rejected upstream by `validateAlwaysApplyInBody` + so this branch only handles the legitimate absence case. */ + derivedAlwaysApply = false; + } + } + if (derivedAlwaysApply !== undefined) { + setPayload.alwaysApply = derivedAlwaysApply; + } const updateOps: Record = { $set: setPayload, @@ -1120,6 +1460,7 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk getSkillById, getSkillByName, listSkillsByAccess, + listAlwaysApplySkills, updateSkill, deleteSkill, deleteUserSkills, diff --git a/packages/data-schemas/src/schema/message.ts b/packages/data-schemas/src/schema/message.ts index aeba714b89d3..eb512129a96a 100644 --- a/packages/data-schemas/src/schema/message.ts +++ b/packages/data-schemas/src/schema/message.ts @@ -130,6 +130,14 @@ const messageSchema: Schema = new Schema( * request body, not on the message itself. */ manualSkills: { type: [String], default: undefined }, + /** + * Skill names auto-primed on this turn because their frontmatter declares + * `always-apply: true`. Persisted at turn time (not reconstructed on + * render) because `Skill.alwaysApply` is mutable — if an admin flips the + * flag off later, historical turns must still show the pinned badges on + * the user bubble to preserve the audit trail of what actually ran. + */ + alwaysAppliedSkills: { type: [String], default: undefined }, /* attachments: { type: [ diff --git a/packages/data-schemas/src/schema/skill.ts b/packages/data-schemas/src/schema/skill.ts index 31a39a59b678..c3d30e981ad2 100644 --- a/packages/data-schemas/src/schema/skill.ts +++ b/packages/data-schemas/src/schema/skill.ts @@ -208,6 +208,18 @@ const skillSchema: Schema = new Schema( default: 0, min: 0, }, + /** + * 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 + * column so the `listAlwaysApplySkills` query at the top of every + * request is an indexed lookup, not a frontmatter scan. + */ + alwaysApply: { + type: Boolean, + default: false, + index: true, + }, tenantId: { type: String, index: true, diff --git a/packages/data-schemas/src/types/message.ts b/packages/data-schemas/src/types/message.ts index 7d607d61d96f..5eb47b8e25c6 100644 --- a/packages/data-schemas/src/types/message.ts +++ b/packages/data-schemas/src/types/message.ts @@ -46,6 +46,13 @@ export interface IMessage extends Document { attachments?: unknown[]; /** Skills the user invoked manually via the `$` popover on this turn. UI-only metadata for `ManualSkillPills`. */ manualSkills?: string[]; + /** + * Skills auto-primed on this turn via `always-apply` frontmatter. Persisted + * at turn time so pinned badges survive later flips of the skill's + * `alwaysApply` flag — the audit trail follows what actually ran, not what + * the current catalog says. + */ + alwaysAppliedSkills?: string[]; expiredAt?: Date | null; createdAt?: Date; updatedAt?: Date; diff --git a/packages/data-schemas/src/types/skill.ts b/packages/data-schemas/src/types/skill.ts index c240abdb3664..084d1c2af569 100644 --- a/packages/data-schemas/src/types/skill.ts +++ b/packages/data-schemas/src/types/skill.ts @@ -81,6 +81,13 @@ export interface ISkill { sourceMetadata?: Record; /** Denormalized count of associated `SkillFile` rows. Kept in sync by skill methods. */ fileCount: number; + /** + * When `true`, the skill is auto-primed into every turn — no user `$` + * invocation or model discretion required. Mirrors the `always-apply` YAML + * frontmatter field; indexed so the per-turn "always-apply" query stays + * cheap as the catalog grows. + */ + alwaysApply: boolean; tenantId?: string; createdAt?: Date; updatedAt?: Date; diff --git a/packages/data-schemas/src/utils/index.ts b/packages/data-schemas/src/utils/index.ts index 17e43ac3ca16..cdc69ce6b342 100644 --- a/packages/data-schemas/src/utils/index.ts +++ b/packages/data-schemas/src/utils/index.ts @@ -4,3 +4,4 @@ export * from './tempChatRetention'; export { tenantSafeBulkWrite } from './tenantBulkWrite'; export * from './transactions'; export * from './objectId'; +export * from './yaml'; diff --git a/packages/data-schemas/src/utils/yaml.ts b/packages/data-schemas/src/utils/yaml.ts new file mode 100644 index 000000000000..e33e1d793cd2 --- /dev/null +++ b/packages/data-schemas/src/utils/yaml.ts @@ -0,0 +1,15 @@ +/** + * Strip a trailing YAML inline comment from an unquoted scalar. + * YAML treats ` # ...` (space before hash) as a comment; `#` without a + * preceding space is part of the value (e.g. `hashtag#foo`). A scalar + * that's entirely a comment (`# nothing yet`) collapses to empty so + * callers can treat it as "no value". Applied narrowly — only to + * boolean fields where the token is a single word — to avoid + * accidentally truncating free-form strings like descriptions that + * might legitimately contain `#`. + */ +export function stripYamlTrailingComment(value: string): string { + if (value.trimStart().startsWith('#')) return ''; + const match = value.match(/^(.*?)\s+#.*$/); + return match ? match[1] : value; +}