diff --git a/api/server/services/Files/Code/process.js b/api/server/services/Files/Code/process.js index 6bb352e011..723a718329 100644 --- a/api/server/services/Files/Code/process.js +++ b/api/server/services/Files/Code/process.js @@ -22,7 +22,10 @@ const { getStorageMetadata, getCodeExecutionBaseUrl, buildCodeEnvDownloadQuery, + claimCodeDestination, + createCodeDestinationSet, CODE_API_EXPECTED_PROFILE_HEADER, + sortCodeFilesByDestinationPriority, } = require('@librechat/api'); const { Tools, @@ -1030,13 +1033,13 @@ const getPreviewContextSuffix = (file) => { : ' (preview unavailable)'; }; -const getVisibleCodeFileContextLine = (file, agentResourceIds) => { +const getVisibleCodeFileContextLine = (file, agentResourceIds, destination) => { if (file.context === FileContext.execute_code) { return ''; } const fileSuffix = agentResourceIds.has(file.file_id) ? '' : ' (attached by user)'; - return `\n\t- /mnt/data/${file.filename}${fileSuffix}${getPreviewContextSuffix(file)}`; + return `\n\t- /mnt/data/${destination}${fileSuffix}${getPreviewContextSuffix(file)}`; }; const appendVisibleCodeFileContext = (toolContext, contextLine) => { @@ -1163,6 +1166,13 @@ const primeFiles = async (options) => { const sessions = new Map(); let toolContext = ''; + /* Claim order decides which record keeps the bare `/mnt/data/` path + * when several share a filename, so it is fixed here rather than inherited + * from `getFiles`'s `updatedAt` sort — usage accounting and re-upload both + * bump `updatedAt`, which would repoint paths between turns. */ + const orderedFiles = sortCodeFilesByDestinationPriority(dbFiles); + const destinations = createCodeDestinationSet(); + /* Per-file path counters — emitted at the bottom so a single * grep on `[primeCodeFiles]` shows the input volume, the per-file * paths taken, and the final dispatch summary in one trace. */ @@ -1171,8 +1181,8 @@ const primeFiles = async (options) => { let requiredCodeFiles = 0; const reuploadFailureCategories = new Set(); - for (let i = 0; i < dbFiles.length; i++) { - const file = dbFiles[i]; + for (let i = 0; i < orderedFiles.length; i++) { + const file = orderedFiles[i]; if (!file) { continue; } @@ -1205,9 +1215,19 @@ const primeFiles = async (options) => { * tenant prefix from auth context). */ const pushFile = (overrideSessionId, overrideId) => { + /* Claimed here rather than up front so files that never reach the + * sandbox — no code-env ref, or a failed re-upload — do not reserve a + * name and push a file that does reach it onto a counter. */ + const destination = claimCodeDestination(destinations, file.filename); + if (destination !== file.filename) { + logger.debug( + `[primeCodeFiles] file=${file.file_id} destination=${destination} ` + + `reason=name-collision filename=${file.filename}`, + ); + } toolContext = appendVisibleCodeFileContext( toolContext, - getVisibleCodeFileContextLine(file, agentResourceIds), + getVisibleCodeFileContextLine(file, agentResourceIds, destination), ); /* `id` is the storage file_id (drives codeapi's upload-key * existence check), `resource_id` is the entity that owns @@ -1219,7 +1239,7 @@ const primeFiles = async (options) => { id: overrideId ?? id, resource_id: sourceRef.id, storage_session_id: overrideSessionId ?? session_id, - name: file.filename, + name: destination, kind: sourceRef.kind, ...(sourceRef.kind === 'skill' ? { version: sourceRef.version } : {}), }); diff --git a/api/server/services/Files/Code/process.spec.js b/api/server/services/Files/Code/process.spec.js index 8e2b3b258a..bb6a454249 100644 --- a/api/server/services/Files/Code/process.spec.js +++ b/api/server/services/Files/Code/process.spec.js @@ -113,6 +113,30 @@ jest.mock('@librechat/api', () => { }), codeServerHttpAgent: new http.Agent({ keepAlive: false }), codeServerHttpsAgent: new https.Agent({ keepAlive: false }), + /* Sandbox destination assignment, mirrored the same way the identity + * helpers above are. The real policy — directory-prefix conflicts, the + * byte cap, flattening — lives in + * `packages/api/src/files/code/destinations.ts` under its own + * `destinations.spec.ts`; these stubs carry just enough of it (a counter + * per repeated name, newest-first ordering) for the `primeFiles` tests to + * assert that it is wired to `name` and to the tool context at all. */ + createCodeDestinationSet: () => new Set(), + claimCodeDestination: (set, name) => { + let destination = name; + const dot = name.lastIndexOf('.'); + const stem = dot > 0 ? name.slice(0, dot) : name; + const extension = dot > 0 ? name.slice(dot) : ''; + for (let counter = 2; set.has(destination); counter++) { + destination = `${stem}-${counter}${extension}`; + } + set.add(destination); + return destination; + }, + sortCodeFilesByDestinationPriority: (files) => + [...files].sort((a, b) => { + const delta = new Date(b.createdAt ?? 0).getTime() - new Date(a.createdAt ?? 0).getTime(); + return delta !== 0 ? delta : (a.file_id ?? '').localeCompare(b.file_id ?? ''); + }), }; }); @@ -2847,4 +2871,168 @@ describe('Code Process', () => { expect(mockAxios).toHaveBeenCalledTimes(1); }); }); + + describe('primeFiles resolves sandbox destination collisions (#15443)', () => { + /** + * Codeapi mounts each input at a destination derived from its `name` and + * rejects the whole `/exec` request when two entries land on one — and a + * rejected request never reaches the sandbox, so nothing comes back to + * collapse the pair. Every later turn re-primes both files and fails the + * same way, which is why one duplicate filename kills code execution for + * the rest of the conversation. + * + * Only code-generated outputs are covered by the `(filename, + * conversationId, context, tenantId)` partial unique index; uploads carry + * `context: message_attachment`, so a conversation can hold several + * records sharing a filename. + */ + const { getFiles } = require('~/models'); + const { getStrategyFunctions } = require('~/server/services/Files/strategies'); + + const codeFile = ({ file_id, filename, storage_session_id, createdAt, context }) => ({ + file_id, + filename, + createdAt, + context: context ?? 'message_attachment', + source: 'local', + filepath: `/uploads/${file_id}`, + metadata: { + codeEnvRef: { + kind: 'user', + id: 'user-123', + storage_session_id, + file_id: `${file_id}-sandbox`, + }, + }, + }); + + /** Freshness probe answers "uploaded just now", so every file takes the + * cache-hit path and none of them re-upload. */ + function setupActiveSessions() { + getStrategyFunctions.mockImplementation(() => ({ + getDownloadStream: jest.fn(), + handleFileUpload: jest.fn(), + })); + mockAxios.mockResolvedValue({ data: { lastModified: new Date().toISOString() } }); + } + + const prime = () => + primeFiles({ + req: { user: { id: 'user-123', role: 'USER' } }, + tool_resources: { execute_code: { file_ids: ['older', 'newer'] } }, + agentId: 'agent-id', + }); + + it('gives two uploads sharing a filename distinct destinations', async () => { + setupActiveSessions(); + getFiles.mockResolvedValue([ + codeFile({ + file_id: 'older', + filename: 'image.png', + storage_session_id: 'sess-older', + createdAt: new Date('2026-01-01T00:00:00Z'), + }), + codeFile({ + file_id: 'newer', + filename: 'image.png', + storage_session_id: 'sess-newer', + createdAt: new Date('2026-01-02T00:00:00Z'), + }), + ]); + + const { files, toolContext } = await prime(); + + expect(files).toHaveLength(2); + expect(new Set(files.map((f) => f.name)).size).toBe(2); + /* The newest record keeps the bare path: when an execution rewrote an + * uploaded file in place, the model's next read of that path has to + * find its own edit rather than the superseded original. */ + expect(files.find((f) => f.storage_session_id === 'sess-newer').name).toBe('image.png'); + expect(files.find((f) => f.storage_session_id === 'sess-older').name).toBe('image-2.png'); + expect(toolContext).toContain('/mnt/data/image.png'); + expect(toolContext).toContain('/mnt/data/image-2.png'); + }); + + it('assigns the same destinations regardless of the order getFiles returns', async () => { + /** + * `getFiles` sorts by `updatedAt` desc by default, and usage accounting + * and re-upload both bump `updatedAt` — so claim order cannot come from + * the query. If it did, a path a previous turn told the model about + * would silently point at the other file. + */ + const older = codeFile({ + file_id: 'older', + filename: 'image.png', + storage_session_id: 'sess-older', + createdAt: new Date('2026-01-01T00:00:00Z'), + }); + const newer = codeFile({ + file_id: 'newer', + filename: 'image.png', + storage_session_id: 'sess-newer', + createdAt: new Date('2026-01-02T00:00:00Z'), + }); + + setupActiveSessions(); + getFiles.mockResolvedValue([older, newer]); + const ascending = await prime(); + + setupActiveSessions(); + getFiles.mockResolvedValue([newer, older]); + const descending = await prime(); + + const byFileId = (result) => + Object.fromEntries(result.files.map((f) => [f.storage_session_id, f.name])); + expect(byFileId(ascending)).toEqual(byFileId(descending)); + expect(byFileId(ascending)).toEqual({ + 'sess-newer': 'image.png', + 'sess-older': 'image-2.png', + }); + }); + + it('separates a code output from the upload whose name it reused', async () => { + setupActiveSessions(); + getFiles.mockResolvedValue([ + codeFile({ + file_id: 'older', + filename: 'data.csv', + storage_session_id: 'sess-upload', + createdAt: new Date('2026-01-01T00:00:00Z'), + }), + codeFile({ + file_id: 'newer', + filename: 'data.csv', + storage_session_id: 'sess-output', + createdAt: new Date('2026-01-02T00:00:00Z'), + context: 'execute_code', + }), + ]); + + const { files, toolContext } = await prime(); + + expect(files.find((f) => f.storage_session_id === 'sess-output').name).toBe('data.csv'); + expect(files.find((f) => f.storage_session_id === 'sess-upload').name).toBe('data-2.csv'); + /* Generated outputs are never listed in the tool context — the model + * already knows what it wrote — so only the displaced upload appears. */ + expect(toolContext).toContain('/mnt/data/data-2.csv'); + expect(toolContext).not.toContain('/mnt/data/data.csv '); + }); + + it('leaves a single file on its own filename', async () => { + setupActiveSessions(); + getFiles.mockResolvedValue([ + codeFile({ + file_id: 'older', + filename: 'report.pdf', + storage_session_id: 'sess-only', + createdAt: new Date('2026-01-01T00:00:00Z'), + }), + ]); + + const { files, toolContext } = await prime(); + + expect(files.map((f) => f.name)).toEqual(['report.pdf']); + expect(toolContext).toContain('/mnt/data/report.pdf'); + }); + }); }); diff --git a/packages/api/src/agents/codeFilesSession.spec.ts b/packages/api/src/agents/codeFilesSession.spec.ts index a3aeb05066..4a600f07aa 100644 --- a/packages/api/src/agents/codeFilesSession.spec.ts +++ b/packages/api/src/agents/codeFilesSession.spec.ts @@ -132,21 +132,63 @@ describe('seedCodeFilesIntoSessions', () => { expect(entry.files!.map((f) => f.id).sort()).toEqual(['new-1', 'shared-1', 'skill-1']); }); - it('treats same name + same session as a duplicate; same name + different sessions as distinct', () => { + it('keeps distinct identities that mount at distinct destinations', () => { /** - * The dedupe key is `(session_id, id)` — not `name` alone. Two - * primed uploads can legitimately share a filename when they live - * in different sandbox sessions (e.g. each agent re-uploaded the - * same source file). Both should land in the seed. + * The dedupe key is `(session_id, id)` — not `name` alone. Two primed + * uploads are separate files even when one was re-uploaded into a new + * sandbox session, and both belong in the seed as long as the caller + * resolved them to different mount paths. */ const a = file('id-A', 'sess-A', 'data.csv'); - const b = file('id-B', 'sess-B', 'data.csv'); + const b = file('id-B', 'sess-B', 'data-2.csv'); const result = seedCodeFilesIntoSessions([a, a, b], undefined); const entry = result!.get(Constants.EXECUTE_CODE) as CodeSessionContext; expect(entry.files).toHaveLength(2); expect(entry.files!.map((f) => f.storage_session_id).sort()).toEqual(['sess-A', 'sess-B']); }); + it('drops a distinct identity that would mount at an already-claimed destination', () => { + /** + * Regression for #15443. The identity key cannot see this: one file + * re-uploaded by one agent and cache-hit by another arrives twice with + * different `storage_session_id`s under a single `name`. Codeapi rejects + * the whole `/exec` request on the duplicate destination, and because the + * call never reaches the sandbox nothing comes back to collapse the pair + * — every later turn re-primes it and fails the same way. + */ + const primed = file('id-A', 'sess-A', 'data.csv'); + const reuploaded = file('id-B', 'sess-B', 'data.csv'); + const result = seedCodeFilesIntoSessions([primed, reuploaded], undefined); + const entry = result!.get(Constants.EXECUTE_CODE) as CodeSessionContext; + expect(entry.files).toHaveLength(1); + expect(entry.files![0].storage_session_id).toBe('sess-A'); + }); + + it('lets the prior partition keep a destination an incoming file also wants', () => { + const existing: ToolSessionMap = new Map(); + existing.set(Constants.EXECUTE_CODE, { + session_id: 'skill-sess', + files: [file('skill-1', 'skill-sess', 'skills/report/run.py')], + lastUpdated: 1, + } satisfies CodeSessionContext); + + const result = seedCodeFilesIntoSessions( + [file('user-1', 'user-sess', 'skills/report/run.py'), file('user-2', 'user-sess', 'ok.csv')], + existing, + ); + const entry = result!.get(Constants.EXECUTE_CODE) as CodeSessionContext; + expect(entry.files!.map((f) => f.id)).toEqual(['skill-1', 'user-2']); + }); + + it('rejects an incoming file whose destination is a directory of a claimed one', () => { + const result = seedCodeFilesIntoSessions( + [file('id-A', 'sess-A', 'data/rows.csv'), file('id-B', 'sess-B', 'data')], + undefined, + ); + const entry = result!.get(Constants.EXECUTE_CODE) as CodeSessionContext; + expect(entry.files!.map((f) => f.id)).toEqual(['id-A']); + }); + it('seeds only the requested code-session partition', () => { const statefulKey = 'execute_code:stateful:v1:user'; const existing: ToolSessionMap = new Map(); diff --git a/packages/api/src/agents/codeFilesSession.ts b/packages/api/src/agents/codeFilesSession.ts index fb3962e370..216fc66e47 100644 --- a/packages/api/src/agents/codeFilesSession.ts +++ b/packages/api/src/agents/codeFilesSession.ts @@ -1,4 +1,5 @@ import { Constants } from '@librechat/agents'; +import { logger } from '@librechat/data-schemas'; import type { FileRefs, CodeEnvFile, ToolSessionMap, CodeSessionContext } from '@librechat/agents'; import type { StatefulCodeEnvironment } from 'librechat-data-provider'; import { @@ -6,6 +7,7 @@ import { resolveCodeExecutionContext, type CodeExecutionContext, } from './execution'; +import { createCodeDestinationSet, reserveCodeDestination } from '~/files/code/destinations'; /** * Minimal shape for an agent that may contribute primed code files to its @@ -125,6 +127,15 @@ export function collectCodeExecutionProfileRoutes( * dedupe `_injected_files` would grow proportionally to agent count and * inflate every `/exec` POST. First-seen wins so the original ordering / * source is preserved. + * + * Identity dedupe alone cannot keep the seed valid: codeapi rejects the + * whole `/exec` request when two entries mount at one destination, and a + * file re-uploaded by one agent but cache-hit by another arrives twice + * under different `storage_session_id`s with a single `name`. Sources are + * merged in trust order — skill seed, then primary agent, then the rest — + * so first-seen also wins the destination, and the later copy of an + * already-mounted name is dropped rather than renamed to a path nothing + * told the model about. */ export function seedCodeFilesIntoSessions( files: CodeEnvFile[] | undefined, @@ -139,16 +150,26 @@ export function seedCodeFilesIntoSessions( const prior = sessions.get(sessionKey) as CodeSessionContext | undefined; /** - * Compose `(storage_session_id, id)` as a stable identity. `name` alone - * isn't sufficient — two distinct primed uploads can share a filename - * (different storage sessions, different file_ids). The composite stays - * cheap to compute and the keys are short uuids. + * Identity is `(storage_session_id, id)`, not `name` — two distinct primed + * uploads can share a filename across different storage sessions and + * file_ids, and collapsing those would drop a file the caller resolved to + * its own mount path. The composite stays cheap to compute and the keys + * are short uuids. Destination uniqueness is enforced separately below, + * against the sandbox's one-file-per-path constraint. */ const seenKeys = new Set(); + const destinations = createCodeDestinationSet(); const mergedFiles: FileRefs = []; const pushIfFresh = (f: { id?: string; storage_session_id?: string; name?: string }): void => { const key = `${f.storage_session_id ?? ''}\0${f.id ?? ''}`; if (seenKeys.has(key)) return; + if (f.name != null && !reserveCodeDestination(destinations, f.name)) { + logger.debug( + `[seedCodeFilesIntoSessions] dropped id=${f.id} name=${f.name} ` + + `reason=destination-taken sessionKey=${sessionKey}`, + ); + return; + } seenKeys.add(key); mergedFiles.push(f as FileRefs[number]); }; diff --git a/packages/api/src/files/code/destinations.spec.ts b/packages/api/src/files/code/destinations.spec.ts new file mode 100644 index 0000000000..2976dbafaa --- /dev/null +++ b/packages/api/src/files/code/destinations.spec.ts @@ -0,0 +1,188 @@ +import { + claimCodeDestination, + reserveCodeDestination, + createCodeDestinationSet, + sortCodeFilesByDestinationPriority, +} from './destinations'; + +describe('claimCodeDestination', () => { + it('returns the name untouched when nothing has claimed it', () => { + const set = createCodeDestinationSet(); + expect(claimCodeDestination(set, 'image.png')).toBe('image.png'); + }); + + it('disambiguates repeats with a counter, keeping the extension', () => { + const set = createCodeDestinationSet(); + expect(claimCodeDestination(set, 'image.png')).toBe('image.png'); + expect(claimCodeDestination(set, 'image.png')).toBe('image-2.png'); + expect(claimCodeDestination(set, 'image.png')).toBe('image-3.png'); + }); + + it('preserves directory structure when disambiguating', () => { + const set = createCodeDestinationSet(); + claimCodeDestination(set, 'out/plots/fig.png'); + expect(claimCodeDestination(set, 'out/plots/fig.png')).toBe('out/plots/fig-2.png'); + }); + + it('appends to names without an extension', () => { + const set = createCodeDestinationSet(); + claimCodeDestination(set, 'README'); + expect(claimCodeDestination(set, 'README')).toBe('README-2'); + }); + + it('treats a leading dot as part of the name, not an extension', () => { + const set = createCodeDestinationSet(); + claimCodeDestination(set, '_.env'); + expect(claimCodeDestination(set, '_.env')).toBe('_-2.env'); + }); + + it('skips a counter that a real filename already occupies', () => { + const set = createCodeDestinationSet(); + expect(claimCodeDestination(set, 'image.png')).toBe('image.png'); + expect(claimCodeDestination(set, 'image-2.png')).toBe('image-2.png'); + expect(claimCodeDestination(set, 'image.png')).toBe('image-3.png'); + }); + + it('rejects a destination that a claimed directory prefix would swallow', () => { + const set = createCodeDestinationSet(); + claimCodeDestination(set, 'data/rows.csv'); + expect(claimCodeDestination(set, 'data')).toBe('data-2'); + }); + + it('flattens a destination nested under a claimed file', () => { + const set = createCodeDestinationSet(); + claimCodeDestination(set, 'data'); + expect(claimCodeDestination(set, 'data/rows.csv')).toBe('data__rows.csv'); + }); + + it('still disambiguates once a nested destination has been flattened', () => { + const set = createCodeDestinationSet(); + claimCodeDestination(set, 'data'); + claimCodeDestination(set, 'data__rows.csv'); + expect(claimCodeDestination(set, 'data/rows.csv')).toBe('data__rows-2.csv'); + }); + + it('keeps a disambiguated leaf inside the per-segment byte budget', () => { + const set = createCodeDestinationSet(); + const longName = `${'a'.repeat(251)}.png`; + expect(Buffer.byteLength(longName, 'utf8')).toBe(255); + claimCodeDestination(set, longName); + const second = claimCodeDestination(set, longName); + expect(second).not.toBe(longName); + expect(Buffer.byteLength(second, 'utf8')).toBeLessThanOrEqual(255); + expect(second.endsWith('-2.png')).toBe(true); + }); + + /** + * The counter search only terminates while trimming a name to the byte cap + * leaves the counter intact. An earlier cut-from-the-end trim handed back + * the original name for every counter and hung the request thread, so this + * asserts distinctness at the cap rather than trusting the composition. + */ + it('keeps counters distinct at the byte cap across repeated collisions', () => { + const set = createCodeDestinationSet(); + const longName = `${'a'.repeat(251)}.png`; + const claimed = new Set(); + for (let i = 0; i < 12; i++) { + const destination = claimCodeDestination(set, longName); + expect(Buffer.byteLength(destination, 'utf8')).toBeLessThanOrEqual(255); + expect(claimed.has(destination)).toBe(false); + claimed.add(destination); + } + expect(claimed.size).toBe(12); + }); + + /** + * A name that arrives over budget is passed through untouched — capping + * every name would rewrite paths the caller never had a collision on. Only + * the disambiguated forms are held to the cap, and they must stay distinct + * even when the extension leaves no stem to trim. + */ + it('keeps counters distinct when the extension consumes the whole budget', () => { + const set = createCodeDestinationSet(); + const longExtension = `_.${'b'.repeat(260)}`; + expect(claimCodeDestination(set, longExtension)).toBe(longExtension); + + const claimed = new Set(); + for (let i = 0; i < 5; i++) { + const destination = claimCodeDestination(set, longExtension); + expect(Buffer.byteLength(destination, 'utf8')).toBeLessThanOrEqual(255); + expect(claimed.has(destination)).toBe(false); + claimed.add(destination); + } + expect(claimed.size).toBe(5); + }); +}); + +describe('reserveCodeDestination', () => { + it('accepts a free name and refuses the repeat', () => { + const set = createCodeDestinationSet(); + expect(reserveCodeDestination(set, 'image.png')).toBe(true); + expect(reserveCodeDestination(set, 'image.png')).toBe(false); + }); + + it('refuses either direction of a directory-prefix conflict', () => { + const nested = createCodeDestinationSet(); + reserveCodeDestination(nested, 'data/rows.csv'); + expect(reserveCodeDestination(nested, 'data')).toBe(false); + + const flat = createCodeDestinationSet(); + reserveCodeDestination(flat, 'data'); + expect(reserveCodeDestination(flat, 'data/rows.csv')).toBe(false); + }); + + it('leaves unrelated names alone', () => { + const set = createCodeDestinationSet(); + reserveCodeDestination(set, 'data/rows.csv'); + expect(reserveCodeDestination(set, 'database.csv')).toBe(true); + expect(reserveCodeDestination(set, 'data/other.csv')).toBe(true); + }); + + it('does not reserve a name it refused', () => { + const set = createCodeDestinationSet(); + reserveCodeDestination(set, 'data'); + expect(reserveCodeDestination(set, 'data/rows.csv')).toBe(false); + expect(claimCodeDestination(set, 'data/rows.csv')).toBe('data__rows.csv'); + }); +}); + +describe('sortCodeFilesByDestinationPriority', () => { + it('puts the newest record first so it keeps the bare name', () => { + const older = { file_id: 'a', createdAt: new Date('2026-01-01T00:00:00Z') }; + const newer = { file_id: 'b', createdAt: new Date('2026-02-01T00:00:00Z') }; + expect(sortCodeFilesByDestinationPriority([older, newer])).toEqual([newer, older]); + }); + + it('accepts serialized dates and epoch millis', () => { + const older = { file_id: 'a', createdAt: '2026-01-01T00:00:00.000Z' }; + const newer = { file_id: 'b', createdAt: Date.parse('2026-02-01T00:00:00.000Z') }; + expect(sortCodeFilesByDestinationPriority([older, newer])).toEqual([newer, older]); + }); + + it('breaks ties on file_id so the order is stable across turns', () => { + const createdAt = new Date('2026-01-01T00:00:00Z'); + const first = { file_id: 'aaa', createdAt }; + const second = { file_id: 'bbb', createdAt }; + expect(sortCodeFilesByDestinationPriority([second, first])).toEqual([first, second]); + expect(sortCodeFilesByDestinationPriority([first, second])).toEqual([first, second]); + }); + + it('sorts records without a timestamp last rather than dropping them', () => { + const dated = { file_id: 'a', createdAt: new Date('2026-01-01T00:00:00Z') }; + const undatedEarly = { file_id: 'b' }; + const unparsable = { file_id: 'c', createdAt: 'not-a-date' }; + expect(sortCodeFilesByDestinationPriority([undatedEarly, unparsable, dated])).toEqual([ + dated, + undatedEarly, + unparsable, + ]); + }); + + it('does not mutate its input', () => { + const older = { file_id: 'a', createdAt: new Date('2026-01-01T00:00:00Z') }; + const newer = { file_id: 'b', createdAt: new Date('2026-02-01T00:00:00Z') }; + const input = [older, newer]; + sortCodeFilesByDestinationPriority(input); + expect(input).toEqual([older, newer]); + }); +}); diff --git a/packages/api/src/files/code/destinations.ts b/packages/api/src/files/code/destinations.ts new file mode 100644 index 0000000000..f55a3b1efc --- /dev/null +++ b/packages/api/src/files/code/destinations.ts @@ -0,0 +1,164 @@ +import { appendLeafSuffix, flattenArtifactPath, FILENAME_SEGMENT_MAX_BYTES } from '~/utils/files'; + +/** + * Sandbox input files mount at a destination derived from their `name`, and + * codeapi rejects the whole `/exec` request when two entries resolve to the + * same destination — or when one destination is a directory prefix of + * another (`files contains duplicate destination "x"`, `files contains + * conflicting destinations "a" and "a/b"`). A rejection is fatal for the + * rest of the conversation: the request never reaches the sandbox, so no + * result comes back to collapse the colliding refs, and every later turn + * re-primes the same pair. + * + * Nothing about a LibreChat file record guarantees that uniqueness. Only + * code-generated outputs are covered by the `(filename, conversationId, + * context, tenantId)` partial unique index; user uploads carry + * `context: message_attachment`, so one conversation can hold several + * records sharing a `filename` — two uploads of `image.png`, or an upload + * whose name a later execution wrote back in place. + * + * This module owns the name → destination mapping so every contributor to + * an `/exec` file list agrees on it. + */ +export interface CodeDestinationSet { + readonly names: Set; + readonly ancestors: Set; +} + +export function createCodeDestinationSet(): CodeDestinationSet { + return { names: new Set(), ancestors: new Set() }; +} + +function collectAncestors(destination: string): string[] { + const segments = destination.split('/'); + const ancestors: string[] = []; + for (let i = 1; i < segments.length; i++) { + ancestors.push(segments.slice(0, i).join('/')); + } + return ancestors; +} + +/** True when a claimed file sits at one of `destination`'s parent + * directories, so every name under that directory conflicts. */ +function hasTakenAncestor(set: CodeDestinationSet, destination: string): boolean { + const ancestors = collectAncestors(destination); + for (let i = 0; i < ancestors.length; i++) { + if (set.names.has(ancestors[i])) { + return true; + } + } + return false; +} + +/** Mirrors codeapi's `validateExecuteFiles` conflict rule: an exact match, or + * either destination being a directory prefix of the other. */ +function isTaken(set: CodeDestinationSet, destination: string): boolean { + if (set.names.has(destination) || set.ancestors.has(destination)) { + return true; + } + return hasTakenAncestor(set, destination); +} + +function take(set: CodeDestinationSet, destination: string): void { + set.names.add(destination); + const ancestors = collectAncestors(destination); + for (let i = 0; i < ancestors.length; i++) { + set.ancestors.add(ancestors[i]); + } +} + +/** + * Inserts `-n` before the leaf's extension, preserving directory structure + * (`a/b/c.txt` -> `a/b/c-2.txt`). The leaf is held to the same per-segment + * byte budget `sanitizeFilename` applies, since codeapi caps whole-path + * length and a name already sitting at the cap would otherwise grow past it. + * The counter survives that trim, so distinct counters stay distinct and the + * search below always terminates. + */ +function withCounter(destination: string, counter: number): string { + const slash = destination.lastIndexOf('/'); + const dir = slash === -1 ? '' : destination.slice(0, slash + 1); + const leaf = destination.slice(slash + 1); + return `${dir}${appendLeafSuffix(leaf, `-${counter}`, FILENAME_SEGMENT_MAX_BYTES)}`; +} + +/** + * Claims a destination for `name`, disambiguating with a `-2`, `-3`, ... + * counter when it is already spoken for. Callers that own what the model is + * told about the sandbox use this: both files stay reachable, at names the + * caller can echo into the tool context. + * + * Deterministic for a given claim order, so callers must claim in a stable + * order — see {@link sortCodeFilesByDestinationPriority}. Names beyond the + * first collision on a shared stem can still shift as the colliding set + * grows; that is inherent to mounting by name, and only the already + * degenerate three-way case reaches it. + */ +export function claimCodeDestination(set: CodeDestinationSet, name: string): string { + /* A claimed file sitting at a parent directory makes every name beneath it + * conflict, so bumping the leaf can never clear it. Flattening lifts the + * path out from under that directory and leaves only whole-name conflicts, + * which the counter does clear. */ + const base = hasTakenAncestor(set, name) + ? flattenArtifactPath(name, FILENAME_SEGMENT_MAX_BYTES) + : name; + let destination = base; + for (let counter = 2; isTaken(set, destination); counter++) { + destination = withCounter(base, counter); + } + take(set, destination); + return destination; +} + +/** + * Reserves `name` only if it is free, reporting whether the caller may keep + * the file. Merge points use this rather than {@link claimCodeDestination}: + * a renamed destination there would be one the model was never told about, + * and a conflict at that layer means the same logical file arrived twice + * under different storage pointers, so dropping the later copy loses + * nothing. + */ +export function reserveCodeDestination(set: CodeDestinationSet, name: string): boolean { + if (isTaken(set, name)) { + return false; + } + take(set, name); + return true; +} + +interface CodeDestinationCandidate { + file_id?: string; + createdAt?: Date | string | number; +} + +function toTime(value: Date | string | number | undefined): number { + if (value == null) { + return 0; + } + const time = value instanceof Date ? value.getTime() : new Date(value).getTime(); + return Number.isFinite(time) ? time : 0; +} + +/** + * Orders files so the newest record claims the bare name and older ones take + * the counter. Newest-wins matches `ToolNode.updateCodeSession`, which + * collapses by name the same way once results come back, and it is the right + * answer when an execution rewrote an uploaded file in place: the model's + * next read of that path gets its own edit, not the superseded original. + * + * Ordering is keyed on `createdAt` because it never moves. `updatedAt` — the + * default `getFiles` sort — is bumped by usage accounting and by re-upload, + * so destinations keyed on it would shuffle between turns and silently + * repoint paths that code written in an earlier turn still reads. + */ +export function sortCodeFilesByDestinationPriority( + files: T[], +): T[] { + return [...files].sort((a, b) => { + const delta = toTime(b.createdAt) - toTime(a.createdAt); + if (delta !== 0) { + return delta; + } + return (a.file_id ?? '').localeCompare(b.file_id ?? ''); + }); +} diff --git a/packages/api/src/files/code/index.ts b/packages/api/src/files/code/index.ts index 27f4d24dc7..d24bd420a4 100644 --- a/packages/api/src/files/code/index.ts +++ b/packages/api/src/files/code/index.ts @@ -1,4 +1,5 @@ export * from './classify'; +export * from './destinations'; export * from './extract'; export * from './form'; export * from './identity'; diff --git a/packages/api/src/utils/files.ts b/packages/api/src/utils/files.ts index 88e539e8b8..7520677b46 100644 --- a/packages/api/src/utils/files.ts +++ b/packages/api/src/utils/files.ts @@ -11,7 +11,7 @@ const USER_FACING_UPLOAD_ERRORS = [ const ASCII_FILENAME_SAFE_PATTERN = /^[a-zA-Z0-9._-]$/; const UNSAFE_UNICODE_FILENAME_PATTERN = /[^\p{L}\p{M}\p{N}\p{Emoji}\u200d._-]/gu; -const FILENAME_SEGMENT_MAX_BYTES = 255; +export const FILENAME_SEGMENT_MAX_BYTES = 255; function sanitizeFilenameSegment(segment: string): string { const asciiSanitized = Array.from(segment.normalize('NFC'), (char) => { @@ -63,6 +63,25 @@ function truncateLeafWithSuffix(leaf: string, suffix: string, maxBytes: number): return truncateUtf8Bytes(stem, stemBudget) + suffix + ext; } +/** + * Composes a leaf as ``, trimming the stem so the result + * stays inside `maxBytes`. Unlike {@link truncateLeafWithSuffix}, whose suffix + * marks a truncation and is therefore only applied when one happens, this + * always applies the suffix — callers use it to disambiguate one name from + * another, so a trim that dropped the suffix would hand back the name they + * were trying to move away from. + */ +export function appendLeafSuffix(leaf: string, suffix: string, maxBytes: number): string { + const ext = path.extname(leaf); + const stem = path.basename(leaf, ext); + const suffixBytes = utf8ByteLength(suffix); + const extBytes = utf8ByteLength(ext); + if (extBytes + suffixBytes >= maxBytes) { + return truncateUtf8Bytes(`${stem}${suffix}${ext}`, maxBytes); + } + return `${truncateUtf8Bytes(stem, maxBytes - extBytes - suffixBytes)}${suffix}${ext}`; +} + /** * Resolves a user-facing error message from a file upload error. * When redaction is enabled, maps known error patterns to fixed messages