From 80ce956c94ec4be4c99d12239965d2612f808392 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 9 May 2026 16:09:56 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=93=9C=20fix:=20Scope=20Read=20File=20Pro?= =?UTF-8?q?mpt=20For=20Code=20Agents=20(#13040)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: Scope read_file prompt for code agents * fix: Align code read_file prompt behavior --- .../src/agents/__tests__/initialize.test.ts | 65 ++++++++++ packages/api/src/agents/initialize.ts | 10 +- packages/api/src/agents/skills.ts | 15 +-- packages/api/src/agents/tools.spec.ts | 53 ++++++++- packages/api/src/agents/tools.ts | 111 +++++++++++++++--- 5 files changed, 225 insertions(+), 29 deletions(-) diff --git a/packages/api/src/agents/__tests__/initialize.test.ts b/packages/api/src/agents/__tests__/initialize.test.ts index 546f65068f..b98af340aa 100644 --- a/packages/api/src/agents/__tests__/initialize.test.ts +++ b/packages/api/src/agents/__tests__/initialize.test.ts @@ -5,6 +5,25 @@ */ jest.mock('@librechat/agents', () => ({ ...jest.requireActual('@librechat/agents'), + ReadFileToolDefinition: { + name: 'read_file', + description: 'read skill files using {skillName}/{filePath} and SKILL.md', + parameters: { + type: 'object', + properties: { + file_path: { + type: 'string', + description: 'For skill files: "{skillName}/{path}".', + }, + }, + }, + responseFormat: 'content', + }, + BashExecutionToolDefinition: { + name: 'bash_tool', + description: 'bash', + schema: { type: 'object', properties: {} }, + }, buildBashExecutionToolDescription: ({ enableToolOutputReferences, }: { @@ -1064,6 +1083,52 @@ describe('initializeAgent — execute_code capability expansion', () => { path — the string stays in `agent.tools` as the capability trigger but never appears in the tool definitions the LLM sees. */ expect(names).not.toContain('execute_code'); + const readFile = result.toolDefinitions?.find((d) => d.name === 'read_file'); + expect(readFile?.description).toContain('code-execution sandbox'); + expect(readFile?.description).not.toContain('{skillName}'); + expect(readFile?.description).not.toContain('SKILL.md'); + }); + + it('upgrades read_file to the skill-aware description when active skills are in scope', async () => { + const { agent, req, res, loadTools, db } = createMocks(); + agent.tools = ['execute_code']; + const { Types } = await import('mongoose'); + const skillId = new Types.ObjectId(); + const author = { toString: () => req.user?.id } as unknown as import('mongoose').Types.ObjectId; + + const result = await initializeAgent( + { + req, + res, + agent, + loadTools, + endpointOption: { endpoint: EModelEndpoint.agents }, + allowedProviders: new Set([Providers.OPENAI]), + isInitialAgent: true, + codeEnvAvailable: true, + accessibleSkillIds: [skillId], + }, + { + ...db, + listSkillsByAccess: jest.fn().mockResolvedValue({ + skills: [ + { + _id: skillId, + name: 'data-cleaner', + description: 'Clean tabular data.', + author, + }, + ], + has_more: false, + after: null, + }), + }, + ); + + const readFile = result.toolDefinitions?.find((d) => d.name === 'read_file'); + expect(readFile?.description).toContain('{skillName}/{filePath}'); + expect(readFile?.description).toContain('SKILL.md'); + expect(result.toolDefinitions?.map((d) => d.name)).toContain('skill'); }); it('does not register bash_tool + read_file when codeEnvAvailable=false', async () => { diff --git a/packages/api/src/agents/initialize.ts b/packages/api/src/agents/initialize.ts index b409156028..b4985d356e 100644 --- a/packages/api/src/agents/initialize.ts +++ b/packages/api/src/agents/initialize.ts @@ -777,7 +777,9 @@ export async function initializeAgent( * (backed by `CodeExecutionToolDefinition` + `primeCodeFiles`) is no * longer registered; the string `execute_code` on the agent document * stays as the capability-trigger marker but expands into the - * skill-flavored tool pair here. + * `bash_tool` + `read_file` pair here. The initial `read_file` + * definition is code-only; `injectSkillCatalog` upgrades it later in + * this initializer when skill files are actually available. * * `effectiveCodeEnvAvailable` is the per-agent truth: the admin-level * `params.codeEnvAvailable` AND the agent actually asking for code @@ -792,8 +794,9 @@ export async function initializeAgent( * execute-code-only agents on Google/Vertex still trip the conflict * guard when provider-specific tools are also configured. Also before * `injectSkillCatalog` so the skill path's own - * `registerCodeExecutionTools` call becomes a no-op via the registry - * `.has()` dedupe — exactly one copy of each tool reaches the LLM. + * `registerCodeExecutionTools` call upgrades `read_file` from the + * code-only description to the skill-aware description without adding a + * duplicate — exactly one copy of each tool reaches the LLM. */ const agentRequestsCodeExec = (agent.tools ?? []).includes(Tools.execute_code); const effectiveCodeEnvAvailable = params.codeEnvAvailable === true && agentRequestsCodeExec; @@ -802,6 +805,7 @@ export async function initializeAgent( toolRegistry, toolDefinitions, includeBash: true, + includeSkillFileInstructions: false, enableToolOutputReferences: effectiveCodeEnvAvailable, }); toolDefinitions = codeExecResult.toolDefinitions; diff --git a/packages/api/src/agents/skills.ts b/packages/api/src/agents/skills.ts index 2138f02b2a..45b43df8b8 100644 --- a/packages/api/src/agents/skills.ts +++ b/packages/api/src/agents/skills.ts @@ -426,10 +426,11 @@ export async function injectSkillCatalog( * `skill` tool is conditional on having anything for the model to invoke. * `read_file` + `bash_tool` go through `registerCodeExecutionTools` so * a prior registration from `initializeAgent` (for the `execute_code` - * capability) doesn't produce a duplicate copy. `read_file` is always - * included — manually-primed `disable-model-invocation: true` skills - * still need it to load their `references/*` from storage. `bash_tool` - * follows `codeEnvAvailable` as before. + * capability) upgrades to the skill-aware `read_file` definition without + * producing a duplicate copy. `read_file` is always included — + * manually-primed `disable-model-invocation: true` skills still need it + * to load their `references/*` from storage. `bash_tool` follows + * `codeEnvAvailable` as before. */ let workingDefs: LCTool[] = [...(inputDefs ?? [])]; if (catalogVisibleSkills.length > 0) { @@ -440,9 +441,9 @@ export async function injectSkillCatalog( /** * Forward `enableToolOutputReferences` to keep the skills caller * symmetric with `initializeAgent`'s call. Today `initializeAgent` - * registers `bash_tool` first and the registry `.has()` check makes - * this call a no-op — but if call order ever flips (skills-first), - * a missing flag here would silently produce a `bash_tool` + * registers `bash_tool` first and this call only keeps the existing + * bash definition — but if call order ever flips (skills-first), a + * missing flag here would silently produce a `bash_tool` * description without the `{{toolturn}}` guide, and the * `initializeAgent` pass would become the no-op. Mirror the gate * `initializeAgent` uses (`effectiveCodeEnvAvailable`, which here diff --git a/packages/api/src/agents/tools.spec.ts b/packages/api/src/agents/tools.spec.ts index da24255e9b..2ad9c30db9 100644 --- a/packages/api/src/agents/tools.spec.ts +++ b/packages/api/src/agents/tools.spec.ts @@ -8,8 +8,16 @@ jest.mock('@librechat/agents', () => ({ ...jest.requireActual('@librechat/agents'), ReadFileToolDefinition: { name: 'read_file', - description: 'read file', - parameters: { type: 'object', properties: {} }, + description: 'read skill files using {skillName}/{filePath} and SKILL.md', + parameters: { + type: 'object', + properties: { + file_path: { + type: 'string', + description: 'For skill files: "{skillName}/{path}".', + }, + }, + }, responseFormat: 'content', }, BashExecutionToolDefinition: { @@ -191,6 +199,47 @@ describe('registerCodeExecutionTools', () => { expect(toolRegistry.has('bash_tool')).toBe(false); }); + it('uses a code-only read_file description when skill instructions are disabled', () => { + const toolRegistry = makeRegistry(); + const result = registerCodeExecutionTools({ + toolRegistry, + toolDefinitions: [], + includeBash: true, + includeSkillFileInstructions: false, + }); + + const readFile = result.toolDefinitions.find((d) => d.name === 'read_file'); + expect(readFile?.description).toContain('code-execution sandbox'); + expect(readFile?.description).toContain('/mnt/data/'); + expect(readFile?.description).toContain('truncated around 256KB'); + expect(readFile?.description).toContain('may return an error'); + expect(readFile?.description).not.toContain('{skillName}'); + expect(readFile?.description).not.toContain('SKILL.md'); + expect(JSON.stringify(readFile?.parameters)).not.toContain('{skillName}'); + }); + + it('upgrades a code-only read_file definition when skills are enabled later in the run', () => { + const toolRegistry = makeRegistry(); + const codeOnly = registerCodeExecutionTools({ + toolRegistry, + toolDefinitions: [], + includeBash: true, + includeSkillFileInstructions: false, + }); + const upgraded = registerCodeExecutionTools({ + toolRegistry, + toolDefinitions: codeOnly.toolDefinitions, + includeBash: false, + includeSkillFileInstructions: true, + }); + + const readFile = upgraded.toolDefinitions.find((d) => d.name === 'read_file'); + expect(upgraded.registered).toEqual([]); + expect(readFile?.description).toContain('{skillName}/{filePath}'); + expect(readFile?.description).toContain('SKILL.md'); + expect(toolRegistry.get('read_file')?.description).toBe(readFile?.description); + }); + it('preserves pre-existing unrelated tool definitions', () => { const toolRegistry = makeRegistry(); const existing: LCTool[] = [ diff --git a/packages/api/src/agents/tools.ts b/packages/api/src/agents/tools.ts index 55d333c79e..268b7df01a 100644 --- a/packages/api/src/agents/tools.ts +++ b/packages/api/src/agents/tools.ts @@ -64,6 +64,13 @@ export interface RegisterCodeExecutionToolsParams { * no-op so there is exactly one copy of each tool in `toolDefinitions`. */ includeBash: boolean; + /** + * When `true`, `read_file` tells the model about skill-file paths + * (`{skillName}/{path}`). When `false`, it is described only as a + * code-execution sandbox reader so execute-code-only agents don't get + * prompted to discover skills that are not enabled for the run. + */ + includeSkillFileInstructions?: boolean; /** * When `true`, the registered `bash_tool` description includes the * LLM-facing `{{toolturn}}` reference syntax guide so the @@ -80,11 +87,11 @@ export interface RegisterCodeExecutionToolsResult { } /** - * Hoisted module-level definition for `read_file` so + * Hoisted module-level definition for skill-aware `read_file` so * `registerCodeExecutionTools` doesn't re-allocate on every call. The - * shape is derived entirely from a static `@librechat/agents` export — - * no per-request state — so a single frozen object is safe to share - * across every agent init. + * shape is derived from a static `@librechat/agents` export — no + * per-request state — so a single frozen object is safe to share across + * every agent init. */ const READ_FILE_DEF: LCTool = Object.freeze({ name: ReadFileToolDefinition.name, @@ -93,6 +100,47 @@ const READ_FILE_DEF: LCTool = Object.freeze({ responseFormat: ReadFileToolDefinition.responseFormat, }) as LCTool; +const CODE_READ_FILE_DESCRIPTION = `Read the contents of a file from the code-execution sandbox or from prior code-execution output. Returns text content with line numbers for easy reference. + +BEHAVIOR: +- Text files: returned with numbered lines. +- Large text files are truncated around 256KB with a note to use bash_tool for the full content. +- Binary files and formats that are not safe to serialize as text may return an error. Use bash_tool to inspect or process them. + +CONSTRAINTS: +- Only files produced by code execution or attached to the code-execution sandbox are accessible. +- Use paths returned by tool output or paths under /mnt/data/. +- Do not guess file paths. Use bash_tool to inspect available sandbox files when needed.`; + +const CODE_READ_FILE_PARAMETERS: LCTool['parameters'] = Object.freeze({ + type: 'object', + properties: { + file_path: { + type: 'string', + description: + 'Path to a file from code execution output, such as "/mnt/data/result.csv" or another path returned by the execution tool.', + }, + }, + required: ['file_path'], +}) as LCTool['parameters']; + +const CODE_READ_FILE_DEF: LCTool = Object.freeze({ + name: ReadFileToolDefinition.name, + description: CODE_READ_FILE_DESCRIPTION, + parameters: CODE_READ_FILE_PARAMETERS, + responseFormat: ReadFileToolDefinition.responseFormat, +}) as LCTool; + +function buildReadFileDef(includeSkillFileInstructions: boolean): LCTool { + return includeSkillFileInstructions ? READ_FILE_DEF : CODE_READ_FILE_DEF; +} + +function isCodeOnlyReadFileDef(def: LCTool | undefined): boolean { + return ( + def?.name === ReadFileToolDefinition.name && def?.description === CODE_READ_FILE_DESCRIPTION + ); +} + /** * The `bash_tool` description varies along exactly one axis — whether * the LLM-facing `{{toolturn}}` reference syntax guide is @@ -122,9 +170,11 @@ function buildBashToolDef(opts: { enableToolOutputReferences: boolean }): LCTool } /** - * Idempotently registers the skill-flavored code-execution tool pair - * (`bash_tool` + `read_file`) into the run's tool registry and - * tool-definition list. + * Idempotently registers the code-execution tool pair (`bash_tool` + + * `read_file`) into the run's tool registry and tool-definition list. + * `read_file` can be registered with a code-only description first and + * upgraded to the skill-aware description later in the same run if + * skills are actually injected. * * Replaces the legacy `CodeExecutionToolDefinition` / `execute_code` * registration. `execute_code` as a capability name and as an @@ -136,19 +186,46 @@ function buildBashToolDef(opts: { enableToolOutputReferences: boolean }): LCTool export function registerCodeExecutionTools( params: RegisterCodeExecutionToolsParams, ): RegisterCodeExecutionToolsResult { - const { toolRegistry, toolDefinitions, includeBash, enableToolOutputReferences = false } = params; + const { + toolRegistry, + toolDefinitions, + includeBash, + includeSkillFileInstructions = true, + enableToolOutputReferences = false, + } = params; + const readFileDef = buildReadFileDef(includeSkillFileInstructions); const candidates: LCTool[] = includeBash - ? [READ_FILE_DEF, buildBashToolDef({ enableToolOutputReferences })] - : [READ_FILE_DEF]; + ? [readFileDef, buildBashToolDef({ enableToolOutputReferences })] + : [readFileDef]; - const existingNames = new Set((toolDefinitions ?? []).map((d) => d.name)); + const inputDefinitions = toolDefinitions ?? []; + let workingDefinitions = inputDefinitions; const registered: string[] = []; const newDefs: LCTool[] = []; for (const def of candidates) { + const existingIndex = workingDefinitions.findIndex((d) => d.name === def.name); + const existingDef = existingIndex >= 0 ? workingDefinitions[existingIndex] : undefined; + const registryDef = toolRegistry?.get(def.name); + if ( + def.name === ReadFileToolDefinition.name && + includeSkillFileInstructions && + (isCodeOnlyReadFileDef(existingDef) || isCodeOnlyReadFileDef(registryDef)) + ) { + if (isCodeOnlyReadFileDef(existingDef)) { + workingDefinitions = + workingDefinitions === inputDefinitions ? [...inputDefinitions] : workingDefinitions; + workingDefinitions[existingIndex] = def; + } + if (isCodeOnlyReadFileDef(registryDef)) { + toolRegistry?.set(def.name, def); + } + continue; + } + const inRegistry = toolRegistry?.has(def.name) === true; - const inDefs = existingNames.has(def.name); + const inDefs = existingIndex >= 0; if (inRegistry || inDefs) { continue; } @@ -158,15 +235,15 @@ export function registerCodeExecutionTools( } /** - * Skip the array spread on the common second-call no-op path (both tools - * already registered by the first caller in the same run). Returns the - * input array by reference; callers treat the return value as immutable. + * Skip the array spread on the common second-call path when no tools were + * newly registered. Returns the input array by reference unless an existing + * code-only `read_file` definition was upgraded above. */ if (newDefs.length === 0) { - return { toolDefinitions: toolDefinitions ?? [], registered }; + return { toolDefinitions: workingDefinitions, registered }; } return { - toolDefinitions: [...(toolDefinitions ?? []), ...newDefs], + toolDefinitions: [...workingDefinitions, ...newDefs], registered, }; }