From 6450aa87511f5d2d2b4828112e51556df0963ecc Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 19 Apr 2026 22:44:08 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=B9=20chore:=20address=20audit=20NITs?= =?UTF-8?q?=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 c8515bd0fa..549e9047b5 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 a952d979e1..7030d2a467 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 1db841c0c5..b04a365615 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);