From 76762a20c412e099cf6a0c05ad7f72e53c4b721e Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Wed, 26 Aug 2026 10:03:41 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=AA=BA=20fix:=20Heal=20MCP=20HITL=20Alias?= =?UTF-8?q?es=20Through=20Nested=20Subagents=20(#15258)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../__tests__/run-summarization.test.ts | 43 +++++++++ packages/api/src/agents/hitl/runtime.spec.ts | 46 +++++++++ packages/api/src/agents/hitl/runtime.ts | 95 +++++++++++++------ packages/api/src/agents/run.spec.ts | 62 ++++++++++++ packages/api/src/agents/run.ts | 89 +++++++++++++++-- 5 files changed, 299 insertions(+), 36 deletions(-) diff --git a/packages/api/src/agents/__tests__/run-summarization.test.ts b/packages/api/src/agents/__tests__/run-summarization.test.ts index 7327148dfd..083ef084ba 100644 --- a/packages/api/src/agents/__tests__/run-summarization.test.ts +++ b/packages/api/src/agents/__tests__/run-summarization.test.ts @@ -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; + const hooks = config.hooks as { getMatchers: (event: string) => unknown[] }; + const lazyConfig = ( + (config.graphConfig as { agents: Array> }).agents[0] + .subagentConfigs as Array> + ).find((entry) => entry.configId === lazyChild.configId); + + expect(hooks.getMatchers('PreToolUse')).toHaveLength(1); + await (lazyConfig?.resolveAgentInputs as (context: never) => Promise)({ + signal: new AbortController().signal, + } as never); + expect(hooks.getMatchers('PreToolUse')).toHaveLength(1); + }); }); // --------------------------------------------------------------------------- diff --git a/packages/api/src/agents/hitl/runtime.spec.ts b/packages/api/src/agents/hitl/runtime.spec.ts index 42148eab56..46142fffcc 100644 --- a/packages/api/src/agents/hitl/runtime.spec.ts +++ b/packages/api/src/agents/hitl/runtime.spec.ts @@ -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); + }); }); diff --git a/packages/api/src/agents/hitl/runtime.ts b/packages/api/src/agents/hitl/runtime.ts index 5d96ae05a3..fd35edd7be 100644 --- a/packages/api/src/agents/hitl/runtime.ts +++ b/packages/api/src/agents/hitl/runtime.ts @@ -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; + }, + }; } diff --git a/packages/api/src/agents/run.spec.ts b/packages/api/src/agents/run.spec.ts index dd1c3c48e4..9cb7644b6a 100644 --- a/packages/api/src/agents/run.spec.ts +++ b/packages/api/src/agents/run.spec.ts @@ -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]); + }); +}); diff --git a/packages/api/src/agents/run.ts b/packages/api/src/agents/run.ts index c18a4b387f..dbc18fab69 100644 --- a/packages/api/src/agents/run.ts +++ b/packages/api/src/agents/run.ts @@ -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, depth: number, prebuiltGraphInputs?: ReadonlyMap, + 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, +): MCPToolAlias[] { + const aliases: MCPToolAlias[] = []; + const seenAliases = new Set(); + const visited = new Set(); + const pending: Array = [...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, 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