mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-10-21 17:44:11 +00:00
🔬 fix: address audit findings on discovery helper
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.
This commit is contained in:
Danny Avila
parent
e54440b7d1
commit
e0003544a6
4 changed files with 224 additions and 92 deletions
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
||||
|
|
|
|||
|
|
@ -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<string, Agent> = {
|
||||
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.
|
||||
|
|
|
|||
|
|
@ -141,8 +141,10 @@ export async function discoverConnectedAgents(
|
|||
|
||||
const agentConfigs = new Map<string, InitializedAgent>();
|
||||
const skippedAgentIds = new Set<string>();
|
||||
// Shallow-clone so the sub-agent merges below don't silently mutate
|
||||
// `primaryConfig.userMCPAuthMap` on the caller's object.
|
||||
let userMCPAuthMap: Record<string, Record<string, string>> | 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<string>([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,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue