feat(core): wire up UI for ASK_USER policy decisions in message bus #10630
Conversation
Integrates the message bus ASK_USER flow with CoreToolScheduler to show legacy confirmation UI when policy engine requires user confirmation. Changes: - Add `requiresUserConfirmation` flag to ToolConfirmationResponse type - Update BaseToolInvocation to handle ASK_USER by returning null sentinel - Wire CoreToolScheduler to subscribe to TOOL_CONFIRMATION_REQUEST events - Integrate WebFetchTool as first end-to-end test of ASK_USER flow - Update all test mocks to include new Config methods Flow: 1. Tool calls shouldConfirmExecute() → publishes TOOL_CONFIRMATION_REQUEST 2. MessageBus checks PolicyEngine → ASK_USER decision 3. CoreToolScheduler receives request → responds with requiresUserConfirmation=true 4. Tool receives null → falls back to legacy confirmation UI 5. User sees full confirmation dialog with diffs, URLs, options Tested: - ALLOW policy → auto-executes without UI - DENY policy → blocks execution with error - ASK_USER policy → shows full legacy confirmation UI All preflight checks passing (format, lint, build, typecheck, tests).
Summary of ChangesHello @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 significantly enhances the tool execution policy system by fully integrating the 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
|
![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 successfully wires up the UI for ASK_USER policy decisions from the message bus. The approach of having the CoreToolScheduler respond to TOOL_CONFIRMATION_REQUEST messages to trigger the legacy confirmation UI in the tool is sound. The changes are well-structured and include comprehensive integration tests for the new flow. My main concern is the use of a type-unsafe null sentinel value in BaseToolInvocation to signal the ASK_USER case to subclasses, which undermines the type system and could be a source of bugs in the future. I've left a specific comment with a suggestion for a safer implementation.
|
Size Change: +1 kB (+0.01%) Total Size: 17.8 MB
ℹ️ View Unchanged
|
Refactors the message bus confirmation flow to avoid using a `null` sentinel value. Instead, it now uses a string enum (`ALLOW`, `DENY`, `ASK_USER`) to represent the decision from the message bus. This change improves the type safety of the interaction between the base tool invocation and its subclasses. The following files were updated: - `packages/core/src/tools/tools.ts`: Updated `BaseToolInvocation` with the new `getMessageBusDecision` method. - `packages/core/src/tools/web-fetch.ts`: Updated `WebFetchToolInvocation` to use the new method. - `packages/core/src/tools/message-bus-integration.test.ts`: Updated the tests to reflect the changes in the implementation.
| * When true, indicates that policy decision was ASK_USER and the tool should | ||
| * show its legacy confirmation UI instead of auto-proceeding. | ||
| */ | ||
| requiresUserConfirmation?: boolean; |
There was a problem hiding this comment.
Does this need to be optional?
There was a problem hiding this comment.
The way this is written right now is that this only comes into play when the ASK_USER policy decision is set. Otherwise it's null and falls through to the legacy behviour.
| override async shouldConfirmExecute( | ||
| abortSignal: AbortSignal, | ||
| ): Promise<false> { | ||
| if (this.messageBus) { |
There was a problem hiding this comment.
should this conditional always evaluate to true? Can we assert on it instead of making it a conditional?
There was a problem hiding this comment.
The conditional supports the case where there is no message bus in the test. It's flag guarded so I wanted to test both scenarios. See the test on line 222 for example.
Addresses a comment on the message bus integration test to clarify the intent of a conditional. The `shouldConfirmExecute` method in `TestToolInvocation` now includes a comment explaining why a conditional is used to check for the presence of the message bus, which is necessary for testing the fallback behavior when the message bus is not available.
packages/core/src/tools/tools.ts
Outdated
|
|
||
| timeoutId = setTimeout(() => { | ||
| cleanup(); | ||
| resolve('ALLOW'); // Default to ALLOW on timeout |
There was a problem hiding this comment.
Why do we default to allow for timeout here?
There was a problem hiding this comment.
Good point. I've changed this to ASK_USER.
packages/core/src/tools/tools.ts
Outdated
| } else if (response.confirmed) { | ||
| resolve('ALLOW'); | ||
| } else { | ||
| reject(new Error('Tool execution denied by policy')); |
There was a problem hiding this comment.
Why do we reject instead of resolve to DENY here?
There was a problem hiding this comment.
This was left over from an older implementation before I had the status in the promise. I've changed it to resolve to DENY now to be consistent.
This commit addresses feedback on the message bus confirmation flow by making it more robust and secure. - The `getMessageBusDecision` method now resolves with `'DENY'` instead of rejecting the promise when tool execution is denied or aborted. - The timeout behavior has been changed to resolve to `'ASK_USER'` as a safer default, ensuring user confirmation is requested when the message bus times out. - `console.log` has been changed to `console.debug` for debug-level logging. The tests for `web-fetch` and `message-bus-integration` have been updated to be consistent with these changes.
…cisions in message bus (google-gemini#10630)
3 participants
Summary
Completes the message bus integration by wiring up the UI for
ASK_USERpolicy decisions. When the PolicyEngine decides that a tool requires user confirmation, the system now shows the full legacy confirmation UI instead of blocking or auto-executing.Changes
Core Infrastructure
confirmation-bus/types.ts: AddedrequiresUserConfirmationflag toToolConfirmationResponsetools/tools.ts: UpdatedBaseToolInvocation.handleMessageBusConfirmation()to handle ASK_USER by returningnullas a sentinel valuecore/coreToolScheduler.ts:TOOL_CONFIRMATION_REQUESTmessages in constructor (whenenableMessageBusIntegrationis true)handleToolConfirmationRequest()method to respond withrequiresUserConfirmation=truefor ASK_USER decisionsIntegration & Testing
tools/web-fetch.ts:shouldConfirmExecute()to check fornullreturn and fall through to legacy confirmation UItools/web-fetch.test.ts: Added 8 comprehensive message bus integration testsgetEnableMessageBusIntegration(),getMessageBus(),getPolicyEngine()Flow
When
enableMessageBusIntegration: truein settings:ASK_USER Flow Details
Test Plan
tools.enableMessageBusIntegration: trueNext Steps
This PR enables the ASK_USER flow for WebFetchTool. Follow-up work: