diff --git a/packages/api/src/agents/__tests__/run-summarization.test.ts b/packages/api/src/agents/__tests__/run-summarization.test.ts index 3b7e781e1c9..dfb662bbae9 100644 --- a/packages/api/src/agents/__tests__/run-summarization.test.ts +++ b/packages/api/src/agents/__tests__/run-summarization.test.ts @@ -1,4 +1,6 @@ +import { z } from 'zod'; import { encryptV3, logger } from '@librechat/data-schemas'; +import { DynamicStructuredTool } from '@langchain/core/tools'; import { HumanMessage, AIMessage } from '@langchain/core/messages'; import { CallbackManager } from '@langchain/core/callbacks/manager'; import { @@ -14,6 +16,7 @@ import type { BaseMessage } from '@langchain/core/messages'; import type { OpenAI } from 'openai'; import type { ModelBoundChatModelCallback } from '~/middleware/modelBoundContent'; import type { OpenAIConfiguration, AzureOptions } from '~/types'; +import { clearToolApprovalHooks, registerToolApprovalHook } from '~/agents/hitl/hooks'; import { createRun, isAskUserQuestionAdminDisabled } from '~/agents/run'; import { initializeOpenAI } from '~/endpoints/openai/initialize'; import { getOpenAIConfig } from '~/endpoints/openai/config'; @@ -93,7 +96,15 @@ jest.mock('~/agents/checkpointer', () => ({ import { ChatOpenAI } from '@librechat/agents/llm/openai'; import { ChatOpenRouter } from '@librechat/agents/llm/openrouter'; -import { Run, Providers, buildChildInputs, InMemorySubagentTaskStore } from '@librechat/agents'; +import { + Run, + Providers, + HookRegistry, + ToolNode, + buildChildInputs, + InMemorySubagentTaskStore, + executeHooks, +} from '@librechat/agents'; /** Minimal RunAgent factory */ function makeAgent( @@ -4407,10 +4418,8 @@ describe('createRun deferred-tool replay (HITL resume)', () => { // --------------------------------------------------------------------------- // Suite: HITL wiring gated to resumable callers (Codex J3) // -// The tool-approval wiring (humanInTheLoop switch + PreToolUse hook) must engage ONLY for -// callers that implement the pause/resume lifecycle. AgentClient passes hitlCapable: true; -// the OpenAI-compatible + Responses controllers don't, so an approval-gated tool can't -// pause on a route with no approval surface or resume endpoint. +// Tool-approval policy must apply to every caller, while only an interactive +// caller may pause for an 'ask' decision. API-key endpoints have no resume surface. // --------------------------------------------------------------------------- describe('HITL wiring is gated on hitlCapable', () => { const hitlAppConfig = { @@ -4441,15 +4450,119 @@ describe('HITL wiring is gated on hitlCapable', () => { expect(config.hooks).toBeDefined(); }); - it('does NOT attach HITL for a non-resumable caller even when approval is enabled', async () => { - const config = await runAndGetConfig({ hitlCapable: false }); + it.each([ + [ + { enabled: true, mode: 'bypass', deny: ['delete_*'], allow: ['delete_file'] }, + 'delete_file', + 'deny', + ], + [{ enabled: true, mode: 'dontAsk', allow: ['read_*'] }, 'read_file', 'allow'], + [{ enabled: true, mode: 'dontAsk', allow: ['read_*'] }, 'write_file', 'deny'], + [{ enabled: true, mode: 'default' }, 'write_file', 'ask'], + ] as const)('evaluates headless tool policy for %s on %s', async (policy, toolName, decision) => { + const appConfig = { + ...hitlAppConfig, + endpoints: { [EModelEndpoint.agents]: { toolApproval: policy } }, + } as unknown as AppConfig; + const config = await runAndGetConfig({ hitlCapable: false, appConfig }); expect(config).not.toHaveProperty('humanInTheLoop'); - expect(config.graphConfig).toBeDefined(); - // No checkpointer either — the run is identical to the no-HITL path. expect( (config.graphConfig as { compileOptions?: { checkpointer?: unknown } }).compileOptions ?.checkpointer, ).toBeUndefined(); + const result = await executeHooks({ + registry: config.hooks as HookRegistry, + input: { + hook_event_name: 'PreToolUse', + runId: 'headless-test', + toolName, + toolInput: {}, + toolUseId: 'call-1', + }, + matchQuery: toolName, + }); + expect(result.decision).toBe(decision); + }); + + it.each([ + { policy: { enabled: true, mode: 'bypass', deny: ['delete_record'] }, name: 'delete_record' }, + { policy: { enabled: true, mode: 'default' }, name: 'read_record' }, + ] as const)( + 'blocks a real SDK direct tool before execution for $name', + async ({ policy, name }) => { + const config = await runAndGetConfig({ + appConfig: { + ...hitlAppConfig, + endpoints: { [EModelEndpoint.agents]: { toolApproval: policy } }, + } as unknown as AppConfig, + }); + const body = jest.fn(async () => 'executed'); + const tool = new DynamicStructuredTool({ + name, + description: 'A test-only tool', + schema: z.object({}), + func: body, + }); + const node = new ToolNode({ + tools: [tool], + agentId: 'agent_1', + hookRegistry: config.hooks as HookRegistry, + }); + const result = await node.invoke( + { + messages: [ + new AIMessage({ content: '', tool_calls: [{ id: 'call-1', name, args: {} }] }), + ], + }, + { configurable: { run_id: 'headless-test', thread_id: 'thread-1' } }, + ); + expect(body).not.toHaveBeenCalled(); + expect(JSON.stringify(result)).toContain('Blocked:'); + }, + ); + + it('registers trusted per-run hooks on headless calls without prompting', async () => { + const factory = jest.fn(() => async () => ({ decision: 'deny' as const })); + const unregister = registerToolApprovalHook(factory, { matcher: '^write_file$' }); + try { + const config = await runAndGetConfig({ + user: { id: 'user-1' }, + appConfig: { + ...hitlAppConfig, + endpoints: { + [EModelEndpoint.agents]: { toolApproval: { enabled: true, mode: 'bypass' } }, + }, + } as AppConfig, + }); + expect(factory).toHaveBeenCalledWith(expect.objectContaining({ userId: 'user-1' })); + expect(config).not.toHaveProperty('humanInTheLoop'); + const result = await executeHooks({ + registry: config.hooks as HookRegistry, + input: { + hook_event_name: 'PreToolUse', + runId: 'headless-test', + toolName: 'write_file', + toolInput: {}, + toolUseId: 'call-1', + }, + matchQuery: 'write_file', + }); + expect(result.decision).toBe('deny'); + } finally { + unregister(); + clearToolApprovalHooks(); + } + }); + + it('keeps approval fully off when the endpoint disables it', async () => { + const config = await runAndGetConfig({ + appConfig: { + ...hitlAppConfig, + endpoints: { [EModelEndpoint.agents]: { toolApproval: { enabled: false, deny: ['*'] } } }, + } as AppConfig, + }); + expect((config.hooks as HookRegistry).hasHookFor('PreToolUse')).toBe(false); + expect(config).not.toHaveProperty('humanInTheLoop'); }); it('defaults to non-HITL when hitlCapable is omitted', async () => { @@ -4457,48 +4570,67 @@ describe('HITL wiring is gated on hitlCapable', () => { 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] }, + it.each([true, false])( + 'heals aliases discovered in lazy subagents (HITL=%s)', + async (hitlCapable) => { + 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; + } 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); - }); + await createRun({ + agents: [parent] as never, + signal: new AbortController().signal, + appConfig, + streaming: true, + streamUsage: true, + hitlCapable, + }); + 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); + + const decisionForAlias = async () => + ( + await executeHooks({ + registry: config.hooks as HookRegistry, + input: { + hook_event_name: 'PreToolUse', + runId: 'alias-test', + toolName: alias.name, + toolInput: {}, + toolUseId: 'call-1', + }, + matchQuery: alias.name, + }) + ).decision; + expect(hooks.getMatchers('PreToolUse')).toHaveLength(1); + expect(await decisionForAlias()).toBe('allow'); + await (lazyConfig?.resolveAgentInputs as (context: never) => Promise)({ + signal: new AbortController().signal, + } as never); + expect(hooks.getMatchers('PreToolUse')).toHaveLength(1); + expect(await decisionForAlias()).toBe('deny'); + }, + ); }); // --------------------------------------------------------------------------- diff --git a/packages/api/src/agents/hitl/hooks.ts b/packages/api/src/agents/hitl/hooks.ts index ba74f7a1f72..dc59296d0b6 100644 --- a/packages/api/src/agents/hitl/hooks.ts +++ b/packages/api/src/agents/hitl/hooks.ts @@ -69,10 +69,10 @@ const registeredHooks: RegisteredHook[] = []; * Register a programmatic tool-approval hook (process-wide). Call once at startup. Returns an * unregister function that removes exactly this registration. * - * Inert unless tool approval is enabled AND the caller is HITL-capable — hooks only run inside - * the `PreToolUse` fold of an HITL run (see {@link buildToolApprovalHooks} / - * `buildHITLRunWiring`). They compose with, and can only tighten, the static - * `endpoints.agents.toolApproval` policy. + * Inert unless tool approval is enabled. Hooks run in the `PreToolUse` fold of + * both interactive and headless runs (see {@link buildToolApprovalHooks} / + * `buildHITLRunWiring`). Headless `ask` decisions are denied without pausing. + * They compose with, and can only tighten, the static policy. * * @param factory Builds the per-run hook from its context; return `undefined` to opt out. * @param options.matcher Optional regex string matched against the tool name — omit to run for diff --git a/packages/api/src/agents/hitl/runtime.ts b/packages/api/src/agents/hitl/runtime.ts index 60dba4785f8..3253f61f33b 100644 --- a/packages/api/src/agents/hitl/runtime.ts +++ b/packages/api/src/agents/hitl/runtime.ts @@ -25,10 +25,10 @@ export function buildToolApprovalExecutionConfig( /** * The HITL fragment spread onto a `RunConfig` when tool approval is enabled. * - * Kept as one object so the run seam attaches the opt-in switch and the policy - * hook together — they're meaningless apart. The checkpointer is resolved - * separately (it's an async, process-wide singleton) and merged into - * `graphConfig.compileOptions` at the call site. + * The policy hooks apply to every caller. Only callers that support pause/resume + * also attach the `humanInTheLoop` switch and the durable checkpointer. + * The checkpointer is resolved separately (it's an async, process-wide singleton) + * and merged into `graphConfig.compileOptions` at the call site. */ export interface HITLRunWiring { humanInTheLoop: { enabled: true }; @@ -41,9 +41,9 @@ export interface HITLRunWiring { } /** - * Assemble the run-level HITL wiring for a tool-approval policy, or `undefined` - * when HITL is disabled (the default) — in which case the run attaches nothing - * and behaves exactly as it did before this feature. + * Assemble tool-approval policy hooks and optional interactive HITL wiring, or + * `undefined` when the policy is disabled. Non-resumable callers attach only + * `hooks`; the SDK denies `ask` rather than pausing without a resume surface. * * The returned `hooks` registry carries the static-config `PreToolUse` policy hook built * from {@link mapToolApprovalPolicy} (an enabled policy with no allow/deny/ask lists falls diff --git a/packages/api/src/agents/run.ts b/packages/api/src/agents/run.ts index 44f98bc562b..a420b80d3cf 100644 --- a/packages/api/src/agents/run.ts +++ b/packages/api/src/agents/run.ts @@ -2248,10 +2248,9 @@ export async function createRun({ /** * Whether the caller implements the HITL pause/resume lifecycle (inspects * `run.getInterrupt()`, persists a pending action, exposes a resume route). Gates the - * tool-approval wiring: only AgentClient (chat + resume) sets this. The OpenAI-compatible - * and Responses controllers leave it false, so an approval-gated tool can't pause on a - * route that has no approval surface or resume endpoint (it would otherwise emit a normal - * final response / `[DONE]` with the tool call left unresolved). + * approval pause and checkpointer: only AgentClient (chat + resume) sets this. + * All callers still enforce an enabled tool policy; without this flag the SDK + * blocks `ask` decisions rather than pausing a run with no resume surface. */ hitlCapable?: boolean; /** @@ -2687,12 +2686,10 @@ export async function createRun({ const enableToolOutputReferences = anyAgentHasCodeEnv(agents); /** - * Human-in-the-loop tool approval — OFF by default. When the agents endpoint - * opts in (`toolApproval.enabled`), attach the `PreToolUse` policy hook + the - * `humanInTheLoop` switch, and bind a durable checkpointer so a run that pauses - * for review can be rebuilt and resumed on any worker (see `agents/checkpointer.ts` - * and the resume route). When disabled, nothing attaches and the run is identical - * to before this feature shipped. + * Endpoint tool approval is off by default. An enabled policy installs the + * `PreToolUse` hooks for every run. Only callers with a resume surface also + * enable real HITL interrupts and a durable checkpointer; on headless runs + * the SDK blocks both `deny` and `ask` before executing the tool. */ // Resolve the effective policy through the single seam so BYOM defaults and // future persisted per-agent / per-skill sources do not leak into this call site. @@ -2700,12 +2697,9 @@ export async function createRun({ endpoint: agentsEndpointConfig?.toolApproval, attachedCodeEnvironment: attachedCodeEnvironmentAgentIds.size > 0, }); - // Gate HITL to callers that actually implement the pause/resume lifecycle. The - // OpenAI-compatible + Responses controllers also call createRun/processStream but never - // inspect `run.getInterrupt()` or persist a pending action — so an approval-gated tool - // 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. + // Every caller needs the policy hooks, including API-key ingresses. Only + // AgentClient supports pause/resume; the SDK blocks `ask` without HITL enabled. + // Keep the checkpointer and humanInTheLoop switch exclusive to those callers. /** Both-direction key-spelling aliases collected from every eagerly known * agent, including explicit and graph subagents. Lazy subagents report * theirs through `registerResolvedMCPToolAliases` below. */ @@ -2718,45 +2712,44 @@ export async function createRun({ healToolApprovalPolicy(toolApprovalPolicy, mcpToolAliases), ASK_USER_QUESTION_TOOL_NAME, ); - const hitl = hitlCapable - ? buildHITLRunWiring( - // The ask tool is exempt from the approval prompt (unless explicitly - // listed by the admin) — approving the right to ask a question is a - // pure double-pause; the tool has no side effects to gate. Pattern - // lists are healed against the tools' other key spellings first, so - // 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. - effectiveToolApprovalPolicy(), - { + const approvalWiring = buildHITLRunWiring( + // The ask tool is exempt from the approval prompt (unless explicitly + // listed by the admin) — approving the right to ask a question is a + // pure double-pause; the tool has no side effects to gate. Pattern + // lists are healed against the tools' other key spellings first, so + // 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. + effectiveToolApprovalPolicy(), + { + userId: user?.id, + conversationId: requestBody?.conversationId, + tenantId: tenantId ?? user?.tenantId, + appConfig, + }, + mcpToolAliases, + [ + ...(resolvedToolApprovalHooks ?? + buildToolApprovalHooks({ userId: user?.id, conversationId: requestBody?.conversationId, tenantId: tenantId ?? user?.tenantId, appConfig, - }, - mcpToolAliases, - [ - ...(resolvedToolApprovalHooks ?? - buildToolApprovalHooks({ - userId: user?.id, - conversationId: requestBody?.conversationId, - tenantId: tenantId ?? user?.tenantId, - appConfig, - })), - ...(attachedCodeEnvironmentAgentIds.size > 0 - ? [ - { - hook: createAttachedCodeEnvironmentPolicyHook( - attachedCodeEnvironmentAgentIds, - attachedCodeEnvironmentSettings, - codeApprovalMode, - ), - }, - ] - : []), - ], - ) - : undefined; + })), + ...(attachedCodeEnvironmentAgentIds.size > 0 + ? [ + { + hook: createAttachedCodeEnvironmentPolicyHook( + attachedCodeEnvironmentAgentIds, + attachedCodeEnvironmentSettings, + codeApprovalMode, + ), + }, + ] + : []), + ], + ); + const hitl = hitlCapable ? approvalWiring : undefined; registerResolvedMCPToolAliases = (resolvedAgent) => { if (resolvedAgent.codeExecutionContext?.environmentType === 'attached') { // The admission hook closes over these collections. A lazily resolved agent @@ -2783,7 +2776,7 @@ export async function createRun({ return; } mcpToolAliases.push(...discoveredAliases); - hitl?.addMCPToolAliases(discoveredAliases, effectiveToolApprovalPolicy()); + approvalWiring?.addMCPToolAliases(discoveredAliases, effectiveToolApprovalPolicy()); }; /** * The `ask_user_question` tool pauses via LangGraph `interrupt()` from inside its own @@ -2803,14 +2796,14 @@ export async function createRun({ } /** - * The run's hook registry: the HITL policy hooks (when approval is enabled) + * The run's hook registry: tool policy hooks (when approval is enabled) * plus the steer-drain PostToolBatch hook. Steering registers independently * of the approval policy and requires no checkpointer, but is hard-gated on * SDK support — draining on an SDK that ignores `injectedMessages` would * silently drop the user's words (the steer controller 501s in that case; * this guard is defense in depth). */ - let hooks = hitl?.hooks; + let hooks = approvalWiring?.hooks; if (usesSubagentCompletionWakeups(activeSubagentTasks)) { hooks = hooks ?? new HookRegistry(); hooks.register('PostToolUse', { @@ -2989,9 +2982,8 @@ export async function createRun({ ...(enableToolOutputReferences && { toolOutputReferences: { enabled: true }, }), - // HITL opt-in: the `humanInTheLoop` switch + the PreToolUse policy hook. Spread - // here (not just `compileOptions.checkpointer` above) so an `ask` decision raises - // a real interrupt — without these the run would never pause. Absent when disabled. + // Only resumable callers enable real approval interrupts. The PreToolUse policy + // hook stays in `hooks` for headless callers, where `ask` fails closed. // The steer-drain hook rides the same registry but independently of the approval // policy: a PostToolBatch-only registry keeps the SDK's eager execution fast paths // (it gates on result-altering hooks, not registry presence).