From b0f61d06fefe81feba2a1efe282a524824a9afb5 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 31 Aug 2026 23:46:45 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=97=82=EF=B8=8F=20fix:=20Resolve=20Code?= =?UTF-8?q?=20Interpreter=20Destination=20Collisions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codeapi mounts each `/exec` input at a destination derived from its `name` and rejects the whole request when two entries land on one. LibreChat never enforced that: `primeFiles` sent `name: file.filename` verbatim, and the graph seed deduped by `(storage_session_id, id)` — identity, not destination. Two conversation files sharing a filename therefore killed code execution for the rest of the conversation. The rejected request never reaches the sandbox, so no result comes back to collapse the pair, and every later turn re-primes both and fails identically. Only code-generated outputs are covered by the `(filename, conversationId, context, tenantId)` partial unique index; 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. `packages/api/src/files/code/destinations.ts` now owns the name → destination mapping for every contributor to an `/exec` file list, mirroring codeapi's conflict rule including directory-prefix conflicts: - `primeFiles` claims per pushed file, disambiguating with a `-2`, `-3` counter and echoing the resolved path into the tool context, so the model is told where each file actually mounts. Claim order comes from immutable `createdAt`, not `getFiles`'s `updatedAt` sort, which usage accounting and re-upload both bump — order-dependent destinations would silently repoint a path an earlier turn had already handed the model. Newest wins the bare name, matching `ToolNode.updateCodeSession` and giving an in-place rewrite precedence over the original it superseded. - `seedCodeFilesIntoSessions` drops a later file whose destination is already claimed. Renaming there would invent a path nothing told the model about, and a conflict at that layer means one logical file arrived twice under different storage pointers. Fixes #15443 --- api/server/services/Files/Code/process.js | 32 ++- .../services/Files/Code/process.spec.js | 188 ++++++++++++++++++ .../api/src/agents/codeFilesSession.spec.ts | 54 ++++- packages/api/src/agents/codeFilesSession.ts | 29 ++- .../api/src/files/code/destinations.spec.ts | 188 ++++++++++++++++++ packages/api/src/files/code/destinations.ts | 164 +++++++++++++++ packages/api/src/files/code/index.ts | 1 + packages/api/src/utils/files.ts | 21 +- 8 files changed, 660 insertions(+), 17 deletions(-) create mode 100644 packages/api/src/files/code/destinations.spec.ts create mode 100644 packages/api/src/files/code/destinations.ts 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