fix(core): resolve scheduler hang and improve policy violation visibility#22462
fix(core): resolve scheduler hang and improve policy violation visibility#22462mattKorwel wants to merge 3 commits intomainfrom
Conversation
…lity This PR addresses three core issues with the Policy Engine and Scheduler: 1. Scheduler Hang: Removed a redundant MessageBus listener in Scheduler.ts that caused race conditions in TTY environments. 2. Policy Visibility: Updated ToolGroupMessage.tsx to always display policy violation errors, regardless of verbosity settings. 3. User Feedback: Added emitFeedback to MessageBus.ts to ensure blocked tool calls are reported to the UI.
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 significantly enhances the stability and user experience of the CLI by addressing critical issues related to tool scheduling and policy enforcement. It resolves a scheduler hang, improves the visibility of policy violations in the UI, and provides clearer feedback to users when tool calls are blocked. Highlights
Changelog
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 effectively addresses the three issues outlined in the description. The removal of the redundant MessageBus listener in Scheduler.ts is a crucial fix for the race condition that caused the scheduler to hang. The updates in ToolGroupMessage.tsx and the associated plumbing of ToolErrorType correctly ensure that policy violation errors are always displayed, improving user visibility. Lastly, the addition of emitFeedback in MessageBus.ts provides essential feedback when a tool call is blocked by policy. The changes are well-targeted and correctly implemented.
|
Size Change: -483 B (0%) Total Size: 26.1 MB
ℹ️ View Unchanged
|
mattKorwel
added
area/core
This commit adds a new visual test suite that specifically targets the Policy Engine and UI feedback mechanisms. It validates that policy blocks are visible and that the app can boot correctly with policy rules active.
This commit: 1. Rips out the redundant MessageBus listener in Scheduler.ts that caused race conditions. 2. Adds a unit test to verify the Scheduler no longer subscribes to TOOL_CONFIRMATION_REQUEST. 3. Adds emitFeedback to MessageBus.ts for policy rejections. 4. Adds a unit test to verify that user feedback is emitted on policy denial.
gemini-cli
bot
added
the
status/need-issue
mattKorwel
left a comment
mattKorwel
left a comment
There was a problem hiding this comment.
🤖 Automated Review Assessment (via Gemini CLI)
This PR provides critical fixes for application stability and user feedback visibility. Functional analysis confirms the architectural soundness of the changes.
✅ Key Improvements
- Scheduler Hang Fix: The removal of the redundant
TOOL_CONFIRMATION_REQUESTlistener inScheduler.tsis a high-impact fix. This listener was causing race conditions in TTY environments by pre-emptively denying confirmation requests. - Policy Visibility: The update to
ToolGroupMessage.tsxcorrectly ensures thatPOLICY_VIOLATIONerrors bypass thelowerror verbosity filter. This resolves the "silent failure" issue where users were left without an explanation for denied tool calls. - System Feedback: Adding
emitFeedbackto the denial case inMessageBus.tscorrectly triggers the necessary UI notifications.
⚠️ Critical Observation: Pending Tests
- In
packages/cli/src/ui/PolicyVisual.test.tsx, the most critical tests (should visually render a DENY decisionandshould visually render an ASK_USER prompt) are currently marked withit.todo. - Action Required: Given that this PR is specifically intended to fix and verify these visibility behaviors, these tests should be enabled and verified as passing before merging.
Recommendation: Approve once the it.todo visual tests are enabled and passing.
gemini-cli
bot
removed
the
status/need-issue
gemini-cli
bot
added
the
status/need-issue
There was a problem hiding this comment.
Code Review
This pull request effectively resolves three critical issues: a scheduler hang caused by a redundant listener, suppressed policy violation errors in the UI, and a lack of feedback from the message bus on policy denials. The changes are well-implemented, with the removal of the problematic listener in the scheduler, updated UI filtering logic to ensure policy errors are always visible, and the addition of an event emission for policy denials. The accompanying tests, including new unit and visual validation tests, provide solid verification for these important fixes. The overall changes significantly improve the application's stability and user experience.
1 participant
Summary
This PR fixes three critical issues in the Policy Engine and Scheduler that caused application hangs and suppressed important user feedback. These issues were identified and verified using the new visual validation framework (#22461).
Problem & Solution
1. Scheduler Hang (Redundant Listener)
Schedulerclass contained a redundantMessageBuslistener that immediately responded to tool confirmation requests. In TTY environments, this caused race conditions during initialization, often leading to a permanent hang in the "Initializing..." state.MessageBusnow handles confirmations correctly without internal competition, resolving the startup hang.packages/cli/src/ui/PolicyVisual.test.tsx; the app now boots correctly.2. Policy Visibility (UI Filtering)
ToolGroupMessage.tsxfiltered out tool errors whenui.errorVerbositywas set tolow(default). Since policy denials are model-initiated, they were being silently hidden from the user, leaving the CLI unresponsive without explanation.DENYwith default settings. Note that no error message appears.DENYand observe that "Tool execution denied by policy" is clearly visible.3. MessageBus Feedback
MessageBusperformed policy checks but did not emit system-level feedback events on denial, making it difficult for the UI to notify the user of a block.emitFeedbackto theDENYcase inMessageBus.tsto ensure system-level error notifications are triggered.src/confirmation-bus/message-bus.test.tsconfirms the feedback event is emitted.Validation
Unit Tests
Proper unit tests have been added/updated to verify these fixes:
packages/core/src/scheduler/scheduler.test.ts: Verifies the Scheduler no longer short-circuits confirmations.packages/core/src/confirmation-bus/message-bus.test.ts: Verifies thatUserFeedbackevents are emitted on denial.Visual Tests
packages/cli/src/ui/PolicyVisual.test.tsx: Validates the full loop from policy rule to UI rendering.