mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-09-06 06:28:10 +00:00
fix: judge file consumers where they run, not where they upload
Three related corrections from review, all one theme: upload time cannot see what will read a file. Stop refusing an upload for having no consumer on the agent record. A skill contributes file search or code execution for a turn through its allowed tools, and those are resolved per turn rather than stored on the agent, so an empty tool list was never evidence that nothing would read the file. The list still files an upload under a tool it names; it just no longer refuses one by its silence. This restores the pre-branch behavior for those uploads rather than introducing a new outcome. Warn at boot when a delivery fallback is set to none. The fallback covers every type no override names, so leaving it unannounced hid a wider change than any single override could make. Align the native text matcher with the textual MIME set in the content-protection code, adding javascript and x-yaml, and normalize parameters and case before matching so a browser-sent "text/plain; charset=utf-8" is still recognized. Drops mimeType from resolveUploadDestination, which no longer reads it.
This commit is contained in:
parent
f435cb2a80
commit
7dddc4ef05
6 changed files with 148 additions and 68 deletions
|
|
@ -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.`,
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
52
packages/api/src/files/upload/diagnostics.spec.ts
Normal file
52
packages/api/src/files/upload/diagnostics.spec.ts
Normal file
|
|
@ -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([]);
|
||||
});
|
||||
});
|
||||
|
|
@ -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<string, string> | undefined,
|
||||
export function warnOnUnreachableDeliveryPaths(appConfig?: Pick<AppConfig, 'fileConfig'>): 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);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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 };
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue