🧹 chore: address audit NITs on discovery helper

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.
This commit is contained in:
Danny Avila 2026-04-19 22:44:08 -04:00
parent e0003544a6
commit 6450aa8751
3 changed files with 63 additions and 8 deletions

View file

@ -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];

View file

@ -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<string, Agent> = {
B: makeAgent('B', []),
C: makeAgent('C', []),
};
const authById: Record<string, Record<string, Record<string, string>>> = {
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' }]);

View file

@ -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);