refactor(core): decouple scheduler into orchestration, policy, and confirmation#16895
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 introduces a significant architectural overhaul to the core 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
|
|
Size Change: -2 B (0%) Total Size: 23.2 MB ℹ️ View Unchanged
|
![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 introduces a significant and well-executed refactoring of the scheduler, decoupling orchestration, policy, and confirmation logic into distinct modules. The new "Clean Sweep" architecture is a major improvement, enhancing modularity, testability, and type safety. The addition of comprehensive integration tests for the new scheduler, policy, and confirmation modules is excellent and provides strong confidence in the correctness of the changes.
My review includes one critical feedback item regarding error handling in the waitForConfirmation function, where a fire-and-forget promise could lead to silently swallowed errors and a confusing user experience. Addressing this will further improve the robustness of the new confirmation flow.
| async function waitForConfirmation( | ||
| messageBus: MessageBus, | ||
| correlationId: string, | ||
| signal: AbortSignal, | ||
| ideConfirmation?: Promise<DiffUpdateResult>, | ||
| ): Promise<ConfirmationResult> { | ||
| if (ideConfirmation) { | ||
| // Pipe IDE resolution to MessageBus. This ensures that the coordinator | ||
| // correctly cleans up its listener when the IDE responds first. | ||
| void (async () => { | ||
| try { | ||
| const resolution = await ideConfirmation; | ||
| if (signal.aborted) return; | ||
|
|
||
| await messageBus.publish({ | ||
| type: MessageBusType.TOOL_CONFIRMATION_RESPONSE, | ||
| correlationId, | ||
| confirmed: resolution.status === 'accepted', | ||
| outcome: | ||
| resolution.status === 'accepted' | ||
| ? ToolConfirmationOutcome.ProceedOnce | ||
| : ToolConfirmationOutcome.Cancel, | ||
| payload: resolution.content | ||
| ? { newContent: resolution.content } | ||
| : undefined, | ||
| }); | ||
| } catch (error) { | ||
| debugLogger.warn('Error waiting for confirmation via IDE', error); | ||
| } | ||
| })(); | ||
| } | ||
|
|
||
| return awaitConfirmation(messageBus, correlationId, signal); | ||
| } |
There was a problem hiding this comment.
The use of void (async () => {})() creates a fire-and-forget promise to handle the ideConfirmation. If the ideConfirmation promise rejects (e.g., due to an IDE-side error), the exception is caught and logged as a warning, but it is not propagated. This leads to a confusing user experience where an error in the IDE confirmation flow is silently ignored, and the CLI appears to hang while waiting for a TUI confirmation that the user may not be aware of.
To ensure robustness, errors from the IDE confirmation flow should be propagated. This can be achieved by using Promise.race to handle both the message bus listener and the IDE confirmation flow concurrently. This change ensures that if the IDE flow fails, the entire operation fails clearly, providing immediate feedback to the user.
async function waitForConfirmation(
messageBus: MessageBus,
correlationId: string,
signal: AbortSignal,
ideConfirmation?: Promise<DiffUpdateResult>,
): Promise<ConfirmationResult> {
const busListener = awaitConfirmation(messageBus, correlationId, signal);
if (!ideConfirmation) {
return busListener;
}
// Race the message bus listener with the IDE confirmation handler.
// This ensures that if the IDE confirmation promise rejects, the error
// is propagated, rather than being silently ignored.
const ideHandler = async (): Promise<never> => {
const resolution = await ideConfirmation;
if (signal.aborted) {
// If the operation was cancelled (e.g., via TUI), do not proceed.
// Return a promise that never resolves to let the busListener's
// rejection win the race.
return new Promise(() => {});
}
await messageBus.publish({
type: MessageBusType.TOOL_CONFIRMATION_RESPONSE,
correlationId,
confirmed: resolution.status === 'accepted',
outcome:
resolution.status === 'accepted'
? ToolConfirmationOutcome.ProceedOnce
: ToolConfirmationOutcome.Cancel,
payload: resolution.content
? { newContent: resolution.content }
: undefined,
});
// This promise should never resolve. Its purpose is to either reject if
// `ideConfirmation` fails, or to hang and let `busListener` resolve
// after we publish the message.
return new Promise(() => {});
};
return Promise.race([busListener, ideHandler()]);
}
| tool, | ||
| response: createErrorResponse( | ||
| request, | ||
| e instanceof Error ? e : new Error(String(e)), |
There was a problem hiding this comment.
I think we have getErrorMessage(), can we use that here?
There was a problem hiding this comment.
We do, but that returns a string. Are you saying we should refactor createErrorResponse to take a string instead?
There was a problem hiding this comment.
Oh good point. I have a slight preference for encapsulating logic like this in the helper function but this also seems fine.
| if (ideConfirmation) { | ||
| // Pipe IDE resolution to MessageBus. This ensures that the coordinator | ||
| // correctly cleans up its listener when the IDE responds first. | ||
| void (async () => { |
There was a problem hiding this comment.
Why does this need to be fire and forgotten?
There was a problem hiding this comment.
I switched it to use Promise.race to make it a bit clearer. lmk what you think
There was a problem hiding this comment.
Yeah I think that's better, though I'd still wonder if you could drop the .then() and .catch() as well and return a value from Promise.race() to determine what happens next.
gundermanc
left a comment
gundermanc
left a comment
There was a problem hiding this comment.
Approved with some suggestions. Potential unresolved promise hang?
| spanMetadata.input = requests; | ||
|
|
||
| if (this.isProcessing || this.state.isActive) { | ||
| return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
Another thing I'm realizing looks odd here is we're returning a new Promise<> inside of an async lambda.
Does that mean that the return type of the lambda is Promise<Promise<>>?
There was a problem hiding this comment.
ts should be flattening nested promises. I've extracted things out now. Let me know if this looks better now
abhipatel12
force-pushed
the
abhi/event-driven-tools.5
branch
from
539182b to
c987704
Compare
gemini-cli
Bot
added
the
area/agent
| payload: resolution.content | ||
| ? { newContent: resolution.content } | ||
| : undefined, | ||
| }) as ConfirmationResult, |
…to orchestration, policy, and confirmation (google-gemini#16895)
3 participants
Summary
This PR refactors the
Schedulerinto a "Clean Sweep" architecture, decouplingorchestration from business logic. It extracts policy enforcement and user
confirmation into dedicated, functional modules (
policy.ts,confirmation.ts), leavingScheduler.tsas a pure event-driven orchestrator.This eliminates race conditions, enforces strict type safety, and improves
testability.
This is a no-op PR, just splitting the code into different files. This code is
not being used anywhere currently.
Once this PR is merged, we will start using this code in the main agent's loop.
Details
policy.tsfor permission checks (YOLO, AllowedTools, MCP scopes) and
confirmation.tsfor the interactive TUI/IDE loop.Schedulernow delegates decision-making, focusing solelyon queue management and execution lifecycle.
WeakSetforMessageBuslistenerde-duplication.
scheduler.test.tscovering batch auto-approval, cancellation flows, and executor status
handling.
Related Issues
Fixes #14306
How to Validate
npm run testto verify the new test suite (scheduler.test.ts,policy.test.ts,confirmation.test.ts).npm run preflightto ensure build integrity.Pre-Merge Checklist