diff --git a/api/app/clients/tools/util/handleTools.js b/api/app/clients/tools/util/handleTools.js index 1c5e03340c..90bc59eb90 100644 --- a/api/app/clients/tools/util/handleTools.js +++ b/api/app/clients/tools/util/handleTools.js @@ -249,6 +249,15 @@ const loadTools = async ({ /** @type {Record} */ const toolContextMap = {}; + /** + * @type {import('@librechat/agents').CodeEnvFile[] | undefined} + * Captured by the `execute_code` factory when files are primed. Surfaced + * out of `loadTools` so client.js can seed `Graph.sessions[EXECUTE_CODE]` + * before run start — without that seed, the first `execute_code` / + * `bash_tool` call lands with empty `_injected_files` and the sandbox + * can't see the prior turn's generated artifacts. + */ + let primedCodeFiles; const requestedMCPTools = {}; /** Resolve config-source servers for the current user/tenant context */ @@ -267,6 +276,9 @@ const loadTools = async ({ if (toolContext) { toolContextMap[tool] = toolContext; } + if (files?.length) { + primedCodeFiles = files; + } return createCodeExecutionTool({ user_id: user, files }); }; continue; @@ -474,7 +486,7 @@ const loadTools = async ({ } } loadedTools.push(...(await Promise.all(mcpToolPromises)).flatMap((plugin) => plugin || [])); - return { loadedTools, toolContextMap }; + return { loadedTools, toolContextMap, primedCodeFiles }; }; module.exports = { diff --git a/api/package.json b/api/package.json index 7692c61515..75ac98d7c5 100644 --- a/api/package.json +++ b/api/package.json @@ -44,7 +44,7 @@ "@google/genai": "^1.19.0", "@keyv/redis": "^4.3.3", "@langchain/core": "^0.3.80", - "@librechat/agents": "^3.1.71", + "@librechat/agents": "^3.1.73", "@librechat/api": "*", "@librechat/data-schemas": "*", "@microsoft/microsoft-graph-client": "^3.0.7", diff --git a/api/server/controllers/agents/callbacks.js b/api/server/controllers/agents/callbacks.js index c54293f9f6..111230fcaa 100644 --- a/api/server/controllers/agents/callbacks.js +++ b/api/server/controllers/agents/callbacks.js @@ -553,7 +553,23 @@ function createToolEndCallback({ req, res, artifactPromises, streamId = null }) messageId: metadata.run_id, toolCallId: output.tool_call_id, conversationId: metadata.thread_id, - session_id: output.artifact.session_id, + /** + * Use the FILE's session_id (storage session), not the + * top-level artifact session_id (exec session). The codeapi + * worker reports two distinct ids on a tool result: + * - `artifact.session_id` is the EXEC session — the + * sandbox VM that ran the bash command. Files don't + * live there; it's torn down post-execution. + * - `file.session_id` is the STORAGE session — the + * file-server bucket prefix where artifacts actually + * live and are served from. + * `processCodeOutput` builds `/download/{session_id}/{id}`, + * so passing the exec id resolves to a path the file-server + * doesn't know about and 404s. Fall back to artifact-level + * for older worker payloads that may not populate per-file + * ids. + */ + session_id: file.session_id ?? output.artifact.session_id, }); if (!streamId && !res.headersSent) { return fileMetadata; @@ -754,7 +770,23 @@ function createResponsesToolEndCallback({ req, res, tracker, artifactPromises }) messageId: metadata.run_id, toolCallId: output.tool_call_id, conversationId: metadata.thread_id, - session_id: output.artifact.session_id, + /** + * Use the FILE's session_id (storage session), not the + * top-level artifact session_id (exec session). The codeapi + * worker reports two distinct ids on a tool result: + * - `artifact.session_id` is the EXEC session — the + * sandbox VM that ran the bash command. Files don't + * live there; it's torn down post-execution. + * - `file.session_id` is the STORAGE session — the + * file-server bucket prefix where artifacts actually + * live and are served from. + * `processCodeOutput` builds `/download/{session_id}/{id}`, + * so passing the exec id resolves to a path the file-server + * doesn't know about and 404s. Fall back to artifact-level + * for older worker payloads that may not populate per-file + * ids. + */ + session_id: file.session_id ?? output.artifact.session_id, }); if (!fileMetadata) { diff --git a/api/server/controllers/agents/client.js b/api/server/controllers/agents/client.js index 7d59bd7bec..03efa65894 100644 --- a/api/server/controllers/agents/client.js +++ b/api/server/controllers/agents/client.js @@ -31,6 +31,7 @@ const { injectSkillPrimes, isSkillPrimeMessage, buildSkillPrimeContentParts, + buildInitialToolSessions, } = require('@librechat/api'); const { Callback, @@ -815,6 +816,18 @@ class AgentClient extends BaseClient { ? await this.options.primeInvokedSkills(payload) : undefined; + /** + * Seed `Graph.sessions` with code-env files primed across every + * reachable agent (primary, handoff/addedConvo, and nested + * subagents) plus skill-priming output. The merge logic and its + * run-wide semantics live in `buildInitialToolSessions`; see that + * helper's doc for why this is intentionally NOT per-agent. + */ + const initialSessions = buildInitialToolSessions({ + skillSessions: skillPrimeResult?.initialSessions, + agents: [this.options.agent, ...(this.agentConfigs ? this.agentConfigs.values() : [])], + }); + let { messages: initialMessages, indexTokenCountMap, @@ -943,7 +956,7 @@ class AgentClient extends BaseClient { messages, indexTokenCountMap, initialSummary, - initialSessions: skillPrimeResult?.initialSessions, + initialSessions, calibrationRatio, runId: this.responseMessageId, signal: abortController.signal, diff --git a/api/server/services/Endpoints/agents/initialize.js b/api/server/services/Endpoints/agents/initialize.js index 939a0a7ff6..a2f07c783f 100644 --- a/api/server/services/Endpoints/agents/initialize.js +++ b/api/server/services/Endpoints/agents/initialize.js @@ -186,6 +186,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { ctx.accessibleSkillIds, ctx.codeEnvAvailable === true, ctx.skillPrimedIdsByName, + ctx.activeSkillNames, ); }, toolEndCallback, @@ -324,6 +325,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { tool_resources: primaryConfig.tool_resources, actionsEnabled: primaryConfig.actionsEnabled, accessibleSkillIds: primaryConfig.accessibleSkillIds, + activeSkillNames: primaryConfig.activeSkillNames, codeEnvAvailable: primaryConfig.codeEnvAvailable, skillPrimedIdsByName, }); @@ -397,6 +399,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { tool_resources: config.tool_resources, actionsEnabled: config.actionsEnabled, accessibleSkillIds: config.accessibleSkillIds, + activeSkillNames: config.activeSkillNames, codeEnvAvailable: config.codeEnvAvailable, skillPrimedIdsByName: buildSkillPrimedIdsByName( config.manualSkillPrimes, @@ -456,6 +459,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { tool_resources: config.tool_resources, actionsEnabled: config.actionsEnabled, accessibleSkillIds: config.accessibleSkillIds, + activeSkillNames: config.activeSkillNames, codeEnvAvailable: config.codeEnvAvailable, }); } @@ -562,6 +566,15 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { skillsCapabilityEnabled, ephemeralSkillsToggle, }), + /** Match the primary / handoff / addedConvo paths: forward the + * endpoint-level admin flag so `initializeAgent` can compute the + * per-agent narrowing (admin AND agent.tools includes + * execute_code) into `InitializedAgent.codeEnvAvailable`. Without + * this, a code-enabled subagent loaded only through + * `subagentAgentConfigs` initializes with `codeEnvAvailable: + * false`, so `bash_tool` / `read_file` sandbox fallback are + * silently gated off even though the seed walk found it. */ + codeEnvAvailable, skillStates, defaultActiveOnShare, }, @@ -594,6 +607,8 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { tool_resources: config.tool_resources, actionsEnabled: config.actionsEnabled, accessibleSkillIds: config.accessibleSkillIds, + activeSkillNames: config.activeSkillNames, + codeEnvAvailable: config.codeEnvAvailable, skillPrimedIdsByName: buildSkillPrimedIdsByName( config.manualSkillPrimes, config.alwaysApplySkillPrimes, diff --git a/api/server/services/Endpoints/agents/skillDeps.js b/api/server/services/Endpoints/agents/skillDeps.js index a05b6c5492..ee2c6841da 100644 --- a/api/server/services/Endpoints/agents/skillDeps.js +++ b/api/server/services/Endpoints/agents/skillDeps.js @@ -1,6 +1,10 @@ const { getStrategyFunctions } = require('~/server/services/Files/strategies'); const { batchUploadCodeEnvFiles } = require('~/server/services/Files/Code/crud'); -const { getSessionInfo, checkIfActive } = require('~/server/services/Files/Code/process'); +const { + getSessionInfo, + checkIfActive, + readSandboxFile, +} = require('~/server/services/Files/Code/process'); const { enrichWithSkillConfigurable } = require('@librechat/api'); const db = require('~/models'); @@ -66,6 +70,15 @@ const skillToolDeps = { updateSkillFileCodeEnvIds: db.updateSkillFileCodeEnvIds, getSkillFileByPath: db.getSkillFileByPath, updateSkillFileContent: db.updateSkillFileContent, + /** + * `read_file` falls back to a sandbox `cat` for `/mnt/data/...` paths + * and for `{firstSegment}/...` paths whose first segment isn't a known + * skill name. The handler routes through this when the agent has code + * execution enabled; the codeapi base URL comes from + * `LIBRECHAT_CODE_BASEURL` and the sandbox session id is forwarded by + * the agents-side `ToolNode` via `tc.codeSessionContext`. + */ + readSandboxFile, }; function getSkillToolDeps() { diff --git a/api/server/services/Files/Code/process.js b/api/server/services/Files/Code/process.js index 992d56309f..72460e661e 100644 --- a/api/server/services/Files/Code/process.js +++ b/api/server/services/Files/Code/process.js @@ -162,6 +162,16 @@ const processCodeOutput = async ({ ); } + /** + * Preserve the original `messageId` on update. Each `processCodeOutput` + * call would otherwise overwrite it with the current run's run id, which + * decouples the file from the assistant message that originally created + * it. `getCodeGeneratedFiles` filters by `messageId IN `, so a + * stale id (e.g. from a later regeneration / failed re-read attempt) + * silently excludes the file from priming on subsequent turns. + */ + const persistedMessageId = isUpdate ? (claimed.messageId ?? messageId) : messageId; + if (isImage) { const usage = isUpdate ? (claimed.usage ?? 0) + 1 : 1; const _file = await convertImage(req, buffer, 'high', `${file_id}${fileExt}`); @@ -170,7 +180,7 @@ const processCodeOutput = async ({ ..._file, filepath, file_id, - messageId, + messageId: persistedMessageId, usage, filename: safeName, conversationId, @@ -230,7 +240,7 @@ const processCodeOutput = async ({ const file = { file_id, filepath, - messageId, + messageId: persistedMessageId, object: 'file', filename: safeName, type: mimeType, @@ -374,7 +384,19 @@ const primeFiles = async (options) => { const [path, queryString] = file.metadata.fileIdentifier.split('?'); const [session_id, id] = path.split('/'); - const pushFile = () => { + /** + * `pushFile` accepts optional overrides so the reupload path can + * push the FRESH `(session_id, id)` parsed off the new + * `fileIdentifier`. Without these overrides, the closure would + * capture the stale pre-reupload refs from the outer loop and + * the in-memory `files` array (now consumed by + * `buildInitialToolSessions` to seed `Graph.sessions`) would + * point at a sandbox object that no longer exists. The DB record + * gets the new identifier via `updateFile`, but the seed would + * still inject the old one — bash_tool / read_file would 404 + * trying to mount the file until the next turn re-reads metadata. + */ + const pushFile = (overrideSessionId, overrideId) => { if (!toolContext) { toolContext = `- Note: The following files are available in the "${Tools.execute_code}" tool environment:`; } @@ -389,8 +411,8 @@ const primeFiles = async (options) => { toolContext += `\n\t- /mnt/data/${file.filename}${fileSuffix}`; files.push({ - id, - session_id, + id: overrideId ?? id, + session_id: overrideSessionId ?? session_id, name: file.filename, }); }; @@ -429,8 +451,18 @@ const primeFiles = async (options) => { file_id: file.file_id, metadata: updatedMetadata, }); - sessions.set(session_id, true); - pushFile(); + /** + * Parse the FRESH fileIdentifier returned by the reupload and + * route it through both the dedupe Map and the in-memory + * `files` list. The original `(session_id, id)` parsed at the + * top of this iteration refer to the old, expired/missing + * sandbox object — using them here would silently re-introduce + * the bug `Graph.sessions` seeding is supposed to fix. + */ + const [newPath] = fileIdentifier.split('?'); + const [newSessionId, newId] = newPath.split('/'); + sessions.set(newSessionId, true); + pushFile(newSessionId, newId); } catch (error) { logger.error( `Error re-uploading file ${id} in session ${session_id}: ${error.message}`, @@ -456,9 +488,81 @@ const primeFiles = async (options) => { return { files, toolContext }; }; +/** + * Reads a single file from the code-execution sandbox by shelling `cat` + * through the sandbox `/exec` endpoint. Used by the `read_file` host + * handler when the requested path is a code-env path (`/mnt/data/...`) + * or otherwise not resolvable as a skill file. Resolves to + * `{ content }` from stdout on success, or `null` when the codeapi base + * URL isn't configured / the read returns no content (caller turns that + * into a model-visible error). Throws axios-style errors on transport + * failure so the caller can surface a meaningful error message. + * + * `session_id` and `files` come from the seeded `tc.codeSessionContext` + * (emitted by the agents-side `ToolNode` for `read_file` calls in + * v3.1.72+) so the read lands in the same sandbox session that holds + * the agent's prior-turn artifacts. + * + * @param {Object} params + * @param {string} params.file_path - Absolute path inside the sandbox (e.g. `/mnt/data/foo.txt`). + * @param {string} [params.session_id] - Sandbox session id from the seeded context. + * @param {Array<{id: string, name: string, session_id?: string}>} [params.files] - File refs to mount. + * @returns {Promise<{content: string} | null>} + */ +async function readSandboxFile({ file_path, session_id, files }) { + const baseURL = getCodeBaseURL(); + if (!baseURL) { + return null; + } + + /** Single-quote `file_path` with embedded-quote escaping so a malicious + * filename can't break out of the `cat` command. The handler upstream + * has already established this is a code-env path the model + * legitimately asked to read; this just keeps the shell quoting safe. */ + const safePath = `'${file_path.replace(/'/g, `'\\''`)}'`; + /** @type {Record} */ + const postData = { lang: 'bash', code: `cat ${safePath}` }; + if (session_id) { + postData.session_id = session_id; + } + if (files && files.length > 0) { + postData.files = files; + } + + try { + const response = await axios({ + method: 'post', + url: `${baseURL}/exec`, + data: postData, + headers: { + 'Content-Type': 'application/json', + 'User-Agent': 'LibreChat/1.0', + }, + httpAgent: codeServerHttpAgent, + httpsAgent: codeServerHttpsAgent, + timeout: 15000, + }); + const result = response?.data ?? {}; + if (result.stderr && (result.stdout == null || result.stdout === '')) { + throw new Error(String(result.stderr).trim()); + } + if (result.stdout == null) { + return null; + } + return { content: String(result.stdout) }; + } catch (error) { + logAxiosError({ + message: `Error reading sandbox file "${file_path}"`, + error, + }); + throw error; + } +} + module.exports = { primeFiles, checkIfActive, getSessionInfo, processCodeOutput, + readSandboxFile, }; diff --git a/api/server/services/Files/Code/process.spec.js b/api/server/services/Files/Code/process.spec.js index f4e79806f0..cdeee12600 100644 --- a/api/server/services/Files/Code/process.spec.js +++ b/api/server/services/Files/Code/process.spec.js @@ -51,6 +51,15 @@ jest.mock('@librechat/api', () => { getBasePath: jest.fn(() => ''), sanitizeFilename: jest.fn((name) => name), createAxiosInstance: jest.fn(() => mockAxios), + /** + * Arrow-function indirection (vs. a direct `jest.fn()` reference) so + * tests can per-case `mockReturnValueOnce` / `mockImplementationOnce` + * on `mockClassifyCodeArtifact` / `mockExtractCodeArtifactText`. + * `jest.mock(...)` is hoisted above the outer `const` declarations + * at parse time, so a direct reference here would capture + * `undefined`; the arrow defers the binding to call time. The + * direct-`jest.fn()` mocks below stay constant per file. + */ classifyCodeArtifact: (...args) => mockClassifyCodeArtifact(...args), extractCodeArtifactText: (...args) => mockExtractCodeArtifactText(...args), codeServerHttpAgent: new http.Agent({ keepAlive: false }), @@ -108,7 +117,7 @@ const { determineFileType } = require('~/server/utils'); const { logger } = require('@librechat/data-schemas'); const { codeServerHttpAgent, codeServerHttpsAgent } = require('@librechat/api'); -const { processCodeOutput, getSessionInfo } = require('./process'); +const { processCodeOutput, getSessionInfo, readSandboxFile, primeFiles } = require('./process'); describe('Code Process', () => { const mockReq = { @@ -492,6 +501,165 @@ describe('Code Process', () => { }); }); + describe('persistedMessageId (regression for cross-turn priming)', () => { + /** + * `getCodeGeneratedFiles` filters by `messageId IN ` + * to scope files to the current branch. If `processCodeOutput` overwrote + * the file's `messageId` with the current run's id on every update, a + * file re-touched by a later turn (e.g. a failed read attempt that + * re-shells the same filename) would lose its link to the assistant + * message that originally produced it. Subsequent turns then can't find + * it via `getCodeGeneratedFiles`, the priming chain has nothing to seed, + * and the model thinks its own prior-turn artifact disappeared. + * + * Contract: + * - On UPDATE (claimCodeFile returned an existing record): the persisted + * `messageId` is `claimed.messageId` (preserved). Falls back to the + * current run's `messageId` when the existing record predates the + * `messageId` field (legacy data). + * - On CREATE (new file): the persisted `messageId` is the current run's. + * - The runtime return value ALWAYS uses the current run's `messageId` + * via `Object.assign(file, { messageId, toolCallId })` so the artifact + * attaches to the correct tool_call in the live response. + */ + + /** + * `processCodeOutput` mutates the file object after `createFile` returns + * (`Object.assign(file, { messageId, toolCallId })`) so the runtime + * caller sees the live messageId on the response. Reading + * `createFile.mock.calls[0][0]` directly would therefore reflect the + * post-mutation state because JS captures by reference. To assert + * what was actually PERSISTED, snapshot the args at call time. + */ + function snapshotCreateFileArgs() { + const snapshots = []; + createFile.mockImplementation(async (file) => { + snapshots.push({ ...file }); + return {}; + }); + return snapshots; + } + + it('preserves the original messageId in the persisted record on UPDATE', async () => { + mockClaimCodeFile.mockResolvedValue({ + file_id: 'existing-id', + filename: 'sentinel.txt', + usage: 1, + createdAt: '2024-01-01T00:00:00.000Z', + messageId: 'turn-1-original-msg', + }); + const persisted = snapshotCreateFileArgs(); + + const smallBuffer = Buffer.alloc(100); + mockAxios.mockResolvedValue({ data: smallBuffer }); + + await processCodeOutput({ + ...baseParams, + name: 'sentinel.txt', + messageId: 'turn-2-current-run-msg', + }); + + expect(persisted[0].messageId).toBe('turn-1-original-msg'); + }); + + it('falls back to current run messageId on UPDATE when claimed.messageId is undefined (legacy record)', async () => { + // Legacy record predates the persistedMessageId tracking. + mockClaimCodeFile.mockResolvedValue({ + file_id: 'legacy-id', + filename: 'legacy.txt', + usage: 1, + createdAt: '2024-01-01T00:00:00.000Z', + // messageId intentionally absent + }); + const persisted = snapshotCreateFileArgs(); + + const smallBuffer = Buffer.alloc(100); + mockAxios.mockResolvedValue({ data: smallBuffer }); + + await processCodeOutput({ + ...baseParams, + name: 'legacy.txt', + messageId: 'turn-N-current-run-msg', + }); + + expect(persisted[0].messageId).toBe('turn-N-current-run-msg'); + }); + + it('uses the current run messageId on CREATE (no claimed record)', async () => { + mockClaimCodeFile.mockResolvedValue({ + file_id: 'mock-uuid-1234', + user: 'user-123', + }); + const persisted = snapshotCreateFileArgs(); + + const smallBuffer = Buffer.alloc(100); + mockAxios.mockResolvedValue({ data: smallBuffer }); + + await processCodeOutput({ + ...baseParams, + messageId: 'turn-1-create-msg', + }); + + expect(persisted[0].messageId).toBe('turn-1-create-msg'); + }); + + it('returns the CURRENT run messageId in the runtime response even on UPDATE (artifact attribution)', async () => { + // The persisted DB record keeps the original messageId, but the + // returned object surfaces the live messageId so the artifact lands + // on the correct tool_call in this run's response. + mockClaimCodeFile.mockResolvedValue({ + file_id: 'existing-id', + filename: 'sentinel.txt', + usage: 1, + createdAt: '2024-01-01T00:00:00.000Z', + messageId: 'turn-1-original-msg', + }); + const persisted = snapshotCreateFileArgs(); + + const smallBuffer = Buffer.alloc(100); + mockAxios.mockResolvedValue({ data: smallBuffer }); + + const result = await processCodeOutput({ + ...baseParams, + name: 'sentinel.txt', + messageId: 'turn-2-current-run-msg', + }); + + // DB preserves original + expect(persisted[0].messageId).toBe('turn-1-original-msg'); + // Runtime return surfaces the live (current) messageId + expect(result.messageId).toBe('turn-2-current-run-msg'); + }); + + it('preserves the original messageId on UPDATE for image files too', async () => { + // Same contract as text files; the image branch builds its own file + // record and would silently regress if the ternary diverged there. + mockClaimCodeFile.mockResolvedValue({ + file_id: 'existing-img', + filename: 'chart.png', + usage: 1, + createdAt: '2024-01-01T00:00:00.000Z', + messageId: 'turn-1-image-msg', + }); + const persisted = snapshotCreateFileArgs(); + + const imageBuffer = Buffer.alloc(500); + mockAxios.mockResolvedValue({ data: imageBuffer }); + convertImage.mockResolvedValue({ + filepath: '/uploads/chart.webp', + bytes: 400, + }); + + await processCodeOutput({ + ...baseParams, + name: 'chart.png', + messageId: 'turn-2-current-img-msg', + }); + + expect(persisted[0].messageId).toBe('turn-1-image-msg'); + }); + }); + describe('socket pool isolation', () => { it('should pass dedicated keepAlive:false agents to axios for processCodeOutput', async () => { const smallBuffer = Buffer.alloc(100); @@ -523,4 +691,323 @@ describe('Code Process', () => { }); }); }); + + describe('readSandboxFile', () => { + /** + * `readSandboxFile` shells `cat ` through the sandbox + * `/exec` endpoint. The `file_path` argument is model-controlled, so + * the single-quote escaping is a security boundary — a regression + * here would let a malicious filename break out of the `cat` + * argument and inject arbitrary shell. Lock the contract in tests. + */ + + /** Pull the bash code that the helper would send to /exec, given + * the file_path that the model supplied. */ + function execCodeFor(file_path) { + mockAxios.mockResolvedValueOnce({ data: { stdout: '', stderr: '' } }); + return readSandboxFile({ file_path }).then(() => { + const postData = mockAxios.mock.calls[0][0].data; + return postData.code; + }); + } + + describe('shell quoting (security boundary)', () => { + it('wraps a plain filename in single quotes', async () => { + const code = await execCodeFor('/mnt/data/sentinel.txt'); + expect(code).toBe(`cat '/mnt/data/sentinel.txt'`); + }); + + it("escapes a literal single-quote in the filename via the standard '\\'' sequence", async () => { + // Adversarial filename: `quote'breakout.txt`. Naive + // single-quoting would terminate the quoted string and + // inject the trailing `breakout.txt'` as shell tokens. + const code = await execCodeFor(`/mnt/data/quote'breakout.txt`); + // Expected escape: end the string, escape a literal quote, + // start a new string. POSIX-portable. + expect(code).toBe(`cat '/mnt/data/quote'\\''breakout.txt'`); + }); + + it('does not interpret command substitution syntax inside the quoted argument', async () => { + // `$(rm -rf /)` would expand if the path were unquoted or + // double-quoted. Inside POSIX single-quotes it stays literal. + const code = await execCodeFor('/mnt/data/$(rm -rf /).txt'); + expect(code).toBe(`cat '/mnt/data/$(rm -rf /).txt'`); + }); + + it('does not expand backtick command substitution inside the quoted argument', async () => { + const code = await execCodeFor('/mnt/data/`whoami`.txt'); + expect(code).toBe(`cat '/mnt/data/\`whoami\`.txt'`); + }); + + it('keeps newlines literal inside the quoted argument', async () => { + const code = await execCodeFor('/mnt/data/line1\nline2.txt'); + expect(code).toBe(`cat '/mnt/data/line1\nline2.txt'`); + }); + + it('keeps spaces and other shell metacharacters literal', async () => { + const code = await execCodeFor('/mnt/data/file ; ls -la /etc/passwd'); + expect(code).toBe(`cat '/mnt/data/file ; ls -la /etc/passwd'`); + }); + + it('handles multiple consecutive single-quotes', async () => { + const code = await execCodeFor(`a''b`); + // Each `'` becomes the 4-char escape sequence. + expect(code).toBe(`cat 'a'\\'''\\''b'`); + }); + }); + + describe('payload shape', () => { + it('POSTs to /exec on the configured codeapi base URL with bash language', async () => { + mockAxios.mockResolvedValueOnce({ data: { stdout: 'ok', stderr: '' } }); + + await readSandboxFile({ file_path: '/mnt/data/x.txt' }); + + const call = mockAxios.mock.calls[0][0]; + expect(call.method).toBe('post'); + expect(call.url).toBe('https://code-api.example.com/exec'); + expect(call.data.lang).toBe('bash'); + }); + + it('omits session_id and files when not provided', async () => { + mockAxios.mockResolvedValueOnce({ data: { stdout: '', stderr: '' } }); + + await readSandboxFile({ file_path: '/mnt/data/x.txt' }); + + const data = mockAxios.mock.calls[0][0].data; + expect(data).not.toHaveProperty('session_id'); + expect(data).not.toHaveProperty('files'); + }); + + it('forwards session_id when provided so the read lands in the seeded sandbox', async () => { + mockAxios.mockResolvedValueOnce({ data: { stdout: '', stderr: '' } }); + + await readSandboxFile({ + file_path: '/mnt/data/x.txt', + session_id: 'sess-XYZ', + }); + + expect(mockAxios.mock.calls[0][0].data.session_id).toBe('sess-XYZ'); + }); + + it('forwards files (non-empty array) so prior-turn artifacts are mounted', async () => { + mockAxios.mockResolvedValueOnce({ data: { stdout: '', stderr: '' } }); + + const files = [{ id: 'f1', name: 'sentinel.txt', session_id: 'sess-XYZ' }]; + await readSandboxFile({ + file_path: '/mnt/data/sentinel.txt', + session_id: 'sess-XYZ', + files, + }); + + expect(mockAxios.mock.calls[0][0].data.files).toEqual(files); + }); + + it('omits files when an empty array is provided (cleaner payload)', async () => { + mockAxios.mockResolvedValueOnce({ data: { stdout: '', stderr: '' } }); + + await readSandboxFile({ + file_path: '/mnt/data/x.txt', + session_id: 'sess-XYZ', + files: [], + }); + + expect(mockAxios.mock.calls[0][0].data).not.toHaveProperty('files'); + }); + + it('uses dedicated keepAlive:false agents (matches processCodeOutput pool isolation)', async () => { + mockAxios.mockResolvedValueOnce({ data: { stdout: '', stderr: '' } }); + + await readSandboxFile({ file_path: '/mnt/data/x.txt' }); + + const call = mockAxios.mock.calls[0][0]; + expect(call.httpAgent).toBe(codeServerHttpAgent); + expect(call.httpsAgent).toBe(codeServerHttpsAgent); + }); + }); + + describe('response handling', () => { + it('returns { content: stdout } on success', async () => { + mockAxios.mockResolvedValueOnce({ + data: { stdout: 'sentinel-XYZ-1234\n', stderr: '' }, + }); + + const result = await readSandboxFile({ file_path: '/mnt/data/sentinel.txt' }); + + expect(result).toEqual({ content: 'sentinel-XYZ-1234\n' }); + }); + + it('returns null when getCodeBaseURL is not configured', async () => { + const { getCodeBaseURL } = require('@librechat/agents'); + getCodeBaseURL.mockReturnValueOnce(''); + + const result = await readSandboxFile({ file_path: '/mnt/data/x.txt' }); + + expect(result).toBeNull(); + expect(mockAxios).not.toHaveBeenCalled(); + }); + + it('returns null when stdout is missing entirely (no content to surface)', async () => { + // stdout absent + no stderr = nothing to report; caller turns this + // into a model-visible "Failed to read" message. + mockAxios.mockResolvedValueOnce({ data: { stderr: '' } }); + + const result = await readSandboxFile({ file_path: '/mnt/data/x.txt' }); + + expect(result).toBeNull(); + }); + + it('throws when the command writes to stderr with no stdout (exposes the error to the caller)', async () => { + mockAxios.mockResolvedValueOnce({ + data: { stdout: '', stderr: 'cat: /mnt/data/missing.txt: No such file or directory\n' }, + }); + + await expect(readSandboxFile({ file_path: '/mnt/data/missing.txt' })).rejects.toThrow( + 'cat: /mnt/data/missing.txt: No such file or directory', + ); + }); + + it('returns stdout even when stderr is also present (stdout wins on partial-success)', async () => { + // Some `cat` builds emit warnings on stderr while still producing + // stdout (e.g. unusual line endings). Surface the content. + mockAxios.mockResolvedValueOnce({ + data: { stdout: 'partial', stderr: 'warning: ...' }, + }); + + const result = await readSandboxFile({ file_path: '/mnt/data/x.txt' }); + + expect(result).toEqual({ content: 'partial' }); + }); + + it('rethrows axios transport errors after logging via logAxiosError', async () => { + const { logAxiosError } = require('@librechat/api'); + const transportError = Object.assign(new Error('connect ECONNREFUSED'), { + code: 'ECONNREFUSED', + }); + mockAxios.mockRejectedValueOnce(transportError); + + await expect(readSandboxFile({ file_path: '/mnt/data/x.txt' })).rejects.toBe( + transportError, + ); + expect(logAxiosError).toHaveBeenCalledWith( + expect.objectContaining({ + message: expect.stringContaining('/mnt/data/x.txt'), + error: transportError, + }), + ); + }); + }); + + describe('timeout', () => { + it('uses the same 15s timeout as processCodeOutput (consistent code-server SLA)', async () => { + mockAxios.mockResolvedValueOnce({ data: { stdout: '', stderr: '' } }); + + await readSandboxFile({ file_path: '/mnt/data/x.txt' }); + + expect(mockAxios.mock.calls[0][0].timeout).toBe(15000); + }); + }); + }); + + describe('primeFiles reupload pushes FRESH sandbox ids (Pass-N review P2)', () => { + /** + * Regression: when a primed code file is missing/expired in the + * sandbox (`getSessionInfo` returns null), `primeFiles` re-uploads + * the file via `handleFileUpload` and persists the new + * `fileIdentifier`. Before the fix, the in-memory `files[]` array + * (now consumed by `buildInitialToolSessions` to seed + * `Graph.sessions`) still received the STALE `(session_id, id)` + * parsed from the original `fileIdentifier` at the top of the + * loop. The DB record was correct but the seed referenced a + * sandbox object that no longer existed — the first tool call + * 404'd trying to mount it until the next turn re-read metadata. + * + * Fix: parse the FRESH `fileIdentifier` returned by upload and + * push those ids into both the dedupe Map and the seed list. + */ + + const { getStrategyFunctions } = require('~/server/services/Files/strategies'); + const { updateFile, getFiles } = require('~/models'); + const { filterFilesByAgentAccess } = require('~/server/services/Files/permissions'); + + /** + * Mock the full strategy pair. `primeFiles` calls + * `getStrategyFunctions(file.source)` for the download stream and + * `getStrategyFunctions(FileSources.execute_code)` for the code-env + * upload — both go through the same factory in production. + */ + function setupReuploadMocks(newFileIdentifier) { + const handleFileUpload = jest.fn().mockResolvedValue(newFileIdentifier); + const getDownloadStream = jest.fn().mockResolvedValue('mock-stream'); + getStrategyFunctions.mockImplementation((source) => { + if (source === 'execute_code') return { handleFileUpload }; + return { getDownloadStream }; + }); + updateFile.mockResolvedValue({}); + filterFilesByAgentAccess.mockImplementation(({ files }) => Promise.resolve(files)); + // getSessionInfo is mocked at module level via mockAxios; return null + // to force the reupload path. Each `getSessionInfo` call hits axios. + mockAxios.mockResolvedValue({ data: null }); + return { handleFileUpload, getDownloadStream }; + } + + it('seed receives FRESH session_id + id parsed off the new fileIdentifier on reupload', async () => { + const dbFile = { + file_id: 'librechat-file-id', + filename: 'sentinel.txt', + filepath: '/uploads/sentinel.txt', + source: 'local', + context: 'execute_code', + metadata: { + /* Stale sandbox ref — this is what `getSessionInfo` will 404 on. */ + fileIdentifier: 'OLD_SESSION/OLD_ID', + }, + }; + getFiles.mockResolvedValue([dbFile]); + + setupReuploadMocks('NEW_SESSION/NEW_ID'); + + const result = await primeFiles({ + req: { user: { id: 'user-123', role: 'USER' } }, + tool_resources: { + execute_code: { file_ids: ['librechat-file-id'], files: [] }, + }, + agentId: 'agent-id', + }); + + // The seed list (consumed by buildInitialToolSessions) MUST carry + // the post-reupload ids — not the stale pre-reupload ones. + expect(result.files).toEqual([ + { id: 'NEW_ID', session_id: 'NEW_SESSION', name: 'sentinel.txt' }, + ]); + }); + + it('persists the new fileIdentifier on the DB record (existing behavior, regression-locked)', async () => { + const dbFile = { + file_id: 'librechat-file-id', + filename: 'sentinel.txt', + filepath: '/uploads/sentinel.txt', + source: 'local', + context: 'execute_code', + metadata: { fileIdentifier: 'OLD_SESSION/OLD_ID' }, + }; + getFiles.mockResolvedValue([dbFile]); + + setupReuploadMocks('NEW_SESSION/NEW_ID'); + + await primeFiles({ + req: { user: { id: 'user-123', role: 'USER' } }, + tool_resources: { + execute_code: { file_ids: ['librechat-file-id'], files: [] }, + }, + agentId: 'agent-id', + }); + + expect(updateFile).toHaveBeenCalledWith( + expect.objectContaining({ + file_id: 'librechat-file-id', + metadata: expect.objectContaining({ fileIdentifier: 'NEW_SESSION/NEW_ID' }), + }), + ); + }); + }); }); diff --git a/api/server/services/ToolService.js b/api/server/services/ToolService.js index 690af7f4e4..14243f9b21 100644 --- a/api/server/services/ToolService.js +++ b/api/server/services/ToolService.js @@ -787,12 +787,27 @@ async function loadToolDefinitionsWrapper({ req, res, agent, streamId = null, to toolContextMap[Tools.web_search] = buildWebSearchContext(); } + /** + * `files` carry the upload session_ids; we surface them so client.js can + * seed `Graph.sessions[EXECUTE_CODE]` before run start. Without that seed, + * the agents-side `ToolNode.getCodeSessionContext` returns undefined on + * call #1, `_injected_files` is never set on the tool call, and the + * sandbox can't see the prior turn's generated artifacts on first read. + */ + let primedCodeFiles; if (hasExecuteCode && tool_resources) { try { - const { toolContext } = await primeCodeFiles({ req, tool_resources, agentId: agent.id }); + const { toolContext, files } = await primeCodeFiles({ + req, + tool_resources, + agentId: agent.id, + }); if (toolContext) { toolContextMap[Tools.execute_code] = toolContext; } + if (files?.length) { + primedCodeFiles = files; + } } catch (error) { logger.error('[loadToolDefinitionsWrapper] Error priming code files:', error); } @@ -848,6 +863,7 @@ async function loadToolDefinitionsWrapper({ req, res, agent, streamId = null, to toolDefinitions, hasDeferredTools, actionsEnabled, + primedCodeFiles, }; } @@ -946,7 +962,7 @@ async function loadAgentTools({ }); } - const { loadedTools, toolContextMap } = await loadTools({ + const { loadedTools, toolContextMap, primedCodeFiles } = await loadTools({ agent, signal, userMCPAuthMap, @@ -1035,6 +1051,7 @@ async function loadAgentTools({ hasDeferredTools, actionsEnabled, tools: agentTools, + primedCodeFiles, }; } @@ -1051,6 +1068,7 @@ async function loadAgentTools({ hasDeferredTools, actionsEnabled, tools: agentTools, + primedCodeFiles, }; } @@ -1174,6 +1192,7 @@ async function loadAgentTools({ hasDeferredTools, actionsEnabled, tools: agentTools, + primedCodeFiles, }; } diff --git a/api/test/jestSetup.js b/api/test/jestSetup.js index d62b8467ed..f358027cae 100644 --- a/api/test/jestSetup.js +++ b/api/test/jestSetup.js @@ -1,3 +1,24 @@ +/** + * `undici` (transitive dep of `@librechat/agents` and others) references + * `globalThis.File` from `node:buffer`. Node 20+ exposes it as a global; + * Node 18 / certain WSL toolchains do not, which surfaces as a + * `ReferenceError: File is not defined` at module-load time the first + * time a test imports `@librechat/agents`. Mirror the polyfill in + * `packages/api/jest.setup.cjs` so this Jest suite boots on the same + * Node versions; production code never relies on this — only Jest does. + */ +if (typeof globalThis.File === 'undefined') { + try { + const { File } = require('node:buffer'); + if (File != null) { + globalThis.File = File; + } + } catch { + // Older Node versions without `node:buffer.File`. LibreChat doesn't + // support those anyway; let the test fail loudly. + } +} + // See .env.test.example for an example of the '.env.test' file. require('dotenv').config({ path: './test/.env.test' }); diff --git a/package-lock.json b/package-lock.json index 81a3085516..660682d027 100644 --- a/package-lock.json +++ b/package-lock.json @@ -59,7 +59,7 @@ "@google/genai": "^1.19.0", "@keyv/redis": "^4.3.3", "@langchain/core": "^0.3.80", - "@librechat/agents": "^3.1.71", + "@librechat/agents": "^3.1.73", "@librechat/api": "*", "@librechat/data-schemas": "*", "@microsoft/microsoft-graph-client": "^3.0.7", @@ -11894,9 +11894,9 @@ } }, "node_modules/@librechat/agents": { - "version": "3.1.71", - "resolved": "https://registry.npmjs.org/@librechat/agents/-/agents-3.1.71.tgz", - "integrity": "sha512-xAZQDlEfDJhPluBMugRoe6pZFUgfm+DIDIMCKKEg95DRcXiw8VNggkHfne/1Wf3xoA2l2Qh9RcSu/eARq55CEg==", + "version": "3.1.73", + "resolved": "https://registry.npmjs.org/@librechat/agents/-/agents-3.1.73.tgz", + "integrity": "sha512-EO2Nfj/dNrHjwOECbNGMFrhQqas2PAo1KEE1ckgykPXqWhOlHYoiUpM0zqhd6SShXHEsNgbsOq6II8vk//X4Vw==", "license": "MIT", "dependencies": { "@anthropic-ai/sdk": "^0.73.0", @@ -44232,7 +44232,7 @@ "@google/genai": "^1.19.0", "@keyv/redis": "^4.3.3", "@langchain/core": "^0.3.80", - "@librechat/agents": "^3.1.71", + "@librechat/agents": "^3.1.73", "@librechat/data-schemas": "*", "@modelcontextprotocol/sdk": "^1.29.0", "@smithy/node-http-handler": "^4.4.5", diff --git a/packages/api/jest.config.mjs b/packages/api/jest.config.mjs index 976b794122..b0029fe1a6 100644 --- a/packages/api/jest.config.mjs +++ b/packages/api/jest.config.mjs @@ -35,6 +35,7 @@ export default { // lines: 57, // }, // }, + setupFiles: ['/jest.setup.cjs'], maxWorkers: '50%', restoreMocks: true, testTimeout: 15000, diff --git a/packages/api/jest.setup.cjs b/packages/api/jest.setup.cjs new file mode 100644 index 0000000000..070aec6675 --- /dev/null +++ b/packages/api/jest.setup.cjs @@ -0,0 +1,24 @@ +/** + * `undici` (transitive dep of `@librechat/agents` and others) references + * `globalThis.File` from `node:buffer`. Node 20+ exposes it as a global; + * Node 18 / certain WSL toolchains do not, which surfaces as a + * `ReferenceError: File is not defined` at module-load time the first + * time a test imports `@librechat/agents`. Jest under those Node + * versions blows up before the suite can even start. + * + * Pull `File` from `node:buffer` (available since Node 18.x) and assign + * it onto `globalThis` if missing. Production code never depends on + * this — the polyfill only activates inside Jest. + */ +if (typeof globalThis.File === 'undefined') { + try { + const { File } = require('node:buffer'); + if (File != null) { + globalThis.File = File; + } + } catch { + // Older Node versions without `node:buffer.File`. LibreChat doesn't + // support those anyway; let the test fail loudly rather than mask a + // real environment issue. + } +} diff --git a/packages/api/package.json b/packages/api/package.json index 92414ae51d..6d42037788 100644 --- a/packages/api/package.json +++ b/packages/api/package.json @@ -95,7 +95,7 @@ "@google/genai": "^1.19.0", "@keyv/redis": "^4.3.3", "@langchain/core": "^0.3.80", - "@librechat/agents": "^3.1.71", + "@librechat/agents": "^3.1.73", "@librechat/data-schemas": "*", "@modelcontextprotocol/sdk": "^1.29.0", "@smithy/node-http-handler": "^4.4.5", diff --git a/packages/api/src/agents/codeFilesSession.spec.ts b/packages/api/src/agents/codeFilesSession.spec.ts new file mode 100644 index 0000000000..a02d5089f9 --- /dev/null +++ b/packages/api/src/agents/codeFilesSession.spec.ts @@ -0,0 +1,316 @@ +import { Constants } from '@librechat/agents'; +import type { CodeEnvFile, CodeSessionContext, ToolSessionMap } from '@librechat/agents'; +import { + buildInitialToolSessions, + seedCodeFilesIntoSessions, + type CodeFilesAgent, +} from './codeFilesSession'; + +const file = (id: string, session_id: string, name: string): CodeEnvFile => ({ + id, + session_id, + name, +}); + +describe('seedCodeFilesIntoSessions', () => { + it('returns existing map untouched when files is undefined', () => { + const existing: ToolSessionMap = new Map(); + expect(seedCodeFilesIntoSessions(undefined, existing)).toBe(existing); + }); + + it('returns existing map untouched when files is empty', () => { + const existing: ToolSessionMap = new Map(); + expect(seedCodeFilesIntoSessions([], existing)).toBe(existing); + }); + + it('returns undefined when no files and no existing map', () => { + expect(seedCodeFilesIntoSessions(undefined, undefined)).toBeUndefined(); + }); + + it('creates a new sessions map seeded with EXECUTE_CODE entry on first call', () => { + const files = [file('f1', 'sess-1', 'data.csv')]; + const result = seedCodeFilesIntoSessions(files, undefined); + + expect(result).toBeDefined(); + const entry = result!.get(Constants.EXECUTE_CODE) as CodeSessionContext; + expect(entry.session_id).toBe('sess-1'); + expect(entry.files).toEqual(files); + expect(typeof entry.lastUpdated).toBe('number'); + }); + + it('mutates the passed-in map (in-place seeding so caller can pass to createRun)', () => { + const existing: ToolSessionMap = new Map(); + const result = seedCodeFilesIntoSessions([file('f1', 'sess-1', 'a.csv')], existing); + expect(result).toBe(existing); + expect(existing.has(Constants.EXECUTE_CODE)).toBe(true); + }); + + it('appends incoming files to a prior EXECUTE_CODE entry without dropping skill files', () => { + const skillFile = file('skill-1', 'skill-sess', 'skill/util.py'); + const codeFile = file('user-1', 'user-sess', 'data.csv'); + + const existing: ToolSessionMap = new Map(); + existing.set(Constants.EXECUTE_CODE, { + session_id: 'skill-sess', + files: [skillFile], + lastUpdated: 1, + } satisfies CodeSessionContext); + + const result = seedCodeFilesIntoSessions([codeFile], existing); + const entry = result!.get(Constants.EXECUTE_CODE) as CodeSessionContext; + expect(entry.files).toEqual([skillFile, codeFile]); + expect(entry.session_id).toBe('skill-sess'); + }); + + it('preserves prior representative session_id when merging', () => { + const existing: ToolSessionMap = new Map(); + existing.set(Constants.EXECUTE_CODE, { + session_id: 'skill-sess', + files: [file('skill-1', 'skill-sess', 'a.py')], + lastUpdated: 1, + } satisfies CodeSessionContext); + + const result = seedCodeFilesIntoSessions([file('user-1', 'different-sess', 'b.csv')], existing); + const entry = result!.get(Constants.EXECUTE_CODE) as CodeSessionContext; + expect(entry.session_id).toBe('skill-sess'); + }); + + it('skips seeding when incoming files have no session_id (defensive)', () => { + const fileWithoutSession = { id: 'x', session_id: '', name: 'orphan.csv' } as CodeEnvFile; + const result = seedCodeFilesIntoSessions([fileWithoutSession], undefined); + expect(result).toBeUndefined(); + }); + + it('dedupes incoming files that share session_id + id with existing entries', () => { + /** + * Regression for Codex review #2: shared conversation files commonly + * appear in multiple agents' `primedCodeFiles`. Without dedupe, + * `_injected_files` would grow proportionally to agent count and + * inflate every `/exec` POST. + */ + const shared = file('f1', 'sess-S', 'shared.csv'); + const result = seedCodeFilesIntoSessions([shared, shared, shared], undefined); + const entry = result!.get(Constants.EXECUTE_CODE) as CodeSessionContext; + expect(entry.files).toHaveLength(1); + expect(entry.files![0].id).toBe('f1'); + }); + + it('dedupes incoming files against pre-existing entries on merge', () => { + const existing: ToolSessionMap = new Map(); + existing.set(Constants.EXECUTE_CODE, { + session_id: 'skill-sess', + files: [file('skill-1', 'skill-sess', 'a.py'), file('shared-1', 'sess-S', 'shared.csv')], + lastUpdated: 1, + } satisfies CodeSessionContext); + + const result = seedCodeFilesIntoSessions( + [ + file('shared-1', 'sess-S', 'shared.csv'), // duplicate of prior entry + file('new-1', 'sess-N', 'new.csv'), + ], + existing, + ); + const entry = result!.get(Constants.EXECUTE_CODE) as CodeSessionContext; + /* prior 2 + 1 new (the duplicate is dropped). */ + expect(entry.files).toHaveLength(3); + expect(entry.files!.map((f) => f.id).sort()).toEqual(['new-1', 'shared-1', 'skill-1']); + }); + + it('treats same name + same session as a duplicate; same name + different sessions as distinct', () => { + /** + * The dedupe key is `(session_id, id)` — not `name` alone. Two + * primed uploads can legitimately share a filename when they live + * in different sandbox sessions (e.g. each agent re-uploaded the + * same source file). Both should land in the seed. + */ + const a = file('id-A', 'sess-A', 'data.csv'); + const b = file('id-B', 'sess-B', 'data.csv'); + const result = seedCodeFilesIntoSessions([a, a, b], undefined); + const entry = result!.get(Constants.EXECUTE_CODE) as CodeSessionContext; + expect(entry.files).toHaveLength(2); + expect(entry.files!.map((f) => f.session_id).sort()).toEqual(['sess-A', 'sess-B']); + }); +}); + +describe('buildInitialToolSessions', () => { + const agent = ( + name: string, + primedCodeFiles?: CodeEnvFile[], + subagents?: CodeFilesAgent[], + ): CodeFilesAgent & { __label: string } => ({ + __label: name, + primedCodeFiles, + subagentAgentConfigs: subagents, + }); + + it('returns the skill sessions untouched when no agent contributes files', () => { + const skillSessions: ToolSessionMap = new Map(); + skillSessions.set(Constants.EXECUTE_CODE, { + session_id: 'skill-sess', + files: [file('s1', 'skill-sess', 'a.py')], + lastUpdated: 1, + } satisfies CodeSessionContext); + + const result = buildInitialToolSessions({ + skillSessions, + agents: [agent('primary'), agent('handoff')], + }); + + expect(result).toBe(skillSessions); + const entry = result!.get(Constants.EXECUTE_CODE) as CodeSessionContext; + expect(entry.files).toHaveLength(1); + }); + + it('returns undefined when no skills and no agents have primed files', () => { + const result = buildInitialToolSessions({ + skillSessions: undefined, + agents: [agent('primary')], + }); + expect(result).toBeUndefined(); + }); + + it('merges primary + handoff agents into one EXECUTE_CODE entry', () => { + const primary = agent('primary', [file('p1', 'sess-P', 'data.csv')]); + const handoff = agent('handoff', [file('h1', 'sess-H', 'report.md')]); + + const result = buildInitialToolSessions({ + agents: [primary, handoff], + }); + + const entry = result!.get(Constants.EXECUTE_CODE) as CodeSessionContext; + expect(entry.files).toHaveLength(2); + expect(entry.files!.map((f) => f.name).sort()).toEqual(['data.csv', 'report.md']); + }); + + it('walks recursively into subagentAgentConfigs (P2 fix)', () => { + /** + * Pure subagents are pruned out of `agentConfigs` after init but + * retained on the parent's `subagentAgentConfigs`. The recursive + * walk picks them up so their primed code files seed the same + * shared `Graph.sessions[EXECUTE_CODE]` entry. + */ + const grandchild = agent('grandchild', [file('g1', 'sess-G', 'nested.txt')]); + const child = agent('child', [file('c1', 'sess-C', 'mid.txt')], [grandchild]); + const primary = agent('primary', [file('p1', 'sess-P', 'top.txt')], [child]); + + const result = buildInitialToolSessions({ + agents: [primary], + }); + + const entry = result!.get(Constants.EXECUTE_CODE) as CodeSessionContext; + const names = entry.files!.map((f) => f.name).sort(); + expect(names).toEqual(['mid.txt', 'nested.txt', 'top.txt']); + }); + + it('preserves the skill side representative session_id when merging', () => { + const skillSessions: ToolSessionMap = new Map(); + skillSessions.set(Constants.EXECUTE_CODE, { + session_id: 'skill-sess', + files: [file('s1', 'skill-sess', 'a.py')], + lastUpdated: 1, + } satisfies CodeSessionContext); + + const result = buildInitialToolSessions({ + skillSessions, + agents: [agent('primary', [file('p1', 'agent-sess', 'b.csv')])], + }); + + const entry = result!.get(Constants.EXECUTE_CODE) as CodeSessionContext; + expect(entry.session_id).toBe('skill-sess'); + expect(entry.files).toHaveLength(2); + }); + + it('does not infinite-loop on a cycle in the subagent graph', () => { + /** + * Defensive: a misconfigured subagent graph can have A → B → A. + * The visited Set keyed on object identity must terminate the walk. + */ + const a = agent('A', [file('a1', 'sess-A', 'a.txt')]); + const b = agent('B', [file('b1', 'sess-B', 'b.txt')]); + a.subagentAgentConfigs = [b]; + b.subagentAgentConfigs = [a]; + + const result = buildInitialToolSessions({ agents: [a] }); + + const entry = result!.get(Constants.EXECUTE_CODE) as CodeSessionContext; + expect(entry.files!.map((f) => f.name).sort()).toEqual(['a.txt', 'b.txt']); + }); + + it('skips undefined / null entries in the agents iterable (defensive)', () => { + const result = buildInitialToolSessions({ + agents: [undefined, agent('primary', [file('p1', 'sess-P', 'x.txt')]), null] as Array< + CodeFilesAgent | undefined | null + >, + }); + + const entry = result!.get(Constants.EXECUTE_CODE) as CodeSessionContext; + expect(entry.files).toHaveLength(1); + expect(entry.files![0].name).toBe('x.txt'); + }); + + it('walks primary-first so the primary supplies the representative session_id (no skill seed)', () => { + /** + * Regression: a LIFO stack would visit the last top-level agent + * first, flipping which agent's first file becomes the + * representative `session_id` written to the EXECUTE_CODE entry. + * The walk is FIFO so the primary always lands first. + */ + const primary = agent('primary', [file('p1', 'sess-PRIMARY', 'top.txt')]); + const handoffA = agent('handoff-A', [file('a1', 'sess-A', 'a.txt')]); + const handoffB = agent('handoff-B', [file('b1', 'sess-B', 'b.txt')]); + + const result = buildInitialToolSessions({ + agents: [primary, handoffA, handoffB], + }); + + const entry = result!.get(Constants.EXECUTE_CODE) as CodeSessionContext; + expect(entry.session_id).toBe('sess-PRIMARY'); + /* All three agents still contributed their files into the merged set. */ + expect(entry.files!.map((f) => f.name).sort()).toEqual(['a.txt', 'b.txt', 'top.txt']); + /* And the per-file session_ids are preserved (ToolNode injects per-file). */ + const byName = new Map(entry.files!.map((f) => [f.name, f.session_id])); + expect(byName.get('top.txt')).toBe('sess-PRIMARY'); + expect(byName.get('a.txt')).toBe('sess-A'); + expect(byName.get('b.txt')).toBe('sess-B'); + }); + + it('dedupes shared conversation files across multiple run agents (Codex review #2)', () => { + /** + * The realistic case: a conversation with a primary + handoff + + * subagent all sharing the SAME `tool_resources.execute_code` + * file_ids. Without dedupe, the same file ref would land in + * `_injected_files` 3 times and bloat the `/exec` POST. With + * dedupe, exactly one ref reaches the sandbox. + */ + const sharedFile = file('shared-id', 'sess-shared', 'data.csv'); + const result = buildInitialToolSessions({ + agents: [ + agent('primary', [sharedFile, file('p1', 'sess-P', 'private.csv')]), + agent('handoff', [sharedFile]), + agent('subagent', [sharedFile, file('s1', 'sess-S', 'sub.csv')]), + ], + }); + + const entry = result!.get(Constants.EXECUTE_CODE) as CodeSessionContext; + expect(entry.files).toHaveLength(3); + expect(entry.files!.map((f) => f.id).sort()).toEqual(['p1', 's1', 'shared-id']); + }); + + it('deduplicates a single agent referenced as both primary and a subagent', () => { + /** + * `agentConfigs` may include an agent that is also a subagent of + * another. Visiting it twice would double-merge its files. The + * visited Set prevents that. + */ + const shared = agent('shared', [file('s1', 'sess-S', 'shared.csv')]); + const primary = agent('primary', [file('p1', 'sess-P', 'top.csv')], [shared]); + + const result = buildInitialToolSessions({ + agents: [primary, shared], + }); + + const entry = result!.get(Constants.EXECUTE_CODE) as CodeSessionContext; + expect(entry.files).toHaveLength(2); + expect(entry.files!.map((f) => f.name).sort()).toEqual(['shared.csv', 'top.csv']); + }); +}); diff --git a/packages/api/src/agents/codeFilesSession.ts b/packages/api/src/agents/codeFilesSession.ts new file mode 100644 index 0000000000..de75f2217f --- /dev/null +++ b/packages/api/src/agents/codeFilesSession.ts @@ -0,0 +1,159 @@ +import { Constants } from '@librechat/agents'; +import type { FileRefs, CodeEnvFile, ToolSessionMap, CodeSessionContext } from '@librechat/agents'; + +/** + * Minimal shape for an agent that may contribute primed code files to the + * run-wide sandbox seed. Both `InitializedAgent` and `RunAgent` satisfy it, + * and the recursive walk in {@link buildInitialToolSessions} traverses + * `subagentAgentConfigs` so nested subagents (which aren't in the top-level + * `agentConfigs` map after pure-subagent pruning) still contribute. + */ +export interface CodeFilesAgent { + primedCodeFiles?: CodeEnvFile[]; + subagentAgentConfigs?: CodeFilesAgent[]; +} + +/** + * Merges primed code-execution file references into a `ToolSessionMap` so the + * Graph's `ToolNode` can inject them into the very first `execute_code` tool + * call as `_injected_files`. Without this seed, `ToolNode.getCodeSessionContext` + * has no entry to read on call #1, and the agent-side `CodeExecutor` falls back + * to a `/files/{session_id}` fetch — but `session_id` itself is only populated + * after the first call returns one, so primed files were silently dropped. + * + * Files from `primeFiles` (api/server/services/Files/Code/process.js) carry + * per-file `session_id`s. The map's representative `session_id` is taken from + * the first incoming file (matching `primeInvokedSkills`); per-file ids on the + * `files` array are what `ToolNode` actually uses (`file.session_id ?? codeSession.session_id`). + * + * When an entry already exists (e.g. seeded by `primeInvokedSkills` for skill + * files), incoming files are appended after the existing ones. The pre-existing + * representative `session_id` is preserved so a partial-cache/fresh-prime + * collision doesn't shift which session id `ToolNode` picks for the call. + * + * Files are deduplicated by `session_id + id` as the stable identity key. + * Multiple agents in the same run commonly carry the same primed + * code-execution resources (shared conversation files), and without dedupe + * `_injected_files` would grow proportionally to agent count and inflate + * every `/exec` POST. First-seen wins so the original ordering / source + * is preserved. + */ +export function seedCodeFilesIntoSessions( + files: CodeEnvFile[] | undefined, + existing: ToolSessionMap | undefined, +): ToolSessionMap | undefined { + if (!files || files.length === 0) { + return existing; + } + + const sessions: ToolSessionMap = existing ?? new Map(); + const prior = sessions.get(Constants.EXECUTE_CODE) as CodeSessionContext | undefined; + + /** + * Compose `(session_id, id)` as a stable identity. `name` alone isn't + * sufficient — two distinct primed uploads can share a filename + * (different sessions, different file_ids). The composite stays + * cheap to compute and the keys are short uuids. + */ + const seenKeys = new Set(); + const mergedFiles: FileRefs = []; + const pushIfFresh = (f: { id?: string; session_id?: string; name?: string }): void => { + const key = `${f.session_id ?? ''}\0${f.id ?? ''}`; + if (seenKeys.has(key)) return; + seenKeys.add(key); + mergedFiles.push(f as FileRefs[number]); + }; + if (prior?.files) { + for (const f of prior.files) pushIfFresh(f); + } + for (const f of files) pushIfFresh(f); + + const representativeSessionId = prior?.session_id ?? files[0].session_id; + if (!representativeSessionId) { + return existing; + } + + sessions.set(Constants.EXECUTE_CODE, { + session_id: representativeSessionId, + files: mergedFiles, + lastUpdated: Date.now(), + } satisfies CodeSessionContext); + + return sessions; +} + +/** + * Builds the run-wide initial `ToolSessionMap` for `Graph.sessions`, + * combining skill-priming output with code-resource files primed across + * every agent that may execute code in this run. + * + * **Why "run-wide" (not per-agent):** `Graph.sessions` is a single map + * shared by every `ToolNode` instance in the run by design — the + * agents-library treats the code-execution sandbox as a conversation- + * scoped workspace, not an agent-scoped one. Two agents that both have + * code-execution enabled (a primary + a handoff target, or a parent + + * a subagent) implicitly share session_id and file refs through this + * map. This helper makes that explicit at the seeding boundary: every + * reachable agent's `primedCodeFiles` flows into the same + * `EXECUTE_CODE` entry. If per-agent isolation is ever needed, that + * has to land in the agents library first (per-agent `AgentContext` + * sessions); changing only this helper would diverge from how the + * sandbox actually behaves at runtime. + * + * **Walk order:** primary first, then `agentConfigs` (handoff/addedConvo) + * in iteration order, then recurse into each config's + * `subagentAgentConfigs` breadth-first. Order matters because when no + * skill sessions exist, the FIRST agent's first file supplies the + * representative `session_id` written to `Graph.sessions[EXECUTE_CODE]`. + * `ToolNode` ultimately uses per-file `session_id`s for injection so + * the representative is informational rather than load-bearing, but + * primary-first keeps it predictable and matches the existing + * `loadSubagentsFor` walk pattern in `Endpoints/agents/initialize.js`. + * + * The visited set is keyed by object identity (`Set`) + * so cycles in a malformed agent graph (a subagent that points back at + * its parent) can't infinite-loop the seed. + * + * @param skillSessions - Output of `primeInvokedSkills` — already + * contains an `EXECUTE_CODE` entry when skill files were primed; new + * files from this walk merge into it (representative `session_id` + * from the skill side is preserved). + * @param agents - The complete set of code-execution-capable agents in + * the run. Caller passes `[primaryConfig, ...agentConfigs.values()]`; + * this function recurses into each one's `subagentAgentConfigs`. + */ +export function buildInitialToolSessions(params: { + skillSessions?: ToolSessionMap; + agents: Iterable; +}): ToolSessionMap | undefined { + const { skillSessions, agents } = params; + let sessions = skillSessions; + const visited = new Set(); + /** + * FIFO queue: primary lands at index 0 and gets visited first, so its + * first file is what `seedCodeFilesIntoSessions` records as the + * representative `session_id` (when no skill seed exists). A LIFO + * stack (`pop()`) would visit the last top-level agent first and + * silently flip which agent supplies that id. `Array.shift()` is + * O(n); the agent set is small (handoff + subagents, typically <20) + * so the overhead is negligible vs. the readability win. + */ + const queue: CodeFilesAgent[] = []; + for (const a of agents) { + if (a) queue.push(a); + } + while (queue.length > 0) { + const agent = queue.shift()!; + if (visited.has(agent)) continue; + visited.add(agent); + if (agent.primedCodeFiles && agent.primedCodeFiles.length > 0) { + sessions = seedCodeFilesIntoSessions(agent.primedCodeFiles, sessions); + } + if (agent.subagentAgentConfigs && agent.subagentAgentConfigs.length > 0) { + for (const child of agent.subagentAgentConfigs) { + if (child && !visited.has(child)) queue.push(child); + } + } + } + return sessions; +} diff --git a/packages/api/src/agents/handlers.spec.ts b/packages/api/src/agents/handlers.spec.ts index 0f11e4e7dd..adccf519fe 100644 --- a/packages/api/src/agents/handlers.spec.ts +++ b/packages/api/src/agents/handlers.spec.ts @@ -44,6 +44,20 @@ function invokeHandler( }); } +/** + * Sentinel non-empty `accessibleSkillIds` for fixtures that need to opt + * into "skills are effectively in scope". The `read_file` handler short- + * circuits to the sandbox fallback (or errors when code env isn't + * available) whenever `accessibleSkillIds` is empty — that's the resolved + * output of `resolveAgentScopedSkillIds` (admin capability + ephemeral + * badge / persisted `skills_enabled` + ACL). Tests that mock + * `getSkillByName` directly need this so they reach the lookup. + */ +function skillsInScope(): unknown[] { + const { Types } = jest.requireActual('mongoose') as typeof import('mongoose'); + return [new Types.ObjectId()]; +} + describe('createToolExecuteHandler', () => { describe('code execution session context passthrough', () => { it('passes session_id and _injected_files from codeSessionContext to toolCallConfig', async () => { @@ -180,6 +194,7 @@ describe('createToolExecuteHandler', () => { function createSkillHandler(getSkillByName: ToolExecuteOptions['getSkillByName']) { const loadTools: ToolExecuteOptions['loadTools'] = jest.fn(async () => ({ loadedTools: [], + configurable: { accessibleSkillIds: skillsInScope() }, })); return createToolExecuteHandler({ loadTools, getSkillByName }); } @@ -289,7 +304,10 @@ describe('createToolExecuteHandler', () => { fileCount: 0, })); const handler = createToolExecuteHandler({ - loadTools: jest.fn(async () => ({ loadedTools: [] })), + loadTools: jest.fn(async () => ({ + loadedTools: [], + configurable: { accessibleSkillIds: skillsInScope() }, + })), getSkillByName, }); @@ -329,6 +347,7 @@ describe('createToolExecuteHandler', () => { loadTools: jest.fn(async () => ({ loadedTools: [], configurable: { + accessibleSkillIds: skillsInScope(), skillPrimedIdsByName: { 'manually-primed': primedHex }, }, })), @@ -375,7 +394,10 @@ describe('createToolExecuteHandler', () => { disableModelInvocation: true, })); const handler = createToolExecuteHandler({ - loadTools: jest.fn(async () => ({ loadedTools: [] })), + loadTools: jest.fn(async () => ({ + loadedTools: [], + configurable: { accessibleSkillIds: skillsInScope() }, + })), getSkillByName, getSkillFileByPath: jest.fn(), }); @@ -401,7 +423,10 @@ describe('createToolExecuteHandler', () => { fileCount: 0, })); const handler = createToolExecuteHandler({ - loadTools: jest.fn(async () => ({ loadedTools: [] })), + loadTools: jest.fn(async () => ({ + loadedTools: [], + configurable: { accessibleSkillIds: skillsInScope() }, + })), getSkillByName, }); @@ -435,6 +460,7 @@ describe('createToolExecuteHandler', () => { loadTools: jest.fn(async () => ({ loadedTools: [], configurable: { + accessibleSkillIds: skillsInScope(), skillPrimedIdsByName: { 'manual-only-skill': '507f1f77bcf86cd799439020' }, }, })), @@ -469,6 +495,7 @@ describe('createToolExecuteHandler', () => { loadTools: jest.fn(async () => ({ loadedTools: [], configurable: { + accessibleSkillIds: skillsInScope(), skillPrimedIdsByName: { 'something-else': '507f1f77bcf86cd799439030' }, }, })), @@ -505,6 +532,7 @@ describe('createToolExecuteHandler', () => { loadTools: jest.fn(async () => ({ loadedTools: [], configurable: { + accessibleSkillIds: skillsInScope(), /* Map includes the always-apply skill because `buildSkillPrimedIdsByName` now combines both prime sources. */ skillPrimedIdsByName: { @@ -543,6 +571,7 @@ describe('createToolExecuteHandler', () => { loadTools: jest.fn(async () => ({ loadedTools: [], configurable: { + accessibleSkillIds: skillsInScope(), skillPrimedIdsByName: { collides: primedHex }, }, })), @@ -639,4 +668,396 @@ describe('createToolExecuteHandler', () => { expect(listSkillFiles).toHaveBeenCalledWith(SKILL_ID); }); }); + + describe('read_file sandbox fallback (code-env paths + non-skill segments)', () => { + function makeReadFileHandler(params: { + codeEnvAvailable?: boolean; + accessibleSkillIds?: unknown[]; + activeSkillNames?: Set; + skillPrimedIdsByName?: Record; + readSandboxFile?: ToolExecuteOptions['readSandboxFile']; + getSkillByName?: ToolExecuteOptions['getSkillByName']; + }) { + const loadTools: ToolExecuteOptions['loadTools'] = jest.fn(async () => ({ + loadedTools: [], + configurable: { + codeEnvAvailable: params.codeEnvAvailable === true, + accessibleSkillIds: params.accessibleSkillIds ?? [], + activeSkillNames: params.activeSkillNames, + skillPrimedIdsByName: params.skillPrimedIdsByName, + }, + })); + return createToolExecuteHandler({ + loadTools, + getSkillByName: params.getSkillByName, + readSandboxFile: params.readSandboxFile, + }); + } + + it('routes /mnt/data/ paths to the sandbox fallback when codeEnv is available', async () => { + const readSandboxFile = jest.fn(async () => ({ content: 'hello-world' })); + const handler = makeReadFileHandler({ + codeEnvAvailable: true, + accessibleSkillIds: skillsInScope(), + readSandboxFile, + }); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_mnt_1', + name: Constants.READ_FILE, + args: { file_path: '/mnt/data/sentinel.txt' }, + codeSessionContext: { + session_id: 'sess-X', + files: [{ id: 'f1', name: 'sentinel.txt', session_id: 'sess-X' }], + }, + } as unknown as ToolCallRequest, + ]); + + expect(readSandboxFile).toHaveBeenCalledWith({ + file_path: '/mnt/data/sentinel.txt', + session_id: 'sess-X', + files: [{ id: 'f1', name: 'sentinel.txt', session_id: 'sess-X' }], + }); + expect(result.status).toBe('success'); + expect(result.content).toContain('hello-world'); + }); + + it('returns a clear error for /mnt/data/ when codeEnv is not available', async () => { + const readSandboxFile = jest.fn(); + const handler = makeReadFileHandler({ + codeEnvAvailable: false, + accessibleSkillIds: skillsInScope(), + readSandboxFile, + }); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_mnt_2', + name: Constants.READ_FILE, + args: { file_path: '/mnt/data/sentinel.txt' }, + }, + ]); + + expect(readSandboxFile).not.toHaveBeenCalled(); + expect(result.status).toBe('error'); + expect(result.errorMessage).toContain('code-execution sandbox path'); + }); + + it('falls back to sandbox when first segment is not a known skill name', async () => { + const readSandboxFile = jest.fn(async () => ({ content: 'sandbox-data' })); + const handler = makeReadFileHandler({ + codeEnvAvailable: true, + accessibleSkillIds: skillsInScope(), + activeSkillNames: new Set(['only-real-skill']), + readSandboxFile, + }); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_unknown_skill', + name: Constants.READ_FILE, + args: { file_path: 'not-a-skill/foo.md' }, + codeSessionContext: { session_id: 'sess-Y' }, + } as unknown as ToolCallRequest, + ]); + + expect(readSandboxFile).toHaveBeenCalledWith( + expect.objectContaining({ file_path: 'not-a-skill/foo.md', session_id: 'sess-Y' }), + ); + expect(result.status).toBe('success'); + expect(result.content).toContain('sandbox-data'); + }); + + it('does NOT misroute primed skills outside the catalog cap to the sandbox', async () => { + /** + * Regression for review P2 #2: manual ($-popover) and always-apply + * primes are intentionally resolved off the wider `accessibleSkillIds` + * ACL set BEFORE catalog injection — see `resolveManualSkills`. + * A primed skill name can therefore legitimately be ABSENT from + * `activeSkillNames` (which reflects the catalog after the + * `SKILL_CATALOG_LIMIT` cap and active filter). + * + * Without the primed-bypass, the `activeSkillNames` shortcut would + * misroute `read_file("primed-only-skill/references/foo.md")` to + * the sandbox even though the primed skill is in scope and + * `getSkillByName` would resolve it via the pinned `_id`. + */ + const primedHex = '507f1f77bcf86cd799439060'; + const getSkillByName = jest.fn(async () => ({ + _id: primedHex as unknown as never, + name: 'primed-only-skill', + body: '# Primed skill body', + fileCount: 1, + })); + const readSandboxFile = jest.fn(); + const getSkillFileByPath = jest.fn(async () => ({ + content: 'references content', + mimeType: 'text/markdown', + bytes: 18, + filepath: 'references/foo.md', + source: 'local', + relativePath: 'references/foo.md', + isBinary: false, + })); + const handler = createToolExecuteHandler({ + loadTools: jest.fn(async () => ({ + loadedTools: [], + configurable: { + codeEnvAvailable: true, + accessibleSkillIds: skillsInScope(), + /* Catalog visible names DO NOT include the primed skill — it's + outside the cap. */ + activeSkillNames: new Set(['some-other-skill']), + /* But the prime resolver authorized it for this turn. */ + skillPrimedIdsByName: { 'primed-only-skill': primedHex }, + }, + })), + getSkillByName, + getSkillFileByPath, + readSandboxFile, + }); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_primed_outside_catalog', + name: Constants.READ_FILE, + args: { file_path: 'primed-only-skill/references/foo.md' }, + }, + ]); + + // Primed skill resolved through the existing skill path, not the + // sandbox shortcut. + expect(readSandboxFile).not.toHaveBeenCalled(); + expect(getSkillByName).toHaveBeenCalledWith('primed-only-skill', expect.any(Array), {}); + expect(result.status).toBe('success'); + expect(result.content).toContain('references content'); + }); + + it('routes through sandbox when skills are not effectively enabled (empty accessibleSkillIds)', async () => { + /** + * `accessibleSkillIds: []` is what `resolveAgentScopedSkillIds` + * returns when the admin capability is off, the ephemeral badge is + * off, or the persisted agent has `skills_enabled !== true`. Skip + * the skill resolver entirely. + */ + const readSandboxFile = jest.fn(async () => ({ content: 'sandbox-content' })); + const getSkillByName = jest.fn(); + const handler = makeReadFileHandler({ + codeEnvAvailable: true, + accessibleSkillIds: [], + readSandboxFile, + getSkillByName, + }); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_skills_off', + name: Constants.READ_FILE, + args: { file_path: 'whatever/path.md' }, + }, + ]); + + expect(getSkillByName).not.toHaveBeenCalled(); + expect(readSandboxFile).toHaveBeenCalled(); + expect(result.status).toBe('success'); + expect(result.content).toContain('sandbox-content'); + }); + + it('returns a clear error when skills off + codeEnv off (nowhere to route)', async () => { + const handler = makeReadFileHandler({ + codeEnvAvailable: false, + accessibleSkillIds: [], + }); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_no_route', + name: Constants.READ_FILE, + args: { file_path: 'whatever/path.md' }, + }, + ]); + + expect(result.status).toBe('error'); + expect(result.errorMessage).toContain('Skill files are not available'); + }); + + it('still resolves a real skill when activeSkillNames knows the name', async () => { + const getSkillByName = jest.fn(async () => ({ + _id: 'real-skill-id' as unknown as never, + name: 'real-skill', + body: '# Real Body', + fileCount: 0, + })); + const readSandboxFile = jest.fn(); + const handler = makeReadFileHandler({ + codeEnvAvailable: true, + accessibleSkillIds: skillsInScope(), + activeSkillNames: new Set(['real-skill']), + readSandboxFile, + getSkillByName, + }); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_real_skill', + name: Constants.READ_FILE, + args: { file_path: 'real-skill/SKILL.md' }, + }, + ]); + + expect(getSkillByName).toHaveBeenCalled(); + expect(readSandboxFile).not.toHaveBeenCalled(); + expect(result.status).toBe('success'); + expect(result.content).toContain('Real Body'); + }); + + it('falls back to sandbox for trailing-slash paths (empty relativePath, codeEnv on)', async () => { + /** + * Regression for review Finding #4: `read_file("output/")` is + * malformed-but-unambiguously-not-a-skill. Previously this branch + * dead-ended with `Missing file path after skill name`; with code + * execution available it should route to the sandbox like every + * other malformed-path branch. + */ + const readSandboxFile = jest.fn(async () => ({ content: 'whatever' })); + const handler = makeReadFileHandler({ + codeEnvAvailable: true, + accessibleSkillIds: skillsInScope(), + readSandboxFile, + }); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_trailing_slash', + name: Constants.READ_FILE, + args: { file_path: 'output/' }, + }, + ]); + + expect(readSandboxFile).toHaveBeenCalledWith( + expect.objectContaining({ file_path: 'output/' }), + ); + expect(result.status).toBe('success'); + }); + + it('still errors on trailing-slash paths when codeEnv is off (no sandbox to route to)', async () => { + const readSandboxFile = jest.fn(); + const handler = makeReadFileHandler({ + codeEnvAvailable: false, + accessibleSkillIds: skillsInScope(), + readSandboxFile, + }); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_trailing_slash_no_env', + name: Constants.READ_FILE, + args: { file_path: 'output/' }, + }, + ]); + + expect(readSandboxFile).not.toHaveBeenCalled(); + expect(result.status).toBe('error'); + expect(result.errorMessage).toContain('Missing file path after skill name'); + }); + + it('hints toward bash_tool when readSandboxFile is not configured', async () => { + const handler = makeReadFileHandler({ + codeEnvAvailable: true, + accessibleSkillIds: skillsInScope(), + // readSandboxFile intentionally omitted + }); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_no_callback', + name: Constants.READ_FILE, + args: { file_path: '/mnt/data/x.txt' }, + }, + ]); + + expect(result.status).toBe('error'); + expect(result.errorMessage).toContain('bash_tool'); + }); + + it('caps sandbox fallback content at MAX_READABLE_BYTES before line-numbering (Codex review #1)', async () => { + /** + * Without the cap, `addLineNumbers` would allocate a SECOND + * full-size string with per-line prefixes, materializing ~2x + * the file in memory before downstream truncation kicks in. + * Match the skill-file path's 256KB ceiling: truncate the raw + * content first, then number, and surface the truncation to + * the model so it knows to use `bash_tool` for the rest. + */ + const oversize = 'A'.repeat(300_000); // 300KB > 256KB MAX_READABLE_BYTES + const readSandboxFile = jest.fn(async () => ({ content: oversize })); + const handler = makeReadFileHandler({ + codeEnvAvailable: true, + accessibleSkillIds: skillsInScope(), + readSandboxFile, + }); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_huge_file', + name: Constants.READ_FILE, + args: { file_path: '/mnt/data/huge.log' }, + }, + ]); + + expect(result.status).toBe('success'); + // `addLineNumbers` of a 256KB single-char run roughly preserves + // the 256KB payload size (one char per line is impossible — the + // content is one long line — so the line-prefix overhead is a + // few bytes total). Either way the prefix-stripped content + // length should NOT exceed the cap. + expect((result.content as string).length).toBeLessThan(oversize.length); + expect(result.content).toContain('truncated at 262144 bytes'); + expect(result.content).toContain('bash_tool'); + expect(result.content).toContain('huge.log'); + }); + + it('does not truncate when sandbox content is within MAX_READABLE_BYTES', async () => { + const readSandboxFile = jest.fn(async () => ({ content: 'sentinel-XYZ-1234\n' })); + const handler = makeReadFileHandler({ + codeEnvAvailable: true, + accessibleSkillIds: skillsInScope(), + readSandboxFile, + }); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_small_file', + name: Constants.READ_FILE, + args: { file_path: '/mnt/data/sentinel.txt' }, + }, + ]); + + expect(result.status).toBe('success'); + expect(result.content).not.toContain('truncated'); + expect(result.content).toContain('sentinel-XYZ-1234'); + }); + + it('surfaces sandbox fallback failures with a bash_tool retry hint', async () => { + const readSandboxFile = jest.fn(async () => null); + const handler = makeReadFileHandler({ + codeEnvAvailable: true, + accessibleSkillIds: skillsInScope(), + readSandboxFile, + }); + + const [result] = await invokeHandler(handler, [ + { + id: 'call_null_result', + name: Constants.READ_FILE, + args: { file_path: '/mnt/data/missing.txt' }, + }, + ]); + + expect(result.status).toBe('error'); + expect(result.errorMessage).toContain('Failed to read'); + expect(result.errorMessage).toContain('bash_tool'); + }); + }); }); diff --git a/packages/api/src/agents/handlers.ts b/packages/api/src/agents/handlers.ts index eb8cbb06b8..8ff903a121 100644 --- a/packages/api/src/agents/handlers.ts +++ b/packages/api/src/agents/handlers.ts @@ -119,6 +119,20 @@ export interface ToolExecuteOptions { relativePath: string, update: { content?: string; isBinary?: boolean }, ) => Promise; + /** + * Reads a code-execution sandbox file by shelling `cat` through the + * sandbox `/exec` endpoint. The host implementation supplies the + * codeapi base URL + auth and forwards the seeded `session_id` and + * `files` so the read lands in the same sandbox session that holds + * the agent's prior-turn artifacts. Returns `null` when codeapi is + * unavailable; throws on transport errors so the handler can surface + * a meaningful error message to the model. + */ + readSandboxFile?: (params: { + file_path: string; + session_id?: string; + files?: Array<{ id: string; name: string; session_id?: string }>; + }) => Promise<{ content: string } | null>; } const MAX_READABLE_BYTES = 262_144; @@ -133,6 +147,84 @@ function addLineNumbers(content: string): string { return lines.map((l, i) => `${String(i + 1).padStart(w, ' ')} | ${l}`).join('\n'); } +/** + * Routes a `read_file` call to the code-execution sandbox via the + * host-provided `readSandboxFile` callback. The sandbox session id and + * primed file refs come from `tc.codeSessionContext` (emitted by ToolNode + * for `read_file` tool calls in agents v3.1.72+) so the read lands in the + * same session that holds the agent's prior-turn artifacts. Returns a + * `ToolExecuteResult` with the file content (line-numbered) on success, + * or an instructive error pointing the model at `bash_tool` when the + * sandbox isn't reachable from this configuration. + */ +async function handleSandboxFileFallback( + tc: ToolCallRequest, + filePath: string, + options: ToolExecuteOptions, +): Promise { + const { readSandboxFile } = options; + if (!readSandboxFile) { + return { + toolCallId: tc.id, + status: 'error', + content: '', + errorMessage: `Path "${filePath}" is not a skill file. Use \`bash_tool\` to read code-execution sandbox files (e.g. \`cat ${filePath}\`).`, + }; + } + + const ctx = tc.codeSessionContext as + | { session_id?: string; files?: Array<{ id: string; name: string; session_id?: string }> } + | undefined; + try { + const result = await readSandboxFile({ + file_path: filePath, + session_id: ctx?.session_id, + files: ctx?.files, + }); + if (!result || result.content == null) { + return { + toolCallId: tc.id, + status: 'error', + content: '', + errorMessage: `Failed to read "${filePath}" from the code-execution sandbox. Try \`bash_tool\` (e.g. \`cat ${filePath}\`).`, + }; + } + /** + * Cap before line-numbering. `addLineNumbers` allocates a SECOND + * full-size string with per-line prefixes, so a multi-MB log read + * would materialize ~2x in memory before downstream truncation + * kicks in. Match the skill-file path's `MAX_READABLE_BYTES` + * (256KB) ceiling: truncate the raw content first, then number, + * and surface the truncation to the model so it can use + * `bash_tool head` / `tail` for the rest. + */ + let payload = result.content; + let truncated = false; + if (payload.length > MAX_READABLE_BYTES) { + payload = payload.slice(0, MAX_READABLE_BYTES); + truncated = true; + } + let numbered = addLineNumbers(payload); + if (truncated) { + numbered += `\n\n[truncated at ${MAX_READABLE_BYTES} bytes — use \`bash_tool\` (e.g. \`head -c\` / \`tail\`) to read the rest of "${filePath}"]`; + } + return { + toolCallId: tc.id, + status: 'success', + content: numbered, + }; + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + logger.warn(`[handleReadFileCall] Sandbox fallback failed for "${filePath}": ${message}`); + return { + toolCallId: tc.id, + status: 'error', + content: '', + errorMessage: `Error reading "${filePath}" from the code-execution sandbox: ${message}. Try \`bash_tool\` (e.g. \`cat ${filePath}\`).`, + }; + } +} + async function handleReadFileCall( tc: ToolCallRequest, mergedConfigurable: Record, @@ -151,8 +243,39 @@ async function handleReadFileCall( }; } + const codeEnvAvailable = mergedConfigurable?.codeEnvAvailable === true; + const accessibleIds = (mergedConfigurable?.accessibleSkillIds as Types.ObjectId[]) ?? []; + /** + * `accessibleSkillIds` is the resolver's authoritative output (admin + * capability AND ACL access AND ephemeral badge / persisted + * `skills_enabled`). Empty ⇒ skills are not effectively in scope, so we + * skip skill resolution entirely and route to the sandbox fallback when + * the agent has code-execution available. + */ + const skillsEffectivelyEnabled = accessibleIds.length > 0; + + /** + * Short-circuit absolute code-env paths: the path can never be a skill + * reference (skill paths are relative `{skillName}/...`), and consulting + * `getSkillByName` would just burn a DB round-trip on a guaranteed miss. + */ + if (args.file_path.startsWith('/mnt/data/')) { + if (codeEnvAvailable) { + return handleSandboxFileFallback(tc, args.file_path, options); + } + return { + toolCallId: tc.id, + status: 'error', + content: '', + errorMessage: `Path "${args.file_path}" is a code-execution sandbox path, but this agent does not have code execution enabled.`, + }; + } + const slashIdx = args.file_path.indexOf('/'); if (slashIdx < 1) { + if (codeEnvAvailable) { + return handleSandboxFileFallback(tc, args.file_path, options); + } return { toolCallId: tc.id, status: 'error', @@ -164,6 +287,15 @@ async function handleReadFileCall( const skillName = args.file_path.slice(0, slashIdx); const relativePath = args.file_path.slice(slashIdx + 1); if (!relativePath) { + /** + * `read_file("output/")`: a malformed-but-unambiguously-not-a-skill + * path. Stay consistent with the other malformed-path branches and + * route to the sandbox when code execution is available, instead of + * dead-ending with a skill-centric error message. + */ + if (codeEnvAvailable) { + return handleSandboxFileFallback(tc, args.file_path, options); + } return { toolCallId: tc.id, status: 'error', @@ -172,6 +304,67 @@ async function handleReadFileCall( }; } + /** + * Skills not in scope (admin capability off, ephemeral badge off, or + * persisted `skills_enabled !== true` — all already collapsed into + * `accessibleSkillIds.length === 0` by `resolveAgentScopedSkillIds`): + * route to the sandbox fallback when code execution is available, else + * the lookup truly has nowhere to go. + */ + if (!skillsEffectivelyEnabled) { + if (codeEnvAvailable) { + return handleSandboxFileFallback(tc, args.file_path, options); + } + return { + toolCallId: tc.id, + status: 'error', + content: '', + errorMessage: + 'Skill files are not available for this agent and code execution is not enabled.', + }; + } + + /** + * Read the primed-skills map BEFORE the `activeSkillNames` shortcut. + * + * `activeSkillNames` is the catalog-visible set after the + * `SKILL_CATALOG_LIMIT` cap and the active-state filter run in + * `injectSkillCatalog`. Manual ($-popover) primes and always-apply + * primes are intentionally resolved off the wider `accessibleSkillIds` + * ACL set BEFORE catalog injection — see `resolveManualSkills` for + * why a skill outside the catalog cap can still be authorized for + * direct manual invocation. So a primed skill name may legitimately + * be absent from `activeSkillNames`. Treat any name in + * `skillPrimedIdsByName` as "known" for the gate below; otherwise the + * shortcut would misroute `read_file("primed-skill/references/foo.md")` + * to the sandbox even though the primed skill is in scope. + */ + const skillPrimedIdsByName = + (mergedConfigurable?.skillPrimedIdsByName as Record | undefined) ?? {}; + const primedIdString = skillPrimedIdsByName[skillName]; + const isPrimedThisTurn = primedIdString != null; + + /** + * Skills are in scope, but the first segment isn't a name we know. + * Use the catalog-derived `activeSkillNames` Set (no DB read) to detect + * this and fall through to the sandbox so the model doesn't have to + * eat a wasted `read_file` error before retrying with `bash_tool`. + * Primed names bypass this shortcut even when absent from the catalog + * (see comment on `skillPrimedIdsByName` above). + */ + const activeSkillNames = mergedConfigurable?.activeSkillNames as Set | undefined; + if (activeSkillNames && !activeSkillNames.has(skillName) && !isPrimedThisTurn) { + if (codeEnvAvailable) { + return handleSandboxFileFallback(tc, args.file_path, options); + } + return { + toolCallId: tc.id, + status: 'error', + content: '', + errorMessage: `Skill "${skillName}" not found or not accessible`, + }; + } + if (!getSkillByName) { return { toolCallId: tc.id, @@ -180,12 +373,6 @@ async function handleReadFileCall( errorMessage: 'File reading is not configured', }; } - - const accessibleIds = (mergedConfigurable?.accessibleSkillIds as Types.ObjectId[]) ?? []; - const skillPrimedIdsByName = - (mergedConfigurable?.skillPrimedIdsByName as Record | undefined) ?? {}; - const primedIdString = skillPrimedIdsByName[skillName]; - const isPrimedThisTurn = primedIdString != null; /* On a primed lookup (manual `$` OR always-apply), pin the accessible set to ONLY the primed `_id`. This guarantees the doc whose body got primed is the SAME doc whose files we read, even when same-name diff --git a/packages/api/src/agents/index.ts b/packages/api/src/agents/index.ts index dd482b0689..1a6ae696a0 100644 --- a/packages/api/src/agents/index.ts +++ b/packages/api/src/agents/index.ts @@ -19,6 +19,7 @@ export * from './responses'; export * from './skills'; export * from './skillConfigurable'; export * from './skillFiles'; +export * from './codeFilesSession'; export * from './run'; export * from './tools'; export * from './validation'; diff --git a/packages/api/src/agents/initialize.ts b/packages/api/src/agents/initialize.ts index 3c8086b3a2..042bf60163 100644 --- a/packages/api/src/agents/initialize.ts +++ b/packages/api/src/agents/initialize.ts @@ -91,6 +91,13 @@ export type InitializedAgent = Agent & { codeEnvAvailable: boolean; /** Accessible skill IDs for ACL checking at execute time */ accessibleSkillIds?: import('mongoose').Types.ObjectId[]; + /** + * Names of skills the runtime can resolve, mirroring `accessibleSkillIds`. + * Surfaced to the runtime so handlers like `read_file` can decide if a + * `{firstSegment}/...` path refers to a real skill (vs. a code-env path + * that should be routed to the bash fallback) without an extra DB lookup. + */ + activeSkillNames?: Set; /** Number of skills in the catalog (used to determine if SkillTool should be registered) */ skillCount?: number; /** @@ -109,6 +116,18 @@ export type InitializedAgent = Agent & { * via `unionPrimeAllowedTools` (same pipeline as manual primes). */ alwaysApplySkillPrimes?: ResolvedAlwaysApplySkill[]; + /** + * Pre-uploaded code-env file refs from `tool_resources.execute_code` + * (carries the conversation's prior-turn generated artifacts and any + * user uploads). Captured by the `loadTools` callback; the AgentClient + * merges these across all run agents into `Graph.sessions[EXECUTE_CODE]` + * before run start so the very first `execute_code` / `bash_tool` call + * sees them as `_injected_files`. Without this seed the agents-side + * `CodeExecutor` falls back to `/files/{session_id}` — but `session_id` + * is itself only populated by a previous successful execution, so on + * call #1 the sandbox can't see the files at all. + */ + primedCodeFiles?: import('@librechat/agents').CodeEnvFile[]; }; export const DEFAULT_MAX_CONTEXT_TOKENS = 32000; @@ -150,6 +169,14 @@ export interface InitializeAgentParams { toolDefinitions?: LCTool[]; hasDeferredTools?: boolean; actionsEnabled?: boolean; + /** + * Pre-uploaded code-env file refs for the agent's + * `tool_resources.execute_code`. Bubbled up so the run host can seed + * `Graph.sessions[EXECUTE_CODE]` before the first tool call — + * otherwise `_injected_files` is empty on call #1 and prior-turn + * artifacts don't reach the sandbox. + */ + primedCodeFiles?: import('@librechat/agents').CodeEnvFile[]; } | null>; /** Endpoint option (contains model_parameters and endpoint info) */ endpointOption?: Partial; @@ -622,6 +649,7 @@ export async function initializeAgent( hasDeferredTools, actionsEnabled, tools: structuredTools, + primedCodeFiles, } = loadToolsResult ?? { tools: [], toolContextMap: {}, @@ -630,6 +658,7 @@ export async function initializeAgent( toolDefinitions: [], hasDeferredTools: false, actionsEnabled: undefined, + primedCodeFiles: undefined, }; let toolDefinitions = loadedToolDefinitions; @@ -810,6 +839,7 @@ export async function initializeAgent( * LLM (or a direct-invocation path) names one. */ let executableSkillIds = params.accessibleSkillIds; + let activeSkillNames: Set | undefined; const { accessibleSkillIds } = params; if (accessibleSkillIds && accessibleSkillIds.length > 0) { const skillResult = await injectSkillCatalog({ @@ -827,6 +857,7 @@ export async function initializeAgent( toolDefinitions = skillResult.toolDefinitions; skillCount = skillResult.skillCount; executableSkillIds = skillResult.activeSkillIds; + activeSkillNames = skillResult.activeSkillNames; } const agentMaxContextNum = Number(agentMaxContextTokens) || DEFAULT_MAX_CONTEXT_TOKENS; @@ -863,6 +894,7 @@ export async function initializeAgent( codeEnvAvailable: effectiveCodeEnvAvailable, skillCount, accessibleSkillIds: executableSkillIds, + activeSkillNames, manualSkillPrimes, alwaysApplySkillPrimes, attachments: finalAttachments, @@ -874,6 +906,7 @@ export async function initializeAgent( maxContextTokens != null && maxContextTokens > 0 ? maxContextTokens : Math.max(1024, Math.round(baseContextTokens * (1 - DEFAULT_RESERVE_RATIO))), + primedCodeFiles, }; return initializedAgent; diff --git a/packages/api/src/agents/skillConfigurable.ts b/packages/api/src/agents/skillConfigurable.ts index f9202f57e4..9157701c43 100644 --- a/packages/api/src/agents/skillConfigurable.ts +++ b/packages/api/src/agents/skillConfigurable.ts @@ -35,6 +35,17 @@ export function enrichWithSkillConfigurable( * the exact doc the resolver primed and relaxes the disable-model gate. */ skillPrimedIdsByName?: Record, + /** + * Names of skills the runtime can resolve, captured at agent init by + * `injectSkillCatalog`. Lets `read_file` decide whether a + * `{firstSegment}/...` path is a real skill reference vs. a code-env path + * that should fall back to bash, without an extra `getSkillByName` + * round-trip just to discover the name doesn't resolve. Empty/undefined + * ⇒ skills aren't effectively in scope for this agent (admin capability + * off, ephemeral badge off, or persisted `skills_enabled !== true`), + * which is the same signal as `accessibleSkillIds.length === 0`. + */ + activeSkillNames?: Set, ): { loadedTools: unknown[]; configurable: Record } { return { ...result, @@ -44,6 +55,7 @@ export function enrichWithSkillConfigurable( codeEnvAvailable, accessibleSkillIds, skillPrimedIdsByName, + activeSkillNames, }, }; } diff --git a/packages/api/src/agents/skills.ts b/packages/api/src/agents/skills.ts index 42d29aa8f9..aacc66a5d4 100644 --- a/packages/api/src/agents/skills.ts +++ b/packages/api/src/agents/skills.ts @@ -231,6 +231,15 @@ export interface InjectSkillCatalogResult { * resolvable. */ activeSkillIds: Types.ObjectId[]; + /** + * Names of skills the runtime can resolve, mirroring `activeSkillIds`. + * Surfaced so host-side handlers (e.g. `read_file`) can decide whether a + * `{firstSegment}/...` path is a real skill reference vs. a code-env path + * (`/mnt/data/...`) that should be routed to the bash fallback — without + * issuing an extra `getSkillByName` round-trip just to discover the name + * doesn't resolve. + */ + activeSkillNames: Set; } /** @@ -261,7 +270,12 @@ export async function injectSkillCatalog( } = params; if (!listSkillsByAccess || accessibleSkillIds.length === 0) { - return { toolDefinitions: inputDefs, skillCount: 0, activeSkillIds: [] }; + return { + toolDefinitions: inputDefs, + skillCount: 0, + activeSkillIds: [], + activeSkillNames: new Set(), + }; } type SkillSummary = Awaited>>['skills'][number]; @@ -321,7 +335,12 @@ export async function injectSkillCatalog( } if (activeSkills.length === 0) { - return { toolDefinitions: inputDefs, skillCount: 0, activeSkillIds: [] }; + return { + toolDefinitions: inputDefs, + skillCount: 0, + activeSkillIds: [], + activeSkillNames: new Set(), + }; } if (!reachedEnd && visibleCount < SKILL_CATALOG_LIMIT) { @@ -442,6 +461,7 @@ export async function injectSkillCatalog( toolDefinitions: workingDefs, skillCount: catalogVisibleSkills.length, activeSkillIds: executableSkills.map((s) => s._id), + activeSkillNames: new Set(executableSkills.map((s) => s.name)), }; }