diff --git a/api/server/controllers/agents/openai.js b/api/server/controllers/agents/openai.js index 91e2b98617..e1f6f9c3f0 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 f6530aebb0..ce494defe3 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 5e57d547f5..a952d979e1 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 c07c85c643..1db841c0c5 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,