From aafc1330e1213dfaf120dc98100d4d5accd2bd96 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 19 Apr 2026 23:22:59 -0400 Subject: [PATCH 1/6] =?UTF-8?q?=F0=9F=94=81=20refactor:=20Rebase=20always-?= =?UTF-8?q?apply=20work=20onto=20merged=20structured-frontmatter=20columns?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 6 (disable-model-invocation / user-invocable / allowed-tools) landed first on feat/agent-skills. Reconcile this branch with the new mainline: - Thread alwaysApplySkillPrimes through unionPrimeAllowedTools alongside manualSkillPrimes, applying the combined MAX_PRIMED_SKILLS_PER_TURN ceiling before loading tools. - Add `_id` to ResolvedAlwaysApplySkill to match Phase 6's ResolvedManualSkill shape (read_file name-collision protection). - Register 'always-apply' in ALLOWED_FRONTMATTER_KEYS / FRONTMATTER_KIND so Phase 6's validator recognizes it. - Drop frontmatter from the listSkillsByAccess projection; the backfill helper remains as defensive code but its read path is no longer exercised on summary rows (no legacy rows exist — the branch never shipped), saving ~200KB per page. - Retire the corresponding "backfills legacy on summaries" test. - Plumb listAlwaysApplySkills through the JS controllers + endpoint initializer so the always-apply resolver sees a real DB method. --- api/app/clients/BaseClient.js | 17 + .../agents/__tests__/openai.spec.js | 7 + .../agents/__tests__/responses.unit.spec.js | 7 + api/server/controllers/agents/client.js | 75 ++-- api/server/controllers/agents/openai.js | 21 +- api/server/controllers/agents/responses.js | 21 +- .../services/Endpoints/agents/initialize.js | 2 + .../Chat/Messages/Content/Container.tsx | 9 +- .../Messages/Content/ManualSkillPills.tsx | 53 ++- .../__tests__/ManualSkillPills.test.tsx | 37 +- .../src/components/Messages/ContentRender.tsx | 3 +- .../Skills/display/SkillDetailHeader.tsx | 14 +- .../components/Skills/lists/SkillListItem.tsx | 12 +- client/src/locales/en/translation.json | 2 + .../api/src/agents/__tests__/skills.test.ts | 376 ++++++++++++++++++ packages/api/src/agents/initialize.ts | 129 ++++-- packages/api/src/agents/skills.ts | 339 +++++++++++++++- .../api/src/skills/__tests__/import.test.ts | 127 ++++++ packages/api/src/skills/handlers.ts | 4 + packages/api/src/skills/import.ts | 104 ++++- packages/data-provider/src/schemas.ts | 8 + packages/data-provider/src/types/skills.ts | 9 + .../data-schemas/src/methods/skill.spec.ts | 312 +++++++++++++-- packages/data-schemas/src/methods/skill.ts | 337 +++++++++++++++- packages/data-schemas/src/schema/message.ts | 8 + packages/data-schemas/src/schema/skill.ts | 12 + packages/data-schemas/src/types/message.ts | 7 + packages/data-schemas/src/types/skill.ts | 7 + 28 files changed, 1933 insertions(+), 126 deletions(-) create mode 100644 packages/api/src/skills/__tests__/import.test.ts 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..aa2627be666d 100644 --- a/api/server/controllers/agents/__tests__/openai.spec.js +++ b/api/server/controllers/agents/__tests__/openai.spec.js @@ -57,6 +57,13 @@ 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, + }), 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..8d2c1cf144f5 100644 --- a/api/server/controllers/agents/__tests__/responses.unit.spec.js +++ b/api/server/controllers/agents/__tests__/responses.unit.spec.js @@ -53,6 +53,13 @@ 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, + }), 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..b3dc5830f42e 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 { @@ -274,6 +274,7 @@ const OpenAIChatCompletionController = async (req, res) => { getToolFilesByIds: db.getToolFilesByIds, getCodeGeneratedFiles: db.getCodeGeneratedFiles, listSkillsByAccess: db.listSkillsByAccess, + listAlwaysApplySkills: db.listAlwaysApplySkills, getSkillByName: db.getSkillByName, }, ); @@ -350,17 +351,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..bd1745277606 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, @@ -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; } diff --git a/api/server/services/Endpoints/agents/initialize.js b/api/server/services/Endpoints/agents/initialize.js index b5a251c1ebc2..6e3651833c1c 100644 --- a/api/server/services/Endpoints/agents/initialize.js +++ b/api/server/services/Endpoints/agents/initialize.js @@ -291,6 +291,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { getCodeGeneratedFiles: db.getCodeGeneratedFiles, filterFilesByAgentAccess, listSkillsByAccess: db.listSkillsByAccess, + listAlwaysApplySkills: db.listAlwaysApplySkills, getSkillByName: db.getSkillByName, }, ); @@ -389,6 +390,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { getCodeGeneratedFiles: db.getCodeGeneratedFiles, filterFilesByAgentAccess, listSkillsByAccess: db.listSkillsByAccess, + listAlwaysApplySkills: db.listAlwaysApplySkills, getSkillByName: db.getSkillByName, }, ); 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..0b52ac8f3650 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,374 @@ 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 }); + }); +}); diff --git a/packages/api/src/agents/initialize.ts b/packages/api/src/agents/initialize.ts index 4ef4432ffa12..0cff414851ed 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,80 @@ 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; + + /** + * 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 +772,7 @@ export async function initializeAgent( skillCount, accessibleSkillIds: executableSkillIds, manualSkillPrimes, + alwaysApplySkillPrimes, attachments: finalAttachments, toolContextMap: toolContextMap ?? {}, useLegacyContent: !!options.useLegacyContent, diff --git a/packages/api/src/agents/skills.ts b/packages/api/src/agents/skills.ts index 1a6c642471e0..886ee0ae164e 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,21 @@ 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'; +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 @@ -444,6 +476,20 @@ export interface ResolvedManualSkill { allowedTools?: string[]; } +/** + * Result of resolving an `always-apply` skill into prime-ready form. + * Intentionally identical in shape to `ResolvedManualSkill` — both feed + * the same prime-injection pipeline and the same `unionPrimeAllowedTools` + * helper. The distinction lives on `additional_kwargs.trigger` of the + * spliced `HumanMessage`, not on this resolver output. + */ +export interface ResolvedAlwaysApplySkill { + _id: Types.ObjectId; + name: string; + body: string; + allowedTools?: string[]; +} + /** * Resolves user-provided skill names to `{ name, body }` pairs ready for * priming. Filters out: @@ -578,6 +624,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[]; @@ -646,7 +868,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 +881,116 @@ 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; +} + +/** + * 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. + * + * Enforces a combined ceiling (`maxPrimesPerTurn`, default + * `MAX_PRIMED_SKILLS_PER_TURN`) by truncating always-apply first so + * manual is never silently dropped. + */ +export function injectSkillPrimes(params: InjectSkillPrimesParams): InjectSkillPrimesResult { + const { + initialMessages, + manualSkillPrimes = [], + alwaysApplySkillPrimes = [], + maxPrimesPerTurn = MAX_PRIMED_SKILLS_PER_TURN, + } = params; + let { indexTokenCountMap } = params; + + let alwaysApply = alwaysApplySkillPrimes; + 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, + }; + } + + 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, + }; +} + 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..cdd0a4fc3b78 100644 --- a/packages/api/src/skills/import.ts +++ b/packages/api/src/skills/import.ts @@ -32,20 +32,71 @@ function unquoteYaml(value: string): string { return value; } -/** YAML frontmatter parser — extracts name + description from SKILL.md. */ -function parseFrontmatter(raw: string): { name: string; description: string } { +/** + * 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. A scalar that's entirely a + * comment (`# nothing here`) collapses to empty so callers can treat + * it as "no value". Applied only to boolean tokens here since those are + * single-word and comment-safe; free-form scalars like descriptions + * might legitimately contain `#`. + */ +function stripYamlTrailingComment(value: string): string { + if (value.trimStart().startsWith('#')) return ''; + const match = value.match(/^(.*?)\s+#.*$/); + return match ? match[1] : value; +} + +/** Parse a YAML scalar as a strict boolean. Returns `undefined` when neither. */ +function parseBooleanScalar(value: string): boolean | undefined { + const lowered = stripYamlTrailingComment(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) { @@ -57,9 +108,26 @@ function parseFrontmatter(raw: string): { name: string; description: string } { name = value; } else if (key === 'description') { description = value; + } else if (key === 'always-apply') { + // The outer `value` was already run through `unquoteYaml`, which + // only handles whole-line quoting. For `always-apply: "true" # note` + // the quote check misses (line doesn't end with a quote), so strip + // the comment first and then unquote the remainder. + const stripped = stripYamlTrailingComment(value).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 +288,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 +320,7 @@ async function handleMarkdown( body: content, author: authorId, authorName, + alwaysApply, tenantId, }); @@ -317,7 +396,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 +428,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..3ba5124bbe66 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,245 @@ 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); + } + }); }); describe('SkillFile methods', () => { diff --git a/packages/data-schemas/src/methods/skill.ts b/packages/data-schemas/src/methods/skill.ts index 2b425fe2b295..5aceea4ffb0d 100644 --- a/packages/data-schemas/src/methods/skill.ts +++ b/packages/data-schemas/src/methods/skill.ts @@ -195,6 +195,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 +240,7 @@ const ALLOWED_FRONTMATTER_KEYS = new Set([ 'argument-hint', 'user-invocable', 'disable-model-invocation', + 'always-apply', 'model', 'effort', 'context', @@ -237,6 +267,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 +468,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 +484,7 @@ export type UpdateSkillInput = { body?: string; frontmatter?: Record; category?: string; + alwaysApply?: boolean; }; /** @@ -575,6 +613,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 +654,160 @@ export type CreateSkillResult = { warnings: ValidationIssue[]; }; +/** + * Strip a trailing YAML inline comment from an already-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. + */ +function stripYamlTrailingComment(value: string): string { + if (value.trimStart().startsWith('#')) return ''; + const match = value.match(/^(.*?)\s+#.*$/); + return match ? match[1] : value; +} + +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 +866,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 +910,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, }); @@ -808,7 +1035,7 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk × 100/page is wasted bandwidth on every list call once backfill is no longer needed. */ .select( - 'name displayTitle description category author authorName version source sourceMetadata fileCount tenantId disableModelInvocation userInvocable allowedTools frontmatter createdAt updatedAt', + 'name displayTitle description category author authorName version source sourceMetadata fileCount alwaysApply tenantId disableModelInvocation userInvocable allowedTools createdAt updatedAt', ) .lean(); @@ -837,6 +1064,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 +1151,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 +1186,43 @@ 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. + * + * 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; + } + } + if (derivedAlwaysApply !== undefined) { + setPayload.alwaysApply = derivedAlwaysApply; + } const updateOps: Record = { $set: setPayload, @@ -1120,6 +1454,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; From 25621283030e31cd44219497544b839f0fada3d1 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 20 Apr 2026 01:00:44 -0400 Subject: [PATCH 2/6] =?UTF-8?q?=F0=9F=A7=B9=20fix:=20Dedupe=20manual/alway?= =?UTF-8?q?s-apply=20overlap,=20share=20YAML=20util,=20tidy=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review findings: - Cross-list dedup: when a user $-invokes a skill that is also marked always-apply, the always-apply copy is now dropped so the same SKILL.md body never primes twice in one turn. Manual wins (explicit intent, closer to the user message). Dedup runs in both initializeAgent (so persisted user-bubble pills stay in sync) and injectSkillPrimes (defense-in-depth at splice time). New test cases cover single-overlap, partial-overlap, and dedup-before-cap. - DRY: extract stripYamlTrailingComment to packages/data-schemas/src/utils/yaml.ts; packages/api/src/skills/import.ts now imports the shared helper. Also drop the redundant inner stripYamlTrailingComment call inside parseBooleanScalar — the call site already strips. - Mark injectManualSkillPrimes as @deprecated in favor of injectSkillPrimes (kept for external consumers of @librechat/api). - Document SKILL_TRIGGER_MODEL as forward-looking plumbing for the model-invoked path rather than leaving it as a bare unused export. - Replace the stale "frontmatter is included" comment on listSkillsByAccess with an accurate explanation of why it was intentionally excluded. --- .../api/src/agents/__tests__/skills.test.ts | 49 +++++++++++++++++++ packages/api/src/agents/initialize.ts | 28 +++++++++++ packages/api/src/agents/skills.ts | 41 +++++++++++++++- packages/api/src/skills/import.ts | 22 +++------ packages/data-schemas/src/methods/skill.ts | 42 ++++++---------- packages/data-schemas/src/utils/index.ts | 1 + packages/data-schemas/src/utils/yaml.ts | 15 ++++++ 7 files changed, 153 insertions(+), 45 deletions(-) create mode 100644 packages/data-schemas/src/utils/yaml.ts diff --git a/packages/api/src/agents/__tests__/skills.test.ts b/packages/api/src/agents/__tests__/skills.test.ts index 0b52ac8f3650..1035faff19ad 100644 --- a/packages/api/src/agents/__tests__/skills.test.ts +++ b/packages/api/src/agents/__tests__/skills.test.ts @@ -1734,4 +1734,53 @@ describe('injectSkillPrimes', () => { // 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/initialize.ts b/packages/api/src/agents/initialize.ts index 0cff414851ed..0d0b9aa95376 100644 --- a/packages/api/src/agents/initialize.ts +++ b/packages/api/src/agents/initialize.ts @@ -483,6 +483,34 @@ export async function initializeAgent( 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 diff --git a/packages/api/src/agents/skills.ts b/packages/api/src/agents/skills.ts index 886ee0ae164e..c9e9128e3895 100644 --- a/packages/api/src/agents/skills.ts +++ b/packages/api/src/agents/skills.ts @@ -66,6 +66,12 @@ export const SKILL_MESSAGE_SOURCE = 'skill'; * (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'; @@ -841,6 +847,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, @@ -906,6 +918,12 @@ export interface InjectSkillPrimesResult { 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; } /** @@ -918,9 +936,15 @@ export interface InjectSkillPrimesResult { * 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. + * 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 { @@ -932,6 +956,19 @@ export function injectSkillPrimes(params: InjectSkillPrimesParams): InjectSkillP 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) { @@ -951,6 +988,7 @@ export function injectSkillPrimes(params: InjectSkillPrimesParams): InjectSkillP inserted: 0, insertIdx: -1, alwaysApplyDropped, + alwaysApplyDedupedFromManual, }; } @@ -988,6 +1026,7 @@ export function injectSkillPrimes(params: InjectSkillPrimesParams): InjectSkillP inserted: numPrimes, insertIdx, alwaysApplyDropped, + alwaysApplyDedupedFromManual, }; } diff --git a/packages/api/src/skills/import.ts b/packages/api/src/skills/import.ts index cdd0a4fc3b78..d83ae97be76b 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 { @@ -33,23 +33,13 @@ function unquoteYaml(value: string): string { } /** - * 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. A scalar that's entirely a - * comment (`# nothing here`) collapses to empty so callers can treat - * it as "no value". Applied only to boolean tokens here since those are - * single-word and comment-safe; free-form scalars like descriptions - * might legitimately contain `#`. + * 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 stripYamlTrailingComment(value: string): string { - if (value.trimStart().startsWith('#')) return ''; - const match = value.match(/^(.*?)\s+#.*$/); - return match ? match[1] : value; -} - -/** Parse a YAML scalar as a strict boolean. Returns `undefined` when neither. */ function parseBooleanScalar(value: string): boolean | undefined { - const lowered = stripYamlTrailingComment(value).trim().toLowerCase(); + const lowered = value.trim().toLowerCase(); if (lowered === 'true') { return true; } diff --git a/packages/data-schemas/src/methods/skill.ts b/packages/data-schemas/src/methods/skill.ts index 5aceea4ffb0d..04af1bcb1c60 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'; @@ -654,21 +655,6 @@ export type CreateSkillResult = { warnings: ValidationIssue[]; }; -/** - * Strip a trailing YAML inline comment from an already-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. - */ -function stripYamlTrailingComment(value: string): string { - if (value.trimStart().startsWith('#')) return ''; - const match = value.match(/^(.*?)\s+#.*$/); - return match ? match[1] : value; -} - type BodyAlwaysApplyResult = | { status: 'absent' } | { status: 'valid'; value: boolean } @@ -1024,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 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); } 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; +} From fe078cc25efefce25899d858f6d518e673946830 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 20 Apr 2026 21:27:51 -0400 Subject: [PATCH 3/6] =?UTF-8?q?=F0=9F=94=92=20fix:=20Include=20always-appl?= =?UTF-8?q?y=20primes=20in=20skillPrimedIdsByName=20+=20clear=20alwaysAppl?= =?UTF-8?q?y=20on=20body=20opt-out?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs flagged by Codex review: P1 (read_file): `manualSkillPrimedIdsByName` only carried manual-invocation primes, so an always-apply skill with `disable-model-invocation: true` was blocked from reading its own bundled files, and same-name collisions could resolve to a different doc than the one whose body got primed. - Rename `buildManualSkillPrimedIdsByName` → `buildSkillPrimedIdsByName` (accepts both manual + always-apply prime arrays). - Rename the configurable field `manualSkillPrimedIdsByName` → `skillPrimedIdsByName` throughout the plumbing (skillConfigurable.ts, handlers.ts, CJS callers, tests). - Overlap resolution: manual wins on the rare edge case where the same name appears in both arrays (upstream dedup should prevent this, but defensive merging treats manual as authoritative). - New tests: (1) gate-relaxation fires for always-apply primes, (2) `_id` pinning works for always-apply same-name collisions. P2 (updateSkill): when a body update had no `always-apply:` key, `extractAlwaysApplyFromBody` returned `absent` and the column was left untouched. A skill that was once `alwaysApply: true` would keep auto-priming even after its SKILL.md no longer declared the flag. - Treat `absent` as a positive "not always-apply" declaration when the body is explicitly submitted; flip the column to `false`. - Explicit top-level `alwaysApply` still wins (three-source precedence unchanged). - New tests: body removes key → false, body has no frontmatter at all → false, explicit + body-without-key → explicit wins. --- api/server/controllers/agents/openai.js | 7 +- api/server/controllers/agents/responses.js | 12 ++- .../services/Endpoints/agents/initialize.js | 19 ++-- .../services/Endpoints/agents/skillDeps.js | 52 +++++++--- packages/api/src/agents/handlers.spec.ts | 96 +++++++++++++++++-- packages/api/src/agents/handlers.ts | 48 +++++----- packages/api/src/agents/skillConfigurable.ts | 26 ++--- .../data-schemas/src/methods/skill.spec.ts | 59 ++++++++++++ packages/data-schemas/src/methods/skill.ts | 20 ++++ 9 files changed, 272 insertions(+), 67 deletions(-) diff --git a/api/server/controllers/agents/openai.js b/api/server/controllers/agents/openai.js index b3dc5830f42e..adb80da9af37 100644 --- a/api/server/controllers/agents/openai.js +++ b/api/server/controllers/agents/openai.js @@ -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'); @@ -333,7 +333,10 @@ const OpenAIChatCompletionController = async (req, res) => { req, primaryConfig.accessibleSkillIds, undefined, - buildManualSkillPrimedIdsByName(primaryConfig.manualSkillPrimes), + buildSkillPrimedIdsByName( + primaryConfig.manualSkillPrimes, + primaryConfig.alwaysApplySkillPrimes, + ), ); }, toolEndCallback, diff --git a/api/server/controllers/agents/responses.js b/api/server/controllers/agents/responses.js index bd1745277606..0727cdc69193 100644 --- a/api/server/controllers/agents/responses.js +++ b/api/server/controllers/agents/responses.js @@ -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'); @@ -522,7 +522,10 @@ const createResponse = async (req, res) => { req, primaryConfig.accessibleSkillIds, undefined, - buildManualSkillPrimedIdsByName(primaryConfig.manualSkillPrimes), + buildSkillPrimedIdsByName( + primaryConfig.manualSkillPrimes, + primaryConfig.alwaysApplySkillPrimes, + ), ); }, toolEndCallback, @@ -695,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 6e3651833c1c..452aff285dab 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, @@ -299,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, @@ -313,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; 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/packages/api/src/agents/handlers.spec.ts b/packages/api/src/agents/handlers.spec.ts index 77fb42274923..237cd27a51ac 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,87 @@ 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 [, accessibleIdsArg, lookupOptions] = getSkillByName.mock.calls[0] as [ + string, + Array<{ toString(): string }>, + Record, + ]; + 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/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/data-schemas/src/methods/skill.spec.ts b/packages/data-schemas/src/methods/skill.spec.ts index 3ba5124bbe66..4d8ea062a9ee 100644 --- a/packages/data-schemas/src/methods/skill.spec.ts +++ b/packages/data-schemas/src/methods/skill.spec.ts @@ -1128,6 +1128,65 @@ describe('Skill CRUD methods', () => { 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 04af1bcb1c60..55607b26ad12 100644 --- a/packages/data-schemas/src/methods/skill.ts +++ b/packages/data-schemas/src/methods/skill.ts @@ -1183,6 +1183,13 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk * 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. @@ -1204,6 +1211,19 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk 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) { From e8e8cb72a69b96c4e114f8751319bc94aacb4f19 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 20 Apr 2026 21:31:53 -0400 Subject: [PATCH 4/6] =?UTF-8?q?=F0=9F=A7=B5=20refactor:=20Collapse=20dupli?= =?UTF-8?q?cate=20prime=20types=20+=20tighten=20parse=20+=20test=20hygiene?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sanity-check review follow-ups: - Collapse `ResolvedManualSkill` / `ResolvedAlwaysApplySkill` into a single `ResolvedSkillPrime` canonical interface with two backward- compatible type aliases. Both resolvers feed the same pipeline stages (injectSkillPrimes, unionPrimeAllowedTools, buildSkillPrimedIdsByName); the per-source distinction lives on `additional_kwargs.trigger`, not on the resolver output. - Move the `always-apply` branch in `parseFrontmatter` to operate on the raw post-colon text. The outer `unquoteYaml` was fine today because it's idempotent on non-quoted strings, but running it twice (once per line, once after stripping the inline comment) would be fragile if the unquoter ever grows richer YAML-escape handling. - Add the missing `alwaysApplyDedupedFromManual: 0` field to the `injectSkillPrimes` mocks in `openai.spec.js` and `responses.unit.spec.js` so they match the full `InjectSkillPrimesResult` contract. - Insert the blank line between the `unionPrimeAllowedTools` and `resolveAlwaysApplySkills` describe blocks. --- .../agents/__tests__/openai.spec.js | 1 + .../agents/__tests__/responses.unit.spec.js | 1 + .../api/src/agents/__tests__/skills.test.ts | 1 + packages/api/src/agents/skills.ts | 33 ++++++++++++------- packages/api/src/skills/import.ts | 19 ++++++----- 5 files changed, 35 insertions(+), 20 deletions(-) diff --git a/api/server/controllers/agents/__tests__/openai.spec.js b/api/server/controllers/agents/__tests__/openai.spec.js index aa2627be666d..c2b9ab993c22 100644 --- a/api/server/controllers/agents/__tests__/openai.spec.js +++ b/api/server/controllers/agents/__tests__/openai.spec.js @@ -63,6 +63,7 @@ jest.mock('@librechat/api', () => ({ inserted: 0, insertIdx: -1, alwaysApplyDropped: 0, + alwaysApplyDedupedFromManual: 0, }), buildNonStreamingResponse: jest.fn().mockReturnValue({ id: 'resp-123' }), createOpenAIStreamTracker: jest.fn().mockReturnValue({ diff --git a/api/server/controllers/agents/__tests__/responses.unit.spec.js b/api/server/controllers/agents/__tests__/responses.unit.spec.js index 8d2c1cf144f5..0dc07572aefd 100644 --- a/api/server/controllers/agents/__tests__/responses.unit.spec.js +++ b/api/server/controllers/agents/__tests__/responses.unit.spec.js @@ -59,6 +59,7 @@ jest.mock('@librechat/api', () => ({ inserted: 0, insertIdx: -1, alwaysApplyDropped: 0, + alwaysApplyDedupedFromManual: 0, }), createToolExecuteHandler: jest.fn().mockReturnValue({ handle: jest.fn() }), // Responses API diff --git a/packages/api/src/agents/__tests__/skills.test.ts b/packages/api/src/agents/__tests__/skills.test.ts index 1035faff19ad..85fedf2eec11 100644 --- a/packages/api/src/agents/__tests__/skills.test.ts +++ b/packages/api/src/agents/__tests__/skills.test.ts @@ -1364,6 +1364,7 @@ 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); diff --git a/packages/api/src/agents/skills.ts b/packages/api/src/agents/skills.ts index c9e9128e3895..9a8745befda6 100644 --- a/packages/api/src/agents/skills.ts +++ b/packages/api/src/agents/skills.ts @@ -462,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 @@ -483,18 +492,18 @@ export interface ResolvedManualSkill { } /** - * Result of resolving an `always-apply` skill into prime-ready form. - * Intentionally identical in shape to `ResolvedManualSkill` — both feed - * the same prime-injection pipeline and the same `unionPrimeAllowedTools` - * helper. The distinction lives on `additional_kwargs.trigger` of the - * spliced `HumanMessage`, not on this resolver output. + * 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 interface ResolvedAlwaysApplySkill { - _id: Types.ObjectId; - name: string; - body: string; - allowedTools?: string[]; -} +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 diff --git a/packages/api/src/skills/import.ts b/packages/api/src/skills/import.ts index d83ae97be76b..f0223b8700ce 100644 --- a/packages/api/src/skills/import.ts +++ b/packages/api/src/skills/import.ts @@ -93,17 +93,20 @@ export function parseFrontmatter(raw: string): { 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') { - // The outer `value` was already run through `unquoteYaml`, which - // only handles whole-line quoting. For `always-apply: "true" # note` - // the quote check misses (line doesn't end with a quote), so strip - // the comment first and then unquote the remainder. - const stripped = stripYamlTrailingComment(value).trim(); + // 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. From abaf2843e2226e67fcfc7d3259804f08f1d815fe Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 20 Apr 2026 22:44:34 -0400 Subject: [PATCH 5/6] =?UTF-8?q?=F0=9F=94=A7=20fix(tsc):=20Cast=20mock.call?= =?UTF-8?q?s=20via=20`unknown`=20for=20strict=20tuple=20destructure?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `getSkillByName.mock.calls[0]` is typed as `[]` by jest's generic default; a direct cast to `[string, ..., ...]` fails TS2352 under `--noEmit` even though the runtime shape matches. Go through `as unknown as [...]` like the earlier test in the same file so CI's type-check step stays green. --- packages/api/src/agents/handlers.spec.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/api/src/agents/handlers.spec.ts b/packages/api/src/agents/handlers.spec.ts index 237cd27a51ac..06069b4e458d 100644 --- a/packages/api/src/agents/handlers.spec.ts +++ b/packages/api/src/agents/handlers.spec.ts @@ -557,11 +557,13 @@ describe('createToolExecuteHandler', () => { }, ]); - const [, accessibleIdsArg, lookupOptions] = getSkillByName.mock.calls[0] as [ + 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 From 06b7aed718855717948a0608c11c4913ef36dbab Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 20 Apr 2026 22:58:04 -0400 Subject: [PATCH 6/6] =?UTF-8?q?=F0=9F=AA=A2=20fix:=20Propagate=20skillPrim?= =?UTF-8?q?edIdsByName=20into=20handoff=20agent=20tool=20context?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Handoff agents go through the same `initializeAgent` flow as the primary (with `listAlwaysApplySkills` now plumbed), so they resolve their own `manualSkillPrimes` and `alwaysApplySkillPrimes` — but the `agentToolContexts.set(...)` for handoff agents didn't carry `skillPrimedIdsByName` into the per-agent context. That meant `handleReadFileCall` fell back to the full ACL set + a `prefer*` flag for handoff agents: same-name collisions could resolve to a different doc than the one whose body got primed, and a `disable-model-invocation: true` skill primed via manual `$` or always-apply inside the handoff flow would be blocked from reading its own bundled files. Build the map via `buildSkillPrimedIdsByName(config.manualSkillPrimes, config.alwaysApplySkillPrimes)` for every handoff tool context so `read_file` behaves identically across primary and handoff agents. --- api/server/services/Endpoints/agents/initialize.js | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/api/server/services/Endpoints/agents/initialize.js b/api/server/services/Endpoints/agents/initialize.js index 452aff285dab..f15c965cf891 100644 --- a/api/server/services/Endpoints/agents/initialize.js +++ b/api/server/services/Endpoints/agents/initialize.js @@ -404,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, @@ -412,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);