diff --git a/api/server/services/Files/Code/process.js b/api/server/services/Files/Code/process.js index 6bb352e011..311d1c76cd 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,26 @@ const getPreviewContextSuffix = (file) => { : ' (preview unavailable)'; }; -const getVisibleCodeFileContextLine = (file, agentResourceIds) => { - if (file.context === FileContext.execute_code) { +/** + * A generated output is normally left out — the model already knows what it + * wrote. That only holds while the file is still where it wrote it: once a + * newer same-named file takes the bare path, the output mounts under a + * suffixed name the model has never seen, and silence would leave it reading + * the newcomer or failing to find its own artifact. + */ +const getVisibleCodeFileContextLine = (file, agentResourceIds, destination) => { + const displaced = destination !== file.filename; + if (file.context === FileContext.execute_code && !displaced) { return ''; } - const fileSuffix = agentResourceIds.has(file.file_id) ? '' : ' (attached by user)'; - return `\n\t- /mnt/data/${file.filename}${fileSuffix}${getPreviewContextSuffix(file)}`; + const origin = + file.context === FileContext.execute_code + ? ` (written earlier as ${file.filename})` + : `${agentResourceIds.has(file.file_id) ? '' : ' (attached by user)'}${ + displaced ? ` (uploaded as ${file.filename})` : '' + }`; + return `\n\t- /mnt/data/${destination}${origin}${getPreviewContextSuffix(file)}`; }; const appendVisibleCodeFileContext = (toolContext, contextLine) => { @@ -1163,6 +1179,17 @@ 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. `file_ids` are + * this agent's own resources; every other candidate came from the + * conversation and is therefore seen by every agent in the run, so shared + * files rank first and land on the same destination whichever agent primes + * them. */ + const orderedFiles = sortCodeFilesByDestinationPriority(dbFiles, agentResourceIds); + 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 +1198,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 +1232,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, file.file_id); + 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 +1256,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..89e56d584e 100644 --- a/api/server/services/Files/Code/process.spec.js +++ b/api/server/services/Files/Code/process.spec.js @@ -113,6 +113,42 @@ 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, the hashed suffix — lives in + * `packages/api/src/files/code/destinations.ts` under its own + * `destinations.spec.ts`; these stubs carry just enough of its shape + * (an identity-derived suffix, shared-then-newest ordering) for the + * `primeFiles` tests to assert that it is wired to `name` and to the tool + * context at all. The suffix here is the raw identity rather than a + * digest so the expectations below read as names. */ + createCodeDestinationSet: () => new Set(), + claimCodeDestination: (set, name, identity) => { + const dot = name.lastIndexOf('.'); + const stem = dot > 0 ? name.slice(0, dot) : name; + const extension = dot > 0 ? name.slice(dot) : ''; + let destination = name; + for (let counter = 1; set.has(destination); counter++) { + const tail = counter === 1 ? '' : `-${counter}`; + destination = `${stem}-${identity}${tail}${extension}`; + } + set.add(destination); + return destination; + }, + sortCodeFilesByDestinationPriority: (files, privateFileIds) => { + const isPrivate = (file) => (privateFileIds?.has(file?.file_id) ? 1 : 0); + const contentTime = (file) => + Math.max( + file?.metadata?.sourceDispatchedAt ?? 0, + new Date(file?.createdAt ?? 0).getTime() || 0, + ); + return [...files].sort((a, b) => { + const scope = isPrivate(a) - isPrivate(b); + if (scope !== 0) return scope; + const delta = contentTime(b) - contentTime(a); + return delta !== 0 ? delta : (a?.file_id ?? '').localeCompare(b?.file_id ?? ''); + }); + }, }; }); @@ -2847,4 +2883,276 @@ 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, + sourceDispatchedAt, + }) => ({ + file_id, + filename, + createdAt, + context: context ?? 'message_attachment', + source: 'local', + filepath: `/uploads/${file_id}`, + metadata: { + ...(sourceDispatchedAt != null ? { sourceDispatchedAt } : {}), + 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 = (tool_resources) => + primeFiles({ + req: { user: { id: 'user-123', role: 'USER' } }, + tool_resources: tool_resources ?? { execute_code: { file_ids: ['older', 'newer'] } }, + agentId: 'agent-id', + }); + + const bySession = (result) => + Object.fromEntries(result.files.map((f) => [f.storage_session_id, f.name])); + + 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(bySession({ files })).toEqual({ + 'sess-newer': 'image.png', + 'sess-older': 'image-older.png', + }); + expect(toolContext).toContain('/mnt/data/image.png'); + /* The displaced file is advertised at the path it actually mounts on, + * alongside the name the user knows it by. */ + expect(toolContext).toContain('/mnt/data/image-older.png'); + expect(toolContext).toContain('(uploaded as image.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(); + + expect(bySession(ascending)).toEqual(bySession(descending)); + expect(bySession(ascending)).toEqual({ + 'sess-newer': 'image.png', + 'sess-older': 'image-older.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(bySession({ files })).toEqual({ + 'sess-output': 'data.csv', + 'sess-upload': 'data-older.csv', + }); + /* The output keeps the path it wrote, so it stays out of the context + * exactly as an undisplaced generated file does. */ + expect(toolContext).toContain('/mnt/data/data-older.csv'); + expect(toolContext).not.toContain('/mnt/data/data.csv'); + }); + + it('advertises a generated output that a newer upload displaced', async () => { + /** + * The model only knows it wrote `/mnt/data/report.png`. Once a newer + * upload takes that path, silence would leave it reading the upload or + * failing to find its own artifact. + */ + setupActiveSessions(); + getFiles.mockResolvedValue([ + codeFile({ + file_id: 'older', + filename: 'report.png', + storage_session_id: 'sess-output', + createdAt: new Date('2026-01-01T00:00:00Z'), + context: 'execute_code', + }), + codeFile({ + file_id: 'newer', + filename: 'report.png', + storage_session_id: 'sess-upload', + createdAt: new Date('2026-01-02T00:00:00Z'), + }), + ]); + + const { toolContext } = await prime(); + + expect(toolContext).toContain('/mnt/data/report-older.png (written earlier as report.png)'); + }); + + it('ranks a rewritten output above an upload created after it', async () => { + setupActiveSessions(); + getFiles.mockResolvedValue([ + codeFile({ + file_id: 'older', + filename: 'data.csv', + storage_session_id: 'sess-output', + createdAt: new Date('2026-01-01T00:00:00Z'), + context: 'execute_code', + sourceDispatchedAt: Date.parse('2026-03-01T00:00:00Z'), + }), + codeFile({ + file_id: 'newer', + filename: 'data.csv', + storage_session_id: 'sess-upload', + createdAt: new Date('2026-02-01T00:00:00Z'), + }), + ]); + + const { files } = await prime(); + + expect(bySession({ files })).toEqual({ + 'sess-output': 'data.csv', + 'sess-upload': 'data-newer.csv', + }); + }); + + it("lets a conversation file outrank the agent's own file of the same name", async () => { + /** + * Every agent in a run primes the conversation's files plus its own, so + * a private file taking the bare path in one agent and not in another + * would leave two agents advertising different paths for the same + * shared file into one mount namespace. + */ + setupActiveSessions(); + getFiles.mockResolvedValue([ + codeFile({ + file_id: 'shared', + filename: 'data.csv', + storage_session_id: 'sess-shared', + createdAt: new Date('2026-01-01T00:00:00Z'), + }), + codeFile({ + file_id: 'agent-own', + filename: 'data.csv', + storage_session_id: 'sess-agent', + createdAt: new Date('2026-06-01T00:00:00Z'), + }), + ]); + + const { files } = await prime({ + execute_code: { + file_ids: ['agent-own'], + files: [{ file_id: 'shared' }], + }, + }); + + expect(bySession({ files })).toEqual({ + 'sess-shared': 'data.csv', + 'sess-agent': 'data-agent-own.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'); + expect(toolContext).not.toContain('uploaded as'); + }); + }); }); 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..c1ce5ae3ce 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,27 @@ 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. + * + * Each agent resolves destinations over its own candidate set, so this can + * still drop a genuinely distinct file: two agents whose *private* + * resources share a filename each claim the bare name locally, and only the + * first survives here while the second agent's tool context keeps + * advertising its own file at that path. `sortCodeFilesByDestinationPriority` + * ranks conversation-scoped files above private ones precisely so the shared + * majority cannot diverge that way; closing the private-versus-private case + * needs one assignment across contributors, which means resolving + * destinations before any agent renders its tool context. Until then this + * drop is the failure floor — before it, the pair reached codeapi together + * and took the whole run down with a rejected request. */ export function seedCodeFilesIntoSessions( files: CodeEnvFile[] | undefined, @@ -139,16 +162,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..b5e3abb374 --- /dev/null +++ b/packages/api/src/files/code/destinations.spec.ts @@ -0,0 +1,307 @@ +import { + claimCodeDestination, + reserveCodeDestination, + createCodeDestinationSet, + sortCodeFilesByDestinationPriority, +} from './destinations'; +import { deterministicHexSuffix } from '~/utils/files'; + +/** The suffix the implementation derives for a displaced file, spelled the + * same way so the expectations read as names rather than as digests. */ +const suffixFor = (identity: string): string => `-${deterministicHexSuffix(identity)}`; + +describe('claimCodeDestination', () => { + it('returns the name untouched when nothing has claimed it', () => { + const set = createCodeDestinationSet(); + expect(claimCodeDestination(set, 'image.png', 'file-a')).toBe('image.png'); + }); + + it('moves a displaced file onto its identity suffix, keeping the extension', () => { + const set = createCodeDestinationSet(); + expect(claimCodeDestination(set, 'image.png', 'file-a')).toBe('image.png'); + expect(claimCodeDestination(set, 'image.png', 'file-b')).toBe( + `image${suffixFor('file-b')}.png`, + ); + expect(claimCodeDestination(set, 'image.png', 'file-c')).toBe( + `image${suffixFor('file-c')}.png`, + ); + }); + + /** + * The reason the suffix is not a collision counter. With `-2`, `-3`, a + * third file arriving under the literal name a displaced file had been + * given would push that file along, and code the model wrote in an earlier + * turn would silently start reading the newcomer. + */ + it('gives a displaced file the same destination whatever else is present', () => { + const alone = createCodeDestinationSet(); + claimCodeDestination(alone, 'image.png', 'winner'); + const withoutNewcomer = claimCodeDestination(alone, 'image.png', 'displaced'); + + const crowded = createCodeDestinationSet(); + claimCodeDestination(crowded, 'image.png', 'winner'); + claimCodeDestination(crowded, 'image-2.png', 'newcomer'); + claimCodeDestination(crowded, `image${suffixFor('unrelated')}.png`, 'unrelated'); + const withNewcomer = claimCodeDestination(crowded, 'image.png', 'displaced'); + + expect(withNewcomer).toBe(withoutNewcomer); + }); + + /** + * The one case where a displaced file does move, and why that is right. + * The model is told the displaced file is at `-.png`, rewrites + * it in place, and `processCodeOutput` registers the output under that + * literal name. Next turn the output is the newest holder of that path, so + * it keeps it and the superseded original steps aside — the model's + * familiar path resolves to its own edit rather than to the bytes it + * replaced. + */ + it('leaves an alias with the later file that rewrote it, not the superseded original', () => { + const firstTurn = createCodeDestinationSet(); + claimCodeDestination(firstTurn, 'image.png', 'newer-upload'); + const alias = claimCodeDestination(firstTurn, 'image.png', 'original'); + expect(alias).toBe(`image${suffixFor('original')}.png`); + + const secondTurn = createCodeDestinationSet(); + const rewrite = claimCodeDestination(secondTurn, alias, 'rewrite-output'); + claimCodeDestination(secondTurn, 'image.png', 'newer-upload'); + const originalNow = claimCodeDestination(secondTurn, 'image.png', 'original'); + + expect(rewrite).toBe(alias); + expect(originalNow).toBe(`image${suffixFor('original')}-2.png`); + }); + + it('preserves directory structure when disambiguating', () => { + const set = createCodeDestinationSet(); + claimCodeDestination(set, 'out/plots/fig.png', 'file-a'); + expect(claimCodeDestination(set, 'out/plots/fig.png', 'file-b')).toBe( + `out/plots/fig${suffixFor('file-b')}.png`, + ); + }); + + it('appends to names without an extension', () => { + const set = createCodeDestinationSet(); + claimCodeDestination(set, 'README', 'file-a'); + expect(claimCodeDestination(set, 'README', 'file-b')).toBe(`README${suffixFor('file-b')}`); + }); + + it('treats a leading dot as part of the name, not an extension', () => { + const set = createCodeDestinationSet(); + claimCodeDestination(set, '_.env', 'file-a'); + expect(claimCodeDestination(set, '_.env', 'file-b')).toBe(`_${suffixFor('file-b')}.env`); + }); + + it('falls back to a counter when two claims share an identity', () => { + const set = createCodeDestinationSet(); + claimCodeDestination(set, 'image.png', 'same'); + expect(claimCodeDestination(set, 'image.png', 'same')).toBe(`image${suffixFor('same')}.png`); + expect(claimCodeDestination(set, 'image.png', 'same')).toBe(`image${suffixFor('same')}-2.png`); + }); + + it('rejects a destination that a claimed directory prefix would swallow', () => { + const set = createCodeDestinationSet(); + claimCodeDestination(set, 'data/rows.csv', 'file-a'); + expect(claimCodeDestination(set, 'data', 'file-b')).toBe(`data${suffixFor('file-b')}`); + }); + + it('flattens a destination nested under a claimed file', () => { + const set = createCodeDestinationSet(); + claimCodeDestination(set, 'data', 'file-a'); + expect(claimCodeDestination(set, 'data/rows.csv', 'file-b')).toBe('data__rows.csv'); + }); + + it('still disambiguates once a nested destination has been flattened', () => { + const set = createCodeDestinationSet(); + claimCodeDestination(set, 'data', 'file-a'); + claimCodeDestination(set, 'data__rows.csv', 'file-b'); + expect(claimCodeDestination(set, 'data/rows.csv', 'file-c')).toBe( + `data__rows${suffixFor('file-c')}.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, 'file-a'); + const second = claimCodeDestination(set, longName, 'file-b'); + expect(second).not.toBe(longName); + expect(Buffer.byteLength(second, 'utf8')).toBeLessThanOrEqual(255); + expect(second.endsWith(`${suffixFor('file-b')}.png`)).toBe(true); + }); + + /** + * The search only terminates while trimming a name to the byte cap leaves + * the suffix intact. An earlier cut-from-the-end trim handed back the + * original name for every attempt and hung the request thread, so this + * asserts distinctness at the cap rather than trusting the composition. + */ + it('keeps destinations 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, `file-${i}`); + 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 destinations distinct when the extension consumes the whole budget', () => { + const set = createCodeDestinationSet(); + const longExtension = `_.${'b'.repeat(260)}`; + expect(claimCodeDestination(set, longExtension, 'file-0')).toBe(longExtension); + + const claimed = new Set(); + for (let i = 1; i < 6; i++) { + const destination = claimCodeDestination(set, longExtension, `file-${i}`); + 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', 'file-b')).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]); + }); + + /** + * `processCodeOutput` claims one row per `(filename, conversationId)` and + * rewrites it in place with `$setOnInsert`, so a repeatedly written output + * keeps its original `createdAt`. Ranking on that alone would hand the bare + * path to an upload the output has since been rewritten over. + */ + it('ranks a rewritten output by its last content write, not its creation', () => { + const output = { + file_id: 'output', + createdAt: new Date('2026-01-01T00:00:00Z'), + metadata: { sourceDispatchedAt: Date.parse('2026-03-01T00:00:00Z') }, + }; + const upload = { file_id: 'upload', createdAt: new Date('2026-02-01T00:00:00Z') }; + expect(sortCodeFilesByDestinationPriority([upload, output])).toEqual([output, upload]); + }); + + it('never lets a stale write stamp drag a record below its creation', () => { + const output = { + file_id: 'output', + createdAt: new Date('2026-03-01T00:00:00Z'), + metadata: { sourceDispatchedAt: Date.parse('2026-01-01T00:00:00Z') }, + }; + const upload = { file_id: 'upload', createdAt: new Date('2026-02-01T00:00:00Z') }; + expect(sortCodeFilesByDestinationPriority([upload, output])).toEqual([output, upload]); + }); + + /** + * Every agent in a run primes the conversation's files plus its own. If a + * private file could outrank a shared one, two agents would advertise + * different paths for the same shared file into one mount namespace. + */ + it('sinks contributor-private files below shared ones regardless of age', () => { + const shared = { file_id: 'shared', createdAt: new Date('2026-01-01T00:00:00Z') }; + const priv = { file_id: 'private', createdAt: new Date('2026-06-01T00:00:00Z') }; + expect(sortCodeFilesByDestinationPriority([priv, shared], new Set(['private']))).toEqual([ + shared, + priv, + ]); + }); + + it('still ranks by recency within each scope', () => { + const sharedOld = { file_id: 's1', createdAt: new Date('2026-01-01T00:00:00Z') }; + const sharedNew = { file_id: 's2', createdAt: new Date('2026-02-01T00:00:00Z') }; + const privOld = { file_id: 'p1', createdAt: new Date('2026-03-01T00:00:00Z') }; + const privNew = { file_id: 'p2', createdAt: new Date('2026-04-01T00:00:00Z') }; + expect( + sortCodeFilesByDestinationPriority( + [privOld, sharedOld, privNew, sharedNew], + new Set(['p1', 'p2']), + ), + ).toEqual([sharedNew, sharedOld, privNew, privOld]); + }); + + 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('preserves holes instead of throwing on them', () => { + const dated = { file_id: 'a', createdAt: new Date('2026-01-01T00:00:00Z') }; + expect(sortCodeFilesByDestinationPriority([null, dated, undefined])).toEqual([ + dated, + null, + undefined, + ]); + }); + + 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..635eb217d1 --- /dev/null +++ b/packages/api/src/files/code/destinations.ts @@ -0,0 +1,222 @@ +import { + appendLeafSuffix, + flattenArtifactPath, + deterministicHexSuffix, + 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 `suffix` before the leaf's extension, preserving directory + * structure (`a/b/c.txt` -> `a/b/c-9f2a11.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 suffix survives that trim, so distinct suffixes stay + * distinct and the search below always terminates. + */ +function withSuffix(destination: string, suffix: string): string { + const slash = destination.lastIndexOf('/'); + const dir = slash === -1 ? '' : destination.slice(0, slash + 1); + const leaf = destination.slice(slash + 1); + return `${dir}${appendLeafSuffix(leaf, suffix, FILENAME_SEGMENT_MAX_BYTES)}`; +} + +/** + * Claims a destination for `name`, falling back to `-` + * 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. + * + * The fallback hashes `identity` — a stable per-file value such as `file_id` + * — rather than counting collisions, so a displaced file keeps the same + * destination for the life of the conversation. A counter would be assigned + * from the current set: a third file arriving under the literal name a + * displaced file had been given would push that file along, and code the + * model wrote in an earlier turn would silently start reading the newcomer. + * The hash does not depend on what else is present, so a file that has been + * told to the model at one path stays there. + * + * One case still moves it, and deliberately: when a *later* file is itself + * named the alias, it takes it and the older file falls to a counter. In + * practice that later file is the model's own in-place rewrite of the + * displaced one — it was told `-.ext`, wrote back to it, and + * `processCodeOutput` registered an output under that literal name — so the + * path the model has been using keeps resolving to the newest content at + * that path, which is the whole point of the recency ordering. Reserving the + * alias against the newcomer instead would hand that path back to the + * superseded original. An *unrelated* file reaching this branch would have + * to be named for the hex digest of another file's `file_id`, which is not + * a name a user can construct. + */ +export function claimCodeDestination( + set: CodeDestinationSet, + name: string, + identity: string, +): string { + /* A claimed file sitting at a parent directory makes every name beneath it + * conflict, so suffixing the leaf can never clear it. Flattening lifts the + * path out from under that directory and leaves only whole-name conflicts, + * which the suffix does clear. */ + const base = hasTakenAncestor(set, name) + ? flattenArtifactPath(name, FILENAME_SEGMENT_MAX_BYTES) + : name; + if (!isTaken(set, base)) { + take(set, base); + return base; + } + const identitySuffix = `-${deterministicHexSuffix(identity)}`; + let destination = withSuffix(base, identitySuffix); + for (let counter = 2; isTaken(set, destination); counter++) { + destination = withSuffix(base, `${identitySuffix}-${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; + /** Last content write for a reused code-output record. `processCodeOutput` + * claims one row per `(filename, conversationId)` and rewrites it in + * place, so `createdAt` marks when the name was first produced, not when + * the bytes behind it last changed. */ + metadata?: { sourceDispatchedAt?: number } | null; +} + +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; +} + +function contentTime(file: CodeDestinationCandidate | null | undefined): number { + return Math.max(toTime(file?.metadata?.sourceDispatchedAt), toTime(file?.createdAt)); +} + +/** + * Orders files so the one holding the newest content claims the bare name and + * the rest take an identity suffix. 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. + * + * Recency is `max(metadata.sourceDispatchedAt, createdAt)`, never `updatedAt`. + * `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. `sourceDispatchedAt` moves only when a generated output's bytes are + * rewritten, which is exactly when the ranking should change. + * + * `privateFileIds` names files that belong to a single contributor rather + * than to the conversation, and sinks them below the shared ones. Each agent + * in a run claims destinations over its own set — the conversation's files + * plus its own — so without this a private file could take a bare name from a + * shared file in one agent and not in another, leaving two agents advertising + * different paths for the same file into one shared mount namespace. + * + * Holes are tolerated and preserved rather than filtered: callers hand this + * a raw query result and keep their own per-entry guard. + */ +export function sortCodeFilesByDestinationPriority( + files: Array, + privateFileIds?: ReadonlySet, +): Array { + const isPrivate = (file: T | null | undefined): number => + privateFileIds != null && file?.file_id != null && privateFileIds.has(file.file_id) ? 1 : 0; + return [...files].sort((a, b) => { + const scope = isPrivate(a) - isPrivate(b); + if (scope !== 0) { + return scope; + } + const delta = contentTime(b) - contentTime(a); + 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..d3e192675b 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 @@ -161,7 +180,7 @@ const ARTIFACT_PATH_TOTAL_MAX_BYTES = 512; * collision becomes likely, vs. the realistic ceiling of single-digit * artifacts per turn). */ -function deterministicHexSuffix(input: string): string { +export function deterministicHexSuffix(input: string): string { return crypto.createHash('sha256').update(input).digest('hex').slice(0, 6); }