From d14b2a40c3b6da55d60f0e07e4a780e3c2937446 Mon Sep 17 00:00:00 2001 From: Adam Weidman Date: Mon, 13 Apr 2026 22:37:55 -0400 Subject: [PATCH] feat(core): composite sessionState key + duplicate agent name warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes: 1. RemoteSessionInvocation now uses a composite key (name::targetUrl) for the static sessionState map. This ensures agents with the same name but different endpoints maintain independent A2A state. Falls back to name-only when no URL can be derived. 2. AgentRegistry.registerAgent now emits a visible warning when a different definition tries to register under an existing name. Override still proceeds to preserve existing precedence order (user → project → extension). The warning surfaces potential naming conflicts to users. --- packages/core/src/agents/registry.test.ts | 51 ++++++---- packages/core/src/agents/registry.ts | 20 +++- .../agents/remote-session-invocation.test.ts | 93 +++++++++++++++++-- .../src/agents/remote-session-invocation.ts | 24 +++-- 4 files changed, 150 insertions(+), 38 deletions(-) diff --git a/packages/core/src/agents/registry.test.ts b/packages/core/src/agents/registry.test.ts index 3d45be1f94..9d7aaf71a1 100644 --- a/packages/core/src/agents/registry.test.ts +++ b/packages/core/src/agents/registry.test.ts @@ -1012,44 +1012,57 @@ describe('AgentRegistry', () => { ); }); - it('should overwrite an existing agent definition', async () => { + it('should override an existing agent but warn on duplicate name', async () => { await registry.testRegisterAgent(MOCK_AGENT_V1); expect(registry.getDefinition('MockAgent')?.description).toBe( 'Mock Description V1', ); + const feedbackSpy = vi.spyOn(coreEvents, 'emitFeedback'); await registry.testRegisterAgent(MOCK_AGENT_V2); + + // Should override to V2 (preserves existing precedence) expect(registry.getDefinition('MockAgent')?.description).toBe( 'Mock Description V2 (Updated)', ); expect(registry.getAllDefinitions()).toHaveLength(1); + // But should emit a warning about the duplicate + expect(feedbackSpy).toHaveBeenCalledWith( + 'warning', + expect.stringContaining("Duplicate agent name 'MockAgent'"), + ); }); - it('should log overwrites when in debug mode', async () => { + it('should not warn when re-registering the same definition (refresh)', async () => { + await registry.testRegisterAgent(MOCK_AGENT_V1); + expect(registry.getDefinition('MockAgent')?.description).toBe( + 'Mock Description V1', + ); + + // Re-registering the exact same object should succeed without warning + const feedbackSpy = vi.spyOn(coreEvents, 'emitFeedback'); + await registry.testRegisterAgent(MOCK_AGENT_V1); + expect(registry.getDefinition('MockAgent')?.description).toBe( + 'Mock Description V1', + ); + expect(feedbackSpy).not.toHaveBeenCalledWith( + 'warning', + expect.stringContaining('Duplicate'), + ); + }); + + it('should warn on duplicate in debug logs', async () => { const debugConfig = makeMockedConfig({ debugMode: true }); const debugRegistry = new TestableAgentRegistry(debugConfig); - const debugLogSpy = vi - .spyOn(debugLogger, 'log') + const warnSpy = vi + .spyOn(debugLogger, 'warn') .mockImplementation(() => {}); await debugRegistry.testRegisterAgent(MOCK_AGENT_V1); await debugRegistry.testRegisterAgent(MOCK_AGENT_V2); - expect(debugLogSpy).toHaveBeenCalledWith( - `[AgentRegistry] Overriding agent 'MockAgent'`, - ); - }); - - it('should not log overwrites when not in debug mode', async () => { - const debugLogSpy = vi - .spyOn(debugLogger, 'log') - .mockImplementation(() => {}); - - await registry.testRegisterAgent(MOCK_AGENT_V1); - await registry.testRegisterAgent(MOCK_AGENT_V2); - - expect(debugLogSpy).not.toHaveBeenCalledWith( - `[AgentRegistry] Overriding agent 'MockAgent'`, + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining('Overriding agent'), ); }); diff --git a/packages/core/src/agents/registry.ts b/packages/core/src/agents/registry.ts index ebb757487c..bcd2e414cc 100644 --- a/packages/core/src/agents/registry.ts +++ b/packages/core/src/agents/registry.ts @@ -317,13 +317,27 @@ export class AgentRegistry { } /** - * Registers an agent definition. If an agent with the same name exists, - * it will be overwritten, respecting the precedence established by the - * initialization order. + * Registers an agent definition. If an agent with the same name already + * exists from a different definition, a warning is emitted to surface + * potential state collision. The new definition still overwrites the old + * one to preserve existing precedence order (user → project → extension). */ protected async registerAgent( definition: AgentDefinition, ): Promise { + const existing = this.agents.get(definition.name); + if (existing && existing !== definition) { + coreEvents.emitFeedback( + 'warning', + `Duplicate agent name '${definition.name}' detected. ` + + `The later definition will override the earlier one. ` + + `Rename one of the agents to avoid this conflict.`, + ); + debugLogger.warn( + `[AgentRegistry] Overriding agent '${definition.name}' — duplicate name from a different definition.`, + ); + } + if (definition.kind === 'local') { this.registerLocalAgent(definition); } else if (definition.kind === 'remote') { diff --git a/packages/core/src/agents/remote-session-invocation.test.ts b/packages/core/src/agents/remote-session-invocation.test.ts index f096d72a89..0b24913eb2 100644 --- a/packages/core/src/agents/remote-session-invocation.test.ts +++ b/packages/core/src/agents/remote-session-invocation.test.ts @@ -211,12 +211,12 @@ describe('RemoteSessionInvocation', () => { it('should pass initial state from static map to session', async () => { const priorState = { contextId: 'ctx-42', taskId: 'task-42' }; - // Seed the static map before constructing the invocation + // Seed the static map before constructing the invocation (composite key) ( RemoteSessionInvocation as unknown as { sessionState: Map; } - ).sessionState.set('test-agent', priorState); + ).sessionState.set('test-agent::http://test-agent/card', priorState); setupMockSession(); @@ -253,12 +253,12 @@ describe('RemoteSessionInvocation', () => { abortSignal: new AbortController().signal, }); - // Verify the state was persisted in the static map + // Verify the state was persisted in the static map (composite key) const storedState = ( RemoteSessionInvocation as unknown as { sessionState: Map; } - ).sessionState.get('test-agent'); + ).sessionState.get('test-agent::http://test-agent/card'); expect(storedState).toEqual(newState); }); @@ -496,11 +496,12 @@ describe('RemoteSessionInvocation', () => { // --------------------------------------------------------------------------- describe('SessionState Management', () => { - it('should use definition.name as session state key', async () => { + it('should use composite name::url as session state key', async () => { const secondDefinition: RemoteAgentDefinition = { ...mockDefinition, name: 'other-agent', displayName: 'Other Agent', + agentCardUrl: 'http://other-agent/card', }; // First agent @@ -533,9 +534,81 @@ describe('RemoteSessionInvocation', () => { } ).sessionState; - // Each agent should have its own entry - expect(stateMap.get('test-agent')).toEqual({ contextId: 'ctx-a' }); - expect(stateMap.get('other-agent')).toEqual({ contextId: 'ctx-b' }); + // Each agent should have its own entry keyed by name::url + expect(stateMap.get('test-agent::http://test-agent/card')).toEqual({ + contextId: 'ctx-a', + }); + expect(stateMap.get('other-agent::http://other-agent/card')).toEqual({ + contextId: 'ctx-b', + }); + }); + + it('should isolate same-name agents with different URLs', async () => { + const defA: RemoteAgentDefinition = { + ...mockDefinition, + agentCardUrl: 'http://host-a/card', + }; + const defB: RemoteAgentDefinition = { + ...mockDefinition, + agentCardUrl: 'http://host-b/card', + }; + + // Agent A + setupMockSession({ sessionState: { contextId: 'ctx-a' } }); + const invA = new RemoteSessionInvocation( + defA, + mockContext, + { query: 'hi' }, + mockMessageBus, + ); + await invA.execute({ abortSignal: new AbortController().signal }); + + // Agent B (same name, different URL) + setupMockSession({ sessionState: { contextId: 'ctx-b' } }); + const invB = new RemoteSessionInvocation( + defB, + mockContext, + { query: 'hi' }, + mockMessageBus, + ); + await invB.execute({ abortSignal: new AbortController().signal }); + + const stateMap = ( + RemoteSessionInvocation as unknown as { + sessionState: Map; + } + ).sessionState; + + expect(stateMap.get('test-agent::http://host-a/card')).toEqual({ + contextId: 'ctx-a', + }); + expect(stateMap.get('test-agent::http://host-b/card')).toEqual({ + contextId: 'ctx-b', + }); + }); + + it('should fall back to name-only key when URL is unavailable', async () => { + const noUrlDef: RemoteAgentDefinition = { + ...mockDefinition, + agentCardUrl: undefined, + }; + + setupMockSession({ sessionState: { contextId: 'ctx-no-url' } }); + const inv = new RemoteSessionInvocation( + noUrlDef, + mockContext, + { query: 'hi' }, + mockMessageBus, + ); + await inv.execute({ abortSignal: new AbortController().signal }); + + const stateMap = ( + RemoteSessionInvocation as unknown as { + sessionState: Map; + } + ).sessionState; + + expect(stateMap.get('test-agent')).toEqual({ contextId: 'ctx-no-url' }); }); it('should persist state even on error', async () => { @@ -562,7 +635,9 @@ describe('RemoteSessionInvocation', () => { } ).sessionState; - expect(stateMap.get('test-agent')).toEqual(stateOnError); + expect(stateMap.get('test-agent::http://test-agent/card')).toEqual( + stateOnError, + ); }); }); }); diff --git a/packages/core/src/agents/remote-session-invocation.ts b/packages/core/src/agents/remote-session-invocation.ts index bf9d557ea6..6b6b5a70e5 100644 --- a/packages/core/src/agents/remote-session-invocation.ts +++ b/packages/core/src/agents/remote-session-invocation.ts @@ -17,6 +17,7 @@ import { type RemoteAgentDefinition, type AgentInputs, type SubagentProgress, + getRemoteAgentTargetUrl, } from './types.js'; import { type AgentLoopContext } from '../config/agent-loop-context.js'; import type { MessageBus } from '../confirmation-bus/message-bus.js'; @@ -38,20 +39,30 @@ export interface SubagentInvocationOptions { * which wraps the A2A client streaming behind the AgentProtocol interface. * * Cross-invocation A2A session state (contextId/taskId) is persisted via a - * static map keyed by agent name, matching the original RemoteAgentInvocation - * behavior. + * static map keyed by a composite of agent name and target URL. This ensures + * agents with the same name but different endpoints maintain independent state. */ export class RemoteSessionInvocation extends BaseToolInvocation< RemoteAgentInputs, ToolResult > { // Persist A2A conversation state across ephemeral invocation instances. - // Keyed by agent name — each remote agent maintains independent state. + // Keyed by composite of name + target URL so agents with the same name + // but different endpoints don't share state. private static readonly sessionState = new Map< string, { contextId?: string; taskId?: string } >(); + /** + * Builds a composite key for the sessionState map. + * Format: `name::targetUrl` (or just `name` if no URL can be derived). + */ + private static sessionKey(definition: RemoteAgentDefinition): string { + const url = getRemoteAgentTargetUrl(definition); + return url ? `${definition.name}::${url}` : definition.name; + } + private readonly _onAgentEvent?: (event: AgentEvent) => void; constructor( @@ -106,9 +117,8 @@ export class RemoteSessionInvocation extends BaseToolInvocation< const agentName = this.definition.displayName ?? this.definition.name; // Seed session with prior A2A conversation state - const priorState = RemoteSessionInvocation.sessionState.get( - this.definition.name, - ); + const stateKey = RemoteSessionInvocation.sessionKey(this.definition); + const priorState = RemoteSessionInvocation.sessionState.get(stateKey); const session = new RemoteSubagentSession( this.definition, this.context, @@ -215,7 +225,7 @@ export class RemoteSessionInvocation extends BaseToolInvocation< } finally { // Persist A2A state for next invocation — even on abort/error RemoteSessionInvocation.sessionState.set( - this.definition.name, + stateKey, session.getSessionState(), ); _signal.removeEventListener('abort', abortListener);