CERBERUS/LibreChat
1
0
Fork 0
mirror of https://github.com/danny-avila/LibreChat.git synced 2026-08-04 14:57:42 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 6

🔀 fix: Resolve Action Tools by Exact Name to Prevent Multi-Action Domain Collision (#12594)

Browse source 
* 🐛 fix: resolve Action tools by exact tool name to prevent multi-action collision

When two OpenAPI Actions on the same Agent share a hostname, the second
action's entry overwrote the first in the encoded-domain Map and one
action's tools silently disappeared from the LLM payload. The buggy
resolution loop also used substring matching, which caused similar
shadowing for any encoded-domain prefix overlap.

This change builds a Map keyed on the full tool name
(`<operationId>_action_<encoded-domain>`) directly, mirroring the exact
lookup pattern that getActionToolDefinitions already uses. Each function
in an action's spec gets its own slot, so two actions sharing a hostname
no longer collide. 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.

Applied at all three call sites that had the buggy pattern:

  - processRequiredActions (assistants/threads path)
  - loadAgentTools (agent build path)
  - loadActionToolsForExecution (agent execution path)

Adds three regression tests covering both ordering directions and the
execution path. Tests fail without the fix and pass with it.

* 🐛 fix: Normalize action tool name at lookup + cover assistants path

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.

* 🐛 fix: Tighten registerActionTools key handling and assistants test

Follow-up to d643444 addressing the second review pass on PR #12594.

**ESLint** — Two prettier errors in the spec file (multi-line arrow
function bodies that should fit on one line). Auto-fixed.

**[MINOR] operationId containing `---` → key mismatch.** The lookup
path collapsed every `actionDomainSeparator` sequence in the full tool
name, but the registration path passed `sig.name` through unchanged.
A `---` that survived into an operationId would shift the underscore
boundary at lookup and miss its own key. Fix in `registerActionTools`:
normalize `sig.name` with the same helper so registration and lookup
always agree on the canonical form. `sanitizeOperationId` strips the
characters that produce `---` in practice, so this is theoretical
hardening, not a fix for a known reproducer.

**[MINOR] Same-operationId + same-hostname silent overwrite.** Two
actions sharing both an operationId and a hostname still produced a
silent `Map.set()` overwrite (the new key is identical, so neither the
operationId nor the domain disambiguates). Added a `setKey` helper
inside `registerActionTools` that logs a `[Actions] operationId
collision: ...` warning whenever a key is already present, naming the
overwriting action_id. The silent-overwrite mode from the original bug
cannot reappear under a different disguise without surfacing in the
logs.

**[NIT] processRequiredActions test simulated a runtime crash.**
`mockCreateActionTool` returned a tool with `_call: jest.fn()`, which
resolves to `undefined`. `processRequiredActions` chains
`.then(handleToolOutput).catch(handleToolError)` directly onto that
return, so `undefined.then(...)` threw synchronously and the outer
try/catch funneled the error into `handleToolError`. Creation count
assertions still passed because `createActionTool` runs before the
crash, but the test was silently exercising the failure path. Updated
the global mock to `_call: jest.fn().mockResolvedValue('{"status":"ok"}')`
so the success path runs end-to-end. The assistants regression test
now executes in ~5ms instead of ~90ms, which corroborates that it's
no longer hitting the synchronous throw.

**[NIT] Duplicated rationale comments.** All three call sites carried
multi-line comment blocks restating why we key on the full tool name.
That rationale now lives canonically in `registerActionTools`'s JSDoc;
the inline blocks collapsed to `// See registerActionTools for the
key-shape rationale.` Net -22 lines of comments.

41/41 tests still pass; lint is clean.

* 🐛 fix: Scope tool-name normalization to the encoded-domain suffix

Follow-up to f22228e addressing the Codex P1 on PR #12594.

**Regression.** The previous commit normalized the entire tool name
(`normalizeActionToolName(sig.name)` at registration, full-name
`.replace()` at lookup) to handle operationIds that theoretically
contained `---`. But `openapiToFunction` uses user-supplied
operationIds verbatim and the fallback `sanitizeOperationId` only
strips characters outside `[a-zA-Z0-9_-]`, so specs can legitimately
produce operationIds like `get_foo---bar` and `get_foo_bar` side by
side. Collapsing `---` to `_` across the entire key merged those two
into a single map slot — one silently overwrote the other, and both
tool requests routed to the surviving entry's request builder.

**Fix.** Limit normalization to the encoded-domain portion of the
full tool name, i.e. the substring after the last `actionDelimiter`.
The operationId half is left verbatim, so hyphens-vs-underscores
remain disambiguating. The short-hostname bug (Must-fix #1 from the
original review) is still covered because the `---` → `_` collapse
still happens on the domain suffix where it matters:

```js
const normalizeActionToolName = (toolName) => {
  const delimiterIndex = toolName.lastIndexOf(actionDelimiter);
  if (delimiterIndex === -1) return toolName;
  const prefixEnd = delimiterIndex + actionDelimiter.length;
  const encodedDomain = toolName.slice(prefixEnd);
  return toolName.slice(0, prefixEnd) + encodedDomain.replace(domainSeparatorRegex, '_');
};
```

`registerActionTools` reverts to `sig.name` verbatim — no more
`normalizeActionToolName(sig.name)` ahead of the key build.

**Regression test.** Added
`loadAgentTools distinguishes operationIds that differ only by
---` vs `_`` under `multi-action domain collision regression`. It
loads two actions sharing a hostname whose operationIds are
`get_foo---bar` and `get_foo_bar` respectively, each pointing at a
different path (`/foo-bar`, `/foo_bar`), and asserts that each tool
resolves to its own request builder. Verified the test fails against
f22228e (`hyphenTool` resolves to `/foo_bar` — the sibling's builder)
and passes with this commit.

42/42 tests pass; lint clean.
This commit is contained in:
Caesar Isidro Va-ay 2026-04-13 21:08:06 +08:00 • committed by GitHub
parent 277fdd2b43
commit 9b9a86d17d
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 473 additions and 145 deletions
Download patch file Download diff file Expand all files Collapse all files

294
api/server/services/ToolService.js
View file

@ -71,6 +71,83 @@ const { getLogStores } = require('~/cache');
const domainSeparatorRegex = new RegExp(actionDomainSeparator, 'g');
/**
* Collapse every `actionDomainSeparator` sequence in the encoded-domain
* suffix of 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 domain, so every read must normalize that suffix or
* short-hostname tools silently fail to resolve.
*
* The operationId portion (everything before the last `actionDelimiter`)
* is deliberately left untouched: `openapiToFunction` preserves hyphens
* in generated operationIds, so two specs can legitimately produce
* operationIds that differ only in hyphens-vs-underscores (e.g.
* `get_foo---bar` vs `get_foo_bar`). Collapsing the operationId would
* merge those into a single map slot and silently drop one tool.
*/
const normalizeActionToolName = (toolName) => {
const delimiterIndex = toolName.lastIndexOf(actionDelimiter);
if (delimiterIndex === -1) {
return toolName;
}
const prefixEnd = delimiterIndex + actionDelimiter.length;
const encodedDomain = toolName.slice(prefixEnd);
return toolName.slice(0, prefixEnd) + encodedDomain.replace(domainSeparatorRegex, '_');
};
/**
* Populate a `toolToAction` map with one slot per fully-qualified tool
* name (`<operationId><actionDelimiter><encoded-domain>`). 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.
*
* Two actions that additionally share the same operationId still
* collide (nothing in the key distinguishes them). That case is
* pathological — `sanitizeOperationId` plus OpenAPI's own uniqueness
* requirement make it very unlikely — but when it does happen we log
* a warning so the silent-overwrite mode from the original bug cannot
* reappear under a different disguise.
*/
const registerActionTools = ({
toolToAction,
functionSignatures,
normalizedDomain,
legacyNormalized,
makeEntry,
}) => {
const setKey = (key, entry) => {
if (toolToAction.has(key)) {
logger.warn(
`[Actions] operationId collision: "${key}" already registered; ` +
`action "${entry.action?.action_id}" overwrites the previous entry. ` +
`Two actions share both the operationId and the encoded hostname.`,
);
}
toolToAction.set(key, entry);
};
for (const sig of functionSignatures) {
const entry = makeEntry(sig);
// Use `sig.name` verbatim: `openapiToFunction` keeps hyphens in
// generated operationIds, so `get_foo---bar` and `get_foo_bar` are
// distinct operations on the same spec. `normalizeActionToolName`
// only touches the encoded-domain suffix at lookup time, so map
// keys and lookups stay consistent without merging distinct
// operationIds into the same slot.
setKey(`${sig.name}${actionDelimiter}${normalizedDomain}`, entry);
if (legacyNormalized !== normalizedDomain) {
setKey(`${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.
@ -271,19 +348,14 @@ 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();
// See registerActionTools for the key-shape rationale.
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 +388,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,42 +400,31 @@ async function processRequiredActions(client, requiredActions) {
const decryptedAction = { ...action };
decryptedAction.metadata = await decryptMetadata(action.metadata);
processedDomains.set(domain, {
action: decryptedAction,
requestBuilders,
encrypted,
registerActionTools({
toolToAction,
functionSignatures,
normalizedDomain,
legacyNormalized,
makeEntry: (sig) => ({
action: decryptedAction,
requestBuilder: requestBuilders[sig.name],
encrypted,
}),
});
// 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.get(normalizeActionToolName(currentAction.tool));
if (!entry) {
continue;
}
const { action, requestBuilders, encrypted } =
actionSetsData.processedDomains.get(currentDomain);
const functionName = currentAction.tool.replace(`${actionDelimiter}${matchedKey}`, '');
const requestBuilder = requestBuilders[functionName];
if (!requestBuilder) {
// throw new Error(`Tool ${currentAction.tool} not found.`);
continue;
}
const { action, requestBuilder, encrypted } = entry;
// We've already decrypted the metadata, so we can pass it directly
const _allowedDomains = appConfig?.actions?.allowedDomains;
@ -1008,17 +1069,15 @@ async function loadAgentTools({
};
}
const processedActionSets = new Map();
const domainLookupMap = new Map();
// See registerActionTools for the key-shape rationale.
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,12 +1122,18 @@ async function loadAgentTools({
true,
);
processedActionSets.set(domain, {
action: decryptedAction,
requestBuilders,
registerActionTools({
toolToAction,
functionSignatures,
zodSchemas,
encrypted,
normalizedDomain,
legacyNormalized,
makeEntry: (sig) => ({
action: decryptedAction,
requestBuilder: requestBuilders[sig.name],
zodSchema: zodSchemas[sig.name],
functionSignature: sig,
encrypted,
}),
});
}
@ -1080,52 +1145,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(normalizeActionToolName(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 +1381,15 @@ 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();
// See registerActionTools for the key-shape rationale.
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 +1432,28 @@ async function loadActionToolsForExecution({
true,
);
processedActionSets.set(domain, {
action: decryptedAction,
requestBuilders,
registerActionTools({
toolToAction,
functionSignatures,
zodSchemas,
encrypted,
normalizedDomain,
legacyNormalized,
makeEntry: (sig) => ({
action: decryptedAction,
requestBuilder: requestBuilders[sig.name],
zodSchema: zodSchemas[sig.name],
functionSignature: sig,
encrypted,
}),
});
}
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(normalizeActionToolName(toolName));
if (!entry) {
continue;
}
const { action, encrypted, zodSchema, requestBuilder, functionSignature } = entry;
const tool = await createActionTool({
userId: req.user.id,
res,
@ -1453,7 +1463,7 @@ async function loadActionToolsForExecution({
encrypted,
requestBuilder,
name: toolName,
description: functionSig?.description ?? '',
description: functionSignature.description,
useSSRFProtection: !Array.isArray(allowedDomains) || allowedDomains.length === 0,
});

324
api/server/services/__tests__/ToolService.spec.js
View file

@ -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(),
@ -75,6 +80,7 @@ jest.mock('~/cache', () => ({
const {
loadAgentTools,
loadToolsForExecution,
processRequiredActions,
resolveAgentCapabilities,
} = require('../ToolService');
@ -536,4 +542,316 @@ 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,
// Resolve instead of returning undefined — processRequiredActions
// chains `.then(handleToolOutput)` directly onto this call, which
// would throw synchronously on an undefined return and mask the
// test as a simulated runtime crash.
_call: jest.fn().mockResolvedValue('{"status":"ok"}'),
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);
});
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 distinguishes operationIds that differ only by `---` vs `_`', async () => {
// `openapiToFunction` uses the user-supplied operationId verbatim
// and only sanitizes the synthetic `<method>_<path>` fallback, and
// `sanitizeOperationId` preserves `-`. So two operations whose
// operationIds differ only by `---` vs `_` (e.g. `get_foo---bar`
// and `get_foo_bar`) are legitimately distinct on the same spec —
// or, here, on two actions sharing a hostname.
//
// Normalization must only touch the encoded-domain suffix after
// `actionDelimiter`; if it also collapsed the operationId, both
// tools would write to the same map slot and resolve to the
// surviving entry's request builder.
const hyphenSpec = {
action_id: 'action_hyphen',
metadata: {
domain: SHARED_DOMAIN,
raw_spec: buildSpec('get_foo---bar', '/foo-bar'),
},
};
const underscoreSpec = {
action_id: 'action_underscore',
metadata: {
domain: SHARED_DOMAIN,
raw_spec: buildSpec('get_foo_bar', '/foo_bar'),
},
};
mockLoadActionSets.mockResolvedValue([hyphenSpec, underscoreSpec]);
const hyphenTool = `get_foo---bar${actionDelimiter}${ENCODED_DOMAIN}`;
const underscoreTool = `get_foo_bar${actionDelimiter}${ENCODED_DOMAIN}`;
const capabilities = [AgentCapabilities.tools, AgentCapabilities.actions];
const req = createMockReq(capabilities);
mockGetEndpointsConfig.mockResolvedValue(createEndpointsConfig(capabilities));
await loadAgentTools({
req,
res: {},
agent: { id: 'agent_hyphen', tools: [hyphenTool, underscoreTool] },
definitionsOnly: false,
});
expect(mockCreateActionTool).toHaveBeenCalledTimes(2);
const callsByName = new Map(mockCreateActionTool.mock.calls.map((c) => [c[0].name, c[0]]));
expect(callsByName.has(hyphenTool)).toBe(true);
expect(callsByName.has(underscoreTool)).toBe(true);
expect(callsByName.get(hyphenTool).requestBuilder.path).toBe('/foo-bar');
expect(callsByName.get(underscoreTool).requestBuilder.path).toBe('/foo_bar');
// Critical: the two must resolve to distinct builders. If the
// operationId half of the key is normalized, both collapse to
// the same map slot and one silently overwrites the other.
expect(callsByName.get(hyphenTool).requestBuilder).not.toBe(
callsByName.get(underscoreTool).requestBuilder,
);
});
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');
});
});
});