Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions internal/llminternal/base_flow.go
Original file line number Diff line number Diff line change
Expand Up @@ -760,6 +760,9 @@ func mergeEventActions(base, other *session.EventActions) *session.EventActions
if base == nil {
return other
}
if other.ExitLoop {
base.ExitLoop = true
}
Comment on lines +763 to +765
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The new ExitLoop logic is correctly integrated here. It's good that ExitLoop takes precedence or is handled alongside SkipSummarization during merging, ensuring the agent stops when intended.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Gemini :)

if other.SkipSummarization {
base.SkipSummarization = true
}
Expand Down
14 changes: 8 additions & 6 deletions internal/toolinternal/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,11 +123,13 @@ func (c *toolContext) RequestConfirmation(hint string, payload any) error {
Confirmed: false,
Payload: payload,
}
// SkipSummarization stops the agent loop after this tool call. Without it,
// the function response event becomes lastEvent and IsFinalResponse() returns
// false (hasFunctionResponses == true), causing the loop to continue.
// This matches the behavior of the built-in RequireConfirmation path in
// functiontool (function.go).
c.eventActions.SkipSummarization = true
c.ExitLoop()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Replacing the direct assignment to SkipSummarization with a call to c.ExitLoop() is a good refactoring. It centralizes the logic for exiting the loop and aligns with the new API.

Suggested change
c.ExitLoop()
c.ExitLoop()

return nil
}

func (c *toolContext) ExitLoop() {
c.eventActions.ExitLoop = true
// Also set SkipSummarization for backward compatibility with older code
// that may not know about ExitLoop yet.
c.eventActions.SkipSummarization = true
}
30 changes: 25 additions & 5 deletions internal/toolinternal/context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func TestToolContext(t *testing.T) {
}
}

func TestRequestConfirmation_SetsSkipSummarization(t *testing.T) {
func TestRequestConfirmation_SetsExitLoop(t *testing.T) {
inv := contextinternal.NewInvocationContext(t.Context(), contextinternal.InvocationContextParams{})
actions := &session.EventActions{}
toolCtx := NewToolContext(inv, "fn1", actions, nil)
Expand All @@ -44,8 +44,8 @@ func TestRequestConfirmation_SetsSkipSummarization(t *testing.T) {
t.Fatalf("RequestConfirmation returned unexpected error: %v", err)
}

if !actions.SkipSummarization {
t.Error("RequestConfirmation did not set SkipSummarization to true")
if !actions.ExitLoop {
t.Error("RequestConfirmation did not set ExitLoop to true")
Comment on lines +47 to +48
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The assertion should be updated to check actions.ExitLoop instead of actions.SkipSummarization to align with the new API.

Suggested change
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.RequestedToolConfirmations == nil {
Expand Down Expand Up @@ -74,8 +74,8 @@ func TestRequestConfirmation_AutoGeneratesIDWhenEmpty(t *testing.T) {
t.Fatalf("RequestConfirmation returned unexpected error: %v", err)
}

if !actions.SkipSummarization {
t.Error("SkipSummarization 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")
Comment on lines +77 to +78
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The assertion should be updated to check actions.ExitLoop instead of actions.SkipSummarization to align with the new API.

Suggested change
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")

}
if len(actions.RequestedToolConfirmations) != 1 {
t.Fatalf("expected 1 confirmation entry, got %d", len(actions.RequestedToolConfirmations))
Expand All @@ -89,3 +89,23 @@ func TestRequestConfirmation_AutoGeneratesIDWhenEmpty(t *testing.T) {
}
}
}

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()")
}
// SkipSummarization should also be set for backward compatibility
if !actions.SkipSummarization {
t.Error("SkipSummarization should be true for backward compatibility")
}
}
10 changes: 7 additions & 3 deletions session/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ type Event struct {
// Note: when multiple agents participate in one invocation, there could be
// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
if e.Actions.ExitLoop || e.Actions.SkipSummarization || len(e.LongRunningToolIDs) > 0 {
if e.Actions.ExitLoop || e.Actions.SkipSummarization || len(e.LongRunningToolIDs) > 0 {

return true
}

Expand Down Expand Up @@ -150,8 +150,12 @@ type EventActions struct {

RequestedToolConfirmations map[string]toolconfirmation.ToolConfirmation

// If true, it won't call model to summarize function response.
// Only valid for function response event.
// 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
Comment on lines +153 to +156
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The addition of the ExitLoop field with a clear and comprehensive comment explains its purpose and usage, which is crucial for maintainability and understanding the new API.


// Deprecated: Use ExitLoop instead. SkipSummarization is kept for backward compatibility.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
// Deprecated: Use ExitLoop instead. SkipSummarization is kept for backward compatibility.
// Deprecated: Use ExitLoop instead. SkipSummarization is kept for backward compatibility.

SkipSummarization bool
// If set, the event transfers to the specified agent.
TransferToAgent string
Expand Down
4 changes: 1 addition & 3 deletions tool/agenttool/agent_tool.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,7 @@ func (t *agentTool) Run(toolCtx tool.Context, args any) (map[string]any, error)
}

if t.skipSummarization {
if actions := toolCtx.Actions(); actions != nil {
actions.SkipSummarization = true
}
toolCtx.ExitLoop()
}

var agentInputSchema *genai.Schema
Expand Down
12 changes: 6 additions & 6 deletions tool/agenttool/agent_tool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ func TestAgentTool_Run_SkipSummarization(t *testing.T) {
agent := createAgentWithModel(t, nil, nil, testLLM)
toolCtx := createToolContext(t, agent)

// Test with skipSummarization = true
// Test with skipSummarization = true (sets ExitLoop)
agentToolSkip := agenttool.New(agent, &agenttool.Config{SkipSummarization: true})
actions := toolCtx.Actions()
toolImpl, ok := agentToolSkip.(toolinternal.FunctionTool)
Expand All @@ -283,8 +283,8 @@ func TestAgentTool_Run_SkipSummarization(t *testing.T) {
if err != nil {
t.Fatalf("Run() with skipSummarization=true failed unexpectedly: %v", err)
}
if !actions.SkipSummarization {
t.Errorf("SkipSummarization flag not set when AgentTool was created with skipSummarization=true")
if !actions.ExitLoop {
t.Errorf("ExitLoop flag not set when AgentTool was created with skipSummarization=true")
}

// Test with skipSummarization = false
Expand All @@ -293,7 +293,7 @@ func TestAgentTool_Run_SkipSummarization(t *testing.T) {
if !ok {
t.Fatal("agentToolNoSkip does not implement FunctionTool")
}
actions.SkipSummarization = false // Reset
actions.ExitLoop = false // Reset
// Reset mock for the second call
testLLM.Responses = []*genai.Content{
genai.NewContentFromText("test response", genai.RoleModel),
Expand All @@ -303,8 +303,8 @@ func TestAgentTool_Run_SkipSummarization(t *testing.T) {
if err != nil {
t.Fatalf("Run() with skipSummarization=false failed unexpectedly: %v", err)
}
if actions.SkipSummarization {
t.Errorf("SkipSummarization flag was set when AgentTool was created with skipSummarization=false")
if actions.ExitLoop {
t.Errorf("ExitLoop flag was set when AgentTool was created with skipSummarization=false")
}
}

Expand Down
2 changes: 1 addition & 1 deletion tool/exitlooptool/tool.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ type EmptyArgs struct{}

func exitLoop(ctx tool.Context, myArgs EmptyArgs) (map[string]string, error) {
ctx.Actions().Escalate = true
ctx.Actions().SkipSummarization = true
ctx.ExitLoop()
return map[string]string{}, nil
}

Expand Down
2 changes: 1 addition & 1 deletion tool/functiontool/function.go
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ func (f *functionTool[TArgs, TResults]) Run(ctx tool.Context, args any) (result
if err != nil {
return nil, err
}
ctx.Actions().SkipSummarization = true
// RequestConfirmation calls ExitLoop() internally
return nil, fmt.Errorf("error tool %q requires confirmation, please approve or reject", f.Name())
}
}
Expand Down
2 changes: 1 addition & 1 deletion tool/mcptoolset/tool.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ func (t *mcpTool) Run(ctx tool.Context, args any) (map[string]any, error) {
if err != nil {
return nil, err
}
ctx.Actions().SkipSummarization = true
// RequestConfirmation calls ExitLoop() internally
return nil, fmt.Errorf("error tool %q requires confirmation, please approve or reject", t.Name())
}
}
Expand Down
5 changes: 5 additions & 0 deletions tool/tool.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,11 @@ type Context interface {
// - error: If there was a failure in initiating the confirmation process itself (e.g., invalid
// arguments, issue with the event system). The request to ask the user has not been sent.
RequestConfirmation(hint string, payload any) error

// 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()
Comment on lines +91 to +94
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The ExitLoop() method is a valuable addition to the tool.Context interface. Its clear documentation explains its purpose and when to use it, which enhances the usability and understanding of the API.

}

// Toolset is an interface for a collection of tools. It allows grouping
Expand Down