feat(core): Tool Confirmation Message Bus foundation (PR 1 of 3)#7835
Merged
allenhutchison merged 13 commits intomainfrom Sep 11, 2025
Merged
feat(core): Tool Confirmation Message Bus foundation (PR 1 of 3)#7835allenhutchison merged 13 commits intomainfrom
allenhutchison merged 13 commits intomainfrom
Conversation
Introduces a pub/sub message bus architecture to decouple tool confirmation logic from core tool implementation. This is the first PR in a series to implement the Tool Confirmation Message Bus as described in issue #7231. ## Changes - Add MessageBus class with event-driven publish/subscribe pattern - Implement PolicyEngine with configurable rules for tool execution control - Integrate MessageBus and PolicyEngine into Config class - Add comprehensive unit tests for both components ## Features - Priority-based policy rules with tool name and args pattern matching - Non-interactive mode support (ASK_USER becomes DENY) - Error handling and message validation - Support for multiple message types (confirmation, rejection, execution) ## Testing - 25 new unit tests covering all functionality - Full test suite passes (1964 total tests) - TypeScript compilation and linting checks pass
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Summary of Changes
Hello @allenhutchison, 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 establishes the foundational architecture for a robust tool confirmation system. It introduces a message bus for event-driven communication and a policy engine to enforce rules around tool execution. This initial phase sets the stage for future enhancements that will enable fine-grained control and auditing of how AI agents interact with external tools, significantly improving security and operational transparency.
Highlights
- Foundation for Tool Confirmation Message Bus: Introduces core components for managing tool execution confirmations, laying the groundwork for controlled tool usage.
- Decoupled Communication: Implements a pub/sub MessageBus for flexible, event-driven handling of tool confirmation messages, promoting modularity.
- Configurable Policy Enforcement: Adds a PolicyEngine to control tool execution based on priority-based rules, including tool name and argument pattern matching, enhancing security.
- Integration with Core Configuration: The new MessageBus and PolicyEngine are integrated into the Config class, making them accessible and configurable throughout the application.
- Robust Testing and Type Safety: Includes comprehensive unit tests for both new components and provides full TypeScript support for enhanced reliability and maintainability.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
Size Change: +7.18 kB (+0.05%) Total Size: 13.2 MB
ℹ️ View Unchanged
|
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a solid foundation for a tool confirmation message bus and a policy engine. The new components are well-designed, typed, and thoroughly tested. The integration into the existing configuration management appears correct. I have identified one critical issue in the PolicyEngine's argument matching logic that could lead to security policies being bypassed. I've included a specific code suggestion to address this vulnerability. Apart from this, the changes are excellent.
gemini-cli
bot
added
kind/enhancement
priority/p2
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
This was referenced Sep 5, 2025
Fixes the security vulnerability identified by Gemini Code Assist where JSON.stringify() could produce different output for the same arguments due to non-deterministic property ordering. This could allow security policies to be bypassed. Changes: - Implement stableStringify() method with sorted keys for consistent output - Ensure pattern matching works regardless of argument property order - Add comprehensive tests for order-independent matching - Test nested objects to ensure deep stability This ensures PolicyEngine rules cannot be bypassed by reordering object properties in tool arguments.
Contributor
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a solid foundation for a tool confirmation message bus and a policy engine. The code is well-structured, clearly written, and includes comprehensive unit tests. The new MessageBus and PolicyEngine classes are well-designed and their integration into the Config class is clean. I've found one critical security vulnerability related to a potential denial-of-service in the PolicyEngine's stableStringify method, for which I've provided a detailed comment and a suggested fix. Besides this issue, the implementation is excellent.
…eStringify Addresses the denial-of-service vulnerability identified in code review where circular references in tool arguments could cause infinite recursion and stack overflow in the stableStringify method. Changes: - Implement WeakSet tracking to detect and handle circular references - Replace recursive calls with inner stringify function that tracks visited objects - Return "[Circular]" placeholder when circular reference detected - Add comprehensive tests for both shallow and deep circular references - Use proper TypeScript types instead of 'any' in tests Security impact: - Prevents potential DoS attacks via crafted tool arguments - Ensures PolicyEngine remains robust against malformed input - Maintains stable stringification while preventing infinite recursion
Contributor
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request lays a solid foundation for the Tool Confirmation Message Bus and Policy Engine. The code is well-structured, clearly written, and accompanied by an excellent and comprehensive suite of unit tests that cover many edge cases. I found one high-severity issue in the stableStringify implementation within the PolicyEngine that could lead to incorrect behavior or errors. My review includes a detailed explanation and a code suggestion to fix it. Overall, this is a great piece of work.
…ringify Fixed multiple security vulnerabilities identified in code review: - Correctly handle repeated non-circular objects (no false positives) - Properly handle undefined and function values per JSON spec - Use Set with ancestor chain tracking instead of WeakSet - Ensure generated JSON is always valid - Add comprehensive test coverage for edge cases The stableStringify method now: - Tracks ancestor chain per branch to detect true circular references - Omits undefined/function values from objects (per JSON spec) - Converts undefined/functions to null in arrays (per JSON spec) - Produces deterministic output with sorted keys for consistent pattern matching
Contributor
Author
|
/gemini review |
- Wrap array test inputs in objects to match Record<string, unknown> type - Add explicit type annotation for testCases array - Ensure all test inputs have correct types for args parameter
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a solid foundation for tool confirmation handling with a new MessageBus and PolicyEngine. The code is well-structured and comes with an impressive and comprehensive set of unit tests. I've found a critical issue in the stableStringify implementation that could lead to incorrect policy enforcement, and a high-severity issue regarding inconsistent use of readonly for a mutable property. Addressing these will make the implementation much more robust and secure.
…mentation Fixes based on code review: - Remove readonly modifier from rules array since it's mutated by addRule/removeRulesForTool - Add support for toJSON methods per JSON.stringify specification - Add comprehensive documentation for stableStringify method - Add tests for toJSON handling including edge cases The stableStringify method now fully respects the JSON specification while maintaining deterministic output for security policy matching.
Contributor
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a solid foundation for the Tool Confirmation Message Bus. The new MessageBus and PolicyEngine classes are well-designed and provide a decoupled, event-driven way to handle tool confirmations. The PolicyEngine is particularly robust, with a priority-based rule system and a secure, deterministic method for serializing tool arguments. The unit test coverage is excellent and covers many edge cases, including complex object structures and circular references.
I have one high-severity suggestion for the MessageBus implementation to improve its error handling robustness by using a safe JSON stringification method, preventing potential crashes from circular references in tool arguments.
Replace JSON.stringify with safeJsonStringify to handle potential circular references in message objects. This prevents TypeError exceptions when logging invalid messages with circular references in toolCall.args.
abhipatel12
reviewed
Sep 9, 2025
Contributor
abhipatel12
left a comment
abhipatel12
left a comment
There was a problem hiding this comment.
These changes look great! Really excited about an event bus. Left a couple of comments. LMK what you think!
| import { MessageBusType, type Message } from './types.js'; | ||
| import { safeJsonStringify } from '../utils/safeJsonStringify.js'; | ||
|
|
||
| export class MessageBus extends EventEmitter { |
Member
There was a problem hiding this comment.
Thinking out loud, I know we want to eliminate dependencies and things, but if we add this generic messageBus we should probably consider whether or not we should use another library for events/messaging.
RXJS is pretty heavy, but has a lot of utilities for dealing with event streams and the like. It can be a pain to learn, but has a mountain of flexibility.
Signals has also been proposed in TC39 - https://github.com/tc39/proposal-signals . Internally, Wiz and Angular have generalized signal implementations.
While potentially dismissible as overkill, I think we should probably consider solutions for messaging (or eliminate them).
Contributor
There was a problem hiding this comment.
Interesting suggestion and this has been on my mind as well. However I'd prefer we go with EventEmitter for now. RXJS could be interesting to consider in the future and I'm not sure our state is yet complex enough for it to be worth it. I would expect that if we decide to migrate, Gemini CLI could make the migration surprisingly easy in the future so I don't think we need to adopt RXJS until we really have cases where the complexity is worth the costs.
Member
There was a problem hiding this comment.
(To be clear this is more "commentary" than an ask. No action is expected here for the interim).
Contributor
Author
There was a problem hiding this comment.
I read through the RXJS and the TC39. I really appreciate the pointers and the suggestions. Thank you!
This commit addresses several comments from PR #7835. - Refactors `removeRulesForTool` in `PolicyEngine` to use `.filter()` for better readability. - Extracts `stableStringify` into its own utility file and makes `ruleMatches` a file-level function. - Adds structural types to message objects in `message-bus.test.ts` to improve type safety. - Makes `ToolExecutionSuccess` and `ToolExecutionFailure` interfaces generic for more flexible typing. - Fixes failing tests in `message-bus.test.ts`.
abhipatel12
approved these changes
Sep 10, 2025
Contributor
abhipatel12
left a comment
abhipatel12
left a comment
There was a problem hiding this comment.
This looks great! LGTM
jacob314
reviewed
Sep 10, 2025
| import { MessageBusType, type Message } from './types.js'; | ||
| import { safeJsonStringify } from '../utils/safeJsonStringify.js'; | ||
|
|
||
| export class MessageBus extends EventEmitter { |
Contributor
There was a problem hiding this comment.
Interesting suggestion and this has been on my mind as well. However I'd prefer we go with EventEmitter for now. RXJS could be interesting to consider in the future and I'm not sure our state is yet complex enough for it to be worth it. I would expect that if we decide to migrate, Gemini CLI could make the migration surprisingly easy in the future so I don't think we need to adopt RXJS until we really have cases where the complexity is worth the costs.
| return false; | ||
| } | ||
| // Use stable JSON stringification with sorted keys to ensure consistent matching | ||
| const argsString = this.stableStringify(toolCall.args); |
Contributor
There was a problem hiding this comment.
should we cache the computation of stableStringify so we don't compute this again for each rule that might match?
| } | ||
|
|
||
| // Create new ancestors set for this branch | ||
| const newAncestors = new Set(ancestors); |
Contributor
There was a problem hiding this comment.
performance nit: can you remove ancestors when leaving a branch rather than creating a new set.
| if (Array.isArray(currentObj)) { | ||
| const items = currentObj.map((item) => { | ||
| // undefined and functions in arrays become null | ||
| if (item === undefined || typeof item === 'function') { |
Contributor
There was a problem hiding this comment.
nit: also check for null here to avoid recursing for this primitive case
| * stableStringify(obj) | ||
| * // Returns: '{"safe":"data"}' | ||
| */ | ||
| private stableStringify(obj: unknown): string { |
Contributor
There was a problem hiding this comment.
generally I worry this method is likely 50X slower than the regular stringify method. Might be worth benchmarking this or at least ensuring we don't call it more than once per tool call
Contributor
Author
There was a problem hiding this comment.
Running benchmark with 10000 iterations...
--- simpleObject ---
JSON.stringify: 0.879ms
stableStringify: 7.036ms
--- nestedObject ---
JSON.stringify: 1.212ms
stableStringify: 8.83ms
--- arrayObject ---
JSON.stringify: 1.061ms
stableStringify: 11.287ms
--- complexObject ---
JSON.stringify: 1.396ms
stableStringify: 6.883ms
Yes slower, not 50x, but have made the changes to ensure it isn't called too often.
This commit addresses performance feedback on the `stableStringify` function and its usage within the `PolicyEngine`. - The `stableStringify` function is optimized to avoid creating new sets for ancestor tracking on each recursion, and to handle nulls from `toJSON` more efficiently. - The `PolicyEngine` now caches the result of `stableStringify` during a `check` operation, preventing it from being called repeatedly for each policy rule with an argument pattern. This significantly reduces the performance overhead when multiple such rules exist.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
giraffe-tree
pushed a commit
to giraffe-tree/gemini-cli
that referenced
this pull request
Oct 10, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR implements the foundation for the Tool Confirmation Message Bus as described in issue #7231. This is the first of three PRs that will fully implement the feature.
What this PR does
MessageBusclass for decoupled tool confirmation handlingPolicyEnginewith configurable rules for tool execution controlConfigclassKey Features
Testing
Next Steps
Fixes part of #7231