mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-31 08:56:48 +00:00
🪺 fix: Heal MCP HITL Aliases Through Nested Subagents (#15258)
This commit is contained in:
Danny Avila
GitHub
parent
52fa88fc54
commit
76762a20c4
5 changed files with 299 additions and 36 deletions
|
|
@ -2977,6 +2977,49 @@ describe('HITL wiring is gated on hitlCapable', () => {
|
|||
const config = await runAndGetConfig({});
|
||||
expect(config).not.toHaveProperty('humanInTheLoop');
|
||||
});
|
||||
|
||||
it('heals aliases discovered when a lazy subagent resolves', async () => {
|
||||
const alias = { name: 'delete_mcp_acme', aliasName: 'acme_delete_mcp_acme' };
|
||||
const resolvedChild = makeAgent({ id: 'lazy-child', mcpToolAliases: [alias] });
|
||||
const lazyChild = {
|
||||
...makeAgent({ id: 'lazy-child' }),
|
||||
configId: 'lazy-child:v1',
|
||||
resolve: jest.fn().mockResolvedValue(resolvedChild),
|
||||
};
|
||||
const parent = makeAgent({
|
||||
subagents: { enabled: true, allowSelf: false },
|
||||
lazySubagentConfigs: [lazyChild],
|
||||
});
|
||||
const appConfig = {
|
||||
...hitlAppConfig,
|
||||
endpoints: {
|
||||
[EModelEndpoint.agents]: {
|
||||
toolApproval: { enabled: true, mode: 'bypass', deny: [alias.aliasName] },
|
||||
},
|
||||
},
|
||||
} as unknown as AppConfig;
|
||||
|
||||
await createRun({
|
||||
agents: [parent] as never,
|
||||
signal: new AbortController().signal,
|
||||
appConfig,
|
||||
streaming: true,
|
||||
streamUsage: true,
|
||||
hitlCapable: true,
|
||||
});
|
||||
const config = (Run.create as jest.Mock).mock.calls[0][0] as Record<string, unknown>;
|
||||
const hooks = config.hooks as { getMatchers: (event: string) => unknown[] };
|
||||
const lazyConfig = (
|
||||
(config.graphConfig as { agents: Array<Record<string, unknown>> }).agents[0]
|
||||
.subagentConfigs as Array<Record<string, unknown>>
|
||||
).find((entry) => entry.configId === lazyChild.configId);
|
||||
|
||||
expect(hooks.getMatchers('PreToolUse')).toHaveLength(1);
|
||||
await (lazyConfig?.resolveAgentInputs as (context: never) => Promise<unknown>)({
|
||||
signal: new AbortController().signal,
|
||||
} as never);
|
||||
expect(hooks.getMatchers('PreToolUse')).toHaveLength(1);
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
|
|
|
|||
|
|
@ -27,6 +27,32 @@ describe('buildHITLRunWiring', () => {
|
|||
const wiring = buildHITLRunWiring({ enabled: true });
|
||||
expect(wiring?.hooks.getMatchers('PreToolUse')).toHaveLength(1);
|
||||
});
|
||||
|
||||
test('updates the baseline policy for aliases learned after run creation', async () => {
|
||||
const wiring = buildHITLRunWiring({ enabled: true, mode: 'dontAsk', allow: ['legacy_tool'] });
|
||||
const policyHook = wiring?.hooks.getMatchers('PreToolUse')[0].hooks[0];
|
||||
expect(
|
||||
await policyHook?.({ toolName: 'current_tool' } as never, new AbortController().signal),
|
||||
).toEqual({ decision: 'deny' });
|
||||
|
||||
wiring?.addMCPToolAliases([{ name: 'current_tool', aliasName: 'legacy_tool' }], {
|
||||
enabled: true,
|
||||
mode: 'dontAsk',
|
||||
allow: ['legacy_tool', 'current_tool'],
|
||||
});
|
||||
expect(
|
||||
await policyHook?.({ toolName: 'current_tool' } as never, new AbortController().signal),
|
||||
).toEqual({ decision: 'allow' });
|
||||
expect(wiring?.hooks.getMatchers('PreToolUse')).toHaveLength(1);
|
||||
|
||||
// Re-resolving the same descriptor must not grow the run-wide hook registry.
|
||||
wiring?.addMCPToolAliases([{ name: 'current_tool', aliasName: 'legacy_tool' }], {
|
||||
enabled: true,
|
||||
mode: 'dontAsk',
|
||||
allow: ['legacy_tool', 'current_tool'],
|
||||
});
|
||||
expect(wiring?.hooks.getMatchers('PreToolUse')).toHaveLength(1);
|
||||
});
|
||||
});
|
||||
|
||||
describe('buildHITLRunWiring host-hook composition', () => {
|
||||
|
|
@ -61,4 +87,24 @@ describe('buildHITLRunWiring host-hook composition', () => {
|
|||
expect.objectContaining({ userId: 'u1', conversationId: 'c1' }),
|
||||
);
|
||||
});
|
||||
|
||||
test('matches lazy aliases without changing host-hook ordering', async () => {
|
||||
const hook = jest.fn(async () => ({ decision: 'deny' as const }));
|
||||
registerToolApprovalHook(() => hook, {
|
||||
matcher: '^legacy_tool$',
|
||||
});
|
||||
const wiring = buildHITLRunWiring({ enabled: true, mode: 'bypass' });
|
||||
const hostHook = wiring?.hooks.getMatchers('PreToolUse')[1].hooks[0];
|
||||
await hostHook?.({ toolName: 'current_tool' } as never, new AbortController().signal);
|
||||
expect(hook).not.toHaveBeenCalled();
|
||||
|
||||
wiring?.addMCPToolAliases([{ name: 'current_tool', aliasName: 'legacy_tool' }], {
|
||||
enabled: true,
|
||||
mode: 'bypass',
|
||||
});
|
||||
await hostHook?.({ toolName: 'current_tool' } as never, new AbortController().signal);
|
||||
expect(hook).toHaveBeenCalledTimes(1);
|
||||
// Baseline policy + host matcher; plugins registered later remain last.
|
||||
expect(wiring?.hooks.getMatchers('PreToolUse')).toHaveLength(2);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -2,12 +2,7 @@ import { HookRegistry, createToolPolicyHook } from '@librechat/agents';
|
|||
import type { TToolApprovalPolicy } from 'librechat-data-provider';
|
||||
import type { MCPToolAlias } from '~/tools/classification';
|
||||
import type { ToolApprovalHookContext } from './hooks';
|
||||
import {
|
||||
isHITLEnabled,
|
||||
mapToolApprovalPolicy,
|
||||
collectAliasMatcherNames,
|
||||
buildAliasMatcherPattern,
|
||||
} from './policy';
|
||||
import { isHITLEnabled, mapToolApprovalPolicy } from './policy';
|
||||
import { buildToolApprovalHooks } from './hooks';
|
||||
|
||||
/**
|
||||
|
|
@ -21,6 +16,11 @@ import { buildToolApprovalHooks } from './hooks';
|
|||
export interface HITLRunWiring {
|
||||
humanInTheLoop: { enabled: true };
|
||||
hooks: HookRegistry;
|
||||
/** Adds aliases discovered while a lazy subagent resolves. */
|
||||
addMCPToolAliases: (
|
||||
aliases: readonly MCPToolAlias[],
|
||||
policy: TToolApprovalPolicy | undefined,
|
||||
) => void;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -46,34 +46,71 @@ export function buildHITLRunWiring(
|
|||
}
|
||||
|
||||
const registry = new HookRegistry();
|
||||
let activePolicy = policy;
|
||||
const aliases = [...mcpToolAliases];
|
||||
const registeredAliases = new Set(
|
||||
aliases.map(({ name, aliasName }) => `${name}\u0000${aliasName}`),
|
||||
);
|
||||
// Static config-driven policy (mode/allow/deny/ask) — the baseline.
|
||||
registry.register('PreToolUse', {
|
||||
hooks: [createToolPolicyHook(mapToolApprovalPolicy(policy) ?? {})],
|
||||
hooks: [
|
||||
async (input, signal) =>
|
||||
createToolPolicyHook(mapToolApprovalPolicy(activePolicy) ?? {})(input, signal),
|
||||
],
|
||||
});
|
||||
|
||||
// Host-registered programmatic hooks — context-aware, layered after the baseline so their
|
||||
// `updatedInput` / `allowedDecisions` win the SDK's last-writer-wins precedence. Each can
|
||||
// carry its own tool-name matcher; the SDK still folds decisions deny > ask > allow.
|
||||
for (const { hook, matcher } of buildToolApprovalHooks(context)) {
|
||||
registry.register(
|
||||
'PreToolUse',
|
||||
matcher ? { pattern: matcher, hooks: [hook] } : { hooks: [hook] },
|
||||
);
|
||||
/** A matcher written against a tool's OTHER key spelling (pre-strip or
|
||||
* current) would silently never fire for the renamed instance, skipping
|
||||
* its argument/user/tenant-specific deny or ask. The SAME hook is
|
||||
* registered again under an exact-name pattern for those aliased names
|
||||
* — a separate entry keeps the admin's regex semantics and the SDK's
|
||||
* pattern-length cap intact, and the name sets are disjoint so the hook
|
||||
* never fires twice for one call. */
|
||||
const aliasNames = matcher ? collectAliasMatcherNames(matcher, mcpToolAliases) : [];
|
||||
if (aliasNames.length > 0) {
|
||||
registry.register('PreToolUse', {
|
||||
pattern: buildAliasMatcherPattern(aliasNames),
|
||||
hooks: [hook],
|
||||
});
|
||||
// Host-registered programmatic hooks — context-aware, layered after the static-policy hook.
|
||||
const programmaticHooks = buildToolApprovalHooks(context);
|
||||
for (const { hook, matcher } of programmaticHooks) {
|
||||
if (matcher == null) {
|
||||
registry.register('PreToolUse', { hooks: [hook] });
|
||||
continue;
|
||||
}
|
||||
registry.register('PreToolUse', {
|
||||
hooks: [
|
||||
async (input, signal) => {
|
||||
let regex: RegExp;
|
||||
try {
|
||||
regex = new RegExp(matcher);
|
||||
} catch {
|
||||
return {};
|
||||
}
|
||||
regex.lastIndex = 0;
|
||||
if (regex.test(input.toolName)) {
|
||||
return hook(input, signal);
|
||||
}
|
||||
for (const { name, aliasName } of aliases) {
|
||||
if (name !== input.toolName) {
|
||||
continue;
|
||||
}
|
||||
regex.lastIndex = 0;
|
||||
if (regex.test(aliasName)) {
|
||||
return hook(input, signal);
|
||||
}
|
||||
}
|
||||
return {};
|
||||
},
|
||||
],
|
||||
});
|
||||
}
|
||||
|
||||
return { humanInTheLoop: { enabled: true }, hooks: registry };
|
||||
return {
|
||||
humanInTheLoop: { enabled: true },
|
||||
hooks: registry,
|
||||
addMCPToolAliases(newAliasCandidates, updatedPolicy) {
|
||||
const newAliases = newAliasCandidates.filter(({ name, aliasName }) => {
|
||||
const key = `${name}\u0000${aliasName}`;
|
||||
if (registeredAliases.has(key)) {
|
||||
return false;
|
||||
}
|
||||
registeredAliases.add(key);
|
||||
return true;
|
||||
});
|
||||
if (newAliases.length === 0) {
|
||||
return;
|
||||
}
|
||||
aliases.push(...newAliases);
|
||||
activePolicy = updatedPolicy;
|
||||
},
|
||||
};
|
||||
}
|
||||
|
|
|
|||
|
|
@ -8,6 +8,7 @@ import {
|
|||
isDeepSeekReasoningProvider,
|
||||
shouldReplayReasoningContent,
|
||||
anyAgentReplaysReasoningContent,
|
||||
collectRunMCPToolAliases,
|
||||
} from './run';
|
||||
|
||||
describe('getRunDiscoveredTools', () => {
|
||||
|
|
@ -409,3 +410,64 @@ describe('anyAgentReplaysReasoningContent', () => {
|
|||
expect(anyAgentReplaysReasoningContent([x])).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe('collectRunMCPToolAliases', () => {
|
||||
const alias = { name: 'delete_mcp_acme', aliasName: 'acme_delete_mcp_acme' };
|
||||
|
||||
it('collects and deduplicates aliases from explicit and graph subagents', () => {
|
||||
const root = {
|
||||
id: 'root',
|
||||
subagentAgentConfigs: [
|
||||
{
|
||||
id: 'explicit',
|
||||
mcpToolAliases: [alias],
|
||||
},
|
||||
],
|
||||
subagentGraphConfigs: [
|
||||
{
|
||||
memberConfigs: [
|
||||
{
|
||||
id: 'graph-member',
|
||||
mcpToolAliases: [alias, { name: 'read_mcp_acme', aliasName: 'acme_read_mcp_acme' }],
|
||||
},
|
||||
],
|
||||
},
|
||||
],
|
||||
};
|
||||
|
||||
expect(collectRunMCPToolAliases([root] as never)).toEqual([
|
||||
alias,
|
||||
{ name: 'read_mcp_acme', aliasName: 'acme_read_mcp_acme' },
|
||||
]);
|
||||
});
|
||||
|
||||
it('is cycle-safe across nested subagents', () => {
|
||||
const root: {
|
||||
id: string;
|
||||
mcpToolAliases: (typeof alias)[];
|
||||
subagentAgentConfigs?: unknown[];
|
||||
} = {
|
||||
id: 'root',
|
||||
mcpToolAliases: [alias],
|
||||
};
|
||||
const child = { id: 'child', subagentAgentConfigs: [root] };
|
||||
root.subagentAgentConfigs = [child];
|
||||
|
||||
expect(collectRunMCPToolAliases([root] as never)).toEqual([alias]);
|
||||
});
|
||||
|
||||
it('collects aliases from a graph member duplicated by a lazy descriptor', () => {
|
||||
const graphAlias = { name: 'write_mcp_acme', aliasName: 'acme_write_mcp_acme' };
|
||||
const root = {
|
||||
id: 'root',
|
||||
lazySubagentConfigs: [{ id: 'shared-agent' }],
|
||||
subagentGraphConfigs: [
|
||||
{
|
||||
memberConfigs: [{ id: 'shared-agent', mcpToolAliases: [graphAlias] }],
|
||||
},
|
||||
],
|
||||
};
|
||||
|
||||
expect(collectRunMCPToolAliases([root] as never)).toEqual([graphAlias]);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -458,6 +458,7 @@ type LazySubagentAgent = Pick<
|
|||
| 'statefulCodeEnvironment'
|
||||
| 'codeSessionKey'
|
||||
| 'includeReasoningHistory'
|
||||
| 'mcpToolAliases'
|
||||
> & {
|
||||
configId: string;
|
||||
subagentAgentConfigs?: RunAgent[];
|
||||
|
|
@ -478,6 +479,7 @@ type SubagentTreeNode = Pick<
|
|||
| 'statefulCodeEnvironment'
|
||||
| 'codeSessionKey'
|
||||
| 'includeReasoningHistory'
|
||||
| 'mcpToolAliases'
|
||||
> & {
|
||||
subagentAgentConfigs?: SubagentTreeNode[];
|
||||
lazySubagentConfigs?: SubagentTreeNode[];
|
||||
|
|
@ -902,6 +904,7 @@ function createLazySubagentConfig(
|
|||
ancestors: Set<string>,
|
||||
depth: number,
|
||||
prebuiltGraphInputs?: ReadonlyMap<string, AgentInputs>,
|
||||
onResolvedAgent?: (agent: RunAgent) => void,
|
||||
): SubagentConfig {
|
||||
return {
|
||||
type: child.id,
|
||||
|
|
@ -920,6 +923,7 @@ function createLazySubagentConfig(
|
|||
if (context.signal.aborted) {
|
||||
throw context.signal.reason ?? new Error('Subagent resolution was aborted.');
|
||||
}
|
||||
onResolvedAgent?.(resolvedChild);
|
||||
const childInputs = buildIsolatedAgentInputs(resolvedChild, toInput);
|
||||
const resolutionState: SubagentBuildState = {
|
||||
configCount: 1,
|
||||
|
|
@ -934,6 +938,8 @@ function createLazySubagentConfig(
|
|||
ancestors,
|
||||
depth,
|
||||
prebuiltGraphInputs,
|
||||
false,
|
||||
onResolvedAgent,
|
||||
);
|
||||
if (grandchildConfigs.length > 0) {
|
||||
childInputs.subagentConfigs = grandchildConfigs;
|
||||
|
|
@ -978,6 +984,44 @@ function enqueueSubagentChildren(
|
|||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Collect MCP key-spelling aliases from every eagerly known agent in the run.
|
||||
* Lazy descriptors are revisited when they resolve, because initializing MCP
|
||||
* tools solely to discover aliases would defeat lazy loading.
|
||||
*/
|
||||
export function collectRunMCPToolAliases(
|
||||
agents: Array<RunAgent | SubagentTreeNode | null | undefined>,
|
||||
): MCPToolAlias[] {
|
||||
const aliases: MCPToolAlias[] = [];
|
||||
const seenAliases = new Set<string>();
|
||||
const visited = new Set<string>();
|
||||
const pending: Array<RunAgent | SubagentTreeNode | null | undefined> = [...agents];
|
||||
|
||||
for (let index = 0; index < pending.length; index++) {
|
||||
const agent = pending[index];
|
||||
if (agent == null) {
|
||||
continue;
|
||||
}
|
||||
for (const alias of agent.mcpToolAliases ?? []) {
|
||||
const key = `${alias.name}\u0000${alias.aliasName}`;
|
||||
if (!seenAliases.has(key)) {
|
||||
seenAliases.add(key);
|
||||
aliases.push(alias);
|
||||
}
|
||||
}
|
||||
if (visited.has(agent.id)) {
|
||||
// The same saved agent can appear as both a lazy descriptor and a
|
||||
// pre-initialized graph member. Keep traversing each representation so
|
||||
// its unique children stay reachable, but avoid descending forever.
|
||||
enqueueSubagentChildren(agent, pending, visited);
|
||||
continue;
|
||||
}
|
||||
visited.add(agent.id);
|
||||
enqueueSubagentChildren(agent, pending, visited);
|
||||
}
|
||||
return aliases;
|
||||
}
|
||||
|
||||
/**
|
||||
* Recursive any-true check across the agent tree: returns `true` if this
|
||||
* agent or any subagent (transitively) has the per-agent codeenv gate
|
||||
|
|
@ -1133,6 +1177,7 @@ function buildSubagentConfigs(
|
|||
depth = 0,
|
||||
prebuiltGraphInputs?: ReadonlyMap<string, AgentInputs>,
|
||||
detachedTasksEnabled = false,
|
||||
onResolvedAgent?: (agent: RunAgent) => void,
|
||||
): SubagentConfigEntry[] {
|
||||
if (!agent.subagents?.enabled) {
|
||||
return [];
|
||||
|
|
@ -1226,6 +1271,8 @@ function buildSubagentConfigs(
|
|||
nextAncestors,
|
||||
childDepth,
|
||||
prebuiltGraphInputs,
|
||||
detachedTasksEnabled,
|
||||
onResolvedAgent,
|
||||
);
|
||||
if (grandchildConfigs.length > 0) {
|
||||
childInputs.subagentConfigs = grandchildConfigs;
|
||||
|
|
@ -1259,6 +1306,7 @@ function buildSubagentConfigs(
|
|||
nextAncestors,
|
||||
childDepth,
|
||||
prebuiltGraphInputs,
|
||||
onResolvedAgent,
|
||||
),
|
||||
);
|
||||
}
|
||||
|
|
@ -1694,6 +1742,9 @@ export async function createRun({
|
|||
|
||||
const agentsEndpointConfig = appConfig?.endpoints?.[EModelEndpoint.agents];
|
||||
|
||||
// Assigned after the run-wide HITL registry is built. Lazy descriptors
|
||||
// capture this indirection now and report their aliases when they resolve.
|
||||
let registerResolvedMCPToolAliases: (agent: RunAgent) => void = () => undefined;
|
||||
const agentInputs: AgentInputs[] = [];
|
||||
const subagentBuildState: SubagentBuildState = {
|
||||
configCount: 0,
|
||||
|
|
@ -1732,6 +1783,7 @@ export async function createRun({
|
|||
0,
|
||||
prebuiltGraphInputs,
|
||||
subagentTasks != null,
|
||||
(resolvedAgent) => registerResolvedMCPToolAliases(resolvedAgent),
|
||||
);
|
||||
if (subagentConfigs.length > 0) {
|
||||
agentInput.subagentConfigs = subagentConfigs;
|
||||
|
|
@ -1800,9 +1852,18 @@ export async function createRun({
|
|||
// would pause with no approval surface or resume endpoint, and the route would emit a
|
||||
// normal final response / `[DONE]` with the tool call dangling. Only AgentClient (chat +
|
||||
// resume) passes `hitlCapable`; without it the run is identical to the no-HITL path.
|
||||
/** Both-direction key-spelling aliases collected at tool classification —
|
||||
* identical in instance and event-driven loading modes. */
|
||||
const mcpToolAliases = agents.flatMap((agent) => agent.mcpToolAliases ?? []);
|
||||
/** Both-direction key-spelling aliases collected from every eagerly known
|
||||
* agent, including explicit and graph subagents. Lazy subagents report
|
||||
* theirs through `registerResolvedMCPToolAliases` below. */
|
||||
const mcpToolAliases = collectRunMCPToolAliases(agents);
|
||||
const mcpToolAliasKeys = new Set(
|
||||
mcpToolAliases.map(({ name, aliasName }) => `${name}\u0000${aliasName}`),
|
||||
);
|
||||
const effectiveToolApprovalPolicy = () =>
|
||||
exemptAskUserQuestionFromApproval(
|
||||
healToolApprovalPolicy(toolApprovalPolicy, mcpToolAliases),
|
||||
ASK_USER_QUESTION_TOOL_NAME,
|
||||
);
|
||||
const hitl = hitlCapable
|
||||
? buildHITLRunWiring(
|
||||
// The ask tool is exempt from the approval prompt (unless explicitly
|
||||
|
|
@ -1812,10 +1873,7 @@ export async function createRun({
|
|||
// admin globs written for pre-strip upstream names keep applying (a
|
||||
// non-matching deny would fail OPEN), and rules written against
|
||||
// current catalog names reach legacy-named instances.
|
||||
exemptAskUserQuestionFromApproval(
|
||||
healToolApprovalPolicy(toolApprovalPolicy, mcpToolAliases),
|
||||
ASK_USER_QUESTION_TOOL_NAME,
|
||||
),
|
||||
effectiveToolApprovalPolicy(),
|
||||
{
|
||||
userId: user?.id,
|
||||
conversationId: requestBody?.conversationId,
|
||||
|
|
@ -1825,6 +1883,23 @@ export async function createRun({
|
|||
mcpToolAliases,
|
||||
)
|
||||
: undefined;
|
||||
registerResolvedMCPToolAliases = (resolvedAgent) => {
|
||||
const discoveredAliases = collectRunMCPToolAliases([resolvedAgent]).filter(
|
||||
({ name, aliasName }) => {
|
||||
const key = `${name}\u0000${aliasName}`;
|
||||
if (mcpToolAliasKeys.has(key)) {
|
||||
return false;
|
||||
}
|
||||
mcpToolAliasKeys.add(key);
|
||||
return true;
|
||||
},
|
||||
);
|
||||
if (discoveredAliases.length === 0) {
|
||||
return;
|
||||
}
|
||||
mcpToolAliases.push(...discoveredAliases);
|
||||
hitl?.addMCPToolAliases(discoveredAliases, effectiveToolApprovalPolicy());
|
||||
};
|
||||
/**
|
||||
* The `ask_user_question` tool pauses via LangGraph `interrupt()` from inside its own
|
||||
* body, which needs only a durable checkpointer — NOT the tool-approval policy
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue