From d643444e8847cba6de7a1f7fbf06ccdcf9fa1573 Mon Sep 17 00:00:00 2001 From: lrreverence Date: Sat, 11 Apr 2026 01:57:13 +0800 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20fix:=20Normalize=20action=20tool?= =?UTF-8?q?=20name=20at=20lookup=20+=20cover=20assistants=20path?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the multi-action domain collision fix. Addresses PR #12594 review feedback: **Must-fix #1 — short-hostname lookup mismatch.** The toolToAction map is keyed on the `_`-collapsed domain, but `agent.tools` and `currentAction.tool` persist the raw `domainParser(..., true)` output, which for hostnames ≤ ENCODED_DOMAIN_LENGTH is a `---`-separated string (e.g. `medium---com`). Exact-match `Map.get()` missed those keys and silently dropped the tool. Fix: normalize every incoming tool name through a new `normalizeActionToolName` helper before the lookup in `loadAgentTools`, `processRequiredActions`, and `loadActionToolsForExecution`. **Must-fix #2 — assistants path coverage.** `processRequiredActions` received the same structural rewrite but had zero tests. Added a regression test under `multi-action domain collision regression` that drives two shared-hostname actions through the assistants path and asserts each tool reaches its own request builder. **Must-fix #3 — legacy encoding branch coverage.** The `if (legacyNormalized !== normalizedDomain)` registration was never exercised by any test. Added a test where `agent.tools` stores the legacy-format name and asserts it still resolves. **Should-fix #4 — DRY the registration loop.** Extracted `registerActionTools({ toolToAction, functionSignatures, normalizedDomain, legacyNormalized, makeEntry })`. All three call sites now share the same key-building logic; the key template lives in one place. **Should-fix #5 — remove stale optional chaining.** In `loadActionToolsForExecution`, `functionSignature?.description ?? ''` became `functionSignature.description` — `sig` is always defined by the iterator, matching the style of `loadAgentTools`. **Should-fix #6 — drop unreachable `!requestBuilder` guard.** Entries in `processRequiredActions` are now pre-built with `requestBuilder: requestBuilders[sig.name]`, which `openapiToFunction` always produces alongside the signature, so the guard is dead. **Should-fix #7 — unwrap `actionSetsData`.** It now holds a bare `Map` instead of `{ toolToAction }`; the sentinel `!actionSetsData` check still works because `new Map()` is truthy. Also added a short-hostname regression test (`loadAgentTools resolves raw ---separated tool names`) that reproduces Must-fix #1: it fails against the previous commit (0 create calls) and passes with the normalization in place. 41 tests, all passing. The 3 new regression tests are under `multi-action domain collision regression` and cover the assistants path, the legacy encoding branch, and the short-hostname lookup path. --- api/server/services/ToolService.js | 100 ++++++++++------ .../services/__tests__/ToolService.spec.js | 111 ++++++++++++++++++ 2 files changed, 177 insertions(+), 34 deletions(-) diff --git a/api/server/services/ToolService.js b/api/server/services/ToolService.js index c2173095fb..f818cc3833 100644 --- a/api/server/services/ToolService.js +++ b/api/server/services/ToolService.js @@ -71,6 +71,43 @@ const { getLogStores } = require('~/cache'); const domainSeparatorRegex = new RegExp(actionDomainSeparator, 'g'); +/** + * Collapse every `actionDomainSeparator` sequence in a fully-qualified + * action tool name to an underscore. Agents can store tool names in the + * raw `domainParser(..., true)` output, which for short hostnames is a + * `---`-separated string (e.g. `medium---com`). The lookup maps below + * are always keyed with the `_`-collapsed form, so every read must + * normalize first or short-hostname tools silently fail to resolve. + */ +const normalizeActionToolName = (toolName) => toolName.replace(domainSeparatorRegex, '_'); + +/** + * Populate a `toolToAction` map with one slot per fully-qualified tool + * name (``). Both the new + * and the legacy encodings of the domain are registered for every + * function so agents whose stored tool names predate the current + * encoding still resolve correctly. + * + * Indexing on the full tool name instead of the encoded domain alone is + * what makes multi-action agents work when two actions share a hostname: + * the operationId disambiguates them, so neither overwrites the other. + */ +const registerActionTools = ({ + toolToAction, + functionSignatures, + normalizedDomain, + legacyNormalized, + makeEntry, +}) => { + for (const sig of functionSignatures) { + const entry = makeEntry(sig); + toolToAction.set(`${sig.name}${actionDelimiter}${normalizedDomain}`, entry); + if (legacyNormalized !== normalizedDomain) { + toolToAction.set(`${sig.name}${actionDelimiter}${legacyNormalized}`, entry); + } + } +}; + /** * Resolves the set of enabled agent capabilities from endpoints config, * falling back to app-level or default capabilities for ephemeral agents. @@ -328,37 +365,32 @@ async function processRequiredActions(client, requiredActions) { const decryptedAction = { ...action }; decryptedAction.metadata = await decryptMetadata(action.metadata); - for (const sig of functionSignatures) { - const entry = { + registerActionTools({ + toolToAction, + functionSignatures, + normalizedDomain, + legacyNormalized, + makeEntry: (sig) => ({ 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 = { toolToAction }; + actionSetsData = toolToAction; } - const entry = actionSetsData.toolToAction.get(currentAction.tool); + const entry = actionSetsData.get(normalizeActionToolName(currentAction.tool)); if (!entry) { continue; } const { action, requestBuilder, encrypted } = entry; - if (!requestBuilder) { - // throw new Error(`Tool ${currentAction.tool} not found.`); - continue; - } - // We've already decrypted the metadata, so we can pass it directly const _allowedDomains = appConfig?.actions?.allowedDomains; tool = await createActionTool({ @@ -1058,19 +1090,19 @@ async function loadAgentTools({ true, ); - for (const sig of functionSignatures) { - const entry = { + registerActionTools({ + toolToAction, + functionSignatures, + normalizedDomain, + legacyNormalized, + makeEntry: (sig) => ({ 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 @@ -1081,7 +1113,7 @@ async function loadAgentTools({ continue; } - const entry = toolToAction.get(toolName); + const entry = toolToAction.get(normalizeActionToolName(toolName)); if (!entry) { continue; } @@ -1382,23 +1414,23 @@ async function loadActionToolsForExecution({ true, ); - for (const sig of functionSignatures) { - const entry = { + registerActionTools({ + toolToAction, + functionSignatures, + normalizedDomain, + legacyNormalized, + makeEntry: (sig) => ({ 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) { - const entry = toolToAction.get(toolName); + const entry = toolToAction.get(normalizeActionToolName(toolName)); if (!entry) { continue; } @@ -1413,7 +1445,7 @@ async function loadActionToolsForExecution({ encrypted, requestBuilder, name: toolName, - description: functionSignature?.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 f38b900bff..730fe88cf5 100644 --- a/api/server/services/__tests__/ToolService.spec.js +++ b/api/server/services/__tests__/ToolService.spec.js @@ -80,6 +80,7 @@ jest.mock('~/cache', () => ({ const { loadAgentTools, loadToolsForExecution, + processRequiredActions, resolveAgentCapabilities, } = require('../ToolService'); @@ -687,5 +688,115 @@ describe('ToolService - Action Capability Gating', () => { expect(mockCreateActionTool).toHaveBeenCalledTimes(2); expectBothActionsResolved(mockCreateActionTool.mock.calls); }); + + it('processRequiredActions resolves both actions when they share a hostname', async () => { + // The assistants/threads path received the same structural rewrite + // as the agent paths. Cover it directly so future regressions in the + // `toolToAction` map shape or the lookup normalization don't slip + // through just because the agent-path tests still pass. + mockLoadActionSets.mockResolvedValue([actionA, actionB]); + const client = { + req: { + user: { id: 'user_123' }, + body: { + assistant_id: 'assistant_collision', + model: 'gpt-4o-mini', + endpoint: 'openAI', + }, + config: {}, + }, + res: {}, + apiKey: 'sk-test', + mappedOrder: new Map(), + seenToolCalls: new Map(), + addContentData: jest.fn(), + }; + + await processRequiredActions(client, [ + { + tool: toolNameA, + toolInput: {}, + toolCallId: 'call_a', + thread_id: 'thread_1', + run_id: 'run_1', + }, + { + tool: toolNameB, + toolInput: {}, + toolCallId: 'call_b', + thread_id: 'thread_1', + run_id: 'run_1', + }, + ]); + + // The assistants path intentionally doesn't forward `name` to + // createActionTool (see ToolService.js — "intentionally not passing + // zodSchema, name, and description for assistants API"), so key + // resolution assertions off the request builder path instead. + expect(mockCreateActionTool).toHaveBeenCalledTimes(2); + const builderPaths = mockCreateActionTool.mock.calls.map( + (c) => c[0].requestBuilder?.path, + ); + expect(builderPaths).toEqual(expect.arrayContaining(['/echo', '/items'])); + // Each call must carry a distinct builder — guards against the bug + // where the surviving action's builders got routed to every tool. + expect(builderPaths[0]).not.toBe(builderPaths[1]); + }); + + it('loadAgentTools resolves legacy-format tool names via the legacy encoding branch', async () => { + // Agents whose tool names predate the current domain encoding store + // them under `legacyDomainEncode`'s output. The map registers both + // encodings per function so these keep resolving after the fix; + // this test exercises the `if (legacyNormalized !== normalizedDomain)` + // branch, which was previously never hit by any test. + mockLoadActionSets.mockResolvedValue([actionA]); + const legacyToolName = `echoMessage${actionDelimiter}${LEGACY_ENCODED_DOMAIN}`; + const capabilities = [AgentCapabilities.tools, AgentCapabilities.actions]; + const req = createMockReq(capabilities); + mockGetEndpointsConfig.mockResolvedValue(createEndpointsConfig(capabilities)); + + await loadAgentTools({ + req, + res: {}, + agent: { id: 'agent_legacy', tools: [legacyToolName] }, + definitionsOnly: false, + }); + + expect(mockCreateActionTool).toHaveBeenCalledTimes(1); + const [callArgs] = mockCreateActionTool.mock.calls[0]; + expect(callArgs.name).toBe(legacyToolName); + expect(callArgs.requestBuilder.path).toBe('/echo'); + }); + + it('loadAgentTools resolves raw `---`-separated tool names from agent.tools', async () => { + // Hostnames at or below ENCODED_DOMAIN_LENGTH round-trip through + // `domainParser(..., true)` as a `---`-separated string, and agents + // persist that raw form in `agent.tools`. The map is always keyed + // with the `_`-collapsed form, so the lookup must normalize the + // incoming name or short-hostname tools silently drop out. + mockDomainParser.mockResolvedValue('shared---dom'); + mockLoadActionSets.mockResolvedValue([actionA, actionB]); + const rawNameA = `echoMessage${actionDelimiter}shared---dom`; + const rawNameB = `listItems${actionDelimiter}shared---dom`; + const capabilities = [AgentCapabilities.tools, AgentCapabilities.actions]; + const req = createMockReq(capabilities); + mockGetEndpointsConfig.mockResolvedValue(createEndpointsConfig(capabilities)); + + await loadAgentTools({ + req, + res: {}, + agent: { id: 'agent_short', tools: [rawNameA, rawNameB] }, + definitionsOnly: false, + }); + + expect(mockCreateActionTool).toHaveBeenCalledTimes(2); + const callsByName = new Map( + mockCreateActionTool.mock.calls.map((c) => [c[0].name, c[0]]), + ); + expect(callsByName.has(rawNameA)).toBe(true); + expect(callsByName.has(rawNameB)).toBe(true); + expect(callsByName.get(rawNameA).requestBuilder.path).toBe('/echo'); + expect(callsByName.get(rawNameB).requestBuilder.path).toBe('/items'); + }); }); });