CERBERUS/LibreChat
1
0
Fork 0
mirror of https://github.com/danny-avila/LibreChat.git synced 2026-05-14 00:19:40 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

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

Browse source 
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.
This commit is contained in:
lrreverence 2026-04-11 01:57:13 +08:00
parent 5d1e44a532
commit d643444e88
2 changed files with 177 additions and 34 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -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 (`<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.
*/
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,
});

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

@ -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');
});
});
});