mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-05-15 06:12:50 -07:00
feat(core): composite sessionState key + duplicate agent name warning
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.
This commit is contained in:
@@ -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'),
|
||||
);
|
||||
});
|
||||
|
||||
|
||||
@@ -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<TOutput extends z.ZodTypeAny>(
|
||||
definition: AgentDefinition<TOutput>,
|
||||
): Promise<void> {
|
||||
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') {
|
||||
|
||||
@@ -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<string, unknown>;
|
||||
}
|
||||
).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<string, { contextId?: string; taskId?: string }>;
|
||||
}
|
||||
).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<string, { contextId?: string; taskId?: string }>;
|
||||
}
|
||||
).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<string, { contextId?: string; taskId?: string }>;
|
||||
}
|
||||
).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,
|
||||
);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user