mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-27 04:07:05 +00:00
🔌 refactor: Decouple bash_tool from Per-User CODE_API_KEY (#12712)
* 🔌 refactor: Decouple bash_tool from Per-User CODE_API_KEY
Phase 4 of Agent Skills umbrella (#12625): gate bash_tool and skill
file priming on the `execute_code` capability only. Thread a boolean
`codeEnvAvailable` through `enrichWithSkillConfigurable` and
`primeInvokedSkills` in place of the old per-user `codeApiKey` +
`loadAuthValues` plumbing. The sandbox API key is the LibreChat-
hosted service key — system-level, not a user secret — so the
per-user lookup was legacy; when needed, it's read directly from
`process.env[EnvVar.CODE_API_KEY]` inside the capability gate.
`handleSkillToolCall` and `primeInvokedSkills` gate sandbox uploads
on `codeEnvAvailable` first, preventing skill-file uploads to the
sandbox when an agent has `execute_code` disabled even if the env
var happens to be set. The agents library resolves the env key
itself for `bash_tool`, so `ToolService.js` drops the
`loadAuthValues` lookup and the "Code execution is not available"
placeholder tool in favor of a plain `createBashExecutionTool({})`
with a loud error log if the env var is missing.
Also fixes a pre-existing `appConfig`-undefined lint error in
`responses.js`/`createResponse` that surfaced when this file was
touched (declares `const appConfig = req.config` at function top,
matching the existing pattern in other controllers).
Preserves the `skillPrimedIdsByName` threading added by Phase 3/5/6
and all Phase 3/5/6 call-site signatures. Adds
`skillConfigurable.spec.ts` (5 cases pinning the new surface) and
`skillFiles.spec.ts` (4-way matrix of capability × env key for
`primeInvokedSkills`).
* 🧪 refactor: Address Codex Review Feedback
Resolves findings from the second codex review on #12712:
- MAJOR: `handlers.spec.ts` now covers the `codeEnvAvailable` gate in
`handleSkillToolCall` across three cases (gate off, gate on + env
set, gate on + env unset). The gate is the critical regression
prevention — a future edit that drops it would silently re-enable
sandbox uploads for agents with `execute_code` disabled.
- MINOR: Hoist `codeEnvAvailable` and `skillPrimedIdsByName` out of
`loadTools` closures in `openai.js` and `responses.js`. Both values
are fixed once `initializeAgent` resolves, so recomputing them on
every tool execution was wasted work. `responses.js` shares a single
pair between its streaming and non-streaming branches.
- MINOR: `skillFiles.spec.ts` now has a test that exercises the full
upload path end-to-end with real file records, asserting
`batchUploadCodeEnvFiles` is called with the env-sourced apiKey and
the correct file set (including the synthetic `SKILL.md`).
- NIT: Finish the `appConfig` extraction in `responses.js/createResponse`
— replaces the remaining `req.config` references with `appConfig` for
consistency with the pattern in other controllers.
No behavioral changes beyond what was already in place; this is
coverage and readability polish.
* 🧷 test: Tighten Spec Hygiene Per Codex Nit Feedback
Round-3 codex review flagged two NITs on the test code added in the
previous commit:
- Replace `_id: 'skill-id' as unknown as never` in the new
`makeSkillHandlerWithFiles` helper with a real `Types.ObjectId`,
matching the pattern used by the primed-skill tests further up in
the same file (and by `skillFiles.spec.ts`). The `never` cast
hides the fact that `_id` really is a string / ObjectId at runtime.
- Replace the ad-hoc `{ on, pipe, read }` stub with a real
`Readable.from(Buffer.from(''))` in the upload-path test. The stub
worked only because `batchUploadCodeEnvFiles` is mocked and never
iterates the stream; `Readable.from` satisfies the same contract
and is robust to any future partial-real replacement of the upload
function.
Pure test-hygiene improvements; no runtime code touched.
* 🧹 chore: Remove Duplicate appConfig Declaration After Rebase
The upstream `2463b6acd` fix declared `const appConfig = req.config`
inside the try block (line 381) to patch the same `no-undef` error
I fixed in this PR at the top of `createResponse` (line 283). The
rebase kept both declarations side-by-side. Drops the inner one —
the outer binding covers every downstream reference already.
This commit is contained in:
parent
5c69d1f7fa
commit
7581540ab6
11 changed files with 396 additions and 165 deletions
|
|
@ -414,6 +414,16 @@ const OpenAIChatCompletionController = async (req, res) => {
|
|||
|
||||
const toolEndCallback = createToolEndCallback({ req, res, artifactPromises, streamId: null });
|
||||
|
||||
/* Stable for the turn: the capability set is the admin config, and
|
||||
the prime lists are fixed once `initializeAgent` resolves. Hoisting
|
||||
these out of `loadTools` avoids recomputing them on every tool
|
||||
execution (and keeps the call-site lean). */
|
||||
const codeEnvAvailable = enabledCapabilities.has(AgentCapabilities.execute_code);
|
||||
const skillPrimedIdsByName = buildSkillPrimedIdsByName(
|
||||
primaryConfig.manualSkillPrimes,
|
||||
primaryConfig.alwaysApplySkillPrimes,
|
||||
);
|
||||
|
||||
const toolExecuteOptions = {
|
||||
loadTools: async (toolNames, agentId) => {
|
||||
const ctx = agentToolContexts.get(agentId) ?? agentToolContexts.get(primaryConfig.id) ?? {};
|
||||
|
|
@ -432,11 +442,8 @@ const OpenAIChatCompletionController = async (req, res) => {
|
|||
result,
|
||||
req,
|
||||
primaryConfig.accessibleSkillIds,
|
||||
undefined,
|
||||
buildSkillPrimedIdsByName(
|
||||
primaryConfig.manualSkillPrimes,
|
||||
primaryConfig.alwaysApplySkillPrimes,
|
||||
),
|
||||
codeEnvAvailable,
|
||||
skillPrimedIdsByName,
|
||||
);
|
||||
},
|
||||
toolEndCallback,
|
||||
|
|
|
|||
|
|
@ -280,6 +280,7 @@ function convertMessagesToOutputItems(messages) {
|
|||
* @param {import('express').Response} res
|
||||
*/
|
||||
const createResponse = async (req, res) => {
|
||||
const appConfig = req.config;
|
||||
const requestStartTime = Date.now();
|
||||
|
||||
// Validate request
|
||||
|
|
@ -291,7 +292,7 @@ const createResponse = async (req, res) => {
|
|||
const request = validation.request;
|
||||
const agentId = request.model;
|
||||
const isStreaming = request.stream === true;
|
||||
const summarizationConfig = req.config?.summarization;
|
||||
const summarizationConfig = appConfig?.summarization;
|
||||
|
||||
// Look up the agent
|
||||
const agent = await db.getAgent({ id: agentId });
|
||||
|
|
@ -344,7 +345,7 @@ const createResponse = async (req, res) => {
|
|||
|
||||
// Build allowed providers set
|
||||
const allowedProviders = new Set(
|
||||
req.config?.endpoints?.[EModelEndpoint.agents]?.allowedProviders,
|
||||
appConfig?.endpoints?.[EModelEndpoint.agents]?.allowedProviders,
|
||||
);
|
||||
|
||||
// Create tool loader
|
||||
|
|
@ -377,7 +378,6 @@ const createResponse = async (req, res) => {
|
|||
getSkillByName: db.getSkillByName,
|
||||
};
|
||||
|
||||
const appConfig = req.config;
|
||||
const enabledCapabilities = new Set(
|
||||
appConfig?.endpoints?.[EModelEndpoint.agents]?.capabilities,
|
||||
);
|
||||
|
|
@ -557,6 +557,16 @@ const createResponse = async (req, res) => {
|
|||
indexTokenCountMap = primeResult.indexTokenCountMap;
|
||||
}
|
||||
|
||||
/* Stable for the turn: the capability set is the admin config, and
|
||||
the prime lists are fixed once `initializeAgent` resolves. Hoisted
|
||||
here so both the streaming and non-streaming `loadTools` closures
|
||||
below read the same values without recomputing per tool execution. */
|
||||
const codeEnvAvailable = enabledCapabilities.has(AgentCapabilities.execute_code);
|
||||
const skillPrimedIdsByName = buildSkillPrimedIdsByName(
|
||||
manualSkillPrimes,
|
||||
alwaysApplySkillPrimes,
|
||||
);
|
||||
|
||||
// Create tracker for streaming or aggregator for non-streaming
|
||||
const tracker = actuallyStreaming ? createResponseTracker() : null;
|
||||
const aggregator = actuallyStreaming ? null : createResponseAggregator();
|
||||
|
|
@ -614,11 +624,8 @@ const createResponse = async (req, res) => {
|
|||
result,
|
||||
req,
|
||||
primaryConfig.accessibleSkillIds,
|
||||
undefined,
|
||||
buildSkillPrimedIdsByName(
|
||||
primaryConfig.manualSkillPrimes,
|
||||
primaryConfig.alwaysApplySkillPrimes,
|
||||
),
|
||||
codeEnvAvailable,
|
||||
skillPrimedIdsByName,
|
||||
);
|
||||
},
|
||||
toolEndCallback,
|
||||
|
|
@ -665,7 +672,7 @@ const createResponse = async (req, res) => {
|
|||
initialSummary,
|
||||
runId: responseId,
|
||||
summarizationConfig,
|
||||
appConfig: req.config,
|
||||
appConfig,
|
||||
signal: abortController.signal,
|
||||
customHandlers: handlers,
|
||||
requestBody: {
|
||||
|
|
@ -706,8 +713,8 @@ const createResponse = async (req, res) => {
|
|||
});
|
||||
|
||||
// Record token usage against balance
|
||||
const balanceConfig = getBalanceConfig(req.config);
|
||||
const transactionsConfig = getTransactionsConfig(req.config);
|
||||
const balanceConfig = getBalanceConfig(appConfig);
|
||||
const transactionsConfig = getTransactionsConfig(appConfig);
|
||||
recordCollectedUsage(
|
||||
{
|
||||
spendTokens: db.spendTokens,
|
||||
|
|
@ -793,11 +800,8 @@ const createResponse = async (req, res) => {
|
|||
result,
|
||||
req,
|
||||
primaryConfig.accessibleSkillIds,
|
||||
undefined,
|
||||
buildSkillPrimedIdsByName(
|
||||
primaryConfig.manualSkillPrimes,
|
||||
primaryConfig.alwaysApplySkillPrimes,
|
||||
),
|
||||
codeEnvAvailable,
|
||||
skillPrimedIdsByName,
|
||||
);
|
||||
},
|
||||
toolEndCallback,
|
||||
|
|
@ -842,7 +846,7 @@ const createResponse = async (req, res) => {
|
|||
initialSummary,
|
||||
runId: responseId,
|
||||
summarizationConfig,
|
||||
appConfig: req.config,
|
||||
appConfig,
|
||||
signal: abortController.signal,
|
||||
customHandlers: handlers,
|
||||
requestBody: {
|
||||
|
|
@ -882,8 +886,8 @@ const createResponse = async (req, res) => {
|
|||
});
|
||||
|
||||
// Record token usage against balance
|
||||
const balanceConfig = getBalanceConfig(req.config);
|
||||
const transactionsConfig = getTransactionsConfig(req.config);
|
||||
const balanceConfig = getBalanceConfig(appConfig);
|
||||
const transactionsConfig = getTransactionsConfig(appConfig);
|
||||
recordCollectedUsage(
|
||||
{
|
||||
spendTokens: db.spendTokens,
|
||||
|
|
|
|||
|
|
@ -1,5 +1,5 @@
|
|||
const { logger } = require('@librechat/data-schemas');
|
||||
const { EnvVar, createContentAggregator } = require('@librechat/agents');
|
||||
const { createContentAggregator } = require('@librechat/agents');
|
||||
const {
|
||||
scopeSkillIds,
|
||||
loadSkillStates,
|
||||
|
|
@ -25,7 +25,6 @@ const {
|
|||
getDefaultHandlers,
|
||||
} = require('~/server/controllers/agents/callbacks');
|
||||
const { loadAgentTools, loadToolsForExecution } = require('~/server/services/ToolService');
|
||||
const { loadAuthValues } = require('~/server/services/Tools/credentials');
|
||||
const { filterFilesByAgentAccess } = require('~/server/services/Files/permissions');
|
||||
const {
|
||||
getSkillToolDeps,
|
||||
|
|
@ -121,6 +120,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => {
|
|||
* - the agent has stored skills (scoped by scopeSkillIds later). */
|
||||
const enabledCapabilities = new Set(appConfig?.endpoints?.[EModelEndpoint.agents]?.capabilities);
|
||||
const skillsCapabilityEnabled = enabledCapabilities.has(AgentCapabilities.skills);
|
||||
const codeEnvAvailable = enabledCapabilities.has(AgentCapabilities.execute_code);
|
||||
const ephemeralSkillsToggle = req.body?.ephemeralAgent?.skills === true;
|
||||
|
||||
const accessibleSkillIds = skillsCapabilityEnabled
|
||||
|
|
@ -139,21 +139,6 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => {
|
|||
accessibleSkillIds,
|
||||
});
|
||||
|
||||
// Resolve code API key once for the entire run (shared by primeInvokedSkills
|
||||
// and enrichWithSkillConfigurable) to avoid redundant auth lookups.
|
||||
let codeApiKey;
|
||||
if (skillsCapabilityEnabled && enabledCapabilities.has(AgentCapabilities.execute_code)) {
|
||||
try {
|
||||
const authValues = await loadAuthValues({
|
||||
userId: req.user.id,
|
||||
authFields: [EnvVar.CODE_API_KEY],
|
||||
});
|
||||
codeApiKey = authValues[EnvVar.CODE_API_KEY];
|
||||
} catch {
|
||||
// non-fatal — primeInvokedSkills and enrichWithSkillConfigurable will work without it
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Agent context store - populated after initialization, accessed by callback via closure.
|
||||
* Maps agentId -> { userMCPAuthMap, agent, tool_resources, toolRegistry, openAIApiKey }
|
||||
|
|
@ -191,7 +176,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => {
|
|||
result,
|
||||
req,
|
||||
ctx.accessibleSkillIds,
|
||||
codeApiKey,
|
||||
codeEnvAvailable,
|
||||
ctx.skillPrimedIdsByName,
|
||||
);
|
||||
},
|
||||
|
|
@ -272,7 +257,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => {
|
|||
accessibleSkillIds,
|
||||
ephemeralSkillsToggle ? undefined : primaryAgent.skills,
|
||||
),
|
||||
codeEnvAvailable: enabledCapabilities.has(AgentCapabilities.execute_code),
|
||||
codeEnvAvailable,
|
||||
skillStates,
|
||||
defaultActiveOnShare,
|
||||
manualSkills,
|
||||
|
|
@ -482,8 +467,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => {
|
|||
req,
|
||||
payload,
|
||||
accessibleSkillIds,
|
||||
codeApiKey,
|
||||
loadAuthValues,
|
||||
codeEnvAvailable,
|
||||
...getSkillToolDeps(),
|
||||
})
|
||||
: undefined;
|
||||
|
|
|
|||
|
|
@ -1,7 +1,6 @@
|
|||
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 { loadAuthValues } = require('~/server/services/Tools/credentials');
|
||||
const { enrichWithSkillConfigurable } = require('@librechat/api');
|
||||
const db = require('~/models');
|
||||
|
||||
|
|
@ -73,34 +72,8 @@ function getSkillToolDeps() {
|
|||
return skillToolDeps;
|
||||
}
|
||||
|
||||
/**
|
||||
* Wraps the TS enrichWithSkillConfigurable with the CJS loadAuthValues dependency.
|
||||
* @param {object} result - The result from loadToolsForExecution
|
||||
* @param {object} req - The Express request object
|
||||
* @param {Array} accessibleSkillIds - Pre-computed accessible skill IDs
|
||||
* @param {string} [preResolvedCodeApiKey] - Pre-resolved code API key (skips redundant lookup)
|
||||
* @param {Record<string, string>} [skillPrimedIdsByName] - Map of name → skill id for skills primed this turn (manual `$`-popover invocation OR always-apply). Pins same-name collision lookups in `read_file` and relaxes the disable-model-invocation gate for the primed doc.
|
||||
* @returns {Promise<object>} Augmented result with skill configurable
|
||||
*/
|
||||
function enrichConfigurable(
|
||||
result,
|
||||
req,
|
||||
accessibleSkillIds,
|
||||
preResolvedCodeApiKey,
|
||||
skillPrimedIdsByName,
|
||||
) {
|
||||
return enrichWithSkillConfigurable(
|
||||
result,
|
||||
req,
|
||||
accessibleSkillIds,
|
||||
loadAuthValues,
|
||||
preResolvedCodeApiKey,
|
||||
skillPrimedIdsByName,
|
||||
);
|
||||
}
|
||||
|
||||
module.exports = {
|
||||
getSkillToolDeps,
|
||||
enrichWithSkillConfigurable: enrichConfigurable,
|
||||
enrichWithSkillConfigurable,
|
||||
buildSkillPrimedIdsByName,
|
||||
};
|
||||
|
|
|
|||
|
|
@ -1273,34 +1273,13 @@ async function loadToolsForExecution({
|
|||
const isBashTool = toolNames.includes(AgentConstants.BASH_TOOL);
|
||||
if (isBashTool) {
|
||||
try {
|
||||
const authValues = await loadAuthValues({
|
||||
userId: req.user.id,
|
||||
authFields: [EnvVar.CODE_API_KEY],
|
||||
});
|
||||
const codeApiKey = authValues[EnvVar.CODE_API_KEY];
|
||||
|
||||
if (codeApiKey) {
|
||||
const bashTool = createBashExecutionTool({ apiKey: codeApiKey });
|
||||
allLoadedTools.push(bashTool);
|
||||
} else {
|
||||
logger.debug('[loadToolsForExecution] bash_tool requested but CODE_API_KEY not available');
|
||||
allLoadedTools.push(
|
||||
toolFn(
|
||||
async () => [
|
||||
'Code execution is not available. Use the read_file tool instead.',
|
||||
undefined,
|
||||
],
|
||||
{
|
||||
name: AgentConstants.BASH_TOOL,
|
||||
description: 'Bash execution (unavailable - no code API key configured)',
|
||||
schema: { type: 'object', properties: { command: { type: 'string' } } },
|
||||
responseFormat: AgentConstants.CONTENT_AND_ARTIFACT,
|
||||
},
|
||||
),
|
||||
);
|
||||
}
|
||||
const bashTool = createBashExecutionTool({});
|
||||
allLoadedTools.push(bashTool);
|
||||
} catch (error) {
|
||||
logger.error('[loadToolsForExecution] Error creating bash tool:', error);
|
||||
logger.error(
|
||||
'[loadToolsForExecution] Failed to create bash_tool — is LIBRECHAT_CODE_API_KEY set in the server environment?',
|
||||
error,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -571,4 +571,99 @@ describe('createToolExecuteHandler', () => {
|
|||
expect(lookupOptions).toEqual({});
|
||||
});
|
||||
});
|
||||
|
||||
describe('skill tool codeEnvAvailable gate (sandbox file priming)', () => {
|
||||
const { Types } = jest.requireActual('mongoose') as typeof import('mongoose');
|
||||
const SKILL_ID = new Types.ObjectId();
|
||||
|
||||
function makeSkillHandlerWithFiles(params: {
|
||||
codeEnvAvailable: boolean;
|
||||
listSkillFiles: jest.Mock;
|
||||
batchUploadCodeEnvFiles?: jest.Mock;
|
||||
}) {
|
||||
const getSkillByName = jest.fn(async () => ({
|
||||
_id: SKILL_ID as unknown as never,
|
||||
name: 'brand-guidelines',
|
||||
body: 'skill body',
|
||||
fileCount: 2,
|
||||
}));
|
||||
/* `loadTools` injects `codeEnvAvailable` into the returned
|
||||
`configurable`, which mirrors production flow through
|
||||
`enrichWithSkillConfigurable`. `req` must be present for the
|
||||
priming branch to enter (the handler guards on it). */
|
||||
const req = { user: { id: 'user-1' } };
|
||||
const loadTools: ToolExecuteOptions['loadTools'] = jest.fn(async () => ({
|
||||
loadedTools: [],
|
||||
configurable: { codeEnvAvailable: params.codeEnvAvailable, req },
|
||||
}));
|
||||
return createToolExecuteHandler({
|
||||
loadTools,
|
||||
getSkillByName,
|
||||
listSkillFiles: params.listSkillFiles as unknown as ToolExecuteOptions['listSkillFiles'],
|
||||
batchUploadCodeEnvFiles: (params.batchUploadCodeEnvFiles ??
|
||||
jest.fn()) as unknown as ToolExecuteOptions['batchUploadCodeEnvFiles'],
|
||||
getStrategyFunctions: jest.fn() as unknown as ToolExecuteOptions['getStrategyFunctions'],
|
||||
});
|
||||
}
|
||||
|
||||
const ORIGINAL_KEY = process.env.LIBRECHAT_CODE_API_KEY;
|
||||
afterEach(() => {
|
||||
if (ORIGINAL_KEY === undefined) {
|
||||
delete process.env.LIBRECHAT_CODE_API_KEY;
|
||||
} else {
|
||||
process.env.LIBRECHAT_CODE_API_KEY = ORIGINAL_KEY;
|
||||
}
|
||||
});
|
||||
|
||||
it('does NOT call listSkillFiles when codeEnvAvailable is false (even when env key is set)', async () => {
|
||||
process.env.LIBRECHAT_CODE_API_KEY = 'present';
|
||||
const listSkillFiles = jest.fn().mockResolvedValue([]);
|
||||
const handler = makeSkillHandlerWithFiles({
|
||||
codeEnvAvailable: false,
|
||||
listSkillFiles,
|
||||
});
|
||||
|
||||
const [result] = await invokeHandler(handler, [
|
||||
{
|
||||
id: 'call_gate_off',
|
||||
name: Constants.SKILL_TOOL,
|
||||
args: { skillName: 'brand-guidelines' },
|
||||
},
|
||||
]);
|
||||
|
||||
expect(result.status).toBe('success');
|
||||
expect(listSkillFiles).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('calls listSkillFiles when codeEnvAvailable is true AND the env key is set', async () => {
|
||||
process.env.LIBRECHAT_CODE_API_KEY = 'present';
|
||||
const listSkillFiles = jest.fn().mockResolvedValue([]);
|
||||
const handler = makeSkillHandlerWithFiles({
|
||||
codeEnvAvailable: true,
|
||||
listSkillFiles,
|
||||
});
|
||||
|
||||
await invokeHandler(handler, [
|
||||
{ id: 'call_gate_on', name: Constants.SKILL_TOOL, args: { skillName: 'brand-guidelines' } },
|
||||
]);
|
||||
|
||||
expect(listSkillFiles).toHaveBeenCalledWith(SKILL_ID);
|
||||
});
|
||||
|
||||
it('does NOT call listSkillFiles when codeEnvAvailable is true but env key is unset (admin misconfig)', async () => {
|
||||
delete process.env.LIBRECHAT_CODE_API_KEY;
|
||||
const listSkillFiles = jest.fn().mockResolvedValue([]);
|
||||
const handler = makeSkillHandlerWithFiles({
|
||||
codeEnvAvailable: true,
|
||||
listSkillFiles,
|
||||
});
|
||||
|
||||
const [result] = await invokeHandler(handler, [
|
||||
{ id: 'call_no_env', name: Constants.SKILL_TOOL, args: { skillName: 'brand-guidelines' } },
|
||||
]);
|
||||
|
||||
expect(result.status).toBe('success');
|
||||
expect(listSkillFiles).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1,5 +1,5 @@
|
|||
import { logger } from '@librechat/data-schemas';
|
||||
import { GraphEvents, Constants, CODE_EXECUTION_TOOLS } from '@librechat/agents';
|
||||
import { EnvVar, GraphEvents, Constants, CODE_EXECUTION_TOOLS } from '@librechat/agents';
|
||||
import type {
|
||||
LCTool,
|
||||
EventHandler,
|
||||
|
|
@ -530,15 +530,19 @@ async function handleSkillToolCall(
|
|||
| { session_id: string; files: Array<{ id: string; session_id: string; name: string }> }
|
||||
| undefined;
|
||||
|
||||
// Prime skill files to code env when the skill has bundled files
|
||||
// Prime skill files to code env — only when the `execute_code` capability
|
||||
// is enabled for this run. The flag is threaded via configurable upstream
|
||||
// so this gate cannot be bypassed by a stray env var.
|
||||
const codeEnvAvailable = mergedConfigurable?.codeEnvAvailable === true;
|
||||
if (
|
||||
codeEnvAvailable &&
|
||||
skill.fileCount > 0 &&
|
||||
req &&
|
||||
listSkillFiles &&
|
||||
getStrategyFunctions &&
|
||||
batchUploadCodeEnvFiles
|
||||
) {
|
||||
const codeApiKey = (mergedConfigurable?.codeApiKey as string) ?? '';
|
||||
const codeApiKey = process.env[EnvVar.CODE_API_KEY] ?? '';
|
||||
if (codeApiKey) {
|
||||
try {
|
||||
const skillFiles = await listSkillFiles(skill._id);
|
||||
|
|
|
|||
70
packages/api/src/agents/skillConfigurable.spec.ts
Normal file
70
packages/api/src/agents/skillConfigurable.spec.ts
Normal file
|
|
@ -0,0 +1,70 @@
|
|||
import { enrichWithSkillConfigurable } from './skillConfigurable';
|
||||
|
||||
describe('enrichWithSkillConfigurable', () => {
|
||||
const req = { user: { id: 'user-1' } };
|
||||
const accessibleSkillIds = ['skill-a', 'skill-b'];
|
||||
|
||||
it('augments configurable with req, accessibleSkillIds, and codeEnvAvailable', () => {
|
||||
const result = enrichWithSkillConfigurable(
|
||||
{ loadedTools: [], configurable: { other: 'value' } },
|
||||
req,
|
||||
accessibleSkillIds,
|
||||
true,
|
||||
);
|
||||
|
||||
expect(result.configurable).toEqual({
|
||||
other: 'value',
|
||||
req,
|
||||
codeEnvAvailable: true,
|
||||
accessibleSkillIds,
|
||||
skillPrimedIdsByName: undefined,
|
||||
});
|
||||
});
|
||||
|
||||
it('propagates codeEnvAvailable=false verbatim (not coerced)', () => {
|
||||
const result = enrichWithSkillConfigurable(
|
||||
{ loadedTools: [], configurable: {} },
|
||||
req,
|
||||
accessibleSkillIds,
|
||||
false,
|
||||
);
|
||||
|
||||
expect(result.configurable.codeEnvAvailable).toBe(false);
|
||||
});
|
||||
|
||||
it('does not inject a codeApiKey key (per-user lookup removed)', () => {
|
||||
const result = enrichWithSkillConfigurable(
|
||||
{ loadedTools: [], configurable: {} },
|
||||
req,
|
||||
accessibleSkillIds,
|
||||
true,
|
||||
);
|
||||
|
||||
expect(result.configurable).not.toHaveProperty('codeApiKey');
|
||||
});
|
||||
|
||||
it('threads skillPrimedIdsByName through unchanged', () => {
|
||||
const primed = { 'brand-guidelines': 'abc123' };
|
||||
const result = enrichWithSkillConfigurable(
|
||||
{ loadedTools: [], configurable: {} },
|
||||
req,
|
||||
accessibleSkillIds,
|
||||
true,
|
||||
primed,
|
||||
);
|
||||
|
||||
expect(result.configurable.skillPrimedIdsByName).toBe(primed);
|
||||
});
|
||||
|
||||
it('preserves loadedTools unchanged', () => {
|
||||
const tools = [{ name: 'x' }];
|
||||
const result = enrichWithSkillConfigurable(
|
||||
{ loadedTools: tools, configurable: undefined },
|
||||
req,
|
||||
accessibleSkillIds,
|
||||
false,
|
||||
);
|
||||
|
||||
expect(result.loadedTools).toBe(tools);
|
||||
});
|
||||
});
|
||||
|
|
@ -1,65 +1,49 @@
|
|||
import { EnvVar } from '@librechat/agents';
|
||||
import { logger } from '@librechat/data-schemas';
|
||||
|
||||
/**
|
||||
* Augments a loadTools result with skill-specific configurable properties.
|
||||
* Loads the code API key and merges it with accessibleSkillIds and the request object.
|
||||
*
|
||||
* `codeEnvAvailable` is threaded as a boolean (true when the agent's
|
||||
* `execute_code` capability is enabled). Downstream skill consumers —
|
||||
* the skill-tool handler (for file priming) and `primeInvokedSkills`
|
||||
* (for history re-priming) — gate sandbox uploads on this flag rather
|
||||
* than on API-key presence, so no sandbox traffic occurs for agents
|
||||
* that lack code-execution capability even if
|
||||
* `process.env.LIBRECHAT_CODE_API_KEY` happens to be set.
|
||||
*
|
||||
* `skillPrimedIdsByName` maps each primed skill name (manual `$` or
|
||||
* always-apply) to the `_id` of the exact doc whose body was primed into
|
||||
* the turn. Skill-tool handlers consult it to:
|
||||
* 1. Relax the `disable-model-invocation` gate on `read_file` for any
|
||||
* primed skill (so a `disable-model-invocation: true` skill whose
|
||||
* body is in context can still load its `references/*` / `scripts/*`
|
||||
* files). Without this, a user manually invoking — or auto-priming
|
||||
* — a disabled skill would get a body that references files the
|
||||
* model is forbidden to open.
|
||||
* 2. Constrain the lookup to the primed `_id` on same-name collisions,
|
||||
* so `read_file` reads from the same doc whose body got primed
|
||||
* (otherwise a newer same-name duplicate could shadow the
|
||||
* resolver's pick and cause body/file mismatch within a single
|
||||
* turn).
|
||||
* always-apply) to the `_id` of the exact doc whose body was primed
|
||||
* into the turn. Skill-tool handlers consult it to:
|
||||
* 1. Relax the `disable-model-invocation` gate on `read_file` for
|
||||
* any primed skill (so a `disable-model-invocation: true` skill
|
||||
* whose body is in context can still load its `references/*` /
|
||||
* `scripts/*` files). Without this, a user manually invoking —
|
||||
* or auto-priming — a disabled skill would get a body that
|
||||
* references files the model is forbidden to open.
|
||||
* 2. Constrain the lookup to the primed `_id` on same-name
|
||||
* collisions, so `read_file` reads from the same doc whose body
|
||||
* got primed (otherwise a newer same-name duplicate could shadow
|
||||
* the resolver's pick and cause body/file mismatch within a
|
||||
* single turn).
|
||||
* Empty/missing → no exception, the gate applies as normal and the
|
||||
* lookup uses the full ACL set.
|
||||
*/
|
||||
export async function enrichWithSkillConfigurable(
|
||||
export function enrichWithSkillConfigurable(
|
||||
result: { loadedTools: unknown[]; configurable?: Record<string, unknown> },
|
||||
req: { user?: { id?: string } },
|
||||
accessibleSkillIds: unknown[],
|
||||
loadAuthValues: (params: {
|
||||
userId: string;
|
||||
authFields: string[];
|
||||
}) => Promise<Record<string, string>>,
|
||||
/** Pre-resolved code API key. When provided, loadAuthValues is skipped. */
|
||||
preResolvedCodeApiKey?: string,
|
||||
codeEnvAvailable: boolean,
|
||||
/**
|
||||
* `{ [skillName]: skillIdString }` for every skill primed this turn
|
||||
* (manual or always-apply). The id pins same-name collision lookups to
|
||||
* the exact doc the resolver primed and relaxes the disable-model gate.
|
||||
*/
|
||||
skillPrimedIdsByName?: Record<string, string>,
|
||||
): Promise<{ loadedTools: unknown[]; configurable: Record<string, unknown> }> {
|
||||
let codeApiKey: string | undefined = preResolvedCodeApiKey;
|
||||
if (!codeApiKey) {
|
||||
try {
|
||||
const authValues = await loadAuthValues({
|
||||
userId: req.user?.id ?? '',
|
||||
authFields: [EnvVar.CODE_API_KEY],
|
||||
});
|
||||
codeApiKey = authValues[EnvVar.CODE_API_KEY];
|
||||
} catch (err) {
|
||||
logger.debug(
|
||||
'[enrichWithSkillConfigurable] loadAuthValues failed:',
|
||||
err instanceof Error ? err.message : err,
|
||||
);
|
||||
}
|
||||
}
|
||||
): { loadedTools: unknown[]; configurable: Record<string, unknown> } {
|
||||
return {
|
||||
...result,
|
||||
configurable: {
|
||||
...result.configurable,
|
||||
req,
|
||||
codeApiKey,
|
||||
codeEnvAvailable,
|
||||
accessibleSkillIds,
|
||||
skillPrimedIdsByName,
|
||||
},
|
||||
|
|
|
|||
147
packages/api/src/agents/skillFiles.spec.ts
Normal file
147
packages/api/src/agents/skillFiles.spec.ts
Normal file
|
|
@ -0,0 +1,147 @@
|
|||
jest.mock('@librechat/data-schemas', () => ({
|
||||
logger: {
|
||||
debug: jest.fn(),
|
||||
info: jest.fn(),
|
||||
warn: jest.fn(),
|
||||
error: jest.fn(),
|
||||
},
|
||||
}));
|
||||
|
||||
const mockExtract = jest.fn();
|
||||
jest.mock('./run', () => ({
|
||||
extractInvokedSkillsFromPayload: (...args: unknown[]) => mockExtract(...args),
|
||||
}));
|
||||
|
||||
import { Readable } from 'stream';
|
||||
import { Types } from 'mongoose';
|
||||
import { primeInvokedSkills } from './skillFiles';
|
||||
import type { PrimeInvokedSkillsDeps } from './skillFiles';
|
||||
|
||||
const SKILL_ID = new Types.ObjectId();
|
||||
|
||||
function makeDeps(overrides: Partial<PrimeInvokedSkillsDeps> = {}): PrimeInvokedSkillsDeps {
|
||||
const listSkillFiles = jest.fn().mockResolvedValue([]);
|
||||
const batchUploadCodeEnvFiles = jest.fn().mockResolvedValue({
|
||||
session_id: 'session-x',
|
||||
files: [],
|
||||
});
|
||||
return {
|
||||
req: { user: { id: 'user-1' } } as PrimeInvokedSkillsDeps['req'],
|
||||
payload: [{ role: 'assistant', content: [] }],
|
||||
accessibleSkillIds: [SKILL_ID],
|
||||
codeEnvAvailable: true,
|
||||
getSkillByName: jest.fn().mockResolvedValue({
|
||||
_id: SKILL_ID,
|
||||
name: 'brand-guidelines',
|
||||
body: 'skill body',
|
||||
fileCount: 2,
|
||||
}),
|
||||
listSkillFiles,
|
||||
getStrategyFunctions: jest.fn(),
|
||||
batchUploadCodeEnvFiles,
|
||||
...overrides,
|
||||
};
|
||||
}
|
||||
|
||||
describe('primeInvokedSkills — execute_code capability gate', () => {
|
||||
const ORIGINAL_KEY = process.env.LIBRECHAT_CODE_API_KEY;
|
||||
|
||||
beforeEach(() => {
|
||||
jest.clearAllMocks();
|
||||
mockExtract.mockReturnValue(new Set(['brand-guidelines']));
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
if (ORIGINAL_KEY === undefined) {
|
||||
delete process.env.LIBRECHAT_CODE_API_KEY;
|
||||
} else {
|
||||
process.env.LIBRECHAT_CODE_API_KEY = ORIGINAL_KEY;
|
||||
}
|
||||
});
|
||||
|
||||
it('skips the batch-upload path when codeEnvAvailable is false (even if env key is set)', async () => {
|
||||
process.env.LIBRECHAT_CODE_API_KEY = 'present';
|
||||
const deps = makeDeps({ codeEnvAvailable: false });
|
||||
|
||||
const result = await primeInvokedSkills(deps);
|
||||
|
||||
expect(result.skills?.get('brand-guidelines')).toBe('skill body');
|
||||
expect(deps.listSkillFiles).not.toHaveBeenCalled();
|
||||
expect(deps.batchUploadCodeEnvFiles).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('skips the batch-upload path when codeEnvAvailable is true but env key is unset', async () => {
|
||||
delete process.env.LIBRECHAT_CODE_API_KEY;
|
||||
const deps = makeDeps({ codeEnvAvailable: true });
|
||||
|
||||
const result = await primeInvokedSkills(deps);
|
||||
|
||||
expect(result.skills?.get('brand-guidelines')).toBe('skill body');
|
||||
expect(deps.listSkillFiles).not.toHaveBeenCalled();
|
||||
expect(deps.batchUploadCodeEnvFiles).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('enters the batch-upload path when codeEnvAvailable is true and env key is set', async () => {
|
||||
process.env.LIBRECHAT_CODE_API_KEY = 'present';
|
||||
const deps = makeDeps({ codeEnvAvailable: true });
|
||||
|
||||
await primeInvokedSkills(deps);
|
||||
|
||||
expect(deps.listSkillFiles).toHaveBeenCalledWith(SKILL_ID);
|
||||
});
|
||||
|
||||
it('actually calls batchUploadCodeEnvFiles with the env-sourced apiKey when files are returned', async () => {
|
||||
process.env.LIBRECHAT_CODE_API_KEY = 'sk-from-env';
|
||||
const fileRecords = [
|
||||
{
|
||||
relativePath: 'references/style.md',
|
||||
filename: 'style.md',
|
||||
filepath: '/storage/brand-guidelines/references/style.md',
|
||||
source: 's3',
|
||||
bytes: 256,
|
||||
},
|
||||
];
|
||||
const listSkillFiles = jest.fn().mockResolvedValue(fileRecords);
|
||||
const getStrategyFunctions = jest.fn().mockReturnValue({
|
||||
/* A real empty Readable — matches the production contract for
|
||||
`getDownloadStream` and works even if `batchUploadCodeEnvFiles`
|
||||
is later replaced with a partially-real implementation that
|
||||
iterates the stream. */
|
||||
getDownloadStream: jest.fn().mockResolvedValue(Readable.from(Buffer.from(''))),
|
||||
});
|
||||
const batchUploadCodeEnvFiles = jest.fn().mockResolvedValue({
|
||||
session_id: 'session-42',
|
||||
files: [{ fileId: 'file-1', filename: 'brand-guidelines/references/style.md' }],
|
||||
});
|
||||
|
||||
const deps = makeDeps({
|
||||
codeEnvAvailable: true,
|
||||
listSkillFiles,
|
||||
getStrategyFunctions,
|
||||
batchUploadCodeEnvFiles,
|
||||
});
|
||||
|
||||
await primeInvokedSkills(deps);
|
||||
|
||||
expect(batchUploadCodeEnvFiles).toHaveBeenCalledTimes(1);
|
||||
const [uploadArgs] = batchUploadCodeEnvFiles.mock.calls[0];
|
||||
expect(uploadArgs.apiKey).toBe('sk-from-env');
|
||||
expect(uploadArgs.entity_id).toBe(SKILL_ID.toString());
|
||||
/* One uploaded file per `fileRecords` entry plus the synthetic
|
||||
SKILL.md that `primeSkillFiles` always prepends. */
|
||||
expect(uploadArgs.files).toHaveLength(fileRecords.length + 1);
|
||||
expect(uploadArgs.files.map((f: { filename: string }) => f.filename)).toEqual(
|
||||
expect.arrayContaining(['brand-guidelines/SKILL.md', 'brand-guidelines/references/style.md']),
|
||||
);
|
||||
});
|
||||
|
||||
it('returns {} early when no skills were invoked, regardless of capability', async () => {
|
||||
mockExtract.mockReturnValue(new Set());
|
||||
const deps = makeDeps({ codeEnvAvailable: true });
|
||||
|
||||
const result = await primeInvokedSkills(deps);
|
||||
|
||||
expect(result).toEqual({});
|
||||
expect(deps.getSkillByName).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
|
@ -225,12 +225,10 @@ export interface PrimeInvokedSkillsDeps {
|
|||
/** Raw message payload (before formatAgentMessages). Used to extract invoked skill names. */
|
||||
payload: Array<Partial<{ role: string; content: unknown }>>;
|
||||
accessibleSkillIds: Types.ObjectId[];
|
||||
/** Pre-resolved code API key. When provided, loadAuthValues is not called (avoids redundant lookups). */
|
||||
codeApiKey?: string;
|
||||
loadAuthValues: (params: {
|
||||
userId: string;
|
||||
authFields: string[];
|
||||
}) => Promise<Record<string, string>>;
|
||||
/** `execute_code` capability flag for the run. When false, the batch-upload
|
||||
* path is skipped entirely — skill bodies still reconstruct for history
|
||||
* rebuilds, but no sandbox traffic is generated. */
|
||||
codeEnvAvailable: boolean;
|
||||
getSkillByName: (
|
||||
name: string,
|
||||
accessibleIds: Types.ObjectId[],
|
||||
|
|
@ -270,21 +268,7 @@ export async function primeInvokedSkills(
|
|||
return {};
|
||||
}
|
||||
|
||||
let apiKey = deps.codeApiKey ?? '';
|
||||
if (!apiKey) {
|
||||
try {
|
||||
const authValues = await deps.loadAuthValues({
|
||||
userId: deps.req.user?.id ?? '',
|
||||
authFields: [EnvVar.CODE_API_KEY],
|
||||
});
|
||||
apiKey = authValues[EnvVar.CODE_API_KEY] ?? '';
|
||||
} catch (err) {
|
||||
logger.debug(
|
||||
'[primeInvokedSkills] loadAuthValues failed:',
|
||||
err instanceof Error ? err.message : err,
|
||||
);
|
||||
}
|
||||
}
|
||||
const apiKey = deps.codeEnvAvailable ? (process.env[EnvVar.CODE_API_KEY] ?? '') : '';
|
||||
|
||||
const skills = new Map<string, string>();
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue