diff --git a/api/server/services/ToolService.js b/api/server/services/ToolService.js index b4d948eda4..c2173095fb 100644 --- a/api/server/services/ToolService.js +++ b/api/server/services/ToolService.js @@ -271,19 +271,19 @@ async function processRequiredActions(client, requiredActions) { assistant_id: client.req.body.assistant_id, })) ?? []; - // Process all action sets once - // Map domains to their processed action sets - const processedDomains = new Map(); - const domainLookupMap = new Map(); + // Index every fully-qualified tool name to its owning action set. + // See loadAgentTools / loadActionToolsForExecution for the rationale: + // keying on the full tool name (operationId + delimiter + encoded + // domain) instead of the encoded domain alone is what allows two + // actions sharing a hostname to coexist on the same assistant + // without one silently shadowing the other. + const toolToAction = new Map(); for (const action of actionSets) { const domain = await domainParser(action.metadata.domain, true); - domainLookupMap.set(domain, domain); - + const normalizedDomain = domain.replace(domainSeparatorRegex, '_'); const legacyDomain = legacyDomainEncode(action.metadata.domain); - if (legacyDomain !== domain) { - domainLookupMap.set(legacyDomain, domain); - } + const legacyNormalized = legacyDomain.replace(domainSeparatorRegex, '_'); const isDomainAllowed = await isActionDomainAllowed( action.metadata.domain, @@ -316,7 +316,7 @@ async function processRequiredActions(client, requiredActions) { } // Process the OpenAPI spec - const { requestBuilders } = openapiToFunction(validationResult.spec); + const { requestBuilders, functionSignatures } = openapiToFunction(validationResult.spec); // Store encrypted values for OAuth flow const encrypted = { @@ -328,37 +328,31 @@ async function processRequiredActions(client, requiredActions) { const decryptedAction = { ...action }; decryptedAction.metadata = await decryptMetadata(action.metadata); - processedDomains.set(domain, { - action: decryptedAction, - requestBuilders, - encrypted, - }); + for (const sig of functionSignatures) { + const entry = { + action: decryptedAction, + requestBuilder: requestBuilders[sig.name], + encrypted, + }; + toolToAction.set(`${sig.name}${actionDelimiter}${normalizedDomain}`, entry); + if (legacyNormalized !== normalizedDomain) { + toolToAction.set(`${sig.name}${actionDelimiter}${legacyNormalized}`, entry); + } + } // Store builders for reuse ActionBuildersMap[action.metadata.domain] = requestBuilders; } - actionSetsData = { domainLookupMap, processedDomains }; + actionSetsData = { toolToAction }; } - let currentDomain = ''; - let matchedKey = ''; - for (const [key, canonical] of actionSetsData.domainLookupMap.entries()) { - if (currentAction.tool.includes(key)) { - currentDomain = canonical; - matchedKey = key; - break; - } - } - - if (!currentDomain || !actionSetsData.processedDomains.has(currentDomain)) { + const entry = actionSetsData.toolToAction.get(currentAction.tool); + if (!entry) { continue; } - const { action, requestBuilders, encrypted } = - actionSetsData.processedDomains.get(currentDomain); - const functionName = currentAction.tool.replace(`${actionDelimiter}${matchedKey}`, ''); - const requestBuilder = requestBuilders[functionName]; + const { action, requestBuilder, encrypted } = entry; if (!requestBuilder) { // throw new Error(`Tool ${currentAction.tool} not found.`); @@ -1008,17 +1002,18 @@ async function loadAgentTools({ }; } - const processedActionSets = new Map(); - const domainLookupMap = new Map(); + // See loadActionToolsForExecution below for the rationale: indexing + // by full tool name (operationId + delimiter + encoded domain) instead + // of by domain alone is what makes multi-action agents work when two + // actions share a hostname. + const toolToAction = new Map(); for (const action of actionSets) { const domain = await domainParser(action.metadata.domain, true); - domainLookupMap.set(domain, domain); - + const normalizedDomain = domain.replace(domainSeparatorRegex, '_'); const legacyDomain = legacyDomainEncode(action.metadata.domain); - if (legacyDomain !== domain) { - domainLookupMap.set(legacyDomain, domain); - } + const legacyNormalized = legacyDomain.replace(domainSeparatorRegex, '_'); + const isDomainAllowed = await isActionDomainAllowed( action.metadata.domain, appConfig?.actions?.allowedDomains, @@ -1063,13 +1058,19 @@ async function loadAgentTools({ true, ); - processedActionSets.set(domain, { - action: decryptedAction, - requestBuilders, - functionSignatures, - zodSchemas, - encrypted, - }); + for (const sig of functionSignatures) { + const entry = { + action: decryptedAction, + requestBuilder: requestBuilders[sig.name], + zodSchema: zodSchemas[sig.name], + functionSignature: sig, + encrypted, + }; + toolToAction.set(`${sig.name}${actionDelimiter}${normalizedDomain}`, entry); + if (legacyNormalized !== normalizedDomain) { + toolToAction.set(`${sig.name}${actionDelimiter}${legacyNormalized}`, entry); + } + } } // Now map tools to the processed action sets @@ -1080,52 +1081,35 @@ async function loadAgentTools({ continue; } - let currentDomain = ''; - let matchedKey = ''; - for (const [key, canonical] of domainLookupMap.entries()) { - if (toolName.includes(key)) { - currentDomain = canonical; - matchedKey = key; - break; - } - } - - if (!currentDomain || !processedActionSets.has(currentDomain)) { + const entry = toolToAction.get(toolName); + if (!entry) { continue; } - const { action, encrypted, zodSchemas, requestBuilders, functionSignatures } = - processedActionSets.get(currentDomain); - const functionName = toolName.replace(`${actionDelimiter}${matchedKey}`, ''); - const functionSig = functionSignatures.find((sig) => sig.name === functionName); - const requestBuilder = requestBuilders[functionName]; - const zodSchema = zodSchemas[functionName]; + const { action, encrypted, zodSchema, requestBuilder, functionSignature } = entry; + const _allowedDomains = appConfig?.actions?.allowedDomains; + const tool = await createActionTool({ + userId: req.user.id, + res, + action, + requestBuilder, + zodSchema, + encrypted, + name: toolName, + description: functionSignature.description, + streamId, + useSSRFProtection: !Array.isArray(_allowedDomains) || _allowedDomains.length === 0, + }); - if (requestBuilder) { - const _allowedDomains = appConfig?.actions?.allowedDomains; - const tool = await createActionTool({ - userId: req.user.id, - res, - action, - requestBuilder, - zodSchema, - encrypted, - name: toolName, - description: functionSig.description, - streamId, - useSSRFProtection: !Array.isArray(_allowedDomains) || _allowedDomains.length === 0, - }); - - if (!tool) { - logger.warn( - `Invalid action: user: ${req.user.id} | agent_id: ${agent.id} | toolName: ${toolName}`, - ); - throw new Error(`{"type":"${ErrorTypes.INVALID_ACTION}"}`); - } - - agentTools.push(tool); - ActionToolMap[toolName] = tool; + if (!tool) { + logger.warn( + `Invalid action: user: ${req.user.id} | agent_id: ${agent.id} | toolName: ${toolName}`, + ); + throw new Error(`{"type":"${ErrorTypes.INVALID_ACTION}"}`); } + + agentTools.push(tool); + ActionToolMap[toolName] = tool; } if (_agentTools.length > 0 && agentTools.length === 0) { @@ -1333,21 +1317,29 @@ async function loadActionToolsForExecution({ return loadedActionTools; } - const processedActionSets = new Map(); - /** Maps both new and legacy normalized domains to their canonical (new) domain key */ - const normalizedToDomain = new Map(); + /** + * Map every fully-qualified tool name to its owning action and the + * specific request builder / zod schema / signature it should resolve to. + * + * Indexing on the full tool name (`_action_`) + * instead of the encoded domain alone is what makes multi-action agents + * work: two actions that share a hostname now occupy distinct slots + * because the operationId disambiguates them. The previous shape stored + * one entry per encoded domain, so the second action of a pair would + * overwrite the first and its tools would silently disappear. + * + * Both the new and the legacy domain encodings are registered for each + * function so agents whose stored tool names predate the current + * encoding still resolve correctly. + */ + const toolToAction = new Map(); const allowedDomains = appConfig?.actions?.allowedDomains; for (const action of actionSets) { const domain = await domainParser(action.metadata.domain, true); const normalizedDomain = domain.replace(domainSeparatorRegex, '_'); - normalizedToDomain.set(normalizedDomain, domain); - const legacyDomain = legacyDomainEncode(action.metadata.domain); const legacyNormalized = legacyDomain.replace(domainSeparatorRegex, '_'); - if (legacyNormalized !== normalizedDomain) { - normalizedToDomain.set(legacyNormalized, domain); - } const isDomainAllowed = await isActionDomainAllowed(action.metadata.domain, allowedDomains); if (!isDomainAllowed) { @@ -1390,60 +1382,28 @@ async function loadActionToolsForExecution({ true, ); - processedActionSets.set(domain, { - action: decryptedAction, - requestBuilders, - functionSignatures, - zodSchemas, - encrypted, - legacyNormalized, - }); + for (const sig of functionSignatures) { + const entry = { + action: decryptedAction, + requestBuilder: requestBuilders[sig.name], + zodSchema: zodSchemas[sig.name], + functionSignature: sig, + encrypted, + }; + toolToAction.set(`${sig.name}${actionDelimiter}${normalizedDomain}`, entry); + if (legacyNormalized !== normalizedDomain) { + toolToAction.set(`${sig.name}${actionDelimiter}${legacyNormalized}`, entry); + } + } } for (const toolName of actionToolNames) { - let currentDomain = ''; - for (const [normalizedDomain, canonicalDomain] of normalizedToDomain.entries()) { - if (toolName.includes(normalizedDomain)) { - currentDomain = canonicalDomain; - break; - } - } - - if (!currentDomain || !processedActionSets.has(currentDomain)) { - continue; - } - - const { action, encrypted, zodSchemas, requestBuilders, functionSignatures, legacyNormalized } = - processedActionSets.get(currentDomain); - const normalizedDomain = currentDomain.replace(domainSeparatorRegex, '_'); - const functionName = toolName.replace(`${actionDelimiter}${normalizedDomain}`, ''); - const functionSig = functionSignatures.find((sig) => sig.name === functionName); - const requestBuilder = requestBuilders[functionName]; - const zodSchema = zodSchemas[functionName]; - - if (!requestBuilder) { - const legacyFnName = toolName.replace(`${actionDelimiter}${legacyNormalized}`, ''); - if (legacyFnName !== toolName && requestBuilders[legacyFnName]) { - const legacyTool = await createActionTool({ - userId: req.user.id, - res, - action, - streamId, - encrypted, - requestBuilder: requestBuilders[legacyFnName], - zodSchema: zodSchemas[legacyFnName], - name: toolName, - description: - functionSignatures.find((sig) => sig.name === legacyFnName)?.description ?? '', - useSSRFProtection: !Array.isArray(allowedDomains) || allowedDomains.length === 0, - }); - if (legacyTool) { - loadedActionTools.push(legacyTool); - } - } + const entry = toolToAction.get(toolName); + if (!entry) { continue; } + const { action, encrypted, zodSchema, requestBuilder, functionSignature } = entry; const tool = await createActionTool({ userId: req.user.id, res, @@ -1453,7 +1413,7 @@ async function loadActionToolsForExecution({ encrypted, requestBuilder, name: toolName, - description: functionSig?.description ?? '', + description: functionSignature?.description ?? '', useSSRFProtection: !Array.isArray(allowedDomains) || allowedDomains.length === 0, }); diff --git a/api/server/services/__tests__/ToolService.spec.js b/api/server/services/__tests__/ToolService.spec.js index 740bb06e5a..f38b900bff 100644 --- a/api/server/services/__tests__/ToolService.spec.js +++ b/api/server/services/__tests__/ToolService.spec.js @@ -31,6 +31,10 @@ jest.mock('~/app/clients/tools/util', () => ({ })); const mockLoadActionSets = jest.fn(); +const mockDomainParser = jest.fn(); +const mockLegacyDomainEncode = jest.fn(); +const mockDecryptMetadata = jest.fn(); +const mockCreateActionTool = jest.fn(); jest.mock('~/server/services/Tools/credentials', () => ({ loadAuthValues: jest.fn().mockResolvedValue({}), })); @@ -52,9 +56,10 @@ jest.mock('~/server/services/Files/Code/process', () => ({ })); jest.mock('../ActionService', () => ({ loadActionSets: (...args) => mockLoadActionSets(...args), - decryptMetadata: jest.fn(), - createActionTool: jest.fn(), - domainParser: jest.fn(), + decryptMetadata: (...args) => mockDecryptMetadata(...args), + createActionTool: (...args) => mockCreateActionTool(...args), + domainParser: (...args) => mockDomainParser(...args), + legacyDomainEncode: (...args) => mockLegacyDomainEncode(...args), })); jest.mock('~/server/services/Threads', () => ({ recordUsage: jest.fn(), @@ -536,4 +541,151 @@ describe('ToolService - Action Capability Gating', () => { expect(enabledCapabilities.has(AgentCapabilities.deferred_tools)).toBe(true); }); }); + + describe('multi-action domain collision regression', () => { + // Two distinct OpenAPI Actions whose `servers[0].url` resolves to the + // same hostname must both contribute their tools to the agent. The + // previous implementation indexed processed action sets by encoded + // domain, so the second action overwrote the first in the map and one + // action's tools silently disappeared from the LLM payload. + // + // The encoded domain we use as the lookup key for the action sets is + // mocked to a fixed string for both actions to make the collision + // condition deterministic without depending on the real base64 + // truncation rules. + const SHARED_DOMAIN = 'https://api.example.com'; + const ENCODED_DOMAIN = 'shared_dom'; + const LEGACY_ENCODED_DOMAIN = 'legacy_dom'; + + const buildSpec = (operationId, path) => + JSON.stringify({ + openapi: '3.0.3', + info: { title: `Mock ${operationId}`, version: '1.0.0' }, + servers: [{ url: SHARED_DOMAIN }], + paths: { + [path]: { + get: { + operationId, + summary: `Mock ${operationId}`, + responses: { + 200: { + description: 'OK', + content: { 'application/json': { schema: { type: 'object' } } }, + }, + }, + }, + }, + }, + }); + + const actionA = { + action_id: 'action_a', + metadata: { + domain: SHARED_DOMAIN, + raw_spec: buildSpec('echoMessage', '/echo'), + }, + }; + const actionB = { + action_id: 'action_b', + metadata: { + domain: SHARED_DOMAIN, + raw_spec: buildSpec('listItems', '/items'), + }, + }; + + const toolNameA = `echoMessage${actionDelimiter}${ENCODED_DOMAIN}`; + const toolNameB = `listItems${actionDelimiter}${ENCODED_DOMAIN}`; + + beforeEach(() => { + // Both actions share a hostname → both call sites get the same encoded + // value back. This is precisely the collision shape that triggered + // the bug in production. + mockDomainParser.mockResolvedValue(ENCODED_DOMAIN); + mockLegacyDomainEncode.mockReturnValue(LEGACY_ENCODED_DOMAIN); + mockDecryptMetadata.mockImplementation(async (metadata) => metadata); + mockCreateActionTool.mockImplementation(async ({ name, requestBuilder }) => ({ + name, + // Surface the request builder identity on the returned tool so + // assertions can verify each tool was wired to the correct action's + // builder, not its sibling's. + _builder: requestBuilder, + _call: jest.fn(), + schema: {}, + description: '', + })); + }); + + const expectBothActionsResolved = (calls) => { + const callsByName = new Map(calls.map((c) => [c[0].name, c[0]])); + expect(callsByName.has(toolNameA)).toBe(true); + expect(callsByName.has(toolNameB)).toBe(true); + // Each tool's request builder must come from the matching action's + // own parsed spec — not the sibling's. The previous bug would either + // route both to the same action's builders (and drop one as + // undefined) or silently skip one entirely. + const builderA = callsByName.get(toolNameA).requestBuilder; + const builderB = callsByName.get(toolNameB).requestBuilder; + expect(builderA).toBeDefined(); + expect(builderB).toBeDefined(); + expect(builderA).not.toBe(builderB); + // Each builder targets its own operation path — confirms the + // request builder lookup didn't cross-contaminate between actions. + expect(builderA.path).toBe('/echo'); + expect(builderB.path).toBe('/items'); + }; + + it('loadAgentTools resolves both actions when they share a hostname', async () => { + mockLoadActionSets.mockResolvedValue([actionA, actionB]); + const capabilities = [AgentCapabilities.tools, AgentCapabilities.actions]; + const req = createMockReq(capabilities); + mockGetEndpointsConfig.mockResolvedValue(createEndpointsConfig(capabilities)); + + await loadAgentTools({ + req, + res: {}, + agent: { id: 'agent_collision', tools: [toolNameA, toolNameB] }, + definitionsOnly: false, + }); + + expect(mockCreateActionTool).toHaveBeenCalledTimes(2); + expectBothActionsResolved(mockCreateActionTool.mock.calls); + }); + + it('loadAgentTools is order-invariant for two actions sharing a hostname', async () => { + // Reverse the actionSets order — what used to flip the "winner" of + // the encoded-domain Map overwrite must now make zero observable + // difference. + mockLoadActionSets.mockResolvedValue([actionB, actionA]); + const capabilities = [AgentCapabilities.tools, AgentCapabilities.actions]; + const req = createMockReq(capabilities); + mockGetEndpointsConfig.mockResolvedValue(createEndpointsConfig(capabilities)); + + await loadAgentTools({ + req, + res: {}, + agent: { id: 'agent_collision', tools: [toolNameA, toolNameB] }, + definitionsOnly: false, + }); + + expect(mockCreateActionTool).toHaveBeenCalledTimes(2); + expectBothActionsResolved(mockCreateActionTool.mock.calls); + }); + + it('loadToolsForExecution resolves both actions when they share a hostname', async () => { + mockLoadActionSets.mockResolvedValue([actionA, actionB]); + const req = createMockReq([AgentCapabilities.actions]); + req.config = {}; + + await loadToolsForExecution({ + req, + res: {}, + agent: { id: 'agent_collision' }, + toolNames: [toolNameA, toolNameB], + actionsEnabled: true, + }); + + expect(mockCreateActionTool).toHaveBeenCalledTimes(2); + expectBothActionsResolved(mockCreateActionTool.mock.calls); + }); + }); });