diff --git a/api/server/services/Files/process.js b/api/server/services/Files/process.js index 66be35152a..a91129c47f 100644 --- a/api/server/services/Files/process.js +++ b/api/server/services/Files/process.js @@ -724,17 +724,11 @@ const processAgentFileUpload = async ({ req, res, metadata, sseStream }) => { const destination = resolveUploadDestination({ toolResource: tool_resource, deliveryPath: llmDeliveryPath, - mimeType: file.mimetype, agentTools: metadata.agentTools, hasAgent: agent_id != null, isMessageAttachment: messageAttachment, }); - if (destination.rejection === 'no-consumer') { - throw new Error( - `Files of type ${file.mimetype} are not sent to the model and can only be used by the code interpreter or file search. Enable one of those tools for this agent, or upload a supported file type.`, - ); - } if (destination.rejection === 'no-agent-resource') { throw new Error( `Files of type ${file.mimetype} cannot be saved to an agent on their own. Attach the file to a message, or enable the code interpreter or file search so the agent has somewhere to keep it.`, diff --git a/api/server/services/Files/process.spec.js b/api/server/services/Files/process.spec.js index e66738a92c..8dd074253c 100644 --- a/api/server/services/Files/process.spec.js +++ b/api/server/services/Files/process.spec.js @@ -2414,45 +2414,27 @@ describe('startExpiredFileSweep', () => { }); }); -describe('unreachable unified uploads', () => { - const makeUnreachableReq = () => { +describe('uploads with no consumer on the agent record', () => { + test('accepts a type the record shows no tool for', async () => { + /* Skills contribute file search and code execution for a turn without being written + * to agent.tools, so an empty list is not evidence that nothing will read the file. + * Reaching storage, which this suite leaves unwired, proves it was not refused. */ const req = makeReq({ mimetype: 'application/zip', ocrConfig: null }); req.body.endpoint = EModelEndpoint.agents; - return req; - }; + getStrategyFunctions.mockClear(); - test('refuses a type no tool can consume', async () => { - /* Nothing extracts a zip, so with neither code nor search enabled it would sit in the - * composer unreadable while the model answered as though it were available. */ - await expect( - processAgentFileUpload({ - req: makeUnreachableReq(), - res: mockRes, - metadata: { - agent_id: 'agent-abc', - message_file: 'true', - file_id: 'file-uuid-zip', - agentTools: [], - }, - }), - ).rejects.toThrow(/code interpreter or file search/i); - }); - - test('lets it past the guard when a file tool can consume it', async () => { - /* Storage is not wired up in this suite, so the upload still fails further along. - * What matters is that it is no longer refused for having no consumer. */ - const error = await processAgentFileUpload({ - req: makeUnreachableReq(), + await processAgentFileUpload({ + req, res: mockRes, metadata: { agent_id: 'agent-abc', message_file: 'true', file_id: 'file-uuid-zip', - agentTools: [EToolResources.execute_code], + agentTools: [], }, - }).catch((thrown) => thrown); + }).catch(() => {}); - expect(String(error?.message ?? '')).not.toMatch(/code interpreter or file search/i); + expect(getStrategyFunctions).toHaveBeenCalled(); }); }); diff --git a/packages/api/src/files/upload/diagnostics.spec.ts b/packages/api/src/files/upload/diagnostics.spec.ts new file mode 100644 index 0000000000..51e2b5e35c --- /dev/null +++ b/packages/api/src/files/upload/diagnostics.spec.ts @@ -0,0 +1,52 @@ +import { logger } from '@librechat/data-schemas'; +import { warnOnUnreachableDeliveryPaths } from './diagnostics'; + +jest.mock('@librechat/data-schemas', () => ({ + logger: { warn: jest.fn() }, +})); + +const warned = (): string[] => (logger.warn as jest.Mock).mock.calls.map(([msg]) => String(msg)); + +describe('warnOnUnreachableDeliveryPaths', () => { + beforeEach(() => { + (logger.warn as jest.Mock).mockClear(); + }); + + it('warns for a fallback that routes everything off the model path', () => { + /* The fallback covers every type no override names, so leaving it unannounced hides + * a wider change than any single override could make. */ + warnOnUnreachableDeliveryPaths({ + fileConfig: { defaultLLMDeliveryPath: { fallback: 'none' } }, + }); + + expect(warned()).toEqual([expect.stringContaining('fallback is set to "none"')]); + }); + + it('warns for an override that routes one type off the model path', () => { + warnOnUnreachableDeliveryPaths({ + fileConfig: { defaultLLMDeliveryPath: { overrides: { 'application/pdf': 'none' } } }, + }); + + expect(warned()).toEqual([expect.stringContaining('"application/pdf" is set to "none"')]); + }); + + it('names the endpoint a warning came from', () => { + warnOnUnreachableDeliveryPaths({ + fileConfig: { + endpoints: { openAI: { defaultLLMDeliveryPath: { fallback: 'none' } } }, + }, + }); + + expect(warned()).toEqual([expect.stringContaining('for "openAI"')]); + }); + + it('stays quiet when every type still reaches the model', () => { + warnOnUnreachableDeliveryPaths({ + fileConfig: { + defaultLLMDeliveryPath: { fallback: 'text', overrides: { 'image/*': 'provider' } }, + }, + }); + + expect(warned()).toEqual([]); + }); +}); diff --git a/packages/api/src/files/upload/diagnostics.ts b/packages/api/src/files/upload/diagnostics.ts index 9c993f6462..dbdaf1f21c 100644 --- a/packages/api/src/files/upload/diagnostics.ts +++ b/packages/api/src/files/upload/diagnostics.ts @@ -1,37 +1,44 @@ import { logger } from '@librechat/data-schemas'; +import type { TDefaultLLMDeliveryPathConfig } from 'librechat-data-provider'; import type { AppConfig } from '@librechat/data-schemas'; /** - * Warns about delivery-path overrides that keep a file off the model path. Routing a type - * to `none` is a legitimate choice, but a silent one: uploads still succeed and only a - * file tool can reach them, so an operator who set it by accident has nothing to notice. + * Warns about delivery settings that keep a file off the model path. Routing a type to + * `none` is a legitimate choice, but a silent one: uploads still succeed and only a file + * tool can reach them, so an operator who set it by accident has nothing to notice. A + * `none` fallback is the louder case, since it covers every type no override names. */ -export function warnOnUnreachableDeliveryPaths(appConfig?: AppConfig): void { - const warnForOverrides = ( - overrides: Record | undefined, +export function warnOnUnreachableDeliveryPaths(appConfig?: Pick): void { + const warnForConfig = ( + config: TDefaultLLMDeliveryPathConfig | undefined, scope?: string, ): void => { - if (!overrides) { + if (!config) { return; } - for (const [mimeType, destination] of Object.entries(overrides)) { + const where = scope ? ` for "${scope}"` : ''; + if (config.fallback === 'none') { + logger.warn( + `[Config] defaultLLMDeliveryPath${where}: fallback is set to "none" — every type without an override will only be accessible through tool provisioning`, + ); + } + for (const [mimeType, destination] of Object.entries(config.overrides ?? {})) { if (destination !== 'none') { continue; } - const where = scope ? ` for "${scope}"` : ''; logger.warn( `[Config] defaultLLMDeliveryPath${where}: "${mimeType}" is set to "none" — files of this type will only be accessible through tool provisioning`, ); } }; - warnForOverrides(appConfig?.fileConfig?.defaultLLMDeliveryPath?.overrides); + warnForConfig(appConfig?.fileConfig?.defaultLLMDeliveryPath); const endpoints = appConfig?.fileConfig?.endpoints; if (!endpoints) { return; } for (const [endpoint, config] of Object.entries(endpoints)) { - warnForOverrides(config?.defaultLLMDeliveryPath?.overrides, endpoint); + warnForConfig(config?.defaultLLMDeliveryPath, endpoint); } } diff --git a/packages/data-provider/src/resolve-llm-delivery-path.spec.ts b/packages/data-provider/src/resolve-llm-delivery-path.spec.ts index cbedbce1d7..2fedaed9f7 100644 --- a/packages/data-provider/src/resolve-llm-delivery-path.spec.ts +++ b/packages/data-provider/src/resolve-llm-delivery-path.spec.ts @@ -1,5 +1,6 @@ import type { TDefaultLLMDeliveryPathConfig } from './file-config'; import { + isNativelyReadableText, resolveUploadDestination, resolveDefaultLLMDeliveryPath, SYSTEM_LLM_DELIVERY_DEFAULTS, @@ -286,7 +287,7 @@ describe('resolveDefaultLLMDeliveryPath', () => { }); describe('resolveUploadDestination', () => { - const base = { mimeType: 'application/zip', hasAgent: true, isMessageAttachment: false }; + const base = { hasAgent: true, isMessageAttachment: false }; it('keeps an explicit resource and normalizes ocr to context', () => { expect( @@ -304,10 +305,17 @@ describe('resolveUploadDestination', () => { ); }); - it('refuses a type no enabled tool can read', () => { + it('does not refuse an upload for having no consumer on the agent record', () => { + /* A skill can contribute file search or code execution for the turn without appearing + * in agent.tools, so an empty list is not evidence that nothing will read the file. */ expect( - resolveUploadDestination({ ...base, deliveryPath: 'none', agentTools: [] }).rejection, - ).toBe('no-consumer'); + resolveUploadDestination({ + ...base, + deliveryPath: 'none', + agentTools: [], + isMessageAttachment: true, + }).rejection, + ).toBeUndefined(); }); it('does not judge an unknown tool set', () => { @@ -332,7 +340,6 @@ describe('resolveUploadDestination', () => { expect( resolveUploadDestination({ ...base, - mimeType: 'image/png', deliveryPath: 'provider', agentTools: [], }).rejection, @@ -343,10 +350,39 @@ describe('resolveUploadDestination', () => { expect( resolveUploadDestination({ ...base, - mimeType: 'image/png', deliveryPath: 'provider', isMessageAttachment: true, }), ).toEqual({}); }); }); + +describe('isNativelyReadableText', () => { + it('admits the application types whose payload is text', () => { + /* Kept in step with the textual set in the content-protection code. Missing one sends + * a readable file down the extractor path, where no parser claims it and it is lost. */ + for (const mimeType of [ + 'application/json', + 'application/javascript', + 'application/sql', + 'application/xml', + 'application/x-yaml', + 'application/yaml', + 'text/markdown', + 'message/rfc822', + ]) { + expect(isNativelyReadableText(mimeType)).toBe(true); + } + }); + + it('rejects types whose bytes are not text', () => { + for (const mimeType of ['application/zip', 'application/pdf', 'image/png']) { + expect(isNativelyReadableText(mimeType)).toBe(false); + } + }); + + it('ignores parameters and case, as browsers send both', () => { + expect(isNativelyReadableText('text/plain; charset=utf-8')).toBe(true); + expect(isNativelyReadableText('Application/JSON')).toBe(true); + }); +}); diff --git a/packages/data-provider/src/resolve-llm-delivery-path.ts b/packages/data-provider/src/resolve-llm-delivery-path.ts index 763b716ced..f4c53a0890 100644 --- a/packages/data-provider/src/resolve-llm-delivery-path.ts +++ b/packages/data-provider/src/resolve-llm-delivery-path.ts @@ -44,7 +44,7 @@ const TEXT_RECOVERABLE_MIME_TYPES: RegExp[] = [ /^text\//, /^image\//, /^audio\//, - /^application\/(json|xml|sql|yaml|csv|typescript|x-sh|vnd\.coffeescript)$/, + /^application\/(json|javascript|xml|sql|yaml|x-yaml|csv|typescript|x-sh|vnd\.coffeescript)$/, /^application\/pdf$/, /* Only the formats the built-in document parser handles. Presentations and graphics * are absent from documentParserMimeTypes, so on a deployment without OCR they would @@ -59,11 +59,27 @@ const TEXT_RECOVERABLE_MIME_TYPES: RegExp[] = [ * Types whose bytes are text already, so reading them directly is meaningful. Everything * else needs a real extractor: decoding it as UTF-8 produces mojibake rather than content. */ +/** Application types whose payload is text. Mirrors the set the content-protection code + * treats as textual, plus the source and data formats this pipeline also accepts. */ +const TEXTUAL_APPLICATION_MIME_TYPES = new Set([ + 'application/json', + 'application/javascript', + 'application/sql', + 'application/xml', + 'application/x-yaml', + 'application/yaml', + 'application/csv', + 'application/typescript', + 'application/x-sh', + 'application/vnd.coffeescript', +]); + export function isNativelyReadableText(mimeType: string): boolean { + const normalized = mimeType.split(';', 1)[0].trim().toLowerCase(); return ( - /^text\//.test(mimeType) || - /^application\/(json|xml|sql|yaml|csv|typescript|x-sh|vnd\.coffeescript)$/.test(mimeType) || - mimeType === 'message/rfc822' + normalized.startsWith('text/') || + TEXTUAL_APPLICATION_MIME_TYPES.has(normalized) || + normalized === 'message/rfc822' ); } @@ -206,7 +222,7 @@ export function resolveUploadLLMDeliveryPath({ } /** Why an upload cannot be accepted, when nothing would be able to read it. */ -export type UploadRejection = 'no-consumer' | 'no-agent-resource'; +export type UploadRejection = 'no-agent-resource'; /** * Where a unified upload will end up, and whether it can be accepted at all. @@ -222,13 +238,11 @@ export type UploadRejection = 'no-consumer' | 'no-agent-resource'; export function resolveUploadDestination(params: { toolResource?: string | null; deliveryPath: TDefaultLLMDeliveryPath; - mimeType: string; agentTools?: string[]; hasAgent: boolean; isMessageAttachment: boolean; }): { toolResource?: string; rejection?: UploadRejection } { - const { toolResource, deliveryPath, mimeType, agentTools, hasAgent, isMessageAttachment } = - params; + const { toolResource, deliveryPath, agentTools, hasAgent, isMessageAttachment } = params; if (toolResource) { return { @@ -241,18 +255,13 @@ export function resolveUploadDestination(params: { return { toolResource: EToolResources.context }; } + /* Skills contribute file tools per turn without being stored on the agent, so this list + * can name a consumer but its silence proves nothing. Used to file an upload, never to + * refuse one. */ const consumingTool = agentTools?.find( (tool) => tool === EToolResources.execute_code || tool === EToolResources.file_search, ); - if (deliveryPath === 'none' && !hasTextExtractionPath(mimeType)) { - /* An administrator who routes a readable type to none has chosen tool-only access - * deliberately and is warned about it at boot, so only unreadable types are refused. */ - if (agentTools != null && consumingTool == null) { - return { rejection: 'no-consumer' }; - } - } - if (!isMessageAttachment && deliveryPath === 'none' && consumingTool) { return { toolResource: consumingTool }; }