dockerize app, improve ui/ux, add setup instructions - #7
Merged
Merged
Conversation
…atgpt-clone into easy-config-setup
danny-avila
added a commit
that referenced
this pull request
Apr 6, 2023
dockerize app, improve ui/ux, add setup instructions
ClaraLeigh
pushed a commit
to ClaraLeigh/chatgpt-clone
that referenced
this pull request
May 4, 2023
…oauth-passport Feat client server oauth passport
danny-avila
added a commit
that referenced
this pull request
Jun 20, 2024
* WIP: MERGE prompts/frontend (#1) * added schema for prompt and promptgroup, added model methods for prompts, added routes for prompts * * updated promptGroup Schema * updated model methods for prompts (get, add, delete) * slight fixes in prompt routes * * Created Files Management components * Created Vector Stores components * Added file management route in the routes folder * Completed UI for Files list, Compeleted UI for vector stores list, Completed UI for upload file modal, Completed UI for preview file, Completed UI for preview vector store * Fixed style and UI fixes for file dashboard, file list and vector stores list * added responsiveness classes for vector store page * fixed responsiveness of file page, dashboard page, and main page * fixed styling and responsiveness issues on dashboard page, file list page and vector store page * added queries and mutations for prompts and promptGroups, added relevant endpoints in data-provider, added relevant components prompts, added and updated relevant APIs * added types on mutation queries data service, updated prompt attributes * feature: Prompts and prompt groups management, added relevant APIs, added types for data service/queries/mutations, added relevant mutation and queries * chore: typing clarifications * added drop down on prompts mgmt dashboard * Fixes: fixed version switching issue on tags update or labels update, added cross button on create prompt group, fixed list updation on prompt group renaiming, added CSV upload button * Feature: Added oneliner and category attributes in prompt group, added schema for categories, added schema methods and route for categories * chore: typing and lint issues * chore: more type and linter fixes * chore: linting * chore: prompt controller and backend typing example; MOVE TO CONTROLLER DIRECTORY * chore: more type fixes * style: prompt name changes * chore: more type changes, and stateful prompt name change without flickering * fix: Return result of savePrompt in patchPrompt API endpoint * fix: navigation prompt queries; refactor: name 'prompt-groups' to just 'groups' * refactor: fetch prompt groups rewrite * refactor(prompts): query/mutation statefulness * refactor: remove `isActive` field * refactor: remove labels, consolidate logic * style: width, layout shift * refactor: improve hover toggle behavior and styling * refactor: add useParams hook to PromptListItem for dynamic rendering and add timeout ref for blur timeout * chore: hide upload button * refactor: import Button component from correct location in PromptSidePanel * style: prompt editor styling * style: fix more layout shifts * style: container scroll * refactor: Rename CreatePrompt component to CreatePromptForm * refactor: use react-hook-form * refactor: Add Prompts components and routes to Dashboard * style: skeletons for loading * fix: optimize makePromptProduction * refactor: consolidate variables * feat: create prompt form validation * refactor: Consolidate variables and update mutation hooks * style: minor touchups * chore: Update lucide-react npm dependency to version 0.394.0 and npm audit fix * refactor: add a new icon for the Prompts heading. * style: Update PromptsView heading to use h1 instead of h2 and other minor margin issues * chore: wording * refactor: Update PromptsView heading to use h1 instead of h2, consolidate variables, and add new icons * refactor: Prompts Button for Mobile * feature: added category field in prompt group, added relevant API and static data on BE to support FE UI for category in prompt group * chore: template for prompt cards --------- Co-authored-by: Fawadpot <contactfawada@gmail.com> * WIP: Prompts/frontend Continued (#2) * chore: loading style, remove unused component * feat: Add CategorySelector component for prompt group category selection * feat: add categories to create prompt * feat: prompt versions styling * feat: optimistic updates for prompt production state * refactor: optimize form state and show if prompt field is dirty with cross icon, also other styling changes * chore: remove unused code and localizations * fix: light mode styling * WIP: SidePanel Prompts * refactor: move to groups directory * refactor: rename GroupsSidePanel to GroupSidePanel and update imports * style: ListCard * refactor: isProduction changes * refactor: infinite query with productionPrompt * refactor: optimize snippets and prompts, and styling * refactor: Update getSnippet function to accept a length parameter * chore: localizations * feat: prompts navigation to chat and vice versa * fix: create prompt * feat: remember last selected category for creating prompts * fix(promptGroups): fix pagination and add usePromptGroupsNav hook * Prompts/frontend 3 (#3) * fix: stateful issues with prompt groups * style: improved layout * refactor: improve variable naming in Eng.ts * refactor: theme selector styling improvements * added prompt cards on chat new page, with dark mode, added API to fetch random prompts, added types for useQuery Slightly improved usePromptGroupNav logic to fetch updated result for pageSize, updated prompt cards view with darkmode and responsiveness fixed page size option buttons styling to match the theme added dark mode on create prompt page and prompt edit/preview page fixed page size option buttons styling to match the theme added dark mode on create prompt page and prompt edit/preview page * WIP: Prompts/frontend (#4) * fix: optimize and fix paginated query * fix: remove unique constraint on names * refactor: button links and styling * style: menu border light mode * feat: Add Auto-Send Switch component for prompts groups * refactor(ChatView): use form context for submission text * chore: clear convo state on navigation to dashboard routes * chore: save prompt edit name on tab, remove console log * feat: basic prompt submission * refactor: move Auto-Send Switch * style(ListCard): border styling * feat: Add function to detect variables in text * feat: Add OriginalDialog component to UI library * chore(ui): Update SelectDropDown options list class to use text-xs size * refactor: submitMessage hook now includes submitPrompt, make compatible to document query selector * WIP: Variable Dialog * feat: variable submission working for both auto-send and non-autosend * feat: dashboard breadcrumbs and prompts/chat navigation * refactor: dashboard breadcrumb and dashboard link to chat navigation * refactor: Update VariableDialog and VariableForm styles * Prompts: Admin features (#5) * fix: link issue * fix: usePromptGroupsNav add missing dep. * style: dashbreadcrumb and sidepanel text color * temp fix: remove refetch on pageNumber change * fix: handle multiple variable replacement * WIP: create project schema and add project groups to fetch * feat: Add functionality to add prompt group IDs to a project * feat: Add caching for startup config in config route * chore: remove prompt landing * style: Update Skeleton component with additional background styling * chore: styling and types * WIP: SharePrompt first draft * feat(SharePrompt): form validation * feat: shared global indicators * refactor: prompt details * refactor: change NoPromptGroup directory * feat: preview prompt * feat: remove/add global prompts, add rbac-related enums * refactor: manage prompts location * WIP: first draft admin settings for prompts * feat: SystemRoles enum * refactor: update PromptDetails component styling * style: ellipsis custom class for showing more preview text * WIP: initial role schema and initialization * style: improved margins for single unordered lists * fix: use custom chat form context to prevent re-renders from FormProvider * feat: Role mutations for Prompt Permissions * feat: fetch user role * feat: update AdminSettings form default values from user role values * refactor: rename PromptPermissions to Permissions for general definitions * feat: initial role checks * feat: Add optional `bodyProps` parameter to generateCheckAccess middleware * refactor: UI access checks * Prompts: delete (#6) * Fixed delete prompt version API, fixed types and logic for prompt version deletion, updated prompt delete mutation logic * chore: Update return type of deletePrompt function in Prompt.js --------- Co-authored-by: Fawadpot <contactfawada@gmail.com> * chore: Update package-lock.json version to 0.7.4-rc1 and fast-xml-parser to 4.4.0 * feat: toast for saving admin settings, add timer no-access navigation * feat: always make prod * feat: Add localization to category labels in CategorySelector component * feat: Update category label localization in CategorySelector component * fix: Enable making prompt production in Prompt API --------- Co-authored-by: Fawadpot <contactfawada@gmail.com>
danny-avila
added a commit
that referenced
this pull request
Jun 21, 2024
* 🗨️ feat: Prompts (#7) * WIP: MERGE prompts/frontend (#1) * added schema for prompt and promptgroup, added model methods for prompts, added routes for prompts * * updated promptGroup Schema * updated model methods for prompts (get, add, delete) * slight fixes in prompt routes * * Created Files Management components * Created Vector Stores components * Added file management route in the routes folder * Completed UI for Files list, Compeleted UI for vector stores list, Completed UI for upload file modal, Completed UI for preview file, Completed UI for preview vector store * Fixed style and UI fixes for file dashboard, file list and vector stores list * added responsiveness classes for vector store page * fixed responsiveness of file page, dashboard page, and main page * fixed styling and responsiveness issues on dashboard page, file list page and vector store page * added queries and mutations for prompts and promptGroups, added relevant endpoints in data-provider, added relevant components prompts, added and updated relevant APIs * added types on mutation queries data service, updated prompt attributes * feature: Prompts and prompt groups management, added relevant APIs, added types for data service/queries/mutations, added relevant mutation and queries * chore: typing clarifications * added drop down on prompts mgmt dashboard * Fixes: fixed version switching issue on tags update or labels update, added cross button on create prompt group, fixed list updation on prompt group renaiming, added CSV upload button * Feature: Added oneliner and category attributes in prompt group, added schema for categories, added schema methods and route for categories * chore: typing and lint issues * chore: more type and linter fixes * chore: linting * chore: prompt controller and backend typing example; MOVE TO CONTROLLER DIRECTORY * chore: more type fixes * style: prompt name changes * chore: more type changes, and stateful prompt name change without flickering * fix: Return result of savePrompt in patchPrompt API endpoint * fix: navigation prompt queries; refactor: name 'prompt-groups' to just 'groups' * refactor: fetch prompt groups rewrite * refactor(prompts): query/mutation statefulness * refactor: remove `isActive` field * refactor: remove labels, consolidate logic * style: width, layout shift * refactor: improve hover toggle behavior and styling * refactor: add useParams hook to PromptListItem for dynamic rendering and add timeout ref for blur timeout * chore: hide upload button * refactor: import Button component from correct location in PromptSidePanel * style: prompt editor styling * style: fix more layout shifts * style: container scroll * refactor: Rename CreatePrompt component to CreatePromptForm * refactor: use react-hook-form * refactor: Add Prompts components and routes to Dashboard * style: skeletons for loading * fix: optimize makePromptProduction * refactor: consolidate variables * feat: create prompt form validation * refactor: Consolidate variables and update mutation hooks * style: minor touchups * chore: Update lucide-react npm dependency to version 0.394.0 and npm audit fix * refactor: add a new icon for the Prompts heading. * style: Update PromptsView heading to use h1 instead of h2 and other minor margin issues * chore: wording * refactor: Update PromptsView heading to use h1 instead of h2, consolidate variables, and add new icons * refactor: Prompts Button for Mobile * feature: added category field in prompt group, added relevant API and static data on BE to support FE UI for category in prompt group * chore: template for prompt cards --------- Co-authored-by: Fawadpot <contactfawada@gmail.com> * WIP: Prompts/frontend Continued (#2) * chore: loading style, remove unused component * feat: Add CategorySelector component for prompt group category selection * feat: add categories to create prompt * feat: prompt versions styling * feat: optimistic updates for prompt production state * refactor: optimize form state and show if prompt field is dirty with cross icon, also other styling changes * chore: remove unused code and localizations * fix: light mode styling * WIP: SidePanel Prompts * refactor: move to groups directory * refactor: rename GroupsSidePanel to GroupSidePanel and update imports * style: ListCard * refactor: isProduction changes * refactor: infinite query with productionPrompt * refactor: optimize snippets and prompts, and styling * refactor: Update getSnippet function to accept a length parameter * chore: localizations * feat: prompts navigation to chat and vice versa * fix: create prompt * feat: remember last selected category for creating prompts * fix(promptGroups): fix pagination and add usePromptGroupsNav hook * Prompts/frontend 3 (#3) * fix: stateful issues with prompt groups * style: improved layout * refactor: improve variable naming in Eng.ts * refactor: theme selector styling improvements * added prompt cards on chat new page, with dark mode, added API to fetch random prompts, added types for useQuery Slightly improved usePromptGroupNav logic to fetch updated result for pageSize, updated prompt cards view with darkmode and responsiveness fixed page size option buttons styling to match the theme added dark mode on create prompt page and prompt edit/preview page fixed page size option buttons styling to match the theme added dark mode on create prompt page and prompt edit/preview page * WIP: Prompts/frontend (#4) * fix: optimize and fix paginated query * fix: remove unique constraint on names * refactor: button links and styling * style: menu border light mode * feat: Add Auto-Send Switch component for prompts groups * refactor(ChatView): use form context for submission text * chore: clear convo state on navigation to dashboard routes * chore: save prompt edit name on tab, remove console log * feat: basic prompt submission * refactor: move Auto-Send Switch * style(ListCard): border styling * feat: Add function to detect variables in text * feat: Add OriginalDialog component to UI library * chore(ui): Update SelectDropDown options list class to use text-xs size * refactor: submitMessage hook now includes submitPrompt, make compatible to document query selector * WIP: Variable Dialog * feat: variable submission working for both auto-send and non-autosend * feat: dashboard breadcrumbs and prompts/chat navigation * refactor: dashboard breadcrumb and dashboard link to chat navigation * refactor: Update VariableDialog and VariableForm styles * Prompts: Admin features (#5) * fix: link issue * fix: usePromptGroupsNav add missing dep. * style: dashbreadcrumb and sidepanel text color * temp fix: remove refetch on pageNumber change * fix: handle multiple variable replacement * WIP: create project schema and add project groups to fetch * feat: Add functionality to add prompt group IDs to a project * feat: Add caching for startup config in config route * chore: remove prompt landing * style: Update Skeleton component with additional background styling * chore: styling and types * WIP: SharePrompt first draft * feat(SharePrompt): form validation * feat: shared global indicators * refactor: prompt details * refactor: change NoPromptGroup directory * feat: preview prompt * feat: remove/add global prompts, add rbac-related enums * refactor: manage prompts location * WIP: first draft admin settings for prompts * feat: SystemRoles enum * refactor: update PromptDetails component styling * style: ellipsis custom class for showing more preview text * WIP: initial role schema and initialization * style: improved margins for single unordered lists * fix: use custom chat form context to prevent re-renders from FormProvider * feat: Role mutations for Prompt Permissions * feat: fetch user role * feat: update AdminSettings form default values from user role values * refactor: rename PromptPermissions to Permissions for general definitions * feat: initial role checks * feat: Add optional `bodyProps` parameter to generateCheckAccess middleware * refactor: UI access checks * Prompts: delete (#6) * Fixed delete prompt version API, fixed types and logic for prompt version deletion, updated prompt delete mutation logic * chore: Update return type of deletePrompt function in Prompt.js --------- Co-authored-by: Fawadpot <contactfawada@gmail.com> * chore: Update package-lock.json version to 0.7.4-rc1 and fast-xml-parser to 4.4.0 * feat: toast for saving admin settings, add timer no-access navigation * feat: always make prod * feat: Add localization to category labels in CategorySelector component * feat: Update category label localization in CategorySelector component * fix: Enable making prompt production in Prompt API --------- Co-authored-by: Fawadpot <contactfawada@gmail.com> * feat: Add helper fn for dark mode detection in ThemeProvider * style: surface-primary definition * fix(useHasAccess): utilize user.role and not just USER role * fix: empty category and role fetch * refactort: increase max height to options list and use label if no localization is found * fix: update CategorySelector to handle empty category value and improve localization * refactor: move prompts to own store/reactquery modules, add in filter WIP * refactor: Rename AutoSendSwitch to AutoSendPrompt * style: theming commit * style: fix slight coloring issue for convos in dark mode * style: better composition for prompts side panel * style: remove gray-750 and make it gray-850 * chore: adjust theming * feat: filter all prompt groups and properly remove prompts from projects * refactor: optimize delete prompt groups further * chore: localization * feat: Add uniqueProperty filtering to normalizeData function * WIP: filter prompts * chore: Update FilterPrompts component to include User icon in FilterItem * feat(FilterPrompts): set categories * feat: more system filters and show selected category icon * style: always make prod, flips switch to avoid mis-clicks * style: ui/ux loading/no prompts * chore: style FilterPrompts ChatView * fix: handle missing role edge case * style: special variables * feat: special variables * refactor: improve replaceSpecialVars function in prompts.ts * feat: simple/advanced editor modes * chore: bump versions * feat: localizations and hide production button on simple mode * fix: error connecting layout shift * fix: prompts CRUD for admins * fix: secure single group fetch * style: sidepanel styling * style(PromptName): bring edit button closer to name * style: mobile prompts header * style: mobile prompts header continued * style: align send prompts switch right * feat: description * Update special variables description in Eng.ts * feat: update/create/preview oneliner * fix: allow empty oneliner update * style: loading improvement and always make selected prompt Production if simple mode * fix: production index set and remove unused props * fix(ci): mock initializeRoles * fix: address #3128 * fix: address #3128 * feat: add deletion confirmation dialog * fix: mobile UI issues * style: prompt library UI update * style: focus, logcal tab order * style: Refactor SelectDropDown component to improve code readability and maintainability * chore: bump data-provider * chore: fix labels * refactor: confirm delete prompt version --------- Co-authored-by: Marco Beretta <81851188+berry-13@users.noreply.github.com>
wilfoa
referenced
this pull request
in whiletrue-industries/LibreChat
Apr 19, 2026
Addresses RTL audit sections #7, #10, and the relevant rows of #11. - Web/SourceHovercard.tsx, Web/Citation.tsx: ml-/mr- and text-left replaced with ms-/me-/text-start so citation anchors, the favicon, and the hovercard "more details" disclosure render on the correct side under Hebrew. - Chat/Messages hot path (MinimalHoverButtons, HoverButtons, SearchButtons, Fork, ToolCall, WebSearch, AgentHandoff, Thinking, AgentUpdate, EditMessage, Parts/EditTextPart, Input/MentionItem, Input/ChatForm): blanket swap of ml-/mr- to ms-/me-, pl-/pr- to ps-/pe-, and the absolute left-/right- positions on the thinking copy button and agent-update branch line to logical start-/end- (including the -start-6 / start-0 on the AgentUpdate branch). Textarea classes in EditMessage and EditTextPart are now `text-start` since the explicit `dir` attribute on each textarea already anchors text to the leading edge. - Nav/AccountSettings, Nav/Settings: text-left -> text-start, -ml-[8px] -> -ms-[8px], md:pr-0.5 -> md:pe-0.5, and the inline `marginLeft` style uses `marginInlineStart` so the avatar and settings tab strip render correctly under RTL. - Conversations/Conversations.tsx, Convo.tsx, ConvoOptions/ConvoOptions.tsx, Nav/Favorites/FavoriteItem.tsx, Nav/Favorites/FavoritesList.tsx: pl-/pr-/ml-/mr-/right- swapped to ps-/pe-/ms-/me-/end- so conversation rows, menu icons, and favorite rows mirror cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
danny-avila
added a commit
that referenced
this pull request
Apr 19, 2026
Codex P2 + reviewer #1: Single `loadTools` call with the union of `agent.tools + allowed-tools`. The earlier two-call approach dropped `userMCPAuthMap` / `toolContextMap` / `actionsEnabled` from the skill-added pass — an MCP tool gained via `allowed-tools` would be visible to the model but fail at execution without per-user auth context. Resolution of `manualSkillPrimes` is hoisted before `loadTools` so the union can be computed up-front; the dropped-tools debug log now compares loaded vs. requested across the single call. Codex P3 + reviewer #2: `injectSkillCatalog.activeSkillIds` now includes `disable-model-invocation: true` skills. The runtime ACL check in `handleSkillToolCall` previously couldn't reach the explicit "cannot be invoked by the model" rejection because the broader access set excluded those skills. Catalog text and tool registration still gate on the visible subset (zero-context-token guarantee preserved); only the per-user `isActive` filter is a hard exclusion now. Reviewer #1 (try/catch around loadTools, MAJOR): A single bad `allowed-tools` entry from a shared skill could crash the entire turn. Now wrapped — on failure with extras, retry with just `agent.tools` and continue (the dropped-tools debug log surfaces what vanished). If the retry-without-extras still throws, propagate; the agent's own tools are the load-bearing surface. Reviewer #3 (integration tests, MAJOR): Added six tests in `initialize.test.ts` covering the full `allowed-tools` loading path: union pass-through, no-extras short-circuit, agent-baseline dedup, loadTools throw + retry, propagated throw without extras, and the empty-tools edge case. Smaller cleanups bundled in: - Reviewer #4: Moved `logger` import to the package-imports section (was wedged among local imports). - Reviewer #5: Removed unused index on `disableModelInvocation` (filtering happens application-side in `injectSkillCatalog`; index cost write overhead for zero query benefit). - Reviewer #6: Swapped order of `userInvocable` and body checks in `resolveManualSkills` so the more authoritative author-decision reason surfaces first when both apply. - Reviewer #8: Documented the `allowedTools` enforcement gap on the schema + type — model-invoked skills (mid-turn `skill` tool calls) do NOT trigger tool union, since adding tools after the graph starts would require a rebuild. Manual / always-apply (Phase 5) primes are the supported paths. - Reviewer #9: Renamed `dmi` / `ui` / `at` locals to `disableModelInvocationRaw` / `userInvocableRaw` / `allowedToolsRaw` in `deriveStructuredFrontmatterFields`. Reviewer #7 (DRY shared `getSkillByName` return type) deferred — field sets diverge meaningfully across the three call sites (handler needs `body + fileCount`; resolver needs `author + allowedTools + userInvocable`; the InitializeAgentDbMethods contract needs the superset). A `Pick<>`-based consolidation is a follow-up cleanup.
danny-avila
added a commit
that referenced
this pull request
Apr 20, 2026
…ocs) Sanity-pass review surfaced 7 findings; addressed 6 (the 7th — DRY on inline `getSkillByName` return types — is acknowledged tech debt deferred to a follow-up). #1 [MAJOR, DRY]: The 4-line `manualSkillPrimedIdsByName` map construction was duplicated across 4 CJS call sites (openai.js, responses.js x2, initialize.js). Extracted `buildManualSkillPrimedIdsByName` helper in `skillDeps.js`; all four sites now call the helper. If `ResolvedManualSkill` ever renames `_id` or gains identifying fields, only the helper changes. #2 [MINOR, type safety]: `handleReadFileCall` was casting a hex string to `Types.ObjectId[]` via `as unknown as`, relying on mongoose's auto-cast in `$in` queries. Replaced with `new Types.ObjectId(...)` so any future consumer comparing with `.equals()` / `===` gets the correct value type. Imported `Types` as a value (was type-only). #5 [MINOR, test gap]: Added a test for the worst-case silent-failure path — both the union and base-only `loadTools` calls return undefined. The agent gets no tools but the turn doesn't crash hard; pinning that contract. #4 [MINOR, performance]: Added a TODO on the `listSkillsByAccess` projection noting the `frontmatter` field can be dropped once a write migration backfills all pre-Phase-6 skills' columns. ~2KB/skill × 100/page is wasted bandwidth post-backfill. #6 [NIT, docs]: `backfillDerivedFromFrontmatter` JSDoc said "Pure" right before "mutates its undefined fields in place". Replaced with "Side-effect-free w.r.t. the DB (no writes), but mutates its argument in place" which describes both halves accurately. #7 [NIT, test determinism]: Replaced `await new Promise(r => setTimeout(r, 5))` in two same-name collision tests with explicit `updateOne` setting `updatedAt: new Date(Date.now() - 1000)` on the older doc. Removes the wall-clock race on fast CI runners. The pagination test (line 480) still uses setTimeout — that test is pre-existing and order is incidental, not load-bearing. Existing test fixtures updated to use valid 24-char hex ObjectIds (required by the iter-9 test that constructs a real `ObjectId`). #3 [MINOR, deferred]: Inline `getSkillByName` return type duplicated across `handlers.ts`, `initialize.ts`, `skills.ts`. Reviewer acknowledged this as deferred; field sets diverge across call sites (handler needs `fileCount`, resolver needs `author`/`allowedTools`). A `Pick<>`-based consolidation is a clean follow-up.
danny-avila
added a commit
that referenced
this pull request
Apr 20, 2026
…le` / `allowed-tools` (#12745) * 🧬 feat: Persist `disable-model-invocation` / `user-invocable` / `allowed-tools` Adds first-class columns mirroring the three runtime-enforced frontmatter fields, with a `deriveStructuredFrontmatterFields` helper that maps from frontmatter at create/update time and re-syncs (via `$unset`) when fields are removed. `listSkillsByAccess` projection includes them so the Phase 6 catalog filter and popover filter can both read off the summary row. Marks `invocationMode` as @deprecated on `TSkill` and the `InvocationMode` enum — the runtime now reads the persisted pair instead. * 🛡️ feat: Enforce frontmatter at runtime (catalog, skill tool, manual resolver, tool union) Wires the persisted columns into actual runtime behavior across all four invocation paths: - `injectSkillCatalog` excludes `disableModelInvocation: true` skills before catalog formatting — they cost zero context tokens and stay invisible to the model. - `handleSkillToolCall` rejects with a clear error when the model names a skill marked `disable-model-invocation: true` (defends against a stale-cache or hallucinated invocation getting past the catalog filter). - `resolveManualSkills` skips `userInvocable: false` skills with a warn log so an API-direct caller can't bypass the popover-side filter. - `unionPrimeAllowedTools` collects skill-declared `allowed-tools` minus what's already on the agent; `initialize.ts` re-runs `loadTools` for the extras and merges resulting `toolDefinitions` into the agent's effective set for the turn. Tool-name resolution is tolerant — unknown names silently drop with a debug log so cross-ecosystem skills referencing yet-to-be-implemented tools (Claude Code's `edit_file`, etc.) import without breaking. The agent document is never modified; the union is turn-scoped. Helper exports (`unionPrimeAllowedTools`) are structured so Phase 5's always-apply primes flow through the same union (combined `[...manualPrimes, ...alwaysApplyPrimes]`) once the resolver lands. Skill handler wire format gains the three fields so clients can render them on detail / list views. * 🎛️ feat: `$` popover reads `userInvocable` instead of UI-only `invocationMode` Replaces the phase-1 UI-only `invocationMode` check with the persisted `userInvocable` field (mirrors the `user-invocable` frontmatter). Skills authored with `user-invocable: false` no longer surface in the popover; the backend resolver enforces the same rule for defense-in-depth. Default-visible behavior is preserved: skills without an explicit `userInvocable` value (older rows, freshly imported skills that don't declare the field) stay visible — only an explicit `false` hides them. Test fixture updated to reflect the new field. * 🔧 fix: Address Phase 6 review findings Codex P2 + reviewer #1: Single `loadTools` call with the union of `agent.tools + allowed-tools`. The earlier two-call approach dropped `userMCPAuthMap` / `toolContextMap` / `actionsEnabled` from the skill-added pass — an MCP tool gained via `allowed-tools` would be visible to the model but fail at execution without per-user auth context. Resolution of `manualSkillPrimes` is hoisted before `loadTools` so the union can be computed up-front; the dropped-tools debug log now compares loaded vs. requested across the single call. Codex P3 + reviewer #2: `injectSkillCatalog.activeSkillIds` now includes `disable-model-invocation: true` skills. The runtime ACL check in `handleSkillToolCall` previously couldn't reach the explicit "cannot be invoked by the model" rejection because the broader access set excluded those skills. Catalog text and tool registration still gate on the visible subset (zero-context-token guarantee preserved); only the per-user `isActive` filter is a hard exclusion now. Reviewer #1 (try/catch around loadTools, MAJOR): A single bad `allowed-tools` entry from a shared skill could crash the entire turn. Now wrapped — on failure with extras, retry with just `agent.tools` and continue (the dropped-tools debug log surfaces what vanished). If the retry-without-extras still throws, propagate; the agent's own tools are the load-bearing surface. Reviewer #3 (integration tests, MAJOR): Added six tests in `initialize.test.ts` covering the full `allowed-tools` loading path: union pass-through, no-extras short-circuit, agent-baseline dedup, loadTools throw + retry, propagated throw without extras, and the empty-tools edge case. Smaller cleanups bundled in: - Reviewer #4: Moved `logger` import to the package-imports section (was wedged among local imports). - Reviewer #5: Removed unused index on `disableModelInvocation` (filtering happens application-side in `injectSkillCatalog`; index cost write overhead for zero query benefit). - Reviewer #6: Swapped order of `userInvocable` and body checks in `resolveManualSkills` so the more authoritative author-decision reason surfaces first when both apply. - Reviewer #8: Documented the `allowedTools` enforcement gap on the schema + type — model-invoked skills (mid-turn `skill` tool calls) do NOT trigger tool union, since adding tools after the graph starts would require a rebuild. Manual / always-apply (Phase 5) primes are the supported paths. - Reviewer #9: Renamed `dmi` / `ui` / `at` locals to `disableModelInvocationRaw` / `userInvocableRaw` / `allowedToolsRaw` in `deriveStructuredFrontmatterFields`. Reviewer #7 (DRY shared `getSkillByName` return type) deferred — field sets diverge meaningfully across the three call sites (handler needs `body + fileCount`; resolver needs `author + allowedTools + userInvocable`; the InitializeAgentDbMethods contract needs the superset). A `Pick<>`-based consolidation is a follow-up cleanup. * 🔧 fix: Address codex iter 2 — catalog quota + duplicate-name dedup P1: `injectSkillCatalog` cap now counts only model-visible skills, not the merged active set. The previous behavior let a tenant with many `disable-model-invocation: true` rows near the top of the cursor exhaust the 100-slot quota before any invocable skill got scanned — the catalog could end up empty even though invocable skills existed further down the paginated results. `MAX_CATALOG_PAGES` stays the ceiling on scan budget; only `visibleCount` drives the early-exit on quota fill. P2: When an invocable and a `disable-model-invocation: true` skill share a name, drop the disabled doc(s) from `activeSkillIds`. Without this dedup, `getSkillByName` (which sorts by `updatedAt` desc) could pick the disabled doc and every model call to the cataloged name would fail with "cannot be invoked by the model" instead of executing the visible skill. When ONLY a disabled doc exists for a name, it stays in `activeSkillIds` so the explicit-rejection error path still fires for hallucinated invocations. Tests: 3 new cases in `injectSkillCatalog` covering (a) cap counted on visible skills only, (b) same-name collision drops disabled doc, (c) sole-disabled-name case keeps the disabled doc. * 🔒 fix: Apply `disable-model-invocation` gate to read_file too (codex iter 3 P1) `activeSkillIds` is shared between the `skill` and `read_file` handlers. The skill-tool gate was applied last iteration, but `handleReadFileCall` authorized purely on `getSkillByName(..., accessibleIds)` — so a model that learned a hidden skill's name (stale catalog or hallucination) could still read its `SKILL.md` body or bundled files via `read_file`, defeating the contract. Same explicit rejection now fires from both handlers; no change needed to the ACL set itself (disabled docs stay in `activeSkillIds` so the explicit error path keeps firing). Two new tests in `handlers.spec.ts` cover the read_file gate and regression-protect the happy path. * 🔧 fix: Address codex iter 4 — manual-prime exception + legacy frontmatter backfill P1: Scope the `read_file` `disableModelInvocation` gate to AUTONOMOUS model probes only. A user-invoked `$` skill that is also marked `disable-model-invocation: true` had its bundled `references/*` / `scripts/*` files unreadable, leaving the manually-primed skill body referencing files the model couldn't load. Now the handler bypasses the gate when the skill name appears in `manualSkillNames` (the per-turn allowlist threaded from `manualSkillPrimes` → `agentToolContexts` → `enrichWithSkillConfigurable` → `mergedConfigurable`). Defense-in-depth: the bypass is scoped to the specific names in the allowlist; a different disabled skill name is still rejected. P2: Read-time fallback for legacy skills authored before Phase 6 landed the structured columns. `user-invocable: false` / `disable-model-invocation: true` set in `frontmatter` (the validator already accepted those keys) but with no derived column would incorrectly evaluate as "user-invocable / model-allowed" until a save backfilled the columns. New `backfillDerivedFromFrontmatter` helper fills undefined columns from frontmatter at read time in both `getSkillByName` and `listSkillsByAccess` — column wins when both are set, frontmatter fills the gap when only it's set. No DB writes; the next `updateSkill` naturally persists. `listSkillsByAccess` projection expanded to include `frontmatter` (bounded by validator, payload impact small) so summaries can also be backfilled. Sticky-primed disabled skills (ones invoked in prior turns of the same conversation) are not yet in the manual-prime allowlist — same- turn manual invocation is the load-bearing path codex flagged; the sticky-turn case is a known limitation tracked for a follow-up. Tests: 2 new in handlers.spec.ts (manual-prime allows + name-scoped block holds), 3 new in skill.spec.ts (legacy backfill via getSkillByName + listSkillsByAccess + column-wins precedence). * 🔧 fix: Address codex iter 5 — propagate manualSkillNames + keep read_file P1: `enrichWithSkillConfigurable` is also called from `openai.js` and `responses.js` (the OpenAI Responses + completions endpoints). Both were ignoring the new `manualSkillNames` parameter, which meant the manual-prime exception in the `read_file` gate (iter 4) only worked on the agents endpoint. Now all three call sites pass `primaryConfig.manualSkillPrimes?.map(p => p.name)` so manual `$` invocations of disabled skills work consistently across endpoints. P2: When every accessible skill is `disable-model-invocation: true`, the catalog text and `skill` tool are correctly omitted (no model- reachable targets) — but `read_file` and `bash_tool` MUST still be registered. A user manually invoking such a skill gets its SKILL.md body primed into context; if the body references `references/foo.md` or `scripts/run.sh`, those reads need a registered tool. Restructured `injectSkillCatalog` so `skill` registration is gated on `catalogVisibleSkills.length > 0` while `read_file` (always) and `bash_tool` (when codeEnvAvailable) register whenever any active skill is in scope. Tests: existing all-disabled test rewritten to assert read_file IS registered + skill is NOT; new test confirms bash_tool joins it when codeEnvAvailable. * 🔧 fix: Address codex iter 6 — name-collision consistency via preferInvocable P2a (resolveManualSkills): a name collision between an older user-invocable doc and a newer non-user-invocable doc made manual `$` invocation silently no-op. The popover surfaced the older invocable doc; resolver looked it up by name; `getSkillByName` returned the newer non-invocable doc; resolver skipped on `userInvocable: false`. P2b (handler / runtime ACL): with same-name duplicates (e.g. older invocable + newer disabled), the manual prime resolved to one doc while later `read_file` / `skill` execution resolved a different doc through `activeSkillIds`. Model could follow one SKILL.md body while reading files from a different skill. Both root-cause: `getSkillByName` always returned the newest match and let the caller filter, but with collisions the newest can be something the caller didn't want. Fix: extend `getSkillByName` with `options.preferInvocable`. When true, prefer the newest doc satisfying BOTH `userInvocable !== false` AND `disableModelInvocation !== true` (with frontmatter backfill); fall back to the newest match otherwise. Fast path preserved when caller doesn't opt in. Callers passing `preferInvocable: true`: - `resolveManualSkills` — picks the popover-visible invocable doc even when a newer disabled / non-user-invocable duplicate exists. - `handleSkillToolCall` — keeps execution aligned with the catalog; falls back to the disabled doc only when no invocable variant exists (so the explicit "cannot be invoked by the model" gate still fires for the hallucinated-disabled-name case). - `handleReadFileCall` — same alignment, plus the manual-prime exception added in iter 4 still applies. Tests: - 2 new in skill.spec.ts (preferInvocable picks invocable when collision exists; falls back to newest when no clean-invocable exists). - 1 new in skills.test.ts (resolver passes preferInvocable through). - 2 new in handlers.spec.ts (skill tool + read_file pass it). - Existing initialize.test.ts assertion updated for the new option. * 🔧 fix: Address codex iter 7 — split preferInvocable into per-axis flags The previous unified `preferInvocable` filter required BOTH `userInvocable !== false` AND `disableModelInvocation !== true`. That was wrong for the model paths: `userInvocable: false` skills are model-only and remain valid `skill` / `read_file` invocation targets. A duplicate-name scenario where the newer cataloged doc was model- only would let the older user-invocable variant shadow it on every model call. Split the option into two independent axes: - `preferUserInvocable` — for manual paths (`$` popover). Skips docs with `userInvocable: false`. Disable-model-invocation status is irrelevant; iter 4 explicitly supports manual prime of disabled skills. - `preferModelInvocable` — for model paths (`skill` / `read_file` handlers). Skips docs with `disableModelInvocation: true`. User- invocable status is irrelevant; model-only skills are valid here. Both flags fall back to the newest match when no preferred doc exists, so the explicit-rejection error paths still fire correctly in the sole-disabled-name case. Callers updated: - `resolveManualSkills` → `preferUserInvocable: true` - `handleSkillToolCall` / `handleReadFileCall` → `preferModelInvocable: true` Tests: - New spec test for preferModelInvocable not filtering on userInvocable. - Existing preferInvocable test renamed/split to cover the new axes. - New test asserts preferUserInvocable still returns disabled docs (preserves iter 4 manual-disabled support). - Caller tests assert each path passes the right single flag and does NOT pass the wrong one. * 🔧 fix: TypeScript type-check failure in handlers.spec.ts (CI green) `jest.fn(async () => ...)` without explicit args infers an empty tuple for the call signature, so `mock.calls[0][2]` flagged as "Tuple type '[]' has no element at index '2'." Cast to `unknown[]` then narrow to the expected option shape. Behavior unchanged. Caught by the `Type check @librechat/api` CI step (.github/workflows/backend-review.yml). * 🔧 fix: Address codex iter 8 — undefined-result fallback + read_file alignment P1 (loadTools returning undefined): Production loaders (`createToolLoader` in `initialize.js` / `openai.js` / `responses.js`) wrap `loadAgentTools` in try/catch and return `undefined` on failure rather than throwing. Without explicit handling, my iter-1 try/catch only fired for thrown errors — a silent-failure on a skill-added tool would fall through to the empty fallback and silently DROP the agent's baseline tools for the turn (much worse than just losing the extras). Added an `undefined`-result branch that retries with just `agent.tools`, mirroring the throw branch. Test pins both behaviors. P2 (read_file alignment with manual prime): When a skill is in this turn's `manualSkillNames`, the `read_file` handler now uses `preferUserInvocable` instead of `preferModelInvocable`. Same name-collision rule as `resolveManualSkills`, so the doc whose files get read is the same doc whose body got primed. For autonomous probes (skill not in `manualSkillNames`), the handler keeps `preferModelInvocable` to align with the catalog the model saw. Two new tests cover both branches and regression-protect that the wrong flag isn't passed. * 🔧 fix: Address codex iter 9 — pin read_file lookup to primed skill _id P1 (manually-primed disabled IDs were dropped from activeSkillIds): The `executableSkills` dedup in `injectSkillCatalog` correctly drops `disable-model-invocation: true` duplicates when an invocable doc shares the name — but `resolveManualSkills` legitimately primes disabled docs (iter 4 supports manual `$` invocation of disabled skills). When the resolver primed a disabled doc, the read_file handler couldn't find it in the (deduped) `activeSkillIds` and either resolved a different same-name skill or returned not-found. Fix: `ResolvedManualSkill` now carries `_id`; the legacy `initialize.js` / `openai.js` / `responses.js` controllers build a `manualSkillPrimedIdsByName` map and `enrichWithSkillConfigurable` passes it into `mergedConfigurable`. `handleReadFileCall` now pins its lookup's `accessibleIds` to `[primedId]` whenever the requested skill is in the map. The constrained set guarantees the lookup returns the EXACT doc the resolver primed — body/files come from the same source even when same-name duplicates exist or the dedup removed the prime's id from `activeSkillIds`. Autonomous read_file probes (skill not in the manual-primed map) keep the full ACL set + `preferModelInvocable` so they align with the catalog the model saw and the disabled-only case still fires the explicit-rejection gate. Test fixture changes flow from `_id` becoming required on `ResolvedManualSkill`. `buildSkillPrimeContentParts` / `injectManualSkillPrimes` widen their param types to `Pick<...>` because they only read `name` / `body` and shouldn't force test literals to invent placeholder ids. * 🧹 fix: Address independent reviewer findings (DRY + types + tests + docs) Sanity-pass review surfaced 7 findings; addressed 6 (the 7th — DRY on inline `getSkillByName` return types — is acknowledged tech debt deferred to a follow-up). #1 [MAJOR, DRY]: The 4-line `manualSkillPrimedIdsByName` map construction was duplicated across 4 CJS call sites (openai.js, responses.js x2, initialize.js). Extracted `buildManualSkillPrimedIdsByName` helper in `skillDeps.js`; all four sites now call the helper. If `ResolvedManualSkill` ever renames `_id` or gains identifying fields, only the helper changes. #2 [MINOR, type safety]: `handleReadFileCall` was casting a hex string to `Types.ObjectId[]` via `as unknown as`, relying on mongoose's auto-cast in `$in` queries. Replaced with `new Types.ObjectId(...)` so any future consumer comparing with `.equals()` / `===` gets the correct value type. Imported `Types` as a value (was type-only). #5 [MINOR, test gap]: Added a test for the worst-case silent-failure path — both the union and base-only `loadTools` calls return undefined. The agent gets no tools but the turn doesn't crash hard; pinning that contract. #4 [MINOR, performance]: Added a TODO on the `listSkillsByAccess` projection noting the `frontmatter` field can be dropped once a write migration backfills all pre-Phase-6 skills' columns. ~2KB/skill × 100/page is wasted bandwidth post-backfill. #6 [NIT, docs]: `backfillDerivedFromFrontmatter` JSDoc said "Pure" right before "mutates its undefined fields in place". Replaced with "Side-effect-free w.r.t. the DB (no writes), but mutates its argument in place" which describes both halves accurately. #7 [NIT, test determinism]: Replaced `await new Promise(r => setTimeout(r, 5))` in two same-name collision tests with explicit `updateOne` setting `updatedAt: new Date(Date.now() - 1000)` on the older doc. Removes the wall-clock race on fast CI runners. The pagination test (line 480) still uses setTimeout — that test is pre-existing and order is incidental, not load-bearing. Existing test fixtures updated to use valid 24-char hex ObjectIds (required by the iter-9 test that constructs a real `ObjectId`). #3 [MINOR, deferred]: Inline `getSkillByName` return type duplicated across `handlers.ts`, `initialize.ts`, `skills.ts`. Reviewer acknowledged this as deferred; field sets diverge across call sites (handler needs `fileCount`, resolver needs `author`/`allowedTools`). A `Pick<>`-based consolidation is a clean follow-up.
9 tasks
danny-avila
added a commit
that referenced
this pull request
Apr 21, 2026
…ing (#12649) * feat: Skill runtime integration — catalog injection, tool registration, execute handler Wires the @librechat/agents SkillTool primitive into LibreChat's agent runtime: **Enums:** - Add `skills` to AgentCapabilities + defaultAgentCapabilities **Data layer:** - Add `getSkillByName(name, accessibleIds)` — compound query that combines name lookup + ACL check in one findOne **Agent initialization (packages/api/src/agents/initialize.ts):** - Accept `accessibleSkillIds` param and `listSkillsByAccess` db method - Query accessible skills, format catalog via `formatSkillCatalog()`, append to `additional_instructions` (appears in agent system prompt) - Register `SkillToolDefinition` + `createSkillTool()` when catalog is non-empty (tool appears in model's tool list) - Store `accessibleSkillIds` and `skillCount` on InitializedAgent **Execute handler (packages/api/src/agents/handlers.ts):** - Add `getSkillByName` to `ToolExecuteOptions` - `handleSkillToolCall()` intercepts `Constants.SKILL_TOOL`: extracts skillName, loads body from DB with ACL check, substitutes $ARGUMENTS, returns ToolExecuteResult with injectedMessages (skill body as isMeta user message) **Caller wiring:** - initialize.js: query skill IDs via findAccessibleResources, pass to initializeAgent + store on agentToolContexts, add getSkillByName to toolExecuteOptions, pass accessibleSkillIds through loadTools configurable - openai.js + responses.js: same pattern for their flows Requires @librechat/agents >= 3.1.65 (PR #91 exports). * feat: Skills toggle in tools menu + backend capability gating Frontend: - Add skills?: boolean to TEphemeralAgent type - Add LAST_SKILLS_TOGGLE_ to LocalStorageKeys for persistence - Add skillsEnabled to useAgentCapabilities hook - Add skills useToolToggle to BadgeRowContext with localStorage init - New Skills.tsx badge component (Scroll icon, cyan theme, permission-gated via PermissionTypes.SKILLS) - Add skills entry to ToolsDropdown with toggle + pin - Render Skills badge in BadgeRow ephemeral section Backend: - Extract injectSkillCatalog() into packages/api/src/agents/skills.ts (reduces initializeAgent module size, reusable helper) - initializeAgent delegates to helper instead of inline block - Capability-gate the findAccessibleResources query: - Agents endpoint: checks AgentCapabilities.skills in admin config - OpenAI/Responses controllers: checks ephemeralAgent.skills toggle - ACL query runs once per run, result shared across all agents * refactor: remove createSkillTool() instance from injectSkillCatalog SkillTool is event-driven only. The tool definition in toolDefinitions is sufficient for the LLM to see the tool schema. No tool instance is needed since the host handler intercepts via ON_TOOL_EXECUTE before tool.invoke() is ever called. Removes tools from InjectSkillCatalogParams/Result, drops the createSkillTool import. * feat: skill file priming, bash tool, and invoked skills state Multi-file skill support: - New primeSkillFiles() helper (packages/api/src/agents/skillFiles.ts) uploads skill files + SKILL.md body to code execution environment - handleSkillToolCall primes files on invocation when skill.fileCount > 0, returns session info as artifact so ToolNode stores the session - Skill-primed files available to subsequent bash/code tool calls Bash tool auto-registration: - BashExecutionToolDefinition added alongside SkillToolDefinition when skills are enabled, giving the model a bash tool for running scripts Conversation state: - Add invokedSkillIds field to conversation schema (Mongoose + Zod) - handleSkillToolCall updates conversation with $addToSet on success - Enables re-priming skill files on subsequent runs (future) Dependency wiring: - Pass listSkillFiles, getStrategyFunctions, uploadCodeEnvFile, updateConversation through ToolExecuteOptions - Pass req and codeApiKey through mergedConfigurable - All three controller entry points wired (initialize.js, openai.js, responses.js) * fix: load bash_tool instance in loadToolsForExecution, remove file listing - Add createBashExecutionTool to loadToolsForExecution alongside PTC/ToolSearch pattern: loads CODE_API_KEY, creates bash tool instance on demand - Add BASH_TOOL and SKILL_TOOL to specialToolNames set so they don't go through the generic loadTools path (bash is created here, skill is intercepted in handler before tool.invoke) - Remove file name listing from skill content text — it's the skill author's responsibility to disclose files in SKILL.md, not the framework * feat: batch upload for skill files, replace sequential uploads - Add batchUploadCodeEnvFiles() to crud.js: single POST to /upload/batch with all files in one multipart request, returns shared session_id - Rewrite primeSkillFiles to collect all streams (SKILL.md + bundled files) then do one batch upload instead of N sequential uploads - Replace uploadCodeEnvFile with batchUploadCodeEnvFiles across all callers (handlers.ts, initialize.js, openai.js, responses.js) * refactor: remove invokedSkillIds from conversation schema Skills aren't re-loaded between runs, so conversation-level state for invoked skills doesn't help. Skill state will live on messages instead (like tool_search discoveredTools and summaries), enabling in-place re-injection on follow-up runs. Removes invokedSkillIds from: convo Mongoose schema, IConversation interface, Zod schema, ToolExecuteOptions.updateConversation, and all three caller wiring points. * feat: smart skill file re-priming with session freshness checking Schema: - Add codeEnvIdentifier field to ISkillFile (type + Mongoose schema) - Add updateSkillFileCodeEnvIds batch method (uses tenantSafeBulkWrite) - Export checkIfActive from Code/process.js Extraction: - Add extractInvokedSkillsFromHistory() to run.ts — scans message history for AIMessage tool_calls where name === 'skill', extracts skillName args. Follows same pattern as extractDiscoveredToolsFromHistory. Smart re-priming in primeSkillFiles: - Before batch uploading, checks if existing codeEnvIdentifiers are still active via getSessionInfo + checkIfActive (23h threshold) - If session is still active, returns cached references (zero uploads) - If stale or missing, batch-uploads everything and persists new identifiers on SkillFile documents (fire-and-forget) - Single session check covers all files (batch shares one session_id) Wiring: - Pass getSessionInfo, checkIfActive, updateSkillFileCodeEnvIds through ToolExecuteOptions and all three controller entry points * feat: wire skill file re-priming at run start via initialSessions Flow: 1. initialize.js creates primeInvokedSkills callback with all deps 2. client.js calls it with message history before createRun 3. extractInvokedSkillsFromHistory scans for skill tool calls 4. For each invoked skill with files, primeSkillFiles uploads/checks 5. Returns initialSessions map passed to createRun 6. createRun passes initialSessions to Run.create (via RunConfig) 7. Run constructor seeds Graph.sessions, making skill files available to subsequent bash/code tool calls via ToolNode session injection Requires @librechat/agents with initialSessions on RunConfig (PR #94). * refactor: use CODE_EXECUTION_TOOLS set for code tool checks Import CODE_EXECUTION_TOOLS from @librechat/agents and replace inline constant checks in handlers.ts and callbacks.js. Fixes missing bash tool coverage in the session context injection (handlers.ts) and code output processing (callbacks.js). * refactor: move primeInvokedSkills to packages/api, add skill body re-injection Moves primeInvokedSkills from an inline closure in initialize.js (with dynamic requires) to a proper exported function in packages/api skillFiles.ts with explicit typed dependencies. Key changes: - primeInvokedSkills now returns both initialSessions (for file priming) AND injectedMessages (skill bodies for context continuity) - createRun accepts invokedSkillMessages and appends skill bodies to systemContent so the model retains skill instructions across runs - initialize.js calls the packaged function with all deps passed explicitly - client.js passes both initialSessions and injectedMessages to createRun * fix: move dynamic requires to top-level module imports Move primeInvokedSkills, getStrategyFunctions, batchUploadCodeEnvFiles, getSessionInfo, and checkIfActive from inline requires to top-level module requires where they belong. * refactor: skill body reconstruction via formatAgentMessages, not systemContent Replaces the lazy systemContent approach with proper message-level reconstruction: SDK (formatAgentMessages): - New invokedSkillBodies param (Map<string, string>) - Reconstructs HumanMessages after skill ToolMessages at the correct position in the message sequence, matching where ToolNode originally injected them LibreChat: - extractInvokedSkillsFromPayload replaces extractInvokedSkillsFromHistory (works with raw TPayload before formatAgentMessages, not BaseMessage[]) - primeInvokedSkills now takes payload instead of messages, returns skillBodies Map instead of injectedMessages - client.js calls primeInvokedSkills BEFORE formatAgentMessages, passes skillBodies through as the 4th param - Removed invokedSkillMessages from createRun (no more systemContent hack) - Single-pass: skill detection happens inside formatAgentMessages' existing tool_call processing loop, zero extra message iterations * refactor: rename skillBodies to skills for consistency with SDK param * refactor: move auth loading into primeInvokedSkills, pass loadAuthValues as dep The payload/accessibleSkillIds guard and CODE_API_KEY loading now live inside primeInvokedSkills (packages/api) rather than in the CJS caller. initialize.js passes loadAuthValues as a dependency and the callback is only created when skillsCapabilityEnabled. * feat: ReadFile tool + conditional bash registration + skill path namespacing ReadFile tool (read_file): - General-purpose file reader, event-driven (ON_TOOL_EXECUTE) - Schema: { file_path: string } — "{skillName}/{path}" convention - handleReadFileCall: resolves skill name from path, ACL check, reads from DB cache or storage, binary detection, size limits (256KB), lazy caching (512KB), line numbers in output - SKILL.md special case: reads skill.body directly - Dispatched alongside SKILL_TOOL in createToolExecuteHandler - Added to specialToolNames in ToolService Conditional tool registration: - ReadFile + SkillTool: always registered when skills enabled - BashTool: only registered when codeEnvAvailable === true - codeEnvAvailable passed through InitializeAgentParams from caller Skill file path namespacing: - primeSkillFiles now uploads as "{skillName}/SKILL.md" and "{skillName}/{relativePath}" instead of flat names - Prevents file collisions when multiple skills are invoked Wiring: - getSkillFileByPath + updateSkillFileContent passed through ToolExecuteOptions in all three callers * feat: return images/PDFs as artifacts from read_file, tighten caching Binary artifact support: - Images (png, jpeg, gif, webp) returned as base64 in artifact.content with type: 'image_url', processed by existing callback attachment flow - PDFs returned as base64 artifact similarly - Binary size limit: 10MB (MAX_BINARY_BYTES) - Other binary files still return metadata + bash fallback Caching: - Text cached only on first read (file.content == null check) - Binary flag cached only on first detection (file.isBinary == null) - Skill files are immutable; no redundant cache writes Registration: - ReadFileToolDefinition now includes responseFormat: 'content_and_artifact' * chore: update @librechat/agents to version 3.1.66-dev.0 and add peer dependencies in package-lock.json and package.json files * fix: resolve review findings #1,#2,#4,#5,#6,#10,#13 Critical: - #1: primeInvokedSkills now accumulates files across all skills into one session entry instead of overwriting. Parallel processing via Promise.allSettled. - #2: codeEnvAvailable now computed and passed in openai.js and responses.js (was missing, bash tool never registered in those flows) Major: - #4: relativePath in updateSkillFileCodeEnvIds now strips the {skillName}/ prefix to match SkillFile documents. SKILL.md filter uses endsWith instead of exact match. - #5: File priming guarded on apiKey being non-empty (skip when not configured instead of failing with auth error) - #6: Skills processed in parallel via Promise.allSettled instead of sequential for-of loop Minor: - #10: Use top-level imports in initialize.js instead of inline requires - #13: Log warning when skill catalog reaches the 100-skill limit * fix: resolve followup review findings N1,N2,N4 N1 (CRITICAL): Wire skill deps into responses.js non-streaming path. Was completely missing getSkillByName, file strategy functions, etc. N2 (MAJOR): Single batch upload for ALL skills' files. Resolves skills in parallel (Phase 1), then collects all file streams across skills and does ONE batchUploadCodeEnvFiles call (Phase 2). All files share one session_id, eliminating cross-session isolation issues. N4 (MINOR): Move inline require() to top-level in openai.js and responses.js, consistent with initialize.js. * fix: add mocks for new file strategy imports in controller tests * fix: restore session freshness check, parallelize file lookups, add warnings R1: Re-add session freshness check before batch upload. Checks any existing codeEnvIdentifier via getSessionInfo + checkIfActive. If the session is still active (23h window), returns cached file references with zero re-uploads. R2: listSkillFiles calls parallelized via Promise.all (were sequential in the for-of loop). R3: Log warning when skill record lookup fails during identifier persistence (was a silent empty-string fallback). * fix: guard freshness cache on single-session consistency * fix: multi-session freshness check (code env handles mixed sessions natively) The code execution environment fetches each file by its own {session_id, fileId} pair independently — no single-session requirement. Removed the sessionIds.size === 1 guard. Now checks ALL distinct sessions for freshness. If every session is still active (23h window), returns cached references with per-file session_ids preserved. If any session expired, falls through to re-upload everything in a single batch. * perf: parallelize session freshness checks via Promise.all * fix: add optional chaining for session info retrieval in primeInvokedSkills Updated the primeInvokedSkills function to use optional chaining for getSessionInfo and checkIfActive methods, ensuring safer access and preventing potential runtime errors when these methods are undefined. * fix: address review findings #1-#9 + Codex P1/P2 + session probe Critical: - #1/Codex P1: Add codeApiKey loading to openai.js and responses.js loadTools configurable (was missing, file priming broken in 2/3 paths) - Codex P1: Fix cached file name prefix in primeSkillFiles cache path (was sf.relativePath, now ${skill.name}/${sf.relativePath}) Major: - Codex P2: Honor ephemeral skills toggle in agents endpoint (check ephemeralAgent?.skills !== false alongside admin capability) - #4: Early size check using file.bytes from DB before streaming (prevents full-file buffer for oversized files) Minor: - #5: Replace Record<string, any> with Record<string, boolean | string> - #6: Localize Pin/Unpin aria-labels with com_ui_pin/com_ui_unpin - #8: Parallelize stream acquisition in primeSkillFiles via Promise.allSettled - #9: Log warning for partial batch upload failures with filenames Performance: - Session probe optimization: getSessionInfo now hits per-object endpoint (GET /sessions/{sid}/objects/{fid}) instead of listing entire session (GET /files/{sid}?detail=summary). O(1) stat vs O(N) list + linear scan. * refactor: extract shared skill wiring helper + add unit tests DRY (#3): - New skillDeps.js exports getSkillToolDeps() with all 9 skill-related deps (getSkillByName, listSkillFiles, getStrategyFunctions, etc.) - Replaces 5 identical copy-paste blocks across initialize.js, openai.js, responses.js (streaming + non-streaming paths) - One place to maintain when skill deps change Tests (#2): - 8 unit tests for extractInvokedSkillsFromPayload covering: string args, object args, missing skill tool_calls, non-assistant messages, malformed JSON, empty skillName, empty payload, dedup * fix: remove @jest/globals import, use global jest env * fix: resolve round 2 review findings R2-1 through R2-7 R2-1 (toggle semantics): openai.js + responses.js now check admin capability (AgentCapabilities.skills) alongside ephemeral toggle. Aligns with initialize.js. R2-2 (swallowed error): primeInvokedSkills now logs updateSkillFileCodeEnvIds failures (was .catch(() => {})) R2-4 (test cast): Record<string, string> → Record<string, unknown> R2-5 (DRY regression): Extract enrichWithSkillConfigurable() into skillDeps.js. Replaces 4 identical loadAuthValues blocks. Each loadTools callback is now a one-liner. JSDoc added (R2-6). R2-7 (sequential streams): primeInvokedSkills now uses Promise.allSettled for parallel stream acquisition. * fix: require explicit skills toggle + treat partial cache as miss - initialize.js: change ephemeralSkillsToggle !== false to === true (unset toggle no longer enables skills) - primeSkillFiles cache: require ALL files to have codeEnvIdentifier before using cache (partial persistence = cache miss = re-upload) - primeInvokedSkills cache: same check (allFilesWithIds.length must equal total file count) * fix: pass entity_id=skillId on batch upload, eliminates per-user cache thrashing primeSkillFiles now passes entity_id: skill._id.toString() to batchUploadCodeEnvFiles. This scopes the code env session to the skill, not the user. All users sharing a skill share the same uploaded files — no more cache thrashing from overwriting each other's codeEnvIdentifier. The stored codeEnvIdentifier now includes ?entity_id= suffix so freshness checks pass the entity_id through to the per-object stat endpoint. Both primeSkillFiles and primeInvokedSkills store consistent identifier formats. * fix: pass entity_id on multi-skill batch upload, consistent identifier format * Revert "fix: pass entity_id on multi-skill batch upload, consistent identifier format" This reverts commit c85ce21. * refactor: per-skill upload in primeInvokedSkills, eliminate multi-skill batch Replace the monolithic multi-skill batch upload with per-skill primeSkillFiles calls. Each skill gets its own session with entity_id=skillId, ensuring: - Correct session auth (entity_id matches on freshness checks) - Per-skill freshness caching (only expired skills re-upload) - Shared skill sessions work across users (same entity_id=skillId) - Code env handles mixed session_ids natively The big batch block (stream collection, single upload, identifier mapping) is replaced by a simple loop over primeSkillFiles, which already handles freshness caching, batch upload, and identifier persistence per-skill. * fix: resolve review findings #1,#3-5,#7,#9-11 Critical: - #1: Strip ?entity_id= query string before splitting codeEnvIdentifier into session_id/fileId (was corrupting cached file IDs in 4 locations) Major: - #4: Parallelize per-skill primeSkillFiles via Promise.allSettled - #5: Add logger.warn to all empty .catch(() => {}) on cache writes Minor: - #7: Add logger.debug to enrichWithSkillConfigurable catch block - #9: Use error instanceof Error guard in batchUploadCodeEnvFiles - #10: Move enrichWithSkillConfigurable to TypeScript in packages/api (skillConfigurable.ts), skillDeps.js wraps with loadAuthValues dep - #11: Reduce MAX_BINARY_BYTES from 10MB to 5MB (~11.5MB peak with b64) * fix: forward entity_id in session probe + always register bash tool Codex P2 (entity_id in probe): getSessionInfo now preserves and forwards query params (including entity_id) to the per-object stat endpoint. Without this, identifiers stored as ...?entity_id=... would fail auth checks because the entity_id scope was dropped. Codex P2 (bash tool availability): Remove codeEnvAvailable gate from injectSkillCatalog. Bash tool definition is now always registered when skills are enabled. Actual tool instance creation still happens at execution time in loadToolsForExecution (which loads per-user credentials). This ensures users with per-user CODE_API_KEY get bash without requiring a global env var at init time. Removes codeEnvAvailable from InjectSkillCatalogParams, InitializeAgentParams, and all three controller entry points. * fix: add debug logging to primeInvokedSkills catch, rename export alias * fix: stub bash tool when no key + remove PDF artifact path Codex P1 (bash tool): When CODE_API_KEY is unavailable, create a stub tool that returns "Code execution is not available. Use read_file instead." This prevents "tool not found" errors from the model repeatedly calling bash_tool in no-code-env deployments while still registering the definition for per-user credential users. Codex P2 (PDF artifacts): Remove PDF image_url artifact path. The host artifact pipeline processes image_url via saveBase64Image which fails for PDFs. PDFs now fall through to the generic binary handler ("Use bash to process"). TODO comment for future document artifact support. Also: isImageOrPdf → isImage in early size checks (PDFs are no longer treated as artifact candidates). * fix: remove dead PDF_MIME constant, hoist skillToolDeps, document session_id - #7: Remove unused PDF_MIME constant (dead code after PDF artifact removal) - #11: Hoist skillToolDeps to module-level constant (avoid per-call allocation) - #6: Document that CodeSessionContext.session_id is a representative value; ToolNode uses per-file session_id from the files array * fix: call toolEndCallback for skill/read_file artifacts + clear codeEnvIdentifier on re-upload Codex P1 (toolEndCallback bypass): skill and read_file handler branches returned early, bypassing the toolEndCallback that processes artifacts (image attachments). Now calls toolEndCallback when the result has an artifact, using the same metadata pattern as the normal tool.invoke path. Codex P1 (stale identifiers): upsertSkillFile now $unset's codeEnvIdentifier alongside content and isBinary when a file is re-uploaded. Prevents the freshness cache from returning references to old file content after a skill file is replaced. * fix: add session_id comment at cached path, rename skillResult to handlerResult * fix: return content_and_artifact from bash stub so result.content is populated * fix: deterministic skill lookup, dedup warning, and multi-session freshness check - getSkillByName: add sort({updatedAt:-1}) so name collisions resolve deterministically to the most recently updated skill - injectSkillCatalog: warn when multiple accessible skills share a name - primeSkillFiles: check ALL distinct sessions for freshness, not just the first file's session, preventing stale refs after partial bulkWrite * refactor: update icon import in Skills component - Replaced the Scroll icon with ScrollText in the Skills component for improved clarity and consistency in the UI. * fix: SKILL.md cache parity, gate bash_tool on code env, fix read_file too-large message - primeSkillFiles: filter SKILL.md from returned files array on fresh upload so cached and non-cached paths return identical file sets (SKILL.md is still on disk in the session for bash access) - injectSkillCatalog: only register bash_tool when codeEnvAvailable is true; thread the flag from all three CJS callers via execute_code capability check - handleReadFileCall: tell the model to invoke the skill first before suggesting /mnt/data paths for oversized files * fix: use EnvVar constant, deduplicate auth lookup, validate batch upload, stream byte limit - Replace hardcoded 'LIBRECHAT_CODE_API_KEY' with EnvVar.CODE_API_KEY in skillConfigurable.ts and skillFiles.ts - Resolve code API key once at run start in initialize.js and pass to both primeInvokedSkills and enrichWithSkillConfigurable via optional preResolvedCodeApiKey param, eliminating redundant loadAuthValues calls - Add response structure validation in batchUploadCodeEnvFiles before accessing session_id/files to surface unexpected responses early - Add streaming byte counter in handleReadFileCall that aborts and destroys the stream when accumulated bytes exceed MAX_BINARY_BYTES, preventing full file buffering when DB metadata is inaccurate * refactor: update icon import in ToolsDropdown component - Replaced the Scroll icon with ScrollText in the ToolsDropdown component for improved clarity and consistency in the UI. * fix: partial upload failure detection, EnvVar in initialize.js, declaration ordering - primeSkillFiles: return null (failure) when batch upload partially succeeds — missing bundled files would cause runtime bash/read failures with missing paths in code env - initialize.js: replace hardcoded 'LIBRECHAT_CODE_API_KEY' with EnvVar.CODE_API_KEY imported from @librechat/agents - initialize.js: move enabledCapabilities, accessibleSkillIds, and codeApiKey declarations before the toolExecuteOptions closure that references them (eliminates reliance on temporal dead zone hoisting)
danny-avila
added a commit
that referenced
this pull request
Apr 21, 2026
…efaults (#12692) * feat: per-user skill active/inactive toggle with ownership-aware defaults - Add `skillStates` map (Record<string, boolean>) to user schema for per-user active/inactive overrides on skills - Add `defaultActiveOnShare` to interface.skills config (default: false) so admins can control whether shared skills auto-activate - Add GET/POST /api/user/settings/skills/active endpoints with validation - Add React Query hooks with optimistic mutations for skill states - Add useSkillActiveState hook with ownership-aware resolution: owned skills default active, shared skills default inactive - Add toggle switch UI to SkillListItem and SkillDetail components - Filter inactive skills in injectSkillCatalog before agent injection - Add localization keys for active/inactive labels * fix: use Record instead of Map for IUser.skillStates Mongoose .lean() flattens Map to a plain object, causing type incompatibility with IUser in methods that return lean documents. * fix: address review findings for skill active states - Fail-closed when userId is absent: filter rejects all shared skills instead of passing them through unfiltered (Codex P1) - Validate Mongoose Map key characters (reject . and $) in controller to return 400 instead of a 500 from schema validation (Codex P2) - Block toggle while initial skill states query is loading to prevent overwriting server-side overrides with an empty snapshot (Codex P2) - Extract shared SkillToggle component, eliminating duplicate toggle markup in SkillListItem and SkillDetail (Finding #3) - Move skill state query/mutation hooks from Favorites.ts to Skills/queries.ts per feature-directory convention (Finding #4) - Fix hardcoded English aria-label in SkillListItem by passing the localized string from the parent SkillList (Finding #5) - Fix inline arrow in SkillList render loop: pass stable callback reference so SkillListItem memo() is not invalidated (Finding #1) - Extract toRecord() helper in controller to DRY the Map-to-Object conversion (Finding #6) - Remove Promise.resolve wrapping synchronous config read (Finding #8) - Remove unused TUpdateSkillStatesRequest type (Finding #12) * fix: forward tabIndex on SkillToggle to preserve list keyboard nav The original inline toggle had tabIndex={-1} so the row itself remained the sole tab target. The extraction into SkillToggle dropped this prop, making every list toggle a tab stop. Add an optional tabIndex prop and pass -1 from SkillListItem. * fix: plumb skillStates to all agent entry points, isolate toggle keydown - Add skillStates/defaultActiveOnShare loading to openai.js and responses.js controllers so shared-skill activation is respected across all agent entry points, not just initialize.js (Codex P1) - Stop keydown propagation on SkillToggle so Enter/Space does not bubble to the parent row's navigation handler (Codex P2) * fix: paginate catalog fetch and serialize toggle writes - Paginate listSkillsByAccess (up to 10 pages of 100) until the active catalog quota is filled, so inactive shared skills in recent positions do not starve active owned skills past the first page (Codex P1) - Extend listSkillsByAccess interface with cursor/has_more/after for catalog pagination - Serialize skill-state writes via a ref queue: one in-flight request at a time, with the latest desired state sent when the previous one settles. Prevents last-response-wins races where an older request overwrites newer toggles (Codex P2) * fix: share write queue across hook instances, block toggle on fetch error - Move the write queue from a per-instance useRef to a module-scoped object so every mount of useSkillActiveState (SkillList, SkillDetail, etc.) serializes against the same in-flight slot. Prior per-instance queues allowed two components to race full-map POSTs (Codex P1) - Extend the toggle guard beyond isLoading: also block when isError is true or data is undefined. Prevents a failed GET from seeding a toggle with an empty baseline that would wipe server-side overrides on the next successful POST (Codex P1) * fix: stale closure, orphan cleanup, and cap-error UX - Read toggle baseline from React Query cache via queryClient.getQueryData instead of the captured skillStates closure. The closure can be stale between onMutate's setQueryData and the next render, so rapid successive toggles would build on old state and drop earlier changes (Codex P1) - Surface the MAX_SKILL_STATES_EXCEEDED error code with a specific toast key (com_ui_skill_states_limit) so users understand the 200-cap rather than seeing a generic error - Prune orphaned entries (skillIds whose Skill doc no longer exists) on both GET and POST in SkillStatesController. Self-heals over time without needing cascade-delete hooks or a migration job. Uses one indexed Skill._id query per request * test: pin skill active-state precedence with unit tests Extract the active-state resolution logic from a closure inside injectSkillCatalog into an exported resolveSkillActive helper, then cover every branch of the precedence matrix: - Fails closed when userId is absent (even with defaultActiveOnShare=true) - Explicit override wins over ownership and config (both true and false) - Owned skills default to active when no override is set - Shared skills default to defaultActiveOnShare value - Undefined skillStates behaves identically to an empty object - defaultActiveOnShare defaults to false when omitted - Owned skills ignore defaultActiveOnShare entirely Closes Finding #2 from the pre-rebase comprehensive review. Mirrors the existing scopeSkillIds test style; injectSkillCatalog now calls resolveSkillActive instead of inlining the closure. * refactor: limit skill active toggle to detail header, drop label - Remove the per-row toggle from SkillListItem and the active-state plumbing (hook call, isSkillEnabled/onToggleEnabled/toggleAriaLabel props) from SkillList. The detail view is now the single place to change a skill's active state - Drop dim/muted styling for inactive skills in the sidebar: without a control there, the visual indication has nowhere to land - Resize SkillToggle to match neighbor buttons: outer h-9 container, h-6 w-11 track with size-5 knob, no label span. The 'Active' / 'Inactive' text that accompanied the detail-view toggle is removed - Remove the now-unused label prop and tabIndex prop (the tabIndex existed only for the list-row context) from SkillToggle. Drop the onKeyDown stopPropagation for the same reason - Remove now-orphaned com_ui_skill_active / com_ui_skill_inactive translation keys * style: shrink SkillToggle track to h-5 w-9 with size-4 knob Container stays at h-9 to match neighbor button heights. The toggle track itself drops from h-6 w-11 to h-5 w-9, with a size-4 knob travelling 1.125rem on activation. Visually lighter inside the row. * fix: remove redundant skillStates entries that match the resolved default When a toggle lands on the ownership/config default, delete the key from the map instead of persisting `{id: defaultValue}`. Without this, a user toggling a skill off and back on would leave `{id: true}` for an owned skill (whose default is already true), silently consuming a slot against the 200-entry cap. Repeated round-trip toggles could exhaust the quota with zero meaningful overrides (Codex P2). Preserves the exceptions-list invariant that the runtime-resolution design depends on. * fix: prune before enforcing skill-state cap; reject non-ObjectId keys Reorder the update controller so pruneOrphans runs before the 200-cap check. Without this, a user near the cap with some orphaned entries (skills deleted since their last GET) could send a payload that would pass after pruning but gets rejected by the raw-size check first. Add a sanity cap on raw payload size (2 * MAX_SKILL_STATES) so abusive inputs do not reach the DB query, and enforce the real cap on the pruned result instead. Harden pruneOrphans: the earlier early-return path could pass non-ObjectId keys through unchanged. Now only valid ObjectIds are returned, and the Skill-model-unavailable fallback filters by format. Also add isValidObjectIdString validation at the input boundary so malformed (but otherwise non-Mongo-unsafe) keys never reach persistence (Codex P2 x2). * fix: enforce active filter at execute time, prune revoked shares, scope queue per user P1: injectSkillCatalog now returns activeSkillIds (the filtered set that appears in the catalog). initializeAgent uses that set as the stored accessibleSkillIds on the initialized agent, so getSkillByName at runtime cannot resolve a deactivated skill — even if the LLM hallucinates a name or the user invokes by direct-invocation shorthand. Previously the executor authorized against the full ACL set, bypassing the active-state guarantee (Codex P1). P2: pruneOrphans now checks user access via findAccessibleResources in addition to skill existence. When a share is revoked, the user's skillStates entry for that skill had no cleanup path and silently consumed the 200-cap. Self-heals on both GET and POST. One extra ACL query per settings read/write; scoped to a single user so no N-user amplification (Codex P2). P2: the write queue moves from a single module-scoped object to a Map keyed by userId. Logout/login in the same tab can no longer flush the previous user's pending snapshot under the new session's auth. Each userId gets its own pending/inFlight slot; the in-flight request retains its original auth via the cookie already attached when sent, so the race window closes (Codex P2). * refactor: extract skillStates helpers to packages/api; add tests; polish Address the remaining valid findings from the comprehensive review: - Extract toRecord, loadSkillStates, validateSkillStatesPayload, and pruneOrphanSkillStates into packages/api/src/skills/skillStates.ts as TypeScript. The controller in /api shrinks to a ~90-line thin wrapper that builds live dependency adapters for Mongoose + the permission service (Review #2 DRY, #3 workspace boundary) - Replace the triplicated 12-line skillStates loading block in initialize.js, openai.js, and responses.js with a single call to loadSkillStates from @librechat/api. One helper, three sites - Swap console.error for the project logger in the controller (Review #7) - Remove the redundant INVALID_KEY_PATTERN regex: a valid ObjectId cannot contain . or $, so isValidObjectIdString already covers it (Review #11) - Parameterize the 200-cap error toast with {{0}} interpolation driven by the error response's `limit` field, so future changes to MAX_SKILL_STATES update the UI message automatically (Review #12) - Add 24 unit tests for the new skillStates helpers (toRecord, resolveDefaultActiveOnShare, loadSkillStates, validateSkillStates- Payload, pruneOrphanSkillStates) covering success paths, malformed input, cap boundaries, and parallel-query behavior (Review #4) - Add 10 tests for injectSkillCatalog pagination covering empty accessible set, missing listSkillsByAccess, single-page filter, owned-vs-shared defaults, explicit-override precedence, multi-page collection, MAX_CATALOG_PAGES safety cap, early termination on has_more=false, additional_instructions injection, and fail-closed without userId (Review #5) Total test count: 60 (was 26 on this surface). * fix: rename skillStates ValidationError to avoid barrel-export collision packages/api/src/types/error.ts already exports a ValidationError (MongooseError extension). Re-exporting a different shape from skills/skillStates.ts through the skills barrel caused TS2308 in CI because the root index re-exports both. Rename to SkillStatesValidationError to keep the exports disjoint. * refactor: tighten tests and absorb caller guard into loadSkillStates Address the followup review findings: - Add optional `accessibleSkillIds` param to loadSkillStates so the helper short-circuits to defaults when no skills are accessible. All three controllers drop the residual 7-line conditional wrapper in favor of a single destructured call (Review #2) - Remove the unreachable `typeof key !== 'string'` check from validateSkillStatesPayload: Object.entries always yields string keys per the JS spec (Review #3) - Replace the two `as unknown as` agent casts in the injectSkillCatalog tests with a `makeAgent()` factory typed directly as the function's parameter shape (Review #4) - Tighten the MAX_CATALOG_PAGES assertion from `toBeLessThanOrEqual(11)` to `toHaveBeenCalledTimes(10)` — the loop deterministically makes exactly 10 page fetches before hitting the cap (Review #1) - Rewrite the parallel-execution test for pruneOrphanSkillStates using deferred promises instead of microtask-order assertions. The test now inspects `toHaveBeenCalledTimes(1)` on both mocks after a single Promise.resolve() yield, pinning Promise.all usage without relying on push-order into a shared array (Review #5) - Evict stale writeQueue entries on user change via a module-scoped `lastSeenUserId` sentinel. When a different user's toggle is the first one after a logout/login, the previous user's queue entry is deleted. Keeps the Map bounded without adding hook-instance effect cleanup (Review #6) * fix(test): mock loadSkillStates in openai and responses controller specs The prior refactor replaced the inline 12-line skillStates loading block with a call to loadSkillStates from @librechat/api. Both controller spec files mock @librechat/api as a flat object, so any new named import from that package is undefined in the test env. Calling `await loadSkillStates(...)` threw before recordCollectedUsage ran, surfacing as "undefined is not iterable" on the test's array destructure of `mockRecordCollectedUsage.mock.calls[0]`. Add the missing mock to both spec files alongside the existing scopeSkillIds stub. * fix: abandon stale skillStates write queues on user switch Close the cross-session leak window where an in-flight flush loop still holds a reference to a previous user's queue: it could fire its next mutateAsync under the new session's auth cookies and persist the stale snapshot to the new user's document (Codex P1). Add an `abandoned` flag on `WriteQueue`. Three mechanisms cooperate: - `getWriteQueue` marks every non-active queue abandoned when the user differs from the last-seen identity (pre-existing eviction site, now more aggressive). - A `useEffect` on `userId` calls the same abandonment pass on every render with a new active identity, covering the window between logout/login and the new user's first toggle (when `getWriteQueue` would otherwise not fire). - The flush loop checks `!queue.abandoned` in its while condition so the second and later iterations exit without firing another `mutateAsync` after the session changes. The first iteration's in-flight request (already dispatched under the original user's cookies) still runs to completion or failure on its own — only the subsequent iterations, which are the dangerous ones, are blocked.
danny-avila
added a commit
that referenced
this pull request
Apr 21, 2026
…le` / `allowed-tools` (#12745) * 🧬 feat: Persist `disable-model-invocation` / `user-invocable` / `allowed-tools` Adds first-class columns mirroring the three runtime-enforced frontmatter fields, with a `deriveStructuredFrontmatterFields` helper that maps from frontmatter at create/update time and re-syncs (via `$unset`) when fields are removed. `listSkillsByAccess` projection includes them so the Phase 6 catalog filter and popover filter can both read off the summary row. Marks `invocationMode` as @deprecated on `TSkill` and the `InvocationMode` enum — the runtime now reads the persisted pair instead. * 🛡️ feat: Enforce frontmatter at runtime (catalog, skill tool, manual resolver, tool union) Wires the persisted columns into actual runtime behavior across all four invocation paths: - `injectSkillCatalog` excludes `disableModelInvocation: true` skills before catalog formatting — they cost zero context tokens and stay invisible to the model. - `handleSkillToolCall` rejects with a clear error when the model names a skill marked `disable-model-invocation: true` (defends against a stale-cache or hallucinated invocation getting past the catalog filter). - `resolveManualSkills` skips `userInvocable: false` skills with a warn log so an API-direct caller can't bypass the popover-side filter. - `unionPrimeAllowedTools` collects skill-declared `allowed-tools` minus what's already on the agent; `initialize.ts` re-runs `loadTools` for the extras and merges resulting `toolDefinitions` into the agent's effective set for the turn. Tool-name resolution is tolerant — unknown names silently drop with a debug log so cross-ecosystem skills referencing yet-to-be-implemented tools (Claude Code's `edit_file`, etc.) import without breaking. The agent document is never modified; the union is turn-scoped. Helper exports (`unionPrimeAllowedTools`) are structured so Phase 5's always-apply primes flow through the same union (combined `[...manualPrimes, ...alwaysApplyPrimes]`) once the resolver lands. Skill handler wire format gains the three fields so clients can render them on detail / list views. * 🎛️ feat: `$` popover reads `userInvocable` instead of UI-only `invocationMode` Replaces the phase-1 UI-only `invocationMode` check with the persisted `userInvocable` field (mirrors the `user-invocable` frontmatter). Skills authored with `user-invocable: false` no longer surface in the popover; the backend resolver enforces the same rule for defense-in-depth. Default-visible behavior is preserved: skills without an explicit `userInvocable` value (older rows, freshly imported skills that don't declare the field) stay visible — only an explicit `false` hides them. Test fixture updated to reflect the new field. * 🔧 fix: Address Phase 6 review findings Codex P2 + reviewer #1: Single `loadTools` call with the union of `agent.tools + allowed-tools`. The earlier two-call approach dropped `userMCPAuthMap` / `toolContextMap` / `actionsEnabled` from the skill-added pass — an MCP tool gained via `allowed-tools` would be visible to the model but fail at execution without per-user auth context. Resolution of `manualSkillPrimes` is hoisted before `loadTools` so the union can be computed up-front; the dropped-tools debug log now compares loaded vs. requested across the single call. Codex P3 + reviewer #2: `injectSkillCatalog.activeSkillIds` now includes `disable-model-invocation: true` skills. The runtime ACL check in `handleSkillToolCall` previously couldn't reach the explicit "cannot be invoked by the model" rejection because the broader access set excluded those skills. Catalog text and tool registration still gate on the visible subset (zero-context-token guarantee preserved); only the per-user `isActive` filter is a hard exclusion now. Reviewer #1 (try/catch around loadTools, MAJOR): A single bad `allowed-tools` entry from a shared skill could crash the entire turn. Now wrapped — on failure with extras, retry with just `agent.tools` and continue (the dropped-tools debug log surfaces what vanished). If the retry-without-extras still throws, propagate; the agent's own tools are the load-bearing surface. Reviewer #3 (integration tests, MAJOR): Added six tests in `initialize.test.ts` covering the full `allowed-tools` loading path: union pass-through, no-extras short-circuit, agent-baseline dedup, loadTools throw + retry, propagated throw without extras, and the empty-tools edge case. Smaller cleanups bundled in: - Reviewer #4: Moved `logger` import to the package-imports section (was wedged among local imports). - Reviewer #5: Removed unused index on `disableModelInvocation` (filtering happens application-side in `injectSkillCatalog`; index cost write overhead for zero query benefit). - Reviewer #6: Swapped order of `userInvocable` and body checks in `resolveManualSkills` so the more authoritative author-decision reason surfaces first when both apply. - Reviewer #8: Documented the `allowedTools` enforcement gap on the schema + type — model-invoked skills (mid-turn `skill` tool calls) do NOT trigger tool union, since adding tools after the graph starts would require a rebuild. Manual / always-apply (Phase 5) primes are the supported paths. - Reviewer #9: Renamed `dmi` / `ui` / `at` locals to `disableModelInvocationRaw` / `userInvocableRaw` / `allowedToolsRaw` in `deriveStructuredFrontmatterFields`. Reviewer #7 (DRY shared `getSkillByName` return type) deferred — field sets diverge meaningfully across the three call sites (handler needs `body + fileCount`; resolver needs `author + allowedTools + userInvocable`; the InitializeAgentDbMethods contract needs the superset). A `Pick<>`-based consolidation is a follow-up cleanup. * 🔧 fix: Address codex iter 2 — catalog quota + duplicate-name dedup P1: `injectSkillCatalog` cap now counts only model-visible skills, not the merged active set. The previous behavior let a tenant with many `disable-model-invocation: true` rows near the top of the cursor exhaust the 100-slot quota before any invocable skill got scanned — the catalog could end up empty even though invocable skills existed further down the paginated results. `MAX_CATALOG_PAGES` stays the ceiling on scan budget; only `visibleCount` drives the early-exit on quota fill. P2: When an invocable and a `disable-model-invocation: true` skill share a name, drop the disabled doc(s) from `activeSkillIds`. Without this dedup, `getSkillByName` (which sorts by `updatedAt` desc) could pick the disabled doc and every model call to the cataloged name would fail with "cannot be invoked by the model" instead of executing the visible skill. When ONLY a disabled doc exists for a name, it stays in `activeSkillIds` so the explicit-rejection error path still fires for hallucinated invocations. Tests: 3 new cases in `injectSkillCatalog` covering (a) cap counted on visible skills only, (b) same-name collision drops disabled doc, (c) sole-disabled-name case keeps the disabled doc. * 🔒 fix: Apply `disable-model-invocation` gate to read_file too (codex iter 3 P1) `activeSkillIds` is shared between the `skill` and `read_file` handlers. The skill-tool gate was applied last iteration, but `handleReadFileCall` authorized purely on `getSkillByName(..., accessibleIds)` — so a model that learned a hidden skill's name (stale catalog or hallucination) could still read its `SKILL.md` body or bundled files via `read_file`, defeating the contract. Same explicit rejection now fires from both handlers; no change needed to the ACL set itself (disabled docs stay in `activeSkillIds` so the explicit error path keeps firing). Two new tests in `handlers.spec.ts` cover the read_file gate and regression-protect the happy path. * 🔧 fix: Address codex iter 4 — manual-prime exception + legacy frontmatter backfill P1: Scope the `read_file` `disableModelInvocation` gate to AUTONOMOUS model probes only. A user-invoked `$` skill that is also marked `disable-model-invocation: true` had its bundled `references/*` / `scripts/*` files unreadable, leaving the manually-primed skill body referencing files the model couldn't load. Now the handler bypasses the gate when the skill name appears in `manualSkillNames` (the per-turn allowlist threaded from `manualSkillPrimes` → `agentToolContexts` → `enrichWithSkillConfigurable` → `mergedConfigurable`). Defense-in-depth: the bypass is scoped to the specific names in the allowlist; a different disabled skill name is still rejected. P2: Read-time fallback for legacy skills authored before Phase 6 landed the structured columns. `user-invocable: false` / `disable-model-invocation: true` set in `frontmatter` (the validator already accepted those keys) but with no derived column would incorrectly evaluate as "user-invocable / model-allowed" until a save backfilled the columns. New `backfillDerivedFromFrontmatter` helper fills undefined columns from frontmatter at read time in both `getSkillByName` and `listSkillsByAccess` — column wins when both are set, frontmatter fills the gap when only it's set. No DB writes; the next `updateSkill` naturally persists. `listSkillsByAccess` projection expanded to include `frontmatter` (bounded by validator, payload impact small) so summaries can also be backfilled. Sticky-primed disabled skills (ones invoked in prior turns of the same conversation) are not yet in the manual-prime allowlist — same- turn manual invocation is the load-bearing path codex flagged; the sticky-turn case is a known limitation tracked for a follow-up. Tests: 2 new in handlers.spec.ts (manual-prime allows + name-scoped block holds), 3 new in skill.spec.ts (legacy backfill via getSkillByName + listSkillsByAccess + column-wins precedence). * 🔧 fix: Address codex iter 5 — propagate manualSkillNames + keep read_file P1: `enrichWithSkillConfigurable` is also called from `openai.js` and `responses.js` (the OpenAI Responses + completions endpoints). Both were ignoring the new `manualSkillNames` parameter, which meant the manual-prime exception in the `read_file` gate (iter 4) only worked on the agents endpoint. Now all three call sites pass `primaryConfig.manualSkillPrimes?.map(p => p.name)` so manual `$` invocations of disabled skills work consistently across endpoints. P2: When every accessible skill is `disable-model-invocation: true`, the catalog text and `skill` tool are correctly omitted (no model- reachable targets) — but `read_file` and `bash_tool` MUST still be registered. A user manually invoking such a skill gets its SKILL.md body primed into context; if the body references `references/foo.md` or `scripts/run.sh`, those reads need a registered tool. Restructured `injectSkillCatalog` so `skill` registration is gated on `catalogVisibleSkills.length > 0` while `read_file` (always) and `bash_tool` (when codeEnvAvailable) register whenever any active skill is in scope. Tests: existing all-disabled test rewritten to assert read_file IS registered + skill is NOT; new test confirms bash_tool joins it when codeEnvAvailable. * 🔧 fix: Address codex iter 6 — name-collision consistency via preferInvocable P2a (resolveManualSkills): a name collision between an older user-invocable doc and a newer non-user-invocable doc made manual `$` invocation silently no-op. The popover surfaced the older invocable doc; resolver looked it up by name; `getSkillByName` returned the newer non-invocable doc; resolver skipped on `userInvocable: false`. P2b (handler / runtime ACL): with same-name duplicates (e.g. older invocable + newer disabled), the manual prime resolved to one doc while later `read_file` / `skill` execution resolved a different doc through `activeSkillIds`. Model could follow one SKILL.md body while reading files from a different skill. Both root-cause: `getSkillByName` always returned the newest match and let the caller filter, but with collisions the newest can be something the caller didn't want. Fix: extend `getSkillByName` with `options.preferInvocable`. When true, prefer the newest doc satisfying BOTH `userInvocable !== false` AND `disableModelInvocation !== true` (with frontmatter backfill); fall back to the newest match otherwise. Fast path preserved when caller doesn't opt in. Callers passing `preferInvocable: true`: - `resolveManualSkills` — picks the popover-visible invocable doc even when a newer disabled / non-user-invocable duplicate exists. - `handleSkillToolCall` — keeps execution aligned with the catalog; falls back to the disabled doc only when no invocable variant exists (so the explicit "cannot be invoked by the model" gate still fires for the hallucinated-disabled-name case). - `handleReadFileCall` — same alignment, plus the manual-prime exception added in iter 4 still applies. Tests: - 2 new in skill.spec.ts (preferInvocable picks invocable when collision exists; falls back to newest when no clean-invocable exists). - 1 new in skills.test.ts (resolver passes preferInvocable through). - 2 new in handlers.spec.ts (skill tool + read_file pass it). - Existing initialize.test.ts assertion updated for the new option. * 🔧 fix: Address codex iter 7 — split preferInvocable into per-axis flags The previous unified `preferInvocable` filter required BOTH `userInvocable !== false` AND `disableModelInvocation !== true`. That was wrong for the model paths: `userInvocable: false` skills are model-only and remain valid `skill` / `read_file` invocation targets. A duplicate-name scenario where the newer cataloged doc was model- only would let the older user-invocable variant shadow it on every model call. Split the option into two independent axes: - `preferUserInvocable` — for manual paths (`$` popover). Skips docs with `userInvocable: false`. Disable-model-invocation status is irrelevant; iter 4 explicitly supports manual prime of disabled skills. - `preferModelInvocable` — for model paths (`skill` / `read_file` handlers). Skips docs with `disableModelInvocation: true`. User- invocable status is irrelevant; model-only skills are valid here. Both flags fall back to the newest match when no preferred doc exists, so the explicit-rejection error paths still fire correctly in the sole-disabled-name case. Callers updated: - `resolveManualSkills` → `preferUserInvocable: true` - `handleSkillToolCall` / `handleReadFileCall` → `preferModelInvocable: true` Tests: - New spec test for preferModelInvocable not filtering on userInvocable. - Existing preferInvocable test renamed/split to cover the new axes. - New test asserts preferUserInvocable still returns disabled docs (preserves iter 4 manual-disabled support). - Caller tests assert each path passes the right single flag and does NOT pass the wrong one. * 🔧 fix: TypeScript type-check failure in handlers.spec.ts (CI green) `jest.fn(async () => ...)` without explicit args infers an empty tuple for the call signature, so `mock.calls[0][2]` flagged as "Tuple type '[]' has no element at index '2'." Cast to `unknown[]` then narrow to the expected option shape. Behavior unchanged. Caught by the `Type check @librechat/api` CI step (.github/workflows/backend-review.yml). * 🔧 fix: Address codex iter 8 — undefined-result fallback + read_file alignment P1 (loadTools returning undefined): Production loaders (`createToolLoader` in `initialize.js` / `openai.js` / `responses.js`) wrap `loadAgentTools` in try/catch and return `undefined` on failure rather than throwing. Without explicit handling, my iter-1 try/catch only fired for thrown errors — a silent-failure on a skill-added tool would fall through to the empty fallback and silently DROP the agent's baseline tools for the turn (much worse than just losing the extras). Added an `undefined`-result branch that retries with just `agent.tools`, mirroring the throw branch. Test pins both behaviors. P2 (read_file alignment with manual prime): When a skill is in this turn's `manualSkillNames`, the `read_file` handler now uses `preferUserInvocable` instead of `preferModelInvocable`. Same name-collision rule as `resolveManualSkills`, so the doc whose files get read is the same doc whose body got primed. For autonomous probes (skill not in `manualSkillNames`), the handler keeps `preferModelInvocable` to align with the catalog the model saw. Two new tests cover both branches and regression-protect that the wrong flag isn't passed. * 🔧 fix: Address codex iter 9 — pin read_file lookup to primed skill _id P1 (manually-primed disabled IDs were dropped from activeSkillIds): The `executableSkills` dedup in `injectSkillCatalog` correctly drops `disable-model-invocation: true` duplicates when an invocable doc shares the name — but `resolveManualSkills` legitimately primes disabled docs (iter 4 supports manual `$` invocation of disabled skills). When the resolver primed a disabled doc, the read_file handler couldn't find it in the (deduped) `activeSkillIds` and either resolved a different same-name skill or returned not-found. Fix: `ResolvedManualSkill` now carries `_id`; the legacy `initialize.js` / `openai.js` / `responses.js` controllers build a `manualSkillPrimedIdsByName` map and `enrichWithSkillConfigurable` passes it into `mergedConfigurable`. `handleReadFileCall` now pins its lookup's `accessibleIds` to `[primedId]` whenever the requested skill is in the map. The constrained set guarantees the lookup returns the EXACT doc the resolver primed — body/files come from the same source even when same-name duplicates exist or the dedup removed the prime's id from `activeSkillIds`. Autonomous read_file probes (skill not in the manual-primed map) keep the full ACL set + `preferModelInvocable` so they align with the catalog the model saw and the disabled-only case still fires the explicit-rejection gate. Test fixture changes flow from `_id` becoming required on `ResolvedManualSkill`. `buildSkillPrimeContentParts` / `injectManualSkillPrimes` widen their param types to `Pick<...>` because they only read `name` / `body` and shouldn't force test literals to invent placeholder ids. * 🧹 fix: Address independent reviewer findings (DRY + types + tests + docs) Sanity-pass review surfaced 7 findings; addressed 6 (the 7th — DRY on inline `getSkillByName` return types — is acknowledged tech debt deferred to a follow-up). #1 [MAJOR, DRY]: The 4-line `manualSkillPrimedIdsByName` map construction was duplicated across 4 CJS call sites (openai.js, responses.js x2, initialize.js). Extracted `buildManualSkillPrimedIdsByName` helper in `skillDeps.js`; all four sites now call the helper. If `ResolvedManualSkill` ever renames `_id` or gains identifying fields, only the helper changes. #2 [MINOR, type safety]: `handleReadFileCall` was casting a hex string to `Types.ObjectId[]` via `as unknown as`, relying on mongoose's auto-cast in `$in` queries. Replaced with `new Types.ObjectId(...)` so any future consumer comparing with `.equals()` / `===` gets the correct value type. Imported `Types` as a value (was type-only). #5 [MINOR, test gap]: Added a test for the worst-case silent-failure path — both the union and base-only `loadTools` calls return undefined. The agent gets no tools but the turn doesn't crash hard; pinning that contract. #4 [MINOR, performance]: Added a TODO on the `listSkillsByAccess` projection noting the `frontmatter` field can be dropped once a write migration backfills all pre-Phase-6 skills' columns. ~2KB/skill × 100/page is wasted bandwidth post-backfill. #6 [NIT, docs]: `backfillDerivedFromFrontmatter` JSDoc said "Pure" right before "mutates its undefined fields in place". Replaced with "Side-effect-free w.r.t. the DB (no writes), but mutates its argument in place" which describes both halves accurately. #7 [NIT, test determinism]: Replaced `await new Promise(r => setTimeout(r, 5))` in two same-name collision tests with explicit `updateOne` setting `updatedAt: new Date(Date.now() - 1000)` on the older doc. Removes the wall-clock race on fast CI runners. The pagination test (line 480) still uses setTimeout — that test is pre-existing and order is incidental, not load-bearing. Existing test fixtures updated to use valid 24-char hex ObjectIds (required by the iter-9 test that constructs a real `ObjectId`). #3 [MINOR, deferred]: Inline `getSkillByName` return type duplicated across `handlers.ts`, `initialize.ts`, `skills.ts`. Reviewer acknowledged this as deferred; field sets diverge across call sites (handler needs `fileCount`, resolver needs `author`/`allowedTools`). A `Pick<>`-based consolidation is a clean follow-up.
danny-avila
added a commit
that referenced
this pull request
Apr 21, 2026
…ps, no-op allocation Addresses the comprehensive review of PR #12767: - **Finding #1** (MINOR, observability): `initializeAgent` now emits a debug log when an agent lists `execute_code` in its tools but the runtime gate is off (`params.codeEnvAvailable` !== true). The event-driven `loadToolDefinitionsWrapper` path doesn't log capability-disabled warnings, so without this the tool silently vanishes from the LLM's definitions with zero trace. Operators debugging "why isn't code interpreter working?" now get a signal at the initialize layer. - **Finding #5** (NIT, allocation): `registerCodeExecutionTools` now returns the input `toolDefinitions` array by reference on the no-op path (both tools already registered by a prior caller in the same run) instead of allocating a fresh spread array every time. The common dual-call scenario — `initializeAgent` then `injectSkillCatalog` — saves one O(n) copy per request. - **Finding #4** (NIT, DRY): Collapsed the duplicated 6-line JSDoc comment in `openai.js`, `responses.js`, and `addedConvo.js` into either a one-line `@see DiscoverConnectedAgentsParams.codeEnvAvailable` pointer (the two JS call sites) or a compact 3-line block referring back to the canonical source (addedConvo's @PARAM). - **Finding #2** (MINOR, test gap): Added `api/server/services/Endpoints/agents/addedConvo.spec.js` with three cases covering `codeEnvAvailable=true`, `codeEnvAvailable=false`, and omitted (undefined) passthrough. A future refactor that drops the param from destructuring now surfaces here instead of silently regressing multi-convo parallel agents with `execute_code`. - **Finding #3** (MINOR, test gap): Added `api/server/controllers/agents/__tests__/client.memory.spec.js` pinning the capability-flag derivation that `AgentClient::useMemory` uses — six cases covering present/absent/null/undefined config shapes plus an enum-literal pin (`'execute_code'` / `'agents'`). Catches enum renames or config-path shifts that would otherwise silently strip `bash_tool` + `read_file` from memory agents. Finding #7 (jest.mock scoping, confidence 40) left as-is: the reviewer's own risk assessment noted `buildToolSet` doesn't touch the mocked exports, and restructuring a file-level `jest.mock` to `jest.doMock` + dynamic `import()` introduces more complexity than the speculative risk justifies. The existing mock is scoped to the test file and contains the same stubs the adjacent `skills.test.ts` already uses. Finding #6 (PR description commit count) addressed out-of-band via PR description update. All existing tests pass, typecheck clean, lint clean across touched files. New tests: 9 cases across 2 new spec files.
12 tasks
OnyxBishop
pushed a commit
to OnyxBishop/LibreChat
that referenced
this pull request
Apr 22, 2026
…ain Collision (LibreChat-AI#12594) * 🐛 fix: resolve Action tools by exact tool name to prevent multi-action collision When two OpenAPI Actions on the same Agent share a hostname, the second action's entry overwrote the first in the encoded-domain Map and one action's tools silently disappeared from the LLM payload. The buggy resolution loop also used substring matching, which caused similar shadowing for any encoded-domain prefix overlap. This change builds a Map keyed on the full tool name (`<operationId>_action_<encoded-domain>`) directly, mirroring the exact lookup pattern that getActionToolDefinitions already uses. Each function in an action's spec gets its own slot, so two actions sharing a hostname no longer collide. Both the new and the legacy domain encodings are registered for each function so agents whose stored tool names predate the current encoding still resolve. Applied at all three call sites that had the buggy pattern: - processRequiredActions (assistants/threads path) - loadAgentTools (agent build path) - loadActionToolsForExecution (agent execution path) Adds three regression tests covering both ordering directions and the execution path. Tests fail without the fix and pass with it. * 🐛 fix: Normalize action tool name at lookup + cover assistants path Follow-up to the multi-action domain collision fix. Addresses PR LibreChat-AI#12594 review feedback: **Must-fix LibreChat-AI#1 — short-hostname lookup mismatch.** The toolToAction map is keyed on the `_`-collapsed domain, but `agent.tools` and `currentAction.tool` persist the raw `domainParser(..., true)` output, which for hostnames ≤ ENCODED_DOMAIN_LENGTH is a `---`-separated string (e.g. `medium---com`). Exact-match `Map.get()` missed those keys and silently dropped the tool. Fix: normalize every incoming tool name through a new `normalizeActionToolName` helper before the lookup in `loadAgentTools`, `processRequiredActions`, and `loadActionToolsForExecution`. **Must-fix LibreChat-AI#2 — assistants path coverage.** `processRequiredActions` received the same structural rewrite but had zero tests. Added a regression test under `multi-action domain collision regression` that drives two shared-hostname actions through the assistants path and asserts each tool reaches its own request builder. **Must-fix LibreChat-AI#3 — legacy encoding branch coverage.** The `if (legacyNormalized !== normalizedDomain)` registration was never exercised by any test. Added a test where `agent.tools` stores the legacy-format name and asserts it still resolves. **Should-fix LibreChat-AI#4 — DRY the registration loop.** Extracted `registerActionTools({ toolToAction, functionSignatures, normalizedDomain, legacyNormalized, makeEntry })`. All three call sites now share the same key-building logic; the key template lives in one place. **Should-fix LibreChat-AI#5 — remove stale optional chaining.** In `loadActionToolsForExecution`, `functionSignature?.description ?? ''` became `functionSignature.description` — `sig` is always defined by the iterator, matching the style of `loadAgentTools`. **Should-fix LibreChat-AI#6 — drop unreachable `!requestBuilder` guard.** Entries in `processRequiredActions` are now pre-built with `requestBuilder: requestBuilders[sig.name]`, which `openapiToFunction` always produces alongside the signature, so the guard is dead. **Should-fix LibreChat-AI#7 — unwrap `actionSetsData`.** It now holds a bare `Map` instead of `{ toolToAction }`; the sentinel `!actionSetsData` check still works because `new Map()` is truthy. Also added a short-hostname regression test (`loadAgentTools resolves raw ---separated tool names`) that reproduces Must-fix LibreChat-AI#1: it fails against the previous commit (0 create calls) and passes with the normalization in place. 41 tests, all passing. The 3 new regression tests are under `multi-action domain collision regression` and cover the assistants path, the legacy encoding branch, and the short-hostname lookup path. * 🐛 fix: Tighten registerActionTools key handling and assistants test Follow-up to d643444 addressing the second review pass on PR LibreChat-AI#12594. **ESLint** — Two prettier errors in the spec file (multi-line arrow function bodies that should fit on one line). Auto-fixed. **[MINOR] operationId containing `---` → key mismatch.** The lookup path collapsed every `actionDomainSeparator` sequence in the full tool name, but the registration path passed `sig.name` through unchanged. A `---` that survived into an operationId would shift the underscore boundary at lookup and miss its own key. Fix in `registerActionTools`: normalize `sig.name` with the same helper so registration and lookup always agree on the canonical form. `sanitizeOperationId` strips the characters that produce `---` in practice, so this is theoretical hardening, not a fix for a known reproducer. **[MINOR] Same-operationId + same-hostname silent overwrite.** Two actions sharing both an operationId and a hostname still produced a silent `Map.set()` overwrite (the new key is identical, so neither the operationId nor the domain disambiguates). Added a `setKey` helper inside `registerActionTools` that logs a `[Actions] operationId collision: ...` warning whenever a key is already present, naming the overwriting action_id. The silent-overwrite mode from the original bug cannot reappear under a different disguise without surfacing in the logs. **[NIT] processRequiredActions test simulated a runtime crash.** `mockCreateActionTool` returned a tool with `_call: jest.fn()`, which resolves to `undefined`. `processRequiredActions` chains `.then(handleToolOutput).catch(handleToolError)` directly onto that return, so `undefined.then(...)` threw synchronously and the outer try/catch funneled the error into `handleToolError`. Creation count assertions still passed because `createActionTool` runs before the crash, but the test was silently exercising the failure path. Updated the global mock to `_call: jest.fn().mockResolvedValue('{"status":"ok"}')` so the success path runs end-to-end. The assistants regression test now executes in ~5ms instead of ~90ms, which corroborates that it's no longer hitting the synchronous throw. **[NIT] Duplicated rationale comments.** All three call sites carried multi-line comment blocks restating why we key on the full tool name. That rationale now lives canonically in `registerActionTools`'s JSDoc; the inline blocks collapsed to `// See registerActionTools for the key-shape rationale.` Net -22 lines of comments. 41/41 tests still pass; lint is clean. * 🐛 fix: Scope tool-name normalization to the encoded-domain suffix Follow-up to f22228e addressing the Codex P1 on PR LibreChat-AI#12594. **Regression.** The previous commit normalized the entire tool name (`normalizeActionToolName(sig.name)` at registration, full-name `.replace()` at lookup) to handle operationIds that theoretically contained `---`. But `openapiToFunction` uses user-supplied operationIds verbatim and the fallback `sanitizeOperationId` only strips characters outside `[a-zA-Z0-9_-]`, so specs can legitimately produce operationIds like `get_foo---bar` and `get_foo_bar` side by side. Collapsing `---` to `_` across the entire key merged those two into a single map slot — one silently overwrote the other, and both tool requests routed to the surviving entry's request builder. **Fix.** Limit normalization to the encoded-domain portion of the full tool name, i.e. the substring after the last `actionDelimiter`. The operationId half is left verbatim, so hyphens-vs-underscores remain disambiguating. The short-hostname bug (Must-fix LibreChat-AI#1 from the original review) is still covered because the `---` → `_` collapse still happens on the domain suffix where it matters: ```js const normalizeActionToolName = (toolName) => { const delimiterIndex = toolName.lastIndexOf(actionDelimiter); if (delimiterIndex === -1) return toolName; const prefixEnd = delimiterIndex + actionDelimiter.length; const encodedDomain = toolName.slice(prefixEnd); return toolName.slice(0, prefixEnd) + encodedDomain.replace(domainSeparatorRegex, '_'); }; ``` `registerActionTools` reverts to `sig.name` verbatim — no more `normalizeActionToolName(sig.name)` ahead of the key build. **Regression test.** Added `loadAgentTools distinguishes operationIds that differ only by ---` vs `_`` under `multi-action domain collision regression`. It loads two actions sharing a hostname whose operationIds are `get_foo---bar` and `get_foo_bar` respectively, each pointing at a different path (`/foo-bar`, `/foo_bar`), and asserts that each tool resolves to its own request builder. Verified the test fails against f22228e (`hyphenTool` resolves to `/foo_bar` — the sibling's builder) and passes with this commit. 42/42 tests pass; lint clean.
danny-avila
added a commit
that referenced
this pull request
Apr 22, 2026
…ToolAuth contract, doc appConfig Addresses the comprehensive review of Phase 8. Findings mapped: **#1 (MINOR): `verifyToolAuth` unconditional auth for execute_code** - Added doc comment explicitly stating the deployment contract (admin capability → reachable sandbox; no per-check health probe to keep UI-gate queries O(1)). - New `api/server/controllers/__tests__/tools.verifyToolAuth.spec.js` with 4 regression tests pinning the contract: 1. `authenticated: true` + `SYSTEM_DEFINED` for execute_code. 2. 404 for unknown tool IDs. 3. `loadAuthValues` is never consulted (catches a future revert that would resurface the per-user key-entry dialog). 4. Response `message` is never `USER_PROVIDED`. **#2 (MINOR): `openai/service.ts` undocumented `appConfig` dependency** - Expanded the `ChatCompletionDependencies.appConfig` JSDoc to spell out that omitting it silently disables code execution for agents with `execute_code` in their tools. External consumers of `createAgentChatCompletion` now have the contract documented at the type boundary. **#5 (NIT): `registerCodeExecutionTools` re-allocates tool defs** - Hoisted `READ_FILE_DEF` and `BASH_TOOL_DEF` to module-level `Object.freeze`d constants. The shapes derive entirely from static `@librechat/agents` exports, so a single frozen object per tool is safe to share across every agent init. Eliminates the ~4-property allocations on every call (including the common second-call no-op path). **#6 (NIT): Verbose history-priming comment in initialize.js** - Trimmed the 16-line `handlePrimeInvokedSkills` block to a 5-line summary with `@see InitializedAgent.codeEnvAvailable` pointer. The canonical narrowing explanation lives on the type; the controller comment is just the ACL-vs-capability rationale. **Skipped:** - #3 (memory spec tests a mirror function): reviewer self-dismissed as a design tradeoff; the enum-literal pin already catches the highest-risk drift vector. - #4 (cross-repo contract for `createCodeExecutionTool`): user will explicitly install the latest `@librechat/agents` dev version once the companion PR publishes, so the version pin will be authoritative. - #7 (migration/deprecation note for self-hosters): out of scope per user direction — release notes handle this. Tests verified: 679 packages/api + 840 backend api tests pass. Typecheck + lint clean.
danny-avila
added a commit
that referenced
this pull request
Apr 22, 2026
Refines the #12776 fix series based on automated review feedback. - Scope save-time pruning to the current agent only. When a PATCH carries tool_resources, strip orphans from the incoming payload and pay the DB round-trip only then. Removes the collection-wide updateMany previously triggered when tool_resources was absent (Codex P2 / Copilot). - Wrap the orphan check in try/catch so a transient db.getFiles failure can't turn a good save into a 500 (comprehensive review #1). - Replace Object.values(EToolResources) casts with an explicit list of agent-side categories in both orphans.ts and agent.ts. code_interpreter belongs to the Assistants API and isn't a key of AgentToolResources — including it was a type lie and generated dead MongoDB clauses (comprehensive review #3, #8). - Export TOOL_RESOURCE_KEYS from @librechat/api and consume it in the migration script, dropping one duplicated definition (#4). - Cap migration results.details at 50 sample entries so the memory footprint stays bounded on deployments with thousands of corrupted agents (Codex P3). - Add migrate:orphaned-agent-files:batch npm script to match the convention set by migrate-agent-permissions / migrate-prompt-permissions (#7). - Add controller-level tests covering the three orphan-pruning paths: strip from incoming tool_resources, leave alone when tool_resources is absent, swallow db.getFiles errors and still save (#6). - Back pre-existing "should validate tool_resources in updates" test's file_ids with real File docs — the new pruning would otherwise strip them, and that test is about OCR conversion / schema filtering, not file existence. Register the File model in beforeAll so the fixture works.
danny-avila
added a commit
that referenced
this pull request
Apr 22, 2026
* 🧹 fix: Prune Orphaned File References on File Deletion Deleting a file via the Manage Files tab left its file_id in every agent's tool_resources.*.file_ids. Stubs accumulate until the frontend dedupe keys them as duplicates and blocks all new uploads (issue #12776). - Add removeAgentResourceFilesFromAllAgents in packages/data-schemas: a single updateMany/$pullAll across every EToolResources category. - Invoke it from processDeleteRequest after db.deleteFiles so every referencing agent is cleaned up, not just the one passed in req.body. - Wrap the cleanup in try/catch so a stale agent update cannot mask a successful file deletion. * 🧼 fix: Prune Orphaned File References on Agent Update Already-affected agents would stay broken even after the delete-time fix: the stubs sit on the agent document until something strips them. Heal them on the next save (issue #12776). - Add collectToolResourceFileIds + stripFileIdsFromToolResources helpers in @librechat/api — centralizing the tool_resources traversal used by the controller and the follow-up migration script. - In updateAgentHandler, check the effective tool_resources against the files collection. When orphans are found, either strip them from the incoming tool_resources (if the update sets them) or run the bulk cleanup (if the update leaves tool_resources untouched). * 🧰 chore: Add Migration to Clean Up Orphaned Agent File References Complements the delete-time and save-time fixes by healing agents that already accumulated orphan stubs before the upgrade (issue #12776). The script is idempotent — re-running it on a clean database is a no-op. - Add config/migrate-orphaned-agent-files.js following the existing migrate-*.js convention: --dry-run by default omitted (writes by default) and --batch-size= tuning knob. Streams agents via cursor. - Register migrate:orphaned-agent-files and :dry-run npm scripts. - Reuse collectToolResourceFileIds from @librechat/api so migration and runtime share the same traversal logic. * 🩹 fix: Address Codex/Copilot Review on Orphaned Agent File Cleanup Refines the #12776 fix series based on automated review feedback. - Scope save-time pruning to the current agent only. When a PATCH carries tool_resources, strip orphans from the incoming payload and pay the DB round-trip only then. Removes the collection-wide updateMany previously triggered when tool_resources was absent (Codex P2 / Copilot). - Wrap the orphan check in try/catch so a transient db.getFiles failure can't turn a good save into a 500 (comprehensive review #1). - Replace Object.values(EToolResources) casts with an explicit list of agent-side categories in both orphans.ts and agent.ts. code_interpreter belongs to the Assistants API and isn't a key of AgentToolResources — including it was a type lie and generated dead MongoDB clauses (comprehensive review #3, #8). - Export TOOL_RESOURCE_KEYS from @librechat/api and consume it in the migration script, dropping one duplicated definition (#4). - Cap migration results.details at 50 sample entries so the memory footprint stays bounded on deployments with thousands of corrupted agents (Codex P3). - Add migrate:orphaned-agent-files:batch npm script to match the convention set by migrate-agent-permissions / migrate-prompt-permissions (#7). - Add controller-level tests covering the three orphan-pruning paths: strip from incoming tool_resources, leave alone when tool_resources is absent, swallow db.getFiles errors and still save (#6). - Back pre-existing "should validate tool_resources in updates" test's file_ids with real File docs — the new pruning would otherwise strip them, and that test is about OCR conversion / schema filtering, not file existence. Register the File model in beforeAll so the fixture works. * 🩹 fix: Tighten TOOL_RESOURCE_KEYS Type and Align Migration Sample Output Two follow-ups from the second review pass. - Type data-schemas' TOOL_RESOURCE_KEYS as ReadonlyArray<keyof AgentToolResources> instead of readonly string[]. Data-schemas depends on data-provider, so the import is clean. Catches typos and aligns with the matching export in @librechat/api — doesn't guarantee exhaustiveness, but that's a TypeScript limitation, not a workspace one. - Align the migration's console output with DETAIL_SAMPLE_LIMIT: print every collected detail (up to 50) and, when more agents were affected than the sample size allowed, show a truncation notice. The old hard cap of 25 meant affected agents in the 26-50 range were collected but never shown. * ✅ test: Add Integration Coverage for Orphan Cleanup Paths (#12776) Exercise the delete-time and migration paths end-to-end against a real in-memory Mongo. Catches integration bugs the isolated unit tests on each layer couldn't. - api/server/services/Files/process.integration.spec.js — the primary repro: seed an Agent + File, call processDeleteRequest, assert the file_id disappears from every referencing agent's tool_resources while unrelated agents stay untouched. Also covers the no-op case and confirms a failure in the new cleanup step cannot roll back the file deletion itself. - api/test/migrate-orphaned-agent-files.spec.js — drives the migration module: --dry-run reports without writing, apply mode prunes across every tool_resource category, re-running is idempotent, and DETAIL_SAMPLE_LIMIT caps the in-memory sample on wide corruption. Mocks only the connect helper (the spec owns the mongoose instance) so the real migration code path — cursor, $pullAll, reduce — runs. * 🔒 fix: Run Orphan Cleanup Migration in System Tenant Context Codex P2 catch: under TENANT_ISOLATION_STRICT=true, the migration throws on the very first Agent.countDocuments() because the tenant isolation plugin fail-closes on queries without tenant context — which makes migrate:orphaned-agent-files unusable on the exact deployments most likely to have accumulated corruption. - Wrap the scan/prune body in runAsSystem so queries bypass the tenant filter (SYSTEM_TENANT_ID sentinel). The migration legitimately needs cross-tenant visibility — this is the same pattern seedDatabase and the S3 refresh job already use. - Add a regression test that spies on Agent.countDocuments() and asserts the active tenantStorage context is SYSTEM_TENANT_ID during the call. Pins the wrap against future regressions without the brittleness of toggling the strict-mode env var (which caches on first read). Note: the delete-time and save-time paths already run inside an authenticated HTTP request where tenantStorage.run is set by auth middleware, so the cleanup naturally scopes to the current tenant — which is the correct behavior there since file ownership is tenant-scoped. * 🧹 chore: Drop Unused path Import From Process Integration Spec Leftover from an earlier iteration that resolved the migration path via path.resolve before I switched to a relative require. The import does nothing now — removing it.
danny-avila
added a commit
that referenced
this pull request
Apr 22, 2026
…riming (#12649) * feat: Skill runtime integration — catalog injection, tool registration, execute handler Wires the @librechat/agents SkillTool primitive into LibreChat's agent runtime: **Enums:** - Add `skills` to AgentCapabilities + defaultAgentCapabilities **Data layer:** - Add `getSkillByName(name, accessibleIds)` — compound query that combines name lookup + ACL check in one findOne **Agent initialization (packages/api/src/agents/initialize.ts):** - Accept `accessibleSkillIds` param and `listSkillsByAccess` db method - Query accessible skills, format catalog via `formatSkillCatalog()`, append to `additional_instructions` (appears in agent system prompt) - Register `SkillToolDefinition` + `createSkillTool()` when catalog is non-empty (tool appears in model's tool list) - Store `accessibleSkillIds` and `skillCount` on InitializedAgent **Execute handler (packages/api/src/agents/handlers.ts):** - Add `getSkillByName` to `ToolExecuteOptions` - `handleSkillToolCall()` intercepts `Constants.SKILL_TOOL`: extracts skillName, loads body from DB with ACL check, substitutes $ARGUMENTS, returns ToolExecuteResult with injectedMessages (skill body as isMeta user message) **Caller wiring:** - initialize.js: query skill IDs via findAccessibleResources, pass to initializeAgent + store on agentToolContexts, add getSkillByName to toolExecuteOptions, pass accessibleSkillIds through loadTools configurable - openai.js + responses.js: same pattern for their flows Requires @librechat/agents >= 3.1.65 (PR #91 exports). * feat: Skills toggle in tools menu + backend capability gating Frontend: - Add skills?: boolean to TEphemeralAgent type - Add LAST_SKILLS_TOGGLE_ to LocalStorageKeys for persistence - Add skillsEnabled to useAgentCapabilities hook - Add skills useToolToggle to BadgeRowContext with localStorage init - New Skills.tsx badge component (Scroll icon, cyan theme, permission-gated via PermissionTypes.SKILLS) - Add skills entry to ToolsDropdown with toggle + pin - Render Skills badge in BadgeRow ephemeral section Backend: - Extract injectSkillCatalog() into packages/api/src/agents/skills.ts (reduces initializeAgent module size, reusable helper) - initializeAgent delegates to helper instead of inline block - Capability-gate the findAccessibleResources query: - Agents endpoint: checks AgentCapabilities.skills in admin config - OpenAI/Responses controllers: checks ephemeralAgent.skills toggle - ACL query runs once per run, result shared across all agents * refactor: remove createSkillTool() instance from injectSkillCatalog SkillTool is event-driven only. The tool definition in toolDefinitions is sufficient for the LLM to see the tool schema. No tool instance is needed since the host handler intercepts via ON_TOOL_EXECUTE before tool.invoke() is ever called. Removes tools from InjectSkillCatalogParams/Result, drops the createSkillTool import. * feat: skill file priming, bash tool, and invoked skills state Multi-file skill support: - New primeSkillFiles() helper (packages/api/src/agents/skillFiles.ts) uploads skill files + SKILL.md body to code execution environment - handleSkillToolCall primes files on invocation when skill.fileCount > 0, returns session info as artifact so ToolNode stores the session - Skill-primed files available to subsequent bash/code tool calls Bash tool auto-registration: - BashExecutionToolDefinition added alongside SkillToolDefinition when skills are enabled, giving the model a bash tool for running scripts Conversation state: - Add invokedSkillIds field to conversation schema (Mongoose + Zod) - handleSkillToolCall updates conversation with $addToSet on success - Enables re-priming skill files on subsequent runs (future) Dependency wiring: - Pass listSkillFiles, getStrategyFunctions, uploadCodeEnvFile, updateConversation through ToolExecuteOptions - Pass req and codeApiKey through mergedConfigurable - All three controller entry points wired (initialize.js, openai.js, responses.js) * fix: load bash_tool instance in loadToolsForExecution, remove file listing - Add createBashExecutionTool to loadToolsForExecution alongside PTC/ToolSearch pattern: loads CODE_API_KEY, creates bash tool instance on demand - Add BASH_TOOL and SKILL_TOOL to specialToolNames set so they don't go through the generic loadTools path (bash is created here, skill is intercepted in handler before tool.invoke) - Remove file name listing from skill content text — it's the skill author's responsibility to disclose files in SKILL.md, not the framework * feat: batch upload for skill files, replace sequential uploads - Add batchUploadCodeEnvFiles() to crud.js: single POST to /upload/batch with all files in one multipart request, returns shared session_id - Rewrite primeSkillFiles to collect all streams (SKILL.md + bundled files) then do one batch upload instead of N sequential uploads - Replace uploadCodeEnvFile with batchUploadCodeEnvFiles across all callers (handlers.ts, initialize.js, openai.js, responses.js) * refactor: remove invokedSkillIds from conversation schema Skills aren't re-loaded between runs, so conversation-level state for invoked skills doesn't help. Skill state will live on messages instead (like tool_search discoveredTools and summaries), enabling in-place re-injection on follow-up runs. Removes invokedSkillIds from: convo Mongoose schema, IConversation interface, Zod schema, ToolExecuteOptions.updateConversation, and all three caller wiring points. * feat: smart skill file re-priming with session freshness checking Schema: - Add codeEnvIdentifier field to ISkillFile (type + Mongoose schema) - Add updateSkillFileCodeEnvIds batch method (uses tenantSafeBulkWrite) - Export checkIfActive from Code/process.js Extraction: - Add extractInvokedSkillsFromHistory() to run.ts — scans message history for AIMessage tool_calls where name === 'skill', extracts skillName args. Follows same pattern as extractDiscoveredToolsFromHistory. Smart re-priming in primeSkillFiles: - Before batch uploading, checks if existing codeEnvIdentifiers are still active via getSessionInfo + checkIfActive (23h threshold) - If session is still active, returns cached references (zero uploads) - If stale or missing, batch-uploads everything and persists new identifiers on SkillFile documents (fire-and-forget) - Single session check covers all files (batch shares one session_id) Wiring: - Pass getSessionInfo, checkIfActive, updateSkillFileCodeEnvIds through ToolExecuteOptions and all three controller entry points * feat: wire skill file re-priming at run start via initialSessions Flow: 1. initialize.js creates primeInvokedSkills callback with all deps 2. client.js calls it with message history before createRun 3. extractInvokedSkillsFromHistory scans for skill tool calls 4. For each invoked skill with files, primeSkillFiles uploads/checks 5. Returns initialSessions map passed to createRun 6. createRun passes initialSessions to Run.create (via RunConfig) 7. Run constructor seeds Graph.sessions, making skill files available to subsequent bash/code tool calls via ToolNode session injection Requires @librechat/agents with initialSessions on RunConfig (PR #94). * refactor: use CODE_EXECUTION_TOOLS set for code tool checks Import CODE_EXECUTION_TOOLS from @librechat/agents and replace inline constant checks in handlers.ts and callbacks.js. Fixes missing bash tool coverage in the session context injection (handlers.ts) and code output processing (callbacks.js). * refactor: move primeInvokedSkills to packages/api, add skill body re-injection Moves primeInvokedSkills from an inline closure in initialize.js (with dynamic requires) to a proper exported function in packages/api skillFiles.ts with explicit typed dependencies. Key changes: - primeInvokedSkills now returns both initialSessions (for file priming) AND injectedMessages (skill bodies for context continuity) - createRun accepts invokedSkillMessages and appends skill bodies to systemContent so the model retains skill instructions across runs - initialize.js calls the packaged function with all deps passed explicitly - client.js passes both initialSessions and injectedMessages to createRun * fix: move dynamic requires to top-level module imports Move primeInvokedSkills, getStrategyFunctions, batchUploadCodeEnvFiles, getSessionInfo, and checkIfActive from inline requires to top-level module requires where they belong. * refactor: skill body reconstruction via formatAgentMessages, not systemContent Replaces the lazy systemContent approach with proper message-level reconstruction: SDK (formatAgentMessages): - New invokedSkillBodies param (Map<string, string>) - Reconstructs HumanMessages after skill ToolMessages at the correct position in the message sequence, matching where ToolNode originally injected them LibreChat: - extractInvokedSkillsFromPayload replaces extractInvokedSkillsFromHistory (works with raw TPayload before formatAgentMessages, not BaseMessage[]) - primeInvokedSkills now takes payload instead of messages, returns skillBodies Map instead of injectedMessages - client.js calls primeInvokedSkills BEFORE formatAgentMessages, passes skillBodies through as the 4th param - Removed invokedSkillMessages from createRun (no more systemContent hack) - Single-pass: skill detection happens inside formatAgentMessages' existing tool_call processing loop, zero extra message iterations * refactor: rename skillBodies to skills for consistency with SDK param * refactor: move auth loading into primeInvokedSkills, pass loadAuthValues as dep The payload/accessibleSkillIds guard and CODE_API_KEY loading now live inside primeInvokedSkills (packages/api) rather than in the CJS caller. initialize.js passes loadAuthValues as a dependency and the callback is only created when skillsCapabilityEnabled. * feat: ReadFile tool + conditional bash registration + skill path namespacing ReadFile tool (read_file): - General-purpose file reader, event-driven (ON_TOOL_EXECUTE) - Schema: { file_path: string } — "{skillName}/{path}" convention - handleReadFileCall: resolves skill name from path, ACL check, reads from DB cache or storage, binary detection, size limits (256KB), lazy caching (512KB), line numbers in output - SKILL.md special case: reads skill.body directly - Dispatched alongside SKILL_TOOL in createToolExecuteHandler - Added to specialToolNames in ToolService Conditional tool registration: - ReadFile + SkillTool: always registered when skills enabled - BashTool: only registered when codeEnvAvailable === true - codeEnvAvailable passed through InitializeAgentParams from caller Skill file path namespacing: - primeSkillFiles now uploads as "{skillName}/SKILL.md" and "{skillName}/{relativePath}" instead of flat names - Prevents file collisions when multiple skills are invoked Wiring: - getSkillFileByPath + updateSkillFileContent passed through ToolExecuteOptions in all three callers * feat: return images/PDFs as artifacts from read_file, tighten caching Binary artifact support: - Images (png, jpeg, gif, webp) returned as base64 in artifact.content with type: 'image_url', processed by existing callback attachment flow - PDFs returned as base64 artifact similarly - Binary size limit: 10MB (MAX_BINARY_BYTES) - Other binary files still return metadata + bash fallback Caching: - Text cached only on first read (file.content == null check) - Binary flag cached only on first detection (file.isBinary == null) - Skill files are immutable; no redundant cache writes Registration: - ReadFileToolDefinition now includes responseFormat: 'content_and_artifact' * chore: update @librechat/agents to version 3.1.66-dev.0 and add peer dependencies in package-lock.json and package.json files * fix: resolve review findings #1,#2,#4,#5,#6,#10,#13 Critical: - #1: primeInvokedSkills now accumulates files across all skills into one session entry instead of overwriting. Parallel processing via Promise.allSettled. - #2: codeEnvAvailable now computed and passed in openai.js and responses.js (was missing, bash tool never registered in those flows) Major: - #4: relativePath in updateSkillFileCodeEnvIds now strips the {skillName}/ prefix to match SkillFile documents. SKILL.md filter uses endsWith instead of exact match. - #5: File priming guarded on apiKey being non-empty (skip when not configured instead of failing with auth error) - #6: Skills processed in parallel via Promise.allSettled instead of sequential for-of loop Minor: - #10: Use top-level imports in initialize.js instead of inline requires - #13: Log warning when skill catalog reaches the 100-skill limit * fix: resolve followup review findings N1,N2,N4 N1 (CRITICAL): Wire skill deps into responses.js non-streaming path. Was completely missing getSkillByName, file strategy functions, etc. N2 (MAJOR): Single batch upload for ALL skills' files. Resolves skills in parallel (Phase 1), then collects all file streams across skills and does ONE batchUploadCodeEnvFiles call (Phase 2). All files share one session_id, eliminating cross-session isolation issues. N4 (MINOR): Move inline require() to top-level in openai.js and responses.js, consistent with initialize.js. * fix: add mocks for new file strategy imports in controller tests * fix: restore session freshness check, parallelize file lookups, add warnings R1: Re-add session freshness check before batch upload. Checks any existing codeEnvIdentifier via getSessionInfo + checkIfActive. If the session is still active (23h window), returns cached file references with zero re-uploads. R2: listSkillFiles calls parallelized via Promise.all (were sequential in the for-of loop). R3: Log warning when skill record lookup fails during identifier persistence (was a silent empty-string fallback). * fix: guard freshness cache on single-session consistency * fix: multi-session freshness check (code env handles mixed sessions natively) The code execution environment fetches each file by its own {session_id, fileId} pair independently — no single-session requirement. Removed the sessionIds.size === 1 guard. Now checks ALL distinct sessions for freshness. If every session is still active (23h window), returns cached references with per-file session_ids preserved. If any session expired, falls through to re-upload everything in a single batch. * perf: parallelize session freshness checks via Promise.all * fix: add optional chaining for session info retrieval in primeInvokedSkills Updated the primeInvokedSkills function to use optional chaining for getSessionInfo and checkIfActive methods, ensuring safer access and preventing potential runtime errors when these methods are undefined. * fix: address review findings #1-#9 + Codex P1/P2 + session probe Critical: - #1/Codex P1: Add codeApiKey loading to openai.js and responses.js loadTools configurable (was missing, file priming broken in 2/3 paths) - Codex P1: Fix cached file name prefix in primeSkillFiles cache path (was sf.relativePath, now ${skill.name}/${sf.relativePath}) Major: - Codex P2: Honor ephemeral skills toggle in agents endpoint (check ephemeralAgent?.skills !== false alongside admin capability) - #4: Early size check using file.bytes from DB before streaming (prevents full-file buffer for oversized files) Minor: - #5: Replace Record<string, any> with Record<string, boolean | string> - #6: Localize Pin/Unpin aria-labels with com_ui_pin/com_ui_unpin - #8: Parallelize stream acquisition in primeSkillFiles via Promise.allSettled - #9: Log warning for partial batch upload failures with filenames Performance: - Session probe optimization: getSessionInfo now hits per-object endpoint (GET /sessions/{sid}/objects/{fid}) instead of listing entire session (GET /files/{sid}?detail=summary). O(1) stat vs O(N) list + linear scan. * refactor: extract shared skill wiring helper + add unit tests DRY (#3): - New skillDeps.js exports getSkillToolDeps() with all 9 skill-related deps (getSkillByName, listSkillFiles, getStrategyFunctions, etc.) - Replaces 5 identical copy-paste blocks across initialize.js, openai.js, responses.js (streaming + non-streaming paths) - One place to maintain when skill deps change Tests (#2): - 8 unit tests for extractInvokedSkillsFromPayload covering: string args, object args, missing skill tool_calls, non-assistant messages, malformed JSON, empty skillName, empty payload, dedup * fix: remove @jest/globals import, use global jest env * fix: resolve round 2 review findings R2-1 through R2-7 R2-1 (toggle semantics): openai.js + responses.js now check admin capability (AgentCapabilities.skills) alongside ephemeral toggle. Aligns with initialize.js. R2-2 (swallowed error): primeInvokedSkills now logs updateSkillFileCodeEnvIds failures (was .catch(() => {})) R2-4 (test cast): Record<string, string> → Record<string, unknown> R2-5 (DRY regression): Extract enrichWithSkillConfigurable() into skillDeps.js. Replaces 4 identical loadAuthValues blocks. Each loadTools callback is now a one-liner. JSDoc added (R2-6). R2-7 (sequential streams): primeInvokedSkills now uses Promise.allSettled for parallel stream acquisition. * fix: require explicit skills toggle + treat partial cache as miss - initialize.js: change ephemeralSkillsToggle !== false to === true (unset toggle no longer enables skills) - primeSkillFiles cache: require ALL files to have codeEnvIdentifier before using cache (partial persistence = cache miss = re-upload) - primeInvokedSkills cache: same check (allFilesWithIds.length must equal total file count) * fix: pass entity_id=skillId on batch upload, eliminates per-user cache thrashing primeSkillFiles now passes entity_id: skill._id.toString() to batchUploadCodeEnvFiles. This scopes the code env session to the skill, not the user. All users sharing a skill share the same uploaded files — no more cache thrashing from overwriting each other's codeEnvIdentifier. The stored codeEnvIdentifier now includes ?entity_id= suffix so freshness checks pass the entity_id through to the per-object stat endpoint. Both primeSkillFiles and primeInvokedSkills store consistent identifier formats. * fix: pass entity_id on multi-skill batch upload, consistent identifier format * Revert "fix: pass entity_id on multi-skill batch upload, consistent identifier format" This reverts commit c85ce21. * refactor: per-skill upload in primeInvokedSkills, eliminate multi-skill batch Replace the monolithic multi-skill batch upload with per-skill primeSkillFiles calls. Each skill gets its own session with entity_id=skillId, ensuring: - Correct session auth (entity_id matches on freshness checks) - Per-skill freshness caching (only expired skills re-upload) - Shared skill sessions work across users (same entity_id=skillId) - Code env handles mixed session_ids natively The big batch block (stream collection, single upload, identifier mapping) is replaced by a simple loop over primeSkillFiles, which already handles freshness caching, batch upload, and identifier persistence per-skill. * fix: resolve review findings #1,#3-5,#7,#9-11 Critical: - #1: Strip ?entity_id= query string before splitting codeEnvIdentifier into session_id/fileId (was corrupting cached file IDs in 4 locations) Major: - #4: Parallelize per-skill primeSkillFiles via Promise.allSettled - #5: Add logger.warn to all empty .catch(() => {}) on cache writes Minor: - #7: Add logger.debug to enrichWithSkillConfigurable catch block - #9: Use error instanceof Error guard in batchUploadCodeEnvFiles - #10: Move enrichWithSkillConfigurable to TypeScript in packages/api (skillConfigurable.ts), skillDeps.js wraps with loadAuthValues dep - #11: Reduce MAX_BINARY_BYTES from 10MB to 5MB (~11.5MB peak with b64) * fix: forward entity_id in session probe + always register bash tool Codex P2 (entity_id in probe): getSessionInfo now preserves and forwards query params (including entity_id) to the per-object stat endpoint. Without this, identifiers stored as ...?entity_id=... would fail auth checks because the entity_id scope was dropped. Codex P2 (bash tool availability): Remove codeEnvAvailable gate from injectSkillCatalog. Bash tool definition is now always registered when skills are enabled. Actual tool instance creation still happens at execution time in loadToolsForExecution (which loads per-user credentials). This ensures users with per-user CODE_API_KEY get bash without requiring a global env var at init time. Removes codeEnvAvailable from InjectSkillCatalogParams, InitializeAgentParams, and all three controller entry points. * fix: add debug logging to primeInvokedSkills catch, rename export alias * fix: stub bash tool when no key + remove PDF artifact path Codex P1 (bash tool): When CODE_API_KEY is unavailable, create a stub tool that returns "Code execution is not available. Use read_file instead." This prevents "tool not found" errors from the model repeatedly calling bash_tool in no-code-env deployments while still registering the definition for per-user credential users. Codex P2 (PDF artifacts): Remove PDF image_url artifact path. The host artifact pipeline processes image_url via saveBase64Image which fails for PDFs. PDFs now fall through to the generic binary handler ("Use bash to process"). TODO comment for future document artifact support. Also: isImageOrPdf → isImage in early size checks (PDFs are no longer treated as artifact candidates). * fix: remove dead PDF_MIME constant, hoist skillToolDeps, document session_id - #7: Remove unused PDF_MIME constant (dead code after PDF artifact removal) - #11: Hoist skillToolDeps to module-level constant (avoid per-call allocation) - #6: Document that CodeSessionContext.session_id is a representative value; ToolNode uses per-file session_id from the files array * fix: call toolEndCallback for skill/read_file artifacts + clear codeEnvIdentifier on re-upload Codex P1 (toolEndCallback bypass): skill and read_file handler branches returned early, bypassing the toolEndCallback that processes artifacts (image attachments). Now calls toolEndCallback when the result has an artifact, using the same metadata pattern as the normal tool.invoke path. Codex P1 (stale identifiers): upsertSkillFile now $unset's codeEnvIdentifier alongside content and isBinary when a file is re-uploaded. Prevents the freshness cache from returning references to old file content after a skill file is replaced. * fix: add session_id comment at cached path, rename skillResult to handlerResult * fix: return content_and_artifact from bash stub so result.content is populated * fix: deterministic skill lookup, dedup warning, and multi-session freshness check - getSkillByName: add sort({updatedAt:-1}) so name collisions resolve deterministically to the most recently updated skill - injectSkillCatalog: warn when multiple accessible skills share a name - primeSkillFiles: check ALL distinct sessions for freshness, not just the first file's session, preventing stale refs after partial bulkWrite * refactor: update icon import in Skills component - Replaced the Scroll icon with ScrollText in the Skills component for improved clarity and consistency in the UI. * fix: SKILL.md cache parity, gate bash_tool on code env, fix read_file too-large message - primeSkillFiles: filter SKILL.md from returned files array on fresh upload so cached and non-cached paths return identical file sets (SKILL.md is still on disk in the session for bash access) - injectSkillCatalog: only register bash_tool when codeEnvAvailable is true; thread the flag from all three CJS callers via execute_code capability check - handleReadFileCall: tell the model to invoke the skill first before suggesting /mnt/data paths for oversized files * fix: use EnvVar constant, deduplicate auth lookup, validate batch upload, stream byte limit - Replace hardcoded 'LIBRECHAT_CODE_API_KEY' with EnvVar.CODE_API_KEY in skillConfigurable.ts and skillFiles.ts - Resolve code API key once at run start in initialize.js and pass to both primeInvokedSkills and enrichWithSkillConfigurable via optional preResolvedCodeApiKey param, eliminating redundant loadAuthValues calls - Add response structure validation in batchUploadCodeEnvFiles before accessing session_id/files to surface unexpected responses early - Add streaming byte counter in handleReadFileCall that aborts and destroys the stream when accumulated bytes exceed MAX_BINARY_BYTES, preventing full file buffering when DB metadata is inaccurate * refactor: update icon import in ToolsDropdown component - Replaced the Scroll icon with ScrollText in the ToolsDropdown component for improved clarity and consistency in the UI. * fix: partial upload failure detection, EnvVar in initialize.js, declaration ordering - primeSkillFiles: return null (failure) when batch upload partially succeeds — missing bundled files would cause runtime bash/read failures with missing paths in code env - initialize.js: replace hardcoded 'LIBRECHAT_CODE_API_KEY' with EnvVar.CODE_API_KEY imported from @librechat/agents - initialize.js: move enabledCapabilities, accessibleSkillIds, and codeApiKey declarations before the toolExecuteOptions closure that references them (eliminates reliance on temporal dead zone hoisting)
danny-avila
added a commit
that referenced
this pull request
Apr 22, 2026
…efaults (#12692) * feat: per-user skill active/inactive toggle with ownership-aware defaults - Add `skillStates` map (Record<string, boolean>) to user schema for per-user active/inactive overrides on skills - Add `defaultActiveOnShare` to interface.skills config (default: false) so admins can control whether shared skills auto-activate - Add GET/POST /api/user/settings/skills/active endpoints with validation - Add React Query hooks with optimistic mutations for skill states - Add useSkillActiveState hook with ownership-aware resolution: owned skills default active, shared skills default inactive - Add toggle switch UI to SkillListItem and SkillDetail components - Filter inactive skills in injectSkillCatalog before agent injection - Add localization keys for active/inactive labels * fix: use Record instead of Map for IUser.skillStates Mongoose .lean() flattens Map to a plain object, causing type incompatibility with IUser in methods that return lean documents. * fix: address review findings for skill active states - Fail-closed when userId is absent: filter rejects all shared skills instead of passing them through unfiltered (Codex P1) - Validate Mongoose Map key characters (reject . and $) in controller to return 400 instead of a 500 from schema validation (Codex P2) - Block toggle while initial skill states query is loading to prevent overwriting server-side overrides with an empty snapshot (Codex P2) - Extract shared SkillToggle component, eliminating duplicate toggle markup in SkillListItem and SkillDetail (Finding #3) - Move skill state query/mutation hooks from Favorites.ts to Skills/queries.ts per feature-directory convention (Finding #4) - Fix hardcoded English aria-label in SkillListItem by passing the localized string from the parent SkillList (Finding #5) - Fix inline arrow in SkillList render loop: pass stable callback reference so SkillListItem memo() is not invalidated (Finding #1) - Extract toRecord() helper in controller to DRY the Map-to-Object conversion (Finding #6) - Remove Promise.resolve wrapping synchronous config read (Finding #8) - Remove unused TUpdateSkillStatesRequest type (Finding #12) * fix: forward tabIndex on SkillToggle to preserve list keyboard nav The original inline toggle had tabIndex={-1} so the row itself remained the sole tab target. The extraction into SkillToggle dropped this prop, making every list toggle a tab stop. Add an optional tabIndex prop and pass -1 from SkillListItem. * fix: plumb skillStates to all agent entry points, isolate toggle keydown - Add skillStates/defaultActiveOnShare loading to openai.js and responses.js controllers so shared-skill activation is respected across all agent entry points, not just initialize.js (Codex P1) - Stop keydown propagation on SkillToggle so Enter/Space does not bubble to the parent row's navigation handler (Codex P2) * fix: paginate catalog fetch and serialize toggle writes - Paginate listSkillsByAccess (up to 10 pages of 100) until the active catalog quota is filled, so inactive shared skills in recent positions do not starve active owned skills past the first page (Codex P1) - Extend listSkillsByAccess interface with cursor/has_more/after for catalog pagination - Serialize skill-state writes via a ref queue: one in-flight request at a time, with the latest desired state sent when the previous one settles. Prevents last-response-wins races where an older request overwrites newer toggles (Codex P2) * fix: share write queue across hook instances, block toggle on fetch error - Move the write queue from a per-instance useRef to a module-scoped object so every mount of useSkillActiveState (SkillList, SkillDetail, etc.) serializes against the same in-flight slot. Prior per-instance queues allowed two components to race full-map POSTs (Codex P1) - Extend the toggle guard beyond isLoading: also block when isError is true or data is undefined. Prevents a failed GET from seeding a toggle with an empty baseline that would wipe server-side overrides on the next successful POST (Codex P1) * fix: stale closure, orphan cleanup, and cap-error UX - Read toggle baseline from React Query cache via queryClient.getQueryData instead of the captured skillStates closure. The closure can be stale between onMutate's setQueryData and the next render, so rapid successive toggles would build on old state and drop earlier changes (Codex P1) - Surface the MAX_SKILL_STATES_EXCEEDED error code with a specific toast key (com_ui_skill_states_limit) so users understand the 200-cap rather than seeing a generic error - Prune orphaned entries (skillIds whose Skill doc no longer exists) on both GET and POST in SkillStatesController. Self-heals over time without needing cascade-delete hooks or a migration job. Uses one indexed Skill._id query per request * test: pin skill active-state precedence with unit tests Extract the active-state resolution logic from a closure inside injectSkillCatalog into an exported resolveSkillActive helper, then cover every branch of the precedence matrix: - Fails closed when userId is absent (even with defaultActiveOnShare=true) - Explicit override wins over ownership and config (both true and false) - Owned skills default to active when no override is set - Shared skills default to defaultActiveOnShare value - Undefined skillStates behaves identically to an empty object - defaultActiveOnShare defaults to false when omitted - Owned skills ignore defaultActiveOnShare entirely Closes Finding #2 from the pre-rebase comprehensive review. Mirrors the existing scopeSkillIds test style; injectSkillCatalog now calls resolveSkillActive instead of inlining the closure. * refactor: limit skill active toggle to detail header, drop label - Remove the per-row toggle from SkillListItem and the active-state plumbing (hook call, isSkillEnabled/onToggleEnabled/toggleAriaLabel props) from SkillList. The detail view is now the single place to change a skill's active state - Drop dim/muted styling for inactive skills in the sidebar: without a control there, the visual indication has nowhere to land - Resize SkillToggle to match neighbor buttons: outer h-9 container, h-6 w-11 track with size-5 knob, no label span. The 'Active' / 'Inactive' text that accompanied the detail-view toggle is removed - Remove the now-unused label prop and tabIndex prop (the tabIndex existed only for the list-row context) from SkillToggle. Drop the onKeyDown stopPropagation for the same reason - Remove now-orphaned com_ui_skill_active / com_ui_skill_inactive translation keys * style: shrink SkillToggle track to h-5 w-9 with size-4 knob Container stays at h-9 to match neighbor button heights. The toggle track itself drops from h-6 w-11 to h-5 w-9, with a size-4 knob travelling 1.125rem on activation. Visually lighter inside the row. * fix: remove redundant skillStates entries that match the resolved default When a toggle lands on the ownership/config default, delete the key from the map instead of persisting `{id: defaultValue}`. Without this, a user toggling a skill off and back on would leave `{id: true}` for an owned skill (whose default is already true), silently consuming a slot against the 200-entry cap. Repeated round-trip toggles could exhaust the quota with zero meaningful overrides (Codex P2). Preserves the exceptions-list invariant that the runtime-resolution design depends on. * fix: prune before enforcing skill-state cap; reject non-ObjectId keys Reorder the update controller so pruneOrphans runs before the 200-cap check. Without this, a user near the cap with some orphaned entries (skills deleted since their last GET) could send a payload that would pass after pruning but gets rejected by the raw-size check first. Add a sanity cap on raw payload size (2 * MAX_SKILL_STATES) so abusive inputs do not reach the DB query, and enforce the real cap on the pruned result instead. Harden pruneOrphans: the earlier early-return path could pass non-ObjectId keys through unchanged. Now only valid ObjectIds are returned, and the Skill-model-unavailable fallback filters by format. Also add isValidObjectIdString validation at the input boundary so malformed (but otherwise non-Mongo-unsafe) keys never reach persistence (Codex P2 x2). * fix: enforce active filter at execute time, prune revoked shares, scope queue per user P1: injectSkillCatalog now returns activeSkillIds (the filtered set that appears in the catalog). initializeAgent uses that set as the stored accessibleSkillIds on the initialized agent, so getSkillByName at runtime cannot resolve a deactivated skill — even if the LLM hallucinates a name or the user invokes by direct-invocation shorthand. Previously the executor authorized against the full ACL set, bypassing the active-state guarantee (Codex P1). P2: pruneOrphans now checks user access via findAccessibleResources in addition to skill existence. When a share is revoked, the user's skillStates entry for that skill had no cleanup path and silently consumed the 200-cap. Self-heals on both GET and POST. One extra ACL query per settings read/write; scoped to a single user so no N-user amplification (Codex P2). P2: the write queue moves from a single module-scoped object to a Map keyed by userId. Logout/login in the same tab can no longer flush the previous user's pending snapshot under the new session's auth. Each userId gets its own pending/inFlight slot; the in-flight request retains its original auth via the cookie already attached when sent, so the race window closes (Codex P2). * refactor: extract skillStates helpers to packages/api; add tests; polish Address the remaining valid findings from the comprehensive review: - Extract toRecord, loadSkillStates, validateSkillStatesPayload, and pruneOrphanSkillStates into packages/api/src/skills/skillStates.ts as TypeScript. The controller in /api shrinks to a ~90-line thin wrapper that builds live dependency adapters for Mongoose + the permission service (Review #2 DRY, #3 workspace boundary) - Replace the triplicated 12-line skillStates loading block in initialize.js, openai.js, and responses.js with a single call to loadSkillStates from @librechat/api. One helper, three sites - Swap console.error for the project logger in the controller (Review #7) - Remove the redundant INVALID_KEY_PATTERN regex: a valid ObjectId cannot contain . or $, so isValidObjectIdString already covers it (Review #11) - Parameterize the 200-cap error toast with {{0}} interpolation driven by the error response's `limit` field, so future changes to MAX_SKILL_STATES update the UI message automatically (Review #12) - Add 24 unit tests for the new skillStates helpers (toRecord, resolveDefaultActiveOnShare, loadSkillStates, validateSkillStates- Payload, pruneOrphanSkillStates) covering success paths, malformed input, cap boundaries, and parallel-query behavior (Review #4) - Add 10 tests for injectSkillCatalog pagination covering empty accessible set, missing listSkillsByAccess, single-page filter, owned-vs-shared defaults, explicit-override precedence, multi-page collection, MAX_CATALOG_PAGES safety cap, early termination on has_more=false, additional_instructions injection, and fail-closed without userId (Review #5) Total test count: 60 (was 26 on this surface). * fix: rename skillStates ValidationError to avoid barrel-export collision packages/api/src/types/error.ts already exports a ValidationError (MongooseError extension). Re-exporting a different shape from skills/skillStates.ts through the skills barrel caused TS2308 in CI because the root index re-exports both. Rename to SkillStatesValidationError to keep the exports disjoint. * refactor: tighten tests and absorb caller guard into loadSkillStates Address the followup review findings: - Add optional `accessibleSkillIds` param to loadSkillStates so the helper short-circuits to defaults when no skills are accessible. All three controllers drop the residual 7-line conditional wrapper in favor of a single destructured call (Review #2) - Remove the unreachable `typeof key !== 'string'` check from validateSkillStatesPayload: Object.entries always yields string keys per the JS spec (Review #3) - Replace the two `as unknown as` agent casts in the injectSkillCatalog tests with a `makeAgent()` factory typed directly as the function's parameter shape (Review #4) - Tighten the MAX_CATALOG_PAGES assertion from `toBeLessThanOrEqual(11)` to `toHaveBeenCalledTimes(10)` — the loop deterministically makes exactly 10 page fetches before hitting the cap (Review #1) - Rewrite the parallel-execution test for pruneOrphanSkillStates using deferred promises instead of microtask-order assertions. The test now inspects `toHaveBeenCalledTimes(1)` on both mocks after a single Promise.resolve() yield, pinning Promise.all usage without relying on push-order into a shared array (Review #5) - Evict stale writeQueue entries on user change via a module-scoped `lastSeenUserId` sentinel. When a different user's toggle is the first one after a logout/login, the previous user's queue entry is deleted. Keeps the Map bounded without adding hook-instance effect cleanup (Review #6) * fix(test): mock loadSkillStates in openai and responses controller specs The prior refactor replaced the inline 12-line skillStates loading block with a call to loadSkillStates from @librechat/api. Both controller spec files mock @librechat/api as a flat object, so any new named import from that package is undefined in the test env. Calling `await loadSkillStates(...)` threw before recordCollectedUsage ran, surfacing as "undefined is not iterable" on the test's array destructure of `mockRecordCollectedUsage.mock.calls[0]`. Add the missing mock to both spec files alongside the existing scopeSkillIds stub. * fix: abandon stale skillStates write queues on user switch Close the cross-session leak window where an in-flight flush loop still holds a reference to a previous user's queue: it could fire its next mutateAsync under the new session's auth cookies and persist the stale snapshot to the new user's document (Codex P1). Add an `abandoned` flag on `WriteQueue`. Three mechanisms cooperate: - `getWriteQueue` marks every non-active queue abandoned when the user differs from the last-seen identity (pre-existing eviction site, now more aggressive). - A `useEffect` on `userId` calls the same abandonment pass on every render with a new active identity, covering the window between logout/login and the new user's first toggle (when `getWriteQueue` would otherwise not fire). - The flush loop checks `!queue.abandoned` in its while condition so the second and later iterations exit without firing another `mutateAsync` after the session changes. The first iteration's in-flight request (already dispatched under the original user's cookies) still runs to completion or failure on its own — only the subsequent iterations, which are the dangerous ones, are blocked.
danny-avila
added a commit
that referenced
this pull request
Apr 22, 2026
…le` / `allowed-tools` (#12745) * 🧬 feat: Persist `disable-model-invocation` / `user-invocable` / `allowed-tools` Adds first-class columns mirroring the three runtime-enforced frontmatter fields, with a `deriveStructuredFrontmatterFields` helper that maps from frontmatter at create/update time and re-syncs (via `$unset`) when fields are removed. `listSkillsByAccess` projection includes them so the Phase 6 catalog filter and popover filter can both read off the summary row. Marks `invocationMode` as @deprecated on `TSkill` and the `InvocationMode` enum — the runtime now reads the persisted pair instead. * 🛡️ feat: Enforce frontmatter at runtime (catalog, skill tool, manual resolver, tool union) Wires the persisted columns into actual runtime behavior across all four invocation paths: - `injectSkillCatalog` excludes `disableModelInvocation: true` skills before catalog formatting — they cost zero context tokens and stay invisible to the model. - `handleSkillToolCall` rejects with a clear error when the model names a skill marked `disable-model-invocation: true` (defends against a stale-cache or hallucinated invocation getting past the catalog filter). - `resolveManualSkills` skips `userInvocable: false` skills with a warn log so an API-direct caller can't bypass the popover-side filter. - `unionPrimeAllowedTools` collects skill-declared `allowed-tools` minus what's already on the agent; `initialize.ts` re-runs `loadTools` for the extras and merges resulting `toolDefinitions` into the agent's effective set for the turn. Tool-name resolution is tolerant — unknown names silently drop with a debug log so cross-ecosystem skills referencing yet-to-be-implemented tools (Claude Code's `edit_file`, etc.) import without breaking. The agent document is never modified; the union is turn-scoped. Helper exports (`unionPrimeAllowedTools`) are structured so Phase 5's always-apply primes flow through the same union (combined `[...manualPrimes, ...alwaysApplyPrimes]`) once the resolver lands. Skill handler wire format gains the three fields so clients can render them on detail / list views. * 🎛️ feat: `$` popover reads `userInvocable` instead of UI-only `invocationMode` Replaces the phase-1 UI-only `invocationMode` check with the persisted `userInvocable` field (mirrors the `user-invocable` frontmatter). Skills authored with `user-invocable: false` no longer surface in the popover; the backend resolver enforces the same rule for defense-in-depth. Default-visible behavior is preserved: skills without an explicit `userInvocable` value (older rows, freshly imported skills that don't declare the field) stay visible — only an explicit `false` hides them. Test fixture updated to reflect the new field. * 🔧 fix: Address Phase 6 review findings Codex P2 + reviewer #1: Single `loadTools` call with the union of `agent.tools + allowed-tools`. The earlier two-call approach dropped `userMCPAuthMap` / `toolContextMap` / `actionsEnabled` from the skill-added pass — an MCP tool gained via `allowed-tools` would be visible to the model but fail at execution without per-user auth context. Resolution of `manualSkillPrimes` is hoisted before `loadTools` so the union can be computed up-front; the dropped-tools debug log now compares loaded vs. requested across the single call. Codex P3 + reviewer #2: `injectSkillCatalog.activeSkillIds` now includes `disable-model-invocation: true` skills. The runtime ACL check in `handleSkillToolCall` previously couldn't reach the explicit "cannot be invoked by the model" rejection because the broader access set excluded those skills. Catalog text and tool registration still gate on the visible subset (zero-context-token guarantee preserved); only the per-user `isActive` filter is a hard exclusion now. Reviewer #1 (try/catch around loadTools, MAJOR): A single bad `allowed-tools` entry from a shared skill could crash the entire turn. Now wrapped — on failure with extras, retry with just `agent.tools` and continue (the dropped-tools debug log surfaces what vanished). If the retry-without-extras still throws, propagate; the agent's own tools are the load-bearing surface. Reviewer #3 (integration tests, MAJOR): Added six tests in `initialize.test.ts` covering the full `allowed-tools` loading path: union pass-through, no-extras short-circuit, agent-baseline dedup, loadTools throw + retry, propagated throw without extras, and the empty-tools edge case. Smaller cleanups bundled in: - Reviewer #4: Moved `logger` import to the package-imports section (was wedged among local imports). - Reviewer #5: Removed unused index on `disableModelInvocation` (filtering happens application-side in `injectSkillCatalog`; index cost write overhead for zero query benefit). - Reviewer #6: Swapped order of `userInvocable` and body checks in `resolveManualSkills` so the more authoritative author-decision reason surfaces first when both apply. - Reviewer #8: Documented the `allowedTools` enforcement gap on the schema + type — model-invoked skills (mid-turn `skill` tool calls) do NOT trigger tool union, since adding tools after the graph starts would require a rebuild. Manual / always-apply (Phase 5) primes are the supported paths. - Reviewer #9: Renamed `dmi` / `ui` / `at` locals to `disableModelInvocationRaw` / `userInvocableRaw` / `allowedToolsRaw` in `deriveStructuredFrontmatterFields`. Reviewer #7 (DRY shared `getSkillByName` return type) deferred — field sets diverge meaningfully across the three call sites (handler needs `body + fileCount`; resolver needs `author + allowedTools + userInvocable`; the InitializeAgentDbMethods contract needs the superset). A `Pick<>`-based consolidation is a follow-up cleanup. * 🔧 fix: Address codex iter 2 — catalog quota + duplicate-name dedup P1: `injectSkillCatalog` cap now counts only model-visible skills, not the merged active set. The previous behavior let a tenant with many `disable-model-invocation: true` rows near the top of the cursor exhaust the 100-slot quota before any invocable skill got scanned — the catalog could end up empty even though invocable skills existed further down the paginated results. `MAX_CATALOG_PAGES` stays the ceiling on scan budget; only `visibleCount` drives the early-exit on quota fill. P2: When an invocable and a `disable-model-invocation: true` skill share a name, drop the disabled doc(s) from `activeSkillIds`. Without this dedup, `getSkillByName` (which sorts by `updatedAt` desc) could pick the disabled doc and every model call to the cataloged name would fail with "cannot be invoked by the model" instead of executing the visible skill. When ONLY a disabled doc exists for a name, it stays in `activeSkillIds` so the explicit-rejection error path still fires for hallucinated invocations. Tests: 3 new cases in `injectSkillCatalog` covering (a) cap counted on visible skills only, (b) same-name collision drops disabled doc, (c) sole-disabled-name case keeps the disabled doc. * 🔒 fix: Apply `disable-model-invocation` gate to read_file too (codex iter 3 P1) `activeSkillIds` is shared between the `skill` and `read_file` handlers. The skill-tool gate was applied last iteration, but `handleReadFileCall` authorized purely on `getSkillByName(..., accessibleIds)` — so a model that learned a hidden skill's name (stale catalog or hallucination) could still read its `SKILL.md` body or bundled files via `read_file`, defeating the contract. Same explicit rejection now fires from both handlers; no change needed to the ACL set itself (disabled docs stay in `activeSkillIds` so the explicit error path keeps firing). Two new tests in `handlers.spec.ts` cover the read_file gate and regression-protect the happy path. * 🔧 fix: Address codex iter 4 — manual-prime exception + legacy frontmatter backfill P1: Scope the `read_file` `disableModelInvocation` gate to AUTONOMOUS model probes only. A user-invoked `$` skill that is also marked `disable-model-invocation: true` had its bundled `references/*` / `scripts/*` files unreadable, leaving the manually-primed skill body referencing files the model couldn't load. Now the handler bypasses the gate when the skill name appears in `manualSkillNames` (the per-turn allowlist threaded from `manualSkillPrimes` → `agentToolContexts` → `enrichWithSkillConfigurable` → `mergedConfigurable`). Defense-in-depth: the bypass is scoped to the specific names in the allowlist; a different disabled skill name is still rejected. P2: Read-time fallback for legacy skills authored before Phase 6 landed the structured columns. `user-invocable: false` / `disable-model-invocation: true` set in `frontmatter` (the validator already accepted those keys) but with no derived column would incorrectly evaluate as "user-invocable / model-allowed" until a save backfilled the columns. New `backfillDerivedFromFrontmatter` helper fills undefined columns from frontmatter at read time in both `getSkillByName` and `listSkillsByAccess` — column wins when both are set, frontmatter fills the gap when only it's set. No DB writes; the next `updateSkill` naturally persists. `listSkillsByAccess` projection expanded to include `frontmatter` (bounded by validator, payload impact small) so summaries can also be backfilled. Sticky-primed disabled skills (ones invoked in prior turns of the same conversation) are not yet in the manual-prime allowlist — same- turn manual invocation is the load-bearing path codex flagged; the sticky-turn case is a known limitation tracked for a follow-up. Tests: 2 new in handlers.spec.ts (manual-prime allows + name-scoped block holds), 3 new in skill.spec.ts (legacy backfill via getSkillByName + listSkillsByAccess + column-wins precedence). * 🔧 fix: Address codex iter 5 — propagate manualSkillNames + keep read_file P1: `enrichWithSkillConfigurable` is also called from `openai.js` and `responses.js` (the OpenAI Responses + completions endpoints). Both were ignoring the new `manualSkillNames` parameter, which meant the manual-prime exception in the `read_file` gate (iter 4) only worked on the agents endpoint. Now all three call sites pass `primaryConfig.manualSkillPrimes?.map(p => p.name)` so manual `$` invocations of disabled skills work consistently across endpoints. P2: When every accessible skill is `disable-model-invocation: true`, the catalog text and `skill` tool are correctly omitted (no model- reachable targets) — but `read_file` and `bash_tool` MUST still be registered. A user manually invoking such a skill gets its SKILL.md body primed into context; if the body references `references/foo.md` or `scripts/run.sh`, those reads need a registered tool. Restructured `injectSkillCatalog` so `skill` registration is gated on `catalogVisibleSkills.length > 0` while `read_file` (always) and `bash_tool` (when codeEnvAvailable) register whenever any active skill is in scope. Tests: existing all-disabled test rewritten to assert read_file IS registered + skill is NOT; new test confirms bash_tool joins it when codeEnvAvailable. * 🔧 fix: Address codex iter 6 — name-collision consistency via preferInvocable P2a (resolveManualSkills): a name collision between an older user-invocable doc and a newer non-user-invocable doc made manual `$` invocation silently no-op. The popover surfaced the older invocable doc; resolver looked it up by name; `getSkillByName` returned the newer non-invocable doc; resolver skipped on `userInvocable: false`. P2b (handler / runtime ACL): with same-name duplicates (e.g. older invocable + newer disabled), the manual prime resolved to one doc while later `read_file` / `skill` execution resolved a different doc through `activeSkillIds`. Model could follow one SKILL.md body while reading files from a different skill. Both root-cause: `getSkillByName` always returned the newest match and let the caller filter, but with collisions the newest can be something the caller didn't want. Fix: extend `getSkillByName` with `options.preferInvocable`. When true, prefer the newest doc satisfying BOTH `userInvocable !== false` AND `disableModelInvocation !== true` (with frontmatter backfill); fall back to the newest match otherwise. Fast path preserved when caller doesn't opt in. Callers passing `preferInvocable: true`: - `resolveManualSkills` — picks the popover-visible invocable doc even when a newer disabled / non-user-invocable duplicate exists. - `handleSkillToolCall` — keeps execution aligned with the catalog; falls back to the disabled doc only when no invocable variant exists (so the explicit "cannot be invoked by the model" gate still fires for the hallucinated-disabled-name case). - `handleReadFileCall` — same alignment, plus the manual-prime exception added in iter 4 still applies. Tests: - 2 new in skill.spec.ts (preferInvocable picks invocable when collision exists; falls back to newest when no clean-invocable exists). - 1 new in skills.test.ts (resolver passes preferInvocable through). - 2 new in handlers.spec.ts (skill tool + read_file pass it). - Existing initialize.test.ts assertion updated for the new option. * 🔧 fix: Address codex iter 7 — split preferInvocable into per-axis flags The previous unified `preferInvocable` filter required BOTH `userInvocable !== false` AND `disableModelInvocation !== true`. That was wrong for the model paths: `userInvocable: false` skills are model-only and remain valid `skill` / `read_file` invocation targets. A duplicate-name scenario where the newer cataloged doc was model- only would let the older user-invocable variant shadow it on every model call. Split the option into two independent axes: - `preferUserInvocable` — for manual paths (`$` popover). Skips docs with `userInvocable: false`. Disable-model-invocation status is irrelevant; iter 4 explicitly supports manual prime of disabled skills. - `preferModelInvocable` — for model paths (`skill` / `read_file` handlers). Skips docs with `disableModelInvocation: true`. User- invocable status is irrelevant; model-only skills are valid here. Both flags fall back to the newest match when no preferred doc exists, so the explicit-rejection error paths still fire correctly in the sole-disabled-name case. Callers updated: - `resolveManualSkills` → `preferUserInvocable: true` - `handleSkillToolCall` / `handleReadFileCall` → `preferModelInvocable: true` Tests: - New spec test for preferModelInvocable not filtering on userInvocable. - Existing preferInvocable test renamed/split to cover the new axes. - New test asserts preferUserInvocable still returns disabled docs (preserves iter 4 manual-disabled support). - Caller tests assert each path passes the right single flag and does NOT pass the wrong one. * 🔧 fix: TypeScript type-check failure in handlers.spec.ts (CI green) `jest.fn(async () => ...)` without explicit args infers an empty tuple for the call signature, so `mock.calls[0][2]` flagged as "Tuple type '[]' has no element at index '2'." Cast to `unknown[]` then narrow to the expected option shape. Behavior unchanged. Caught by the `Type check @librechat/api` CI step (.github/workflows/backend-review.yml). * 🔧 fix: Address codex iter 8 — undefined-result fallback + read_file alignment P1 (loadTools returning undefined): Production loaders (`createToolLoader` in `initialize.js` / `openai.js` / `responses.js`) wrap `loadAgentTools` in try/catch and return `undefined` on failure rather than throwing. Without explicit handling, my iter-1 try/catch only fired for thrown errors — a silent-failure on a skill-added tool would fall through to the empty fallback and silently DROP the agent's baseline tools for the turn (much worse than just losing the extras). Added an `undefined`-result branch that retries with just `agent.tools`, mirroring the throw branch. Test pins both behaviors. P2 (read_file alignment with manual prime): When a skill is in this turn's `manualSkillNames`, the `read_file` handler now uses `preferUserInvocable` instead of `preferModelInvocable`. Same name-collision rule as `resolveManualSkills`, so the doc whose files get read is the same doc whose body got primed. For autonomous probes (skill not in `manualSkillNames`), the handler keeps `preferModelInvocable` to align with the catalog the model saw. Two new tests cover both branches and regression-protect that the wrong flag isn't passed. * 🔧 fix: Address codex iter 9 — pin read_file lookup to primed skill _id P1 (manually-primed disabled IDs were dropped from activeSkillIds): The `executableSkills` dedup in `injectSkillCatalog` correctly drops `disable-model-invocation: true` duplicates when an invocable doc shares the name — but `resolveManualSkills` legitimately primes disabled docs (iter 4 supports manual `$` invocation of disabled skills). When the resolver primed a disabled doc, the read_file handler couldn't find it in the (deduped) `activeSkillIds` and either resolved a different same-name skill or returned not-found. Fix: `ResolvedManualSkill` now carries `_id`; the legacy `initialize.js` / `openai.js` / `responses.js` controllers build a `manualSkillPrimedIdsByName` map and `enrichWithSkillConfigurable` passes it into `mergedConfigurable`. `handleReadFileCall` now pins its lookup's `accessibleIds` to `[primedId]` whenever the requested skill is in the map. The constrained set guarantees the lookup returns the EXACT doc the resolver primed — body/files come from the same source even when same-name duplicates exist or the dedup removed the prime's id from `activeSkillIds`. Autonomous read_file probes (skill not in the manual-primed map) keep the full ACL set + `preferModelInvocable` so they align with the catalog the model saw and the disabled-only case still fires the explicit-rejection gate. Test fixture changes flow from `_id` becoming required on `ResolvedManualSkill`. `buildSkillPrimeContentParts` / `injectManualSkillPrimes` widen their param types to `Pick<...>` because they only read `name` / `body` and shouldn't force test literals to invent placeholder ids. * 🧹 fix: Address independent reviewer findings (DRY + types + tests + docs) Sanity-pass review surfaced 7 findings; addressed 6 (the 7th — DRY on inline `getSkillByName` return types — is acknowledged tech debt deferred to a follow-up). #1 [MAJOR, DRY]: The 4-line `manualSkillPrimedIdsByName` map construction was duplicated across 4 CJS call sites (openai.js, responses.js x2, initialize.js). Extracted `buildManualSkillPrimedIdsByName` helper in `skillDeps.js`; all four sites now call the helper. If `ResolvedManualSkill` ever renames `_id` or gains identifying fields, only the helper changes. #2 [MINOR, type safety]: `handleReadFileCall` was casting a hex string to `Types.ObjectId[]` via `as unknown as`, relying on mongoose's auto-cast in `$in` queries. Replaced with `new Types.ObjectId(...)` so any future consumer comparing with `.equals()` / `===` gets the correct value type. Imported `Types` as a value (was type-only). #5 [MINOR, test gap]: Added a test for the worst-case silent-failure path — both the union and base-only `loadTools` calls return undefined. The agent gets no tools but the turn doesn't crash hard; pinning that contract. #4 [MINOR, performance]: Added a TODO on the `listSkillsByAccess` projection noting the `frontmatter` field can be dropped once a write migration backfills all pre-Phase-6 skills' columns. ~2KB/skill × 100/page is wasted bandwidth post-backfill. #6 [NIT, docs]: `backfillDerivedFromFrontmatter` JSDoc said "Pure" right before "mutates its undefined fields in place". Replaced with "Side-effect-free w.r.t. the DB (no writes), but mutates its argument in place" which describes both halves accurately. #7 [NIT, test determinism]: Replaced `await new Promise(r => setTimeout(r, 5))` in two same-name collision tests with explicit `updateOne` setting `updatedAt: new Date(Date.now() - 1000)` on the older doc. Removes the wall-clock race on fast CI runners. The pagination test (line 480) still uses setTimeout — that test is pre-existing and order is incidental, not load-bearing. Existing test fixtures updated to use valid 24-char hex ObjectIds (required by the iter-9 test that constructs a real `ObjectId`). #3 [MINOR, deferred]: Inline `getSkillByName` return type duplicated across `handlers.ts`, `initialize.ts`, `skills.ts`. Reviewer acknowledged this as deferred; field sets diverge across call sites (handler needs `fileCount`, resolver needs `author`/`allowedTools`). A `Pick<>`-based consolidation is a clean follow-up.
danny-avila
added a commit
that referenced
this pull request
Apr 22, 2026
* 🛠️ feat: Add registerCodeExecutionTools helper
Idempotently registers `bash_tool` + `read_file` in the run's tool
registry and tool-definition list via a registry `.has()` dedupe. Sets
up the single code-execution tool path shared by:
- `initializeAgent` (when an agent has `execute_code` in its tools and
the capability is enabled for the run)
- `injectSkillCatalog` (when skills are active; unconditional read_file,
bash_tool follows `codeEnvAvailable`)
Both callers reach the helper in the same initialization sequence, so
the second call becomes a no-op and exactly one copy of each tool
reaches the LLM — no more double registration for agents that combine
`execute_code` capability with active skills.
Unit-tested on a fresh run, idempotence (second call, overlap with
prior tooldefs, partial overlap), and the no-registry variant.
* 🔀 refactor: Route injectSkillCatalog bash_tool + read_file through registerCodeExecutionTools
The `skill` tool is still registered inline (it's skill-path-specific),
but `bash_tool` + `read_file` now flow through the shared idempotent
helper so a prior registration from the execute_code path doesn't
produce a duplicate copy later in the same run. Behavior preserved:
- `read_file` always registers when any active skill is in scope —
manually-primed `disable-model-invocation: true` skills still need it
to load `references/*` from storage.
- `bash_tool` follows `codeEnvAvailable` exactly as before.
Adds a test pinning the cross-call dedupe: when `injectSkillCatalog`
runs AFTER `registerCodeExecutionTools` has already seeded the registry
+ tool definitions with bash_tool/read_file, the resulting
`toolDefinitions` still contains exactly one copy of each.
* 🪄 feat: Expand `execute_code` tool name into bash_tool + read_file at initialize-time
When an agent's `tools` include `execute_code` and the `execute_code`
capability is enabled for the run, `initializeAgent` now registers
`bash_tool` + `read_file` via `registerCodeExecutionTools` before
`injectSkillCatalog`. The legacy `execute_code` tool definition is no
longer handed to the LLM — `execute_code` remains on the agent
document as a capability-trigger marker, but the runtime expands it
into the skill-flavored tool pair.
Call ordering matters: the `execute_code` registration runs BEFORE
`injectSkillCatalog`, so the skill path's own `registerCodeExecutionTools`
call inside `injectSkillCatalog` becomes a no-op via the registry's
`.has()` check. Exactly one copy of each tool reaches the LLM whether
the agent has:
- only `execute_code` (legacy path)
- only skills
- both
No data migration needed — `agent.tools: ['execute_code']` stays in
the DB unchanged; the expansion is a runtime operation.
Three tests cover the matrix: execute_code + capability on →
bash_tool + read_file registered; execute_code + capability off →
neither registered; no execute_code + capability on → neither
registered.
* 🗑️ refactor: Drop CodeExecutionToolDefinition from the builtin registry
Removes the legacy `execute_code` entry from `agentToolDefinitions` and
the corresponding import. With the initialize-time expansion in place,
nothing consults `getToolDefinition('execute_code')` for a tool schema
any more — the capability gate still filters on the string
`execute_code`, but the actual tool definitions the LLM sees come from
`registerCodeExecutionTools` (i.e. `bash_tool` + `read_file`).
`loadToolDefinitions` in `packages/api/src/tools/definitions.ts`
silently drops `execute_code` when it no longer resolves in the
registry — that's the expected path and is now covered by an updated
test. No caller of `getToolDefinition('execute_code')` expects a
non-undefined result after this change.
* 🔌 refactor: Read CODE_API_KEY from env for primeCodeFiles + PTC
Finishes the Phase 4 server-env-keyed rollout on the two remaining
`loadAuthValues({ authFields: [EnvVar.CODE_API_KEY] })` sites in
`ToolService.js`:
- `primeCodeFiles` (user-attached file priming on execute_code agents)
- Programmatic Tool Calling (`createProgrammaticToolCallingTool`)
Both now read `process.env[EnvVar.CODE_API_KEY]` directly, matching
`bash_tool`'s pattern. The per-user plugin-auth path is no longer
consulted for code-env credentials anywhere in the hot path — the
agents library owns the actual tool-call execution and also reads the
env var internally.
Priming still fires for existing user-file workflows so the legacy
`toolContextMap[execute_code]` hint ("files available at /mnt/data/...")
stays in the prompt; only the key lookup changed.
* 🔧 fix: Type the pre-seeded dedupe-test tools as LCTool
CI TypeScript type checks caught `{ parameters: {} }` in the new
cross-call dedupe test: `LCTool.parameters` is a `JsonSchemaType`,
not `{}`. Use `{ type: 'object', properties: {} }` and type the
local registry Map through the parameter-derived shape so the
pre-seeded values match what `toolRegistry.set` expects.
* 🛡️ fix: Run execute_code expansion before GOOGLE_TOOL_CONFLICT gate
Codex review caught a latent regression: the original Phase 8 placement
ran `registerCodeExecutionTools` after `hasAgentTools` was computed,
so an execute-code-only agent on Google/Vertex with provider-specific
`options.tools` populated would no longer trip `GOOGLE_TOOL_CONFLICT`
— the legacy `CodeExecutionToolDefinition` used to populate
`toolDefinitions` before the guard, but after dropping it from the
registry, `toolDefinitions` stayed empty until my expansion ran
downstream of the guard. Mixed provider + agent tools would silently
flow through to the LLM.
Fix moves the `execute_code` expansion to BEFORE `hasAgentTools`
computation. `bash_tool` + `read_file` now contribute to the check
the same way the legacy `execute_code` def did. Covered by a new
test that pins the Google+execute_code+provider-tools scenario —
the `rejects.toThrow(/google_tool_conflict/)` path would have
silently passed on the prior placement.
* 🔗 fix: Thread codeEnvAvailable through handoff sub-agents
Round-2 codex review caught the other half of the execute_code
expansion gap: `discoverConnectedAgents` omitted `codeEnvAvailable`
from its forwarded `initializeAgent` params, so handoff sub-agents
with `agent.tools: ['execute_code']` lost the `bash_tool` + `read_file`
registration (pre-Phase 8 the legacy `CodeExecutionToolDefinition`
would have landed in their `toolDefinitions` via the registry).
- Add `codeEnvAvailable?` to `DiscoverConnectedAgentsParams` and
forward it verbatim on every sub-agent `initializeAgent` call.
- Update the three JS call sites that construct the primary's
`codeEnvAvailable` (`services/Endpoints/agents/initialize.js`,
`controllers/agents/openai.js`, `controllers/agents/responses.js`)
to pass the same flag into `discoverConnectedAgents` — one
authoritative source per request.
- Two regression tests in `discovery.spec.ts` pin the true/false
passthrough so a future refactor that drops the param-forwarding
surfaces immediately.
Left intentionally unchanged: `packages/api/src/agents/openai/service.ts`
(public API helper with no in-repo caller). External consumers of
`createAgentChatCompletion` who want code execution should pass a
`codeEnvAvailable`-aware `initializeAgent` via `deps` — documenting
the full public-API surface is out of scope for this Phase 8 PR.
* 🔗 fix: Thread codeEnvAvailable through addedConvo + memory-agent paths
Round-3 codex review caught the last two production `initializeAgent`
callers missing the Phase-8 capability flag:
- `api/server/services/Endpoints/agents/addedConvo.js` (multi-convo
parallel agent execution). Added `codeEnvAvailable` to
`processAddedConvo`'s destructured params and forwarded it into
the per-added-agent `initializeAgent` call. Caller in
`api/server/services/Endpoints/agents/initialize.js` passes the
same `codeEnvAvailable` it computed for the primary.
- `api/server/controllers/agents/client.js` (`useMemory` — memory
extraction agent). Computes its own `codeEnvAvailable` from
`appConfig?.endpoints?.[EModelEndpoint.agents]?.capabilities` and
forwards into `initializeAgent`. Memory agents rarely list
`execute_code`, but if one does, pre-Phase 8 they got the legacy
`execute_code` tool registered unconditionally — the passthrough
restores parity.
With this, every production caller of `initializeAgent` explicitly
resolves the capability: main chat flow (primary + handoff), OpenAI
chat completions (primary + handoff), Responses API (primary + handoff),
added convo parallel agents, and memory agents. The one remaining
caller, `packages/api/src/agents/openai/service.ts::createAgentChatCompletion`,
is a public API helper with no in-repo consumer (external callers
must pass a capability-aware `initializeAgent` via `deps`).
* 🪤 fix: Remove duplicate appConfig declaration causing TDZ ReferenceError
The Responses API controller had TWO `const appConfig = req.config;`
bindings inside `createResponse`: one at the top of the function
(added by the Phase 4 `bash_tool` decouple) and one inside the try
block (added by the polish PR #12760). Because `const` is block-scoped
with a temporal dead zone, the inner redeclaration put `appConfig` in
TDZ for the entire try block, so any earlier reference inside the
try — notably `appConfig?.endpoints?.[EModelEndpoint.agents]?.allowedProviders`
at line 348 — threw `ReferenceError: Cannot access 'appConfig'
before initialization`. The error was silently swallowed by the
outer try/catch, leaving `recordCollectedUsage` unreached and the
six `responses.unit.spec.js` token-usage tests failing.
Removing the inner redeclaration fixes the six failing tests
(verified: 11/11 pass locally post-fix, 0 regressions elsewhere).
The outer function-scoped binding already provides `appConfig` to
every downstream reference.
* 🔗 fix: Thread codeEnvAvailable through the OpenAI chat-completion public API
Round-4 codex review (legitimate on the type-safety angle, even though the
runtime concern was already covered): the `createAgentChatCompletion`
helper defines its own narrower `InitializeAgentParams` interface locally,
and the type was missing `codeEnvAvailable`. External consumers who
supply a capability-aware `deps.initializeAgent` couldn't route
`codeEnvAvailable` through without a type-cast workaround.
- Widen the local `InitializeAgentParams` interface to include
`codeEnvAvailable?: boolean` (matches the real
`packages/api/src/agents/initialize.ts` type).
- Derive `codeEnvAvailable` inside `createAgentChatCompletion` from
`deps.appConfig?.endpoints?.agents?.capabilities` (the same source
the in-repo controllers use) and forward to `deps.initializeAgent`.
Uses a string literal `'execute_code'` lookup so this file stays free
of a `librechat-data-provider` import — keeping the dependency surface
of the public helper minimal.
With this, external consumers of `createAgentChatCompletion` who pass
`appConfig` with the agents capabilities get `bash_tool` + `read_file`
registration automatically; consumers who don't pass `appConfig` retain
the existing "explicit opt-in" semantics (the flag stays `undefined`,
expansion is skipped).
* 🧹 chore: Review-driven polish — observability log, JSDoc DRY, test gaps, no-op allocation
Addresses the comprehensive review of PR #12767:
- **Finding #1** (MINOR, observability): `initializeAgent` now emits a
debug log when an agent lists `execute_code` in its tools but the
runtime gate is off (`params.codeEnvAvailable` !== true). The
event-driven `loadToolDefinitionsWrapper` path doesn't log
capability-disabled warnings, so without this the tool silently
vanishes from the LLM's definitions with zero trace. Operators
debugging "why isn't code interpreter working?" now get a signal at
the initialize layer.
- **Finding #5** (NIT, allocation): `registerCodeExecutionTools` now
returns the input `toolDefinitions` array by reference on the no-op
path (both tools already registered by a prior caller in the same
run) instead of allocating a fresh spread array every time. The
common dual-call scenario — `initializeAgent` then
`injectSkillCatalog` — saves one O(n) copy per request.
- **Finding #4** (NIT, DRY): Collapsed the duplicated 6-line JSDoc
comment in `openai.js`, `responses.js`, and `addedConvo.js` into
either a one-line `@see DiscoverConnectedAgentsParams.codeEnvAvailable`
pointer (the two JS call sites) or a compact 3-line block referring
back to the canonical source (addedConvo's @PARAM).
- **Finding #2** (MINOR, test gap): Added
`api/server/services/Endpoints/agents/addedConvo.spec.js` with three
cases covering `codeEnvAvailable=true`, `codeEnvAvailable=false`,
and omitted (undefined) passthrough. A future refactor that drops
the param from destructuring now surfaces here instead of silently
regressing multi-convo parallel agents with `execute_code`.
- **Finding #3** (MINOR, test gap): Added
`api/server/controllers/agents/__tests__/client.memory.spec.js`
pinning the capability-flag derivation that `AgentClient::useMemory`
uses — six cases covering present/absent/null/undefined config shapes
plus an enum-literal pin (`'execute_code'` / `'agents'`). Catches
enum renames or config-path shifts that would otherwise silently
strip `bash_tool` + `read_file` from memory agents.
Finding #7 (jest.mock scoping, confidence 40) left as-is: the
reviewer's own risk assessment noted `buildToolSet` doesn't touch
the mocked exports, and restructuring a file-level `jest.mock` to
`jest.doMock` + dynamic `import()` introduces more complexity than
the speculative risk justifies. The existing mock is scoped to the
test file and contains the same stubs the adjacent
`skills.test.ts` already uses.
Finding #6 (PR description commit count) addressed out-of-band via
PR description update.
All existing tests pass, typecheck clean, lint clean across touched
files. New tests: 9 cases across 2 new spec files.
* 🧽 refactor: Replace hardcoded 'execute_code' string with AgentCapabilities enum in service.ts
Follow-up review (conf 55) caught that `openai/service.ts`'s Phase 8
`codeEnvAvailable` derivation used the literal `'execute_code'` while
every in-repo controller uses `AgentCapabilities.execute_code` from
`librechat-data-provider`. The file deliberately uses local type
interfaces to keep the public API helper's type surface small, but
that pattern was never a ban on single-value imports from the data
provider — `packages/api` already depends on it. Importing the enum
value means a future rename of `AgentCapabilities.execute_code`
propagates to this file automatically, matching the in-repo
controllers' behavior.
Other follow-up findings left as-is per the reviewer's own verdict:
- #2 (memory spec mirrors the production expression rather than
calling `AgentClient::useMemory` directly): reviewer flagged as
"not blocking" / "design-philosophy observation." The test file's
JSDoc already explicitly documents the tradeoff and pins the enum
literals to catch the most likely drift vector. Standing up
`AgentClient` + all its mocks for a one-line regression guard is
disproportionate.
- #3 (`addedConvo.spec.js` mock signature vs. underlying
`loadAddedAgent` arity): reviewer's own confidence 25 noted the
mock matches the wrapper's actual call pattern in the production
file. Not a real gap.
- #4 was self-retracted as a false alarm.
* 🗑️ refactor: Fully deprecate CODE_API_KEY — remove all LibreChat-side references
The code-execution sandbox no longer authenticates via a per-run
`CODE_API_KEY` (frontend or backend). Auth moved server-side into the
agents library / sandbox service, so LibreChat drops every reference:
**Backend plumbing:**
- `api/server/services/Files/Code/crud.js`: `getCodeOutputDownloadStream`,
`uploadCodeEnvFile`, `batchUploadCodeEnvFiles` no longer accept
`apiKey` or send the `X-API-Key` header.
- `api/server/services/Files/Code/process.js`: `processCodeOutput`,
`getSessionInfo`, `primeFiles` drop the `apiKey` param throughout.
- `api/server/services/ToolService.js`: stop reading
`process.env[EnvVar.CODE_API_KEY]` for `primeCodeFiles` and PTC; the
agents library handles auth internally. Remove the now-dead
`loadAuthValues` + `EnvVar` imports. Drop the misleading
"LIBRECHAT_CODE_API_KEY" hint from the bash_tool error log.
- `api/server/services/Files/process.js`: remove the `loadAuthValues`
call around `uploadCodeEnvFile`.
- `api/server/routes/files/files.js`: code-env file download no longer
fetches a per-user key.
- `api/server/controllers/tools.js`: `execute_code` is no longer a
tool that needs verifyToolAuth with `[EnvVar.CODE_API_KEY]` — the
endpoint always reports system-authenticated so the client skips
the key-entry dialog. `processCodeOutput` called without `apiKey`.
- `api/server/controllers/agents/callbacks.js`: `processCodeOutput`
invoked without the loadAuthValues round trip, for both LegacyHandler
and Responses-API handlers.
- `api/app/clients/tools/util/handleTools.js`: `createCodeExecutionTool`
called with just `user_id` + files.
**packages/api:**
- `packages/api/src/agents/skillFiles.ts`: `PrimeSkillFilesParams`,
`PrimeInvokedSkillsDeps`, `primeSkillFiles`, `primeInvokedSkills` all
drop the `apiKey` param; the gate is purely `codeEnvAvailable`.
- `packages/api/src/agents/handlers.ts`: `handleSkillToolCall` drops
the `process.env[EnvVar.CODE_API_KEY]` read; skill-file priming is
now gated solely on `codeEnvAvailable`. `ToolExecuteOptions`
signatures drop apiKey from `batchUploadCodeEnvFiles` and
`getSessionInfo`.
- `packages/api/src/agents/skillConfigurable.ts`: JSDoc no longer
references the env var.
- `packages/api/src/tools/classification.ts`: PTC creation no longer
gated on `loadAuthValues`; `buildToolClassification` drops the
`loadAuthValues` dep entirely (no LibreChat-side callers need it for
this path anymore).
- `packages/api/src/tools/definitions.ts`: `LoadToolDefinitionsDeps`
drops the `loadAuthValues` field.
**Frontend:**
- Delete `client/src/hooks/Plugins/useAuthCodeTool.ts`,
`useCodeApiKeyForm.ts`, and
`client/src/components/SidePanel/Agents/Code/ApiKeyDialog.tsx` —
the install/revoke dialogs for CODE_API_KEY are fully dead.
- `BadgeRowContext.tsx`: drop `codeApiKeyForm` from the context type and
provider. `codeInterpreter` toggle treated as always authenticated
(sandbox auth is server-side).
- `ToolsDropdown.tsx`, `ToolDialogs.tsx`, `CodeInterpreter.tsx`,
`RunCode.tsx`, `SidePanel/Agents/Code/Action.tsx` +`Form.tsx`: all
API-key dialog trigger refs, "Configure code interpreter" gear
buttons, and auth-verification plumbing removed. The
"Code Interpreter" toggle is now a plain `AgentCapabilities.execute_code`
checkbox — no key-entry gate.
- `client/src/locales/en/translation.json`: drop the three
`com_ui_librechat_code_api*` keys and `com_ui_add_code_interpreter_api_key`.
Other locales are externally automated per CLAUDE.md.
**Config:**
- `.env.example`: remove the `# LIBRECHAT_CODE_API_KEY=your-key` section
and its header.
**Tests:**
- `crud.spec.js`: assertions flipped to pin "no X-API-Key header" and
"no apiKey param".
- `skillFiles.spec.ts`: removed env-var save/restore; tests now pin
that the batch-upload path is gated solely on `codeEnvAvailable` and
that no apiKey is threaded through.
- `handlers.spec.ts`: same — just the `codeEnvAvailable` gate pins
remain.
- `classification.spec.ts`: remove the two tests that asserted
`loadAuthValues` was (not) called for PTC.
- `definitions.spec.ts`: drop every `loadAuthValues: mockLoadAuthValues`
entry from the deps shape.
- `process.spec.js`: strip the mock of `EnvVar.CODE_API_KEY`.
**Comment hygiene:**
- `tools.ts`, `initialize.ts`, `registry/definitions.ts`: shortened
stale comment references to "legacy `execute_code` tool" without
naming the retired env var.
Tests verified: 678 packages/api tests pass, 836 backend api tests
pass. Typecheck clean, lint clean. Only remaining CODE_API_KEY
mentions in the code are two regression-guard assertions:
- `crud.spec.js`: pins "no X-API-Key header" stays absent.
- `skillConfigurable.spec.ts`: pins `configurable` never grows a
`codeApiKey` field.
* 🧹 chore: Remove the last two CODE_API_KEY name mentions in LibreChat
Follow-up to the prior full deprecation commit: two tests still named
the retired identifier in their regression-guard assertions.
- `packages/api/src/agents/skillConfigurable.spec.ts`: drop the
"does not inject a codeApiKey key" test. The `codeApiKey` field is
gone from the production configurable shape, so an absence-assertion
naming it re-introduces the retired identifier in code.
- `api/server/services/Files/Code/crud.spec.js`: rename the
"without an X-API-Key header" case back to "should request stream
response from the correct URL" and drop the
`expect(headers).not.toHaveProperty('X-API-Key')` assertion. The
surrounding request-shape checks (URL, timeout, responseType) still
pin the behavior; the explicit header-absence line was named-after
the deprecated contract.
Result: `grep -rn "CODE_API_KEY\|codeApiKey\|LIBRECHAT_CODE_API_KEY"`
against the LibreChat source tree returns zero hits. The only
remaining `X-API-Key` strings in this repo are on unrelated OpenAPI
Action + MCP server auth configurations, where the string is
user-facing config, not a LibreChat-owned identifier.
Tests: 677 packages/api pass (2 pre-existing summarization e2e failures
unrelated); 126 api-workspace controller/service tests pass.
Typecheck and lint clean.
* 🎯 fix: Narrow codeEnvAvailable to per-agent (admin cap AND agent.tools)
Before this commit, `codeEnvAvailable` was computed in the three JS
controllers as the admin-level capability flag only
(`enabledCapabilities.has(AgentCapabilities.execute_code)`) and passed
through `initializeAgent` → `injectSkillCatalog` / `primeInvokedSkills` /
`enrichWithSkillConfigurable` unchanged. A skills-only agent whose
`tools` array didn't include `execute_code` still got `bash_tool`
registered (via `injectSkillCatalog`) and skill files re-primed to the
sandbox on every turn — wrong, because the agent never opted in to
code execution.
**Fix:** `initializeAgent` now computes the per-agent effective value
once as `params.codeEnvAvailable === true && agent.tools.includes(Tools.execute_code)`,
reuses the same boolean for:
1. The `execute_code` → `bash_tool + read_file` expansion gate
(previously already consulted `agent.tools`; now shares the single
`effectiveCodeEnvAvailable` binding).
2. The `injectSkillCatalog` call (previously got the raw admin flag).
3. The returned `InitializedAgent.codeEnvAvailable` field (new, typed as
required boolean).
**Controllers (initialize.js, openai.js, responses.js):** store
`primaryConfig.codeEnvAvailable` in `agentToolContexts.set(primaryId, ...)`,
capture `config.codeEnvAvailable` in every handoff `onAgentInitialized`
callback, and read it from the per-agent ctx inside the
`toolExecuteOptions.loadTools` runtime closure. The hoisted
`const codeEnvAvailable = enabledCapabilities.has(...)` locals in the
two OpenAI-compat controllers are gone — they were shadowing the
narrowed per-agent value.
**primeInvokedSkills:** `handlePrimeInvokedSkills` in
`services/Endpoints/agents/initialize.js` now uses
`primaryConfig.codeEnvAvailable` (per-agent, narrowed) instead of the
raw admin flag. A skills-only primary agent won't re-prime historical
skill files to the sandbox even when the admin enabled the capability
globally.
**Efficiency:** one extra `&&` in `initializeAgent`. No runtime hot-path
cost — the `includes()` scan on `agent.tools` was already happening for
the `execute_code` expansion gate; it's now just bound to a local. Tool
execution closures read `ctx.codeEnvAvailable === true` (property
access + strict equality, O(1)).
**Ephemeral-agent note:** per-agent narrowing is authoritative for both
persisted and ephemeral flows. The ephemeral toggle
(`ephemeralAgent.execute_code`) is reconciled into `agent.tools`
upstream in `packages/api/src/agents/added.ts`, so
`agent.tools.includes('execute_code')` is the single source of truth
by the time `initializeAgent` runs.
**Tests:** two new regression tests pin the narrowing contract:
- `initialize.test.ts` — four-quadrant matrix on
`InitializedAgent.codeEnvAvailable` (cap on × agent asks, cap on ×
doesn't ask, cap off × asks, neither). Catches future refactors that
drop either half of the AND.
- `skills.test.ts` — `injectSkillCatalog` with `codeEnvAvailable: false`
against an active skill catalog must NOT register `bash_tool` even
though it still registers `read_file` + `skill`. This is the state
a skills-only agent gets post-narrowing.
All 191 affected packages/api tests pass + 836 backend api tests pass.
Typecheck clean, lint clean.
* 🧽 refactor: Comprehensive-review polish — hoist tool defs, pin verifyToolAuth contract, doc appConfig
Addresses the comprehensive review of Phase 8. Findings mapped:
**#1 (MINOR): `verifyToolAuth` unconditional auth for execute_code**
- Added doc comment explicitly stating the deployment contract
(admin capability → reachable sandbox; no per-check health probe
to keep UI-gate queries O(1)).
- New `api/server/controllers/__tests__/tools.verifyToolAuth.spec.js`
with 4 regression tests pinning the contract:
1. `authenticated: true` + `SYSTEM_DEFINED` for execute_code.
2. 404 for unknown tool IDs.
3. `loadAuthValues` is never consulted (catches a future revert
that would resurface the per-user key-entry dialog).
4. Response `message` is never `USER_PROVIDED`.
**#2 (MINOR): `openai/service.ts` undocumented `appConfig` dependency**
- Expanded the `ChatCompletionDependencies.appConfig` JSDoc to spell
out that omitting it silently disables code execution for agents
with `execute_code` in their tools. External consumers of
`createAgentChatCompletion` now have the contract documented at
the type boundary.
**#5 (NIT): `registerCodeExecutionTools` re-allocates tool defs**
- Hoisted `READ_FILE_DEF` and `BASH_TOOL_DEF` to module-level
`Object.freeze`d constants. The shapes derive entirely from
static `@librechat/agents` exports, so a single frozen object per
tool is safe to share across every agent init. Eliminates the
~4-property allocations on every call (including the common
second-call no-op path).
**#6 (NIT): Verbose history-priming comment in initialize.js**
- Trimmed the 16-line `handlePrimeInvokedSkills` block to a 5-line
summary with `@see InitializedAgent.codeEnvAvailable` pointer.
The canonical narrowing explanation lives on the type; the
controller comment is just the ACL-vs-capability rationale.
**Skipped:**
- #3 (memory spec tests a mirror function): reviewer self-dismissed
as a design tradeoff; the enum-literal pin already catches the
highest-risk drift vector.
- #4 (cross-repo contract for `createCodeExecutionTool`): user will
explicitly install the latest `@librechat/agents` dev version
once the companion PR publishes, so the version pin will be
authoritative.
- #7 (migration/deprecation note for self-hosters): out of scope
per user direction — release notes handle this.
Tests verified: 679 packages/api + 840 backend api tests pass.
Typecheck + lint clean.
* 🔧 chore: Update @librechat/agents version to 3.1.68-dev.1 across package-lock and package.json files
This commit updates the version of the `@librechat/agents` package from `3.1.68-dev.0` to `3.1.68-dev.1` in the `package-lock.json` and relevant `package.json` files. This change ensures consistency across the project and incorporates any updates or fixes from the new version.
danny-avila
added a commit
that referenced
this pull request
Apr 25, 2026
…riming (#12649) * feat: Skill runtime integration — catalog injection, tool registration, execute handler Wires the @librechat/agents SkillTool primitive into LibreChat's agent runtime: **Enums:** - Add `skills` to AgentCapabilities + defaultAgentCapabilities **Data layer:** - Add `getSkillByName(name, accessibleIds)` — compound query that combines name lookup + ACL check in one findOne **Agent initialization (packages/api/src/agents/initialize.ts):** - Accept `accessibleSkillIds` param and `listSkillsByAccess` db method - Query accessible skills, format catalog via `formatSkillCatalog()`, append to `additional_instructions` (appears in agent system prompt) - Register `SkillToolDefinition` + `createSkillTool()` when catalog is non-empty (tool appears in model's tool list) - Store `accessibleSkillIds` and `skillCount` on InitializedAgent **Execute handler (packages/api/src/agents/handlers.ts):** - Add `getSkillByName` to `ToolExecuteOptions` - `handleSkillToolCall()` intercepts `Constants.SKILL_TOOL`: extracts skillName, loads body from DB with ACL check, substitutes $ARGUMENTS, returns ToolExecuteResult with injectedMessages (skill body as isMeta user message) **Caller wiring:** - initialize.js: query skill IDs via findAccessibleResources, pass to initializeAgent + store on agentToolContexts, add getSkillByName to toolExecuteOptions, pass accessibleSkillIds through loadTools configurable - openai.js + responses.js: same pattern for their flows Requires @librechat/agents >= 3.1.65 (PR #91 exports). * feat: Skills toggle in tools menu + backend capability gating Frontend: - Add skills?: boolean to TEphemeralAgent type - Add LAST_SKILLS_TOGGLE_ to LocalStorageKeys for persistence - Add skillsEnabled to useAgentCapabilities hook - Add skills useToolToggle to BadgeRowContext with localStorage init - New Skills.tsx badge component (Scroll icon, cyan theme, permission-gated via PermissionTypes.SKILLS) - Add skills entry to ToolsDropdown with toggle + pin - Render Skills badge in BadgeRow ephemeral section Backend: - Extract injectSkillCatalog() into packages/api/src/agents/skills.ts (reduces initializeAgent module size, reusable helper) - initializeAgent delegates to helper instead of inline block - Capability-gate the findAccessibleResources query: - Agents endpoint: checks AgentCapabilities.skills in admin config - OpenAI/Responses controllers: checks ephemeralAgent.skills toggle - ACL query runs once per run, result shared across all agents * refactor: remove createSkillTool() instance from injectSkillCatalog SkillTool is event-driven only. The tool definition in toolDefinitions is sufficient for the LLM to see the tool schema. No tool instance is needed since the host handler intercepts via ON_TOOL_EXECUTE before tool.invoke() is ever called. Removes tools from InjectSkillCatalogParams/Result, drops the createSkillTool import. * feat: skill file priming, bash tool, and invoked skills state Multi-file skill support: - New primeSkillFiles() helper (packages/api/src/agents/skillFiles.ts) uploads skill files + SKILL.md body to code execution environment - handleSkillToolCall primes files on invocation when skill.fileCount > 0, returns session info as artifact so ToolNode stores the session - Skill-primed files available to subsequent bash/code tool calls Bash tool auto-registration: - BashExecutionToolDefinition added alongside SkillToolDefinition when skills are enabled, giving the model a bash tool for running scripts Conversation state: - Add invokedSkillIds field to conversation schema (Mongoose + Zod) - handleSkillToolCall updates conversation with $addToSet on success - Enables re-priming skill files on subsequent runs (future) Dependency wiring: - Pass listSkillFiles, getStrategyFunctions, uploadCodeEnvFile, updateConversation through ToolExecuteOptions - Pass req and codeApiKey through mergedConfigurable - All three controller entry points wired (initialize.js, openai.js, responses.js) * fix: load bash_tool instance in loadToolsForExecution, remove file listing - Add createBashExecutionTool to loadToolsForExecution alongside PTC/ToolSearch pattern: loads CODE_API_KEY, creates bash tool instance on demand - Add BASH_TOOL and SKILL_TOOL to specialToolNames set so they don't go through the generic loadTools path (bash is created here, skill is intercepted in handler before tool.invoke) - Remove file name listing from skill content text — it's the skill author's responsibility to disclose files in SKILL.md, not the framework * feat: batch upload for skill files, replace sequential uploads - Add batchUploadCodeEnvFiles() to crud.js: single POST to /upload/batch with all files in one multipart request, returns shared session_id - Rewrite primeSkillFiles to collect all streams (SKILL.md + bundled files) then do one batch upload instead of N sequential uploads - Replace uploadCodeEnvFile with batchUploadCodeEnvFiles across all callers (handlers.ts, initialize.js, openai.js, responses.js) * refactor: remove invokedSkillIds from conversation schema Skills aren't re-loaded between runs, so conversation-level state for invoked skills doesn't help. Skill state will live on messages instead (like tool_search discoveredTools and summaries), enabling in-place re-injection on follow-up runs. Removes invokedSkillIds from: convo Mongoose schema, IConversation interface, Zod schema, ToolExecuteOptions.updateConversation, and all three caller wiring points. * feat: smart skill file re-priming with session freshness checking Schema: - Add codeEnvIdentifier field to ISkillFile (type + Mongoose schema) - Add updateSkillFileCodeEnvIds batch method (uses tenantSafeBulkWrite) - Export checkIfActive from Code/process.js Extraction: - Add extractInvokedSkillsFromHistory() to run.ts — scans message history for AIMessage tool_calls where name === 'skill', extracts skillName args. Follows same pattern as extractDiscoveredToolsFromHistory. Smart re-priming in primeSkillFiles: - Before batch uploading, checks if existing codeEnvIdentifiers are still active via getSessionInfo + checkIfActive (23h threshold) - If session is still active, returns cached references (zero uploads) - If stale or missing, batch-uploads everything and persists new identifiers on SkillFile documents (fire-and-forget) - Single session check covers all files (batch shares one session_id) Wiring: - Pass getSessionInfo, checkIfActive, updateSkillFileCodeEnvIds through ToolExecuteOptions and all three controller entry points * feat: wire skill file re-priming at run start via initialSessions Flow: 1. initialize.js creates primeInvokedSkills callback with all deps 2. client.js calls it with message history before createRun 3. extractInvokedSkillsFromHistory scans for skill tool calls 4. For each invoked skill with files, primeSkillFiles uploads/checks 5. Returns initialSessions map passed to createRun 6. createRun passes initialSessions to Run.create (via RunConfig) 7. Run constructor seeds Graph.sessions, making skill files available to subsequent bash/code tool calls via ToolNode session injection Requires @librechat/agents with initialSessions on RunConfig (PR #94). * refactor: use CODE_EXECUTION_TOOLS set for code tool checks Import CODE_EXECUTION_TOOLS from @librechat/agents and replace inline constant checks in handlers.ts and callbacks.js. Fixes missing bash tool coverage in the session context injection (handlers.ts) and code output processing (callbacks.js). * refactor: move primeInvokedSkills to packages/api, add skill body re-injection Moves primeInvokedSkills from an inline closure in initialize.js (with dynamic requires) to a proper exported function in packages/api skillFiles.ts with explicit typed dependencies. Key changes: - primeInvokedSkills now returns both initialSessions (for file priming) AND injectedMessages (skill bodies for context continuity) - createRun accepts invokedSkillMessages and appends skill bodies to systemContent so the model retains skill instructions across runs - initialize.js calls the packaged function with all deps passed explicitly - client.js passes both initialSessions and injectedMessages to createRun * fix: move dynamic requires to top-level module imports Move primeInvokedSkills, getStrategyFunctions, batchUploadCodeEnvFiles, getSessionInfo, and checkIfActive from inline requires to top-level module requires where they belong. * refactor: skill body reconstruction via formatAgentMessages, not systemContent Replaces the lazy systemContent approach with proper message-level reconstruction: SDK (formatAgentMessages): - New invokedSkillBodies param (Map<string, string>) - Reconstructs HumanMessages after skill ToolMessages at the correct position in the message sequence, matching where ToolNode originally injected them LibreChat: - extractInvokedSkillsFromPayload replaces extractInvokedSkillsFromHistory (works with raw TPayload before formatAgentMessages, not BaseMessage[]) - primeInvokedSkills now takes payload instead of messages, returns skillBodies Map instead of injectedMessages - client.js calls primeInvokedSkills BEFORE formatAgentMessages, passes skillBodies through as the 4th param - Removed invokedSkillMessages from createRun (no more systemContent hack) - Single-pass: skill detection happens inside formatAgentMessages' existing tool_call processing loop, zero extra message iterations * refactor: rename skillBodies to skills for consistency with SDK param * refactor: move auth loading into primeInvokedSkills, pass loadAuthValues as dep The payload/accessibleSkillIds guard and CODE_API_KEY loading now live inside primeInvokedSkills (packages/api) rather than in the CJS caller. initialize.js passes loadAuthValues as a dependency and the callback is only created when skillsCapabilityEnabled. * feat: ReadFile tool + conditional bash registration + skill path namespacing ReadFile tool (read_file): - General-purpose file reader, event-driven (ON_TOOL_EXECUTE) - Schema: { file_path: string } — "{skillName}/{path}" convention - handleReadFileCall: resolves skill name from path, ACL check, reads from DB cache or storage, binary detection, size limits (256KB), lazy caching (512KB), line numbers in output - SKILL.md special case: reads skill.body directly - Dispatched alongside SKILL_TOOL in createToolExecuteHandler - Added to specialToolNames in ToolService Conditional tool registration: - ReadFile + SkillTool: always registered when skills enabled - BashTool: only registered when codeEnvAvailable === true - codeEnvAvailable passed through InitializeAgentParams from caller Skill file path namespacing: - primeSkillFiles now uploads as "{skillName}/SKILL.md" and "{skillName}/{relativePath}" instead of flat names - Prevents file collisions when multiple skills are invoked Wiring: - getSkillFileByPath + updateSkillFileContent passed through ToolExecuteOptions in all three callers * feat: return images/PDFs as artifacts from read_file, tighten caching Binary artifact support: - Images (png, jpeg, gif, webp) returned as base64 in artifact.content with type: 'image_url', processed by existing callback attachment flow - PDFs returned as base64 artifact similarly - Binary size limit: 10MB (MAX_BINARY_BYTES) - Other binary files still return metadata + bash fallback Caching: - Text cached only on first read (file.content == null check) - Binary flag cached only on first detection (file.isBinary == null) - Skill files are immutable; no redundant cache writes Registration: - ReadFileToolDefinition now includes responseFormat: 'content_and_artifact' * chore: update @librechat/agents to version 3.1.66-dev.0 and add peer dependencies in package-lock.json and package.json files * fix: resolve review findings #1,#2,#4,#5,#6,#10,#13 Critical: - #1: primeInvokedSkills now accumulates files across all skills into one session entry instead of overwriting. Parallel processing via Promise.allSettled. - #2: codeEnvAvailable now computed and passed in openai.js and responses.js (was missing, bash tool never registered in those flows) Major: - #4: relativePath in updateSkillFileCodeEnvIds now strips the {skillName}/ prefix to match SkillFile documents. SKILL.md filter uses endsWith instead of exact match. - #5: File priming guarded on apiKey being non-empty (skip when not configured instead of failing with auth error) - #6: Skills processed in parallel via Promise.allSettled instead of sequential for-of loop Minor: - #10: Use top-level imports in initialize.js instead of inline requires - #13: Log warning when skill catalog reaches the 100-skill limit * fix: resolve followup review findings N1,N2,N4 N1 (CRITICAL): Wire skill deps into responses.js non-streaming path. Was completely missing getSkillByName, file strategy functions, etc. N2 (MAJOR): Single batch upload for ALL skills' files. Resolves skills in parallel (Phase 1), then collects all file streams across skills and does ONE batchUploadCodeEnvFiles call (Phase 2). All files share one session_id, eliminating cross-session isolation issues. N4 (MINOR): Move inline require() to top-level in openai.js and responses.js, consistent with initialize.js. * fix: add mocks for new file strategy imports in controller tests * fix: restore session freshness check, parallelize file lookups, add warnings R1: Re-add session freshness check before batch upload. Checks any existing codeEnvIdentifier via getSessionInfo + checkIfActive. If the session is still active (23h window), returns cached file references with zero re-uploads. R2: listSkillFiles calls parallelized via Promise.all (were sequential in the for-of loop). R3: Log warning when skill record lookup fails during identifier persistence (was a silent empty-string fallback). * fix: guard freshness cache on single-session consistency * fix: multi-session freshness check (code env handles mixed sessions natively) The code execution environment fetches each file by its own {session_id, fileId} pair independently — no single-session requirement. Removed the sessionIds.size === 1 guard. Now checks ALL distinct sessions for freshness. If every session is still active (23h window), returns cached references with per-file session_ids preserved. If any session expired, falls through to re-upload everything in a single batch. * perf: parallelize session freshness checks via Promise.all * fix: add optional chaining for session info retrieval in primeInvokedSkills Updated the primeInvokedSkills function to use optional chaining for getSessionInfo and checkIfActive methods, ensuring safer access and preventing potential runtime errors when these methods are undefined. * fix: address review findings #1-#9 + Codex P1/P2 + session probe Critical: - #1/Codex P1: Add codeApiKey loading to openai.js and responses.js loadTools configurable (was missing, file priming broken in 2/3 paths) - Codex P1: Fix cached file name prefix in primeSkillFiles cache path (was sf.relativePath, now ${skill.name}/${sf.relativePath}) Major: - Codex P2: Honor ephemeral skills toggle in agents endpoint (check ephemeralAgent?.skills !== false alongside admin capability) - #4: Early size check using file.bytes from DB before streaming (prevents full-file buffer for oversized files) Minor: - #5: Replace Record<string, any> with Record<string, boolean | string> - #6: Localize Pin/Unpin aria-labels with com_ui_pin/com_ui_unpin - #8: Parallelize stream acquisition in primeSkillFiles via Promise.allSettled - #9: Log warning for partial batch upload failures with filenames Performance: - Session probe optimization: getSessionInfo now hits per-object endpoint (GET /sessions/{sid}/objects/{fid}) instead of listing entire session (GET /files/{sid}?detail=summary). O(1) stat vs O(N) list + linear scan. * refactor: extract shared skill wiring helper + add unit tests DRY (#3): - New skillDeps.js exports getSkillToolDeps() with all 9 skill-related deps (getSkillByName, listSkillFiles, getStrategyFunctions, etc.) - Replaces 5 identical copy-paste blocks across initialize.js, openai.js, responses.js (streaming + non-streaming paths) - One place to maintain when skill deps change Tests (#2): - 8 unit tests for extractInvokedSkillsFromPayload covering: string args, object args, missing skill tool_calls, non-assistant messages, malformed JSON, empty skillName, empty payload, dedup * fix: remove @jest/globals import, use global jest env * fix: resolve round 2 review findings R2-1 through R2-7 R2-1 (toggle semantics): openai.js + responses.js now check admin capability (AgentCapabilities.skills) alongside ephemeral toggle. Aligns with initialize.js. R2-2 (swallowed error): primeInvokedSkills now logs updateSkillFileCodeEnvIds failures (was .catch(() => {})) R2-4 (test cast): Record<string, string> → Record<string, unknown> R2-5 (DRY regression): Extract enrichWithSkillConfigurable() into skillDeps.js. Replaces 4 identical loadAuthValues blocks. Each loadTools callback is now a one-liner. JSDoc added (R2-6). R2-7 (sequential streams): primeInvokedSkills now uses Promise.allSettled for parallel stream acquisition. * fix: require explicit skills toggle + treat partial cache as miss - initialize.js: change ephemeralSkillsToggle !== false to === true (unset toggle no longer enables skills) - primeSkillFiles cache: require ALL files to have codeEnvIdentifier before using cache (partial persistence = cache miss = re-upload) - primeInvokedSkills cache: same check (allFilesWithIds.length must equal total file count) * fix: pass entity_id=skillId on batch upload, eliminates per-user cache thrashing primeSkillFiles now passes entity_id: skill._id.toString() to batchUploadCodeEnvFiles. This scopes the code env session to the skill, not the user. All users sharing a skill share the same uploaded files — no more cache thrashing from overwriting each other's codeEnvIdentifier. The stored codeEnvIdentifier now includes ?entity_id= suffix so freshness checks pass the entity_id through to the per-object stat endpoint. Both primeSkillFiles and primeInvokedSkills store consistent identifier formats. * fix: pass entity_id on multi-skill batch upload, consistent identifier format * Revert "fix: pass entity_id on multi-skill batch upload, consistent identifier format" This reverts commit c85ce21. * refactor: per-skill upload in primeInvokedSkills, eliminate multi-skill batch Replace the monolithic multi-skill batch upload with per-skill primeSkillFiles calls. Each skill gets its own session with entity_id=skillId, ensuring: - Correct session auth (entity_id matches on freshness checks) - Per-skill freshness caching (only expired skills re-upload) - Shared skill sessions work across users (same entity_id=skillId) - Code env handles mixed session_ids natively The big batch block (stream collection, single upload, identifier mapping) is replaced by a simple loop over primeSkillFiles, which already handles freshness caching, batch upload, and identifier persistence per-skill. * fix: resolve review findings #1,#3-5,#7,#9-11 Critical: - #1: Strip ?entity_id= query string before splitting codeEnvIdentifier into session_id/fileId (was corrupting cached file IDs in 4 locations) Major: - #4: Parallelize per-skill primeSkillFiles via Promise.allSettled - #5: Add logger.warn to all empty .catch(() => {}) on cache writes Minor: - #7: Add logger.debug to enrichWithSkillConfigurable catch block - #9: Use error instanceof Error guard in batchUploadCodeEnvFiles - #10: Move enrichWithSkillConfigurable to TypeScript in packages/api (skillConfigurable.ts), skillDeps.js wraps with loadAuthValues dep - #11: Reduce MAX_BINARY_BYTES from 10MB to 5MB (~11.5MB peak with b64) * fix: forward entity_id in session probe + always register bash tool Codex P2 (entity_id in probe): getSessionInfo now preserves and forwards query params (including entity_id) to the per-object stat endpoint. Without this, identifiers stored as ...?entity_id=... would fail auth checks because the entity_id scope was dropped. Codex P2 (bash tool availability): Remove codeEnvAvailable gate from injectSkillCatalog. Bash tool definition is now always registered when skills are enabled. Actual tool instance creation still happens at execution time in loadToolsForExecution (which loads per-user credentials). This ensures users with per-user CODE_API_KEY get bash without requiring a global env var at init time. Removes codeEnvAvailable from InjectSkillCatalogParams, InitializeAgentParams, and all three controller entry points. * fix: add debug logging to primeInvokedSkills catch, rename export alias * fix: stub bash tool when no key + remove PDF artifact path Codex P1 (bash tool): When CODE_API_KEY is unavailable, create a stub tool that returns "Code execution is not available. Use read_file instead." This prevents "tool not found" errors from the model repeatedly calling bash_tool in no-code-env deployments while still registering the definition for per-user credential users. Codex P2 (PDF artifacts): Remove PDF image_url artifact path. The host artifact pipeline processes image_url via saveBase64Image which fails for PDFs. PDFs now fall through to the generic binary handler ("Use bash to process"). TODO comment for future document artifact support. Also: isImageOrPdf → isImage in early size checks (PDFs are no longer treated as artifact candidates). * fix: remove dead PDF_MIME constant, hoist skillToolDeps, document session_id - #7: Remove unused PDF_MIME constant (dead code after PDF artifact removal) - #11: Hoist skillToolDeps to module-level constant (avoid per-call allocation) - #6: Document that CodeSessionContext.session_id is a representative value; ToolNode uses per-file session_id from the files array * fix: call toolEndCallback for skill/read_file artifacts + clear codeEnvIdentifier on re-upload Codex P1 (toolEndCallback bypass): skill and read_file handler branches returned early, bypassing the toolEndCallback that processes artifacts (image attachments). Now calls toolEndCallback when the result has an artifact, using the same metadata pattern as the normal tool.invoke path. Codex P1 (stale identifiers): upsertSkillFile now $unset's codeEnvIdentifier alongside content and isBinary when a file is re-uploaded. Prevents the freshness cache from returning references to old file content after a skill file is replaced. * fix: add session_id comment at cached path, rename skillResult to handlerResult * fix: return content_and_artifact from bash stub so result.content is populated * fix: deterministic skill lookup, dedup warning, and multi-session freshness check - getSkillByName: add sort({updatedAt:-1}) so name collisions resolve deterministically to the most recently updated skill - injectSkillCatalog: warn when multiple accessible skills share a name - primeSkillFiles: check ALL distinct sessions for freshness, not just the first file's session, preventing stale refs after partial bulkWrite * refactor: update icon import in Skills component - Replaced the Scroll icon with ScrollText in the Skills component for improved clarity and consistency in the UI. * fix: SKILL.md cache parity, gate bash_tool on code env, fix read_file too-large message - primeSkillFiles: filter SKILL.md from returned files array on fresh upload so cached and non-cached paths return identical file sets (SKILL.md is still on disk in the session for bash access) - injectSkillCatalog: only register bash_tool when codeEnvAvailable is true; thread the flag from all three CJS callers via execute_code capability check - handleReadFileCall: tell the model to invoke the skill first before suggesting /mnt/data paths for oversized files * fix: use EnvVar constant, deduplicate auth lookup, validate batch upload, stream byte limit - Replace hardcoded 'LIBRECHAT_CODE_API_KEY' with EnvVar.CODE_API_KEY in skillConfigurable.ts and skillFiles.ts - Resolve code API key once at run start in initialize.js and pass to both primeInvokedSkills and enrichWithSkillConfigurable via optional preResolvedCodeApiKey param, eliminating redundant loadAuthValues calls - Add response structure validation in batchUploadCodeEnvFiles before accessing session_id/files to surface unexpected responses early - Add streaming byte counter in handleReadFileCall that aborts and destroys the stream when accumulated bytes exceed MAX_BINARY_BYTES, preventing full file buffering when DB metadata is inaccurate * refactor: update icon import in ToolsDropdown component - Replaced the Scroll icon with ScrollText in the ToolsDropdown component for improved clarity and consistency in the UI. * fix: partial upload failure detection, EnvVar in initialize.js, declaration ordering - primeSkillFiles: return null (failure) when batch upload partially succeeds — missing bundled files would cause runtime bash/read failures with missing paths in code env - initialize.js: replace hardcoded 'LIBRECHAT_CODE_API_KEY' with EnvVar.CODE_API_KEY imported from @librechat/agents - initialize.js: move enabledCapabilities, accessibleSkillIds, and codeApiKey declarations before the toolExecuteOptions closure that references them (eliminates reliance on temporal dead zone hoisting)
danny-avila
added a commit
that referenced
this pull request
Apr 25, 2026
…efaults (#12692) * feat: per-user skill active/inactive toggle with ownership-aware defaults - Add `skillStates` map (Record<string, boolean>) to user schema for per-user active/inactive overrides on skills - Add `defaultActiveOnShare` to interface.skills config (default: false) so admins can control whether shared skills auto-activate - Add GET/POST /api/user/settings/skills/active endpoints with validation - Add React Query hooks with optimistic mutations for skill states - Add useSkillActiveState hook with ownership-aware resolution: owned skills default active, shared skills default inactive - Add toggle switch UI to SkillListItem and SkillDetail components - Filter inactive skills in injectSkillCatalog before agent injection - Add localization keys for active/inactive labels * fix: use Record instead of Map for IUser.skillStates Mongoose .lean() flattens Map to a plain object, causing type incompatibility with IUser in methods that return lean documents. * fix: address review findings for skill active states - Fail-closed when userId is absent: filter rejects all shared skills instead of passing them through unfiltered (Codex P1) - Validate Mongoose Map key characters (reject . and $) in controller to return 400 instead of a 500 from schema validation (Codex P2) - Block toggle while initial skill states query is loading to prevent overwriting server-side overrides with an empty snapshot (Codex P2) - Extract shared SkillToggle component, eliminating duplicate toggle markup in SkillListItem and SkillDetail (Finding #3) - Move skill state query/mutation hooks from Favorites.ts to Skills/queries.ts per feature-directory convention (Finding #4) - Fix hardcoded English aria-label in SkillListItem by passing the localized string from the parent SkillList (Finding #5) - Fix inline arrow in SkillList render loop: pass stable callback reference so SkillListItem memo() is not invalidated (Finding #1) - Extract toRecord() helper in controller to DRY the Map-to-Object conversion (Finding #6) - Remove Promise.resolve wrapping synchronous config read (Finding #8) - Remove unused TUpdateSkillStatesRequest type (Finding #12) * fix: forward tabIndex on SkillToggle to preserve list keyboard nav The original inline toggle had tabIndex={-1} so the row itself remained the sole tab target. The extraction into SkillToggle dropped this prop, making every list toggle a tab stop. Add an optional tabIndex prop and pass -1 from SkillListItem. * fix: plumb skillStates to all agent entry points, isolate toggle keydown - Add skillStates/defaultActiveOnShare loading to openai.js and responses.js controllers so shared-skill activation is respected across all agent entry points, not just initialize.js (Codex P1) - Stop keydown propagation on SkillToggle so Enter/Space does not bubble to the parent row's navigation handler (Codex P2) * fix: paginate catalog fetch and serialize toggle writes - Paginate listSkillsByAccess (up to 10 pages of 100) until the active catalog quota is filled, so inactive shared skills in recent positions do not starve active owned skills past the first page (Codex P1) - Extend listSkillsByAccess interface with cursor/has_more/after for catalog pagination - Serialize skill-state writes via a ref queue: one in-flight request at a time, with the latest desired state sent when the previous one settles. Prevents last-response-wins races where an older request overwrites newer toggles (Codex P2) * fix: share write queue across hook instances, block toggle on fetch error - Move the write queue from a per-instance useRef to a module-scoped object so every mount of useSkillActiveState (SkillList, SkillDetail, etc.) serializes against the same in-flight slot. Prior per-instance queues allowed two components to race full-map POSTs (Codex P1) - Extend the toggle guard beyond isLoading: also block when isError is true or data is undefined. Prevents a failed GET from seeding a toggle with an empty baseline that would wipe server-side overrides on the next successful POST (Codex P1) * fix: stale closure, orphan cleanup, and cap-error UX - Read toggle baseline from React Query cache via queryClient.getQueryData instead of the captured skillStates closure. The closure can be stale between onMutate's setQueryData and the next render, so rapid successive toggles would build on old state and drop earlier changes (Codex P1) - Surface the MAX_SKILL_STATES_EXCEEDED error code with a specific toast key (com_ui_skill_states_limit) so users understand the 200-cap rather than seeing a generic error - Prune orphaned entries (skillIds whose Skill doc no longer exists) on both GET and POST in SkillStatesController. Self-heals over time without needing cascade-delete hooks or a migration job. Uses one indexed Skill._id query per request * test: pin skill active-state precedence with unit tests Extract the active-state resolution logic from a closure inside injectSkillCatalog into an exported resolveSkillActive helper, then cover every branch of the precedence matrix: - Fails closed when userId is absent (even with defaultActiveOnShare=true) - Explicit override wins over ownership and config (both true and false) - Owned skills default to active when no override is set - Shared skills default to defaultActiveOnShare value - Undefined skillStates behaves identically to an empty object - defaultActiveOnShare defaults to false when omitted - Owned skills ignore defaultActiveOnShare entirely Closes Finding #2 from the pre-rebase comprehensive review. Mirrors the existing scopeSkillIds test style; injectSkillCatalog now calls resolveSkillActive instead of inlining the closure. * refactor: limit skill active toggle to detail header, drop label - Remove the per-row toggle from SkillListItem and the active-state plumbing (hook call, isSkillEnabled/onToggleEnabled/toggleAriaLabel props) from SkillList. The detail view is now the single place to change a skill's active state - Drop dim/muted styling for inactive skills in the sidebar: without a control there, the visual indication has nowhere to land - Resize SkillToggle to match neighbor buttons: outer h-9 container, h-6 w-11 track with size-5 knob, no label span. The 'Active' / 'Inactive' text that accompanied the detail-view toggle is removed - Remove the now-unused label prop and tabIndex prop (the tabIndex existed only for the list-row context) from SkillToggle. Drop the onKeyDown stopPropagation for the same reason - Remove now-orphaned com_ui_skill_active / com_ui_skill_inactive translation keys * style: shrink SkillToggle track to h-5 w-9 with size-4 knob Container stays at h-9 to match neighbor button heights. The toggle track itself drops from h-6 w-11 to h-5 w-9, with a size-4 knob travelling 1.125rem on activation. Visually lighter inside the row. * fix: remove redundant skillStates entries that match the resolved default When a toggle lands on the ownership/config default, delete the key from the map instead of persisting `{id: defaultValue}`. Without this, a user toggling a skill off and back on would leave `{id: true}` for an owned skill (whose default is already true), silently consuming a slot against the 200-entry cap. Repeated round-trip toggles could exhaust the quota with zero meaningful overrides (Codex P2). Preserves the exceptions-list invariant that the runtime-resolution design depends on. * fix: prune before enforcing skill-state cap; reject non-ObjectId keys Reorder the update controller so pruneOrphans runs before the 200-cap check. Without this, a user near the cap with some orphaned entries (skills deleted since their last GET) could send a payload that would pass after pruning but gets rejected by the raw-size check first. Add a sanity cap on raw payload size (2 * MAX_SKILL_STATES) so abusive inputs do not reach the DB query, and enforce the real cap on the pruned result instead. Harden pruneOrphans: the earlier early-return path could pass non-ObjectId keys through unchanged. Now only valid ObjectIds are returned, and the Skill-model-unavailable fallback filters by format. Also add isValidObjectIdString validation at the input boundary so malformed (but otherwise non-Mongo-unsafe) keys never reach persistence (Codex P2 x2). * fix: enforce active filter at execute time, prune revoked shares, scope queue per user P1: injectSkillCatalog now returns activeSkillIds (the filtered set that appears in the catalog). initializeAgent uses that set as the stored accessibleSkillIds on the initialized agent, so getSkillByName at runtime cannot resolve a deactivated skill — even if the LLM hallucinates a name or the user invokes by direct-invocation shorthand. Previously the executor authorized against the full ACL set, bypassing the active-state guarantee (Codex P1). P2: pruneOrphans now checks user access via findAccessibleResources in addition to skill existence. When a share is revoked, the user's skillStates entry for that skill had no cleanup path and silently consumed the 200-cap. Self-heals on both GET and POST. One extra ACL query per settings read/write; scoped to a single user so no N-user amplification (Codex P2). P2: the write queue moves from a single module-scoped object to a Map keyed by userId. Logout/login in the same tab can no longer flush the previous user's pending snapshot under the new session's auth. Each userId gets its own pending/inFlight slot; the in-flight request retains its original auth via the cookie already attached when sent, so the race window closes (Codex P2). * refactor: extract skillStates helpers to packages/api; add tests; polish Address the remaining valid findings from the comprehensive review: - Extract toRecord, loadSkillStates, validateSkillStatesPayload, and pruneOrphanSkillStates into packages/api/src/skills/skillStates.ts as TypeScript. The controller in /api shrinks to a ~90-line thin wrapper that builds live dependency adapters for Mongoose + the permission service (Review #2 DRY, #3 workspace boundary) - Replace the triplicated 12-line skillStates loading block in initialize.js, openai.js, and responses.js with a single call to loadSkillStates from @librechat/api. One helper, three sites - Swap console.error for the project logger in the controller (Review #7) - Remove the redundant INVALID_KEY_PATTERN regex: a valid ObjectId cannot contain . or $, so isValidObjectIdString already covers it (Review #11) - Parameterize the 200-cap error toast with {{0}} interpolation driven by the error response's `limit` field, so future changes to MAX_SKILL_STATES update the UI message automatically (Review #12) - Add 24 unit tests for the new skillStates helpers (toRecord, resolveDefaultActiveOnShare, loadSkillStates, validateSkillStates- Payload, pruneOrphanSkillStates) covering success paths, malformed input, cap boundaries, and parallel-query behavior (Review #4) - Add 10 tests for injectSkillCatalog pagination covering empty accessible set, missing listSkillsByAccess, single-page filter, owned-vs-shared defaults, explicit-override precedence, multi-page collection, MAX_CATALOG_PAGES safety cap, early termination on has_more=false, additional_instructions injection, and fail-closed without userId (Review #5) Total test count: 60 (was 26 on this surface). * fix: rename skillStates ValidationError to avoid barrel-export collision packages/api/src/types/error.ts already exports a ValidationError (MongooseError extension). Re-exporting a different shape from skills/skillStates.ts through the skills barrel caused TS2308 in CI because the root index re-exports both. Rename to SkillStatesValidationError to keep the exports disjoint. * refactor: tighten tests and absorb caller guard into loadSkillStates Address the followup review findings: - Add optional `accessibleSkillIds` param to loadSkillStates so the helper short-circuits to defaults when no skills are accessible. All three controllers drop the residual 7-line conditional wrapper in favor of a single destructured call (Review #2) - Remove the unreachable `typeof key !== 'string'` check from validateSkillStatesPayload: Object.entries always yields string keys per the JS spec (Review #3) - Replace the two `as unknown as` agent casts in the injectSkillCatalog tests with a `makeAgent()` factory typed directly as the function's parameter shape (Review #4) - Tighten the MAX_CATALOG_PAGES assertion from `toBeLessThanOrEqual(11)` to `toHaveBeenCalledTimes(10)` — the loop deterministically makes exactly 10 page fetches before hitting the cap (Review #1) - Rewrite the parallel-execution test for pruneOrphanSkillStates using deferred promises instead of microtask-order assertions. The test now inspects `toHaveBeenCalledTimes(1)` on both mocks after a single Promise.resolve() yield, pinning Promise.all usage without relying on push-order into a shared array (Review #5) - Evict stale writeQueue entries on user change via a module-scoped `lastSeenUserId` sentinel. When a different user's toggle is the first one after a logout/login, the previous user's queue entry is deleted. Keeps the Map bounded without adding hook-instance effect cleanup (Review #6) * fix(test): mock loadSkillStates in openai and responses controller specs The prior refactor replaced the inline 12-line skillStates loading block with a call to loadSkillStates from @librechat/api. Both controller spec files mock @librechat/api as a flat object, so any new named import from that package is undefined in the test env. Calling `await loadSkillStates(...)` threw before recordCollectedUsage ran, surfacing as "undefined is not iterable" on the test's array destructure of `mockRecordCollectedUsage.mock.calls[0]`. Add the missing mock to both spec files alongside the existing scopeSkillIds stub. * fix: abandon stale skillStates write queues on user switch Close the cross-session leak window where an in-flight flush loop still holds a reference to a previous user's queue: it could fire its next mutateAsync under the new session's auth cookies and persist the stale snapshot to the new user's document (Codex P1). Add an `abandoned` flag on `WriteQueue`. Three mechanisms cooperate: - `getWriteQueue` marks every non-active queue abandoned when the user differs from the last-seen identity (pre-existing eviction site, now more aggressive). - A `useEffect` on `userId` calls the same abandonment pass on every render with a new active identity, covering the window between logout/login and the new user's first toggle (when `getWriteQueue` would otherwise not fire). - The flush loop checks `!queue.abandoned` in its while condition so the second and later iterations exit without firing another `mutateAsync` after the session changes. The first iteration's in-flight request (already dispatched under the original user's cookies) still runs to completion or failure on its own — only the subsequent iterations, which are the dangerous ones, are blocked.
danny-avila
added a commit
that referenced
this pull request
Apr 25, 2026
…le` / `allowed-tools` (#12745) * 🧬 feat: Persist `disable-model-invocation` / `user-invocable` / `allowed-tools` Adds first-class columns mirroring the three runtime-enforced frontmatter fields, with a `deriveStructuredFrontmatterFields` helper that maps from frontmatter at create/update time and re-syncs (via `$unset`) when fields are removed. `listSkillsByAccess` projection includes them so the Phase 6 catalog filter and popover filter can both read off the summary row. Marks `invocationMode` as @deprecated on `TSkill` and the `InvocationMode` enum — the runtime now reads the persisted pair instead. * 🛡️ feat: Enforce frontmatter at runtime (catalog, skill tool, manual resolver, tool union) Wires the persisted columns into actual runtime behavior across all four invocation paths: - `injectSkillCatalog` excludes `disableModelInvocation: true` skills before catalog formatting — they cost zero context tokens and stay invisible to the model. - `handleSkillToolCall` rejects with a clear error when the model names a skill marked `disable-model-invocation: true` (defends against a stale-cache or hallucinated invocation getting past the catalog filter). - `resolveManualSkills` skips `userInvocable: false` skills with a warn log so an API-direct caller can't bypass the popover-side filter. - `unionPrimeAllowedTools` collects skill-declared `allowed-tools` minus what's already on the agent; `initialize.ts` re-runs `loadTools` for the extras and merges resulting `toolDefinitions` into the agent's effective set for the turn. Tool-name resolution is tolerant — unknown names silently drop with a debug log so cross-ecosystem skills referencing yet-to-be-implemented tools (Claude Code's `edit_file`, etc.) import without breaking. The agent document is never modified; the union is turn-scoped. Helper exports (`unionPrimeAllowedTools`) are structured so Phase 5's always-apply primes flow through the same union (combined `[...manualPrimes, ...alwaysApplyPrimes]`) once the resolver lands. Skill handler wire format gains the three fields so clients can render them on detail / list views. * 🎛️ feat: `$` popover reads `userInvocable` instead of UI-only `invocationMode` Replaces the phase-1 UI-only `invocationMode` check with the persisted `userInvocable` field (mirrors the `user-invocable` frontmatter). Skills authored with `user-invocable: false` no longer surface in the popover; the backend resolver enforces the same rule for defense-in-depth. Default-visible behavior is preserved: skills without an explicit `userInvocable` value (older rows, freshly imported skills that don't declare the field) stay visible — only an explicit `false` hides them. Test fixture updated to reflect the new field. * 🔧 fix: Address Phase 6 review findings Codex P2 + reviewer #1: Single `loadTools` call with the union of `agent.tools + allowed-tools`. The earlier two-call approach dropped `userMCPAuthMap` / `toolContextMap` / `actionsEnabled` from the skill-added pass — an MCP tool gained via `allowed-tools` would be visible to the model but fail at execution without per-user auth context. Resolution of `manualSkillPrimes` is hoisted before `loadTools` so the union can be computed up-front; the dropped-tools debug log now compares loaded vs. requested across the single call. Codex P3 + reviewer #2: `injectSkillCatalog.activeSkillIds` now includes `disable-model-invocation: true` skills. The runtime ACL check in `handleSkillToolCall` previously couldn't reach the explicit "cannot be invoked by the model" rejection because the broader access set excluded those skills. Catalog text and tool registration still gate on the visible subset (zero-context-token guarantee preserved); only the per-user `isActive` filter is a hard exclusion now. Reviewer #1 (try/catch around loadTools, MAJOR): A single bad `allowed-tools` entry from a shared skill could crash the entire turn. Now wrapped — on failure with extras, retry with just `agent.tools` and continue (the dropped-tools debug log surfaces what vanished). If the retry-without-extras still throws, propagate; the agent's own tools are the load-bearing surface. Reviewer #3 (integration tests, MAJOR): Added six tests in `initialize.test.ts` covering the full `allowed-tools` loading path: union pass-through, no-extras short-circuit, agent-baseline dedup, loadTools throw + retry, propagated throw without extras, and the empty-tools edge case. Smaller cleanups bundled in: - Reviewer #4: Moved `logger` import to the package-imports section (was wedged among local imports). - Reviewer #5: Removed unused index on `disableModelInvocation` (filtering happens application-side in `injectSkillCatalog`; index cost write overhead for zero query benefit). - Reviewer #6: Swapped order of `userInvocable` and body checks in `resolveManualSkills` so the more authoritative author-decision reason surfaces first when both apply. - Reviewer #8: Documented the `allowedTools` enforcement gap on the schema + type — model-invoked skills (mid-turn `skill` tool calls) do NOT trigger tool union, since adding tools after the graph starts would require a rebuild. Manual / always-apply (Phase 5) primes are the supported paths. - Reviewer #9: Renamed `dmi` / `ui` / `at` locals to `disableModelInvocationRaw` / `userInvocableRaw` / `allowedToolsRaw` in `deriveStructuredFrontmatterFields`. Reviewer #7 (DRY shared `getSkillByName` return type) deferred — field sets diverge meaningfully across the three call sites (handler needs `body + fileCount`; resolver needs `author + allowedTools + userInvocable`; the InitializeAgentDbMethods contract needs the superset). A `Pick<>`-based consolidation is a follow-up cleanup. * 🔧 fix: Address codex iter 2 — catalog quota + duplicate-name dedup P1: `injectSkillCatalog` cap now counts only model-visible skills, not the merged active set. The previous behavior let a tenant with many `disable-model-invocation: true` rows near the top of the cursor exhaust the 100-slot quota before any invocable skill got scanned — the catalog could end up empty even though invocable skills existed further down the paginated results. `MAX_CATALOG_PAGES` stays the ceiling on scan budget; only `visibleCount` drives the early-exit on quota fill. P2: When an invocable and a `disable-model-invocation: true` skill share a name, drop the disabled doc(s) from `activeSkillIds`. Without this dedup, `getSkillByName` (which sorts by `updatedAt` desc) could pick the disabled doc and every model call to the cataloged name would fail with "cannot be invoked by the model" instead of executing the visible skill. When ONLY a disabled doc exists for a name, it stays in `activeSkillIds` so the explicit-rejection error path still fires for hallucinated invocations. Tests: 3 new cases in `injectSkillCatalog` covering (a) cap counted on visible skills only, (b) same-name collision drops disabled doc, (c) sole-disabled-name case keeps the disabled doc. * 🔒 fix: Apply `disable-model-invocation` gate to read_file too (codex iter 3 P1) `activeSkillIds` is shared between the `skill` and `read_file` handlers. The skill-tool gate was applied last iteration, but `handleReadFileCall` authorized purely on `getSkillByName(..., accessibleIds)` — so a model that learned a hidden skill's name (stale catalog or hallucination) could still read its `SKILL.md` body or bundled files via `read_file`, defeating the contract. Same explicit rejection now fires from both handlers; no change needed to the ACL set itself (disabled docs stay in `activeSkillIds` so the explicit error path keeps firing). Two new tests in `handlers.spec.ts` cover the read_file gate and regression-protect the happy path. * 🔧 fix: Address codex iter 4 — manual-prime exception + legacy frontmatter backfill P1: Scope the `read_file` `disableModelInvocation` gate to AUTONOMOUS model probes only. A user-invoked `$` skill that is also marked `disable-model-invocation: true` had its bundled `references/*` / `scripts/*` files unreadable, leaving the manually-primed skill body referencing files the model couldn't load. Now the handler bypasses the gate when the skill name appears in `manualSkillNames` (the per-turn allowlist threaded from `manualSkillPrimes` → `agentToolContexts` → `enrichWithSkillConfigurable` → `mergedConfigurable`). Defense-in-depth: the bypass is scoped to the specific names in the allowlist; a different disabled skill name is still rejected. P2: Read-time fallback for legacy skills authored before Phase 6 landed the structured columns. `user-invocable: false` / `disable-model-invocation: true` set in `frontmatter` (the validator already accepted those keys) but with no derived column would incorrectly evaluate as "user-invocable / model-allowed" until a save backfilled the columns. New `backfillDerivedFromFrontmatter` helper fills undefined columns from frontmatter at read time in both `getSkillByName` and `listSkillsByAccess` — column wins when both are set, frontmatter fills the gap when only it's set. No DB writes; the next `updateSkill` naturally persists. `listSkillsByAccess` projection expanded to include `frontmatter` (bounded by validator, payload impact small) so summaries can also be backfilled. Sticky-primed disabled skills (ones invoked in prior turns of the same conversation) are not yet in the manual-prime allowlist — same- turn manual invocation is the load-bearing path codex flagged; the sticky-turn case is a known limitation tracked for a follow-up. Tests: 2 new in handlers.spec.ts (manual-prime allows + name-scoped block holds), 3 new in skill.spec.ts (legacy backfill via getSkillByName + listSkillsByAccess + column-wins precedence). * 🔧 fix: Address codex iter 5 — propagate manualSkillNames + keep read_file P1: `enrichWithSkillConfigurable` is also called from `openai.js` and `responses.js` (the OpenAI Responses + completions endpoints). Both were ignoring the new `manualSkillNames` parameter, which meant the manual-prime exception in the `read_file` gate (iter 4) only worked on the agents endpoint. Now all three call sites pass `primaryConfig.manualSkillPrimes?.map(p => p.name)` so manual `$` invocations of disabled skills work consistently across endpoints. P2: When every accessible skill is `disable-model-invocation: true`, the catalog text and `skill` tool are correctly omitted (no model- reachable targets) — but `read_file` and `bash_tool` MUST still be registered. A user manually invoking such a skill gets its SKILL.md body primed into context; if the body references `references/foo.md` or `scripts/run.sh`, those reads need a registered tool. Restructured `injectSkillCatalog` so `skill` registration is gated on `catalogVisibleSkills.length > 0` while `read_file` (always) and `bash_tool` (when codeEnvAvailable) register whenever any active skill is in scope. Tests: existing all-disabled test rewritten to assert read_file IS registered + skill is NOT; new test confirms bash_tool joins it when codeEnvAvailable. * 🔧 fix: Address codex iter 6 — name-collision consistency via preferInvocable P2a (resolveManualSkills): a name collision between an older user-invocable doc and a newer non-user-invocable doc made manual `$` invocation silently no-op. The popover surfaced the older invocable doc; resolver looked it up by name; `getSkillByName` returned the newer non-invocable doc; resolver skipped on `userInvocable: false`. P2b (handler / runtime ACL): with same-name duplicates (e.g. older invocable + newer disabled), the manual prime resolved to one doc while later `read_file` / `skill` execution resolved a different doc through `activeSkillIds`. Model could follow one SKILL.md body while reading files from a different skill. Both root-cause: `getSkillByName` always returned the newest match and let the caller filter, but with collisions the newest can be something the caller didn't want. Fix: extend `getSkillByName` with `options.preferInvocable`. When true, prefer the newest doc satisfying BOTH `userInvocable !== false` AND `disableModelInvocation !== true` (with frontmatter backfill); fall back to the newest match otherwise. Fast path preserved when caller doesn't opt in. Callers passing `preferInvocable: true`: - `resolveManualSkills` — picks the popover-visible invocable doc even when a newer disabled / non-user-invocable duplicate exists. - `handleSkillToolCall` — keeps execution aligned with the catalog; falls back to the disabled doc only when no invocable variant exists (so the explicit "cannot be invoked by the model" gate still fires for the hallucinated-disabled-name case). - `handleReadFileCall` — same alignment, plus the manual-prime exception added in iter 4 still applies. Tests: - 2 new in skill.spec.ts (preferInvocable picks invocable when collision exists; falls back to newest when no clean-invocable exists). - 1 new in skills.test.ts (resolver passes preferInvocable through). - 2 new in handlers.spec.ts (skill tool + read_file pass it). - Existing initialize.test.ts assertion updated for the new option. * 🔧 fix: Address codex iter 7 — split preferInvocable into per-axis flags The previous unified `preferInvocable` filter required BOTH `userInvocable !== false` AND `disableModelInvocation !== true`. That was wrong for the model paths: `userInvocable: false` skills are model-only and remain valid `skill` / `read_file` invocation targets. A duplicate-name scenario where the newer cataloged doc was model- only would let the older user-invocable variant shadow it on every model call. Split the option into two independent axes: - `preferUserInvocable` — for manual paths (`$` popover). Skips docs with `userInvocable: false`. Disable-model-invocation status is irrelevant; iter 4 explicitly supports manual prime of disabled skills. - `preferModelInvocable` — for model paths (`skill` / `read_file` handlers). Skips docs with `disableModelInvocation: true`. User- invocable status is irrelevant; model-only skills are valid here. Both flags fall back to the newest match when no preferred doc exists, so the explicit-rejection error paths still fire correctly in the sole-disabled-name case. Callers updated: - `resolveManualSkills` → `preferUserInvocable: true` - `handleSkillToolCall` / `handleReadFileCall` → `preferModelInvocable: true` Tests: - New spec test for preferModelInvocable not filtering on userInvocable. - Existing preferInvocable test renamed/split to cover the new axes. - New test asserts preferUserInvocable still returns disabled docs (preserves iter 4 manual-disabled support). - Caller tests assert each path passes the right single flag and does NOT pass the wrong one. * 🔧 fix: TypeScript type-check failure in handlers.spec.ts (CI green) `jest.fn(async () => ...)` without explicit args infers an empty tuple for the call signature, so `mock.calls[0][2]` flagged as "Tuple type '[]' has no element at index '2'." Cast to `unknown[]` then narrow to the expected option shape. Behavior unchanged. Caught by the `Type check @librechat/api` CI step (.github/workflows/backend-review.yml). * 🔧 fix: Address codex iter 8 — undefined-result fallback + read_file alignment P1 (loadTools returning undefined): Production loaders (`createToolLoader` in `initialize.js` / `openai.js` / `responses.js`) wrap `loadAgentTools` in try/catch and return `undefined` on failure rather than throwing. Without explicit handling, my iter-1 try/catch only fired for thrown errors — a silent-failure on a skill-added tool would fall through to the empty fallback and silently DROP the agent's baseline tools for the turn (much worse than just losing the extras). Added an `undefined`-result branch that retries with just `agent.tools`, mirroring the throw branch. Test pins both behaviors. P2 (read_file alignment with manual prime): When a skill is in this turn's `manualSkillNames`, the `read_file` handler now uses `preferUserInvocable` instead of `preferModelInvocable`. Same name-collision rule as `resolveManualSkills`, so the doc whose files get read is the same doc whose body got primed. For autonomous probes (skill not in `manualSkillNames`), the handler keeps `preferModelInvocable` to align with the catalog the model saw. Two new tests cover both branches and regression-protect that the wrong flag isn't passed. * 🔧 fix: Address codex iter 9 — pin read_file lookup to primed skill _id P1 (manually-primed disabled IDs were dropped from activeSkillIds): The `executableSkills` dedup in `injectSkillCatalog` correctly drops `disable-model-invocation: true` duplicates when an invocable doc shares the name — but `resolveManualSkills` legitimately primes disabled docs (iter 4 supports manual `$` invocation of disabled skills). When the resolver primed a disabled doc, the read_file handler couldn't find it in the (deduped) `activeSkillIds` and either resolved a different same-name skill or returned not-found. Fix: `ResolvedManualSkill` now carries `_id`; the legacy `initialize.js` / `openai.js` / `responses.js` controllers build a `manualSkillPrimedIdsByName` map and `enrichWithSkillConfigurable` passes it into `mergedConfigurable`. `handleReadFileCall` now pins its lookup's `accessibleIds` to `[primedId]` whenever the requested skill is in the map. The constrained set guarantees the lookup returns the EXACT doc the resolver primed — body/files come from the same source even when same-name duplicates exist or the dedup removed the prime's id from `activeSkillIds`. Autonomous read_file probes (skill not in the manual-primed map) keep the full ACL set + `preferModelInvocable` so they align with the catalog the model saw and the disabled-only case still fires the explicit-rejection gate. Test fixture changes flow from `_id` becoming required on `ResolvedManualSkill`. `buildSkillPrimeContentParts` / `injectManualSkillPrimes` widen their param types to `Pick<...>` because they only read `name` / `body` and shouldn't force test literals to invent placeholder ids. * 🧹 fix: Address independent reviewer findings (DRY + types + tests + docs) Sanity-pass review surfaced 7 findings; addressed 6 (the 7th — DRY on inline `getSkillByName` return types — is acknowledged tech debt deferred to a follow-up). #1 [MAJOR, DRY]: The 4-line `manualSkillPrimedIdsByName` map construction was duplicated across 4 CJS call sites (openai.js, responses.js x2, initialize.js). Extracted `buildManualSkillPrimedIdsByName` helper in `skillDeps.js`; all four sites now call the helper. If `ResolvedManualSkill` ever renames `_id` or gains identifying fields, only the helper changes. #2 [MINOR, type safety]: `handleReadFileCall` was casting a hex string to `Types.ObjectId[]` via `as unknown as`, relying on mongoose's auto-cast in `$in` queries. Replaced with `new Types.ObjectId(...)` so any future consumer comparing with `.equals()` / `===` gets the correct value type. Imported `Types` as a value (was type-only). #5 [MINOR, test gap]: Added a test for the worst-case silent-failure path — both the union and base-only `loadTools` calls return undefined. The agent gets no tools but the turn doesn't crash hard; pinning that contract. #4 [MINOR, performance]: Added a TODO on the `listSkillsByAccess` projection noting the `frontmatter` field can be dropped once a write migration backfills all pre-Phase-6 skills' columns. ~2KB/skill × 100/page is wasted bandwidth post-backfill. #6 [NIT, docs]: `backfillDerivedFromFrontmatter` JSDoc said "Pure" right before "mutates its undefined fields in place". Replaced with "Side-effect-free w.r.t. the DB (no writes), but mutates its argument in place" which describes both halves accurately. #7 [NIT, test determinism]: Replaced `await new Promise(r => setTimeout(r, 5))` in two same-name collision tests with explicit `updateOne` setting `updatedAt: new Date(Date.now() - 1000)` on the older doc. Removes the wall-clock race on fast CI runners. The pagination test (line 480) still uses setTimeout — that test is pre-existing and order is incidental, not load-bearing. Existing test fixtures updated to use valid 24-char hex ObjectIds (required by the iter-9 test that constructs a real `ObjectId`). #3 [MINOR, deferred]: Inline `getSkillByName` return type duplicated across `handlers.ts`, `initialize.ts`, `skills.ts`. Reviewer acknowledged this as deferred; field sets diverge across call sites (handler needs `fileCount`, resolver needs `author`/`allowedTools`). A `Pick<>`-based consolidation is a clean follow-up.
danny-avila
added a commit
that referenced
this pull request
Apr 25, 2026
* 🛠️ feat: Add registerCodeExecutionTools helper
Idempotently registers `bash_tool` + `read_file` in the run's tool
registry and tool-definition list via a registry `.has()` dedupe. Sets
up the single code-execution tool path shared by:
- `initializeAgent` (when an agent has `execute_code` in its tools and
the capability is enabled for the run)
- `injectSkillCatalog` (when skills are active; unconditional read_file,
bash_tool follows `codeEnvAvailable`)
Both callers reach the helper in the same initialization sequence, so
the second call becomes a no-op and exactly one copy of each tool
reaches the LLM — no more double registration for agents that combine
`execute_code` capability with active skills.
Unit-tested on a fresh run, idempotence (second call, overlap with
prior tooldefs, partial overlap), and the no-registry variant.
* 🔀 refactor: Route injectSkillCatalog bash_tool + read_file through registerCodeExecutionTools
The `skill` tool is still registered inline (it's skill-path-specific),
but `bash_tool` + `read_file` now flow through the shared idempotent
helper so a prior registration from the execute_code path doesn't
produce a duplicate copy later in the same run. Behavior preserved:
- `read_file` always registers when any active skill is in scope —
manually-primed `disable-model-invocation: true` skills still need it
to load `references/*` from storage.
- `bash_tool` follows `codeEnvAvailable` exactly as before.
Adds a test pinning the cross-call dedupe: when `injectSkillCatalog`
runs AFTER `registerCodeExecutionTools` has already seeded the registry
+ tool definitions with bash_tool/read_file, the resulting
`toolDefinitions` still contains exactly one copy of each.
* 🪄 feat: Expand `execute_code` tool name into bash_tool + read_file at initialize-time
When an agent's `tools` include `execute_code` and the `execute_code`
capability is enabled for the run, `initializeAgent` now registers
`bash_tool` + `read_file` via `registerCodeExecutionTools` before
`injectSkillCatalog`. The legacy `execute_code` tool definition is no
longer handed to the LLM — `execute_code` remains on the agent
document as a capability-trigger marker, but the runtime expands it
into the skill-flavored tool pair.
Call ordering matters: the `execute_code` registration runs BEFORE
`injectSkillCatalog`, so the skill path's own `registerCodeExecutionTools`
call inside `injectSkillCatalog` becomes a no-op via the registry's
`.has()` check. Exactly one copy of each tool reaches the LLM whether
the agent has:
- only `execute_code` (legacy path)
- only skills
- both
No data migration needed — `agent.tools: ['execute_code']` stays in
the DB unchanged; the expansion is a runtime operation.
Three tests cover the matrix: execute_code + capability on →
bash_tool + read_file registered; execute_code + capability off →
neither registered; no execute_code + capability on → neither
registered.
* 🗑️ refactor: Drop CodeExecutionToolDefinition from the builtin registry
Removes the legacy `execute_code` entry from `agentToolDefinitions` and
the corresponding import. With the initialize-time expansion in place,
nothing consults `getToolDefinition('execute_code')` for a tool schema
any more — the capability gate still filters on the string
`execute_code`, but the actual tool definitions the LLM sees come from
`registerCodeExecutionTools` (i.e. `bash_tool` + `read_file`).
`loadToolDefinitions` in `packages/api/src/tools/definitions.ts`
silently drops `execute_code` when it no longer resolves in the
registry — that's the expected path and is now covered by an updated
test. No caller of `getToolDefinition('execute_code')` expects a
non-undefined result after this change.
* 🔌 refactor: Read CODE_API_KEY from env for primeCodeFiles + PTC
Finishes the Phase 4 server-env-keyed rollout on the two remaining
`loadAuthValues({ authFields: [EnvVar.CODE_API_KEY] })` sites in
`ToolService.js`:
- `primeCodeFiles` (user-attached file priming on execute_code agents)
- Programmatic Tool Calling (`createProgrammaticToolCallingTool`)
Both now read `process.env[EnvVar.CODE_API_KEY]` directly, matching
`bash_tool`'s pattern. The per-user plugin-auth path is no longer
consulted for code-env credentials anywhere in the hot path — the
agents library owns the actual tool-call execution and also reads the
env var internally.
Priming still fires for existing user-file workflows so the legacy
`toolContextMap[execute_code]` hint ("files available at /mnt/data/...")
stays in the prompt; only the key lookup changed.
* 🔧 fix: Type the pre-seeded dedupe-test tools as LCTool
CI TypeScript type checks caught `{ parameters: {} }` in the new
cross-call dedupe test: `LCTool.parameters` is a `JsonSchemaType`,
not `{}`. Use `{ type: 'object', properties: {} }` and type the
local registry Map through the parameter-derived shape so the
pre-seeded values match what `toolRegistry.set` expects.
* 🛡️ fix: Run execute_code expansion before GOOGLE_TOOL_CONFLICT gate
Codex review caught a latent regression: the original Phase 8 placement
ran `registerCodeExecutionTools` after `hasAgentTools` was computed,
so an execute-code-only agent on Google/Vertex with provider-specific
`options.tools` populated would no longer trip `GOOGLE_TOOL_CONFLICT`
— the legacy `CodeExecutionToolDefinition` used to populate
`toolDefinitions` before the guard, but after dropping it from the
registry, `toolDefinitions` stayed empty until my expansion ran
downstream of the guard. Mixed provider + agent tools would silently
flow through to the LLM.
Fix moves the `execute_code` expansion to BEFORE `hasAgentTools`
computation. `bash_tool` + `read_file` now contribute to the check
the same way the legacy `execute_code` def did. Covered by a new
test that pins the Google+execute_code+provider-tools scenario —
the `rejects.toThrow(/google_tool_conflict/)` path would have
silently passed on the prior placement.
* 🔗 fix: Thread codeEnvAvailable through handoff sub-agents
Round-2 codex review caught the other half of the execute_code
expansion gap: `discoverConnectedAgents` omitted `codeEnvAvailable`
from its forwarded `initializeAgent` params, so handoff sub-agents
with `agent.tools: ['execute_code']` lost the `bash_tool` + `read_file`
registration (pre-Phase 8 the legacy `CodeExecutionToolDefinition`
would have landed in their `toolDefinitions` via the registry).
- Add `codeEnvAvailable?` to `DiscoverConnectedAgentsParams` and
forward it verbatim on every sub-agent `initializeAgent` call.
- Update the three JS call sites that construct the primary's
`codeEnvAvailable` (`services/Endpoints/agents/initialize.js`,
`controllers/agents/openai.js`, `controllers/agents/responses.js`)
to pass the same flag into `discoverConnectedAgents` — one
authoritative source per request.
- Two regression tests in `discovery.spec.ts` pin the true/false
passthrough so a future refactor that drops the param-forwarding
surfaces immediately.
Left intentionally unchanged: `packages/api/src/agents/openai/service.ts`
(public API helper with no in-repo caller). External consumers of
`createAgentChatCompletion` who want code execution should pass a
`codeEnvAvailable`-aware `initializeAgent` via `deps` — documenting
the full public-API surface is out of scope for this Phase 8 PR.
* 🔗 fix: Thread codeEnvAvailable through addedConvo + memory-agent paths
Round-3 codex review caught the last two production `initializeAgent`
callers missing the Phase-8 capability flag:
- `api/server/services/Endpoints/agents/addedConvo.js` (multi-convo
parallel agent execution). Added `codeEnvAvailable` to
`processAddedConvo`'s destructured params and forwarded it into
the per-added-agent `initializeAgent` call. Caller in
`api/server/services/Endpoints/agents/initialize.js` passes the
same `codeEnvAvailable` it computed for the primary.
- `api/server/controllers/agents/client.js` (`useMemory` — memory
extraction agent). Computes its own `codeEnvAvailable` from
`appConfig?.endpoints?.[EModelEndpoint.agents]?.capabilities` and
forwards into `initializeAgent`. Memory agents rarely list
`execute_code`, but if one does, pre-Phase 8 they got the legacy
`execute_code` tool registered unconditionally — the passthrough
restores parity.
With this, every production caller of `initializeAgent` explicitly
resolves the capability: main chat flow (primary + handoff), OpenAI
chat completions (primary + handoff), Responses API (primary + handoff),
added convo parallel agents, and memory agents. The one remaining
caller, `packages/api/src/agents/openai/service.ts::createAgentChatCompletion`,
is a public API helper with no in-repo consumer (external callers
must pass a capability-aware `initializeAgent` via `deps`).
* 🪤 fix: Remove duplicate appConfig declaration causing TDZ ReferenceError
The Responses API controller had TWO `const appConfig = req.config;`
bindings inside `createResponse`: one at the top of the function
(added by the Phase 4 `bash_tool` decouple) and one inside the try
block (added by the polish PR #12760). Because `const` is block-scoped
with a temporal dead zone, the inner redeclaration put `appConfig` in
TDZ for the entire try block, so any earlier reference inside the
try — notably `appConfig?.endpoints?.[EModelEndpoint.agents]?.allowedProviders`
at line 348 — threw `ReferenceError: Cannot access 'appConfig'
before initialization`. The error was silently swallowed by the
outer try/catch, leaving `recordCollectedUsage` unreached and the
six `responses.unit.spec.js` token-usage tests failing.
Removing the inner redeclaration fixes the six failing tests
(verified: 11/11 pass locally post-fix, 0 regressions elsewhere).
The outer function-scoped binding already provides `appConfig` to
every downstream reference.
* 🔗 fix: Thread codeEnvAvailable through the OpenAI chat-completion public API
Round-4 codex review (legitimate on the type-safety angle, even though the
runtime concern was already covered): the `createAgentChatCompletion`
helper defines its own narrower `InitializeAgentParams` interface locally,
and the type was missing `codeEnvAvailable`. External consumers who
supply a capability-aware `deps.initializeAgent` couldn't route
`codeEnvAvailable` through without a type-cast workaround.
- Widen the local `InitializeAgentParams` interface to include
`codeEnvAvailable?: boolean` (matches the real
`packages/api/src/agents/initialize.ts` type).
- Derive `codeEnvAvailable` inside `createAgentChatCompletion` from
`deps.appConfig?.endpoints?.agents?.capabilities` (the same source
the in-repo controllers use) and forward to `deps.initializeAgent`.
Uses a string literal `'execute_code'` lookup so this file stays free
of a `librechat-data-provider` import — keeping the dependency surface
of the public helper minimal.
With this, external consumers of `createAgentChatCompletion` who pass
`appConfig` with the agents capabilities get `bash_tool` + `read_file`
registration automatically; consumers who don't pass `appConfig` retain
the existing "explicit opt-in" semantics (the flag stays `undefined`,
expansion is skipped).
* 🧹 chore: Review-driven polish — observability log, JSDoc DRY, test gaps, no-op allocation
Addresses the comprehensive review of PR #12767:
- **Finding #1** (MINOR, observability): `initializeAgent` now emits a
debug log when an agent lists `execute_code` in its tools but the
runtime gate is off (`params.codeEnvAvailable` !== true). The
event-driven `loadToolDefinitionsWrapper` path doesn't log
capability-disabled warnings, so without this the tool silently
vanishes from the LLM's definitions with zero trace. Operators
debugging "why isn't code interpreter working?" now get a signal at
the initialize layer.
- **Finding #5** (NIT, allocation): `registerCodeExecutionTools` now
returns the input `toolDefinitions` array by reference on the no-op
path (both tools already registered by a prior caller in the same
run) instead of allocating a fresh spread array every time. The
common dual-call scenario — `initializeAgent` then
`injectSkillCatalog` — saves one O(n) copy per request.
- **Finding #4** (NIT, DRY): Collapsed the duplicated 6-line JSDoc
comment in `openai.js`, `responses.js`, and `addedConvo.js` into
either a one-line `@see DiscoverConnectedAgentsParams.codeEnvAvailable`
pointer (the two JS call sites) or a compact 3-line block referring
back to the canonical source (addedConvo's @PARAM).
- **Finding #2** (MINOR, test gap): Added
`api/server/services/Endpoints/agents/addedConvo.spec.js` with three
cases covering `codeEnvAvailable=true`, `codeEnvAvailable=false`,
and omitted (undefined) passthrough. A future refactor that drops
the param from destructuring now surfaces here instead of silently
regressing multi-convo parallel agents with `execute_code`.
- **Finding #3** (MINOR, test gap): Added
`api/server/controllers/agents/__tests__/client.memory.spec.js`
pinning the capability-flag derivation that `AgentClient::useMemory`
uses — six cases covering present/absent/null/undefined config shapes
plus an enum-literal pin (`'execute_code'` / `'agents'`). Catches
enum renames or config-path shifts that would otherwise silently
strip `bash_tool` + `read_file` from memory agents.
Finding #7 (jest.mock scoping, confidence 40) left as-is: the
reviewer's own risk assessment noted `buildToolSet` doesn't touch
the mocked exports, and restructuring a file-level `jest.mock` to
`jest.doMock` + dynamic `import()` introduces more complexity than
the speculative risk justifies. The existing mock is scoped to the
test file and contains the same stubs the adjacent
`skills.test.ts` already uses.
Finding #6 (PR description commit count) addressed out-of-band via
PR description update.
All existing tests pass, typecheck clean, lint clean across touched
files. New tests: 9 cases across 2 new spec files.
* 🧽 refactor: Replace hardcoded 'execute_code' string with AgentCapabilities enum in service.ts
Follow-up review (conf 55) caught that `openai/service.ts`'s Phase 8
`codeEnvAvailable` derivation used the literal `'execute_code'` while
every in-repo controller uses `AgentCapabilities.execute_code` from
`librechat-data-provider`. The file deliberately uses local type
interfaces to keep the public API helper's type surface small, but
that pattern was never a ban on single-value imports from the data
provider — `packages/api` already depends on it. Importing the enum
value means a future rename of `AgentCapabilities.execute_code`
propagates to this file automatically, matching the in-repo
controllers' behavior.
Other follow-up findings left as-is per the reviewer's own verdict:
- #2 (memory spec mirrors the production expression rather than
calling `AgentClient::useMemory` directly): reviewer flagged as
"not blocking" / "design-philosophy observation." The test file's
JSDoc already explicitly documents the tradeoff and pins the enum
literals to catch the most likely drift vector. Standing up
`AgentClient` + all its mocks for a one-line regression guard is
disproportionate.
- #3 (`addedConvo.spec.js` mock signature vs. underlying
`loadAddedAgent` arity): reviewer's own confidence 25 noted the
mock matches the wrapper's actual call pattern in the production
file. Not a real gap.
- #4 was self-retracted as a false alarm.
* 🗑️ refactor: Fully deprecate CODE_API_KEY — remove all LibreChat-side references
The code-execution sandbox no longer authenticates via a per-run
`CODE_API_KEY` (frontend or backend). Auth moved server-side into the
agents library / sandbox service, so LibreChat drops every reference:
**Backend plumbing:**
- `api/server/services/Files/Code/crud.js`: `getCodeOutputDownloadStream`,
`uploadCodeEnvFile`, `batchUploadCodeEnvFiles` no longer accept
`apiKey` or send the `X-API-Key` header.
- `api/server/services/Files/Code/process.js`: `processCodeOutput`,
`getSessionInfo`, `primeFiles` drop the `apiKey` param throughout.
- `api/server/services/ToolService.js`: stop reading
`process.env[EnvVar.CODE_API_KEY]` for `primeCodeFiles` and PTC; the
agents library handles auth internally. Remove the now-dead
`loadAuthValues` + `EnvVar` imports. Drop the misleading
"LIBRECHAT_CODE_API_KEY" hint from the bash_tool error log.
- `api/server/services/Files/process.js`: remove the `loadAuthValues`
call around `uploadCodeEnvFile`.
- `api/server/routes/files/files.js`: code-env file download no longer
fetches a per-user key.
- `api/server/controllers/tools.js`: `execute_code` is no longer a
tool that needs verifyToolAuth with `[EnvVar.CODE_API_KEY]` — the
endpoint always reports system-authenticated so the client skips
the key-entry dialog. `processCodeOutput` called without `apiKey`.
- `api/server/controllers/agents/callbacks.js`: `processCodeOutput`
invoked without the loadAuthValues round trip, for both LegacyHandler
and Responses-API handlers.
- `api/app/clients/tools/util/handleTools.js`: `createCodeExecutionTool`
called with just `user_id` + files.
**packages/api:**
- `packages/api/src/agents/skillFiles.ts`: `PrimeSkillFilesParams`,
`PrimeInvokedSkillsDeps`, `primeSkillFiles`, `primeInvokedSkills` all
drop the `apiKey` param; the gate is purely `codeEnvAvailable`.
- `packages/api/src/agents/handlers.ts`: `handleSkillToolCall` drops
the `process.env[EnvVar.CODE_API_KEY]` read; skill-file priming is
now gated solely on `codeEnvAvailable`. `ToolExecuteOptions`
signatures drop apiKey from `batchUploadCodeEnvFiles` and
`getSessionInfo`.
- `packages/api/src/agents/skillConfigurable.ts`: JSDoc no longer
references the env var.
- `packages/api/src/tools/classification.ts`: PTC creation no longer
gated on `loadAuthValues`; `buildToolClassification` drops the
`loadAuthValues` dep entirely (no LibreChat-side callers need it for
this path anymore).
- `packages/api/src/tools/definitions.ts`: `LoadToolDefinitionsDeps`
drops the `loadAuthValues` field.
**Frontend:**
- Delete `client/src/hooks/Plugins/useAuthCodeTool.ts`,
`useCodeApiKeyForm.ts`, and
`client/src/components/SidePanel/Agents/Code/ApiKeyDialog.tsx` —
the install/revoke dialogs for CODE_API_KEY are fully dead.
- `BadgeRowContext.tsx`: drop `codeApiKeyForm` from the context type and
provider. `codeInterpreter` toggle treated as always authenticated
(sandbox auth is server-side).
- `ToolsDropdown.tsx`, `ToolDialogs.tsx`, `CodeInterpreter.tsx`,
`RunCode.tsx`, `SidePanel/Agents/Code/Action.tsx` +`Form.tsx`: all
API-key dialog trigger refs, "Configure code interpreter" gear
buttons, and auth-verification plumbing removed. The
"Code Interpreter" toggle is now a plain `AgentCapabilities.execute_code`
checkbox — no key-entry gate.
- `client/src/locales/en/translation.json`: drop the three
`com_ui_librechat_code_api*` keys and `com_ui_add_code_interpreter_api_key`.
Other locales are externally automated per CLAUDE.md.
**Config:**
- `.env.example`: remove the `# LIBRECHAT_CODE_API_KEY=your-key` section
and its header.
**Tests:**
- `crud.spec.js`: assertions flipped to pin "no X-API-Key header" and
"no apiKey param".
- `skillFiles.spec.ts`: removed env-var save/restore; tests now pin
that the batch-upload path is gated solely on `codeEnvAvailable` and
that no apiKey is threaded through.
- `handlers.spec.ts`: same — just the `codeEnvAvailable` gate pins
remain.
- `classification.spec.ts`: remove the two tests that asserted
`loadAuthValues` was (not) called for PTC.
- `definitions.spec.ts`: drop every `loadAuthValues: mockLoadAuthValues`
entry from the deps shape.
- `process.spec.js`: strip the mock of `EnvVar.CODE_API_KEY`.
**Comment hygiene:**
- `tools.ts`, `initialize.ts`, `registry/definitions.ts`: shortened
stale comment references to "legacy `execute_code` tool" without
naming the retired env var.
Tests verified: 678 packages/api tests pass, 836 backend api tests
pass. Typecheck clean, lint clean. Only remaining CODE_API_KEY
mentions in the code are two regression-guard assertions:
- `crud.spec.js`: pins "no X-API-Key header" stays absent.
- `skillConfigurable.spec.ts`: pins `configurable` never grows a
`codeApiKey` field.
* 🧹 chore: Remove the last two CODE_API_KEY name mentions in LibreChat
Follow-up to the prior full deprecation commit: two tests still named
the retired identifier in their regression-guard assertions.
- `packages/api/src/agents/skillConfigurable.spec.ts`: drop the
"does not inject a codeApiKey key" test. The `codeApiKey` field is
gone from the production configurable shape, so an absence-assertion
naming it re-introduces the retired identifier in code.
- `api/server/services/Files/Code/crud.spec.js`: rename the
"without an X-API-Key header" case back to "should request stream
response from the correct URL" and drop the
`expect(headers).not.toHaveProperty('X-API-Key')` assertion. The
surrounding request-shape checks (URL, timeout, responseType) still
pin the behavior; the explicit header-absence line was named-after
the deprecated contract.
Result: `grep -rn "CODE_API_KEY\|codeApiKey\|LIBRECHAT_CODE_API_KEY"`
against the LibreChat source tree returns zero hits. The only
remaining `X-API-Key` strings in this repo are on unrelated OpenAPI
Action + MCP server auth configurations, where the string is
user-facing config, not a LibreChat-owned identifier.
Tests: 677 packages/api pass (2 pre-existing summarization e2e failures
unrelated); 126 api-workspace controller/service tests pass.
Typecheck and lint clean.
* 🎯 fix: Narrow codeEnvAvailable to per-agent (admin cap AND agent.tools)
Before this commit, `codeEnvAvailable` was computed in the three JS
controllers as the admin-level capability flag only
(`enabledCapabilities.has(AgentCapabilities.execute_code)`) and passed
through `initializeAgent` → `injectSkillCatalog` / `primeInvokedSkills` /
`enrichWithSkillConfigurable` unchanged. A skills-only agent whose
`tools` array didn't include `execute_code` still got `bash_tool`
registered (via `injectSkillCatalog`) and skill files re-primed to the
sandbox on every turn — wrong, because the agent never opted in to
code execution.
**Fix:** `initializeAgent` now computes the per-agent effective value
once as `params.codeEnvAvailable === true && agent.tools.includes(Tools.execute_code)`,
reuses the same boolean for:
1. The `execute_code` → `bash_tool + read_file` expansion gate
(previously already consulted `agent.tools`; now shares the single
`effectiveCodeEnvAvailable` binding).
2. The `injectSkillCatalog` call (previously got the raw admin flag).
3. The returned `InitializedAgent.codeEnvAvailable` field (new, typed as
required boolean).
**Controllers (initialize.js, openai.js, responses.js):** store
`primaryConfig.codeEnvAvailable` in `agentToolContexts.set(primaryId, ...)`,
capture `config.codeEnvAvailable` in every handoff `onAgentInitialized`
callback, and read it from the per-agent ctx inside the
`toolExecuteOptions.loadTools` runtime closure. The hoisted
`const codeEnvAvailable = enabledCapabilities.has(...)` locals in the
two OpenAI-compat controllers are gone — they were shadowing the
narrowed per-agent value.
**primeInvokedSkills:** `handlePrimeInvokedSkills` in
`services/Endpoints/agents/initialize.js` now uses
`primaryConfig.codeEnvAvailable` (per-agent, narrowed) instead of the
raw admin flag. A skills-only primary agent won't re-prime historical
skill files to the sandbox even when the admin enabled the capability
globally.
**Efficiency:** one extra `&&` in `initializeAgent`. No runtime hot-path
cost — the `includes()` scan on `agent.tools` was already happening for
the `execute_code` expansion gate; it's now just bound to a local. Tool
execution closures read `ctx.codeEnvAvailable === true` (property
access + strict equality, O(1)).
**Ephemeral-agent note:** per-agent narrowing is authoritative for both
persisted and ephemeral flows. The ephemeral toggle
(`ephemeralAgent.execute_code`) is reconciled into `agent.tools`
upstream in `packages/api/src/agents/added.ts`, so
`agent.tools.includes('execute_code')` is the single source of truth
by the time `initializeAgent` runs.
**Tests:** two new regression tests pin the narrowing contract:
- `initialize.test.ts` — four-quadrant matrix on
`InitializedAgent.codeEnvAvailable` (cap on × agent asks, cap on ×
doesn't ask, cap off × asks, neither). Catches future refactors that
drop either half of the AND.
- `skills.test.ts` — `injectSkillCatalog` with `codeEnvAvailable: false`
against an active skill catalog must NOT register `bash_tool` even
though it still registers `read_file` + `skill`. This is the state
a skills-only agent gets post-narrowing.
All 191 affected packages/api tests pass + 836 backend api tests pass.
Typecheck clean, lint clean.
* 🧽 refactor: Comprehensive-review polish — hoist tool defs, pin verifyToolAuth contract, doc appConfig
Addresses the comprehensive review of Phase 8. Findings mapped:
**#1 (MINOR): `verifyToolAuth` unconditional auth for execute_code**
- Added doc comment explicitly stating the deployment contract
(admin capability → reachable sandbox; no per-check health probe
to keep UI-gate queries O(1)).
- New `api/server/controllers/__tests__/tools.verifyToolAuth.spec.js`
with 4 regression tests pinning the contract:
1. `authenticated: true` + `SYSTEM_DEFINED` for execute_code.
2. 404 for unknown tool IDs.
3. `loadAuthValues` is never consulted (catches a future revert
that would resurface the per-user key-entry dialog).
4. Response `message` is never `USER_PROVIDED`.
**#2 (MINOR): `openai/service.ts` undocumented `appConfig` dependency**
- Expanded the `ChatCompletionDependencies.appConfig` JSDoc to spell
out that omitting it silently disables code execution for agents
with `execute_code` in their tools. External consumers of
`createAgentChatCompletion` now have the contract documented at
the type boundary.
**#5 (NIT): `registerCodeExecutionTools` re-allocates tool defs**
- Hoisted `READ_FILE_DEF` and `BASH_TOOL_DEF` to module-level
`Object.freeze`d constants. The shapes derive entirely from
static `@librechat/agents` exports, so a single frozen object per
tool is safe to share across every agent init. Eliminates the
~4-property allocations on every call (including the common
second-call no-op path).
**#6 (NIT): Verbose history-priming comment in initialize.js**
- Trimmed the 16-line `handlePrimeInvokedSkills` block to a 5-line
summary with `@see InitializedAgent.codeEnvAvailable` pointer.
The canonical narrowing explanation lives on the type; the
controller comment is just the ACL-vs-capability rationale.
**Skipped:**
- #3 (memory spec tests a mirror function): reviewer self-dismissed
as a design tradeoff; the enum-literal pin already catches the
highest-risk drift vector.
- #4 (cross-repo contract for `createCodeExecutionTool`): user will
explicitly install the latest `@librechat/agents` dev version
once the companion PR publishes, so the version pin will be
authoritative.
- #7 (migration/deprecation note for self-hosters): out of scope
per user direction — release notes handle this.
Tests verified: 679 packages/api + 840 backend api tests pass.
Typecheck + lint clean.
* 🔧 chore: Update @librechat/agents version to 3.1.68-dev.1 across package-lock and package.json files
This commit updates the version of the `@librechat/agents` package from `3.1.68-dev.0` to `3.1.68-dev.1` in the `package-lock.json` and relevant `package.json` files. This change ensures consistency across the project and incorporates any updates or fixes from the new version.
4 tasks
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.