From ccd673bf05d6918efca13cb6d7bda49b51927adf Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 31 Aug 2026 09:47:32 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=B0=20fix:=20Keep=20File-Backed=20Agen?= =?UTF-8?q?t=20Capabilities=20and=20Their=20Saved=20Tools=20in=20Agreement?= =?UTF-8?q?=20(#15418)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An agent with knowledge files showed File Search as selected in the builder while the save omitted `file_search` from its tools. At runtime the tool was gone, the row still looked selected so re-adding it changed nothing, and the next save re-confirmed the broken state. Recovery needed a MongoDB write. The builder's model for file-backed capabilities is stated in ToolsSection: they stay selected while they hold files, and removal is routed to the file manager rather than the capability flag — mirroring the file-only `context` built-in. Three places implemented that model separately and drifted: - display resolved it from the flag OR the file count, - persistence resolved it from the flag alone, - the removal guard lived inline in the tools list and was missing entirely from the marketplace dialog. That last gap is how the bad state was reached. `ToolsMarketplaceDialog` toggled a selected File Search straight to `file_search: false` with no file check, producing flag-off-with-files — a state display cannot show and persistence silently drops. `items/capabilities.ts` now owns the capability table and the one rule, and display, removal and persistence all derive from it: - persistence resolves the same way display does, so a file-holding capability keeps its tool (this also heals already-broken agents on their next save), - both toggle paths route removal of a file-holding built-in to the file dialog, - the removal guard takes file counts rather than the agent, so a component can pass the hydrated entries it already watches while submit reads the form — the rule is shared, the source of the count need not be. Submission also stops mutating `data.tools` in place and dedupes the result. Supersedes the persistence half of #15380, which fixed the save but left the marketplace path able to recreate the state and kept the rule in a third copy. Fixes #15368 --- .../SidePanel/Agents/AgentPanel.tsx | 17 +- .../Agents/Tools/ToolsMarketplaceDialog.tsx | 21 ++- .../SidePanel/Agents/Tools/ToolsSection.tsx | 34 ++-- .../__tests__/ToolsMarketplaceDialog.spec.tsx | 20 ++ .../items/__tests__/capabilities.spec.ts | 177 ++++++++++++++++++ .../Agents/Tools/items/capabilities.ts | 109 +++++++++++ .../SidePanel/Agents/Tools/items/selectors.ts | 7 +- 7 files changed, 343 insertions(+), 42 deletions(-) create mode 100644 client/src/components/SidePanel/Agents/Tools/items/__tests__/capabilities.spec.ts create mode 100644 client/src/components/SidePanel/Agents/Tools/items/capabilities.ts diff --git a/client/src/components/SidePanel/Agents/AgentPanel.tsx b/client/src/components/SidePanel/Agents/AgentPanel.tsx index b9132dd694..63a9b593fa 100644 --- a/client/src/components/SidePanel/Agents/AgentPanel.tsx +++ b/client/src/components/SidePanel/Agents/AgentPanel.tsx @@ -5,7 +5,6 @@ import { Button, useToastContext } from '@librechat/client'; import { useWatch, useForm, FormProvider } from 'react-hook-form'; import { useGetModelsQuery } from 'librechat-data-provider/react-query'; import { - Tools, MemoryScope, SystemRoles, ResourceType, @@ -36,6 +35,7 @@ import { import { useResourcePermissions } from '~/hooks/useResourcePermissions'; import { useSelectAgent, useLocalize, useAuthContext } from '~/hooks'; import { useAgentPanelContext } from '~/Providers/AgentPanelContext'; +import { resolveCapabilityTools } from './Tools/items/capabilities'; import AgentPanelSkeleton from './AgentPanelSkeleton'; import AdvancedPanel from './Advanced/AdvancedPanel'; import { Panel, isEphemeralAgent } from '~/common'; @@ -564,20 +564,7 @@ export default function AgentPanel() { const onSubmit = useCallback( async (data: AgentForm) => { - const tools = data.tools ?? []; - - if (data.execute_code === true) { - tools.push(Tools.execute_code); - } - if (data.file_search === true) { - tools.push(Tools.file_search); - } - if (data.web_search === true) { - tools.push(Tools.web_search); - } - if (data.memory === true) { - tools.push(Tools.memory); - } + const tools = Array.from(new Set([...(data.tools ?? []), ...resolveCapabilityTools(data)])); const { payload: basePayload, provider, model } = composeAgentUpdatePayload(data, agent_id); diff --git a/client/src/components/SidePanel/Agents/Tools/ToolsMarketplaceDialog.tsx b/client/src/components/SidePanel/Agents/Tools/ToolsMarketplaceDialog.tsx index e64c90a2d8..7f092e600f 100644 --- a/client/src/components/SidePanel/Agents/Tools/ToolsMarketplaceDialog.tsx +++ b/client/src/components/SidePanel/Agents/Tools/ToolsMarketplaceDialog.tsx @@ -11,6 +11,7 @@ import { useToastContext, } from '@librechat/client'; import type { AgentItem, AgentItemKind, ItemFilter } from './items/types'; +import type { CapabilityFileCounts } from './items/capabilities'; import type { AgentForm } from '~/common'; import { itemKey, @@ -19,7 +20,8 @@ import { matchesMcpServer, mcpServerIds, } from './items/selectors'; -import { useAgentItems, useUninstallToolCredentials } from './hooks'; +import { useAgentFileEntries, useAgentItems, useUninstallToolCredentials } from './hooks'; +import { requiresFileManagerRemoval } from './items/capabilities'; import AddMcpServerDialog from './ItemDialog/AddMcpServerDialog'; import { computeToggleAction } from './items/mutations'; import { useLocalize, useToolFavorites } from '~/hooks'; @@ -86,6 +88,11 @@ export default function ToolsMarketplaceDialog({ ); const selectedIds = useMemo(() => new Set(selectedItems.map(itemKey)), [selectedItems]); + const { knowledgeFiles, codeFiles } = useAgentFileEntries(); + const fileCounts: CapabilityFileCounts = useMemo( + () => ({ knowledge_files: knowledgeFiles.length, code_files: codeFiles.length }), + [knowledgeFiles, codeFiles], + ); const counts = useMemo( () => ({ @@ -106,7 +113,15 @@ export default function ToolsMarketplaceDialog({ const handleToggle = useCallback( (item: AgentItem) => { - const patch = computeToggleAction(item, { selected: selectedIds.has(itemKey(item)) }); + const selected = selectedIds.has(itemKey(item)); + /** A file-backed built-in cannot be switched off by its flag while it holds + * files — the row would stay selected and the save would disagree with it — so + * send the user to the dialog that manages those files, as the tools list does. */ + if (selected && item.kind === 'builtin' && requiresFileManagerRemoval(item.id, fileCounts)) { + setDetailItem(item); + return; + } + const patch = computeToggleAction(item, { selected }); switch (patch.type) { case 'builtin': setValue(patch.field as keyof AgentForm, patch.value as never, { shouldDirty: true }); @@ -160,7 +175,7 @@ export default function ToolsMarketplaceDialog({ break; } }, - [getValues, setValue, selectedIds, uninstallToolCredentials, catalog], + [getValues, setValue, selectedIds, uninstallToolCredentials, catalog, fileCounts], ); const handleCardClick = useCallback( diff --git a/client/src/components/SidePanel/Agents/Tools/ToolsSection.tsx b/client/src/components/SidePanel/Agents/Tools/ToolsSection.tsx index 882bb7a475..4f06069d00 100644 --- a/client/src/components/SidePanel/Agents/Tools/ToolsSection.tsx +++ b/client/src/components/SidePanel/Agents/Tools/ToolsSection.tsx @@ -19,16 +19,18 @@ import { } from '@librechat/client'; import type { TPlugin } from 'librechat-data-provider'; import type { ReactNode } from 'react'; +import type { CapabilityFileCounts } from './items/capabilities'; import type { AgentItem } from './items/types'; import type { AgentForm } from '~/common'; import { + useAgentFileEntries, useAgentItems, useResolvedSkills, - useAgentFileEntries, useUninstallToolCredentials, } from './hooks'; import { computeToggleAction, skillsEnabledTransition } from './items/mutations'; import { useListSkillsQuery, useDeleteAgentAction } from '~/data-provider'; +import { requiresFileManagerRemoval } from './items/capabilities'; import { useRemoveMCPTool, useVisibleTools } from '~/hooks/MCP'; import ToolsMarketplaceDialog from './ToolsMarketplaceDialog'; import { useLocalize, useHasAccess } from '~/hooks'; @@ -117,7 +119,6 @@ export default function ToolsSection({ agentId }: Props) { ); const uninstallToolCredentials = useUninstallToolCredentials(); - const { knowledgeFiles, codeFiles } = useAgentFileEntries(); const { selected, tools } = useAgentItems({ agentId, @@ -125,27 +126,16 @@ export default function ToolsSection({ agentId }: Props) { skillsPermission: showSkills, }); - /** File-backed built-ins stay selected while they hold files, so flipping - * the capability flag off would leave the row visible. Route their removal - * to the config dialog (where files are managed) instead, mirroring the - * file-only `context` built-in. */ + const { knowledgeFiles, codeFiles } = useAgentFileEntries(); + const fileCounts: CapabilityFileCounts = useMemo( + () => ({ knowledge_files: knowledgeFiles.length, code_files: codeFiles.length }), + [knowledgeFiles, codeFiles], + ); + const opensFileManagerOnRemove = useCallback( - (item: AgentItem): boolean => { - if (item.kind !== 'builtin') { - return false; - } - if (item.id === 'context') { - return true; - } - if (item.id === 'execute_code') { - return codeFiles.length > 0; - } - if (item.id === 'file_search') { - return knowledgeFiles.length > 0; - } - return false; - }, - [codeFiles, knowledgeFiles], + (item: AgentItem): boolean => + item.kind === 'builtin' && requiresFileManagerRemoval(item.id, fileCounts), + [fileCounts], ); const handleQuickRemove = useCallback( diff --git a/client/src/components/SidePanel/Agents/Tools/__tests__/ToolsMarketplaceDialog.spec.tsx b/client/src/components/SidePanel/Agents/Tools/__tests__/ToolsMarketplaceDialog.spec.tsx index c67da9c50a..725dcb92bd 100644 --- a/client/src/components/SidePanel/Agents/Tools/__tests__/ToolsMarketplaceDialog.spec.tsx +++ b/client/src/components/SidePanel/Agents/Tools/__tests__/ToolsMarketplaceDialog.spec.tsx @@ -69,11 +69,18 @@ jest.mock('react-router-dom', () => ({ useNavigate: () => jest.fn(), })); +let mockFileEntries: { + contextFiles: Array<[string, unknown]>; + knowledgeFiles: Array<[string, unknown]>; + codeFiles: Array<[string, unknown]>; +} = { contextFiles: [], knowledgeFiles: [], codeFiles: [] }; + jest.mock('../hooks', () => { const { buildCatalog } = jest.requireActual('../items/catalog'); const { deriveSelectedItems } = jest.requireActual('../items/selectors'); return { useUninstallToolCredentials: () => jest.fn(), + useAgentFileEntries: () => mockFileEntries, /** Mirrors the real pipeline over the mocked panel context + form watches. */ useAgentItems: ({ agentId }: { agentId: string }) => { const { useAgentPanelContext } = jest.requireMock('~/Providers'); @@ -161,6 +168,7 @@ describe('ToolsMarketplaceDialog', () => { mockMcpServersMap = new Map(); mockToggleFavorite.mockClear(); mockFavoriteKeys = new Set(); + mockFileEntries = { contextFiles: [], knowledgeFiles: [], codeFiles: [] }; }); test('renders cards from catalog when open', () => { @@ -185,6 +193,18 @@ describe('ToolsMarketplaceDialog', () => { ); }); + test('routes a file-holding built-in to its file dialog instead of clearing the flag', () => { + /** Clearing the flag here left the row still selected (it reads the file count) while + * the save wrote the tool off the flag alone, silently dropping it from the agent. */ + mockExecuteCode = true; + mockFileEntries = { contextFiles: [], knowledgeFiles: [], codeFiles: [['c1', {}]] }; + + render(); + fireEvent.click(screen.getByRole('button', { name: /com_ui_run_code/ })); + + expect(mockSetValue).not.toHaveBeenCalledWith('execute_code', false, expect.anything()); + }); + test('disabling Code Interpreter clears programmatic MCP callers immediately', () => { mockExecuteCode = true; mockGetValues.mockImplementation((name: string) => diff --git a/client/src/components/SidePanel/Agents/Tools/items/__tests__/capabilities.spec.ts b/client/src/components/SidePanel/Agents/Tools/items/__tests__/capabilities.spec.ts new file mode 100644 index 0000000000..ee6e4bc869 --- /dev/null +++ b/client/src/components/SidePanel/Agents/Tools/items/__tests__/capabilities.spec.ts @@ -0,0 +1,177 @@ +import { AgentCapabilities, EToolResources, Tools } from 'librechat-data-provider'; +import type { AgentForm, TAgentOption } from '~/common'; +import { + countCapabilityFiles, + FILE_BACKED_CAPABILITIES, + isFileBackedCapabilityEnabled, + requiresFileManagerRemoval, + resolveCapabilityTools, +} from '../capabilities'; + +const fileEntry = (id: string) => [id, { file_id: id }] as [string, never]; + +const agentWith = (overrides: Partial = {}): TAgentOption => + ({ id: 'agent_1', ...overrides }) as TAgentOption; + +const formWith = (overrides: Partial = {}): AgentForm => + ({ id: 'agent_1', tools: [], ...overrides }) as AgentForm; + +const knowledge = FILE_BACKED_CAPABILITIES.find( + (entry) => entry.capability === AgentCapabilities.file_search, +)!; + +describe('countCapabilityFiles', () => { + it('counts the hydrated form entries', () => { + const agent = agentWith({ knowledge_files: [fileEntry('f1'), fileEntry('f2')] }); + + expect(countCapabilityFiles(agent, knowledge)).toBe(2); + }); + + it('falls back to persisted ids before the file map hydrates', () => { + const agent = agentWith({ + tool_resources: { [EToolResources.file_search]: { file_ids: ['f1', 'f2', 'f3'] } }, + }); + + expect(countCapabilityFiles(agent, knowledge)).toBe(3); + }); + + it('prefers hydrated entries over persisted ids', () => { + const agent = agentWith({ + knowledge_files: [fileEntry('f1')], + tool_resources: { [EToolResources.file_search]: { file_ids: ['f1', 'f2', 'f3'] } }, + }); + + expect(countCapabilityFiles(agent, knowledge)).toBe(1); + }); + + it('reports no files for an agent that holds none', () => { + expect(countCapabilityFiles(agentWith(), knowledge)).toBe(0); + expect(countCapabilityFiles(undefined, knowledge)).toBe(0); + }); +}); + +describe('isFileBackedCapabilityEnabled', () => { + it('is on when the flag is set', () => { + expect(isFileBackedCapabilityEnabled(true, 0)).toBe(true); + }); + + it('is on when files are attached without the flag', () => { + expect(isFileBackedCapabilityEnabled(false, 1)).toBe(true); + expect(isFileBackedCapabilityEnabled(undefined, 1)).toBe(true); + }); + + it('is off only with neither', () => { + expect(isFileBackedCapabilityEnabled(false, 0)).toBe(false); + expect(isFileBackedCapabilityEnabled(undefined, 0)).toBe(false); + }); +}); + +describe('resolveCapabilityTools', () => { + it('persists file_search for an agent holding knowledge files with the flag cleared', () => { + const data = formWith({ + [AgentCapabilities.file_search]: false, + agent: agentWith({ knowledge_files: [fileEntry('f1')] }), + }); + + expect(resolveCapabilityTools(data)).toContain(Tools.file_search); + }); + + it('persists file_search from the flag alone before any file is attached', () => { + const data = formWith({ [AgentCapabilities.file_search]: true, agent: agentWith() }); + + expect(resolveCapabilityTools(data)).toEqual([Tools.file_search]); + }); + + it('persists execute_code for an agent holding code files with the flag cleared', () => { + const data = formWith({ + [AgentCapabilities.execute_code]: false, + agent: agentWith({ code_files: [fileEntry('c1')] }), + }); + + expect(resolveCapabilityTools(data)).toContain(Tools.execute_code); + }); + + it('resolves file-backed capabilities from the unhydrated agent document', () => { + const data = formWith({ + [AgentCapabilities.file_search]: false, + agent: agentWith({ + tool_resources: { [EToolResources.file_search]: { file_ids: ['f1'] } }, + }), + }); + + expect(resolveCapabilityTools(data)).toContain(Tools.file_search); + }); + + it('omits capabilities that are neither enabled nor backed by files', () => { + expect(resolveCapabilityTools(formWith({ agent: agentWith() }))).toEqual([]); + }); + + it('persists flag-only capabilities from their flag alone', () => { + const data = formWith({ + [AgentCapabilities.web_search]: true, + [AgentCapabilities.memory]: true, + agent: agentWith(), + }); + + expect(resolveCapabilityTools(data)).toEqual([Tools.web_search, Tools.memory]); + }); + + it('does not resurrect a flag-only capability from attached files', () => { + const data = formWith({ + [AgentCapabilities.web_search]: false, + agent: agentWith({ knowledge_files: [fileEntry('f1')] }), + }); + + expect(resolveCapabilityTools(data)).not.toContain(Tools.web_search); + }); +}); + +describe('requiresFileManagerRemoval', () => { + const counts = (knowledge = 0, code = 0) => ({ + knowledge_files: knowledge, + code_files: code, + }); + + it('routes a file-holding capability to the file manager', () => { + expect(requiresFileManagerRemoval(AgentCapabilities.file_search, counts(1))).toBe(true); + expect(requiresFileManagerRemoval(AgentCapabilities.execute_code, counts(0, 1))).toBe(true); + }); + + it('lets a capability with no files be switched off by its flag', () => { + expect(requiresFileManagerRemoval(AgentCapabilities.file_search, counts())).toBe(false); + expect(requiresFileManagerRemoval(AgentCapabilities.execute_code, counts(1))).toBe(false); + }); + + it('always routes the file-only context built-in to the file manager', () => { + expect(requiresFileManagerRemoval(EToolResources.context, counts())).toBe(true); + }); + + it('leaves flag-only built-ins alone', () => { + expect(requiresFileManagerRemoval(AgentCapabilities.web_search, counts(1, 1))).toBe(false); + expect(requiresFileManagerRemoval('artifacts', counts(1, 1))).toBe(false); + }); +}); + +describe('display and persistence agree', () => { + /** The reported bug: the builder showed File Search as selected off the file count + * while the save wrote it off the flag alone, so it was dropped from an agent that + * still listed its knowledge files. Both sides now read the same rule. */ + it.each([ + [true, 0], + [false, 1], + [true, 1], + [false, 0], + ])('flag=%s files=%s', (flag, fileCount) => { + const displayed = isFileBackedCapabilityEnabled(flag, fileCount); + const persisted = resolveCapabilityTools( + formWith({ + [AgentCapabilities.file_search]: flag, + agent: agentWith({ + knowledge_files: Array.from({ length: fileCount }, (_, i) => fileEntry(`f${i}`)), + }), + }), + ).includes(Tools.file_search); + + expect(persisted).toBe(displayed); + }); +}); diff --git a/client/src/components/SidePanel/Agents/Tools/items/capabilities.ts b/client/src/components/SidePanel/Agents/Tools/items/capabilities.ts new file mode 100644 index 0000000000..2ebede3478 --- /dev/null +++ b/client/src/components/SidePanel/Agents/Tools/items/capabilities.ts @@ -0,0 +1,109 @@ +import { AgentCapabilities, EToolResources, Tools } from 'librechat-data-provider'; +import type { AgentForm, TAgentOption } from '~/common'; + +/** + * Built-in capabilities whose selection is backed by attached files. These are on + * while they hold files: the builder routes their removal to the file manager rather + * than the capability flag, so "flag off while files remain" is a state the UI cannot + * show and the user cannot reach deliberately. + * + * Display, removal and persistence all derive from this table. They used to encode the + * rule separately and drifted — the builder showed File Search as selected off the file + * count while the save wrote it off the flag alone, so the tool was silently dropped + * from an agent that still listed its knowledge files. + */ +export const FILE_BACKED_CAPABILITIES = [ + { + capability: AgentCapabilities.file_search, + tool: Tools.file_search, + files: 'knowledge_files', + resource: EToolResources.file_search, + }, + { + capability: AgentCapabilities.execute_code, + tool: Tools.execute_code, + files: 'code_files', + resource: EToolResources.execute_code, + }, +] as const; + +export type FileBackedCapability = (typeof FILE_BACKED_CAPABILITIES)[number]; + +/** Built-in capabilities that are a plain on/off flag, with no files behind them. */ +const FLAG_ONLY_CAPABILITIES = [ + { capability: AgentCapabilities.web_search, tool: Tools.web_search }, + { capability: AgentCapabilities.memory, tool: Tools.memory }, +] as const; + +/** + * How many files an agent holds for a file-backed capability. Prefers the hydrated + * form entries and falls back to the persisted ids, which are all that exist before + * the file map hydrates — a save during that window must not read "no files" and + * drop the tool. + */ +export function countCapabilityFiles( + agent: TAgentOption | undefined, + { files, resource }: Pick, +): number { + const entries = agent?.[files]; + if (entries != null) { + return entries.length; + } + return agent?.tool_resources?.[resource]?.file_ids?.length ?? 0; +} + +/** Whether a file-backed capability is on, given its flag and the files it holds. */ +export function isFileBackedCapabilityEnabled(flag: boolean | undefined, fileCount: number) { + return flag === true || fileCount > 0; +} + +/** File counts per capability, supplied by the caller from its best source. */ +export type CapabilityFileCounts = Record; + +/** + * Whether removing this built-in has to go through the file manager. A capability + * holding files cannot be switched off by clearing its flag — the row would stay + * selected and the save would disagree with it — so the caller opens the dialog + * where the files themselves are managed. + * + * Takes counts rather than the agent so a component can pass the hydrated entries it + * already watches, while `resolveCapabilityTools` reads the form on submit where no + * hook is available. The rule is shared; the source of the count need not be. + */ +export function requiresFileManagerRemoval(itemId: string, counts: CapabilityFileCounts): boolean { + if (itemId === EToolResources.context) { + return true; + } + const entry = FILE_BACKED_CAPABILITIES.find(({ capability }) => capability === itemId); + if (entry == null) { + return false; + } + return counts[entry.files] > 0; +} + +/** + * The built-in capability tools a submission should persist, resolved with the same + * rule the builder displays. Returned separately from `data.tools` so the caller + * decides how to merge them. + */ +export function resolveCapabilityTools(data: AgentForm): string[] { + const tools: string[] = []; + + for (const entry of FILE_BACKED_CAPABILITIES) { + const enabled = isFileBackedCapabilityEnabled( + data[entry.capability], + countCapabilityFiles(data.agent, entry), + ); + if (enabled) { + tools.push(entry.tool); + } + } + + for (const { capability, tool } of FLAG_ONLY_CAPABILITIES) { + if (data[capability] === true) { + tools.push(tool); + } + } + + return tools; +} diff --git a/client/src/components/SidePanel/Agents/Tools/items/selectors.ts b/client/src/components/SidePanel/Agents/Tools/items/selectors.ts index 36bc9937c5..ec2821acf7 100644 --- a/client/src/components/SidePanel/Agents/Tools/items/selectors.ts +++ b/client/src/components/SidePanel/Agents/Tools/items/selectors.ts @@ -6,6 +6,7 @@ import { } from 'librechat-data-provider'; import type { Action } from 'librechat-data-provider'; import type { AgentItem, AgentItemKind } from './types'; +import { isFileBackedCapabilityEnabled } from './capabilities'; export interface FormSelection { execute_code: boolean; @@ -35,12 +36,14 @@ export function itemKey(item: Pick): string { function isBuiltinSelected(item: AgentItem, form: FormSelection): boolean { if (item.kind !== 'builtin') return false; switch (item.id) { + /** File-backed built-ins share their on/off rule with removal and persistence + * (see `capabilities.ts`), so the three cannot drift apart again. */ case 'execute_code': - return form.execute_code || form.code_files.length > 0; + return isFileBackedCapabilityEnabled(form.execute_code, form.code_files.length); case 'web_search': return form.web_search; case 'file_search': - return form.file_search || form.knowledge_files.length > 0; + return isFileBackedCapabilityEnabled(form.file_search, form.knowledge_files.length); case 'memory': return form.memory; case 'artifacts':