mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-27 04:07:05 +00:00
🗄️ fix: Gate Request-Scoped MCP Servers Out of Persistent Tool Cache (#13672)
* 🗄️ fix: Gate Request-Scoped MCP Servers Out of Persistent Tool Cache PR #13626 established that request-scoped MCP servers (runtime OPENID/GRAPH/BODY placeholders) must not use the persistent 12h tool cache, but only gated three of five touchpoints. The panel endpoint still back-filled the cache and the OAuth callback still wrote to it, while agent loading read those entries ungated — pinning ephemeral model-spec/agent toolsets to stale definitions for up to 12h. Centralize the invariant in createMCPToolCacheService: a getServerConfig resolver dep gates both writers and a new service-owned getMCPServerTools read, so every current and future caller is covered. Callers that already hold the parsed config pass it to skip resolution; the per-call skipCache flag and duplicated call-site gates are removed in favor of the single config-based mechanism. Resolution failures fail open to preserve prior behavior. * 🩹 fix: Address Codex Review on Cache Gating - Repair getCachedTools.spec.js, which destructured the relocated getMCPServerTools directly from the module; its coverage now lives in the service-level tools.spec.ts. - Resolve the merged (Config-tier-aware) server config in the OAuth callback before writing tool definitions, so the cache gate detects request-scoped servers supplied via admin Config overlays that the base registry lookup cannot see. - Discover tools actively for request-scoped servers in the panel endpoint via ephemeral reinitialization: such servers have no stored app/user connections, so the previous getServerToolFunctions fallback returned an empty toolset once the cache read was gated. * 🧵 fix: Address Second Codex Review on Cache Gating - Resolve the merged server config before the OAuth callback reconnects, so the connection itself uses Config-tier overlays rather than only the subsequent cache write. - Pass Config-tier candidates into the panel's request-scoped discovery, matching the reinitialize route: reinitMCPServer forwards configServers (not the provided serverConfig) to its OAuth discovery fallback. - Document the accepted read-path trade-off: the gate resolver sees base configs only, all writers pass merged configs, so a pre-gating or overlay-divergent entry survives at most one cache TTL. * 🚏 chore: Rework Cache Gating for BODY-Only Request Scoping After #13673 narrowed requiresEphemeralUserConnection to BODY placeholders, the central gate follows the predicate unchanged, but the panel's active discovery no longer serves a purpose: the only remaining request-scoped class cannot connect outside a chat turn, so the reinitialization attempt would always fail at the missing-body check. Remove that path; OpenID/Graph servers are persistent user-scoped again and flow through the stored-connection and cache lookups as before. Flip test fixtures that used OPENID placeholders to denote request-scoped configs over to BODY placeholders. * 🪟 fix: Check Config Overlays in Agent-Loading Cache Reads The cache service's registry resolver sees only base YAML/DB configs, so a BODY placeholder introduced by a request-tier Config overlay was invisible to the gate on the agent-loading read path: model-spec and ephemeral-agent expansion could read a leftover persistent entry and pin stale concrete tool names instead of the mcp_all fresh-discovery path. Check the raw overlay candidate inline in loadEphemeralAgent and loadAddedAgent — a pure placeholder scan with no extra IO — and skip the cache read when the overlay makes the server request-scoped. Widen UserScopedConnectionConfig so raw (pre-inspection) configs qualify for the scoping predicates, which only check key presence. * 🧪 test: Guard Run-Scoped MCP Definition Handoff Boundaries The original ClickHouse breaker storm regressed precisely at field pass-through boundaries that unit tests of each end could not see: initializeAgent dropping mcpAvailableTools from its destructure, and the agent tool context losing it on the way into ON_TOOL_EXECUTE. Add direct guards on both hops: the loadTools result must surface on the initialized agent, and the captured toolExecuteOptions closure must forward it to loadToolsForExecution.
This commit is contained in:
parent
5ceabad5f3
commit
49859c04a2
20 changed files with 515 additions and 134 deletions
|
|
@ -1,12 +1,6 @@
|
|||
const { CacheKeys } = require('librechat-data-provider');
|
||||
|
||||
jest.mock('@librechat/data-schemas', () => ({
|
||||
logger: {
|
||||
error: jest.fn(),
|
||||
},
|
||||
}));
|
||||
jest.mock('~/cache/getLogStores');
|
||||
const { logger } = require('@librechat/data-schemas');
|
||||
const getLogStores = require('~/cache/getLogStores');
|
||||
|
||||
const mockCache = { get: jest.fn(), set: jest.fn(), delete: jest.fn() };
|
||||
|
|
@ -16,7 +10,6 @@ const {
|
|||
ToolCacheKeys,
|
||||
getCachedTools,
|
||||
setCachedTools,
|
||||
getMCPServerTools,
|
||||
invalidateCachedTools,
|
||||
} = require('../getCachedTools');
|
||||
|
||||
|
|
@ -74,41 +67,10 @@ describe('getCachedTools', () => {
|
|||
expect(mockCache.delete).toHaveBeenCalledWith(ToolCacheKeys.GLOBAL);
|
||||
});
|
||||
|
||||
it('getMCPServerTools should use TOOL_CACHE namespace', async () => {
|
||||
mockCache.get.mockResolvedValue(null);
|
||||
await getMCPServerTools('user1', 'github');
|
||||
expect(getLogStores).toHaveBeenCalledWith(CacheKeys.TOOL_CACHE);
|
||||
expect(mockCache.get).toHaveBeenCalledWith(ToolCacheKeys.MCP_SERVER('user1', 'github'));
|
||||
});
|
||||
|
||||
it('getMCPServerTools should return null when the cache lookup fails', async () => {
|
||||
const error = new Error('cache unavailable');
|
||||
mockCache.get.mockRejectedValue(error);
|
||||
|
||||
await expect(getMCPServerTools('user1', 'github')).resolves.toBeNull();
|
||||
expect(logger.error).toHaveBeenCalledWith(
|
||||
'[getMCPServerTools] Error fetching cached tools for github:',
|
||||
error,
|
||||
);
|
||||
});
|
||||
|
||||
it('getMCPServerTools should return null when the cache store is unavailable', async () => {
|
||||
const error = new Error('cache store unavailable');
|
||||
getLogStores.mockImplementationOnce(() => {
|
||||
throw error;
|
||||
});
|
||||
|
||||
await expect(getMCPServerTools('user1', 'github')).resolves.toBeNull();
|
||||
expect(logger.error).toHaveBeenCalledWith(
|
||||
'[getMCPServerTools] Error fetching cached tools for github:',
|
||||
error,
|
||||
);
|
||||
});
|
||||
|
||||
it('should NOT use CONFIG_STORE namespace', async () => {
|
||||
mockCache.get.mockResolvedValue(null);
|
||||
await getCachedTools();
|
||||
await getMCPServerTools('user1', 'github');
|
||||
await getCachedTools({ userId: 'user1', serverName: 'github' });
|
||||
mockCache.set.mockResolvedValue(true);
|
||||
await setCachedTools({ tool1: {} });
|
||||
mockCache.delete.mockResolvedValue(true);
|
||||
|
|
|
|||
|
|
@ -1,5 +1,4 @@
|
|||
const { CacheKeys, Time } = require('librechat-data-provider');
|
||||
const { logger } = require('@librechat/data-schemas');
|
||||
const getLogStores = require('~/cache/getLogStores');
|
||||
|
||||
/**
|
||||
|
|
@ -82,27 +81,9 @@ async function invalidateCachedTools(options = {}) {
|
|||
await Promise.all(keysToDelete.map((key) => cache.delete(key)));
|
||||
}
|
||||
|
||||
/**
|
||||
* Gets MCP tools for a specific server from cache
|
||||
* @function getMCPServerTools
|
||||
* @param {string} userId - The user ID
|
||||
* @param {string} serverName - The MCP server name
|
||||
* @returns {Promise<LCAvailableTools|null>} The available tools for the server
|
||||
*/
|
||||
async function getMCPServerTools(userId, serverName) {
|
||||
try {
|
||||
const cache = getLogStores(CacheKeys.TOOL_CACHE);
|
||||
return (await cache.get(ToolCacheKeys.MCP_SERVER(userId, serverName))) || null;
|
||||
} catch (error) {
|
||||
logger.error(`[getMCPServerTools] Error fetching cached tools for ${serverName}:`, error);
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
module.exports = {
|
||||
ToolCacheKeys,
|
||||
getCachedTools,
|
||||
setCachedTools,
|
||||
getMCPServerTools,
|
||||
invalidateCachedTools,
|
||||
};
|
||||
|
|
|
|||
|
|
@ -1,13 +1,17 @@
|
|||
const { createMCPToolCacheService } = require('@librechat/api');
|
||||
const { createMCPToolCacheService, MCPServersRegistry } = require('@librechat/api');
|
||||
const { getCachedTools, setCachedTools } = require('./getCachedTools');
|
||||
|
||||
const { mergeAppTools, cacheMCPServerTools, updateMCPServerTools } = createMCPToolCacheService({
|
||||
getCachedTools,
|
||||
setCachedTools,
|
||||
});
|
||||
const { mergeAppTools, cacheMCPServerTools, updateMCPServerTools, getMCPServerTools } =
|
||||
createMCPToolCacheService({
|
||||
getCachedTools,
|
||||
setCachedTools,
|
||||
getServerConfig: (serverName, userId) =>
|
||||
MCPServersRegistry.getInstance().getServerConfig(serverName, userId),
|
||||
});
|
||||
|
||||
module.exports = {
|
||||
mergeAppTools,
|
||||
getMCPServerTools,
|
||||
cacheMCPServerTools,
|
||||
updateMCPServerTools,
|
||||
};
|
||||
|
|
|
|||
|
|
@ -539,6 +539,48 @@ describe('initializeClient — subagent loading', () => {
|
|||
expect(arg.actionsEnabled).toBe(true);
|
||||
});
|
||||
|
||||
it('threads run-scoped MCP tool definitions into ON_TOOL_EXECUTE loading', async () => {
|
||||
/** Regression guard for the request-scoped MCP/PTC handoff: the
|
||||
* `mcpAvailableTools` discovered at run start must survive
|
||||
* `buildAgentToolContext` and reach `loadToolsForExecution`, otherwise
|
||||
* request-scoped servers reinitialize on every programmatic tool call
|
||||
* and can trip the MCP circuit breaker under parallel calls. */
|
||||
const mcpTool = 'list_tables_mcp_ClickHouse';
|
||||
const mcpAvailableTools = {
|
||||
ClickHouse: {
|
||||
[mcpTool]: {
|
||||
type: 'function',
|
||||
function: {
|
||||
name: mcpTool,
|
||||
description: 'List tables',
|
||||
parameters: { type: 'object', properties: {} },
|
||||
},
|
||||
},
|
||||
},
|
||||
};
|
||||
const primaryConfig = {
|
||||
...makePrimaryConfig({}),
|
||||
toolRegistry: new Map([[mcpTool, { name: mcpTool }]]),
|
||||
mcpAvailableTools,
|
||||
};
|
||||
mockInitializeAgent.mockResolvedValue(primaryConfig);
|
||||
|
||||
await initializeClient({
|
||||
req: makeSubagentReq(),
|
||||
res: {},
|
||||
signal: new AbortController().signal,
|
||||
endpointOption: makeEndpointOption(),
|
||||
});
|
||||
|
||||
expect(capturedToolExecuteOptions?.loadTools).toBeInstanceOf(Function);
|
||||
await capturedToolExecuteOptions.loadTools([mcpTool], PRIMARY_ID);
|
||||
|
||||
expect(mockLoadToolsForExecution).toHaveBeenCalledTimes(1);
|
||||
expect(mockLoadToolsForExecution).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ mcpAvailableTools }),
|
||||
);
|
||||
});
|
||||
|
||||
it('deduplicates repeated ids in subagents.agent_ids', async () => {
|
||||
const subAgent = await createAgent({
|
||||
id: DUPLICATE_SUBAGENT_ID,
|
||||
|
|
|
|||
|
|
@ -16,7 +16,6 @@ const {
|
|||
isActionDomainAllowed,
|
||||
buildWebSearchContext,
|
||||
buildImageToolContext,
|
||||
requiresEphemeralUserConnection,
|
||||
buildToolClassification,
|
||||
getMissingCustomUserVars,
|
||||
buildWebSearchDynamicContext,
|
||||
|
|
@ -755,12 +754,11 @@ async function loadToolDefinitionsWrapper({ req, res, agent, streamId = null, to
|
|||
return null;
|
||||
}
|
||||
|
||||
const requestScoped = requiresEphemeralUserConnection(serverConfig);
|
||||
if (mcpAvailableTools[serverName]) {
|
||||
return mcpAvailableTools[serverName];
|
||||
}
|
||||
|
||||
const cached = requestScoped ? null : await getMCPServerTools(userId, serverName);
|
||||
const cached = await getMCPServerTools(userId, serverName, serverConfig);
|
||||
if (cached) {
|
||||
rememberMCPAvailableTools(serverName, cached);
|
||||
await addPendingOAuthServer();
|
||||
|
|
|
|||
|
|
@ -219,7 +219,7 @@ async function reinitMCPServer({
|
|||
userId: user.id,
|
||||
serverName,
|
||||
tools,
|
||||
skipCache: ephemeralServer,
|
||||
serverConfig,
|
||||
});
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -144,6 +144,11 @@ describe('reinitMCPServer — customUserVars gating (issue #10969)', () => {
|
|||
it('disconnects ephemeral BODY-scoped connections after loading tools', async () => {
|
||||
const disconnect = jest.fn().mockResolvedValue(undefined);
|
||||
const tools = [{ name: 'search', inputSchema: { type: 'object', properties: {} } }];
|
||||
const serverConfig = {
|
||||
type: 'streamable-http',
|
||||
url: 'https://thingy.example.com/messages/{{LIBRECHAT_BODY_MESSAGEID}}/mcp',
|
||||
source: 'yaml',
|
||||
};
|
||||
mockGetConnection.mockResolvedValue({
|
||||
disconnect,
|
||||
fetchTools: jest.fn().mockResolvedValue(tools),
|
||||
|
|
@ -152,11 +157,7 @@ describe('reinitMCPServer — customUserVars gating (issue #10969)', () => {
|
|||
await reinitMCPServer({
|
||||
user,
|
||||
serverName,
|
||||
serverConfig: {
|
||||
type: 'streamable-http',
|
||||
url: 'https://thingy.example.com/messages/{{LIBRECHAT_BODY_MESSAGEID}}/mcp',
|
||||
source: 'yaml',
|
||||
},
|
||||
serverConfig,
|
||||
requestBody: { messageId: 'msg-789' },
|
||||
userMCPAuthMap: undefined,
|
||||
});
|
||||
|
|
@ -165,7 +166,7 @@ describe('reinitMCPServer — customUserVars gating (issue #10969)', () => {
|
|||
expect(mockUpdateMCPServerTools).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
tools,
|
||||
skipCache: true,
|
||||
serverConfig,
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -472,7 +472,11 @@ describe('ToolService - Action Capability Gating', () => {
|
|||
});
|
||||
|
||||
expect(result.toolDefinitions).toEqual([mcpTool]);
|
||||
expect(mockGetMCPServerTools).toHaveBeenCalledWith(req.user.id, serverName);
|
||||
expect(mockGetMCPServerTools).toHaveBeenCalledWith(
|
||||
req.user.id,
|
||||
serverName,
|
||||
expect.objectContaining({ requiresOAuth: true }),
|
||||
);
|
||||
expect(reinitMCPServer).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
serverName,
|
||||
|
|
@ -542,7 +546,11 @@ describe('ToolService - Action Capability Gating', () => {
|
|||
definitionsOnly: true,
|
||||
});
|
||||
|
||||
expect(mockGetMCPServerTools).toHaveBeenCalledWith(req.user.id, serverName);
|
||||
expect(mockGetMCPServerTools).toHaveBeenCalledWith(
|
||||
req.user.id,
|
||||
serverName,
|
||||
expect.objectContaining({ requiresOAuth: true }),
|
||||
);
|
||||
expect(reinitMCPServer).toHaveBeenCalledTimes(1);
|
||||
expect(reinitMCPServer).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
|
|
@ -756,7 +764,13 @@ describe('ToolService - Action Capability Gating', () => {
|
|||
requestBody: req.body,
|
||||
}),
|
||||
);
|
||||
expect(mockGetMCPServerTools).not.toHaveBeenCalled();
|
||||
expect(mockGetMCPServerTools).toHaveBeenCalledWith(
|
||||
req.user.id,
|
||||
serverName,
|
||||
expect.objectContaining({
|
||||
url: expect.stringContaining('LIBRECHAT_BODY_MESSAGEID'),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('returns run-scoped MCP tool definitions for request-scoped servers', async () => {
|
||||
|
|
@ -801,7 +815,13 @@ describe('ToolService - Action Capability Gating', () => {
|
|||
|
||||
expect(result.toolDefinitions).toEqual([mcpTool]);
|
||||
expect(result.mcpAvailableTools).toEqual({ [serverName]: availableTools });
|
||||
expect(mockGetMCPServerTools).not.toHaveBeenCalled();
|
||||
expect(mockGetMCPServerTools).toHaveBeenCalledWith(
|
||||
req.user.id,
|
||||
serverName,
|
||||
expect.objectContaining({
|
||||
url: expect.stringContaining('LIBRECHAT_BODY_MESSAGEID'),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('should preserve pending-flow expiry for OAuth URLs captured during discovery', async () => {
|
||||
|
|
@ -897,7 +917,11 @@ describe('ToolService - Action Capability Gating', () => {
|
|||
|
||||
expect(result.toolDefinitions).toEqual([mcpTool]);
|
||||
expect(mockGetServerConfig).not.toHaveBeenCalled();
|
||||
expect(mockGetMCPServerTools).toHaveBeenCalledWith(req.user.id, serverName);
|
||||
expect(mockGetMCPServerTools).toHaveBeenCalledWith(
|
||||
req.user.id,
|
||||
serverName,
|
||||
expect.objectContaining({ url: 'https://config.example.com/mcp' }),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue