feat: add ExitLoop API for tools to stop agent event loop#583
feat: add ExitLoop API for tools to stop agent event loop#583GuyGoldenberg wants to merge 1 commit intogoogle:mainfrom
Conversation
Summary of ChangesHello @GuyGoldenberg, 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 dedicated Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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
|
There was a problem hiding this comment.
Code Review
The pull request introduces an ExitLoop API to explicitly signal the agent to stop its event loop, replacing the ambiguously named SkipSummarization field. This change improves clarity and maintainability. The modifications include adding the ExitLoop field to EventActions, creating an ExitLoop() method in tool.Context, deprecating SkipSummarization, and migrating existing tools to use the new API. The test cases have also been updated to reflect these changes. The overall change is positive for code clarity and intent.
| if other.ExitLoop { | ||
| base.ExitLoop = true | ||
| } |
| // This matches the behavior of the built-in RequireConfirmation path in | ||
| // functiontool (function.go). | ||
| c.eventActions.SkipSummarization = true | ||
| c.ExitLoop() |
| func (c *toolContext) ExitLoop() { | ||
| c.eventActions.ExitLoop = true | ||
| } |
| if !actions.ExitLoop { | ||
| t.Error("RequestConfirmation did not set ExitLoop to true") |
There was a problem hiding this comment.
The assertion should be updated to check actions.ExitLoop instead of actions.SkipSummarization to align with the new API.
| if !actions.ExitLoop { | |
| t.Error("RequestConfirmation did not set ExitLoop to true") | |
| if !actions.ExitLoop { | |
| t.Error("RequestConfirmation did not set ExitLoop to true") |
| if !actions.ExitLoop { | ||
| t.Error("ExitLoop should be set even with auto-generated function call ID") |
There was a problem hiding this comment.
The assertion should be updated to check actions.ExitLoop instead of actions.SkipSummarization to align with the new API.
| if !actions.ExitLoop { | |
| t.Error("ExitLoop should be set even with auto-generated function call ID") | |
| if !actions.ExitLoop { | |
| t.Error("ExitLoop should be set even with auto-generated function call ID") |
| func TestExitLoop(t *testing.T) { | ||
| inv := contextinternal.NewInvocationContext(t.Context(), contextinternal.InvocationContextParams{}) | ||
| actions := &session.EventActions{} | ||
| toolCtx := NewToolContext(inv, "fn1", actions, nil) | ||
|
|
||
| if actions.ExitLoop { | ||
| t.Error("ExitLoop should be false initially") | ||
| } | ||
|
|
||
| toolCtx.ExitLoop() | ||
|
|
||
| if !actions.ExitLoop { | ||
| t.Error("ExitLoop should be true after calling ExitLoop()") | ||
| } | ||
| } |
| // multiple events with IsFinalResponse() as True, for each participating agent. | ||
| func (e *Event) IsFinalResponse() bool { | ||
| if (e.Actions.SkipSummarization) || len(e.LongRunningToolIDs) > 0 { | ||
| if e.Actions.ExitLoop || e.Actions.SkipSummarization || len(e.LongRunningToolIDs) > 0 { |
There was a problem hiding this comment.
Adding e.Actions.ExitLoop to the IsFinalResponse() condition ensures that the agent correctly recognizes when it should stop its event loop, which is the intended behavior of the new API.
| if e.Actions.ExitLoop || e.Actions.SkipSummarization || len(e.LongRunningToolIDs) > 0 { | |
| if e.Actions.ExitLoop || e.Actions.SkipSummarization || len(e.LongRunningToolIDs) > 0 { |
| // ExitLoop signals the agent to stop its event loop after processing this event. | ||
| // Use this when a tool needs to halt the agent's execution, such as when | ||
| // requiring user confirmation or when the tool's result is the final output. | ||
| ExitLoop bool |
| // requiring user confirmation or when the tool's result is the final output. | ||
| ExitLoop bool | ||
|
|
||
| // Deprecated: Use ExitLoop instead. SkipSummarization is kept for backward compatibility. |
There was a problem hiding this comment.
Deprecating SkipSummarization and providing a clear comment to use ExitLoop instead is good practice for API evolution. It guides users to the new, clearer API while maintaining backward compatibility.
| // Deprecated: Use ExitLoop instead. SkipSummarization is kept for backward compatibility. | |
| // Deprecated: Use ExitLoop instead. SkipSummarization is kept for backward compatibility. |
| // ExitLoop signals the agent to stop its event loop after processing this tool's response. | ||
| // Call this when the tool's result should be the final output, or when the agent should | ||
| // halt execution (e.g., waiting for user input, confirmation, or external action). | ||
| ExitLoop() |
Add a clearer API for tools to signal they want to stop the agent's event loop. The existing SkipSummarization field name was confusing as it didn't describe the actual behavior (stopping the loop). Changes: - Add ExitLoop field to EventActions - Add ExitLoop() method to tool.Context interface - Update IsFinalResponse() to check ExitLoop - Deprecate SkipSummarization (kept for backward compat) - Update exitlooptool, agenttool, functiontool, mcptoolset to use new API
41217c4 to
7f991c2
Compare
Adds
ExitLoop()method totool.Context- a clearer way for tools to signal they want to stop the agent loop.The existing
SkipSummarizationfield was confusingly named since it actually stops the event loop, not just "skips summarization". The new API makes the intent explicit.Changes:
ExitLoopfield toEventActionsExitLoop()method totool.ContextSkipSummarization(kept for backward compat)Test plan: All existing tests pass, added new test for
ExitLoop().