-
Notifications
You must be signed in to change notification settings - Fork 9.3k
feat: Add enableSubagents configuration and wire up subagent registration
#9988
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @abhipatel12, 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 lays the foundational groundwork for integrating experimental subagents into the system. It introduces a new configuration option to enable this feature, sets up the necessary infrastructure for subagents to be recognized and utilized as tools, and ensures their operations adhere to existing policy enforcement mechanisms. A significant architectural improvement was also made by resolving a critical circular dependency, enhancing the modularity and stability of the codebase. 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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces the foundational components for the experimental "Subagents" feature. The changes are well-structured, adding a new configuration flag enableSubagents and the necessary logic in the Config class to dynamically register agent definitions as tools. The implementation of the AgentExecutor is robust, with good separation of concerns and important security considerations like an allowlist for non-interactive tools and runtime authorization checks. The introduction of a new AgentRegistry and the SubagentToolWrapper provides a clean abstraction for this new capability. Additionally, the PR thoughtfully addresses and resolves a circular dependency that was exposed by these changes. The new test suites for the agent components are comprehensive and cover many edge cases.
My review identifies one high-severity issue related to the generation of fallback IDs for tool calls, which could lead to collisions and incorrect agent behavior in rare but possible scenarios. A fix is suggested to ensure unique ID generation. Overall, this is a solid contribution that lays the groundwork for a powerful new feature.
|
Size Change: +39.2 kB (+0.22%) Total Size: 17.5 MB
ℹ️ View Unchanged
|
047283f to
4b167da
Compare
| useModelRouter, | ||
| enableMessageBusIntegration: | ||
| settings.tools?.enableMessageBusIntegration ?? false, | ||
| enableSubagents: settings.experimental?.enableSubagents ?? false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be fine for now, but it would be great to have a list where you list the subagents you want to enable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that sounds great! I think as we discover a need for it, we can add more properties
| for (const definition of agentDefinitions) { | ||
| // We must respect the main allowed/exclude lists for agents too. | ||
| const excludeTools = this.getExcludeTools() || []; | ||
| const allowedTools = this.getAllowedTools(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. ok. So instead of having a list of enabled agents we are explicit considering them as tools and having the list of allowed/excluded here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, in the event that a user says they want to disable a subagent, we would need to respect that. For now, since we don't have a special flag --disable-subagent, we can respect it via the tool flag list.
- Adds `enableSubagents` to CLI and Core configuration. - Updates `settingsSchema` to include the new setting. - Updates `Config` in core to register subagents as tools when enabled. - Updates `SubagentToolWrapper` and `AgentInvocation` to propagate the `messageBus`. - Adds comprehensive tests for all new functionality. - Fixes a circular dependency between `Config` and `GlobTool` by extracting the tool name to a new `tool-names.ts` constants file.
4b167da to
e7fbf64
Compare
TLDR
This PR introduces the necessary configuration and wiring to enable the experimental "Subagents" feature. It adds a new
enableSubagentssetting, updates the coreConfigto dynamically register subagents as tools when this setting is enabled, and ensures theMessageBusis correctly propagated for policy enforcement. It also resolves a circular dependency that was exposed by these changes.Dive Deeper
Configuration Changes
experimental.enableSubagentsto the CLIsettingsSchema. It defaults tofalseand requires a restart to take effect.Configclasses to read and expose this setting.Subagent Registration
packages/core/src/config/config.ts, anAgentRegistryis now initialized.createToolRegistrymethod has been updated. IfenableSubagentsistrue, it retrieves all agent definitions from the registry, wraps them in aSubagentToolWrapper, and registers them as tools.allowedToolsandexcludeToolsconfiguration, allowing users to control which subagents are available.Message Bus Propagation
SubagentToolWrapperandSubagentInvocationto accept an optionalMessageBusin their constructors.Configclass now passes theMessageBuswhen creating theSubagentToolWrapper. This ensures that any actions taken by the subagents will still participate in the standard confirmation and policy enforcement flow.Circular Dependency Fix
The introduction of
AgentRegistryintoConfigexposed a circular dependency:Config->AgentRegistry->CodebaseInvestigatorAgent->GlobTool->Config.This was causing
glob.test.tsto fail. The fix involved decoupling the tool name from the tool implementation:packages/core/src/tools/tool-names.tsto hold tool name constants.GlobToolto use the constantGLOB_TOOL_NAME.CodebaseInvestigatorAgentto import the name fromtool-names.tsinstead of importing theGlobToolclass, thus breaking the import cycle.Reviewer Test Plan
1. Verify Unit Tests
Run preflight and ensure all tests pass
2. Verify Configuration Behavior
/toolsto see if you see the example codebase investigator).Testing Matrix