feat(policy): support subagent-specific policies in TOML#21431
feat(policy): support subagent-specific policies in TOML#21431
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 enhances the policy enforcement system by introducing granular control over tool usage for subagents. It allows developers to define policies that are specific to individual subagents, preventing universal application of rules unless explicitly desired. This change provides greater flexibility and precision in managing agent behavior and permissions within the system. Highlights
Activity
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
|
|
Size Change: +941 B (0%) Total Size: 26 MB ℹ️ View Unchanged
|
|
This PR is a followup from this PR #21133 |
There was a problem hiding this comment.
Code Review
This pull request introduces support for subagent-specific policies in TOML configuration files, enhancing control over agent behavior. However, the current implementation contains critical flaws that allow subagents to bypass these policies. Specifically, the Proxy used to inject subagent names into the MessageBus is bypassed when tools use the request method due to incorrect this binding, and the PolicyEngine fails to propagate the subagent identifier for shell commands. These issues compromise the integrity of the subagent isolation model. Additionally, while the Proxy solution is clever, it introduces potential fragility and maintainability concerns.
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
| const value = Reflect.get(target, prop, receiver); | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | ||
| return typeof value === 'function' ? value.bind(target) : value; |
There was a problem hiding this comment.
The Proxy implementation used to inject the subagent name into MessageBus messages incorrectly binds all methods to the target (the original MessageBus instance). When a tool calls messageBus.request(...), the request method is executed with this set to the original MessageBus. Since MessageBus.request internally calls this.publish(...), it calls the original publish method instead of the proxied one, bypassing the subagent name injection. This allows subagents to bypass any policies that rely on the subagent field. This Proxy approach, while clever, introduces fragility and makes the code harder to reason about and maintain, as it relies on assumptions about internal MessageBus implementation and could break with future changes.
| return typeof value === 'function' ? value.bind(target) : value; | |
| return typeof value === 'function' ? value.bind(receiver) : value; |
|
I've applied the suggested fix to bind the proxy functions to |
|
@abhipatel12 This links to the parent issue #20195 |
|
|
@scidomino Thanks for the review! I've addressed the feedback:
I've also resolved the merge conflicts with the |
|
@gundermanc Thanks for the suggestion! As per @scidomino's recommendation above, I've actually just replaced the |
|
|
||
| // Create an override object to inject the subagent name into tool confirmation requests | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion | ||
| const subagentMessageBus = Object.create( |
There was a problem hiding this comment.
nit: can we type the local const instead of casting:
const subagentMessageBus: typeof parentMessageBus = Object.create(
parentMessageBus,
);There was a problem hiding this comment.
I think doing so lets us delete the suppression above.
| runtimeContext: Config, | ||
| onActivity?: ActivityCallback, | ||
| ): Promise<LocalAgentExecutor<TOutput>> { | ||
| const parentMessageBus = runtimeContext.getMessageBus(); |
There was a problem hiding this comment.
Do we need to add any tests for this path?
| } | ||
|
|
||
| // Check subagent if specified (only for PolicyRule, SafetyCheckerRule doesn't have it) | ||
| if ('subagent' in rule && rule.subagent) { |
There was a problem hiding this comment.
Do we need tests for this case?
|
|
||
| # (Optional) The name of a subagent. If provided, the rule only applies to tool calls | ||
| # made by this specific subagent. | ||
| subagent = "generalist" |
There was a problem hiding this comment.
Can we configure different values of this rule for multiple subagents by having the same rule muliple times, each with a different subagent?
| * The name of the subagent this rule applies to. | ||
| * If undefined, the rule applies regardless of whether it's the main agent or a subagent. | ||
| */ | ||
| subagent?: string; |
There was a problem hiding this comment.
Have you considered instead structuring the schema to be a map from subagent name => rules, instead of having a flat set of rules with the subagent name?
I wonder if that might prevent some potential invalid states, like having conflicting configs for the same subagent.
gundermanc
left a comment
There was a problem hiding this comment.
Approved with some suggestions.
|
@gundermanc Good catch! I've updated the implementation to strongly type the local |
I think you can typically assign an |
Summary
This PR introduces the ability to specify a subagent directly in policy rules via the
.tomlconfiguration file. By adding asubagentproperty toPolicyRule, we allow users to restrict rules to specific subagents, preventing these rules from applying universally across all agents unless desired.Details
subagenttoPolicyRuleSchemain thetoml-loader.ToolConfirmationRequestinmessage-busto take an optionalsubagentfield.LocalAgentExecutorto inject the executing subagent's name via aProxyapplied to itsMessageBus(which was safely implemented directly via method override instead of an unsafeProxyobject), automatically populatingsubagentfor any subagent-originatedTOOL_CONFIRMATION_REQUEST.PolicyEnginenow includes an explicit check forsubagentwithinruleMatches. If a rule defines asubagent, it will only trigger if the subagent executing the tool matches.subagentis not specified in the.tomlfile, the rule applies universally (to both main and subagents).Related Issues
Fixes #21134
How to Validate
npm run preflight.policy/toml-loader.test.tsandagents/local-executor.test.ts.subagentflag (e.g.subagent = "docs-writer") and verify it's only triggered by that subagent.Pre-Merge Checklist