From 94fed5bcd532111161e5408bf3f86632b8ebf8db Mon Sep 17 00:00:00 2001 From: "lia-by-librechat[bot]" <328778573+lia-by-librechat[bot]@users.noreply.github.com> Date: Thu, 17 Sep 2026 02:59:25 +0000 Subject: [PATCH 01/27] =?UTF-8?q?=F0=9F=A5=A1=20fix:=20Delete=20Storage=20?= =?UTF-8?q?and=20Embeddings=20With=20Owned=20Agent=20Files=20(#16007)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The agent tool-resource branch of DELETE /files removed a file's association from the agent and answered 200 without reaching processDeleteRequest, so the bytes on disk and the RAG chunks of a file the caller owned survived the delete. A replaced File Search document kept answering from its old chunks (#15930). An attached file now goes through the full delete pass, but only when nothing else holds it: a file the caller does not own, and a file any other (agent, tool_resource) pair still references, are unlinked and left whole. Nothing is unlinked until the destroy it depends on has succeeded. - packages/api/src/files/deletion.ts: deleteAgentResourceFiles owns the operation with its db methods and delete pass injected. - packages/data-schemas: getSharedResourceFileIds answers the last-reference question, excluding the pair being removed by the agent's _id. - Local/crud.js: unlinkFile propagates non-ENOENT, so a file whose bytes survive is no longer reported as deleted. Co-authored-by: Mohammed Alshyakh --- api/server/routes/files/files.js | 37 ++- api/server/routes/files/files.test.js | 239 +++++++++++++++++ .../Files/Local/__tests__/crud-delete.spec.js | 70 +++++ api/server/services/Files/Local/crud.js | 10 + api/server/services/Files/process.js | 20 +- api/server/services/Files/process.spec.js | 83 ++++++ packages/api/src/files/deletion.spec.ts | 247 +++++++++++++++++- packages/api/src/files/deletion.ts | 172 ++++++++++++ .../data-schemas/src/methods/agent.spec.ts | 142 ++++++++++ packages/data-schemas/src/methods/agent.ts | 72 +++++ 10 files changed, 1064 insertions(+), 28 deletions(-) create mode 100644 api/server/services/Files/Local/__tests__/crud-delete.spec.js diff --git a/api/server/routes/files/files.js b/api/server/routes/files/files.js index de84c67c950..f6b97e587f4 100644 --- a/api/server/routes/files/files.js +++ b/api/server/routes/files/files.js @@ -8,6 +8,7 @@ const { refreshS3FileUrls, handleFilesUsageRequest, buildDeleteFilesResponse, + deleteAgentResourceFiles, shouldUseUploadSse, startUploadSseStream, sendUploadPolicyError, @@ -245,20 +246,36 @@ router.delete('/', async (req, res) => { }); } - const toolResourceFiles = agent.tool_resources?.[req.body.tool_resource]?.file_ids ?? []; - const agentFiles = files - .filter((f) => toolResourceFiles.includes(f.file_id)) - .map((file) => ({ tool_resource: req.body.tool_resource, file_id: file.file_id })); - if (agentFiles.length === 0) { + const agentDeletion = await deleteAgentResourceFiles( + { + agentId: req.body.agent_id, + agentObjectId: agent._id.toString(), + toolResource: req.body.tool_resource, + requestedFileIds: fileIds, + attachedFileIds: agent.tool_resources?.[req.body.tool_resource]?.file_ids ?? [], + files: dbFiles.map((file) => ({ + file_id: file.file_id, + owner: file.user?.toString() ?? null, + file, + })), + userId: req.user.id.toString(), + }, + { + getSharedResourceFileIds: db.getSharedResourceFileIds, + removeAgentResourceFiles: db.removeAgentResourceFiles, + deleteFiles: (agentFiles) => processDeleteRequest({ req, files: agentFiles }), + }, + ); + + if (agentDeletion.outcome == null) { res.status(200).json({ message: 'File associations removed successfully from agent' }); return; } - await db.removeAgentResourceFiles({ - agent_id: req.body.agent_id, - files: agentFiles, - }); - res.status(200).json({ message: 'File associations removed successfully from agent' }); + logger.debug( + `[/files] Agent files deleted successfully: ${agentDeletion.destroyedFileIds.join(', ')}`, + ); + sendDeleteResult(agentDeletion.outcome, 'Files deleted successfully'); return; } diff --git a/api/server/routes/files/files.test.js b/api/server/routes/files/files.test.js index 2ad58d350dd..9df2acb0623 100644 --- a/api/server/routes/files/files.test.js +++ b/api/server/routes/files/files.test.js @@ -327,6 +327,245 @@ describe('File Routes - Delete with Agent Access', () => { expect(updatedAgent.tool_resources.file_search.file_ids).toEqual([]); }); + it('deletes storage and embeddings for an attached file the caller owns', async () => { + const ownedFileId = uuidv4(); + await createFile({ + user: otherUserId, + file_id: ownedFileId, + filename: 'owned-knowledge.txt', + filepath: '/uploads/owned-knowledge.txt', + bytes: 100, + type: 'text/plain', + source: FileSources.vectordb, + embedded: true, + }); + + const agent = await createAgent({ + id: uuidv4(), + name: 'Test Agent', + provider: 'openai', + model: 'gpt-4', + author: otherUserId, + tool_resources: { + file_search: { + file_ids: [ownedFileId], + }, + }, + }); + + const response = await request(app) + .delete('/files') + .send({ + agent_id: agent.id, + tool_resource: 'file_search', + files: [{ file_id: ownedFileId, filepath: '/uploads/owned-knowledge.txt' }], + }); + + expect(response.status).toBe(200); + expect(response.body.message).toBe('Files deleted successfully'); + expect(processDeleteRequest).toHaveBeenCalledTimes(1); + + const [{ req, files: deletedFiles }] = processDeleteRequest.mock.calls[0]; + expect(deletedFiles.map((file) => file.file_id)).toEqual([ownedFileId]); + expect(deletedFiles[0].source).toBe(FileSources.vectordb); + expect(req.body.agent_id).toBe(agent.id); + expect(req.body.tool_resource).toBe('file_search'); + }); + + it('unlinks another user’s attached file while deleting the caller’s own', async () => { + const ownedFileId = uuidv4(); + await createFile({ + user: otherUserId, + file_id: ownedFileId, + filename: 'owned-knowledge.txt', + filepath: '/uploads/owned-knowledge.txt', + bytes: 100, + type: 'text/plain', + source: FileSources.vectordb, + embedded: true, + }); + + const agent = await createAgent({ + id: uuidv4(), + name: 'Test Agent', + provider: 'openai', + model: 'gpt-4', + author: otherUserId, + tool_resources: { + file_search: { + file_ids: [ownedFileId, fileId], + }, + }, + }); + + const response = await request(app) + .delete('/files') + .send({ + agent_id: agent.id, + tool_resource: 'file_search', + files: [ + { file_id: ownedFileId, filepath: '/uploads/owned-knowledge.txt' }, + { file_id: fileId, filepath: '/uploads/test.txt' }, + ], + }); + + expect(response.status).toBe(200); + expect(response.body.message).toBe('Files deleted successfully'); + + const [{ files: deletedFiles }] = processDeleteRequest.mock.calls[0]; + expect(deletedFiles.map((file) => file.file_id)).toEqual([ownedFileId]); + + const updatedAgent = await Agent.findOne({ id: agent.id }).lean(); + expect(updatedAgent.tool_resources.file_search.file_ids).toEqual([ownedFileId]); + + const retainedFile = await File.findOne({ file_id: fileId }).lean(); + expect(retainedFile).toBeTruthy(); + }); + + it('keeps a file the same agent holds under another tool resource', async () => { + const sharedFileId = uuidv4(); + await createFile({ + user: otherUserId, + file_id: sharedFileId, + filename: 'dual-purpose.txt', + filepath: '/uploads/dual-purpose.txt', + bytes: 100, + type: 'text/plain', + source: FileSources.vectordb, + embedded: true, + }); + + /* One agent can hold the same file under two resources, so the reference being removed is the + `(agent, tool_resource)` pair rather than the agent. */ + const agent = await createAgent({ + id: uuidv4(), + name: 'Test Agent', + provider: 'openai', + model: 'gpt-4', + author: otherUserId, + tool_resources: { + file_search: { file_ids: [sharedFileId] }, + context: { file_ids: [sharedFileId] }, + }, + }); + + const response = await request(app) + .delete('/files') + .send({ + agent_id: agent.id, + tool_resource: 'file_search', + files: [{ file_id: sharedFileId, filepath: '/uploads/dual-purpose.txt' }], + }); + + expect(response.status).toBe(200); + expect(response.body.message).toBe('File associations removed successfully from agent'); + expect(processDeleteRequest).not.toHaveBeenCalled(); + + const updatedAgent = await Agent.findOne({ id: agent.id }).lean(); + expect(updatedAgent.tool_resources.file_search.file_ids).toEqual([]); + expect(updatedAgent.tool_resources.context.file_ids).toEqual([sharedFileId]); + + const retainedFile = await File.findOne({ file_id: sharedFileId }).lean(); + expect(retainedFile).toBeTruthy(); + }); + + it('keeps a file a duplicated agent still references, unlinking it here only', async () => { + const sharedFileId = uuidv4(); + await createFile({ + user: otherUserId, + file_id: sharedFileId, + filename: 'shared-knowledge.txt', + filepath: '/uploads/shared-knowledge.txt', + bytes: 100, + type: 'text/plain', + source: FileSources.vectordb, + embedded: true, + }); + + const agent = await createAgent({ + id: uuidv4(), + name: 'Test Agent', + provider: 'openai', + model: 'gpt-4', + author: otherUserId, + tool_resources: { file_search: { file_ids: [sharedFileId] } }, + }); + + /* Duplicating an agent copies file_ids rather than the files behind them, and lands them + under `context`, so the second holder is found across tool resources. */ + const duplicate = await createAgent({ + id: uuidv4(), + name: 'Test Agent (copy)', + provider: 'openai', + model: 'gpt-4', + author: otherUserId, + tool_resources: { context: { file_ids: [sharedFileId] } }, + }); + + const response = await request(app) + .delete('/files') + .send({ + agent_id: agent.id, + tool_resource: 'file_search', + files: [{ file_id: sharedFileId, filepath: '/uploads/shared-knowledge.txt' }], + }); + + expect(response.status).toBe(200); + expect(response.body.message).toBe('File associations removed successfully from agent'); + expect(processDeleteRequest).not.toHaveBeenCalled(); + + const updatedAgent = await Agent.findOne({ id: agent.id }).lean(); + expect(updatedAgent.tool_resources.file_search.file_ids).toEqual([]); + + const untouchedDuplicate = await Agent.findOne({ id: duplicate.id }).lean(); + expect(untouchedDuplicate.tool_resources.context.file_ids).toEqual([sharedFileId]); + + const retainedFile = await File.findOne({ file_id: sharedFileId }).lean(); + expect(retainedFile).toBeTruthy(); + }); + + it('leaves an owned file alone when the tool resource does not hold it', async () => { + const ownedFileId = uuidv4(); + await createFile({ + user: otherUserId, + file_id: ownedFileId, + filename: 'detached-knowledge.txt', + filepath: '/uploads/detached-knowledge.txt', + bytes: 100, + type: 'text/plain', + source: FileSources.vectordb, + embedded: true, + }); + + const agent = await createAgent({ + id: uuidv4(), + name: 'Test Agent', + provider: 'openai', + model: 'gpt-4', + author: otherUserId, + tool_resources: { + file_search: { + file_ids: [fileId], + }, + }, + }); + + const response = await request(app) + .delete('/files') + .send({ + agent_id: agent.id, + tool_resource: 'file_search', + files: [{ file_id: ownedFileId, filepath: '/uploads/detached-knowledge.txt' }], + }); + + expect(response.status).toBe(200); + expect(response.body.message).toBe('File associations removed successfully from agent'); + expect(processDeleteRequest).not.toHaveBeenCalled(); + + const updatedAgent = await Agent.findOne({ id: agent.id }).lean(); + expect(updatedAgent.tool_resources.file_search.file_ids).toEqual([fileId]); + }); + it('rejects invalid agent tool_resource values before unlinking', async () => { const agent = await createAgent({ id: uuidv4(), diff --git a/api/server/services/Files/Local/__tests__/crud-delete.spec.js b/api/server/services/Files/Local/__tests__/crud-delete.spec.js new file mode 100644 index 00000000000..7a7a30340ab --- /dev/null +++ b/api/server/services/Files/Local/__tests__/crud-delete.spec.js @@ -0,0 +1,70 @@ +/** Exactly what `deleteLocalFile` reaches for, so the double does not depend on a built package. */ +jest.mock('@librechat/api', () => ({ + deleteRagFile: jest.fn().mockResolvedValue(undefined), + stripCacheBust: (filepath) => String(filepath).split('?')[0], +})); +jest.mock('@librechat/data-schemas', () => ({ + logger: { warn: jest.fn(), error: jest.fn() }, +})); + +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const { deleteLocalFile } = require('../crud'); + +/* The resolved promise of a delete is what `processDeleteRequest` reads to decide that a record may + lose its metadata and its agent references, so this adapter may only resolve once the bytes are + actually gone. Storage that was already missing is the one benign case. */ +describe('deleteLocalFile failure reporting', () => { + const userId = 'user-1'; + let tmpBase; + let req; + + beforeEach(() => { + jest.restoreAllMocks(); + tmpBase = fs.mkdtempSync(path.join(os.tmpdir(), 'crud-delete-')); + fs.mkdirSync(path.join(tmpBase, 'uploads', userId), { recursive: true }); + req = { + user: { id: userId }, + config: { + paths: { + publicPath: path.join(tmpBase, 'public'), + uploads: path.join(tmpBase, 'uploads'), + }, + }, + }; + }); + + afterEach(() => { + fs.rmSync(tmpBase, { recursive: true, force: true }); + }); + + const uploadedFile = (filename) => { + const filepath = path.join(tmpBase, 'uploads', userId, filename); + fs.writeFileSync(filepath, 'contents'); + return { file_id: 'file-1', filepath: `/uploads/${userId}/${filename}` }; + }; + + it('removes the file and resolves', async () => { + const file = uploadedFile('knowledge.txt'); + + await expect(deleteLocalFile(req, file)).resolves.toBeUndefined(); + expect(fs.existsSync(path.join(tmpBase, 'uploads', userId, 'knowledge.txt'))).toBe(false); + }); + + it('resolves when the file is already gone', async () => { + await expect( + deleteLocalFile(req, { file_id: 'file-1', filepath: `/uploads/${userId}/missing.txt` }), + ).resolves.toBeUndefined(); + }); + + it('rejects when the bytes survive the delete', async () => { + const file = uploadedFile('locked.txt'); + jest + .spyOn(fs.promises, 'unlink') + .mockRejectedValue(Object.assign(new Error('permission denied'), { code: 'EACCES' })); + + await expect(deleteLocalFile(req, file)).rejects.toThrow('permission denied'); + expect(fs.existsSync(path.join(tmpBase, 'uploads', userId, 'locked.txt'))).toBe(true); + }); +}); diff --git a/api/server/services/Files/Local/crud.js b/api/server/services/Files/Local/crud.js index fe524550986..9555df49c03 100644 --- a/api/server/services/Files/Local/crud.js +++ b/api/server/services/Files/Local/crud.js @@ -208,11 +208,21 @@ const isValidPath = (req, base, subfolder, filepath) => { /** * @param {string} filepath */ +/** + * A file whose bytes are still on disk must not be reported as deleted: callers use the resolved + * promise to decide that a record may lose its metadata and its agent references. Storage that was + * already gone is the one benign case, and `processDeleteRequest` treats it as deleted by design. + */ const unlinkFile = async (filepath) => { try { await fs.promises.unlink(filepath); } catch (error) { + if (error?.code === 'ENOENT') { + logger.warn('Local file was already missing during delete:', error); + return; + } logger.error('Error deleting file:', error); + throw error; } }; diff --git a/api/server/services/Files/process.js b/api/server/services/Files/process.js index 05b02ae6df6..35117ab248e 100644 --- a/api/server/services/Files/process.js +++ b/api/server/services/Files/process.js @@ -265,16 +265,8 @@ const processDeleteRequest = async ({ req, files }) => { await initializeClients(); } - const agentFiles = []; - for (const file of files) { const source = file.source ?? FileSources.local; - if (req.body.agent_id && req.body.tool_resource) { - agentFiles.push({ - tool_resource: req.body.tool_resource, - file_id: file.file_id, - }); - } if (source === FileSources.text) { resolvedFileIds.add(file.file_id); @@ -313,15 +305,6 @@ const processDeleteRequest = async ({ req, files }) => { }); } - if (agentFiles.length > 0) { - promises.push( - db.removeAgentResourceFiles({ - agent_id: req.body.agent_id, - files: agentFiles, - }), - ); - } - await Promise.allSettled(promises); const deletedFileIds = [...resolvedFileIds]; let metadataDeletedFileIds = deletedFileIds; @@ -334,6 +317,9 @@ const processDeleteRequest = async ({ req, files }) => { metadataDeletedFileIds = []; throw error; } + /* The only place a delete removes agent references, and it runs after the metadata delete + succeeded: a file that kept its storage, its chunks or its record keeps its references too, + so the agent it was removed from can be asked again (see issue #12776). */ if (metadataDeletedFileIds.length > 0) { try { await db.removeAgentResourceFilesFromAllAgents({ file_ids: metadataDeletedFileIds }); diff --git a/api/server/services/Files/process.spec.js b/api/server/services/Files/process.spec.js index efbb8a1b008..33ec48b7861 100644 --- a/api/server/services/Files/process.spec.js +++ b/api/server/services/Files/process.spec.js @@ -2610,6 +2610,89 @@ describe('processDeleteRequest', () => { expect(result).toEqual({ deletedFileIds: [], failedFileIds: ['embedded-file'] }); }); + it('keeps a failed agent file attached so the delete can be retried', async () => { + const deleteFile = jest.fn().mockRejectedValue(new Error('rag unavailable')); + getStrategyFunctions.mockReturnValue({ deleteFile }); + const req = { + body: { agent_id: 'agent_1', tool_resource: 'file_search' }, + config: {}, + user: { id: 'user-123', tenantId: 'tenant-a' }, + }; + + const result = await processDeleteRequest({ + req, + files: [ + { + file_id: 'knowledge-file', + filepath: '/uploads/knowledge.txt', + source: FileSources.local, + }, + ], + }); + + expect(result).toEqual({ deletedFileIds: [], failedFileIds: ['knowledge-file'] }); + expect(db.deleteFiles).not.toHaveBeenCalled(); + expect(db.removeAgentResourceFiles).not.toHaveBeenCalled(); + expect(db.removeAgentResourceFilesFromAllAgents).not.toHaveBeenCalled(); + }); + + it('strips agent references only for the files it deleted', async () => { + getStrategyFunctions.mockReturnValue({ + deleteFile: jest + .fn() + .mockImplementation((_req, file) => + file.file_id === 'kept-file' + ? Promise.reject(new Error('rag unavailable')) + : Promise.resolve(undefined), + ), + }); + db.deleteFiles.mockResolvedValue({ deletedCount: 1 }); + const req = { + body: { agent_id: 'agent_1', tool_resource: 'file_search' }, + config: {}, + user: { id: 'user-123', tenantId: 'tenant-a' }, + }; + + const result = await processDeleteRequest({ + req, + files: [ + { file_id: 'gone-file', filepath: '/uploads/gone.txt', source: FileSources.local }, + { file_id: 'kept-file', filepath: '/uploads/kept.txt', source: FileSources.local }, + ], + }); + + expect(result).toEqual({ deletedFileIds: ['gone-file'], failedFileIds: ['kept-file'] }); + expect(db.removeAgentResourceFilesFromAllAgents).toHaveBeenCalledWith({ + file_ids: ['gone-file'], + }); + }); + + it('keeps agent references when the metadata delete fails', async () => { + getStrategyFunctions.mockReturnValue({ deleteFile: jest.fn().mockResolvedValue(undefined) }); + db.deleteFiles.mockRejectedValue(new Error('mongo unavailable')); + const req = { + body: { agent_id: 'agent_1', tool_resource: 'file_search' }, + config: {}, + user: { id: 'user-123', tenantId: 'tenant-a' }, + }; + + await expect( + processDeleteRequest({ + req, + files: [ + { + file_id: 'knowledge-file', + filepath: '/uploads/knowledge.txt', + source: FileSources.local, + }, + ], + }), + ).rejects.toThrow('mongo unavailable'); + + expect(db.removeAgentResourceFiles).not.toHaveBeenCalled(); + expect(db.removeAgentResourceFilesFromAllAgents).not.toHaveBeenCalled(); + }); + it('does not delete vector storage when primary embedded file deletion fails', async () => { const primaryDelete = jest.fn().mockRejectedValue(new Error('permission denied')); const vectorDelete = jest.fn().mockResolvedValue(undefined); diff --git a/packages/api/src/files/deletion.spec.ts b/packages/api/src/files/deletion.spec.ts index 409c1d2f824..86bbebdea0c 100644 --- a/packages/api/src/files/deletion.spec.ts +++ b/packages/api/src/files/deletion.spec.ts @@ -1,4 +1,10 @@ -import { buildDeleteFilesResponse, PARTIAL_FILE_DELETION_MESSAGE } from './deletion'; +import type { AgentResourceFileInput } from './deletion'; +import { + buildDeleteFilesResponse, + deleteAgentResourceFiles, + partitionAgentResourceFiles, + PARTIAL_FILE_DELETION_MESSAGE, +} from './deletion'; describe('delete files response', () => { it('reports the caller’s success message when nothing failed', () => { @@ -32,3 +38,242 @@ describe('delete files response', () => { }); }); }); + +type TestFile = { file_id: string; filename: string }; + +const input = (file_id: string, owner: string | null): AgentResourceFileInput => ({ + file_id, + owner, + file: { file_id, filename: `${file_id}.txt` }, +}); + +describe('partition agent resource files', () => { + const userId = 'user-1'; + const owned = input('owned', userId); + const foreign = input('foreign', 'user-2'); + + it('makes an attached file the caller owns a candidate for the delete pass', () => { + expect( + partitionAgentResourceFiles({ + requestedFileIds: ['owned'], + attachedFileIds: ['owned'], + files: [owned], + toolResource: 'file_search', + userId, + }), + ).toEqual({ ownedFiles: [owned], unlinkOnlyFiles: [] }); + }); + + it('only unlinks an attached file owned by someone else', () => { + expect( + partitionAgentResourceFiles({ + requestedFileIds: ['foreign'], + attachedFileIds: ['foreign'], + files: [foreign], + toolResource: 'file_search', + userId, + }), + ).toEqual({ + ownedFiles: [], + unlinkOnlyFiles: [{ tool_resource: 'file_search', file_id: 'foreign' }], + }); + }); + + it('only unlinks an attached file whose metadata record is gone', () => { + expect( + partitionAgentResourceFiles({ + requestedFileIds: ['missing'], + attachedFileIds: ['missing'], + files: [], + toolResource: 'file_search', + userId, + }), + ).toEqual({ + ownedFiles: [], + unlinkOnlyFiles: [{ tool_resource: 'file_search', file_id: 'missing' }], + }); + }); + + it('does not treat an ownerless record as the caller’s', () => { + expect( + partitionAgentResourceFiles({ + requestedFileIds: ['orphan'], + attachedFileIds: ['orphan'], + files: [input('orphan', null)], + toolResource: 'file_search', + userId, + }), + ).toEqual({ + ownedFiles: [], + unlinkOnlyFiles: [{ tool_resource: 'file_search', file_id: 'orphan' }], + }); + }); + + it('ignores files the tool resource does not hold', () => { + expect( + partitionAgentResourceFiles({ + requestedFileIds: ['owned', 'elsewhere'], + attachedFileIds: ['owned'], + files: [owned, input('elsewhere', userId)], + toolResource: 'file_search', + userId, + }), + ).toEqual({ ownedFiles: [owned], unlinkOnlyFiles: [] }); + }); + + it('splits a mixed request and counts each file once', () => { + expect( + partitionAgentResourceFiles({ + requestedFileIds: ['owned', 'foreign', 'owned'], + attachedFileIds: ['owned', 'foreign'], + files: [owned, foreign], + toolResource: 'ocr', + userId, + }), + ).toEqual({ + ownedFiles: [owned], + unlinkOnlyFiles: [{ tool_resource: 'ocr', file_id: 'foreign' }], + }); + }); +}); + +describe('delete agent resource files', () => { + const userId = 'user-1'; + + const makeDeps = (sharedFileIds: string[] = [], calls: string[] = []) => ({ + getSharedResourceFileIds: jest.fn().mockResolvedValue(sharedFileIds), + removeAgentResourceFiles: jest.fn().mockImplementation(() => { + calls.push('unlink'); + return Promise.resolve(undefined); + }), + deleteFiles: jest.fn().mockImplementation((files: TestFile[]) => { + calls.push('destroy'); + return Promise.resolve({ + deletedFileIds: files.map((file) => file.file_id), + failedFileIds: [], + }); + }), + }); + + const run = ( + files: Array>, + deps: ReturnType, + attachedFileIds?: string[], + ) => + deleteAgentResourceFiles( + { + agentId: 'agent_1', + agentObjectId: '65f000000000000000000001', + toolResource: 'file_search', + requestedFileIds: files.map((file) => file.file_id), + attachedFileIds: attachedFileIds ?? files.map((file) => file.file_id), + files, + userId, + }, + deps, + ); + + it('destroys a file this agent was the last to reference', async () => { + const deps = makeDeps(); + const result = await run([input('owned', userId)], deps); + + expect(deps.deleteFiles).toHaveBeenCalledWith([{ file_id: 'owned', filename: 'owned.txt' }]); + expect(deps.removeAgentResourceFiles).not.toHaveBeenCalled(); + expect(result).toEqual({ + outcome: { deletedFileIds: ['owned'], failedFileIds: [] }, + unlinkedFileIds: [], + destroyedFileIds: ['owned'], + }); + }); + + it('excludes the agent by a globally unique identity, not its logical id', async () => { + const deps = makeDeps(); + await run([input('owned', userId)], deps); + + expect(deps.getSharedResourceFileIds).toHaveBeenCalledWith({ + file_ids: ['owned'], + excludeAgentObjectId: '65f000000000000000000001', + excludeToolResource: 'file_search', + }); + }); + + it('keeps a file another agent still references, unlinking it here only', async () => { + const deps = makeDeps(['shared']); + const result = await run([input('shared', userId)], deps); + + expect(deps.deleteFiles).not.toHaveBeenCalled(); + expect(deps.removeAgentResourceFiles).toHaveBeenCalledWith({ + agent_id: 'agent_1', + files: [{ tool_resource: 'file_search', file_id: 'shared' }], + }); + expect(result).toEqual({ + outcome: null, + unlinkedFileIds: ['shared'], + destroyedFileIds: [], + }); + }); + + it('destroys the last-reference file and unlinks the shared one in the same request', async () => { + const deps = makeDeps(['shared']); + const result = await run([input('shared', userId), input('owned', userId)], deps); + + expect(deps.deleteFiles).toHaveBeenCalledWith([{ file_id: 'owned', filename: 'owned.txt' }]); + expect(deps.removeAgentResourceFiles).toHaveBeenCalledWith({ + agent_id: 'agent_1', + files: [{ tool_resource: 'file_search', file_id: 'shared' }], + }); + expect(result.unlinkedFileIds).toEqual(['shared']); + expect(result.destroyedFileIds).toEqual(['owned']); + }); + + it('unlinks a file the caller does not own without asking to destroy it', async () => { + const deps = makeDeps(); + const result = await run([input('foreign', 'user-2')], deps); + + expect(deps.getSharedResourceFileIds).not.toHaveBeenCalled(); + expect(deps.deleteFiles).not.toHaveBeenCalled(); + expect(deps.removeAgentResourceFiles).toHaveBeenCalledWith({ + agent_id: 'agent_1', + files: [{ tool_resource: 'file_search', file_id: 'foreign' }], + }); + expect(result.outcome).toBeNull(); + }); + + it('touches nothing when the tool resource holds none of the requested files', async () => { + const deps = makeDeps(); + const result = await run([input('owned', userId)], deps, []); + + expect(deps.getSharedResourceFileIds).not.toHaveBeenCalled(); + expect(deps.removeAgentResourceFiles).not.toHaveBeenCalled(); + expect(deps.deleteFiles).not.toHaveBeenCalled(); + expect(result).toEqual({ outcome: null, unlinkedFileIds: [], destroyedFileIds: [] }); + }); + + it('destroys before removing any reference, so a failed destroy leaves the file attached', async () => { + const calls: string[] = []; + const deps = makeDeps(['shared'], calls); + await run([input('shared', userId), input('owned', userId)], deps); + + expect(calls).toEqual(['destroy', 'unlink']); + }); + + it('reports what the delete pass deleted rather than what it was handed', async () => { + const deps = makeDeps(); + deps.deleteFiles.mockResolvedValue({ + deletedFileIds: ['owned'], + failedFileIds: ['other-owned'], + }); + const result = await run([input('owned', userId), input('other-owned', userId)], deps); + + expect(result.destroyedFileIds).toEqual(['owned']); + }); + + it('passes a partial delete outcome back to the caller', async () => { + const deps = makeDeps(); + deps.deleteFiles.mockResolvedValue({ deletedFileIds: [], failedFileIds: ['owned'] }); + const result = await run([input('owned', userId)], deps); + + expect(result.outcome).toEqual({ deletedFileIds: [], failedFileIds: ['owned'] }); + expect(deps.removeAgentResourceFiles).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/api/src/files/deletion.ts b/packages/api/src/files/deletion.ts index 92f176a23fb..c320814063a 100644 --- a/packages/api/src/files/deletion.ts +++ b/packages/api/src/files/deletion.ts @@ -6,6 +6,52 @@ export type FileDeletionOutcome = { failedFileIds?: string[]; }; +/** The `{ tool_resource, file_id }` pair an agent resource unlink takes. */ +export type AgentResourceFileRef = { + tool_resource: string; + file_id: string; +}; + +/** + * One requested file, as this module needs to see it: an id, an owner already normalized to a + * plain string by the caller, and the opaque record the delete pass will be handed. Ownership + * arrives as a string so no storage type crosses into this package. + */ +export type AgentResourceFileInput = { + file_id: string; + owner: string | null; + file: TFile; +}; + +export type AgentResourceDeletion = { + /** Attached files the caller owns: candidates for storage, vector and metadata deletion. */ + ownedFiles: Array>; + /** Attached files owned by someone else, or with no metadata left: unlink the association only. */ + unlinkOnlyFiles: AgentResourceFileRef[]; +}; + +export type AgentResourceDeletionDeps = { + /** Which of these ids another agent still references; a shared file is never destroyed. */ + getSharedResourceFileIds: (params: { + file_ids: string[]; + excludeAgentObjectId: string; + excludeToolResource: string; + }) => Promise; + removeAgentResourceFiles: (params: { + agent_id: string; + files: AgentResourceFileRef[]; + }) => Promise; + /** The full delete pass, injected: storage, vectors, metadata, and the unlink of what it deleted. */ + deleteFiles: (files: TFile[]) => Promise; +}; + +export type AgentResourceDeletionResult = { + /** `null` when nothing was destroyed, so the caller answers with the unlink message. */ + outcome: FileDeletionOutcome | null; + unlinkedFileIds: string[]; + destroyedFileIds: string[]; +}; + export const PARTIAL_FILE_DELETION_MESSAGE = 'Some files could not be deleted'; /** @@ -25,3 +71,129 @@ export const buildDeleteFilesResponse = ( failedFileIds, }; }; + +/** + * Splits the files a delete request names for one agent tool resource into the ones whose storage + * and embeddings may go with the unlink, and the ones only the association can be removed for. + * + * A file the caller owns is theirs to destroy, so it becomes a candidate for the full delete pass; + * a file that belongs to another user, or that no longer has a metadata record, keeps its bytes and + * its chunks and loses only its link to the agent. A file the request names but the tool resource + * does not hold appears in neither list. + */ +export const partitionAgentResourceFiles = ({ + requestedFileIds, + attachedFileIds, + files, + toolResource, + userId, +}: { + requestedFileIds: string[]; + attachedFileIds: string[]; + files: Array>; + toolResource: string; + userId: string; +}): AgentResourceDeletion => { + const attached = new Set(attachedFileIds); + const inputsById = new Map(files.map((input) => [input.file_id, input])); + const seen = new Set(); + const ownedFiles: Array> = []; + const unlinkOnlyFiles: AgentResourceFileRef[] = []; + + for (const fileId of requestedFileIds) { + if (!attached.has(fileId) || seen.has(fileId)) { + continue; + } + seen.add(fileId); + const input = inputsById.get(fileId); + if (input != null && input.owner === userId) { + ownedFiles.push(input); + continue; + } + unlinkOnlyFiles.push({ tool_resource: toolResource, file_id: fileId }); + } + + return { ownedFiles, unlinkOnlyFiles }; +}; + +/** + * Removes files from one agent tool resource, destroying only what this agent was the last holder + * of. + * + * Three outcomes, decided per file. A file the caller does not own is unlinked and left whole. A + * file the caller owns that any other `(agent, tool_resource)` pair still references is unlinked here + * and left whole too: + * duplicating an agent copies `file_ids` rather than the files behind them, so destroying the bytes + * would empty the other agent's knowledge without touching its configuration. Only a file the + * caller owns and this agent was the last to reference goes through the delete pass, which removes + * the storage, the vector chunks and the metadata, and unlinks exactly what it managed to delete. + */ +export const deleteAgentResourceFiles = async ( + { + agentId, + agentObjectId, + toolResource, + requestedFileIds, + attachedFileIds, + files, + userId, + }: { + agentId: string; + /** The agent's globally unique `_id`; `id` alone repeats across tenants. */ + agentObjectId: string; + toolResource: string; + requestedFileIds: string[]; + attachedFileIds: string[]; + files: Array>; + userId: string; + }, + deps: AgentResourceDeletionDeps, +): Promise => { + const { ownedFiles, unlinkOnlyFiles } = partitionAgentResourceFiles({ + requestedFileIds, + attachedFileIds, + files, + toolResource, + userId, + }); + + const sharedFileIds = + ownedFiles.length === 0 + ? new Set() + : new Set( + await deps.getSharedResourceFileIds({ + file_ids: ownedFiles.map((input) => input.file_id), + excludeAgentObjectId: agentObjectId, + excludeToolResource: toolResource, + }), + ); + + const destroyable: Array> = []; + const unlinkOnly = [...unlinkOnlyFiles]; + for (const input of ownedFiles) { + if (sharedFileIds.has(input.file_id)) { + unlinkOnly.push({ tool_resource: toolResource, file_id: input.file_id }); + continue; + } + destroyable.push(input); + } + + /* The destroy runs before any reference is removed, and the delete pass strips the references of + the files it actually deleted. A reference removed ahead of a destroy that then fails is what + strands a file: it leaves the agent panel while its storage, chunks or metadata remain, and + neither this route nor the client's retry queue can name it again. */ + const outcome = + destroyable.length === 0 + ? null + : await deps.deleteFiles(destroyable.map((input) => input.file)); + + if (unlinkOnly.length > 0) { + await deps.removeAgentResourceFiles({ agent_id: agentId, files: unlinkOnly }); + } + + return { + outcome, + unlinkedFileIds: unlinkOnly.map((ref) => ref.file_id), + destroyedFileIds: outcome?.deletedFileIds ?? [], + }; +}; diff --git a/packages/data-schemas/src/methods/agent.spec.ts b/packages/data-schemas/src/methods/agent.spec.ts index fc24723e3c3..57facac58df 100644 --- a/packages/data-schemas/src/methods/agent.spec.ts +++ b/packages/data-schemas/src/methods/agent.spec.ts @@ -65,6 +65,7 @@ let revertAgentVersion: AgentMethods['revertAgentVersion']; let addAgentResourceFile: AgentMethods['addAgentResourceFile']; let removeAgentResourceFiles: AgentMethods['removeAgentResourceFiles']; let removeAgentResourceFilesFromAllAgents: AgentMethods['removeAgentResourceFilesFromAllAgents']; +let getSharedResourceFileIds: AgentMethods['getSharedResourceFileIds']; let getListAgentsByAccess: AgentMethods['getListAgentsByAccess']; let getAgentManagementListByAccess: AgentMethods['getAgentManagementListByAccess']; let generateActionMetadataHash: AgentMethods['generateActionMetadataHash']; @@ -117,6 +118,7 @@ beforeAll(async () => { addAgentResourceFile = methods.addAgentResourceFile; removeAgentResourceFiles = methods.removeAgentResourceFiles; removeAgentResourceFilesFromAllAgents = methods.removeAgentResourceFilesFromAllAgents; + getSharedResourceFileIds = methods.getSharedResourceFileIds; getListAgentsByAccess = methods.getListAgentsByAccess; getAgentManagementListByAccess = methods.getAgentManagementListByAccess; generateActionMetadataHash = methods.generateActionMetadataHash; @@ -4238,6 +4240,146 @@ describe('Agent Methods', () => { ).rejects.toThrow('Agent not found for removing resource files'); }); + describe('getSharedResourceFileIds', () => { + beforeEach(async () => { + await Agent.deleteMany({}); + }); + + test('reports a file another agent still references, across tool resources', async () => { + const sharedFileId = `file_${uuidv4()}`; + const soleFileId = `file_${uuidv4()}`; + + const agent = await createBasicAgent(); + const duplicate = await createBasicAgent(); + + await addAgentResourceFile({ + agent_id: agent.id, + tool_resource: EToolResources.file_search, + file_id: sharedFileId, + }); + await addAgentResourceFile({ + agent_id: agent.id, + tool_resource: EToolResources.file_search, + file_id: soleFileId, + }); + await addAgentResourceFile({ + agent_id: duplicate.id, + tool_resource: EToolResources.context, + file_id: sharedFileId, + }); + + const shared = await getSharedResourceFileIds({ + file_ids: [sharedFileId, soleFileId], + excludeAgentObjectId: String(agent._id), + }); + + expect(shared).toEqual([sharedFileId]); + }); + + test('does not count the excluded agent’s own reference', async () => { + const fileId = `file_${uuidv4()}`; + const agent = await createBasicAgent(); + + await addAgentResourceFile({ + agent_id: agent.id, + tool_resource: EToolResources.file_search, + file_id: fileId, + }); + + expect( + await getSharedResourceFileIds({ + file_ids: [fileId], + excludeAgentObjectId: String(agent._id), + }), + ).toEqual([]); + expect(await getSharedResourceFileIds({ file_ids: [fileId] })).toEqual([fileId]); + }); + + test('reports a file the same agent still holds under another tool resource', async () => { + const fileId = `file_${uuidv4()}`; + const agent = await createBasicAgent(); + + await addAgentResourceFile({ + agent_id: agent.id, + tool_resource: EToolResources.file_search, + file_id: fileId, + }); + await addAgentResourceFile({ + agent_id: agent.id, + tool_resource: EToolResources.context, + file_id: fileId, + }); + + expect( + await getSharedResourceFileIds({ + file_ids: [fileId], + excludeAgentObjectId: String(agent._id), + excludeToolResource: EToolResources.file_search, + }), + ).toEqual([fileId]); + }); + + test('does not count the pair being removed', async () => { + const fileId = `file_${uuidv4()}`; + const agent = await createBasicAgent(); + + await addAgentResourceFile({ + agent_id: agent.id, + tool_resource: EToolResources.file_search, + file_id: fileId, + }); + + expect( + await getSharedResourceFileIds({ + file_ids: [fileId], + excludeAgentObjectId: String(agent._id), + excludeToolResource: EToolResources.file_search, + }), + ).toEqual([]); + }); + + test('does not skip another tenant’s agent that shares the logical id', async () => { + const sharedId = `agent_${uuidv4()}`; + const fileId = `file_${uuidv4()}`; + const tenantA = `tenant-${uuidv4()}`; + const tenantB = `tenant-${uuidv4()}`; + + /* `id` is unique only with `tenantId`, so two tenants can hold the same logical agent id. + Excluding by `id` would call the file unreferenced and destroy tenant B's bytes. */ + const agentA = await tenantStorage.run({ tenantId: tenantA }, () => + createBasicAgent({ id: sharedId }), + ); + await tenantStorage.run({ tenantId: tenantB }, () => createBasicAgent({ id: sharedId })); + + await tenantStorage.run({ tenantId: tenantA }, () => + addAgentResourceFile({ + agent_id: sharedId, + tool_resource: EToolResources.file_search, + file_id: fileId, + }), + ); + await tenantStorage.run({ tenantId: tenantB }, () => + addAgentResourceFile({ + agent_id: sharedId, + tool_resource: EToolResources.file_search, + file_id: fileId, + }), + ); + + expect( + await getSharedResourceFileIds({ + file_ids: [fileId], + excludeAgentObjectId: String(agentA._id), + excludeToolResource: EToolResources.file_search, + }), + ).toEqual([fileId]); + }); + + test('answers without querying when given no file_ids', async () => { + expect(await getSharedResourceFileIds({ file_ids: [] })).toEqual([]); + }); + }); + describe('removeAgentResourceFilesFromAllAgents', () => { beforeEach(async () => { await Agent.deleteMany({}); diff --git a/packages/data-schemas/src/methods/agent.ts b/packages/data-schemas/src/methods/agent.ts index ceeaa354b0b..e25aa52cc93 100644 --- a/packages/data-schemas/src/methods/agent.ts +++ b/packages/data-schemas/src/methods/agent.ts @@ -650,6 +650,15 @@ export function createAgentMethods( }: { file_ids: string[]; }) => Promise<{ matchedCount: number; modifiedCount: number }>; + getSharedResourceFileIds: ({ + file_ids, + excludeAgentObjectId, + excludeToolResource, + }: { + file_ids: string[]; + excludeAgentObjectId?: string; + excludeToolResource?: string; + }) => Promise; } { const { removeAllPermissions, getActions, getSoleOwnedResourceIds, isExternalSkillId } = deps; @@ -1229,6 +1238,68 @@ export function createAgentMethods( }; } + /** + * Reports which of the given file_ids keep a reference once the caller's own is removed, so a + * caller can tell a last reference from a shared one. + * + * The unit of a reference is the `(agent, tool_resource)` pair rather than the agent: + * `addAgentResourceFile` stores `file_ids` per resource, so one agent can hold the same file under + * both `file_search` and `context`, and removing it from one leaves the other needing the bytes. + * `excludeToolResource` narrows the exclusion to the pair being removed; omitting it excludes the + * whole agent. + * + * The agent is excluded by `_id`, because `id` is unique only together with `tenantId`: matching on + * `id` alone would skip another tenant's agent of the same name and call its file unreferenced. + * + * Duplicating an agent copies `file_ids` rather than the files behind them, so one file record + * can back two agents; destroying its bytes on behalf of one agent would empty the other. This + * is deliberately not scoped by tenant: a reference is a reference, whoever holds it. + */ + async function getSharedResourceFileIds({ + file_ids, + excludeAgentObjectId, + excludeToolResource, + }: { + file_ids: string[]; + excludeAgentObjectId?: string; + excludeToolResource?: string; + }): Promise { + if (!file_ids || file_ids.length === 0) { + return []; + } + + const Agent = mongoose.models.Agent as Model; + const requested = new Set(file_ids); + const searchParameter: FilterQuery = { + $or: TOOL_RESOURCE_KEYS.map((key) => ({ + [`tool_resources.${key}.file_ids`]: { $in: file_ids }, + })), + }; + + const agents = await Agent.find(searchParameter, { _id: 1, tool_resources: 1 }).lean(); + const shared = new Set(); + for (const agent of agents) { + const isExcludedAgent = + excludeAgentObjectId != null && String(agent._id) === excludeAgentObjectId; + for (const key of TOOL_RESOURCE_KEYS) { + if (isExcludedAgent && (excludeToolResource == null || key === excludeToolResource)) { + continue; + } + const fileIds = agent.tool_resources?.[key]?.file_ids; + if (fileIds == null) { + continue; + } + for (const fileId of fileIds) { + if (requested.has(fileId)) { + shared.add(fileId); + } + } + } + } + + return [...shared]; + } + /** * Deletes an agent based on the provided search parameter. */ @@ -1698,6 +1769,7 @@ export function createAgentMethods( generateActionMetadataHash, removeAgentFromUserFavorites, removeAgentResourceFilesFromAllAgents, + getSharedResourceFileIds, }; } From db674eda60ed0f5a13ffb416a26961f81f94680f Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 18 Sep 2026 06:04:47 -0400 Subject: [PATCH 02/27] =?UTF-8?q?=F0=9F=94=93=20fix:=20Stop=20Code=20Outpu?= =?UTF-8?q?ts=20and=20Tool-Routed=20Spreadsheets=20From=20Locking=20Thread?= =?UTF-8?q?s=20(#16058)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since #15694 bounded each turn's attachments across history, a thread whose history counts more than `fileLimit` model-bound files is refused on every turn, including turns that attach nothing. Two kinds of file reached that count that never belonged in the prompt. Code outputs: priming clears an expired sandbox reference on the turn's copy of the record so the file is re-provisioned, and a route-less record without a reference was classified as prompt content, so it counted toward the limit and could be encoded as media. Code outputs now stay tool-owned regardless of reference liveness, through one predicate shared by admission, BaseClient delivery and the child run-file encoder. Tool-routed spreadsheets: with `textFallbackWithoutTools`, #16027 delivered the fallback text until a tool held a copy. Run Code receives its copy only on its first call, which a refused turn never makes, so the text counted on every later turn. An enabled Run Code is a reader again for the types it can read; File Search still reads only what its store holds. --- api/app/clients/BaseClient.js | 9 +- api/app/clients/specs/BaseClient.test.js | 57 ++++++++- packages/api/src/agents/attachments.test.ts | 115 +++++++++++++++++- packages/api/src/agents/attachments.ts | 40 ++++-- .../api/src/agents/files/delivery.spec.ts | 17 ++- packages/api/src/agents/files/encode.spec.ts | 36 +++++- packages/api/src/agents/files/encode.ts | 9 +- .../src/resolve-llm-delivery-path.spec.ts | 20 ++- .../src/resolve-llm-delivery-path.ts | 36 +++--- 9 files changed, 276 insertions(+), 63 deletions(-) diff --git a/api/app/clients/BaseClient.js b/api/app/clients/BaseClient.js index a685ee3091d..a39c5ca3c8b 100644 --- a/api/app/clients/BaseClient.js +++ b/api/app/clients/BaseClient.js @@ -21,6 +21,7 @@ const { collectModelBoundHistoricalFileIdState, projectModelBoundSourceFiles, isModelBoundAttachmentFile, + isToolOwnedAttachment, withBalanceReservations, findCheckpointSummaryPart, getSummaryPartText, @@ -1844,13 +1845,7 @@ class BaseClient { /* An explicit `provider` path is authoritative: lazy provisioning stamps * `embedded`/`codeEnvRef` on files that are still meant for the model, so the * legacy tool-provisioning exclusion only applies to records without one. */ - if ( - deliveryPath !== 'provider' && - (file.embedded === true || - file.metadata?.codeEnvRef != null || - file.metadata?.codeEnvRefs != null || - file.metadata?.fileIdentifier != null) - ) { + if (deliveryPath !== 'provider' && isToolOwnedAttachment(file)) { allFiles.push(file); continue; } diff --git a/api/app/clients/specs/BaseClient.test.js b/api/app/clients/specs/BaseClient.test.js index 7277e634879..4a3fd4b7f6d 100644 --- a/api/app/clients/specs/BaseClient.test.js +++ b/api/app/clients/specs/BaseClient.test.js @@ -3584,13 +3584,13 @@ describe('BaseClient', () => { expect(TestClient.getTextContextAttachments([file])).toEqual([]); }); - test('delivers the text when the tool that would read the file has yet to receive it', () => { - /* An upload that named no destination is filed under no tool, so the enabled tool alone - * cannot serve it and withholding the text left it readable by nothing. */ + test('delivers the text when File Search has yet to receive the file', () => { + /* An upload that named no destination is filed under no tool, so an enabled search tool + * alone cannot serve it and withholding the text left it readable by nothing. */ routeCsvToTools(); TestClient.options.agent = { provider: EModelEndpoint.openAI, - fileConsumers: { executeCode: true, fileSearch: true }, + fileConsumers: { executeCode: false, fileSearch: true }, }; TestClient.options.agent.deliveryRouting = resolveTurnDeliveryRouting({ agent: TestClient.options.agent, @@ -3614,6 +3614,32 @@ describe('BaseClient', () => { ]); }); + test('leaves a file Run Code can read with Run Code before the sandbox holds it', () => { + /* Run Code uploads the file on its first call, so the text stays off the prompt, where it + * would otherwise count toward the history limits until that call. */ + routeCsvToTools(); + TestClient.options.agent = { + provider: EModelEndpoint.openAI, + fileConsumers: { executeCode: true, fileSearch: true }, + }; + TestClient.options.agent.deliveryRouting = resolveTurnDeliveryRouting({ + agent: TestClient.options.agent, + config: TestClient.options.req?.config, + }); + const file = { + file_id: 'unprovisioned-csv', + filename: 'sales.csv', + type: 'text/csv', + text: 'region,total', + llmDeliveryPath: 'none', + metadata: { destinationChosen: false }, + }; + + expect(TestClient.getAttachmentDeliveryPath(file)).toBe('none'); + expect(TestClient.getTextContextAttachments([file])).toEqual([]); + expect(TestClient.resolveTurnAttachments([file])).toEqual([file]); + }); + test('does not fall back on an endpoint that has not enabled it', () => { routeCsvToTools({ textFallbackWithoutTools: false }); TestClient.options.agent = { @@ -3931,6 +3957,29 @@ describe('BaseClient', () => { expect(message.image_urls).toEqual(['encoded-image']); }); + test('keeps a code output out of the prompt once its expired sandbox reference is cleared', async () => { + /* Priming clears a dead sandbox reference on the turn's copy of the record so the file + * is re-provisioned. The output still belongs to the sandbox that wrote it. */ + const message = {}; + const file = { + user: 'user1', + file_id: 'code-output-chart', + filename: 'chart.png', + filepath: '/uploads/chart.png', + type: 'image/png', + bytes: 100, + source: 'local', + context: 'execute_code', + metadata: {}, + }; + + const result = await TestClient.processAttachments(message, [file]); + + expect(result).toEqual([file]); + expect(TestClient.addImageURLs).not.toHaveBeenCalled(); + expect(message.image_urls).toBeUndefined(); + }); + test('keeps excluding embedded legacy files that have no delivery path', async () => { const message = {}; const file = { diff --git a/packages/api/src/agents/attachments.test.ts b/packages/api/src/agents/attachments.test.ts index 66745db1533..92610dae91b 100644 --- a/packages/api/src/agents/attachments.test.ts +++ b/packages/api/src/agents/attachments.test.ts @@ -1,5 +1,5 @@ import { logger } from '@librechat/data-schemas'; -import { EModelEndpoint, FileSources } from 'librechat-data-provider'; +import { EModelEndpoint, FileContext, FileSources } from 'librechat-data-provider'; import type { IMongoFile } from '@librechat/data-schemas'; import type { ServerRequest } from '~/types'; @@ -18,7 +18,9 @@ import { getAgentContextAttachments, buildAgentContextAttachmentsByAgentId, isModelBoundAttachmentFile, + isToolOwnedAttachment, } from './attachments'; +import { applyTurnDelivery, resolveTurnDeliveryRouting } from './files/delivery'; const makeTextFile = (file_id: string, filename: string, text: string): IMongoFile => ({ @@ -670,3 +672,114 @@ describe('agent attachment helpers', () => { ).resolves.toEqual(new Map()); }); }); + +describe('files that belong to a tool', () => { + const XLSX = 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet'; + const sandboxRef = { + kind: 'user', + id: 'user-1', + file_id: 'sandbox-file', + storage_session_id: 's1', + }; + + it('keeps a code output tool-owned after priming clears its expired sandbox references', () => { + const output = { + file_id: 'rows-json', + type: 'application/json', + context: FileContext.execute_code, + text: '{"rows":[]}', + metadata: {}, + } as unknown as IMongoFile; + + expect(isToolOwnedAttachment(output)).toBe(true); + expect(isModelBoundAttachmentFile(output)).toBe(false); + }); + + it('still treats a route-less user upload without tool references as prompt content', () => { + const upload = { + file_id: 'legacy-upload', + type: 'application/pdf', + context: FileContext.message_attachment, + metadata: {}, + } as unknown as IMongoFile; + + expect(isToolOwnedAttachment(upload)).toBe(false); + expect(isModelBoundAttachmentFile(upload)).toBe(true); + }); + + it('admits a Run Code thread shaped like the one the history limit locked', () => { + /* Spreadsheets routed to tools with text stored for the fallback, one screenshot, and the + * code outputs of an earlier run whose expired sandbox references priming cleared. Counted + * as prompt attachments, the outputs and the spreadsheets' fallback text filled the per-turn + * count past its default of ten, so every later turn was refused. */ + const config = { + fileConfig: { + textFallbackWithoutTools: true, + endpoints: { + default: { defaultLLMDeliveryPath: { overrides: { [XLSX]: 'none' as const } } }, + }, + }, + }; + const routing = resolveTurnDeliveryRouting({ + agent: { provider: EModelEndpoint.bedrock, endpoint: EModelEndpoint.bedrock }, + config, + }); + const consumers = { executeCode: true, fileSearch: false }; + const sheet = (file_id: string, extra: object = {}) => + ({ + file_id, + type: XLSX, + bytes: 200_000, + source: FileSources.local, + context: FileContext.message_attachment, + llmDeliveryPath: 'none', + text: 'x'.repeat(110_000), + metadata: { destinationChosen: false }, + ...extra, + }) as unknown as IMongoFile; + const olderSheets = ['q3-actuals', 'q3-budget', 'q3-map'].map((id) => + sheet(id, { + llmDeliveryPath: 'text', + metadata: { destinationChosen: false, codeEnvRef: sandboxRef }, + }), + ); + const newSheets = ['s1', 's2', 's3', 's4', 's5', 's6'].map((id) => sheet(id)); + const screenshot = { + file_id: 'screenshot', + type: 'image/png', + bytes: 380_971, + source: FileSources.local, + context: FileContext.message_attachment, + llmDeliveryPath: 'provider', + metadata: { destinationChosen: false }, + } as unknown as IMongoFile; + const outputs = Array.from( + { length: 8 }, + (_, index) => + ({ + file_id: `output-${index}`, + type: 'application/json', + bytes: 20_000, + source: FileSources.local, + context: FileContext.execute_code, + text: 'y'.repeat(20_000), + metadata: {}, + }) as unknown as IMongoFile, + ); + + const admitted = applyTurnDelivery([...olderSheets, screenshot, ...newSheets, ...outputs], { + routing, + consumers, + }).filter(isModelBoundAttachmentFile); + + expect(admitted.map((file) => file.file_id)).toEqual(['screenshot']); + expect(() => + assertAgentAttachmentLimits({ + attachments: admitted, + fileConfig: config.fileConfig, + endpoint: EModelEndpoint.bedrock, + countRepeatedExtractedText: true, + }), + ).not.toThrow(); + }); +}); diff --git a/packages/api/src/agents/attachments.ts b/packages/api/src/agents/attachments.ts index 2606905ff04..447af3c9218 100644 --- a/packages/api/src/agents/attachments.ts +++ b/packages/api/src/agents/attachments.ts @@ -1,9 +1,10 @@ import { logger } from '@librechat/data-schemas'; import { - EModelEndpoint, + FileContext, FileSources, - getEndpointFileConfig, + EModelEndpoint, mergeFileConfig, + getEndpointFileConfig, } from 'librechat-data-provider'; import type { IMongoFile } from '@librechat/data-schemas'; import type { TokenCountFn } from '~/utils/text'; @@ -22,10 +23,34 @@ type AttachmentTelemetryFile = FileWithId & { source?: string | null; type?: string | null; text?: string | null; + context?: string | null; + embedded?: boolean | null; llmDeliveryPath?: string | null; metadata?: (IMongoFile['metadata'] & { pageCount?: number | null }) | null; }; +/** + * Whether a record without a delivery route belongs to a tool rather than to the prompt. + * + * A code output lives in the sandbox that wrote it, and it stays the tool's while an expired + * sandbox copy is re-provisioned: priming clears the dead references on the turn's copy of the + * record, which must not turn the output into a prompt attachment that counts toward the turn's + * limits. Any other record belongs to a tool once a tool has provisioned it. + */ +export function isToolOwnedAttachment(file: AttachmentTelemetryFile): boolean { + const metadata = file.metadata as + | (IMongoFile['metadata'] & { fileIdentifier?: unknown }) + | null + | undefined; + return ( + file.context === FileContext.execute_code || + file.embedded === true || + metadata?.codeEnvRef != null || + metadata?.codeEnvRefs != null || + metadata?.fileIdentifier != null + ); +} + /** Whether a hydrated file contributes content to the model prompt itself. */ export function isModelBoundAttachmentFile( file: AttachmentTelemetryFile | null | undefined, @@ -46,16 +71,7 @@ export function isModelBoundAttachmentFile( if (file.llmDeliveryPath === 'provider') { return true; } - const metadata = file.metadata as - | (IMongoFile['metadata'] & { fileIdentifier?: unknown }) - | null - | undefined; - return !( - (file as IMongoFile).embedded === true || - metadata?.codeEnvRef != null || - metadata?.codeEnvRefs != null || - metadata?.fileIdentifier != null - ); + return !isToolOwnedAttachment(file); } type AgentAttachmentLimitRequest = { diff --git a/packages/api/src/agents/files/delivery.spec.ts b/packages/api/src/agents/files/delivery.spec.ts index d36fa721d62..181501f375d 100644 --- a/packages/api/src/agents/files/delivery.spec.ts +++ b/packages/api/src/agents/files/delivery.spec.ts @@ -118,14 +118,23 @@ describe('applyTurnDelivery', () => { expect(applyTurnDelivery(files, { agent, config, consumers: runsCode })).toBe(files); }); - it('delivers text for a file the tool that would read it has yet to receive', () => { - /* An upload that named no destination is filed under no tool, so the enabled tool alone is - * not what serves it: withholding the text on that basis left it readable by nothing. */ - expect(applyTurnDelivery([csv], { agent, config, consumers: runsCode })).toEqual([ + it('delivers text for a file File Search has yet to receive', () => { + /* An upload that named no destination is filed under no tool, so an enabled search tool + * alone is not what serves it: withholding the text on that basis left it readable by + * nothing. */ + const searchesFiles: TurnFileConsumers = { executeCode: false, fileSearch: true }; + expect(applyTurnDelivery([csv], { agent, config, consumers: searchesFiles })).toEqual([ { ...csv, llmDeliveryPath: 'text' }, ]); }); + it('leaves a file Run Code can read with Run Code before the sandbox holds it', () => { + /* Run Code uploads the file on its first call. Delivering the text instead would count it + * toward the turn's limits until that call, and a refused turn never makes it. */ + const files = [csv]; + expect(applyTurnDelivery(files, { agent, config, consumers: runsCode })).toBe(files); + }); + it('marks nothing where the endpoint has not enabled the fallback', () => { const files = [csv]; const disabled = { diff --git a/packages/api/src/agents/files/encode.spec.ts b/packages/api/src/agents/files/encode.spec.ts index d999273eadb..10c29025be9 100644 --- a/packages/api/src/agents/files/encode.spec.ts +++ b/packages/api/src/agents/files/encode.spec.ts @@ -1,4 +1,4 @@ -import { FileSources, ImageDetail } from 'librechat-data-provider'; +import { FileContext, FileSources, ImageDetail } from 'librechat-data-provider'; import type { TFile } from 'librechat-data-provider'; import type { RunFileEncodingAgent, RunFileMessageEncoderDeps } from './encode'; import type { ServerRequest } from '~/types'; @@ -172,6 +172,7 @@ describe('createRunFileMessageEncoder', () => { agents: { noReader: { provider: 'openAI', fileConsumers: { executeCode: false, fileSearch: false } }, runsCode: { provider: 'openAI', fileConsumers: { executeCode: true, fileSearch: false } }, + searches: { provider: 'openAI', fileConsumers: { executeCode: false, fileSearch: true } }, unknown: { provider: 'openAI' }, }, fileConfig: { @@ -184,8 +185,9 @@ describe('createRunFileMessageEncoder', () => { }, }); - /* A tool serves a file only once it holds it, so the child that runs code keeps the file off - * its prompt for the copy the sandbox has, and receives the text for the one it does not. */ + /* Run Code uploads the file on its first call, so the child that runs code keeps it off the + * prompt whether or not its sandbox holds a copy yet. File search serves only what its + * store holds, so the searching child receives the text for a file never embedded. */ const sandboxCsv: TFile = { ...csv, metadata: { @@ -199,16 +201,18 @@ describe('createRunFileMessageEncoder', () => { }, }; - const [noReader, runsCode, unprovisioned, unknown] = await Promise.all([ + const [noReader, runsCode, unprovisioned, searches, unknown] = await Promise.all([ harness.encode([csv], 'noReader'), harness.encode([sandboxCsv], 'runsCode'), harness.encode([csv], 'runsCode'), + harness.encode([csv], 'searches'), harness.encode([csv], 'unknown'), ]); expect(JSON.stringify(noReader[0].content)).toContain('region,total'); expect(runsCode).toEqual([]); - expect(JSON.stringify(unprovisioned[0].content)).toContain('region,total'); + expect(unprovisioned).toEqual([]); + expect(JSON.stringify(searches[0].content)).toContain('region,total'); expect(unknown).toEqual([]); expect(harness.extractText).toHaveBeenCalledWith( expect.objectContaining({ attachments: [{ ...csv, llmDeliveryPath: 'text' }] }), @@ -363,6 +367,28 @@ describe('createRunFileMessageEncoder', () => { }, ); + it('leaves code outputs with cleared sandbox references out of the child prompt and its count budget', async () => { + /* Priming clears a dead reference on the turn's copy so the output is re-provisioned; the + * output still belongs to the sandbox, so it neither reaches the prompt nor spends the + * one-file budget. */ + const harness = setup({ fileConfig: { endpoints: { openAI: { fileLimit: 1 } } } }); + const output = (file_id: string): TFile => ({ + ...pdf, + file_id, + filename: `${file_id}.png`, + type: 'image/png', + text: undefined, + llmDeliveryPath: undefined, + context: FileContext.execute_code, + metadata: {}, + }); + + await expect(harness.encode([output('chart-a'), output('chart-b')], 'child')).resolves.toEqual( + [], + ); + expect(harness.encodeImages).not.toHaveBeenCalled(); + }); + it('includes permanent child context in the count budget before encoding shared files', async () => { const harness = setup({ agents: { diff --git a/packages/api/src/agents/files/encode.ts b/packages/api/src/agents/files/encode.ts index a5312c52992..e04d6cbed86 100644 --- a/packages/api/src/agents/files/encode.ts +++ b/packages/api/src/agents/files/encode.ts @@ -16,6 +16,7 @@ import type { BaseMessage } from '@librechat/agents/langchain'; import type { ServerRequest, StrategyFunctions } from '~/types'; import type { TokenCountFn } from '~/utils/text'; import { + isToolOwnedAttachment, isModelBoundAttachmentFile, assertAgentAttachmentLimits, AgentAttachmentPolicyError, @@ -160,13 +161,7 @@ export function createRunFileMessageEncoder( } /* Provisioning may add tool references to native files. Only legacy records use * those references to decide whether their bytes belong in the prompt. */ - if ( - deliveryPath !== 'provider' && - (file.embedded === true || - file.metadata?.codeEnvRef != null || - file.metadata?.codeEnvRefs != null || - file.metadata?.fileIdentifier != null) - ) { + if (deliveryPath !== 'provider' && isToolOwnedAttachment(file)) { continue; } if (file.type.startsWith('image/')) { diff --git a/packages/data-provider/src/resolve-llm-delivery-path.spec.ts b/packages/data-provider/src/resolve-llm-delivery-path.spec.ts index bc147bfd0f8..ab3728cedd8 100644 --- a/packages/data-provider/src/resolve-llm-delivery-path.spec.ts +++ b/packages/data-provider/src/resolve-llm-delivery-path.spec.ts @@ -793,13 +793,22 @@ describe('hasTurnFileConsumer', () => { expect(hasTurnFileConsumer('video/mp4', { executeCode: false, fileSearch: true })).toBe(false); }); - it('counts an enabled tool only where the record shows it holds the file', () => { + it('counts File Search only where the record shows the vector store holds the file', () => { const consumers = { executeCode: false, fileSearch: true }; expect(hasTurnFileConsumer('text/csv', consumers, { embedded: true })).toBe(true); expect(hasTurnFileConsumer('text/csv', consumers, { embedded: false })).toBe(false); expect(hasTurnFileConsumer('text/csv', consumers, {})).toBe(false); }); + it('counts an enabled Run Code as a reader before the sandbox holds a copy', () => { + /* Its first call uploads the file, so no reference is needed in advance. The tool still + * has to be able to read the type. */ + const consumers = { executeCode: true, fileSearch: false }; + expect(hasTurnFileConsumer('text/csv', consumers, {})).toBe(true); + expect(hasTurnFileConsumer('text/csv', consumers, { metadata: {} })).toBe(true); + expect(hasTurnFileConsumer(eml, consumers, {})).toBe(false); + }); + it('pairs the evidence with the tool that can read the type', () => { /* Only the sandbox holds this file, so the tool that can read an email export is the one * without a copy of it, while csv is served by the tool that has one. */ @@ -955,16 +964,17 @@ describe('resolveTurnLLMDeliveryPath', () => { ).toBe('text'); }); - it('delivers text when Run Code is on but no sandbox holds the file', () => { - /* A promoted destination is not uploaded to the sandbox at upload time, so the turn the - * file arrives on carries no reference either. */ + it('leaves a file Run Code can read with Run Code before the sandbox holds it', () => { + /* Delivered text counts toward the turn's attachment limits. A turn those limits refuse + * never runs code, so the file would never become held and every later turn would carry + * the same text and be refused the same way. Run Code uploads the file on its first call. */ expect( resolveTurnLLMDeliveryPath({ file: routedCsv, consumers: { executeCode: true, fileSearch: false }, endpointConfig, }), - ).toBe('text'); + ).toBe('none'); }); it('delivers text where the tool holding the file cannot read this type', () => { diff --git a/packages/data-provider/src/resolve-llm-delivery-path.ts b/packages/data-provider/src/resolve-llm-delivery-path.ts index 1ea941bc550..6b41d378fc8 100644 --- a/packages/data-provider/src/resolve-llm-delivery-path.ts +++ b/packages/data-provider/src/resolve-llm-delivery-path.ts @@ -381,25 +381,25 @@ export function hasToolResourceProvisioning(file: TurnDeliveryFile, toolResource /** * Whether a tool this turn runs can read a file of this type. * - * Passing the record asks the stricter question a delivery decision needs: a tool serves a file - * only once it holds it. A file tool being enabled is not the same as the file having reached - * it, and an upload that named no destination is filed under no tool at all, so judging the - * tool set alone reads an empty vector store as a reader. + * Passing the record asks the stricter question a delivery decision needs where a tool serves + * only what it already holds. File search reads the vector store, and an upload that named no + * destination is filed under no tool at all, so an enabled search tool is a reader only once + * the record shows the store holds the file. Run Code needs no copy in advance: its first call + * uploads every attachment it can read to the sandbox, so enabling it is enough. Requiring a + * copy there would hand the file's text to the prompt on every turn before the first code run, + * and a turn that refuses that text never runs code, so the file would never become held. */ export function hasTurnFileConsumer( mimeType: string, consumers: TurnFileConsumers, file?: TurnDeliveryFile, ): boolean { - const holds = (toolResource: EToolResources): boolean => - file == null || hasToolResourceProvisioning(file, toolResource); + const searchHolds = file == null || hasToolResourceProvisioning(file, EToolResources.file_search); return ( - (consumers.executeCode && - canToolResourceConsume(EToolResources.execute_code, mimeType) && - holds(EToolResources.execute_code)) || + (consumers.executeCode && canToolResourceConsume(EToolResources.execute_code, mimeType)) || (consumers.fileSearch && canToolResourceConsume(EToolResources.file_search, mimeType) && - holds(EToolResources.file_search)) + searchHolds) ); } @@ -454,15 +454,15 @@ export function hasInferredLLMDeliveryPath(file: TurnDeliveryFile): boolean { * A record predating routing and a destination the user chose keep what they stored. An * inferred route re-resolves against the endpoint handling the turn. A `none` route leaves * the file for a tool; where the endpoint enables `textFallbackWithoutTools` and no tool this - * turn runs both reads the file and already holds it, the text extracted at upload is delivered - * rather than the file reaching nothing. Consumers left undefined are unknown and not judged, as - * in {@link resolveUploadDestination}. + * turn runs can read the file, the text extracted at upload is delivered rather than the file + * reaching nothing. Consumers left undefined are unknown and not judged, as in + * {@link resolveUploadDestination}. * - * Holding the file is asked of the record rather than of the tool set, because the two disagree - * for the upload that needs the fallback most: one that named no destination is filed under no - * tool, so enabling file search would otherwise withhold the text for a vector store that never - * received the file, leaving the attachment readable by nothing. Deferred provisioning still - * fills that store on first use, which delivering the text does not prevent. + * For file search, reading is asked of the record rather than of the tool set, because the two + * disagree for the upload that needs the fallback most: one that named no destination is filed + * under no tool, so enabling file search would otherwise withhold the text for a vector store + * that never received the file. Run Code is judged by the tool set: see + * {@link hasTurnFileConsumer}. */ export function resolveTurnLLMDeliveryPath( routing: Partial | undefined, From 02efb1d6b5e6e83fa11232fb2361c1b60c827148 Mon Sep 17 00:00:00 2001 From: "lia-by-librechat[bot]" <328778573+lia-by-librechat[bot]@users.noreply.github.com> Date: Fri, 18 Sep 2026 06:05:39 -0400 Subject: [PATCH 03/27] =?UTF-8?q?=F0=9F=9A=A5=20test:=20Hold=20Group=20Cac?= =?UTF-8?q?he=20Reads=20Until=20Concurrent=20Callers=20Arrive=20(#16059)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `getUserPrincipals caching > deduplicates concurrent cache builds for the same member key` fails intermittently in CI, expecting one `cache.set` and receiving two. It failed that way on an unrelated pull request, on a `data-schemas` job whose diff touched nothing in this package. The test starts three concurrent lookups behind a fake cache whose reads resolve after 10ms and whose `get` never returns what `set` stored, then asserts a single build. `getMemberGroupIds` registers its in-flight entry only after the awaited cache read, so the three callers coalesce only while they overlap. When the scheduler serializes them, the first build finishes and clears its entry before a later caller reads, and that caller takes the miss path alone. In production it would find the value the first build wrote; against a fake that always misses it rebuilds and writes again, which is the second `set`. Reproduced deterministically by resolving the first read immediately and later reads after 50ms: expected 1, received 2, exactly as CI reported. The delay is now an arrival barrier that holds each read until all three are inside the miss window, so the test pins the concurrency it describes instead of depending on timing. Reads arriving after the barrier opens pass straight through, so the second read a lock holder performs cannot deadlock. Both concurrency tests share the store-backed fake cache, and a caller that does miss alone now behaves as it would in production. The tests also became stricter. With the in-flight check disabled they fail at three builds rather than two, so a genuine regression no longer looks like the flake it used to produce. Co-authored-by: Lia --- .../src/methods/userGroup.spec.ts | 46 +++++++++++++------ 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/packages/data-schemas/src/methods/userGroup.spec.ts b/packages/data-schemas/src/methods/userGroup.spec.ts index 46ba4cdc7dc..7a33ac633c8 100644 --- a/packages/data-schemas/src/methods/userGroup.spec.ts +++ b/packages/data-schemas/src/methods/userGroup.spec.ts @@ -484,11 +484,14 @@ describe('userGroup methods', () => { }); describe('getUserPrincipals caching', () => { - function createFakeCache() { + function createFakeCache({ onRead }: { onRead?: () => Promise } = {}) { const store = new Map(); return { store, - get: jest.fn(async (key: string) => store.get(key)), + get: jest.fn(async (key: string) => { + await onRead?.(); + return store.get(key); + }), set: jest.fn(async (key: string, value: unknown) => { store.set(key, value); }), @@ -497,6 +500,29 @@ describe('userGroup methods', () => { }; } + /** + * Holds each cache read until `callers` of them are inside the miss window together, so a + * test about deduplicating concurrent builds actually gets concurrent builds. Delaying + * every read by a fixed few milliseconds instead leaves the overlap up to the scheduler: + * under load the first build can finish before a later caller reads, and that caller then + * takes the miss path on its own, which is correct behaviour but not what such a test is + * asserting. Reads after the barrier opens pass straight through, so a caller that reads + * again mid-build (after taking the lock, say) cannot deadlock. + */ + function arrivalBarrier(callers: number) { + let arrived = 0; + let open!: () => void; + const allArrived = new Promise((resolve) => { + open = resolve; + }); + return async () => { + if (++arrived >= callers) { + open(); + } + await allArrived; + }; + } + function createCachedMethods(cache: ReturnType) { return createUserGroupMethods(mongoose, { getCache: jest.fn(() => cache) }); } @@ -758,14 +784,8 @@ describe('userGroup methods', () => { it('deduplicates concurrent cache builds for the same member key', async () => { const user = await createTestUser({ idOnTheSource: 'dedup-ext-1' }); - const cache = { - get: jest.fn(async () => { - await new Promise((resolve) => setTimeout(resolve, 10)); - return undefined; - }), - set: jest.fn(async () => undefined), - }; - const cachedMethods = createUserGroupMethods(mongoose, { getCache: jest.fn(() => cache) }); + const cache = createFakeCache({ onRead: arrivalBarrier(3) }); + const cachedMethods = createCachedMethods(cache); const params = { userId: user._id.toString(), role: SystemRoles.USER, @@ -787,11 +807,7 @@ describe('userGroup methods', () => { it('shares one lock and DB build across concurrent same-process callers', async () => { const user = await createTestUser({ idOnTheSource: 'lock-ext-1' }); const cache = { - get: jest.fn(async () => { - await new Promise((resolve) => setTimeout(resolve, 10)); - return undefined; - }), - set: jest.fn(async () => undefined), + ...createFakeCache({ onRead: arrivalBarrier(3) }), acquireLock: jest.fn(async () => 'lock-token'), releaseLock: jest.fn(async () => undefined), lockWaitMs: 5000, From 03f182f9d4a5f30cbd66305dae4fad91e65b9644 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 18 Sep 2026 08:57:35 -0400 Subject: [PATCH 04/27] =?UTF-8?q?=F0=9F=94=91=20fix:=20Ask=20for=20MCP=20S?= =?UTF-8?q?ign-In=20When=20a=20Server=20Rejects=20Refreshed=20Tokens=20(#1?= =?UTF-8?q?6075)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When an MCP server's resource endpoint rejects the access tokens its authorization server keeps issuing, a connection being established silently refreshed on every 401 and retried with a token that was rejected again. Once the connect attempts ran out, the failure surfaced as an ordinary connect error, so reinitialize reported `oauthRequired: false`, agent tool loading never showed a sign-in prompt, and agents whose MCP tools all came from that server failed closed. The establishment handler in `MCPConnectionFactory` now remembers the tokens its silent refresh issued. When the server rejects that same credential set again while those tokens are still the stored credential, it starts interactive OAuth instead of refreshing, which issues the authorization URL through `oauthStart` so tool loading prompts for sign-in. A credential another request stored in the meantime, or a different credential set, keeps the existing refresh path. --- packages/api/src/mcp/MCPConnectionFactory.ts | 43 ++++++- ...ectionFactory.oauthSdk.integration.test.ts | 47 ++++++++ .../__tests__/MCPConnectionFactory.test.ts | 111 ++++++++++++++++++ 3 files changed, 200 insertions(+), 1 deletion(-) diff --git a/packages/api/src/mcp/MCPConnectionFactory.ts b/packages/api/src/mcp/MCPConnectionFactory.ts index 07fa1b41145..1b8be2a2dd3 100644 --- a/packages/api/src/mcp/MCPConnectionFactory.ts +++ b/packages/api/src/mcp/MCPConnectionFactory.ts @@ -1553,6 +1553,34 @@ export class MCPConnectionFactory { return true; } + /** + * Whether the server rejected the tokens a silent refresh issued during this connection attempt, + * and they are still the stored credential. A provider can keep refreshing a grant its resource + * server no longer accepts, so refreshing again would repeat the rejection on every connect + * attempt and never ask the user to authorize. Tokens another request stored since then are not + * a rejected renewal; the refresh path adopts them. + */ + private async isRejectedRenewal( + renewed: MCPOAuthTokens, + rejectedCredentialSetId: string | null, + ): Promise { + if (!this.tokenMethods?.findToken) { + return false; + } + if ((renewed.credential_set_id ?? null) !== rejectedCredentialSetId) { + return false; + } + return this.runWithCapturedTenant(() => + MCPTokenStorage.isCurrentAccessToken({ + userId: this.userId!, + serverName: this.serverName, + accessToken: renewed.access_token, + credentialSetId: renewed.credential_set_id, + findToken: this.tokenMethods!.findToken!, + }), + ); + } + private getOAuthReplayExpiresAt(createdAt?: number): number | undefined { if (!createdAt) { return undefined; @@ -1662,6 +1690,8 @@ export class MCPConnectionFactory { const isRequestRecovery = eventName === 'oauthReauthenticationRequired'; let recoveryPhase: OAuthRecoveryPhase = 'silent-refresh'; let eventHandling: Promise | null = null; + /** Tokens a silent refresh issued while this connection was being established. */ + let renewedTokens: MCPOAuthTokens | null = null; const handleOAuthEvent = async (data: OAuthRequiredEvent) => { logger.info(`${this.logPrefix} oauthRequired event received`); @@ -1704,7 +1734,15 @@ export class MCPConnectionFactory { if (!isRequestRecovery || recoveryPhase === 'silent-refresh') { recoveryPhase = 'interactive'; - if (!data.skipSilentRefresh && this.shouldAttemptSilentTokenRefresh(data)) { + if ( + !isRequestRecovery && + renewedTokens != null && + (await this.isRejectedRenewal(renewedTokens, rejectedCredentialSetId)) + ) { + logger.info( + `${this.logPrefix} Server rejected the tokens a silent refresh just issued; starting interactive OAuth`, + ); + } else if (!data.skipSilentRefresh && this.shouldAttemptSilentTokenRefresh(data)) { let refreshedTokens: MCPOAuthTokens | null; try { refreshedTokens = await this.attemptSilentTokenRefresh(rejectedCredentialSetId); @@ -1727,6 +1765,9 @@ export class MCPConnectionFactory { return; } if (refreshedTokens) { + if (!isRequestRecovery) { + renewedTokens = refreshedTokens; + } connection.setOAuthTokens(refreshedTokens); connection.emit('oauthHandled', 'silent-refresh' satisfies t.OAuthHandledSource); return; diff --git a/packages/api/src/mcp/__tests__/MCPConnectionFactory.oauthSdk.integration.test.ts b/packages/api/src/mcp/__tests__/MCPConnectionFactory.oauthSdk.integration.test.ts index 95e17af2132..07aff464b52 100644 --- a/packages/api/src/mcp/__tests__/MCPConnectionFactory.oauthSdk.integration.test.ts +++ b/packages/api/src/mcp/__tests__/MCPConnectionFactory.oauthSdk.integration.test.ts @@ -495,6 +495,53 @@ describe('MCPConnectionFactory OAuth against real SDK Streamable HTTP server', ( } }); + it('starts OAuth once the resource rejects the tokens a refresh issued while connecting', async () => { + server = await createOAuthMCPServer({ + issueRefreshTokens: true, + requireResourceParameter: true, + tokenScopes: ['read'], + scopesSupported: ['read'], + rejectRefreshTokens: 10, + }); + const initialTokens = await issueTokens(server); + await storeTokens(tokenStore, server, initialTokens); + server.issuedTokens.delete(initialTokens.access_token); + + const oauthStart = jest.fn(async (_authorizationUrl: string): Promise => undefined); + await expect( + MCPConnectionFactory.create( + { + serverName: SERVER_NAME, + serverConfig: { + type: 'streamable-http', + url: server.url, + initTimeout: 15000, + }, + }, + { + useOAuth: true, + user: { id: USER_ID } as IUser, + flowManager: createFlowManager(), + tokenMethods: { + findToken: tokenStore.findToken, + createToken: tokenStore.createToken, + updateToken: tokenStore.updateToken, + deleteTokens: tokenStore.deleteTokens, + }, + returnOnOAuth: true, + oauthStart, + }, + ), + ).rejects.toThrow(); + + expect( + server.tokenRequests.filter((request) => request.grantType === 'refresh_token'), + ).toHaveLength(1); + expect(oauthStart).toHaveBeenCalledTimes(1); + const authorizationUrl = new URL(oauthStart.mock.calls[0][0]); + expect(authorizationUrl.searchParams.get('resource')).toBe(server.resourceUrl); + }); + it('does not silently refresh an SDK insufficient_scope challenge before starting OAuth', async () => { server = await createOAuthMCPServer({ issueRefreshTokens: true, diff --git a/packages/api/src/mcp/__tests__/MCPConnectionFactory.test.ts b/packages/api/src/mcp/__tests__/MCPConnectionFactory.test.ts index 674ac4f6164..787270c1dd8 100644 --- a/packages/api/src/mcp/__tests__/MCPConnectionFactory.test.ts +++ b/packages/api/src/mcp/__tests__/MCPConnectionFactory.test.ts @@ -2156,6 +2156,117 @@ describe('MCPConnectionFactory', () => { expect(oauthOptions.oauthStart).not.toHaveBeenCalled(); }); + describe('when the server rejects the tokens a silent refresh issued', () => { + const renewedTokens: MCPOAuthTokens = { + access_token: 'renewed-access', + refresh_token: 'refresh123', + token_type: 'Bearer', + obtained_at: Date.now(), + credential_set_id: 'grant-1', + }; + const challenge = { + serverUrl: 'https://api.example.com', + rejectedCredentialSetId: 'grant-1', + }; + + async function captureEstablishmentHandler(oauthStart: jest.Mock) { + let oauthRequiredHandler: ((data: Record) => Promise) | undefined; + mockConnectionInstance.on.mockImplementation((event, handler) => { + if (event === 'oauthRequired') { + oauthRequiredHandler = handler as (data: Record) => Promise; + } + return mockConnectionInstance; + }); + mockConnectionInstance.isConnected.mockResolvedValue(false); + mockMCPOAuthHandler.generateFlowId.mockReturnValue('flow123'); + mockMCPTokenStorage.forceRefreshTokens.mockResolvedValue(renewedTokens); + mockMCPOAuthHandler.initiateOAuthFlow.mockResolvedValue({ + authorizationUrl: 'https://auth.example.com', + flowId: 'flow123', + flowMetadata: { + serverName: 'test-server', + userId: 'user123', + serverUrl: 'https://api.example.com', + state: 'random-state', + }, + }); + mockFlowManager.getFlowState.mockResolvedValue(null); + mockFlowManager.createFlow.mockReturnValue(new Promise(() => {})); + + try { + await MCPConnectionFactory.create( + { + serverName: 'test-server', + serverConfig: { + ...mockServerConfig, + url: 'https://api.example.com', + type: 'sse' as const, + } as t.SSEOptions, + }, + { + useOAuth: true, + user: mockUser, + flowManager: mockFlowManager, + returnOnOAuth: true, + oauthStart, + tokenMethods: { + findToken: jest.fn(), + createToken: jest.fn(), + updateToken: jest.fn(), + deleteTokens: jest.fn(), + }, + }, + ); + } catch { + // Expected: the mocked connection never reports connected + } + return oauthRequiredHandler!; + } + + it('starts interactive OAuth instead of refreshing the rejected renewal again', async () => { + const oauthStart = jest.fn(); + const handler = await captureEstablishmentHandler(oauthStart); + + await handler(challenge); + await handler(challenge); + + expect(mockMCPTokenStorage.forceRefreshTokens).toHaveBeenCalledTimes(1); + expect(mockMCPTokenStorage.isCurrentAccessToken).toHaveBeenCalledWith( + expect.objectContaining({ accessToken: 'renewed-access', credentialSetId: 'grant-1' }), + ); + expect(mockConnectionInstance.emit).toHaveBeenCalledWith('oauthHandled', 'silent-refresh'); + expect(oauthStart).toHaveBeenCalledWith('https://auth.example.com'); + expect(mockConnectionInstance.emit).toHaveBeenCalledWith( + 'oauthFailed', + expect.objectContaining({ message: 'OAuth flow initiated - return early' }), + ); + }); + + it('refreshes again when another request replaced the renewal', async () => { + const oauthStart = jest.fn(); + const handler = await captureEstablishmentHandler(oauthStart); + mockMCPTokenStorage.isCurrentAccessToken.mockResolvedValue(false); + + await handler(challenge); + await handler(challenge); + + expect(mockMCPTokenStorage.forceRefreshTokens).toHaveBeenCalledTimes(2); + expect(mockMCPOAuthHandler.initiateOAuthFlow).not.toHaveBeenCalled(); + expect(oauthStart).not.toHaveBeenCalled(); + }); + + it('refreshes a credential set the renewal did not issue', async () => { + const oauthStart = jest.fn(); + const handler = await captureEstablishmentHandler(oauthStart); + + await handler(challenge); + await handler({ ...challenge, rejectedCredentialSetId: 'grant-2' }); + + expect(mockMCPTokenStorage.forceRefreshTokens).toHaveBeenCalledTimes(2); + expect(oauthStart).not.toHaveBeenCalled(); + }); + }); + it('should fall back to interactive OAuth when silent refresh throws unexpectedly', async () => { const sseConfig = { ...mockServerConfig, From 5c72d4146e41898d74974a2870f5f6d056d5bc47 Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Fri, 18 Sep 2026 17:29:39 +0200 Subject: [PATCH 05/27] =?UTF-8?q?=F0=9F=93=B1=20fix:=20Fit=20the=20Tool=20?= =?UTF-8?q?Library=20and=20Skills=20Pickers=20to=20Small=20Screens=20(#160?= =?UTF-8?q?67)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 📱 fix: Fit the Tool Library and Skills Pickers to Small Screens The Tool Library dialog assumed a viewport wide enough for its 224px navigation rail: below `md` the rail took more than half of a 390px screen and the card grid was squeezed into the remainder. The rail is now `md`-only, and the same kind and view navigation renders as a horizontally scrollable chip row under the search field. Card and row actions were hover-only, so Configure, Favorite and Remove were unreachable on a touch screen. They stay hidden until hover only where hover exists (`@media (hover: hover)`), and are always visible otherwise. The Skills picker header put the create button, the filter field and the view radio on one row, where the field collapsed to 50px beside a `shrink-0` radio. The view radio and the create button now sit on the title row, the create button flush right, and the filter field owns the row below them. The dialogs fill the viewport below `md` and switch `vh` to `dvh`, so mobile browser chrome no longer cuts off the bottom of the panel. * ♿ fix: Gate Hidden Tool Actions on Touch, Not on Hover Hiding an action until hover has to ask whether a finger can reach it, and `(hover: hover)` does not answer that: on a 2-in-1 it is true because it describes the trackpad, while the touchscreen sits right next to it. Tool cards and tool rows gated their Configure, Favorite and Remove controls on that query, so a finger user of such a device saw no actions at all. `packages/client/tailwind.preset.cjs` already carries `touch` for this exact distinction, `(any-pointer: coarse)` rather than `(pointer: coarse)`. This adds its inverse, `no-touch`, and gates the hidden state on it, so the hide applies only where no coarse pointer exists at all and hover no longer takes part in the decision. * 📐 fix: Split the Skills Header Onto Two Rows Below md The Skills header put the create button, the filter field and the view radio on one row. The radio's segments are `whitespace-nowrap`, so the group never narrows below 261px, and the field — `flex-1`, basis 0 — absorbed the shortfall and measured 50px on a 390px screen. Below md the radio and the create button now take the first line, the create button flush right, and the field takes the second. From md the row is exactly what it was: create button, field, category, radio. One DOM order serves both through `order-*`, so the radio and the create button each exist once. * 🧷 fix: Let the Skills View Radio Wrap Instead of Clipping The three view segments carry `whitespace-nowrap` and `px-4`, so the group has a hard 261px minimum in English and more in longer locales. On a 360px phone that group and the 42px create button exceed the dialog, which is `overflow-hidden`, and the trailing option becomes unreachable. The group now uses the primitive's `wrap` variant and may shrink, so the segments flow onto a second row; at 390px they still occupy one. `wrap` reproduced `inset-y-1` by assuming the group had no vertical padding, so turning it on for a `p-1` group shrank the moving indicator by 8px and left it floating inside its segment. It now measures that padding from the first row's offset, which leaves an unpadded group's geometry unchanged. * 🧪 test: Cover the Small-Screen Tool Library and Skills Header Ten mock-harness scenarios for the behaviour this branch changes, one per acceptance scenario id, asserting rendered geometry and computed styles rather than class strings. The tool library file covers a phone viewport (the dialog fills it, the rail is gone, the chip row filters, and no card crosses the viewport edge — the 744px grid-column overflow), the action gate on all three pointer profiles (visible with a coarse pointer, hover-gated with a mouse, revealed by keyboard focus), the selected tool row, and the unchanged desktop rail. The skills file covers the two-row header below md, the wrapped view radio at 320 and 280 CSS px with every option inside the dialog and the indicator on its checked segment, and the single desktop row. * 🧪 test: Pin the Scenario Contexts and Settle Their Geometry The verifier runs every tagged test in three projects, one of which is a touch phone, so the mouse scenarios asserted a mouse on a device that reports a coarse pointer, and the desktop rail on a 412px screen. They now pin their own viewport and pointer instead of inheriting the project. Geometry and opacity were also read mid-animation: the dialog scales as it opens, which put its left edge at 1.5px rather than 0, and the actions fade over the shared motion duration, which caught keyboard focus at 0.84. Both now poll for the resting value. With nothing selected the skills section offers only the dashed empty-state card, whose accessible name carries the hint line, so the exact-name locator for Add skill never matched. * 📏 fix: Keep the Tool Library Shell and Its Body the Same Height Giving the shell an explicit `md:h-[88vh]` while the body kept its 840px ceiling let the two disagree: on a viewport taller than about 955px the difference became dead space below the catalog, 216px at 1280x1200, and below `md` the full-height sheet left 293px on a 744x1133 tablet. The body carries the height from `md` again, the way it did before this change, so the shell wraps it. Below `md` the shell stays the full-bleed sheet and the body is capped to the same `100dvh`, which it needs because its `h-full` resolves against a grid area sized to the whole catalog. Measured shell and body heights now agree exactly: 840 at 1280x1200, 757 at 1280x860, 1133 at 744x1133 and 844 at 390x844, with the catalog still scrolling inside its own container. * 🎯 fix: Keep the Search Field Clear of the Dialog Close Button `md:px-6` on the search row is a responsive variant, so it is emitted after the base utilities and reset the `pr-12` that reserves room for the close button: on desktop the row's right padding dropped from 48px to 24px and the field ran 16px underneath the button. It now sets only the left side at `md`. Measured gap between the field's right edge and the button: 8px at 1280x860 and at 390x844, with no vertical overlap. Asserted inside the phone-viewport and desktop-rail scenarios rather than as a new id, since it is the same surface. * ⌨️ fix: Read the Skills Header in Tab Order and Keep the Chip Row Draggable Two defects with the same shape: a control that the eye can reach and the keyboard or the mouse cannot. The Skills header used `order-*` to serve both layouts from one DOM order, which left tab order disagreeing with the visual order from `md`: focus went from the rightmost radio group back to the create button and then into the middle. The breakpoint now selects the DOM order instead, the way the marketplace dialog already selects its rail, so each layout reads in the order it renders. Every control is still built once, so there is one radio group and one create button at any width. The mobile chip row hid its scrollbar unconditionally. A narrow desktop window is below `md` as well, and there a wheel scrolls the page rather than the row, so Made by you and Favorites could not be reached with a mouse at all. The hiding is now gated on `touch`, the same `(any-pointer: coarse)` query the rest of this branch uses. Measured: desktop DOM order create, field, radio and mobile radio, create, field, each matching its layout; `scrollbar-width` resolves to `auto` in a 520px mouse window whose row overflows, and to `none` on a touch phone. * 👆 fix: Give the Touch Layout Touch Geometry Two findings with one cause: the coarse-pointer layout inherited the mouse layout's geometry after this branch changed what it shows and in what order. Card actions no longer fade out where a finger can reach them, but they are absolutely positioned over the card's content, so on touch they covered the last line of a three-line description for as long as the card was on screen - an overlap that used to be transient. The card now reserves a strip for them there, `touch:h-36` with `touch:pb-9` on the content, which keeps all three lines. The chip row is that layout's primary navigation and exists only where a finger is what reaches it, yet its chips were about 30px tall and its create control 32px, while the repository already exposes the 44px `theme-control-touch` size for this. Both now carry that floor through `touch:`, and the mouse layout keeps its compact sizing. Measured on a 390x844 touch profile: all eight chips 44px, the card 144px, and the description's box clear of the action cluster. Both are asserted in the phone-viewport and touch-action scenarios. --- .../Agents/Tools/ItemDialog/ItemDialog.tsx | 4 +- .../Agents/Tools/MarketplaceSidebar.tsx | 289 +++++++++++------ .../SidePanel/Agents/Tools/SkillsDialog.tsx | 118 ++++--- .../SidePanel/Agents/Tools/ToolCard.tsx | 17 +- .../SidePanel/Agents/Tools/ToolRow.tsx | 4 +- .../Agents/Tools/ToolsMarketplaceDialog.tsx | 58 +++- .../Tools/__tests__/SkillsDialog.spec.tsx | 3 + .../Agents/Tools/__tests__/ToolCard.spec.tsx | 1 - .../__tests__/ToolsMarketplaceDialog.spec.tsx | 12 + .../skills-picker-narrow-header.spec.ts | 214 +++++++++++++ .../tool-library-small-screen.spec.ts | 294 ++++++++++++++++++ packages/client/src/components/Radio.tsx | 17 +- packages/client/src/theme/tailwind.spec.js | 3 + packages/client/tailwind.preset.cjs | 13 + 14 files changed, 880 insertions(+), 167 deletions(-) create mode 100644 e2e/specs/mock/scenarios/skills-picker-narrow-header.spec.ts create mode 100644 e2e/specs/mock/scenarios/tool-library-small-screen.spec.ts diff --git a/client/src/components/SidePanel/Agents/Tools/ItemDialog/ItemDialog.tsx b/client/src/components/SidePanel/Agents/Tools/ItemDialog/ItemDialog.tsx index 5d96880ebba..9bc726a286d 100644 --- a/client/src/components/SidePanel/Agents/Tools/ItemDialog/ItemDialog.tsx +++ b/client/src/components/SidePanel/Agents/Tools/ItemDialog/ItemDialog.tsx @@ -16,13 +16,13 @@ export default function ItemDialog({ item, agentId, onClose }: Props) { !next && onClose()}> {item && ( -
+
void; } -interface SidebarItemProps { +interface SidebarEntry { + id: string; icon: ReactNode; label: string; active: boolean; @@ -30,39 +31,90 @@ interface SidebarItemProps { count?: number; } -function SidebarItem({ icon, label, active, onClick, count }: SidebarItemProps) { - return ( - - ); -} - -export default function MarketplaceSidebar({ +/** Kind and view navigation shared by the desktop rail and the mobile chip row. */ +function useMarketplaceNav({ activeView, activeKind, onSelectView, onSelectKind, counts, totalCount, - onCreateNew, }: MarketplaceSidebarProps) { const localize = useLocalize(); - const [createOpen, setCreateOpen] = useState(false); + const selectKind = (kind: Kind) => { + onSelectView('marketplace'); + onSelectKind(kind); + }; + const selectView = (view: View) => { + onSelectView(view); + onSelectKind('all'); + }; + + const kindEntries: SidebarEntry[] = [ + { + id: 'all', + icon: , + label: localize('com_ui_all_proper'), + active: activeKind === 'all' && activeView === 'marketplace', + onClick: () => selectKind('all'), + count: totalCount, + }, + { + id: 'builtin', + icon: , + label: localize('com_ui_tools_kind_official'), + active: activeKind === 'builtin' && activeView === 'marketplace', + onClick: () => selectKind('builtin'), + count: counts.builtin, + }, + { + id: 'tool', + icon: , + label: localize('com_ui_tools_kind_tools'), + active: activeKind === 'tool' && activeView === 'marketplace', + onClick: () => selectKind('tool'), + count: counts.tool, + }, + { + id: 'mcp', + icon: , + label: localize('com_ui_tools_kind_mcp'), + active: activeKind === 'mcp' && activeView === 'marketplace', + onClick: () => selectKind('mcp'), + count: counts.mcp, + }, + { + id: 'action', + icon: , + label: localize('com_ui_tools_kind_actions'), + active: activeKind === 'action' && activeView === 'marketplace', + onClick: () => selectKind('action'), + count: counts.action, + }, + ]; + + const viewEntries: SidebarEntry[] = [ + { + id: 'mine', + icon: , + label: localize('com_ui_tools_view_made_by_you'), + active: activeView === 'mine', + onClick: () => selectView('mine'), + }, + { + id: 'favorites', + icon: , + label: localize('com_ui_tools_view_favorites'), + active: activeView === 'favorites', + onClick: () => selectView('favorites'), + }, + ]; + + return { kindEntries, viewEntries }; +} + +function useCreateItems(onCreateNew?: (kind: 'mcp' | 'action') => void) { + const localize = useLocalize(); const { agentsConfig } = useAgentPanelContext(); const hasMcpCreateAccess = useHasAccess({ permissionType: PermissionTypes.MCP_SERVERS, @@ -73,7 +125,7 @@ export default function MarketplaceSidebar({ [agentsConfig], ); - const createItems = useMemo(() => { + return useMemo(() => { const items: Array<{ label: string; icon: ReactNode; onClick: () => void }> = []; if (hasMcpCreateAccess) { items.push({ @@ -91,6 +143,61 @@ export default function MarketplaceSidebar({ } return items; }, [localize, onCreateNew, hasMcpCreateAccess, actionsEnabled]); +} + +function SidebarItem({ icon, label, active, onClick, count }: SidebarEntry) { + return ( + + ); +} + +/** The chip row is this layout's primary navigation, and it only exists where a + * finger is what reaches it, so the chips carry the shared tap-target floor + * rather than the compact height a mouse would be happy with. */ +function SidebarChip({ icon, label, active, onClick, count }: SidebarEntry) { + return ( + + ); +} + +export default function MarketplaceSidebar(props: MarketplaceSidebarProps) { + const localize = useLocalize(); + const [createOpen, setCreateOpen] = useState(false); + const { kindEntries, viewEntries } = useMarketplaceNav(props); + const createItems = useCreateItems(props.onCreateNew); return ( ); } + +/** Mobile stand-in for the sidebar rail: a horizontally scrollable chip row + * rendered under the search field when the rail is hidden. */ +export function MarketplaceFilterBar(props: MarketplaceSidebarProps) { + const localize = useLocalize(); + const [createOpen, setCreateOpen] = useState(false); + const { kindEntries, viewEntries } = useMarketplaceNav(props); + const createItems = useCreateItems(props.onCreateNew); + + return ( + /* The scrollbar hides only where a finger can drag the row instead. A narrow + desktop window is below md too, and there a wheel scrolls the page rather + than this row, so the bar is the only way to reach the trailing views. */ +
+ {createItems.length > 0 && ( + +
+ ); +} diff --git a/client/src/components/SidePanel/Agents/Tools/SkillsDialog.tsx b/client/src/components/SidePanel/Agents/Tools/SkillsDialog.tsx index 92e343fd8e2..063481f475c 100644 --- a/client/src/components/SidePanel/Agents/Tools/SkillsDialog.tsx +++ b/client/src/components/SidePanel/Agents/Tools/SkillsDialog.tsx @@ -11,6 +11,7 @@ import { OGDialogTitle, OGDialogContent, OGDialogDescription, + useMediaQuery, } from '@librechat/client'; import type { TSkill, TSkillSummary } from 'librechat-data-provider'; import type { TranslationKeys } from '~/hooks/useLocalize'; @@ -27,6 +28,7 @@ import ItemDialog from './ItemDialog/ItemDialog'; import { applyFilter } from './items/filtering'; import CategoryFilter from './CategoryFilter'; import { itemKey } from './items/selectors'; +import { cn } from '~/utils'; interface SkillsDialogProps { open: boolean; @@ -46,6 +48,9 @@ export default function SkillsDialog({ open, onOpenChange, agentId }: SkillsDial const localize = useLocalize(); const { user } = useAuthContext(); const { control, getValues, setValue } = useFormContext(); + /** The header's two layouts read in different orders, so the breakpoint selects + * which DOM order renders; same query as the marketplace dialog's rail. */ + const isDesktop = useMediaQuery('(min-width: 768px)'); const hasSkillsAccess = useHasAccess({ permissionType: PermissionTypes.SKILLS, @@ -180,65 +185,96 @@ export default function SkillsDialog({ open, onOpenChange, agentId }: SkillsDial ? 'com_ui_no_skills_found' : undefined; + /** The header is one row from md and two below it, and the two read in different + * orders, so the breakpoint picks the DOM order rather than `order-*` reshuffling + * one. Each control is built once and placed by whichever branch renders. */ + const viewRadio = ( + { + setView(value as SkillView); + setCategory('all'); + }} + className="p-1" + aria-labelledby="skills-view-label" + /> + ); + const createButton = hasCreateAccess ? ( + + ) : null; + const filterField = ( +
+
+
+ +
+ ); + return ( {localize('com_ui_skills_dialog_description')} -
-
+
+
{localize('com_ui_skills')}
-
- {hasCreateAccess && ( - - )} -
-
- + {/* DOM order is the order each breakpoint reads in, so tab order follows + the eye: from md the row is create, field, radio, exactly as before; + below md the radio and the create button take the first line and the + field the second. `order-*` would have kept one subtree, but it left + desktop tabbing from the rightmost radio back to the create button. */} +
- { - setView(value as SkillView); - setCategory('all'); - }} - className="flex-shrink-0 p-1" - aria-labelledby="skills-view-label" - /> + {isDesktop ? ( + <> + {createButton} + {filterField} + {viewRadio} + + ) : ( + <> + {viewRadio} + {createButton} + {filterField} + + )}
-
+
{isSkillsError && (
onToggle(item)} aria-pressed={selected} className={cn( - 'flex h-full w-full cursor-pointer flex-col gap-2 rounded-2xl p-4 text-left', + 'flex h-full w-full cursor-pointer flex-col gap-2 rounded-2xl p-4 text-left touch:pb-9', 'focus:outline-none focus-visible:ring-2 focus-visible:ring-ring-primary', )} > @@ -204,8 +208,8 @@ function ToolCardImpl({ } className={cn( 'flex size-7 items-center justify-center rounded-lg text-text-secondary', - 'opacity-0 transition duration-150 hover:bg-surface-hover hover:text-text-primary', - 'group-focus-within:opacity-100 group-hover:opacity-100', + 'transition duration-150 hover:bg-surface-hover hover:text-text-primary', + 'group-focus-within:opacity-100 group-hover:opacity-100 no-touch:opacity-0', 'focus:outline-none focus-visible:opacity-100 focus-visible:ring-2 focus-visible:ring-ring-primary', )} > @@ -223,10 +227,11 @@ function ToolCardImpl({ aria-label={localize(isFavorited ? 'com_ui_unfavorite' : 'com_ui_favorite')} className={cn( 'flex size-7 items-center justify-center rounded-lg text-text-secondary', - 'opacity-0 transition duration-150 hover:bg-surface-hover hover:text-text-primary', + 'transition duration-150 hover:bg-surface-hover hover:text-text-primary', 'group-focus-within:opacity-100 group-hover:opacity-100', 'focus:outline-none focus-visible:opacity-100 focus-visible:ring-2 focus-visible:ring-ring-primary', - isFavorited && 'text-series-4 opacity-100 hover:text-series-4', + !isFavorited && 'no-touch:opacity-0', + isFavorited && 'text-series-4 hover:text-series-4', )} >