feat(core): wire AgentSession invocations into agent-tool - #26948
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enables AgentSession support within the AgentTool framework, allowing for more robust subagent interactions behind a feature flag. It also includes critical improvements to session state management for remote agents and adds validation logic to the AgentRegistry to prevent and surface duplicate agent name conflicts. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
6833fa1 to
926a4e3
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements session-based subagent invocations, gated by a feature flag, and adds a warning mechanism for duplicate agent names in the registry. It also updates remote session state management to use a composite key of name and URL. Review feedback highlights security concerns and potential memory leaks regarding the use of a static map for session state, suggesting a refactor to session-scoped storage or an LRU cache. Additionally, a redundant method definition in the configuration class was identified for removal.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/core/src/agents/remote-session-invocation.ts (62-65)
The sessionKey method generates a key for the static sessionState Map using only the agent name and URL, but it omits the sessionId. Because sessionState is a static member, it is shared across all instances of RemoteSessionInvocation within the same process. In multi-tenant environments (e.g., if this core library is used in a server-side application), this will cause different user sessions to share the same A2A conversation state (contextId and taskId) if they invoke the same remote agent. This leads to session collision or hijacking, where one user could inadvertently continue or access another user's conversation.
To remediate this, the sessionId should be incorporated into the session key to ensure that conversation state is isolated per session. This will require passing the AgentLoopContext to the sessionKey method.
References
- Avoid module-level global variables for state to prevent race conditions in concurrent environments. Instead, use instance-scoped properties, such as within a session class, for state that is primarily used within that scope.
packages/core/src/agents/remote-session-invocation.ts (53-56)
The use of a static Map for sessionState violates the general rule against module-level global variables for state. This implementation poses a memory leak risk in long-running processes as the map grows indefinitely with every unique agent/URL combination. More critically, it introduces a security vulnerability in multi-session or concurrent environments (such as a server) where A2A conversation state (contextId, taskId) could leak between different users or sessions sharing the same process.
Please refactor this to use session-scoped state (e.g., stored within the Config or a session-scoped service) or, at minimum, use a standard LRUCache with a reasonable size limit to mitigate memory issues.
References
- Avoid module-level global variables for state like caches to prevent race conditions and memory issues in concurrent environments. Instead, use session-scoped or instance-scoped state and leverage standard cache implementations like LRUCache.
- For caching, use the existing
LruCachedependency instead of clearing the entire cache or implementing a custom LRU policy.
packages/core/src/config/config.ts (2584-2586)
The method isAgentSessionEnabled is redundantly defined here. It is already present in the class at line 2560. Please remove this duplicate definition to maintain code clarity and avoid confusion.
There was a problem hiding this comment.
Code Review
This pull request introduces session-based agent invocations gated by a feature flag and adds duplicate agent name detection to the registry. It also updates remote session state management to use composite keys for improved isolation. Feedback identifies a critical security risk where the use of static maps for session state could lead to data leakage between concurrent sessions. Furthermore, the reviewer pointed out redundant and duplicate methods in the configuration class that should be consolidated.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/core/src/agents/remote-session-invocation.ts (121-122)
The RemoteSessionInvocation class uses a static Map named sessionState to persist A2A (Agent-to-Agent) conversation state. Because the map is static, it is shared across all instances within the same process, leading to state leakage between sessions in concurrent environments. Move the sessionState map to a session-scoped object, such as the Config instance or the AgentLoopContext.
References
- Avoid module-level global variables for state like caches to prevent race conditions and memory issues in concurrent environments. Instead, use session-scoped or instance-scoped state and leverage standard cache implementations like LRUCache.
packages/core/src/agents/remote-session-invocation.ts (228-231)
This line persists the A2A conversation state into a static sessionState map. Using a static map for session-specific state leads to cross-session data leakage in long-lived processes. The state should be stored in a session-scoped container to ensure isolation.
References
- Avoid module-level global variables for state like caches to prevent race conditions and memory issues in concurrent environments. Instead, use session-scoped or instance-scoped state and leverage standard cache implementations like LRUCache.
packages/core/src/config/config.ts (2584-2587)
This method is a duplicate of the one already defined at line 2560. Please consolidate this logic to avoid duplication and improve maintainability, as per the project's architectural guidelines.
References
- When adding new functionality, such as filtering, consolidate it with existing similar logic to avoid duplication and improve maintainability.
packages/core/src/config/config.ts (3783-3786)
This method is redundant as it performs the exact same logic as isAgentSessionEnabled(). Instead of maintaining redundant logic or backward compatibility for configuration settings, remove the extra getter entirely to avoid confusion.
References
- Instead of maintaining backward compatibility for deprecated or unshipped configuration settings, remove them and their associated logic entirely to avoid confusion.
packages/core/src/agents/remote-session-invocation.ts (53-56)
The use of a static map for storing session state violates the rule against module-level global variables for state. This approach can lead to race conditions and data leakage in concurrent environments. This state should be moved to a session-scoped store, such as a property on the Config instance.
References
- Avoid module-level global variables for state like caches to prevent race conditions and memory issues in concurrent environments. Instead, use session-scoped or instance-scoped state and leverage standard cache implementations like LRUCache.
There was a problem hiding this comment.
Code Review
This pull request introduces session-based subagent invocations and enhances the agent registry to handle duplicate agent names with warnings. Key changes include updating AgentTool to support event threading, implementing composite keys (name and URL) for remote session state isolation, and adding feature flag checks for session-enabled invocations. Review feedback identifies a critical TypeScript compilation error due to a duplicate method definition in the Config class, a security concern regarding session leakage from static state in RemoteSessionInvocation, and a logic improvement to prevent false-positive warnings in the AgentRegistry when an agent is disabled via overrides.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/core/src/config/config.ts (2584-2587)
The method isAgentSessionEnabled is being added here, but it is already defined earlier in the class (around line 2563 in the current branch state). This duplication will cause a TypeScript compilation error ("Duplicate identifier 'isAgentSessionEnabled'").
packages/core/src/agents/remote-session-invocation.ts (53-56)
The sessionState Map is declared as static, which means it is shared across all instances of RemoteSessionInvocation within the same process. This Map stores A2A conversation state (such as contextId and taskId) for remote agents. In environments where the same process handles multiple user sessions or different conversations (e.g., a long-running interactive shell or a server-side deployment of the core library), this can lead to session leakage. One session could potentially retrieve or overwrite the conversation state of another session if they use a remote agent with the same name and URL. Consider moving this state to a session-scoped object like AgentLoopContext or Config.
References
- Avoid module-level global variables for state like caches to prevent race conditions and memory issues in concurrent environments. Instead, use session-scoped or instance-scoped state.
packages/core/src/agents/registry.ts (340-351)
The warning for duplicate agent names is emitted before checking if the new agent definition is actually enabled. If the later definition is disabled (via settings overrides), it will not actually override the existing one in this.agents (as registerLocalAgent/registerRemoteAgent will return early), making this warning a false positive. The check should verify enablement before emitting the feedback.
const existing = this.agents.get(definition.name);
const overrides = this.config.getAgentsSettings().overrides?.[definition.name];
if (existing && existing !== definition && this.isAgentEnabled(definition, overrides)) {
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.',
);
}
3e54174 to
c34e81e
Compare
926a4e3 to
0dcda82
Compare
c34e81e to
0fea5cb
Compare
0dcda82 to
755885f
Compare
gemini-cli
Bot
added
the
status/need-issue
0fea5cb to
b36d17a
Compare
712c435 to
4ad0926
Compare
c414010 to
ca5d4a2
Compare
be3b6f6 to
c0981bd
Compare
674927f to
fa3c5f5
Compare
c0981bd to
f89ebfb
Compare
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
|
Size Change: +37.5 kB (+0.11%) Total Size: 33.9 MB
ℹ️ View Unchanged
|
927dc8d to
80e1c75
Compare
23c943a to
491e1e4
Compare
491e1e4 to
ab441fa
Compare
78ccdb8 to
9901f6f
Compare
ab441fa to
609ee82
Compare
9901f6f to
abe9fd8
Compare
609ee82 to
23ab766
Compare
abe9fd8 to
0beb5ea
Compare
23ab766 to
6ca7b26
Compare
0beb5ea to
14f14f5
Compare
6ca7b26 to
a983202
Compare
…ure flag Add agentSessionSubagentEnabled setting under experimental.adk that routes subagent invocations through LocalSessionInvocation and RemoteSessionInvocation instead of legacy executors. When disabled (default), all behavior is unchanged. - Add agentSessionSubagentEnabled to ADKSettings, Config, and settings schema - Thread onAgentEvent callback through AgentTool → DelegateInvocation - Route local/remote agents to session invocations when flag is enabled - Browser agent always uses BrowserAgentInvocation regardless of flag - Copy local protocol files needed for the session invocation path
…ion key implementation
6a29749 to
2308d4c
Compare
3 participants
Summary
Wires
AgentSessioninvocations intoAgentToolbehind theadk.agentSessionSubagentEnabledfeature flag.Details
packages/core/src/agents/agent-tool.tsto checkcontext.config.isAgentSessionEnabled().RemoteSessionInvocationandLocalSessionInvocationfor subagent calls.experimental-flag.Related Issues
Relates to #22700
How to Validate
Verified that the codebase builds successfully. Unit tests for
agent-toolshould be run to confirm behavior.Pre-Merge Checklist