From e9e435b796b9088b7d03463466e2ed762181580d Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 18 Apr 2026 10:52:11 -0400 Subject: [PATCH 01/17] =?UTF-8?q?=F0=9F=A4=9D=20fix:=20load=20handoff=20su?= =?UTF-8?q?b-agents=20on=20OpenAI-compat=20endpoints=20(#12726)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extracts the BFS discovery + ACL-gated initialization of handoff sub-agents into a shared `discoverConnectedAgents` helper in `@librechat/api` and wires it into the OpenAI-compatible `/v1/chat/completions` and Open Responses `/v1/responses` controllers. These endpoints previously only passed the primary agent config to `createRun` while keeping `primaryConfig.edges` intact, which forced `MultiAgentGraph` into multi-agent mode without loading the referenced sub-agents and caused StateGraph to throw "Found edge ending at unknown node ". The discovery helper also filters orphaned edges (deleted sub-agents or those the caller lacks VIEW permission on), so API users see the same graceful fallback the chat UI already had. --- .../agents/__tests__/openai.spec.js | 21 + .../agents/__tests__/responses.unit.spec.js | 21 + api/server/controllers/agents/openai.js | 111 +++++- api/server/controllers/agents/responses.js | 125 ++++-- .../services/Endpoints/agents/initialize.js | 163 ++------ packages/api/src/agents/discovery.spec.ts | 374 ++++++++++++++++++ packages/api/src/agents/discovery.ts | 273 +++++++++++++ packages/api/src/agents/index.ts | 1 + packages/api/src/agents/validation.ts | 13 +- 9 files changed, 933 insertions(+), 169 deletions(-) create mode 100644 packages/api/src/agents/discovery.spec.ts create mode 100644 packages/api/src/agents/discovery.ts diff --git a/api/server/controllers/agents/__tests__/openai.spec.js b/api/server/controllers/agents/__tests__/openai.spec.js index c959be6cf4d9..f55b798f14a2 100644 --- a/api/server/controllers/agents/__tests__/openai.spec.js +++ b/api/server/controllers/agents/__tests__/openai.spec.js @@ -46,9 +46,11 @@ jest.mock('@librechat/api', () => ({ .fn() .mockReturnValue({ request: { model: 'agent-123', messages: [], stream: false } }), initializeAgent: jest.fn().mockResolvedValue({ + id: 'agent-123', model: 'gpt-4', model_parameters: {}, toolRegistry: {}, + edges: [], }), getBalanceConfig: mockGetBalanceConfig, createErrorResponse: jest.fn(), @@ -72,6 +74,24 @@ jest.mock('@librechat/api', () => ({ resolveRecursionLimit: jest.fn().mockReturnValue(50), createToolExecuteHandler: jest.fn().mockReturnValue({ handle: jest.fn() }), isChatCompletionValidationFailure: jest.fn().mockReturnValue(false), + discoverConnectedAgents: jest.fn().mockResolvedValue({ + agentConfigs: new Map(), + edges: [], + skippedAgentIds: new Set(), + userMCPAuthMap: undefined, + }), +})); + +jest.mock('~/server/controllers/ModelController', () => ({ + getModelsConfig: jest.fn().mockResolvedValue({}), +})); + +jest.mock('~/server/services/Files/permissions', () => ({ + filterFilesByAgentAccess: jest.fn(), +})); + +jest.mock('~/cache', () => ({ + logViolation: jest.fn(), })); jest.mock('~/server/services/ToolService', () => ({ @@ -91,6 +111,7 @@ jest.mock('~/server/controllers/agents/callbacks', () => ({ jest.mock('~/server/services/PermissionService', () => ({ findAccessibleResources: jest.fn().mockResolvedValue([]), + checkPermission: jest.fn().mockResolvedValue(true), })); const mockUpdateBalance = jest.fn().mockResolvedValue({}); diff --git a/api/server/controllers/agents/__tests__/responses.unit.spec.js b/api/server/controllers/agents/__tests__/responses.unit.spec.js index 26f5f5d30bca..1a6fa6053f14 100644 --- a/api/server/controllers/agents/__tests__/responses.unit.spec.js +++ b/api/server/controllers/agents/__tests__/responses.unit.spec.js @@ -43,9 +43,17 @@ jest.mock('@librechat/api', () => ({ buildToolSet: jest.fn().mockReturnValue(new Set()), createSafeUser: jest.fn().mockReturnValue({ id: 'user-123' }), initializeAgent: jest.fn().mockResolvedValue({ + id: 'agent-123', model: 'claude-3', model_parameters: {}, toolRegistry: {}, + edges: [], + }), + discoverConnectedAgents: jest.fn().mockResolvedValue({ + agentConfigs: new Map(), + edges: [], + skippedAgentIds: new Set(), + userMCPAuthMap: undefined, }), getBalanceConfig: mockGetBalanceConfig, getTransactionsConfig: mockGetTransactionsConfig, @@ -121,6 +129,19 @@ jest.mock('~/server/controllers/agents/callbacks', () => { jest.mock('~/server/services/PermissionService', () => ({ findAccessibleResources: jest.fn().mockResolvedValue([]), + checkPermission: jest.fn().mockResolvedValue(true), +})); + +jest.mock('~/server/controllers/ModelController', () => ({ + getModelsConfig: jest.fn().mockResolvedValue({}), +})); + +jest.mock('~/server/services/Files/permissions', () => ({ + filterFilesByAgentAccess: jest.fn(), +})); + +jest.mock('~/cache', () => ({ + logViolation: jest.fn(), })); const mockUpdateBalance = jest.fn().mockResolvedValue({}); diff --git a/api/server/controllers/agents/openai.js b/api/server/controllers/agents/openai.js index 9fa3af82c303..12e47672f414 100644 --- a/api/server/controllers/agents/openai.js +++ b/api/server/controllers/agents/openai.js @@ -21,6 +21,7 @@ const { createOpenAIStreamTracker, createOpenAIContentAggregator, isChatCompletionValidationFailure, + discoverConnectedAgents, } = require('@librechat/api'); const { buildSummarizationHandlers, @@ -29,7 +30,10 @@ const { agentLogHandlerObj, } = require('~/server/controllers/agents/callbacks'); const { loadAgentTools, loadToolsForExecution } = require('~/server/services/ToolService'); -const { findAccessibleResources } = require('~/server/services/PermissionService'); +const { findAccessibleResources, checkPermission } = require('~/server/services/PermissionService'); +const { getModelsConfig } = require('~/server/controllers/ModelController'); +const { filterFilesByAgentAccess } = require('~/server/services/Files/permissions'); +const { logViolation } = require('~/cache'); const db = require('~/models'); /** @@ -207,6 +211,19 @@ const OpenAIChatCompletionController = async (req, res) => { model_parameters: agent.model_parameters ?? {}, }; + const dbMethods = { + getConvoFiles: db.getConvoFiles, + getFiles: db.getFiles, + getUserKey: db.getUserKey, + getMessages: db.getMessages, + updateFilesUsage: db.updateFilesUsage, + getUserKeyValues: db.getUserKeyValues, + getUserCodeFiles: db.getUserCodeFiles, + getToolFilesByIds: db.getToolFilesByIds, + getCodeGeneratedFiles: db.getCodeGeneratedFiles, + filterFilesByAgentAccess, + }; + const primaryConfig = await initializeAgent( { req, @@ -220,19 +237,71 @@ const OpenAIChatCompletionController = async (req, res) => { allowedProviders, isInitialAgent: true, }, + dbMethods, + ); + + /** + * Per-agent tool-execution context map, keyed by agentId. + * Needed so the ON_TOOL_EXECUTE callback routes each sub-agent's tool calls + * to the correct toolRegistry / userMCPAuthMap / tool_resources. + * @type {Map>, + * tool_resources?: object, + * actionsEnabled?: boolean, + * }>} + */ + const agentToolContexts = new Map(); + agentToolContexts.set(primaryConfig.id, { + agent, + toolRegistry: primaryConfig.toolRegistry, + userMCPAuthMap: primaryConfig.userMCPAuthMap, + tool_resources: primaryConfig.tool_resources, + actionsEnabled: primaryConfig.actionsEnabled, + }); + + const modelsConfig = await getModelsConfig(req); + const { + agentConfigs: handoffAgentConfigs, + edges: discoveredEdges, + userMCPAuthMap: discoveredMCPAuthMap, + } = await discoverConnectedAgents( + { + req, + res, + primaryConfig, + endpointOption, + allowedProviders, + modelsConfig, + loadTools, + requestFiles: [], + conversationId, + parentMessageId, + }, { - getConvoFiles: db.getConvoFiles, - getFiles: db.getFiles, - getUserKey: db.getUserKey, - getMessages: db.getMessages, - updateFilesUsage: db.updateFilesUsage, - getUserKeyValues: db.getUserKeyValues, - getUserCodeFiles: db.getUserCodeFiles, - getToolFilesByIds: db.getToolFilesByIds, - getCodeGeneratedFiles: db.getCodeGeneratedFiles, + getAgent: db.getAgent, + checkPermission, + logViolation, + db: dbMethods, + onAgentInitialized: (agentId, handoffAgent, config) => { + agentToolContexts.set(agentId, { + agent: handoffAgent, + toolRegistry: config.toolRegistry, + userMCPAuthMap: config.userMCPAuthMap, + tool_resources: config.tool_resources, + actionsEnabled: config.actionsEnabled, + }); + }, + initializeAgent, }, ); + // Ensure edges is an array when multi-agent mode is active + // (MultiAgentGraph.categorizeEdges requires edges to be iterable) + const edges = handoffAgentConfigs.size > 0 ? (discoveredEdges ?? []) : discoveredEdges; + primaryConfig.edges = edges; + // Determine if streaming is enabled (check both request and agent config) const streamingDisabled = !!primaryConfig.model_parameters?.disableStreaming; const isStreaming = request.stream === true && !streamingDisabled; @@ -270,17 +339,18 @@ const OpenAIChatCompletionController = async (req, res) => { const toolEndCallback = createToolEndCallback({ req, res, artifactPromises, streamId: null }); const toolExecuteOptions = { - loadTools: async (toolNames) => { + loadTools: async (toolNames, agentId) => { + const ctx = agentToolContexts.get(agentId) ?? agentToolContexts.get(primaryConfig.id) ?? {}; return loadToolsForExecution({ req, res, - agent, toolNames, + agent: ctx.agent ?? agent, signal: abortController.signal, - toolRegistry: primaryConfig.toolRegistry, - userMCPAuthMap: primaryConfig.userMCPAuthMap, - tool_resources: primaryConfig.tool_resources, - actionsEnabled: primaryConfig.actionsEnabled, + toolRegistry: ctx.toolRegistry, + userMCPAuthMap: ctx.userMCPAuthMap, + tool_resources: ctx.tool_resources, + actionsEnabled: ctx.actionsEnabled, }); }, toolEndCallback, @@ -467,11 +537,14 @@ const OpenAIChatCompletionController = async (req, res) => { // Create and run the agent const userId = req.user?.id ?? 'api-user'; - // Extract userMCPAuthMap from primaryConfig (needed for MCP tool connections) - const userMCPAuthMap = primaryConfig.userMCPAuthMap; + // Extract merged userMCPAuthMap (needed for MCP tool connections across + // the primary and any discovered handoff sub-agents) + const userMCPAuthMap = discoveredMCPAuthMap ?? primaryConfig.userMCPAuthMap; + + const runAgents = [primaryConfig, ...handoffAgentConfigs.values()]; const run = await createRun({ - agents: [primaryConfig], + agents: runAgents, messages: formattedMessages, indexTokenCountMap, initialSummary, diff --git a/api/server/controllers/agents/responses.js b/api/server/controllers/agents/responses.js index 7abddf5e2f74..45c3fa2249eb 100644 --- a/api/server/controllers/agents/responses.js +++ b/api/server/controllers/agents/responses.js @@ -12,6 +12,7 @@ const { recordCollectedUsage, getTransactionsConfig, createToolExecuteHandler, + discoverConnectedAgents, // Responses API writeDone, buildResponse, @@ -38,7 +39,10 @@ const { agentLogHandlerObj, } = require('~/server/controllers/agents/callbacks'); const { loadAgentTools, loadToolsForExecution } = require('~/server/services/ToolService'); -const { findAccessibleResources } = require('~/server/services/PermissionService'); +const { findAccessibleResources, checkPermission } = require('~/server/services/PermissionService'); +const { getModelsConfig } = require('~/server/controllers/ModelController'); +const { filterFilesByAgentAccess } = require('~/server/services/Files/permissions'); +const { logViolation } = require('~/cache'); const db = require('~/models'); /** @type {import('@librechat/api').AppConfig | null} */ @@ -345,6 +349,19 @@ const createResponse = async (req, res) => { model_parameters: agent.model_parameters ?? {}, }; + const dbMethods = { + getConvoFiles: db.getConvoFiles, + getFiles: db.getFiles, + getUserKey: db.getUserKey, + getMessages: db.getMessages, + updateFilesUsage: db.updateFilesUsage, + getUserKeyValues: db.getUserKeyValues, + getUserCodeFiles: db.getUserCodeFiles, + getToolFilesByIds: db.getToolFilesByIds, + getCodeGeneratedFiles: db.getCodeGeneratedFiles, + filterFilesByAgentAccess, + }; + const primaryConfig = await initializeAgent( { req, @@ -358,19 +375,71 @@ const createResponse = async (req, res) => { allowedProviders, isInitialAgent: true, }, + dbMethods, + ); + + /** + * Per-agent tool-execution context map, keyed by agentId. Ensures the + * ON_TOOL_EXECUTE callback routes each sub-agent's tool calls to the + * correct toolRegistry / userMCPAuthMap / tool_resources. + * @type {Map>, + * tool_resources?: object, + * actionsEnabled?: boolean, + * }>} + */ + const agentToolContexts = new Map(); + agentToolContexts.set(primaryConfig.id, { + agent, + toolRegistry: primaryConfig.toolRegistry, + userMCPAuthMap: primaryConfig.userMCPAuthMap, + tool_resources: primaryConfig.tool_resources, + actionsEnabled: primaryConfig.actionsEnabled, + }); + + const modelsConfig = await getModelsConfig(req); + const { + agentConfigs: handoffAgentConfigs, + edges: discoveredEdges, + userMCPAuthMap: discoveredMCPAuthMap, + } = await discoverConnectedAgents( + { + req, + res, + primaryConfig, + endpointOption, + allowedProviders, + modelsConfig, + loadTools, + requestFiles: [], + conversationId, + parentMessageId, + }, { - getConvoFiles: db.getConvoFiles, - getFiles: db.getFiles, - getUserKey: db.getUserKey, - getMessages: db.getMessages, - updateFilesUsage: db.updateFilesUsage, - getUserKeyValues: db.getUserKeyValues, - getUserCodeFiles: db.getUserCodeFiles, - getToolFilesByIds: db.getToolFilesByIds, - getCodeGeneratedFiles: db.getCodeGeneratedFiles, + getAgent: db.getAgent, + checkPermission, + logViolation, + db: dbMethods, + onAgentInitialized: (agentId, handoffAgent, config) => { + agentToolContexts.set(agentId, { + agent: handoffAgent, + toolRegistry: config.toolRegistry, + userMCPAuthMap: config.userMCPAuthMap, + tool_resources: config.tool_resources, + actionsEnabled: config.actionsEnabled, + }); + }, + initializeAgent, }, ); + const edges = handoffAgentConfigs.size > 0 ? (discoveredEdges ?? []) : discoveredEdges; + primaryConfig.edges = edges; + const runAgents = [primaryConfig, ...handoffAgentConfigs.values()]; + const mergedMCPAuthMap = discoveredMCPAuthMap ?? primaryConfig.userMCPAuthMap; + // Determine if streaming is enabled (check both request and agent config) const streamingDisabled = !!primaryConfig.model_parameters?.disableStreaming; const actuallyStreaming = isStreaming && !streamingDisabled; @@ -436,17 +505,19 @@ const createResponse = async (req, res) => { // Create tool execute options for event-driven tool execution const toolExecuteOptions = { - loadTools: async (toolNames) => { + loadTools: async (toolNames, agentId) => { + const ctx = + agentToolContexts.get(agentId) ?? agentToolContexts.get(primaryConfig.id) ?? {}; return loadToolsForExecution({ req, res, - agent, toolNames, + agent: ctx.agent ?? agent, signal: abortController.signal, - toolRegistry: primaryConfig.toolRegistry, - userMCPAuthMap: primaryConfig.userMCPAuthMap, - tool_resources: primaryConfig.tool_resources, - actionsEnabled: primaryConfig.actionsEnabled, + toolRegistry: ctx.toolRegistry, + userMCPAuthMap: ctx.userMCPAuthMap, + tool_resources: ctx.tool_resources, + actionsEnabled: ctx.actionsEnabled, }); }, toolEndCallback, @@ -483,10 +554,10 @@ const createResponse = async (req, res) => { // Create and run the agent const userId = req.user?.id ?? 'api-user'; - const userMCPAuthMap = primaryConfig.userMCPAuthMap; + const userMCPAuthMap = mergedMCPAuthMap; const run = await createRun({ - agents: [primaryConfig], + agents: runAgents, messages: formattedMessages, indexTokenCountMap, initialSummary, @@ -601,17 +672,19 @@ const createResponse = async (req, res) => { const toolEndCallback = createToolEndCallback({ req, res, artifactPromises, streamId: null }); const toolExecuteOptions = { - loadTools: async (toolNames) => { + loadTools: async (toolNames, agentId) => { + const ctx = + agentToolContexts.get(agentId) ?? agentToolContexts.get(primaryConfig.id) ?? {}; return loadToolsForExecution({ req, res, - agent, toolNames, + agent: ctx.agent ?? agent, signal: abortController.signal, - toolRegistry: primaryConfig.toolRegistry, - userMCPAuthMap: primaryConfig.userMCPAuthMap, - tool_resources: primaryConfig.tool_resources, - actionsEnabled: primaryConfig.actionsEnabled, + toolRegistry: ctx.toolRegistry, + userMCPAuthMap: ctx.userMCPAuthMap, + tool_resources: ctx.tool_resources, + actionsEnabled: ctx.actionsEnabled, }); }, toolEndCallback, @@ -646,10 +719,10 @@ const createResponse = async (req, res) => { }; const userId = req.user?.id ?? 'api-user'; - const userMCPAuthMap = primaryConfig.userMCPAuthMap; + const userMCPAuthMap = mergedMCPAuthMap; const run = await createRun({ - agents: [primaryConfig], + agents: runAgents, messages: formattedMessages, indexTokenCountMap, initialSummary, diff --git a/api/server/services/Endpoints/agents/initialize.js b/api/server/services/Endpoints/agents/initialize.js index 69767e191cb5..0a329dea0e42 100644 --- a/api/server/services/Endpoints/agents/initialize.js +++ b/api/server/services/Endpoints/agents/initialize.js @@ -3,15 +3,11 @@ const { createContentAggregator } = require('@librechat/agents'); const { initializeAgent, validateAgentModel, - createEdgeCollector, - filterOrphanedEdges, GenerationJobManager, getCustomEndpointConfig, - createSequentialChainEdges, + discoverConnectedAgents, } = require('@librechat/api'); const { - ResourceType, - PermissionBits, EModelEndpoint, isAgentsEndpoint, getResponseSender, @@ -230,63 +226,29 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { actionsEnabled: primaryConfig.actionsEnabled, }); - const agent_ids = primaryConfig.agent_ids; - let userMCPAuthMap = primaryConfig.userMCPAuthMap; - - /** @type {Set} Track agents that failed to load (orphaned references) */ - const skippedAgentIds = new Set(); - - async function processAgent(agentId) { - const agent = await db.getAgent({ id: agentId }); - if (!agent) { - logger.warn( - `[processAgent] Handoff agent ${agentId} not found, skipping (orphaned reference)`, - ); - skippedAgentIds.add(agentId); - return null; - } - - const hasAccess = await checkPermission({ - userId: req.user.id, - role: req.user.role, - resourceType: ResourceType.AGENT, - resourceId: agent._id, - requiredPermission: PermissionBits.VIEW, - }); - - if (!hasAccess) { - logger.warn( - `[processAgent] User ${req.user.id} lacks VIEW access to handoff agent ${agentId}, skipping`, - ); - skippedAgentIds.add(agentId); - return null; - } - - const validationResult = await validateAgentModel({ + const { + agentConfigs: discoveredConfigs, + edges: discoveredEdges, + userMCPAuthMap: discoveredMCPAuthMap, + } = await discoverConnectedAgents( + { req, res, - agent, + primaryConfig, + agent_ids: primaryConfig.agent_ids, + endpointOption, + allowedProviders, modelsConfig, + loadTools, + requestFiles, + conversationId, + parentMessageId, + }, + { + getAgent: db.getAgent, + checkPermission, logViolation, - }); - - if (!validationResult.isValid) { - throw new Error(validationResult.error?.message); - } - - const config = await initializeAgent( - { - req, - res, - agent, - loadTools, - requestFiles, - conversationId, - parentMessageId, - endpointOption, - allowedProviders, - }, - { + db: { getFiles: db.getFiles, getUserKey: db.getUserKey, getMessages: db.getMessages, @@ -298,71 +260,33 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { getCodeGeneratedFiles: db.getCodeGeneratedFiles, filterFilesByAgentAccess, }, - ); - - if (userMCPAuthMap != null) { - Object.assign(userMCPAuthMap, config.userMCPAuthMap ?? {}); - } else { - userMCPAuthMap = config.userMCPAuthMap; - } - - /** Store handoff agent's tool context for ON_TOOL_EXECUTE callback */ - agentToolContexts.set(agentId, { - agent, - toolRegistry: config.toolRegistry, - userMCPAuthMap: config.userMCPAuthMap, - tool_resources: config.tool_resources, - actionsEnabled: config.actionsEnabled, - }); - - agentConfigs.set(agentId, config); - return agent; - } - - const checkAgentInit = (agentId) => agentId === primaryConfig.id || agentConfigs.has(agentId); - - // Graph topology discovery for recursive agent handoffs (BFS) - const { edgeMap, agentsToProcess, collectEdges } = createEdgeCollector( - checkAgentInit, - skippedAgentIds, + onAgentInitialized: (agentId, agent, config) => { + agentConfigs.set(agentId, config); + agentToolContexts.set(agentId, { + agent, + toolRegistry: config.toolRegistry, + userMCPAuthMap: config.userMCPAuthMap, + tool_resources: config.tool_resources, + actionsEnabled: config.actionsEnabled, + }); + }, + // Pass through the `@librechat/api` exports so that tests which + // `jest.mock('@librechat/api')` can override the initializer/validator. + initializeAgent, + validateAgentModel, + }, ); - // Seed with primary agent's edges - collectEdges(primaryConfig.edges); - - // BFS to load and merge all connected agents (enables transitive handoffs: A->B->C) - while (agentsToProcess.size > 0) { - const agentId = agentsToProcess.values().next().value; - agentsToProcess.delete(agentId); - try { - const agent = await processAgent(agentId); - if (agent?.edges?.length) { - collectEdges(agent.edges); - } - } catch (err) { - logger.error(`[initializeClient] Error processing agent ${agentId}:`, err); - skippedAgentIds.add(agentId); - } - } - - /** @deprecated Agent Chain */ - if (agent_ids?.length) { - for (const agentId of agent_ids) { - if (checkAgentInit(agentId)) { - continue; - } - try { - await processAgent(agentId); - } catch (err) { - logger.error(`[initializeClient] Error processing chain agent ${agentId}:`, err); - skippedAgentIds.add(agentId); - } + // Merge any entries the onAgentInitialized callback did not receive + // (defensive: discoverConnectedAgents already returns a canonical map) + for (const [agentId, config] of discoveredConfigs) { + if (!agentConfigs.has(agentId)) { + agentConfigs.set(agentId, config); } - const chain = await createSequentialChainEdges([primaryConfig.id].concat(agent_ids), '{convo}'); - collectEdges(chain); } - let edges = Array.from(edgeMap.values()); + let userMCPAuthMap = discoveredMCPAuthMap; + let edges = discoveredEdges; /** Multi-Convo: Process addedConvo for parallel agent execution */ const { userMCPAuthMap: updatedMCPAuthMap } = await processAddedConvo({ @@ -405,9 +329,6 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { edges = []; } - // Filter out edges referencing non-existent agents (orphaned references) - edges = filterOrphanedEdges(edges, skippedAgentIds); - primaryConfig.edges = edges; let endpointConfig = appConfig.endpoints?.[primaryConfig.endpoint]; diff --git a/packages/api/src/agents/discovery.spec.ts b/packages/api/src/agents/discovery.spec.ts new file mode 100644 index 000000000000..43424e07a266 --- /dev/null +++ b/packages/api/src/agents/discovery.spec.ts @@ -0,0 +1,374 @@ +import type { Agent, GraphEdge } from 'librechat-data-provider'; +import type { Request, Response } from 'express'; +import type { InitializedAgent } from './initialize'; + +jest.mock('@librechat/data-schemas', () => ({ + logger: { + debug: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + }, +})); + +const mockInitializeAgent = jest.fn(); +jest.mock('./initialize', () => ({ + initializeAgent: (...args: unknown[]) => mockInitializeAgent(...args), +})); + +const mockValidateAgentModel = jest.fn(); +jest.mock('./validation', () => ({ + validateAgentModel: (...args: unknown[]) => mockValidateAgentModel(...args), +})); + +import { discoverConnectedAgents } from './discovery'; + +const makeReq = (userId = 'u1', role = 'USER'): Request => + ({ + user: { id: userId, role }, + }) as unknown as Request; + +const makeRes = (): Response => ({}) as unknown as Response; + +const makeAgent = (id: string, edges: GraphEdge[] = []): Agent => + ({ + id, + _id: `mongo-${id}`, + provider: 'openai', + model: 'gpt-4o', + edges, + }) as unknown as Agent; + +const makeConfig = (id: string, edges: GraphEdge[] = []): InitializedAgent => + ({ + id, + provider: 'openai', + model: 'gpt-4o', + edges, + tools: [], + }) as unknown as InitializedAgent; + +describe('discoverConnectedAgents', () => { + beforeEach(() => { + jest.clearAllMocks(); + mockValidateAgentModel.mockResolvedValue({ isValid: true }); + mockInitializeAgent.mockImplementation(async ({ agent }: { agent: Agent }) => + makeConfig(agent.id), + ); + }); + + it('returns empty configs and no edges for a single agent with no edges', async () => { + const primaryConfig = makeConfig('A'); + const result = await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent: jest.fn(), + checkPermission: jest.fn(), + logViolation: jest.fn(), + db: {} as never, + }, + ); + + expect(result.agentConfigs.size).toBe(0); + expect(result.edges).toEqual([]); + expect(result.skippedAgentIds.size).toBe(0); + }); + + it('discovers handoff targets via BFS across a multi-hop chain A -> B -> C', async () => { + const edgesAB: GraphEdge[] = [{ from: 'A', to: 'B', edgeType: 'handoff' }]; + const edgesBC: GraphEdge[] = [{ from: 'B', to: 'C', edgeType: 'handoff' }]; + const primaryConfig = makeConfig('A', edgesAB); + + const agents: Record = { + B: makeAgent('B', edgesBC), + C: makeAgent('C', []), + }; + const getAgent = jest.fn(async ({ id }: { id: string }) => agents[id] ?? null); + const checkPermission = jest.fn().mockResolvedValue(true); + + const onAgentInitialized = jest.fn(); + + const result = await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + onAgentInitialized, + }, + ); + + expect(getAgent).toHaveBeenCalledWith({ id: 'B' }); + expect(getAgent).toHaveBeenCalledWith({ id: 'C' }); + expect(result.agentConfigs.size).toBe(2); + expect(result.agentConfigs.has('B')).toBe(true); + expect(result.agentConfigs.has('C')).toBe(true); + expect(result.edges).toHaveLength(2); + expect(onAgentInitialized).toHaveBeenCalledTimes(2); + expect(onAgentInitialized.mock.calls.map((c) => c[0])).toEqual(['B', 'C']); + }); + + it('skips orphaned agents and filters out edges pointing at them', async () => { + const edges: GraphEdge[] = [ + { from: 'A', to: 'B', edgeType: 'handoff' }, + { from: 'A', to: 'MISSING', edgeType: 'handoff' }, + ]; + const primaryConfig = makeConfig('A', edges); + + const getAgent = jest.fn(async ({ id }: { id: string }) => + id === 'B' ? makeAgent('B', []) : null, + ); + const checkPermission = jest.fn().mockResolvedValue(true); + const onAgentSkipped = jest.fn(); + + const result = await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + onAgentSkipped, + }, + ); + + expect(result.agentConfigs.has('B')).toBe(true); + expect(result.agentConfigs.has('MISSING')).toBe(false); + expect(result.skippedAgentIds.has('MISSING')).toBe(true); + expect(result.edges).toHaveLength(1); + expect(result.edges[0].to).toBe('B'); + expect(onAgentSkipped).toHaveBeenCalledWith('MISSING'); + }); + + it('skips sub-agents the user lacks VIEW permission on and filters their edges', async () => { + const edges: GraphEdge[] = [ + { from: 'A', to: 'B', edgeType: 'handoff' }, + { from: 'A', to: 'FORBIDDEN', edgeType: 'handoff' }, + ]; + const primaryConfig = makeConfig('A', edges); + + const agentMap: Record = { + B: makeAgent('B', []), + FORBIDDEN: makeAgent('FORBIDDEN', []), + }; + const getAgent = jest.fn(async ({ id }: { id: string }) => agentMap[id] ?? null); + const checkPermission = jest.fn( + async ({ resourceId }: { resourceId: unknown }) => resourceId !== 'mongo-FORBIDDEN', + ); + + const result = await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + }, + ); + + expect(result.agentConfigs.has('B')).toBe(true); + expect(result.agentConfigs.has('FORBIDDEN')).toBe(false); + expect(result.skippedAgentIds.has('FORBIDDEN')).toBe(true); + expect(result.edges.map((e) => e.to)).toEqual(['B']); + }); + + it('does not re-initialize agents that are already known (dedup)', async () => { + const edges: GraphEdge[] = [ + { from: 'A', to: 'B', edgeType: 'handoff' }, + { from: 'B', to: 'A', edgeType: 'handoff' }, + { from: 'A', to: 'B', edgeType: 'handoff' }, + ]; + const primaryConfig = makeConfig('A', edges); + + const getAgent = jest.fn(async ({ id }: { id: string }) => + id === 'B' ? makeAgent('B', [{ from: 'B', to: 'A', edgeType: 'handoff' }]) : null, + ); + const checkPermission = jest.fn().mockResolvedValue(true); + + const result = await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + }, + ); + + // B should be fetched once; A is the primary and is never re-fetched + expect(getAgent).toHaveBeenCalledTimes(1); + expect(result.agentConfigs.size).toBe(1); + expect(result.agentConfigs.has('B')).toBe(true); + // Deduped edges: A->B and B->A (not the duplicate A->B) + expect(result.edges).toHaveLength(2); + }); + + it('processes legacy agent_ids chain and adds sequential chain edges', async () => { + const primaryConfig = makeConfig('A', []); + + const agentMap: Record = { + B: makeAgent('B', []), + C: makeAgent('C', []), + }; + const getAgent = jest.fn(async ({ id }: { id: string }) => agentMap[id] ?? null); + const checkPermission = jest.fn().mockResolvedValue(true); + + const result = await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + agent_ids: ['B', 'C'], + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + }, + ); + + expect(result.agentConfigs.size).toBe(2); + expect(result.agentConfigs.has('B')).toBe(true); + expect(result.agentConfigs.has('C')).toBe(true); + // Sequential chain: A -> B -> C (2 direct edges) + expect(result.edges).toHaveLength(2); + expect(result.edges.every((e) => e.edgeType === 'direct')).toBe(true); + }); + + it('merges userMCPAuthMap across primary and sub-agents', async () => { + const primaryConfig = makeConfig('A', [{ from: 'A', to: 'B', edgeType: 'handoff' }]); + primaryConfig.userMCPAuthMap = { serverA: { token: 'primary' } }; + + const getAgent = jest.fn(async () => makeAgent('B', [])); + const checkPermission = jest.fn().mockResolvedValue(true); + + mockInitializeAgent.mockImplementation(async ({ agent }: { agent: Agent }) => { + const cfg = makeConfig(agent.id); + cfg.userMCPAuthMap = { serverB: { token: 'secondary' } }; + return cfg; + }); + + const result = await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + }, + ); + + expect(result.userMCPAuthMap).toEqual({ + serverA: { token: 'primary' }, + serverB: { token: 'secondary' }, + }); + }); + + it('throws when a sub-agent model validation fails', async () => { + mockValidateAgentModel.mockResolvedValueOnce({ + isValid: false, + error: { message: 'bad model' }, + }); + + const primaryConfig = makeConfig('A', [{ from: 'A', to: 'B', edgeType: 'handoff' }]); + + const getAgent = jest.fn(async () => makeAgent('B', [])); + const checkPermission = jest.fn().mockResolvedValue(true); + + const result = await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + }, + ); + + // Validation error is caught inside the BFS and the agent is skipped + expect(result.skippedAgentIds.has('B')).toBe(true); + expect(result.agentConfigs.has('B')).toBe(false); + }); + + it('skips when request has no authenticated user', async () => { + const primaryConfig = makeConfig('A', [{ from: 'A', to: 'B', edgeType: 'handoff' }]); + + const getAgent = jest.fn(async () => makeAgent('B', [])); + const checkPermission = jest.fn(); + + const result = await discoverConnectedAgents( + { + req: { user: undefined } as unknown as Request, + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + }, + ); + + expect(checkPermission).not.toHaveBeenCalled(); + expect(result.skippedAgentIds.has('B')).toBe(true); + expect(result.edges).toHaveLength(0); + }); +}); diff --git a/packages/api/src/agents/discovery.ts b/packages/api/src/agents/discovery.ts new file mode 100644 index 000000000000..9aef0ee9b66d --- /dev/null +++ b/packages/api/src/agents/discovery.ts @@ -0,0 +1,273 @@ +import { logger } from '@librechat/data-schemas'; +import { ResourceType, PermissionBits } from 'librechat-data-provider'; +import type { Agent, GraphEdge, TModelsConfig, TEndpointOption } from 'librechat-data-provider'; +import type { Response as ServerResponse } from 'express'; +import type { ServerRequest } from '~/types'; +import type { + InitializedAgent, + InitializeAgentParams, + InitializeAgentDbMethods, +} from './initialize'; +import type { ValidateAgentModelParams } from './validation'; +import { createEdgeCollector, filterOrphanedEdges } from './edges'; +import { createSequentialChainEdges } from './chain'; +import { validateAgentModel as defaultValidateAgentModel } from './validation'; +import { initializeAgent as defaultInitializeAgent } from './initialize'; + +/** + * Callback invoked after a sub-agent is successfully initialized. + * Used by callers that need to track per-agent tool context (e.g., for + * the ON_TOOL_EXECUTE event handler closure). + */ +export type OnAgentInitializedCallback = ( + agentId: string, + agent: Agent, + config: InitializedAgent, +) => void; + +/** + * Minimal permission check signature used to verify VIEW access on a + * candidate sub-agent before it is loaded into the run. + */ +export type CheckAgentPermission = (params: { + userId: string; + role?: string; + resourceType: string; + resourceId: unknown; + requiredPermission: number; +}) => Promise; + +export interface DiscoverConnectedAgentsParams { + req: ServerRequest; + res: ServerResponse; + /** The already-initialized primary agent config (starting point for BFS). */ + primaryConfig: InitializedAgent; + /** + * Optional legacy chain: agent IDs to append as sequential direct edges. + * Used by the deprecated Agent Chain feature. + */ + agent_ids?: string[]; + endpointOption?: Partial; + allowedProviders: Set; + modelsConfig: TModelsConfig; + loadTools: InitializeAgentParams['loadTools']; + requestFiles?: InitializeAgentParams['requestFiles']; + conversationId?: string | null; + parentMessageId?: string | null; +} + +export interface DiscoverConnectedAgentsDeps { + /** Fetch an agent by string id from the database. */ + getAgent: (filter: { id: string }) => Promise; + /** Permission check (typically a wrapper around PermissionService.checkPermission). */ + checkPermission: CheckAgentPermission; + /** Violation logger passed through to validateAgentModel. */ + logViolation: ValidateAgentModelParams['logViolation']; + /** DB methods consumed by initializeAgent for each sub-agent. */ + db: InitializeAgentDbMethods; + /** Optional callback invoked after each sub-agent is initialized. */ + onAgentInitialized?: OnAgentInitializedCallback; + /** Optional callback invoked when an agent id is skipped (missing or no access). */ + onAgentSkipped?: (agentId: string) => void; + /** + * Optional override for `initializeAgent`. Exists primarily so JS callers + * can inject their test doubles via `jest.mock('@librechat/api')` — since + * this module's own direct import would otherwise bypass that mock. + */ + initializeAgent?: typeof defaultInitializeAgent; + /** Optional override for `validateAgentModel` (same DI rationale). */ + validateAgentModel?: typeof defaultValidateAgentModel; +} + +export interface DiscoverConnectedAgentsResult { + /** Map of agentId -> initialized config for every discovered sub-agent. */ + agentConfigs: Map; + /** Deduplicated, orphan-filtered edges across the primary and sub-agents. */ + edges: GraphEdge[]; + /** Agent ids that were requested but could not be loaded (missing or no access). */ + skippedAgentIds: Set; + /** Merged MCP auth map from the primary and all sub-agents. */ + userMCPAuthMap?: Record>; +} + +/** + * Discovers and initializes all agents reachable from `primaryConfig.edges` + * via BFS. This is the shared graph-topology discovery logic that enables + * multi-agent handoffs (A -> B -> C) in both the primary chat flow and the + * OpenAI-compatible / Responses API controllers. + * + * Skips agents the caller cannot load (missing from DB or lacking VIEW + * permission) and filters out orphaned edges so `createRun` never sees an + * edge pointing at a missing node — which would otherwise trigger a + * `Found edge ending at unknown node` validation error from StateGraph. + */ +export async function discoverConnectedAgents( + params: DiscoverConnectedAgentsParams, + deps: DiscoverConnectedAgentsDeps, +): Promise { + const { + req, + res, + primaryConfig, + agent_ids, + endpointOption, + allowedProviders, + modelsConfig, + loadTools, + requestFiles, + conversationId, + parentMessageId, + } = params; + + const { + getAgent, + checkPermission, + logViolation, + db, + onAgentInitialized, + onAgentSkipped, + initializeAgent = defaultInitializeAgent, + validateAgentModel = defaultValidateAgentModel, + } = deps; + + const agentConfigs = new Map(); + const skippedAgentIds = new Set(); + let userMCPAuthMap: Record> | undefined = + primaryConfig.userMCPAuthMap; + + const markSkipped = (agentId: string): void => { + skippedAgentIds.add(agentId); + onAgentSkipped?.(agentId); + }; + + const processAgent = async (agentId: string): Promise => { + const agent = await getAgent({ id: agentId }); + if (!agent) { + logger.warn( + `[discoverConnectedAgents] Handoff agent ${agentId} not found, skipping (orphaned reference)`, + ); + markSkipped(agentId); + return null; + } + + const userId = req.user?.id; + if (!userId) { + logger.warn( + `[discoverConnectedAgents] No authenticated user on request, skipping handoff agent ${agentId}`, + ); + markSkipped(agentId); + return null; + } + + const hasAccess = await checkPermission({ + userId, + role: req.user?.role, + resourceType: ResourceType.AGENT, + resourceId: agent._id, + requiredPermission: PermissionBits.VIEW, + }); + + if (!hasAccess) { + logger.warn( + `[discoverConnectedAgents] User ${userId} lacks VIEW access to handoff agent ${agentId}, skipping`, + ); + markSkipped(agentId); + return null; + } + + const validation = await validateAgentModel({ + req, + res, + agent, + modelsConfig, + logViolation, + }); + + if (!validation.isValid) { + throw new Error(validation.error?.message); + } + + const config = await initializeAgent( + { + req, + res, + agent, + loadTools, + requestFiles, + conversationId, + parentMessageId, + endpointOption, + allowedProviders, + }, + db, + ); + + if (userMCPAuthMap != null) { + Object.assign(userMCPAuthMap, config.userMCPAuthMap ?? {}); + } else { + userMCPAuthMap = config.userMCPAuthMap; + } + + agentConfigs.set(agentId, config); + onAgentInitialized?.(agentId, agent, config); + return agent; + }; + + const checkAgentInit = (agentId: string): boolean => + agentId === primaryConfig.id || agentConfigs.has(agentId); + + const { edgeMap, agentsToProcess, collectEdges } = createEdgeCollector( + checkAgentInit, + skippedAgentIds, + ); + + collectEdges(primaryConfig.edges); + + while (agentsToProcess.size > 0) { + const agentId = agentsToProcess.values().next().value as string; + agentsToProcess.delete(agentId); + try { + const agent = await processAgent(agentId); + if (agent?.edges?.length) { + collectEdges(agent.edges); + } + } catch (err) { + logger.error(`[discoverConnectedAgents] Error processing agent ${agentId}:`, err); + markSkipped(agentId); + } + } + + /** @deprecated Agent Chain — sequential direct-edge fallback */ + if (agent_ids?.length) { + for (const agentId of agent_ids) { + if (checkAgentInit(agentId)) { + continue; + } + try { + await processAgent(agentId); + } catch (err) { + logger.error(`[discoverConnectedAgents] Error processing chain agent ${agentId}:`, err); + markSkipped(agentId); + } + } + /** + * `createSequentialChainEdges` is typed against `@librechat/agents`'s + * `GraphEdge` (which uses `BaseMessage` from `@langchain/core`) whereas + * `collectEdges` uses the `librechat-data-provider` variant (structural + * `BaseMessage`). The produced chain edges are structurally identical and + * only carry `edgeType`, `from`, `to`, `prompt`, `excludeResults` — + * interchangeable for edge collection purposes. + */ + const chain = await createSequentialChainEdges([primaryConfig.id].concat(agent_ids), '{convo}'); + collectEdges(chain as unknown as GraphEdge[]); + } + + const edges = filterOrphanedEdges(Array.from(edgeMap.values()), skippedAgentIds); + + return { + agentConfigs, + edges, + skippedAgentIds, + userMCPAuthMap, + }; +} diff --git a/packages/api/src/agents/index.ts b/packages/api/src/agents/index.ts index 53f7f60a9337..336eaea479c7 100644 --- a/packages/api/src/agents/index.ts +++ b/packages/api/src/agents/index.ts @@ -3,6 +3,7 @@ export * from './chain'; export * from './client'; export * from './config'; export * from './context'; +export * from './discovery'; export * from './edges'; export * from './handlers'; export * from './initialize'; diff --git a/packages/api/src/agents/validation.ts b/packages/api/src/agents/validation.ts index 8119c9720455..6e3a1c200de3 100644 --- a/packages/api/src/agents/validation.ts +++ b/packages/api/src/agents/validation.ts @@ -3,6 +3,13 @@ import { ViolationTypes, ErrorTypes } from 'librechat-data-provider'; import type { Agent, TModelsConfig } from 'librechat-data-provider'; import type { Request, Response } from 'express'; +/** + * Permissive Request alias used by {@link validateAgentModel}. Accepts either + * the default Express `Request` or the project-specific `ServerRequest` + * (see `~/types/http`), whose `params` type is widened to `unknown`. + */ +type LooseRequest = Request; + /** Avatar schema shared between create and update */ export const agentAvatarSchema = z.object({ filepath: z.string(), @@ -96,13 +103,13 @@ export const agentUpdateSchema = agentBaseSchema.extend({ model: z.string().nullable().optional(), }); -interface ValidateAgentModelParams { - req: Request; +export interface ValidateAgentModelParams { + req: LooseRequest; res: Response; agent: Agent; modelsConfig: TModelsConfig; logViolation: ( - req: Request, + req: LooseRequest, res: Response, type: string, errorMessage: Record, From 5cc74e3870e037858331c69663b05f3dbad99a2b Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 18 Apr 2026 10:59:16 -0400 Subject: [PATCH 02/17] =?UTF-8?q?=F0=9F=A7=AA=20fix:=20use=20ServerRequest?= =?UTF-8?q?=20in=20discovery=20spec=20helpers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI `tsc --noEmit -p packages/api/tsconfig.json` caught that the test helpers typed `req` as `express.Request`, which is not assignable to `DiscoverConnectedAgentsParams.req` (typed as `ServerRequest` whose `user` is `IUser`). Local jest passed because ts-jest is transpile-only, but the CI typecheck uses the full compiler. --- packages/api/src/agents/discovery.spec.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/api/src/agents/discovery.spec.ts b/packages/api/src/agents/discovery.spec.ts index 43424e07a266..337f9bd059cc 100644 --- a/packages/api/src/agents/discovery.spec.ts +++ b/packages/api/src/agents/discovery.spec.ts @@ -1,5 +1,6 @@ import type { Agent, GraphEdge } from 'librechat-data-provider'; -import type { Request, Response } from 'express'; +import type { Response } from 'express'; +import type { ServerRequest } from '~/types'; import type { InitializedAgent } from './initialize'; jest.mock('@librechat/data-schemas', () => ({ @@ -23,10 +24,10 @@ jest.mock('./validation', () => ({ import { discoverConnectedAgents } from './discovery'; -const makeReq = (userId = 'u1', role = 'USER'): Request => +const makeReq = (userId = 'u1', role = 'USER'): ServerRequest => ({ user: { id: userId, role }, - }) as unknown as Request; + }) as unknown as ServerRequest; const makeRes = (): Response => ({}) as unknown as Response; @@ -352,7 +353,7 @@ describe('discoverConnectedAgents', () => { const result = await discoverConnectedAgents( { - req: { user: undefined } as unknown as Request, + req: { user: undefined } as unknown as ServerRequest, res: makeRes(), primaryConfig, allowedProviders: new Set(), From 7418dd83b57aec3b62b302f71157a2cc3ba07b0e Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 18 Apr 2026 11:10:16 -0400 Subject: [PATCH 03/17] =?UTF-8?q?=F0=9F=AA=B2=20fix:=20drop=20orphan=20edg?= =?UTF-8?q?es=20on=20both=20endpoints,=20not=20just=20`to`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the P1 codex finding on #12740: `filterOrphanedEdges` previously only removed edges whose `to` referenced a skipped agent. Edges whose `from` was a skipped agent — the symmetric case in a bidirectional graph like `A <-> B` where `B` is deleted or the user lacks VIEW on it — leaked through to `createRun` and re-triggered `Found edge ending at unknown node ` at StateGraph compile time. The filter now drops an edge if either endpoint references a skipped id, and the existing `to`-only test cases were updated to reflect the stricter behavior. Adds a bidirectional-graph regression test in `discovery.spec.ts`. --- packages/api/src/agents/discovery.spec.ts | 34 +++++++++++++++++++++++ packages/api/src/agents/edges.spec.ts | 30 +++++++++++++------- packages/api/src/agents/edges.ts | 16 +++++++---- 3 files changed, 64 insertions(+), 16 deletions(-) diff --git a/packages/api/src/agents/discovery.spec.ts b/packages/api/src/agents/discovery.spec.ts index 337f9bd059cc..5cc811d55133 100644 --- a/packages/api/src/agents/discovery.spec.ts +++ b/packages/api/src/agents/discovery.spec.ts @@ -202,6 +202,40 @@ describe('discoverConnectedAgents', () => { expect(result.edges.map((e) => e.to)).toEqual(['B']); }); + it('drops edges whose `from` references a skipped agent (bidirectional graph)', async () => { + // Orchestrator A with bidirectional handoffs A<->B. B is inaccessible, + // so `A -> B` and `B -> A` must both be filtered — otherwise `B -> A` + // leaks into createRun and triggers `Found edge ending at unknown node`. + const edges: GraphEdge[] = [ + { from: 'A', to: 'B', edgeType: 'handoff' }, + { from: 'B', to: 'A', edgeType: 'handoff' }, + ]; + const primaryConfig = makeConfig('A', edges); + + const getAgent = jest.fn(async () => makeAgent('B', [])); + const checkPermission = jest.fn().mockResolvedValue(false); + + const result = await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + }, + ); + + expect(result.skippedAgentIds.has('B')).toBe(true); + expect(result.edges).toHaveLength(0); + }); + it('does not re-initialize agents that are already known (dedup)', async () => { const edges: GraphEdge[] = [ { from: 'A', to: 'B', edgeType: 'handoff' }, diff --git a/packages/api/src/agents/edges.spec.ts b/packages/api/src/agents/edges.spec.ts index b23f00f63fe8..8912c729b90a 100644 --- a/packages/api/src/agents/edges.spec.ts +++ b/packages/api/src/agents/edges.spec.ts @@ -131,25 +131,35 @@ describe('edges utilities', () => { expect(filterOrphanedEdges(edges, skipped)).toEqual(edges); }); - it('should filter out edges with orphaned to targets', () => { + it('should filter out edges with orphaned to targets and orphaned from sources', () => { const skipped = new Set(['agent_b']); const result = filterOrphanedEdges(edges, skipped); - // Only filters edges where `to` is in skipped set // agent_a -> agent_b (filtered - to=agent_b is skipped) - // agent_a -> agent_c (kept - to=agent_c not skipped) - // agent_b -> agent_d (kept - to=agent_d not skipped, from is not checked) - expect(result).toHaveLength(2); - expect(result.map((e) => e.to)).toEqual(['agent_c', 'agent_d']); + // agent_a -> agent_c (kept - neither endpoint skipped) + // agent_b -> agent_d (filtered - from=agent_b is skipped; a source-side + // orphan would otherwise leave the graph referencing an absent agent) + expect(result).toHaveLength(1); + expect(result[0].to).toBe('agent_c'); }); it('should filter out multiple orphaned edges', () => { const skipped = new Set(['agent_b', 'agent_c']); const result = filterOrphanedEdges(edges, skipped); - // agent_a -> agent_b (filtered) - // agent_a -> agent_c (filtered) - // agent_b -> agent_d (kept - to=agent_d not skipped) + // agent_a -> agent_b (filtered, to) + // agent_a -> agent_c (filtered, to) + // agent_b -> agent_d (filtered, from) + expect(result).toHaveLength(0); + }); + + it('should filter edges whose `from` is an array containing a skipped id', () => { + const edgesWithArrayFrom: GraphEdge[] = [ + { from: ['agent_a', 'agent_b'], to: 'agent_d', edgeType: 'handoff' }, + { from: 'agent_a', to: 'agent_d', edgeType: 'handoff' }, + ]; + const skipped = new Set(['agent_b']); + const result = filterOrphanedEdges(edgesWithArrayFrom, skipped); expect(result).toHaveLength(1); - expect(result[0].to).toBe('agent_d'); + expect(result[0].from).toBe('agent_a'); }); it('should handle array to values', () => { diff --git a/packages/api/src/agents/edges.ts b/packages/api/src/agents/edges.ts index 9a36105b7428..2e6d4324e553 100644 --- a/packages/api/src/agents/edges.ts +++ b/packages/api/src/agents/edges.ts @@ -30,17 +30,21 @@ export function getEdgeParticipants(edge: GraphEdge): string[] { } /** - * Filters out edges that reference non-existent (orphaned) agents. - * Only filters based on the 'to' field since those are the handoff targets. + * Filters out edges that reference non-existent (orphaned) agents on + * either endpoint. Any edge whose `from` OR `to` references a skipped + * agent id is dropped — leaving a source-side orphan in place would + * still fail graph compilation with `Found edge ending at unknown node` + * because `agents` no longer contains the source. */ export function filterOrphanedEdges(edges: GraphEdge[], skippedAgentIds: Set): GraphEdge[] { if (!edges || skippedAgentIds.size === 0) { return edges; } - return edges.filter((edge) => { - const toIds = Array.isArray(edge.to) ? edge.to : [edge.to]; - return !toIds.some((id) => typeof id === 'string' && skippedAgentIds.has(id)); - }); + const referencesSkipped = (value: string | string[]): boolean => { + const ids = Array.isArray(value) ? value : [value]; + return ids.some((id) => typeof id === 'string' && skippedAgentIds.has(id)); + }; + return edges.filter((edge) => !referencesSkipped(edge.from) && !referencesSkipped(edge.to)); } /** Collects all unique agent IDs referenced across an array of edges. */ From e491a23a77f5b43d33baf60e2e44a1fab91a25e9 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 18 Apr 2026 11:21:19 -0400 Subject: [PATCH 04/17] =?UTF-8?q?=F0=9F=94=92=20fix:=20enforce=20REMOTE=5F?= =?UTF-8?q?AGENT=20ACL=20on=20handoff=20sub-agents=20for=20API=20routes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the second P1 codex finding on #12740: the OpenAI-compat `/v1/chat/completions` and Open Responses `/v1/responses` routes gate the primary agent on `REMOTE_AGENT` (via `createCheckRemoteAgentAccess`), but `discoverConnectedAgents` was checking handoff sub-agents against the looser in-app `AGENT` resource type. That allowed a remote caller who could reach the orchestrator but had only in-app visibility on a sub-agent to invoke it via the API — bypassing the remote-sharing boundary. Adds an optional `resourceType` param to `discoverConnectedAgents` (defaulting to `AGENT` for the chat UI path) and passes `ResourceType.REMOTE_AGENT` from both API controllers so every discovered sub-agent clears the same sharing boundary enforced at route entry. --- api/server/controllers/agents/openai.js | 4 ++++ api/server/controllers/agents/responses.js | 4 ++++ packages/api/src/agents/discovery.spec.ts | 28 ++++++++++++++++++++++ packages/api/src/agents/discovery.ts | 11 ++++++++- 4 files changed, 46 insertions(+), 1 deletion(-) diff --git a/api/server/controllers/agents/openai.js b/api/server/controllers/agents/openai.js index 12e47672f414..91e2b98617d0 100644 --- a/api/server/controllers/agents/openai.js +++ b/api/server/controllers/agents/openai.js @@ -278,6 +278,10 @@ const OpenAIChatCompletionController = async (req, res) => { requestFiles: [], conversationId, parentMessageId, + // The route enforces REMOTE_AGENT on the primary; every discovered + // sub-agent must clear the same sharing boundary, not the looser + // in-app AGENT one. + resourceType: ResourceType.REMOTE_AGENT, }, { getAgent: db.getAgent, diff --git a/api/server/controllers/agents/responses.js b/api/server/controllers/agents/responses.js index 45c3fa2249eb..f6530aebb095 100644 --- a/api/server/controllers/agents/responses.js +++ b/api/server/controllers/agents/responses.js @@ -416,6 +416,10 @@ const createResponse = async (req, res) => { requestFiles: [], conversationId, parentMessageId, + // The route enforces REMOTE_AGENT on the primary; every discovered + // sub-agent must clear the same sharing boundary, not the looser + // in-app AGENT one. + resourceType: ResourceType.REMOTE_AGENT, }, { getAgent: db.getAgent, diff --git a/packages/api/src/agents/discovery.spec.ts b/packages/api/src/agents/discovery.spec.ts index 5cc811d55133..f178cc72f7d6 100644 --- a/packages/api/src/agents/discovery.spec.ts +++ b/packages/api/src/agents/discovery.spec.ts @@ -202,6 +202,34 @@ describe('discoverConnectedAgents', () => { expect(result.edges.map((e) => e.to)).toEqual(['B']); }); + it('passes the configured resourceType (e.g. REMOTE_AGENT) to checkPermission', async () => { + const primaryConfig = makeConfig('A', [{ from: 'A', to: 'B', edgeType: 'handoff' }]); + const getAgent = jest.fn(async () => makeAgent('B', [])); + const checkPermission = jest.fn().mockResolvedValue(true); + + await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + resourceType: 'REMOTE_AGENT', + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + }, + ); + + expect(checkPermission).toHaveBeenCalledWith( + expect.objectContaining({ resourceType: 'REMOTE_AGENT' }), + ); + }); + it('drops edges whose `from` references a skipped agent (bidirectional graph)', async () => { // Orchestrator A with bidirectional handoffs A<->B. B is inaccessible, // so `A -> B` and `B -> A` must both be filtered — otherwise `B -> A` diff --git a/packages/api/src/agents/discovery.ts b/packages/api/src/agents/discovery.ts index 9aef0ee9b66d..9ad9d94161eb 100644 --- a/packages/api/src/agents/discovery.ts +++ b/packages/api/src/agents/discovery.ts @@ -54,6 +54,14 @@ export interface DiscoverConnectedAgentsParams { requestFiles?: InitializeAgentParams['requestFiles']; conversationId?: string | null; parentMessageId?: string | null; + /** + * ResourceType to check each sub-agent's access against. Defaults to + * `AGENT` for the in-app chat flow. Callers whose entry-point gates on + * a different resource type (e.g. the OpenAI-compat controllers gate on + * `REMOTE_AGENT`) must pass the matching resource type so sub-agents + * don't bypass the same sharing boundary enforced at the route. + */ + resourceType?: string; } export interface DiscoverConnectedAgentsDeps { @@ -117,6 +125,7 @@ export async function discoverConnectedAgents( requestFiles, conversationId, parentMessageId, + resourceType = ResourceType.AGENT, } = params; const { @@ -162,7 +171,7 @@ export async function discoverConnectedAgents( const hasAccess = await checkPermission({ userId, role: req.user?.role, - resourceType: ResourceType.AGENT, + resourceType, resourceId: agent._id, requiredPermission: PermissionBits.VIEW, }); From 7ec1ba246f80940b62a1cae98fff9950c379fcc9 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 18 Apr 2026 11:35:20 -0400 Subject: [PATCH 05/17] =?UTF-8?q?=F0=9F=A7=AF=20fix:=20enforce=20allowedPr?= =?UTF-8?q?oviders=20for=20discovered=20sub-agents?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the third P1 codex finding on #12740: `discoverConnectedAgents` forwarded the caller's `endpointOption` verbatim into `initializeAgent`, but on the OpenAI-compat routes that option's `endpoint` is the primary agent's provider (e.g. `openai`), not `agents`. `initializeAgent` only enforces `allowedProviders` when `isAgentsEndpoint(endpointOption.endpoint)` is true, so handoff sub-agents silently bypassed the provider allowlist configured under `endpoints.agents.allowedProviders`. Override `endpointOption.endpoint` to `EModelEndpoint.agents` for every per-sub-agent init call. The primary agent still uses the caller's endpointOption as before — this only affects the BFS-loaded handoff targets. Regression test asserts the override. --- packages/api/src/agents/discovery.spec.ts | 34 +++++++++++++++++++++++ packages/api/src/agents/discovery.ts | 18 ++++++++++-- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/packages/api/src/agents/discovery.spec.ts b/packages/api/src/agents/discovery.spec.ts index f178cc72f7d6..40cb51d93bf6 100644 --- a/packages/api/src/agents/discovery.spec.ts +++ b/packages/api/src/agents/discovery.spec.ts @@ -1,3 +1,4 @@ +import { EModelEndpoint } from 'librechat-data-provider'; import type { Agent, GraphEdge } from 'librechat-data-provider'; import type { Response } from 'express'; import type { ServerRequest } from '~/types'; @@ -202,6 +203,39 @@ describe('discoverConnectedAgents', () => { expect(result.edges.map((e) => e.to)).toEqual(['B']); }); + it('forces `endpoint: agents` on sub-agent init so allowedProviders is always enforced', async () => { + const primaryConfig = makeConfig('A', [{ from: 'A', to: 'B', edgeType: 'handoff' }]); + const getAgent = jest.fn(async () => makeAgent('B', [])); + const checkPermission = jest.fn().mockResolvedValue(true); + + await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + // Caller passes a non-agents endpoint (e.g. the OpenAI-compat + // controllers do this — endpoint = primary provider) + endpointOption: { endpoint: EModelEndpoint.openAI, model_parameters: {} }, + allowedProviders: new Set(['openai']), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + }, + ); + + // The helper must override `endpoint` to 'agents' so + // `isAgentsEndpoint` is true and the allowedProviders guard in + // `initializeAgent` actually runs for sub-agents. + expect(mockInitializeAgent).toHaveBeenCalled(); + const initArgs = mockInitializeAgent.mock.calls[0][0]; + expect(initArgs.endpointOption.endpoint).toBe(EModelEndpoint.agents); + }); + it('passes the configured resourceType (e.g. REMOTE_AGENT) to checkPermission', async () => { const primaryConfig = makeConfig('A', [{ from: 'A', to: 'B', edgeType: 'handoff' }]); const getAgent = jest.fn(async () => makeAgent('B', [])); diff --git a/packages/api/src/agents/discovery.ts b/packages/api/src/agents/discovery.ts index 9ad9d94161eb..b1dc9fcd9705 100644 --- a/packages/api/src/agents/discovery.ts +++ b/packages/api/src/agents/discovery.ts @@ -1,5 +1,5 @@ import { logger } from '@librechat/data-schemas'; -import { ResourceType, PermissionBits } from 'librechat-data-provider'; +import { ResourceType, PermissionBits, EModelEndpoint } from 'librechat-data-provider'; import type { Agent, GraphEdge, TModelsConfig, TEndpointOption } from 'librechat-data-provider'; import type { Response as ServerResponse } from 'express'; import type { ServerRequest } from '~/types'; @@ -196,6 +196,20 @@ export async function discoverConnectedAgents( throw new Error(validation.error?.message); } + /** + * Force `endpoint: agents` on the per-sub-agent init call so + * `initializeAgent`'s `isAgentsEndpoint`-gated `allowedProviders` + * check always fires for handoff sub-agents, regardless of which + * endpoint the caller entered through. Without this, the OpenAI- + * compat routes (whose `endpointOption.endpoint` is the primary + * provider, not `agents`) would silently bypass the provider + * allowlist configured under `endpoints.agents.allowedProviders`. + */ + const subAgentEndpointOption: Partial = { + ...(endpointOption ?? {}), + endpoint: EModelEndpoint.agents, + }; + const config = await initializeAgent( { req, @@ -205,7 +219,7 @@ export async function discoverConnectedAgents( requestFiles, conversationId, parentMessageId, - endpointOption, + endpointOption: subAgentEndpointOption, allowedProviders, }, db, From 4cbfffc9eec65aced6b5802602b134ff1c583d04 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 18 Apr 2026 11:55:52 -0400 Subject: [PATCH 06/17] =?UTF-8?q?=E2=9C=82=EF=B8=8F=20fix:=20prune=20unrea?= =?UTF-8?q?chable=20sub-agents=20after=20orphan-edge=20filtering?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the fourth P1 codex finding on #12740: BFS eagerly initializes every sub-agent referenced in the primary's edge scan, but once `filterOrphanedEdges` drops edges whose endpoints were skipped, some of those sub-agents end up disconnected from the primary. In an `A -> B -> C` graph (edges stored directly on A) where B is skipped (missing or no VIEW), both edges are filtered, but C was already loaded and would still be passed to `createRun` — which flips into multi-agent mode on `agents.length > 1` and turns C into an unintended parallel start node. After filtering edges, compute the set of agent ids reachable from the primary through the surviving edge set and prune `agentConfigs` to that set. Two regression tests added: one for the pruning case, one that confirms agents connected via surviving edges are still kept. --- packages/api/src/agents/discovery.spec.ts | 88 +++++++++++++++++++++++ packages/api/src/agents/discovery.ts | 34 +++++++++ 2 files changed, 122 insertions(+) diff --git a/packages/api/src/agents/discovery.spec.ts b/packages/api/src/agents/discovery.spec.ts index 40cb51d93bf6..5e57d547f5df 100644 --- a/packages/api/src/agents/discovery.spec.ts +++ b/packages/api/src/agents/discovery.spec.ts @@ -264,6 +264,94 @@ describe('discoverConnectedAgents', () => { ); }); + it('prunes sub-agents disconnected from the primary after edge filtering', async () => { + // Graph: primary A has edges [A->B, B->C] stored directly on A. + // BFS loads B and C before permission is checked. B is skipped, and + // both edges reference B so they are filtered out. C was initialized + // but is now unreachable from A through any surviving edge — it must + // NOT be returned (otherwise it becomes a stray start node when + // createRun sees `agents.length > 1`). + const edges: GraphEdge[] = [ + { from: 'A', to: 'B', edgeType: 'handoff' }, + { from: 'B', to: 'C', edgeType: 'handoff' }, + ]; + const primaryConfig = makeConfig('A', edges); + + const agentMap: Record = { + B: makeAgent('B', []), + C: makeAgent('C', []), + }; + const getAgent = jest.fn(async ({ id }: { id: string }) => agentMap[id] ?? null); + // B fails permission, C passes. + const checkPermission = jest.fn( + async ({ resourceId }: { resourceId: unknown }) => resourceId !== 'mongo-B', + ); + + const result = await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + }, + ); + + expect(result.skippedAgentIds.has('B')).toBe(true); + expect(result.edges).toHaveLength(0); + // C is unreachable once B's edges are filtered → must be pruned. + expect(result.agentConfigs.has('C')).toBe(false); + expect(result.agentConfigs.size).toBe(0); + }); + + it('keeps sub-agents that remain reachable via surviving edges', async () => { + // Graph: A -> [B, C]; B is skipped, C is kept. C must remain because + // A -> C survives filtering. + const edges: GraphEdge[] = [ + { from: 'A', to: 'B', edgeType: 'handoff' }, + { from: 'A', to: 'C', edgeType: 'handoff' }, + ]; + const primaryConfig = makeConfig('A', edges); + + const agentMap: Record = { + B: makeAgent('B', []), + C: makeAgent('C', []), + }; + const getAgent = jest.fn(async ({ id }: { id: string }) => agentMap[id] ?? null); + const checkPermission = jest.fn( + async ({ resourceId }: { resourceId: unknown }) => resourceId !== 'mongo-B', + ); + + const result = await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + }, + ); + + expect(result.agentConfigs.has('C')).toBe(true); + expect(result.agentConfigs.has('B')).toBe(false); + expect(result.edges).toHaveLength(1); + expect(result.edges[0].to).toBe('C'); + }); + it('drops edges whose `from` references a skipped agent (bidirectional graph)', async () => { // Orchestrator A with bidirectional handoffs A<->B. B is inaccessible, // so `A -> B` and `B -> A` must both be filtered — otherwise `B -> A` diff --git a/packages/api/src/agents/discovery.ts b/packages/api/src/agents/discovery.ts index b1dc9fcd9705..c07c85c64370 100644 --- a/packages/api/src/agents/discovery.ts +++ b/packages/api/src/agents/discovery.ts @@ -287,6 +287,40 @@ export async function discoverConnectedAgents( const edges = filterOrphanedEdges(Array.from(edgeMap.values()), skippedAgentIds); + /** + * Prune sub-agents that were initialized eagerly during BFS but whose + * connecting edges were later dropped by `filterOrphanedEdges` (e.g. an + * `A -> B -> C` chain where B is skipped, so both edges disappear but + * C had already been loaded from the primary's edge scan). Without this, + * those disconnected agents still flow into `createRun` — which flips + * into multi-agent mode whenever `agents.length > 1` and turns the + * stranded sub-agent into an unintended parallel start node. + */ + const reachable = new Set([primaryConfig.id]); + const frontier: string[] = [primaryConfig.id]; + while (frontier.length > 0) { + const current = frontier.pop() as string; + for (const edge of edges) { + const sources = Array.isArray(edge.from) ? edge.from : [edge.from]; + if (!sources.includes(current)) { + continue; + } + const dests = Array.isArray(edge.to) ? edge.to : [edge.to]; + for (const dest of dests) { + if (typeof dest === 'string' && !reachable.has(dest)) { + reachable.add(dest); + frontier.push(dest); + } + } + } + } + + for (const agentId of [...agentConfigs.keys()]) { + if (!reachable.has(agentId)) { + agentConfigs.delete(agentId); + } + } + return { agentConfigs, edges, From e54440b7d19f88302f36f81acde546df9ebdfcf1 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 18 Apr 2026 12:07:22 -0400 Subject: [PATCH 07/17] =?UTF-8?q?=F0=9F=94=81=20fix:=20don't=20seed=20init?= =?UTF-8?q?ialize.js=20agentConfigs=20from=20the=20pre-pruning=20callback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the fifth P1 codex finding on #12740: `onAgentInitialized` fires during BFS, BEFORE the helper prunes agents that become disconnected once `filterOrphanedEdges` runs. Writing the sub-agent straight into the outer `agentConfigs` there and then only additively merging the pruned `discoveredConfigs` left stranded entries in the outer map, and `AgentClient` would still hand them to `createRun` as extra parallel start nodes (the exact failure mode the pass-4 prune was meant to eliminate for the API controllers). Drop the `agentConfigs.set` from the callback and replace the additive merge with a direct copy from `discoveredConfigs`, which is now the single authoritative source of what the run should see. The per-agent tool context map is still populated during BFS — stale entries there are harmless because they're only read by closure inside `ON_TOOL_EXECUTE` and are unreachable once the agent is not in `agentConfigs`. --- .../services/Endpoints/agents/initialize.js | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/api/server/services/Endpoints/agents/initialize.js b/api/server/services/Endpoints/agents/initialize.js index 0a329dea0e42..c8515bd0fa9a 100644 --- a/api/server/services/Endpoints/agents/initialize.js +++ b/api/server/services/Endpoints/agents/initialize.js @@ -260,8 +260,13 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { getCodeGeneratedFiles: db.getCodeGeneratedFiles, filterFilesByAgentAccess, }, + // The callback fires during BFS, before the helper prunes agents + // whose edges end up filtered. Don't populate `agentConfigs` here — + // `discoveredConfigs` (returned below) is the authoritative pruned + // set. The per-agent tool context map is OK to keep populated even + // for pruned ids: it's only read by closure in ON_TOOL_EXECUTE, + // stale entries are unreachable at runtime. onAgentInitialized: (agentId, agent, config) => { - agentConfigs.set(agentId, config); agentToolContexts.set(agentId, { agent, toolRegistry: config.toolRegistry, @@ -277,12 +282,12 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { }, ); - // Merge any entries the onAgentInitialized callback did not receive - // (defensive: discoverConnectedAgents already returns a canonical map) + // Copy the pruned discovery result into the outer map. Anything the + // helper dropped (skipped or unreachable after edge filtering) is + // intentionally absent. `processAddedConvo` below may still add more + // entries for parallel multi-convo execution. for (const [agentId, config] of discoveredConfigs) { - if (!agentConfigs.has(agentId)) { - agentConfigs.set(agentId, config); - } + agentConfigs.set(agentId, config); } let userMCPAuthMap = discoveredMCPAuthMap; From e0003544a670ad884e0b28ced025713b8e69552c Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 18 Apr 2026 12:24:58 -0400 Subject: [PATCH 08/17] =?UTF-8?q?=F0=9F=94=AC=20fix:=20address=20audit=20f?= =?UTF-8?q?indings=20on=20discovery=20helper?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves findings from a comprehensive external audit of #12740. **Finding 1 (CRITICAL) — stale edges survive the reachability prune.** The pass-4 prune removed unreachable agents from `agentConfigs` but left matching edges in the return value. In an `A -> B -> C -> D` graph (all edges stored on A) where B is skipped, `filterOrphanedEdges` drops A->B and B->C but keeps C->D (neither endpoint is skipped). The caller then sees `agentConfigs` without C/D but `edges` still references them, flipping `createRun` into multi-agent mode with mismatched agents/edges — the exact crash this PR is supposed to fix. Now filter the edge list to the reachable set in the same pass, so the returned shape is self-consistent: every edge endpoint is either the primary id or a key of `agentConfigs`. New regression test covers A->B->C->D with B skipped. **Finding 2 (MAJOR) — unconditional `getModelsConfig` on every API request.** The OpenAI-compat and Responses controllers called `getModelsConfig(req)` and `discoverConnectedAgents` even when the primary agent had no edges (the common single-agent API case). Gate both behind `primaryConfig.edges?.length > 0` so single-agent runs don't pay that cost. **Finding 5 (MINOR) — silent mutation of caller's `primaryConfig.userMCPAuthMap`.** The helper aliased that object and then `Object.assign`'d sub-agent entries into it, changing the caller's config in-place. Shallow-clone up front so the returned merged map is the only destination. **Finding 7 (NIT) — dead `?? []` coalescing.** `filterOrphanedEdges` always returns a concrete array, so the `discoveredEdges ?? []` fallback was never reached. Simplified the `primaryConfig.edges = …` assignment. Also adds a test that verifies `primaryConfig.userMCPAuthMap` is not mutated in-place. --- api/server/controllers/agents/openai.js | 88 ++++++++++---------- api/server/controllers/agents/responses.js | 86 ++++++++++--------- packages/api/src/agents/discovery.spec.ts | 97 ++++++++++++++++++++++ packages/api/src/agents/discovery.ts | 45 +++++++--- 4 files changed, 224 insertions(+), 92 deletions(-) diff --git a/api/server/controllers/agents/openai.js b/api/server/controllers/agents/openai.js index 91e2b98617d0..e1f6f9c3f06e 100644 --- a/api/server/controllers/agents/openai.js +++ b/api/server/controllers/agents/openai.js @@ -261,50 +261,54 @@ const OpenAIChatCompletionController = async (req, res) => { actionsEnabled: primaryConfig.actionsEnabled, }); - const modelsConfig = await getModelsConfig(req); - const { - agentConfigs: handoffAgentConfigs, - edges: discoveredEdges, - userMCPAuthMap: discoveredMCPAuthMap, - } = await discoverConnectedAgents( - { - req, - res, - primaryConfig, - endpointOption, - allowedProviders, - modelsConfig, - loadTools, - requestFiles: [], - conversationId, - parentMessageId, - // The route enforces REMOTE_AGENT on the primary; every discovered - // sub-agent must clear the same sharing boundary, not the looser - // in-app AGENT one. - resourceType: ResourceType.REMOTE_AGENT, - }, - { - getAgent: db.getAgent, - checkPermission, - logViolation, - db: dbMethods, - onAgentInitialized: (agentId, handoffAgent, config) => { - agentToolContexts.set(agentId, { - agent: handoffAgent, - toolRegistry: config.toolRegistry, - userMCPAuthMap: config.userMCPAuthMap, - tool_resources: config.tool_resources, - actionsEnabled: config.actionsEnabled, - }); + // Only run BFS discovery (and pay `getModelsConfig` upfront) when the + // primary has edges to follow — the common API case is single-agent. + let handoffAgentConfigs = new Map(); + let discoveredEdges = []; + let discoveredMCPAuthMap; + if (primaryConfig.edges?.length) { + const modelsConfig = await getModelsConfig(req); + ({ + agentConfigs: handoffAgentConfigs, + edges: discoveredEdges, + userMCPAuthMap: discoveredMCPAuthMap, + } = await discoverConnectedAgents( + { + req, + res, + primaryConfig, + endpointOption, + allowedProviders, + modelsConfig, + loadTools, + requestFiles: [], + conversationId, + parentMessageId, + // The route enforces REMOTE_AGENT on the primary; every discovered + // sub-agent must clear the same sharing boundary, not the looser + // in-app AGENT one. + resourceType: ResourceType.REMOTE_AGENT, }, - initializeAgent, - }, - ); + { + getAgent: db.getAgent, + checkPermission, + logViolation, + db: dbMethods, + onAgentInitialized: (agentId, handoffAgent, config) => { + agentToolContexts.set(agentId, { + agent: handoffAgent, + toolRegistry: config.toolRegistry, + userMCPAuthMap: config.userMCPAuthMap, + tool_resources: config.tool_resources, + actionsEnabled: config.actionsEnabled, + }); + }, + initializeAgent, + }, + )); + } - // Ensure edges is an array when multi-agent mode is active - // (MultiAgentGraph.categorizeEdges requires edges to be iterable) - const edges = handoffAgentConfigs.size > 0 ? (discoveredEdges ?? []) : discoveredEdges; - primaryConfig.edges = edges; + primaryConfig.edges = discoveredEdges; // Determine if streaming is enabled (check both request and agent config) const streamingDisabled = !!primaryConfig.model_parameters?.disableStreaming; diff --git a/api/server/controllers/agents/responses.js b/api/server/controllers/agents/responses.js index f6530aebb095..ce494defe3f6 100644 --- a/api/server/controllers/agents/responses.js +++ b/api/server/controllers/agents/responses.js @@ -399,48 +399,54 @@ const createResponse = async (req, res) => { actionsEnabled: primaryConfig.actionsEnabled, }); - const modelsConfig = await getModelsConfig(req); - const { - agentConfigs: handoffAgentConfigs, - edges: discoveredEdges, - userMCPAuthMap: discoveredMCPAuthMap, - } = await discoverConnectedAgents( - { - req, - res, - primaryConfig, - endpointOption, - allowedProviders, - modelsConfig, - loadTools, - requestFiles: [], - conversationId, - parentMessageId, - // The route enforces REMOTE_AGENT on the primary; every discovered - // sub-agent must clear the same sharing boundary, not the looser - // in-app AGENT one. - resourceType: ResourceType.REMOTE_AGENT, - }, - { - getAgent: db.getAgent, - checkPermission, - logViolation, - db: dbMethods, - onAgentInitialized: (agentId, handoffAgent, config) => { - agentToolContexts.set(agentId, { - agent: handoffAgent, - toolRegistry: config.toolRegistry, - userMCPAuthMap: config.userMCPAuthMap, - tool_resources: config.tool_resources, - actionsEnabled: config.actionsEnabled, - }); + // Only run BFS discovery (and pay `getModelsConfig` upfront) when the + // primary has edges to follow — the common API case is single-agent. + let handoffAgentConfigs = new Map(); + let discoveredEdges = []; + let discoveredMCPAuthMap; + if (primaryConfig.edges?.length) { + const modelsConfig = await getModelsConfig(req); + ({ + agentConfigs: handoffAgentConfigs, + edges: discoveredEdges, + userMCPAuthMap: discoveredMCPAuthMap, + } = await discoverConnectedAgents( + { + req, + res, + primaryConfig, + endpointOption, + allowedProviders, + modelsConfig, + loadTools, + requestFiles: [], + conversationId, + parentMessageId, + // The route enforces REMOTE_AGENT on the primary; every discovered + // sub-agent must clear the same sharing boundary, not the looser + // in-app AGENT one. + resourceType: ResourceType.REMOTE_AGENT, }, - initializeAgent, - }, - ); + { + getAgent: db.getAgent, + checkPermission, + logViolation, + db: dbMethods, + onAgentInitialized: (agentId, handoffAgent, config) => { + agentToolContexts.set(agentId, { + agent: handoffAgent, + toolRegistry: config.toolRegistry, + userMCPAuthMap: config.userMCPAuthMap, + tool_resources: config.tool_resources, + actionsEnabled: config.actionsEnabled, + }); + }, + initializeAgent, + }, + )); + } - const edges = handoffAgentConfigs.size > 0 ? (discoveredEdges ?? []) : discoveredEdges; - primaryConfig.edges = edges; + primaryConfig.edges = discoveredEdges; const runAgents = [primaryConfig, ...handoffAgentConfigs.values()]; const mergedMCPAuthMap = discoveredMCPAuthMap ?? primaryConfig.userMCPAuthMap; diff --git a/packages/api/src/agents/discovery.spec.ts b/packages/api/src/agents/discovery.spec.ts index 5e57d547f5df..a952d979e178 100644 --- a/packages/api/src/agents/discovery.spec.ts +++ b/packages/api/src/agents/discovery.spec.ts @@ -311,6 +311,103 @@ describe('discoverConnectedAgents', () => { expect(result.agentConfigs.size).toBe(0); }); + it('prunes surviving-but-unreachable edges from the return value (A->B->C->D, B skipped)', async () => { + // All three edges are stored on the primary A. When B is skipped, + // `filterOrphanedEdges` removes A->B (to=B) and B->C (from=B) — but + // `C->D` has no skipped endpoint and would otherwise survive, + // referencing agents that the reachability pass then prunes from + // `agentConfigs`. That combination is the exact shape that re-raises + // the "Found edge ending at unknown node" crash in `createRun`. + const edges: GraphEdge[] = [ + { from: 'A', to: 'B', edgeType: 'handoff' }, + { from: 'B', to: 'C', edgeType: 'handoff' }, + { from: 'C', to: 'D', edgeType: 'handoff' }, + ]; + const primaryConfig = makeConfig('A', edges); + + const agentMap: Record = { + B: makeAgent('B', []), + C: makeAgent('C', []), + D: makeAgent('D', []), + }; + const getAgent = jest.fn(async ({ id }: { id: string }) => agentMap[id] ?? null); + const checkPermission = jest.fn( + async ({ resourceId }: { resourceId: unknown }) => resourceId !== 'mongo-B', + ); + + const result = await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + }, + ); + + expect(result.skippedAgentIds.has('B')).toBe(true); + expect(result.agentConfigs.has('C')).toBe(false); + expect(result.agentConfigs.has('D')).toBe(false); + // Every returned edge must reference only agents present in + // `agentConfigs` or the primary — no stranded `C->D` survives. + for (const edge of result.edges) { + const endpoints = [edge.from, edge.to].flatMap((v) => (Array.isArray(v) ? v : [v])); + for (const id of endpoints) { + expect(id === primaryConfig.id || result.agentConfigs.has(id)).toBe(true); + } + } + expect(result.edges).toHaveLength(0); + }); + + it('does not mutate the caller-supplied primaryConfig.userMCPAuthMap', async () => { + const primaryAuth = { serverA: { token: 'primary' } }; + const primaryConfig = makeConfig('A', [{ from: 'A', to: 'B', edgeType: 'handoff' }]); + primaryConfig.userMCPAuthMap = primaryAuth; + + const getAgent = jest.fn(async () => makeAgent('B', [])); + const checkPermission = jest.fn().mockResolvedValue(true); + + mockInitializeAgent.mockImplementation(async ({ agent }: { agent: Agent }) => { + const cfg = makeConfig(agent.id); + cfg.userMCPAuthMap = { serverB: { token: 'secondary' } }; + return cfg; + }); + + const result = await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + }, + ); + + // Caller's map is unchanged — only the returned merged map has the + // sub-agent entries. + expect(primaryConfig.userMCPAuthMap).toBe(primaryAuth); + expect(primaryConfig.userMCPAuthMap).toEqual({ serverA: { token: 'primary' } }); + expect(result.userMCPAuthMap).toEqual({ + serverA: { token: 'primary' }, + serverB: { token: 'secondary' }, + }); + expect(result.userMCPAuthMap).not.toBe(primaryAuth); + }); + it('keeps sub-agents that remain reachable via surviving edges', async () => { // Graph: A -> [B, C]; B is skipped, C is kept. C must remain because // A -> C survives filtering. diff --git a/packages/api/src/agents/discovery.ts b/packages/api/src/agents/discovery.ts index c07c85c64370..1db841c0c580 100644 --- a/packages/api/src/agents/discovery.ts +++ b/packages/api/src/agents/discovery.ts @@ -141,8 +141,10 @@ export async function discoverConnectedAgents( const agentConfigs = new Map(); const skippedAgentIds = new Set(); + // Shallow-clone so the sub-agent merges below don't silently mutate + // `primaryConfig.userMCPAuthMap` on the caller's object. let userMCPAuthMap: Record> | undefined = - primaryConfig.userMCPAuthMap; + primaryConfig.userMCPAuthMap ? { ...primaryConfig.userMCPAuthMap } : undefined; const markSkipped = (agentId: string): void => { skippedAgentIds.add(agentId); @@ -285,22 +287,37 @@ export async function discoverConnectedAgents( collectEdges(chain as unknown as GraphEdge[]); } - const edges = filterOrphanedEdges(Array.from(edgeMap.values()), skippedAgentIds); + const filteredEdges = filterOrphanedEdges(Array.from(edgeMap.values()), skippedAgentIds); /** - * Prune sub-agents that were initialized eagerly during BFS but whose - * connecting edges were later dropped by `filterOrphanedEdges` (e.g. an - * `A -> B -> C` chain where B is skipped, so both edges disappear but - * C had already been loaded from the primary's edge scan). Without this, - * those disconnected agents still flow into `createRun` — which flips - * into multi-agent mode whenever `agents.length > 1` and turns the - * stranded sub-agent into an unintended parallel start node. + * Compute the set of agent ids reachable from the primary through + * surviving edges, then prune both `agentConfigs` AND the edge list to + * that set. Two independent failure modes are in play here: + * + * 1. BFS eagerly initializes sub-agents discovered via the primary's + * edge scan. If an intermediate hop gets skipped (missing / no + * access), its neighbors can still be loaded — those neighbors + * become disconnected once the skipped hop's edges are removed. + * + * 2. `filterOrphanedEdges` only drops edges whose endpoints are in + * `skippedAgentIds`. Edges between two non-skipped but + * unreachable-from-primary agents (e.g. a `C -> D` tail left over + * after `A -> B -> C -> D` loses its B-adjacent edges) survive + * filtering but still reference agents that are about to be pruned + * from `agentConfigs`. Those edges would otherwise flow into + * `createRun`, flip it into multi-agent mode (edges.length > 0) + * with `agents.length === 1`, and trigger the exact + * `Found edge ending at unknown node` crash this PR fixes. + * + * Do both prunes together so the returned shape is self-consistent: + * every agent id referenced by an edge is guaranteed to be either + * `primaryConfig.id` or a key of `agentConfigs`. */ const reachable = new Set([primaryConfig.id]); const frontier: string[] = [primaryConfig.id]; while (frontier.length > 0) { const current = frontier.pop() as string; - for (const edge of edges) { + for (const edge of filteredEdges) { const sources = Array.isArray(edge.from) ? edge.from : [edge.from]; if (!sources.includes(current)) { continue; @@ -321,6 +338,14 @@ export async function discoverConnectedAgents( } } + const edgeEndpointIsReachable = (value: string | string[]): boolean => { + const ids = Array.isArray(value) ? value : [value]; + return ids.every((id) => typeof id !== 'string' || reachable.has(id)); + }; + const edges = filteredEdges.filter( + (edge) => edgeEndpointIsReachable(edge.from) && edgeEndpointIsReachable(edge.to), + ); + return { agentConfigs, edges, From 6450aa87511f5d2d2b4828112e51556df0963ecc Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 19 Apr 2026 22:44:08 -0400 Subject: [PATCH 09/17] =?UTF-8?q?=F0=9F=A7=B9=20chore:=20address=20audit?= =?UTF-8?q?=20NITs=20on=20discovery=20helper?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses two NIT findings from the post-fix audit: **F1** — the shallow clone on `primaryConfig.userMCPAuthMap` was only applied on the primary side; the `else` branch (hit when the primary had no MCP auth and the first sub-agent seeds the map) assigned the sub-agent's `config.userMCPAuthMap` directly, so a later sub-agent's `Object.assign` mutated the first one's map in place. Harmless in practice (per-request ephemeral objects) but asymmetric. Clone in the else branch too. Test added. **F2** — `initialize.js` had a defensive `if (agentConfigs.size > 0 && !edges) edges = []` normalizer. Pre-existing dead code: the helper now always returns a concrete array from `filteredEdges.filter(...)`. Removed for clarity. --- .../services/Endpoints/agents/initialize.js | 8 +-- packages/api/src/agents/discovery.spec.ts | 56 +++++++++++++++++++ packages/api/src/agents/discovery.ts | 7 ++- 3 files changed, 63 insertions(+), 8 deletions(-) diff --git a/api/server/services/Endpoints/agents/initialize.js b/api/server/services/Endpoints/agents/initialize.js index c8515bd0fa9a..549e9047b570 100644 --- a/api/server/services/Endpoints/agents/initialize.js +++ b/api/server/services/Endpoints/agents/initialize.js @@ -328,12 +328,8 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { }); } - // Ensure edges is an array when we have multiple agents (multi-agent mode) - // MultiAgentGraph.categorizeEdges requires edges to be iterable - if (agentConfigs.size > 0 && !edges) { - edges = []; - } - + // `discoverConnectedAgents` always returns a concrete array, so no + // further normalization is needed before handing this to `createRun`. primaryConfig.edges = edges; let endpointConfig = appConfig.endpoints?.[primaryConfig.endpoint]; diff --git a/packages/api/src/agents/discovery.spec.ts b/packages/api/src/agents/discovery.spec.ts index a952d979e178..7030d2a4672b 100644 --- a/packages/api/src/agents/discovery.spec.ts +++ b/packages/api/src/agents/discovery.spec.ts @@ -366,6 +366,62 @@ describe('discoverConnectedAgents', () => { expect(result.edges).toHaveLength(0); }); + it('does not mutate a sub-agent userMCPAuthMap when merging later sub-agents', async () => { + // Primary has no MCP auth; first sub-agent (B) seeds `userMCPAuthMap`, + // and the second sub-agent (C) merges into it. Make sure B's own + // `config.userMCPAuthMap` object is not mutated by C's merge. + const bAuth = { serverB: { token: 'b' } }; + const cAuth = { serverC: { token: 'c' } }; + const primaryConfig = makeConfig('A', [ + { from: 'A', to: 'B', edgeType: 'handoff' }, + { from: 'A', to: 'C', edgeType: 'handoff' }, + ]); + primaryConfig.userMCPAuthMap = undefined; + + const agentMap: Record = { + B: makeAgent('B', []), + C: makeAgent('C', []), + }; + const authById: Record>> = { + B: bAuth, + C: cAuth, + }; + + const getAgent = jest.fn(async ({ id }: { id: string }) => agentMap[id] ?? null); + const checkPermission = jest.fn().mockResolvedValue(true); + mockInitializeAgent.mockImplementation(async ({ agent }: { agent: Agent }) => { + const cfg = makeConfig(agent.id); + cfg.userMCPAuthMap = authById[agent.id]; + return cfg; + }); + + const result = await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + }, + ); + + // B's own map must not have picked up C's entry. + expect(bAuth).toEqual({ serverB: { token: 'b' } }); + expect(cAuth).toEqual({ serverC: { token: 'c' } }); + // The returned merged map should still contain both. + expect(result.userMCPAuthMap).toEqual({ + serverB: { token: 'b' }, + serverC: { token: 'c' }, + }); + }); + it('does not mutate the caller-supplied primaryConfig.userMCPAuthMap', async () => { const primaryAuth = { serverA: { token: 'primary' } }; const primaryConfig = makeConfig('A', [{ from: 'A', to: 'B', edgeType: 'handoff' }]); diff --git a/packages/api/src/agents/discovery.ts b/packages/api/src/agents/discovery.ts index 1db841c0c580..b04a36561529 100644 --- a/packages/api/src/agents/discovery.ts +++ b/packages/api/src/agents/discovery.ts @@ -229,8 +229,11 @@ export async function discoverConnectedAgents( if (userMCPAuthMap != null) { Object.assign(userMCPAuthMap, config.userMCPAuthMap ?? {}); - } else { - userMCPAuthMap = config.userMCPAuthMap; + } else if (config.userMCPAuthMap) { + // Clone so subsequent sub-agent merges don't mutate the first + // sub-agent's own `config.userMCPAuthMap` in place — symmetric with + // the shallow clone applied to the primary's map above. + userMCPAuthMap = { ...config.userMCPAuthMap }; } agentConfigs.set(agentId, config); From 4982f1c3b0f30ed7fb53d76573de36a6cf163250 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 19 Apr 2026 22:54:55 -0400 Subject: [PATCH 10/17] =?UTF-8?q?=F0=9F=95=B8=20fix:=20require=20all=20sou?= =?UTF-8?q?rces=20reachable=20when=20traversing=20fan-in=20edges?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the seventh P1 codex finding on #12740: the reachability BFS advanced through an edge as soon as any of its `from` endpoints matched the current frontier node (`sources.includes(current)`), but the subsequent edge filter required ALL sources to be reachable (`every`). The two-semantics mismatch let a fan-in edge like `{from: ['A','B'], to: 'C'}` mark C reachable purely via A even when B had no path from the primary, then drop the edge itself at filter time. Result: C survived in `agentConfigs` with no surviving edge connecting it to A, so `createRun` flipped into multi-agent mode on `agents.length > 1` and C ran as an unintended parallel root. Replace the BFS with a fixed-point iteration keyed on the same all-sources-reachable predicate used by the filter, so traversal and filtering stay aligned and multi-source edges only fire once every source is in the reachable set. Two regression tests added: - `{from: ['A','B'], to: 'C'}` with B having no incoming path — asserts neither B nor C leak into the result. - `A -> B`, `A -> C`, `['B','C'] -> D` — asserts the fan-in edge fires and D becomes reachable once both B and C are. --- packages/api/src/agents/discovery.spec.ts | 83 +++++++++++++++++++++++ packages/api/src/agents/discovery.ts | 35 +++++++--- 2 files changed, 107 insertions(+), 11 deletions(-) diff --git a/packages/api/src/agents/discovery.spec.ts b/packages/api/src/agents/discovery.spec.ts index 7030d2a4672b..85ad22430118 100644 --- a/packages/api/src/agents/discovery.spec.ts +++ b/packages/api/src/agents/discovery.spec.ts @@ -311,6 +311,89 @@ describe('discoverConnectedAgents', () => { expect(result.agentConfigs.size).toBe(0); }); + it('requires all sources to be reachable before advancing through a multi-source edge', async () => { + // Primary A has a single fan-in edge `{from: ['A','B'], to: 'C'}`. + // B loads successfully but has no incoming path from A, so the edge + // cannot legitimately fire. C must NOT be marked reachable (and thus + // must NOT end up in `agentConfigs`) — otherwise `createRun` sees + // `[primaryConfig, C]`, flips into multi-agent mode, and runs C as an + // unintended root because no edge connects A to C on its own. + const edges: GraphEdge[] = [{ from: ['A', 'B'], to: 'C', edgeType: 'direct' }]; + const primaryConfig = makeConfig('A', edges); + + const agentMap: Record = { + B: makeAgent('B', []), + C: makeAgent('C', []), + }; + const getAgent = jest.fn(async ({ id }: { id: string }) => agentMap[id] ?? null); + const checkPermission = jest.fn().mockResolvedValue(true); + + const result = await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + }, + ); + + // Neither B nor C is reachable from A through the surviving edge set + // (the fan-in edge requires B, which has no incoming path). + expect(result.agentConfigs.has('B')).toBe(false); + expect(result.agentConfigs.has('C')).toBe(false); + expect(result.edges).toHaveLength(0); + }); + + it('advances through a multi-source edge once all sources are reachable', async () => { + // Two independent paths converge: `A -> B` and `A -> C` both reach + // the fan-in edge `['B','C'] -> D`. Once both B and C are reachable, + // D should become reachable in a subsequent fixed-point iteration. + const edges: GraphEdge[] = [ + { from: 'A', to: 'B', edgeType: 'direct' }, + { from: 'A', to: 'C', edgeType: 'direct' }, + { from: ['B', 'C'], to: 'D', edgeType: 'direct' }, + ]; + const primaryConfig = makeConfig('A', edges); + + const agentMap: Record = { + B: makeAgent('B', []), + C: makeAgent('C', []), + D: makeAgent('D', []), + }; + const getAgent = jest.fn(async ({ id }: { id: string }) => agentMap[id] ?? null); + const checkPermission = jest.fn().mockResolvedValue(true); + + const result = await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + }, + ); + + expect(result.agentConfigs.has('B')).toBe(true); + expect(result.agentConfigs.has('C')).toBe(true); + expect(result.agentConfigs.has('D')).toBe(true); + expect(result.edges).toHaveLength(3); + }); + it('prunes surviving-but-unreachable edges from the return value (A->B->C->D, B skipped)', async () => { // All three edges are stored on the primary A. When B is skipped, // `filterOrphanedEdges` removes A->B (to=B) and B->C (from=B) — but diff --git a/packages/api/src/agents/discovery.ts b/packages/api/src/agents/discovery.ts index b04a36561529..0f12b6578898 100644 --- a/packages/api/src/agents/discovery.ts +++ b/packages/api/src/agents/discovery.ts @@ -316,20 +316,36 @@ export async function discoverConnectedAgents( * every agent id referenced by an edge is guaranteed to be either * `primaryConfig.id` or a key of `agentConfigs`. */ + const edgeEndpointIsReachable = ( + value: string | string[], + reachableSet: Set, + ): boolean => { + const ids = Array.isArray(value) ? value : [value]; + return ids.every((id) => typeof id !== 'string' || reachableSet.has(id)); + }; + + // Fixed-point reachability: an edge only advances reachability when ALL + // of its `from` endpoints are already reachable. Matches the edge-filter + // semantics below and handles multi-source / fan-in edges correctly: + // `{ from: ['A', 'B'], to: 'C' }` can only fire when both A and B reach + // it, so C shouldn't be marked reachable just because A is in the source + // list. The previous `sources.includes(current)` BFS over-approximated + // reachability for fan-in edges and left C in `agentConfigs` while the + // edge itself got dropped — `createRun` then saw `agents.length > 1` + // with a disconnected C and ran it as an unintended parallel root. const reachable = new Set([primaryConfig.id]); - const frontier: string[] = [primaryConfig.id]; - while (frontier.length > 0) { - const current = frontier.pop() as string; + let changed = true; + while (changed) { + changed = false; for (const edge of filteredEdges) { - const sources = Array.isArray(edge.from) ? edge.from : [edge.from]; - if (!sources.includes(current)) { + if (!edgeEndpointIsReachable(edge.from, reachable)) { continue; } const dests = Array.isArray(edge.to) ? edge.to : [edge.to]; for (const dest of dests) { if (typeof dest === 'string' && !reachable.has(dest)) { reachable.add(dest); - frontier.push(dest); + changed = true; } } } @@ -341,12 +357,9 @@ export async function discoverConnectedAgents( } } - const edgeEndpointIsReachable = (value: string | string[]): boolean => { - const ids = Array.isArray(value) ? value : [value]; - return ids.every((id) => typeof id !== 'string' || reachable.has(id)); - }; const edges = filteredEdges.filter( - (edge) => edgeEndpointIsReachable(edge.from) && edgeEndpointIsReachable(edge.to), + (edge) => + edgeEndpointIsReachable(edge.from, reachable) && edgeEndpointIsReachable(edge.to, reachable), ); return { From 6879f4599c737a10e25a29413a5e2450c8d3856e Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 19 Apr 2026 23:12:22 -0400 Subject: [PATCH 11/17] =?UTF-8?q?=F0=9F=94=80=20fix:=20match=20SDK=20OR=20?= =?UTF-8?q?semantics=20for=20multi-source=20edge=20reachability?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverts the all-sources-required reachability gate from 4982f1c3b and replaces it with an any-source-reachable model, which matches how `@librechat/agents`'s `MultiAgentGraph.createWorkflow` actually wires multi-source edges at runtime (per-source `builder.addEdge(source, destination)`). With the previous `every` gate, a legitimate handoff edge `{ from: ['A', 'B'], to: 'C' }` where B had no incoming path was pruned along with C, regressing OR-semantics routing that the SDK would otherwise handle correctly. New behavior: 1. Reachability: an edge advances when ANY of its `from` endpoints is already reachable. Fixed-point iteration over `filteredEdges`. 2. Edge filter: keep an edge when it has at least one reachable source AND all destinations are reachable (a missing destination would still crash `StateGraph.compile` with `Found edge ending at unknown node`). 3. Agent prune: keep agents that are reachable OR referenced on any endpoint of a surviving edge. The second clause preserves co-sources in multi-source edges (B in `{ from: ['A','B'], to: 'C' }` when nothing else reaches B) so the SDK's per-source `addEdge` — and the `validateEdgeAgents` safety-net I added to the SDK in #111 — still finds B as a node. The pass-audit A->B->C->D regression test continues to pass: with B skipped, `filterOrphanedEdges` drops both B-adjacent edges, reachability never expands past A, C->D has no reachable source so it gets filtered, and C/D are pruned because they're neither reachable nor referenced. --- packages/api/src/agents/discovery.spec.ts | 30 ++++---- packages/api/src/agents/discovery.ts | 87 ++++++++++++----------- 2 files changed, 64 insertions(+), 53 deletions(-) diff --git a/packages/api/src/agents/discovery.spec.ts b/packages/api/src/agents/discovery.spec.ts index 85ad22430118..84712cf76d1d 100644 --- a/packages/api/src/agents/discovery.spec.ts +++ b/packages/api/src/agents/discovery.spec.ts @@ -311,14 +311,18 @@ describe('discoverConnectedAgents', () => { expect(result.agentConfigs.size).toBe(0); }); - it('requires all sources to be reachable before advancing through a multi-source edge', async () => { - // Primary A has a single fan-in edge `{from: ['A','B'], to: 'C'}`. - // B loads successfully but has no incoming path from A, so the edge - // cannot legitimately fire. C must NOT be marked reachable (and thus - // must NOT end up in `agentConfigs`) — otherwise `createRun` sees - // `[primaryConfig, C]`, flips into multi-agent mode, and runs C as an - // unintended root because no edge connects A to C on its own. - const edges: GraphEdge[] = [{ from: ['A', 'B'], to: 'C', edgeType: 'direct' }]; + it('advances through a multi-source edge on ANY reachable source (SDK OR semantics)', async () => { + // Primary A has a single edge `{from: ['A','B'], to: 'C'}`. B loads + // successfully but has no incoming path from A. The agents SDK adds + // one LangGraph edge per `from` source (see + // `agents/src/graphs/MultiAgentGraph.ts`), so `A -> C` is a real + // routing regardless of B's reachability. Discovery must preserve it: + // - C reachable via A + // - edge kept (A source reachable, C dest reachable) + // - B kept in agentConfigs because it's still referenced by the + // surviving edge; otherwise the SDK's `validateEdgeAgents` + // safety-net rejects the graph for a missing `from` endpoint. + const edges: GraphEdge[] = [{ from: ['A', 'B'], to: 'C', edgeType: 'handoff' }]; const primaryConfig = makeConfig('A', edges); const agentMap: Record = { @@ -345,11 +349,11 @@ describe('discoverConnectedAgents', () => { }, ); - // Neither B nor C is reachable from A through the surviving edge set - // (the fan-in edge requires B, which has no incoming path). - expect(result.agentConfigs.has('B')).toBe(false); - expect(result.agentConfigs.has('C')).toBe(false); - expect(result.edges).toHaveLength(0); + expect(result.agentConfigs.has('B')).toBe(true); + expect(result.agentConfigs.has('C')).toBe(true); + expect(result.edges).toHaveLength(1); + expect(result.edges[0].from).toEqual(['A', 'B']); + expect(result.edges[0].to).toBe('C'); }); it('advances through a multi-source edge once all sources are reachable', async () => { diff --git a/packages/api/src/agents/discovery.ts b/packages/api/src/agents/discovery.ts index 0f12b6578898..ad2c8d5f805f 100644 --- a/packages/api/src/agents/discovery.ts +++ b/packages/api/src/agents/discovery.ts @@ -293,52 +293,47 @@ export async function discoverConnectedAgents( const filteredEdges = filterOrphanedEdges(Array.from(edgeMap.values()), skippedAgentIds); /** - * Compute the set of agent ids reachable from the primary through - * surviving edges, then prune both `agentConfigs` AND the edge list to - * that set. Two independent failure modes are in play here: + * Keep discovery's reachability model aligned with the agents SDK's + * runtime semantics. `MultiAgentGraph.createWorkflow` adds one + * LangGraph edge per `from` source (see + * `src/graphs/MultiAgentGraph.ts`), so a multi-source edge + * `{ from: ['A', 'B'], to: 'C' }` is really `A -> C` OR `B -> C` — + * either source firing routes to `C`. The reachability pass therefore + * advances through an edge whenever ANY of its sources is already + * reachable. * - * 1. BFS eagerly initializes sub-agents discovered via the primary's - * edge scan. If an intermediate hop gets skipped (missing / no - * access), its neighbors can still be loaded — those neighbors - * become disconnected once the skipped hop's edges are removed. + * The edge filter then keeps an edge when it has at least one reachable + * source AND all of its destinations are reachable (a destination not + * present in `agentConfigs` would still trigger the + * `Found edge ending at unknown node` crash this PR fixes). * - * 2. `filterOrphanedEdges` only drops edges whose endpoints are in - * `skippedAgentIds`. Edges between two non-skipped but - * unreachable-from-primary agents (e.g. a `C -> D` tail left over - * after `A -> B -> C -> D` loses its B-adjacent edges) survive - * filtering but still reference agents that are about to be pruned - * from `agentConfigs`. Those edges would otherwise flow into - * `createRun`, flip it into multi-agent mode (edges.length > 0) - * with `agents.length === 1`, and trigger the exact - * `Found edge ending at unknown node` crash this PR fixes. - * - * Do both prunes together so the returned shape is self-consistent: - * every agent id referenced by an edge is guaranteed to be either - * `primaryConfig.id` or a key of `agentConfigs`. + * Finally, prune `agentConfigs`: keep anything reachable from the + * primary OR referenced on any endpoint of a surviving edge. The + * second clause covers co-sources in multi-source edges that have no + * incoming path of their own — e.g. B in `{ from: ['A', 'B'], to: 'C' }` + * when nothing reaches B. The SDK's per-source `addEdge` still + * requires B to be present as a node, and my companion + * `validateEdgeAgents` safety-net fails loudly if it isn't. Keeping B + * in `agentConfigs` cedes that decision to the SDK runtime, which + * handles the standalone starting-node case gracefully; it does not + * re-introduce the `createRun` crash this PR exists to prevent, + * because the edge is no longer orphaned with respect to its agents. */ - const edgeEndpointIsReachable = ( - value: string | string[], - reachableSet: Set, - ): boolean => { + const anyReachable = (value: string | string[], reachableSet: Set): boolean => { + const ids = Array.isArray(value) ? value : [value]; + return ids.some((id) => typeof id === 'string' && reachableSet.has(id)); + }; + const allReachable = (value: string | string[], reachableSet: Set): boolean => { const ids = Array.isArray(value) ? value : [value]; return ids.every((id) => typeof id !== 'string' || reachableSet.has(id)); }; - // Fixed-point reachability: an edge only advances reachability when ALL - // of its `from` endpoints are already reachable. Matches the edge-filter - // semantics below and handles multi-source / fan-in edges correctly: - // `{ from: ['A', 'B'], to: 'C' }` can only fire when both A and B reach - // it, so C shouldn't be marked reachable just because A is in the source - // list. The previous `sources.includes(current)` BFS over-approximated - // reachability for fan-in edges and left C in `agentConfigs` while the - // edge itself got dropped — `createRun` then saw `agents.length > 1` - // with a disconnected C and ran it as an unintended parallel root. const reachable = new Set([primaryConfig.id]); let changed = true; while (changed) { changed = false; for (const edge of filteredEdges) { - if (!edgeEndpointIsReachable(edge.from, reachable)) { + if (!anyReachable(edge.from, reachable)) { continue; } const dests = Array.isArray(edge.to) ? edge.to : [edge.to]; @@ -351,17 +346,29 @@ export async function discoverConnectedAgents( } } + const edges = filteredEdges.filter( + (edge) => anyReachable(edge.from, reachable) && allReachable(edge.to, reachable), + ); + + const referencedByEdge = new Set(); + for (const edge of edges) { + const endpoints = [ + ...(Array.isArray(edge.from) ? edge.from : [edge.from]), + ...(Array.isArray(edge.to) ? edge.to : [edge.to]), + ]; + for (const id of endpoints) { + if (typeof id === 'string') { + referencedByEdge.add(id); + } + } + } + for (const agentId of [...agentConfigs.keys()]) { - if (!reachable.has(agentId)) { + if (!reachable.has(agentId) && !referencedByEdge.has(agentId)) { agentConfigs.delete(agentId); } } - const edges = filteredEdges.filter( - (edge) => - edgeEndpointIsReachable(edge.from, reachable) && edgeEndpointIsReachable(edge.to, reachable), - ); - return { agentConfigs, edges, From 222716d193a57e4609d91b5903bceab5e3fba374 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 19 Apr 2026 23:25:22 -0400 Subject: [PATCH 12/17] =?UTF-8?q?=E2=9C=82=EF=B8=8F=20fix:=20strip=20skipp?= =?UTF-8?q?ed=20co-members=20from=20multi-source/multi-dest=20edges?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses codex pass-9 P2 on #12740. `filterOrphanedEdges` previously dropped an edge whenever any `from` id was skipped, which was correct for scalar edges but over-aggressive for multi-source ones: the agents SDK adds one `builder.addEdge(source, destination)` per source, so `{ from: ['A','B'], to: 'C' }` with B skipped still has a valid `A -> C` route that was being thrown away. Now sanitize each endpoint: - Scalar skipped → drop the whole edge (no route survives). - Array with some skipped → strip the skipped ids, keep the edge with the surviving members. If the array empties out, drop the edge. Symmetric handling for `to` covers multi-destination fan-out when one co-destination is skipped. Tests updated/added: - `strips skipped co-sources from multi-source edges…` - `strips skipped co-destinations from multi-destination edges` - `drops multi-member edges only when every member on a side is skipped` - Discovery-side: `preserves valid routes when one co-source of a multi-source edge is skipped` asserts the end-to-end behavior — skipped co-source B gets stripped from the edge, A->C routing survives, and C remains in `agentConfigs`. --- packages/api/src/agents/discovery.spec.ts | 43 ++++++++++++ packages/api/src/agents/edges.spec.ts | 32 +++++++-- packages/api/src/agents/edges.ts | 79 ++++++++++++++++++++--- 3 files changed, 138 insertions(+), 16 deletions(-) diff --git a/packages/api/src/agents/discovery.spec.ts b/packages/api/src/agents/discovery.spec.ts index 84712cf76d1d..5019d273d3f5 100644 --- a/packages/api/src/agents/discovery.spec.ts +++ b/packages/api/src/agents/discovery.spec.ts @@ -311,6 +311,49 @@ describe('discoverConnectedAgents', () => { expect(result.agentConfigs.size).toBe(0); }); + it('preserves valid routes when one co-source of a multi-source edge is skipped', async () => { + // Primary A has edge `{ from: ['A','B'], to: 'C' }`, but B lacks + // VIEW permission. The SDK treats each source independently, so the + // `A -> C` route is still valid. Discovery must strip B from the + // edge's sources rather than dropping the whole edge. + const edges: GraphEdge[] = [{ from: ['A', 'B'], to: 'C', edgeType: 'handoff' }]; + const primaryConfig = makeConfig('A', edges); + + const agentMap: Record = { + B: makeAgent('B', []), + C: makeAgent('C', []), + }; + const getAgent = jest.fn(async ({ id }: { id: string }) => agentMap[id] ?? null); + // Only B is forbidden. + const checkPermission = jest.fn( + async ({ resourceId }: { resourceId: unknown }) => resourceId !== 'mongo-B', + ); + + const result = await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + }, + ); + + expect(result.skippedAgentIds.has('B')).toBe(true); + expect(result.agentConfigs.has('C')).toBe(true); + expect(result.agentConfigs.has('B')).toBe(false); + expect(result.edges).toHaveLength(1); + expect(result.edges[0].from).toEqual(['A']); + expect(result.edges[0].to).toBe('C'); + }); + it('advances through a multi-source edge on ANY reachable source (SDK OR semantics)', async () => { // Primary A has a single edge `{from: ['A','B'], to: 'C'}`. B loads // successfully but has no incoming path from A. The agents SDK adds diff --git a/packages/api/src/agents/edges.spec.ts b/packages/api/src/agents/edges.spec.ts index 8912c729b90a..447c65fdb557 100644 --- a/packages/api/src/agents/edges.spec.ts +++ b/packages/api/src/agents/edges.spec.ts @@ -151,26 +151,46 @@ describe('edges utilities', () => { expect(result).toHaveLength(0); }); - it('should filter edges whose `from` is an array containing a skipped id', () => { + it('strips skipped co-sources from multi-source edges instead of dropping the whole edge', () => { + // The SDK adds one `builder.addEdge(source, dest)` per source, so + // `agent_a -> agent_d` is a valid route even when `agent_b` can't + // be loaded. Keep the edge with the surviving sources. const edgesWithArrayFrom: GraphEdge[] = [ { from: ['agent_a', 'agent_b'], to: 'agent_d', edgeType: 'handoff' }, { from: 'agent_a', to: 'agent_d', edgeType: 'handoff' }, ]; const skipped = new Set(['agent_b']); const result = filterOrphanedEdges(edgesWithArrayFrom, skipped); - expect(result).toHaveLength(1); - expect(result[0].from).toBe('agent_a'); + expect(result).toHaveLength(2); + expect(result[0].from).toEqual(['agent_a']); + expect(result[0].to).toBe('agent_d'); + expect(result[1].from).toBe('agent_a'); }); - it('should handle array to values', () => { + it('strips skipped co-destinations from multi-destination edges', () => { const edgesWithArray: GraphEdge[] = [ { from: 'agent_a', to: ['agent_b', 'agent_c'], edgeType: 'handoff' }, { from: 'agent_a', to: ['agent_d'], edgeType: 'handoff' }, ]; const skipped = new Set(['agent_b']); const result = filterOrphanedEdges(edgesWithArray, skipped); - expect(result).toHaveLength(1); - expect(result[0].to).toEqual(['agent_d']); + // First edge: `['agent_b','agent_c']` → `['agent_c']` — kept. + // Second edge: unchanged. + expect(result).toHaveLength(2); + expect(result[0].to).toEqual(['agent_c']); + expect(result[1].to).toEqual(['agent_d']); + }); + + it('drops multi-member edges only when every member on a side is skipped', () => { + const edgesSide: GraphEdge[] = [ + // All sources skipped → drop. + { from: ['agent_b', 'agent_c'], to: 'agent_a', edgeType: 'handoff' }, + // All destinations skipped → drop. + { from: 'agent_a', to: ['agent_b', 'agent_c'], edgeType: 'handoff' }, + ]; + const skipped = new Set(['agent_b', 'agent_c']); + const result = filterOrphanedEdges(edgesSide, skipped); + expect(result).toHaveLength(0); }); it('should return original edges array when edges is null/undefined', () => { diff --git a/packages/api/src/agents/edges.ts b/packages/api/src/agents/edges.ts index 2e6d4324e553..9bc3705c608c 100644 --- a/packages/api/src/agents/edges.ts +++ b/packages/api/src/agents/edges.ts @@ -30,21 +30,80 @@ export function getEdgeParticipants(edge: GraphEdge): string[] { } /** - * Filters out edges that reference non-existent (orphaned) agents on - * either endpoint. Any edge whose `from` OR `to` references a skipped - * agent id is dropped — leaving a source-side orphan in place would - * still fail graph compilation with `Found edge ending at unknown node` - * because `agents` no longer contains the source. + * Drops skipped agent ids from an edge endpoint. + * + * Scalars: returns the value unchanged, or `null` if the skipped set + * contains it (the edge has lost its only endpoint on that side and + * must be dropped by the caller). + * + * Arrays: returns a new array with skipped ids removed; if every id was + * skipped, returns `null`. A single-element result stays an array — the + * SDK normalizes both shapes internally, so preserving array-ness is + * simpler than switching representation. + * + * Returns the original reference when nothing changed, so callers can + * cheaply detect and skip a re-spread. + */ +function sanitizeEndpoint( + value: string | string[], + skippedAgentIds: Set, +): string | string[] | null { + if (Array.isArray(value)) { + const kept: string[] = []; + let removed = false; + for (const id of value) { + if (typeof id === 'string' && skippedAgentIds.has(id)) { + removed = true; + continue; + } + kept.push(id); + } + if (kept.length === 0) { + return null; + } + return removed ? kept : value; + } + if (typeof value === 'string' && skippedAgentIds.has(value)) { + return null; + } + return value; +} + +/** + * Drops orphaned agents from edge endpoints, returning only the edges + * that still have at least one valid source and one valid destination. + * + * For multi-source / multi-destination edges, orphaned ids are stripped + * from the array rather than dropping the whole edge: `{ from: ['A','B'], + * to: 'C' }` with B skipped becomes `{ from: ['A'], to: 'C' }`. This + * matches the agents SDK's `MultiAgentGraph.createWorkflow`, which adds + * one LangGraph edge per source/destination pair (`builder.addEdge(src, + * dest)`), so losing one co-source (or one co-destination) doesn't + * invalidate the routes through the remaining members. + * + * An edge is dropped only when it has no surviving source OR no + * surviving destination — the SDK's per-source `addEdge` can't run if + * either side is empty, and any orphaned id left in place would still + * trigger `Found edge ending at unknown node` at compile time. */ export function filterOrphanedEdges(edges: GraphEdge[], skippedAgentIds: Set): GraphEdge[] { if (!edges || skippedAgentIds.size === 0) { return edges; } - const referencesSkipped = (value: string | string[]): boolean => { - const ids = Array.isArray(value) ? value : [value]; - return ids.some((id) => typeof id === 'string' && skippedAgentIds.has(id)); - }; - return edges.filter((edge) => !referencesSkipped(edge.from) && !referencesSkipped(edge.to)); + const result: GraphEdge[] = []; + for (const edge of edges) { + const from = sanitizeEndpoint(edge.from, skippedAgentIds); + const to = sanitizeEndpoint(edge.to, skippedAgentIds); + if (from === null || to === null) { + continue; + } + if (from === edge.from && to === edge.to) { + result.push(edge); + } else { + result.push({ ...edge, from, to }); + } + } + return result; } /** Collects all unique agent IDs referenced across an array of edges. */ From 0de3684be911c4f5ab0ca03b4bfd9375bd258d11 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 19 Apr 2026 23:41:40 -0400 Subject: [PATCH 13/17] =?UTF-8?q?=F0=9F=94=93=20fix:=20respect=20SHARE-on-?= =?UTF-8?q?AGENT=20fallback=20for=20handoff=20ACL=20on=20API=20routes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses codex pass-10 P1 on #12740. The API controllers were handing `discoverConnectedAgents` a raw `PermissionService.checkPermission` call against `ResourceType.REMOTE_AGENT`, but the route-level middleware (`createCheckRemoteAgentAccess`) authorizes the primary agent via `getRemoteAgentPermissions`, which first consults the AGENT ACL and treats owners with the SHARE bit as remotely authorized even without an explicit REMOTE_AGENT grant. The mismatch meant a user could open the primary via `/v1/chat/completions` or `/v1/responses`, but their own owned handoff sub-agents were silently skipped — breaking multi-agent handoffs for the common "owner runs their own multi-agent orchestrator" case. Both controllers now pass `discoverConnectedAgents` a `checkPermission` wrapper that delegates to `getRemoteAgentPermissions` (with `getEffectivePermissions` injected from `PermissionService`) and compares the returned bitmask against the required permission via `hasPermissions`. Sub-agents are now authorized by the exact same rules the route middleware applies to the primary. --- api/server/controllers/agents/openai.js | 28 +++++++++++++++++++--- api/server/controllers/agents/responses.js | 28 +++++++++++++++++++--- 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/api/server/controllers/agents/openai.js b/api/server/controllers/agents/openai.js index e1f6f9c3f06e..f48ca410d090 100644 --- a/api/server/controllers/agents/openai.js +++ b/api/server/controllers/agents/openai.js @@ -1,7 +1,12 @@ const { nanoid } = require('nanoid'); const { logger } = require('@librechat/data-schemas'); const { Callback, ToolEndHandler, formatAgentMessages } = require('@librechat/agents'); -const { EModelEndpoint, ResourceType, PermissionBits } = require('librechat-data-provider'); +const { + EModelEndpoint, + ResourceType, + PermissionBits, + hasPermissions, +} = require('librechat-data-provider'); const { writeSSE, createRun, @@ -22,6 +27,7 @@ const { createOpenAIContentAggregator, isChatCompletionValidationFailure, discoverConnectedAgents, + getRemoteAgentPermissions, } = require('@librechat/api'); const { buildSummarizationHandlers, @@ -30,7 +36,10 @@ const { agentLogHandlerObj, } = require('~/server/controllers/agents/callbacks'); const { loadAgentTools, loadToolsForExecution } = require('~/server/services/ToolService'); -const { findAccessibleResources, checkPermission } = require('~/server/services/PermissionService'); +const { + findAccessibleResources, + getEffectivePermissions, +} = require('~/server/services/PermissionService'); const { getModelsConfig } = require('~/server/controllers/ModelController'); const { filterFilesByAgentAccess } = require('~/server/services/Files/permissions'); const { logViolation } = require('~/cache'); @@ -291,7 +300,20 @@ const OpenAIChatCompletionController = async (req, res) => { }, { getAgent: db.getAgent, - checkPermission, + // Use `getRemoteAgentPermissions` so sub-agent authorization + // matches what the route's `createCheckRemoteAgentAccess` + // middleware does for the primary: AGENT owners with the SHARE + // bit are treated as remotely authorized even without an + // explicit REMOTE_AGENT grant. + checkPermission: async ({ userId, role, resourceId, requiredPermission }) => { + const permissions = await getRemoteAgentPermissions( + { getEffectivePermissions }, + userId, + role, + resourceId, + ); + return hasPermissions(permissions, requiredPermission); + }, logViolation, db: dbMethods, onAgentInitialized: (agentId, handoffAgent, config) => { diff --git a/api/server/controllers/agents/responses.js b/api/server/controllers/agents/responses.js index ce494defe3f6..cfb49c748fa6 100644 --- a/api/server/controllers/agents/responses.js +++ b/api/server/controllers/agents/responses.js @@ -2,7 +2,12 @@ const { nanoid } = require('nanoid'); const { v4: uuidv4 } = require('uuid'); const { logger } = require('@librechat/data-schemas'); const { Callback, ToolEndHandler, formatAgentMessages } = require('@librechat/agents'); -const { EModelEndpoint, ResourceType, PermissionBits } = require('librechat-data-provider'); +const { + EModelEndpoint, + ResourceType, + PermissionBits, + hasPermissions, +} = require('librechat-data-provider'); const { createRun, buildToolSet, @@ -13,6 +18,7 @@ const { getTransactionsConfig, createToolExecuteHandler, discoverConnectedAgents, + getRemoteAgentPermissions, // Responses API writeDone, buildResponse, @@ -39,7 +45,10 @@ const { agentLogHandlerObj, } = require('~/server/controllers/agents/callbacks'); const { loadAgentTools, loadToolsForExecution } = require('~/server/services/ToolService'); -const { findAccessibleResources, checkPermission } = require('~/server/services/PermissionService'); +const { + findAccessibleResources, + getEffectivePermissions, +} = require('~/server/services/PermissionService'); const { getModelsConfig } = require('~/server/controllers/ModelController'); const { filterFilesByAgentAccess } = require('~/server/services/Files/permissions'); const { logViolation } = require('~/cache'); @@ -429,7 +438,20 @@ const createResponse = async (req, res) => { }, { getAgent: db.getAgent, - checkPermission, + // Use `getRemoteAgentPermissions` so sub-agent authorization + // matches what the route's `createCheckRemoteAgentAccess` + // middleware does for the primary: AGENT owners with the SHARE + // bit are treated as remotely authorized even without an + // explicit REMOTE_AGENT grant. + checkPermission: async ({ userId, role, resourceId, requiredPermission }) => { + const permissions = await getRemoteAgentPermissions( + { getEffectivePermissions }, + userId, + role, + resourceId, + ); + return hasPermissions(permissions, requiredPermission); + }, logViolation, db: dbMethods, onAgentInitialized: (agentId, handoffAgent, config) => { From 7cc7aedea0f73a71bed67ab8e1a7dd624b27d668 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 20 Apr 2026 00:07:53 -0400 Subject: [PATCH 14/17] =?UTF-8?q?=F0=9F=8C=B1=20fix:=20preserve=20user-def?= =?UTF-8?q?ined=20parallel-start=20branches?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses codex pass-11 P2 on #12740. The post-filter reachability prune seeded only from `primaryConfig.id`, which killed `MultiAgentGraph`'s legitimate multi-start pattern — a user-defined edge like `X -> Y` where X has no incoming path (X is an intentional parallel starting node, run alongside the primary) was being dropped because neither X nor Y was reachable from the primary. Reconcile the tension with pass-4 ("prune accidental orphans when an intermediate is skipped") by using pre-filter reachability as the signal: - An agent that WAS reachable from the primary via the original (pre-filter) edges but loses that path when `filterOrphanedEdges` runs is an accidental orphan (a skipped hop broke the chain) — prune. - An agent that was NEVER reachable from the primary, even pre-filter, is an intentional parallel start — seed it into post-filter reachability so its component survives. Surviving-edge endpoint references still keep an agent (co-sources in multi-source edges). New test `preserves user-defined parallel-start branches disconnected from the primary` covers the pass-11 scenario; the existing `A->B->C->D, B skipped` regression test continues to pass because C/D were pre-filter reachable through B and lose that reachability after filtering. --- packages/api/src/agents/discovery.spec.ts | 43 ++++++++++ packages/api/src/agents/discovery.ts | 100 ++++++++++++++-------- 2 files changed, 108 insertions(+), 35 deletions(-) diff --git a/packages/api/src/agents/discovery.spec.ts b/packages/api/src/agents/discovery.spec.ts index 5019d273d3f5..158580b9422f 100644 --- a/packages/api/src/agents/discovery.spec.ts +++ b/packages/api/src/agents/discovery.spec.ts @@ -441,6 +441,49 @@ describe('discoverConnectedAgents', () => { expect(result.edges).toHaveLength(3); }); + it('preserves user-defined parallel-start branches disconnected from the primary', async () => { + // Primary A has edges `[A -> B, X -> Y]`. The `X -> Y` branch is + // intentionally disconnected from A — `MultiAgentGraph.analyzeGraph` + // treats X as a starting node (no incoming edges) and runs it in + // parallel with A. Discovery must keep this branch intact. + const edges: GraphEdge[] = [ + { from: 'A', to: 'B', edgeType: 'handoff' }, + { from: 'X', to: 'Y', edgeType: 'direct' }, + ]; + const primaryConfig = makeConfig('A', edges); + + const agentMap: Record = { + B: makeAgent('B', []), + X: makeAgent('X', []), + Y: makeAgent('Y', []), + }; + const getAgent = jest.fn(async ({ id }: { id: string }) => agentMap[id] ?? null); + const checkPermission = jest.fn().mockResolvedValue(true); + + const result = await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + }, + ); + + expect(result.agentConfigs.has('B')).toBe(true); + expect(result.agentConfigs.has('X')).toBe(true); + expect(result.agentConfigs.has('Y')).toBe(true); + expect(result.edges).toHaveLength(2); + expect(result.edges.map((e) => e.to).sort()).toEqual(['B', 'Y']); + }); + it('prunes surviving-but-unreachable edges from the return value (A->B->C->D, B skipped)', async () => { // All three edges are stored on the primary A. When B is skipped, // `filterOrphanedEdges` removes A->B (to=B) and B->C (from=B) — but diff --git a/packages/api/src/agents/discovery.ts b/packages/api/src/agents/discovery.ts index ad2c8d5f805f..25c8a00a8831 100644 --- a/packages/api/src/agents/discovery.ts +++ b/packages/api/src/agents/discovery.ts @@ -290,34 +290,51 @@ export async function discoverConnectedAgents( collectEdges(chain as unknown as GraphEdge[]); } - const filteredEdges = filterOrphanedEdges(Array.from(edgeMap.values()), skippedAgentIds); + const preFilterEdges = Array.from(edgeMap.values()); + const filteredEdges = filterOrphanedEdges(preFilterEdges, skippedAgentIds); /** * Keep discovery's reachability model aligned with the agents SDK's * runtime semantics. `MultiAgentGraph.createWorkflow` adds one - * LangGraph edge per `from` source (see - * `src/graphs/MultiAgentGraph.ts`), so a multi-source edge + * LangGraph edge per `from` source, so a multi-source edge * `{ from: ['A', 'B'], to: 'C' }` is really `A -> C` OR `B -> C` — - * either source firing routes to `C`. The reachability pass therefore - * advances through an edge whenever ANY of its sources is already - * reachable. + * either source firing routes to `C`. Reachability therefore advances + * through an edge whenever ANY of its sources is already reachable. * - * The edge filter then keeps an edge when it has at least one reachable - * source AND all of its destinations are reachable (a destination not - * present in `agentConfigs` would still trigger the - * `Found edge ending at unknown node` crash this PR fixes). + * Two semantics to reconcile when pruning after orphan-filter: * - * Finally, prune `agentConfigs`: keep anything reachable from the - * primary OR referenced on any endpoint of a surviving edge. The - * second clause covers co-sources in multi-source edges that have no - * incoming path of their own — e.g. B in `{ from: ['A', 'B'], to: 'C' }` - * when nothing reaches B. The SDK's per-source `addEdge` still - * requires B to be present as a node, and my companion - * `validateEdgeAgents` safety-net fails loudly if it isn't. Keeping B - * in `agentConfigs` cedes that decision to the SDK runtime, which - * handles the standalone starting-node case gracefully; it does not - * re-introduce the `createRun` crash this PR exists to prevent, - * because the edge is no longer orphaned with respect to its agents. + * 1. Accidental orphans — agents loaded via BFS from the primary's + * edges that lost their only path when an intermediate agent was + * skipped (e.g. `A -> B -> C` with B skipped leaves C stranded). + * These should be pruned; leaving them flips `createRun` into + * multi-agent mode with a disconnected C and the SDK runs C as an + * unintended parallel root. + * + * 2. Intentional multi-start branches — agents referenced by edges the + * user explicitly defined without wiring them to the primary + * (e.g. `A -> B` plus `X -> Y` as two independent starting + * branches). The SDK's `MultiAgentGraph.analyzeGraph` treats + * `no-incoming-edge` agents as start nodes, so these run in + * parallel with the primary by design. These must be preserved. + * + * Both cases produce the same post-filter topology (a component + * disconnected from the primary), but pre-filter they look different: + * accidental orphans WERE reachable from the primary before + * `filterOrphanedEdges` ran; intentional parallel starts were never + * reachable. Use that signal: + * + * - Seed post-filter reachability with the primary AND every agent + * in `agentConfigs` that was NOT reachable from the primary via + * the pre-filter edges. That treats intentional parallel starts + * as roots. + * - Agents that WERE pre-filter reachable but aren't any more have + * lost their connecting edge — they're pruned. + * - Surviving edges are filtered to the post-filter reachable set + * so no stale edge references a pruned agent. + * - Agents referenced as an endpoint in a surviving edge are always + * kept (a multi-source edge co-source like B in + * `{ from: ['A','B'], to: 'C' }` where nothing reaches B still + * needs B present for the SDK's per-source `addEdge` to compile). */ const anyReachable = (value: string | string[], reachableSet: Set): boolean => { const ids = Array.isArray(value) ? value : [value]; @@ -327,25 +344,38 @@ export async function discoverConnectedAgents( const ids = Array.isArray(value) ? value : [value]; return ids.every((id) => typeof id !== 'string' || reachableSet.has(id)); }; - - const reachable = new Set([primaryConfig.id]); - let changed = true; - while (changed) { - changed = false; - for (const edge of filteredEdges) { - if (!anyReachable(edge.from, reachable)) { - continue; - } - const dests = Array.isArray(edge.to) ? edge.to : [edge.to]; - for (const dest of dests) { - if (typeof dest === 'string' && !reachable.has(dest)) { - reachable.add(dest); - changed = true; + const expandReachable = (seeds: Set, edgeList: GraphEdge[]): Set => { + const result = new Set(seeds); + let changed = true; + while (changed) { + changed = false; + for (const edge of edgeList) { + if (!anyReachable(edge.from, result)) { + continue; + } + const dests = Array.isArray(edge.to) ? edge.to : [edge.to]; + for (const dest of dests) { + if (typeof dest === 'string' && !result.has(dest)) { + result.add(dest); + changed = true; + } } } } + return result; + }; + + const preFilterReachable = expandReachable(new Set([primaryConfig.id]), preFilterEdges); + + const postFilterSeeds = new Set([primaryConfig.id]); + for (const agentId of agentConfigs.keys()) { + if (!preFilterReachable.has(agentId)) { + postFilterSeeds.add(agentId); + } } + const reachable = expandReachable(postFilterSeeds, filteredEdges); + const edges = filteredEdges.filter( (edge) => anyReachable(edge.from, reachable) && allReachable(edge.to, reachable), ); From 54de02cb615ab405a2474088b70a6c616d03a84e Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 20 Apr 2026 00:21:15 -0400 Subject: [PATCH 15/17] =?UTF-8?q?=F0=9F=8E=AF=20fix:=20tighten=20parallel-?= =?UTF-8?q?start=20seed=20criterion=20to=20'no=20pre-filter=20incoming=20e?= =?UTF-8?q?dge'?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses codex pass-12 P1 on #12740. The pass-11 seed heuristic — 'agent is in `agentConfigs` but was not pre-filter reachable from the primary' — was too permissive. A downstream agent like Y in `X -> Y` where X gets skipped (missing / no VIEW) was never pre-filter reachable from the primary either, so the old rule promoted Y to a parallel start node and discovery returned `agents: [primary, Y]` with no connecting edge. The SDK then ran Y as an unintended parallel root — exactly the orphan behavior pass-4 wanted to prevent. Tighter criterion: seed a post-filter reachability root only when the agent had NO incoming edge in the pre-filter graph. That matches `MultiAgentGraph.analyzeGraph`'s "no-incoming-edge" definition of a start node applied to the user's original declared topology, so: - `A -> B` plus a user-defined `X -> Y` parallel branch: X has no incoming pre-filter → seeded → X and Y both survive. - `A -> B` plus `X -> Y` with X skipped: Y had an incoming pre-filter (`X -> Y`) → NOT seeded → Y is pruned as the orphan it is. - `A -> B -> C` with B skipped: C had an incoming pre-filter (`B -> C`) → NOT seeded → C is pruned. New test `does not promote a downstream orphan to a parallel start when its only upstream is skipped` locks in the pass-12 scenario. The pass-11 `preserves user-defined parallel-start branches` test continues to hold. --- packages/api/src/agents/discovery.spec.ts | 48 ++++++++++++++++++++++ packages/api/src/agents/discovery.ts | 49 +++++++++++++++++------ 2 files changed, 84 insertions(+), 13 deletions(-) diff --git a/packages/api/src/agents/discovery.spec.ts b/packages/api/src/agents/discovery.spec.ts index 158580b9422f..ea315b1801e8 100644 --- a/packages/api/src/agents/discovery.spec.ts +++ b/packages/api/src/agents/discovery.spec.ts @@ -441,6 +441,54 @@ describe('discoverConnectedAgents', () => { expect(result.edges).toHaveLength(3); }); + it('does not promote a downstream orphan to a parallel start when its only upstream is skipped', async () => { + // Primary A has `[A -> B, X -> Y]`. X is skipped (no VIEW), Y loads. + // Y had an incoming edge pre-filter (`X -> Y`), so it's a downstream + // agent — not a legitimate parallel starting node. When X is + // skipped, Y becomes a stranded orphan and must be pruned; the + // weaker "not pre-filter reachable from primary" rule would have + // incorrectly treated Y as an intentional parallel start. + const edges: GraphEdge[] = [ + { from: 'A', to: 'B', edgeType: 'handoff' }, + { from: 'X', to: 'Y', edgeType: 'direct' }, + ]; + const primaryConfig = makeConfig('A', edges); + + const agentMap: Record = { + B: makeAgent('B', []), + X: makeAgent('X', []), + Y: makeAgent('Y', []), + }; + const getAgent = jest.fn(async ({ id }: { id: string }) => agentMap[id] ?? null); + // X is forbidden — should be skipped. + const checkPermission = jest.fn( + async ({ resourceId }: { resourceId: unknown }) => resourceId !== 'mongo-X', + ); + + const result = await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + }, + ); + + expect(result.skippedAgentIds.has('X')).toBe(true); + expect(result.agentConfigs.has('B')).toBe(true); + expect(result.agentConfigs.has('Y')).toBe(false); + expect(result.edges).toHaveLength(1); + expect(result.edges[0].to).toBe('B'); + }); + it('preserves user-defined parallel-start branches disconnected from the primary', async () => { // Primary A has edges `[A -> B, X -> Y]`. The `X -> Y` branch is // intentionally disconnected from A — `MultiAgentGraph.analyzeGraph` diff --git a/packages/api/src/agents/discovery.ts b/packages/api/src/agents/discovery.ts index 25c8a00a8831..b83288ec5577 100644 --- a/packages/api/src/agents/discovery.ts +++ b/packages/api/src/agents/discovery.ts @@ -317,18 +317,24 @@ export async function discoverConnectedAgents( * `no-incoming-edge` agents as start nodes, so these run in * parallel with the primary by design. These must be preserved. * - * Both cases produce the same post-filter topology (a component - * disconnected from the primary), but pre-filter they look different: - * accidental orphans WERE reachable from the primary before - * `filterOrphanedEdges` ran; intentional parallel starts were never - * reachable. Use that signal: + * Distinguish the two by asking: did the agent have any incoming edge + * in the user's original (pre-filter) graph? If yes, it was wired as + * a downstream step, and losing that wiring post-filter makes it an + * accidental orphan — prune. If no, the user declared it a start + * node; seed it so the SDK's `analyzeGraph` behavior of running + * incoming-less agents in parallel is preserved. * - * - Seed post-filter reachability with the primary AND every agent - * in `agentConfigs` that was NOT reachable from the primary via - * the pre-filter edges. That treats intentional parallel starts - * as roots. - * - Agents that WERE pre-filter reachable but aren't any more have - * lost their connecting edge — they're pruned. + * "No incoming edge pre-filter" is stricter than "not reachable from + * primary pre-filter": a downstream agent like Y in `X -> Y` where X + * is skipped was never reachable from the primary pre-filter either, + * but it's still an orphan (its upstream X would have routed to it). + * The incoming-edge test catches that case correctly. + * + * - Post-filter reachability is seeded with the primary AND every + * agent in `agentConfigs` that had no pre-filter incoming edge + * (legitimate parallel start). + * - Agents whose pre-filter incoming edges got filtered out lose + * reachability and get pruned. * - Surviving edges are filtered to the post-filter reachable set * so no stale edge references a pruned agent. * - Agents referenced as an endpoint in a surviving edge are always @@ -365,11 +371,28 @@ export async function discoverConnectedAgents( return result; }; - const preFilterReachable = expandReachable(new Set([primaryConfig.id]), preFilterEdges); + // A legitimate parallel-start agent is one that has NO incoming edge + // in the pre-filter graph — the user declared it as a starting node. + // "Not reachable from primary pre-filter" is too permissive: a + // downstream agent whose only upstream got skipped (`X -> Y` with X + // skipped but Y loaded) would qualify under that weaker rule and be + // promoted to a parallel root even though it's actually a stranded + // orphan. Using "no incoming edge in pre-filter" tightens the criterion + // to match the SDK's `analyzeGraph` definition of a start node applied + // to the user's ORIGINAL graph topology, before any orphan filtering. + const hadIncomingEdgePreFilter = new Set(); + for (const edge of preFilterEdges) { + const dests = Array.isArray(edge.to) ? edge.to : [edge.to]; + for (const dest of dests) { + if (typeof dest === 'string') { + hadIncomingEdgePreFilter.add(dest); + } + } + } const postFilterSeeds = new Set([primaryConfig.id]); for (const agentId of agentConfigs.keys()) { - if (!preFilterReachable.has(agentId)) { + if (!hadIncomingEdgePreFilter.has(agentId)) { postFilterSeeds.add(agentId); } } From 6ce34624e851e025f73306cf8cd351f313310523 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 20 Apr 2026 00:37:07 -0400 Subject: [PATCH 16/17] =?UTF-8?q?=F0=9F=93=81=20fix:=20don't=20enforce=20A?= =?UTF-8?q?GENT-only=20file=20ACL=20on=20REMOTE=5FAGENT=20API=20callers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses codex pass-13 P1 on #12740. When I refactored the API controllers' DB-method bundle, I inadvertently started forwarding `filterFilesByAgentAccess` into `initializeAgent`. That helper calls `checkPermission` with `resourceType: ResourceType.AGENT`, but these routes authorize callers through `REMOTE_AGENT` (via `getRemoteAgentPermissions`). A user granted `REMOTE_AGENT_VIEWER` on a shared agent but lacking direct `AGENT_VIEW` could invoke the agent yet all its owner-attached context files would get silently filtered out — breaking `file_search`/context retrieval for remote consumers. Drop `filterFilesByAgentAccess` from the OpenAI-compat and Responses controllers' `dbMethods` (and remove the now-unused import). The chat UI's `initialize.js` keeps it since that path legitimately authorizes at the AGENT level. No functional change inside the helper — passing `undefined` simply tells `primeResources` to skip the per-file ACL filter, restoring the pre-refactor API behavior. --- api/server/controllers/agents/openai.js | 8 ++++++-- api/server/controllers/agents/responses.js | 8 ++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/api/server/controllers/agents/openai.js b/api/server/controllers/agents/openai.js index f48ca410d090..6b07993715c4 100644 --- a/api/server/controllers/agents/openai.js +++ b/api/server/controllers/agents/openai.js @@ -41,7 +41,6 @@ const { getEffectivePermissions, } = require('~/server/services/PermissionService'); const { getModelsConfig } = require('~/server/controllers/ModelController'); -const { filterFilesByAgentAccess } = require('~/server/services/Files/permissions'); const { logViolation } = require('~/cache'); const db = require('~/models'); @@ -220,6 +219,12 @@ const OpenAIChatCompletionController = async (req, res) => { model_parameters: agent.model_parameters ?? {}, }; + // `filterFilesByAgentAccess` is intentionally omitted: it calls + // `checkPermission` with `resourceType: AGENT`, but this route + // authorizes callers through `REMOTE_AGENT` (via + // `getRemoteAgentPermissions`), so including it would silently drop + // owner-attached context files for any remote user who has + // `REMOTE_AGENT_VIEWER` but not direct `AGENT_VIEW`. const dbMethods = { getConvoFiles: db.getConvoFiles, getFiles: db.getFiles, @@ -230,7 +235,6 @@ const OpenAIChatCompletionController = async (req, res) => { getUserCodeFiles: db.getUserCodeFiles, getToolFilesByIds: db.getToolFilesByIds, getCodeGeneratedFiles: db.getCodeGeneratedFiles, - filterFilesByAgentAccess, }; const primaryConfig = await initializeAgent( diff --git a/api/server/controllers/agents/responses.js b/api/server/controllers/agents/responses.js index cfb49c748fa6..beade0741489 100644 --- a/api/server/controllers/agents/responses.js +++ b/api/server/controllers/agents/responses.js @@ -50,7 +50,6 @@ const { getEffectivePermissions, } = require('~/server/services/PermissionService'); const { getModelsConfig } = require('~/server/controllers/ModelController'); -const { filterFilesByAgentAccess } = require('~/server/services/Files/permissions'); const { logViolation } = require('~/cache'); const db = require('~/models'); @@ -358,6 +357,12 @@ const createResponse = async (req, res) => { model_parameters: agent.model_parameters ?? {}, }; + // `filterFilesByAgentAccess` is intentionally omitted: it calls + // `checkPermission` with `resourceType: AGENT`, but this route + // authorizes callers through `REMOTE_AGENT` (via + // `getRemoteAgentPermissions`), so including it would silently drop + // owner-attached context files for any remote user who has + // `REMOTE_AGENT_VIEWER` but not direct `AGENT_VIEW`. const dbMethods = { getConvoFiles: db.getConvoFiles, getFiles: db.getFiles, @@ -368,7 +373,6 @@ const createResponse = async (req, res) => { getUserCodeFiles: db.getUserCodeFiles, getToolFilesByIds: db.getToolFilesByIds, getCodeGeneratedFiles: db.getCodeGeneratedFiles, - filterFilesByAgentAccess, }; const primaryConfig = await initializeAgent( From 718b0000b306bc508c88199f143caeb2e5c1a501 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 20 Apr 2026 00:59:17 -0400 Subject: [PATCH 17/17] =?UTF-8?q?=F0=9F=AA=93=20fix:=20strip=20unreachable?= =?UTF-8?q?=20co-sources=20from=20surviving=20multi-source=20edges?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses codex pass-14 P1 on #12740. The earlier pass-8 fix kept any agent referenced as an endpoint of a surviving edge (via a `referencedByEdge` fallback) to avoid the SDK's `validateEdgeAgents` failing on missing nodes. But that fallback propped up unreachable co-sources too: with `[A -> C, X -> B, [B,C] -> D]` and X skipped, `X -> B` gets filtered, the `[B,C] -> D` fan-in survives because C is reachable, and B stays in `agentConfigs` solely because the fan-in still lists it. `MultiAgentGraph.analyzeGraph` then sees B with no incoming edge and runs it as an unintended parallel root. Sanitize surviving edges instead: for a kept edge whose `from` is an array, filter out any co-source that isn't reachable. The SDK's per-source `addEdge` fires independently, so dropping an unreachable co-source doesn't invalidate the remaining routes — in the scenario above `[B,C] -> D` becomes `[C] -> D`, every endpoint of every surviving edge is now reachable, and the agent prune collapses to a strict `reachable.has(agentId)` check. No more referenced-by-edge fallback. Regression test added: `strips unreachable co-sources from surviving multi-source edges (no stray parallel root)` — asserts B is absent from every surviving edge endpoint and the fan-in's `from` is just `['C']`. All 22 prior discovery tests still pass unchanged. --- packages/api/src/agents/discovery.spec.ts | 66 +++++++++++++++++++++++ packages/api/src/agents/discovery.ts | 49 +++++++++++------ 2 files changed, 100 insertions(+), 15 deletions(-) diff --git a/packages/api/src/agents/discovery.spec.ts b/packages/api/src/agents/discovery.spec.ts index ea315b1801e8..5b86cd194ede 100644 --- a/packages/api/src/agents/discovery.spec.ts +++ b/packages/api/src/agents/discovery.spec.ts @@ -441,6 +441,72 @@ describe('discoverConnectedAgents', () => { expect(result.edges).toHaveLength(3); }); + it('strips unreachable co-sources from surviving multi-source edges (no stray parallel root)', async () => { + // Primary has `[A -> C, X -> B, [B,C] -> D]`. X is skipped, so + // `X -> B` is filtered and B loses its only upstream. The fan-in + // edge `[B,C] -> D` still has C as a reachable source, so it + // survives — but discovery must strip B from that edge's `from` + // list and prune B from `agentConfigs`. Leaving B behind would + // make it an incoming-less agent that `MultiAgentGraph.analyzeGraph` + // runs as an unintended parallel root. + const edges: GraphEdge[] = [ + { from: 'A', to: 'C', edgeType: 'handoff' }, + { from: 'X', to: 'B', edgeType: 'handoff' }, + { from: ['B', 'C'], to: 'D', edgeType: 'direct' }, + ]; + const primaryConfig = makeConfig('A', edges); + + const agentMap: Record = { + B: makeAgent('B', []), + C: makeAgent('C', []), + D: makeAgent('D', []), + X: makeAgent('X', []), + }; + const getAgent = jest.fn(async ({ id }: { id: string }) => agentMap[id] ?? null); + // X is forbidden; B, C, D pass. + const checkPermission = jest.fn( + async ({ resourceId }: { resourceId: unknown }) => resourceId !== 'mongo-X', + ); + + const result = await discoverConnectedAgents( + { + req: makeReq(), + res: makeRes(), + primaryConfig, + allowedProviders: new Set(), + modelsConfig: { openai: ['gpt-4o'] }, + loadTools: jest.fn(), + }, + { + getAgent, + checkPermission, + logViolation: jest.fn(), + db: {} as never, + }, + ); + + expect(result.skippedAgentIds.has('X')).toBe(true); + expect(result.agentConfigs.has('B')).toBe(false); + expect(result.agentConfigs.has('C')).toBe(true); + expect(result.agentConfigs.has('D')).toBe(true); + + // Every returned edge must reference only reachable agents — no B + // anywhere, not in a co-source slot and not as a node the SDK would + // need to register. + const endpoints = result.edges.flatMap((edge) => + [edge.from, edge.to].flatMap((v) => (Array.isArray(v) ? v : [v])), + ); + expect(endpoints).not.toContain('B'); + // The fan-in edge survived (C is a reachable co-source) but now + // lists only C as the source. + const fanInEdge = result.edges.find((edge) => { + const to = Array.isArray(edge.to) ? edge.to : [edge.to]; + return to.includes('D'); + }); + expect(fanInEdge).toBeDefined(); + expect(fanInEdge!.from).toEqual(['C']); + }); + it('does not promote a downstream orphan to a parallel start when its only upstream is skipped', async () => { // Primary A has `[A -> B, X -> Y]`. X is skipped (no VIEW), Y loads. // Y had an incoming edge pre-filter (`X -> Y`), so it's a downstream diff --git a/packages/api/src/agents/discovery.ts b/packages/api/src/agents/discovery.ts index b83288ec5577..e773c7be5039 100644 --- a/packages/api/src/agents/discovery.ts +++ b/packages/api/src/agents/discovery.ts @@ -399,25 +399,44 @@ export async function discoverConnectedAgents( const reachable = expandReachable(postFilterSeeds, filteredEdges); - const edges = filteredEdges.filter( - (edge) => anyReachable(edge.from, reachable) && allReachable(edge.to, reachable), - ); - - const referencedByEdge = new Set(); - for (const edge of edges) { - const endpoints = [ - ...(Array.isArray(edge.from) ? edge.from : [edge.from]), - ...(Array.isArray(edge.to) ? edge.to : [edge.to]), - ]; - for (const id of endpoints) { - if (typeof id === 'string') { - referencedByEdge.add(id); - } + /** + * Filter + sanitize edges: + * - Keep an edge if at least one `from` source is reachable AND every + * `to` destination is reachable (a missing destination would still + * crash `StateGraph.compile` with `Found edge ending at unknown + * node`). + * - For kept edges with an array `from`, strip out unreachable + * co-sources. The SDK's per-source `addEdge` fires independently + * (each source becomes its own `addEdge(source, dest)` call), so + * losing an unreachable co-source doesn't invalidate the routes + * through the surviving ones. Leaving the dead co-source in the + * array was propping up agents that `reachable` had already + * excluded — in `MultiAgentGraph.analyzeGraph` they'd then show up + * as incoming-less nodes and execute as unintended parallel roots. + * + * After sanitization every endpoint in every surviving edge is + * guaranteed to be in `reachable`, which lets the agent prune below + * collapse to a strict reachability check. + */ + const edges: GraphEdge[] = []; + for (const edge of filteredEdges) { + if (!anyReachable(edge.from, reachable) || !allReachable(edge.to, reachable)) { + continue; + } + if (!Array.isArray(edge.from)) { + edges.push(edge); + continue; + } + const reachableSources = edge.from.filter((s) => typeof s !== 'string' || reachable.has(s)); + if (reachableSources.length === edge.from.length) { + edges.push(edge); + } else { + edges.push({ ...edge, from: reachableSources }); } } for (const agentId of [...agentConfigs.keys()]) { - if (!reachable.has(agentId) && !referencedByEdge.has(agentId)) { + if (!reachable.has(agentId)) { agentConfigs.delete(agentId); } }