feat: add new functionality to improve user experience #306
Conversation
Signed-off-by: SoulSniper1212 <warush23@gmail.com>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @SoulSniper1212, 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 robust confirmation mechanism for FunctionTools, allowing for both static configuration-based and dynamic runtime-requested user approvals. This enhances safety for critical operations by pausing tool execution until explicit confirmation is received, improving overall user experience and control over agent actions. The changes include new types, interface extensions, core agent flow modifications, and a detailed example. 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 introduces a valuable confirmation feature for tools, enhancing safety for critical operations. The implementation is thorough, covering both static and dynamic confirmation mechanisms, with changes well-integrated across the tool definition, context, session events, and the core agent execution flow. My review includes suggestions for refactoring to improve code clarity and maintainability, clarifying some implementation details, and adding a test case for better coverage. Overall, this is a strong contribution.
| // Check if confirmation was requested | ||
| if toolCtx.Actions().ConfirmationRequest != nil { | ||
| // If confirmation is requested, we need to return an event with the confirmation request | ||
| ev := session.NewEvent(ctx.InvocationID()) | ||
| ev.Author = ctx.Agent().Name() | ||
| ev.Branch = ctx.Branch() | ||
| ev.Actions = *toolCtx.Actions() | ||
| // Set a special status to indicate that confirmation is required | ||
| ev.LLMResponse = model.LLMResponse{ | ||
| Content: &genai.Content{ | ||
| Role: "user", | ||
| Parts: []*genai.Part{ | ||
| { | ||
| Text: fmt.Sprintf("Confirmation required for tool %s: %s", fnCall.Name, toolCtx.Actions().ConfirmationRequest.Hint), | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| // Add custom metadata to indicate this is a confirmation request | ||
| CustomMetadata: map[string]any{ | ||
| "confirmation_required": true, | ||
| "confirmation_request": toolCtx.Actions().ConfirmationRequest, | ||
| }, | ||
| } | ||
| telemetry.TraceToolCall(spans, curTool, fnCall.Args, ev) | ||
| fnResponseEvents = append(fnResponseEvents, ev) | ||
| } else { | ||
| // Normal response when no confirmation needed | ||
| ev := session.NewEvent(ctx.InvocationID()) | ||
| ev.LLMResponse = model.LLMResponse{ | ||
| Content: &genai.Content{ | ||
| Role: "user", | ||
| Parts: []*genai.Part{ | ||
| { | ||
| FunctionResponse: &genai.FunctionResponse{ | ||
| ID: fnCall.ID, | ||
| Name: fnCall.Name, | ||
| Response: result, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
| ev.Author = ctx.Agent().Name() | ||
| ev.Branch = ctx.Branch() | ||
| ev.Actions = *toolCtx.Actions() | ||
| telemetry.TraceToolCall(spans, curTool, fnCall.Args, ev) | ||
| fnResponseEvents = append(fnResponseEvents, ev) | ||
| } |
There was a problem hiding this comment.
There's some duplicated code for creating the session.Event in both the if and else branches. You can refactor this to create the event first, and then populate ev.LLMResponse differently based on whether a confirmation is required. This will make the code more concise and easier to maintain.
ev := session.NewEvent(ctx.InvocationID())
ev.Author = ctx.Agent().Name()
ev.Branch = ctx.Branch()
ev.Actions = *toolCtx.Actions()
// Check if confirmation was requested
if toolCtx.Actions().ConfirmationRequest != nil {
// If confirmation is requested, we need to return an event with the confirmation request
// Set a special status to indicate that confirmation is required
ev.LLMResponse = model.LLMResponse{
Content: &genai.Content{
Role: "user",
Parts: []*genai.Part{
{
Text: fmt.Sprintf("Confirmation required for tool %s: %s", fnCall.Name, toolCtx.Actions().ConfirmationRequest.Hint),
},
},
},
// Add custom metadata to indicate this is a confirmation request
CustomMetadata: map[string]any{
"confirmation_required": true,
"confirmation_request": toolCtx.Actions().ConfirmationRequest,
},
}
} else {
// Normal response when no confirmation needed
ev.LLMResponse = model.LLMResponse{
Content: &genai.Content{
Role: "user",
Parts: []*genai.Part{
{
FunctionResponse: &genai.FunctionResponse{
ID: fnCall.ID,
Name: fnCall.Name,
Response: result,
},
},
},
},
}
}
telemetry.TraceToolCall(spans, curTool, fnCall.Args, ev)
fnResponseEvents = append(fnResponseEvents, ev)| func confirmationRequestProcessor(ctx agent.InvocationContext, req *model.LLMRequest, resp *model.LLMResponse) error { | ||
| // Check if the response contains a confirmation request | ||
| if confirmationReq, ok := resp.CustomMetadata["confirmation_request"]; ok { | ||
| if req.Tools != nil { | ||
| // When there's a confirmation request, we might need to modify the flow | ||
| // For now, we just log that a confirmation is required | ||
| // The actual handling would happen at a higher level | ||
| _ = confirmationReq // Use the confirmation request if needed | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The comment on line 57 mentions logging, but no logging is actually performed. The function's purpose is also a bit unclear as it seems to handle confirmation requests from the LLM response, which is different from the tool-initiated confirmations implemented in the rest of this PR. It would be helpful to either add the logging or update the comment to more accurately reflect the function's current behavior and purpose.
| if toolName == "" { | ||
| // Fallback to function name if tool name isn't set | ||
| toolName = c.functionCallID | ||
| } |
There was a problem hiding this comment.
The fallback to c.functionCallID (which is a UUID) for the toolName can be confusing for consumers of this information. A UUID doesn't provide a meaningful name for the tool. It would be more robust to require a valid toolName to be present in the context when RequestConfirmation is called. Consider returning an error if toolName is empty.
| if toolName == "" { | |
| // Fallback to function name if tool name isn't set | |
| toolName = c.functionCallID | |
| } | |
| if toolName == "" { | |
| // A meaningful tool name is required for a confirmation request. | |
| return fmt.Errorf("tool name is missing in context for confirmation request with hint: %s", hint) | |
| } |
| func TestToolContext_Confirmation(t *testing.T) { | ||
| inv := contextinternal.NewInvocationContext(t.Context(), contextinternal.InvocationContextParams{}) | ||
| actions := &session.EventActions{} | ||
| toolCtx := NewToolContextWithToolName(inv, "fn1", actions, "test_tool") | ||
|
|
||
| hint := "This is a test confirmation" | ||
| payload := map[string]any{"key": "value"} | ||
|
|
||
| // Initially, no confirmation should be requested | ||
| if actions.ConfirmationRequest != nil { | ||
| t.Errorf("ConfirmationRequest should be nil initially, got: %v", actions.ConfirmationRequest) | ||
| } | ||
|
|
||
| // Request confirmation | ||
| err := toolCtx.RequestConfirmation(hint, payload) | ||
| if err == nil { | ||
| t.Errorf("Expected RequestConfirmation to return an error to indicate confirmation is required") | ||
| } | ||
|
|
||
| // Check that confirmation request was stored in actions | ||
| if actions.ConfirmationRequest == nil { | ||
| t.Error("ConfirmationRequest should not be nil after calling RequestConfirmation") | ||
| } else { | ||
| if actions.ConfirmationRequest.Hint != hint { | ||
| t.Errorf("Expected hint %q, got %q", hint, actions.ConfirmationRequest.Hint) | ||
| } | ||
| if actions.ConfirmationRequest.ToolName != "test_tool" { | ||
| t.Errorf("Expected tool name %q, got %q", "test_tool", actions.ConfirmationRequest.ToolName) | ||
| } | ||
| if len(actions.ConfirmationRequest.Payload) != 1 || actions.ConfirmationRequest.Payload["key"] != "value" { | ||
| t.Errorf("Payload was not stored correctly: %v", actions.ConfirmationRequest.Payload) | ||
| } | ||
| } | ||
|
|
||
| // Try to request another confirmation - should fail | ||
| err = toolCtx.RequestConfirmation("Another request", map[string]any{}) | ||
| if err == nil { | ||
| t.Error("Expected second call to RequestConfirmation to fail") | ||
| } | ||
| } |
There was a problem hiding this comment.
This test covers the primary functionality of RequestConfirmation well. To improve test coverage, consider adding a test case for the fallback logic in RequestConfirmation where toolName is not provided during context creation. This would ensure the behavior for that edge case is tested and correct.
| if f.cfg.RequireConfirmation { | ||
| instruction := "NOTE: This tool requires explicit user confirmation before execution." | ||
| if decl.Description != "" { | ||
| decl.Description += "\n\n" + instruction | ||
| } else { | ||
| decl.Description = instruction | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic for appending an instruction to the tool's description is a duplicate of the logic for IsLongRunning just above. To improve maintainability and reduce code duplication, consider extracting this into a shared helper function.
For example:
func appendInstruction(description, instruction string) string {
if description != "" {
return description + "\n\n" + instruction
}
return instruction
}
There was a problem hiding this comment.
It would be preferable if the extra instructions for confirmation were not hard coded or required. We should be able to adjust this depending on the model/agent because one size does not fit all
Resolved conflict in internal/llminternal/base_flow.go by adopting upstream's NewToolContext() instead of NewToolContextWithToolName(). The upstream version uses a simpler approach without passing the tool name parameter. Additional changes from upstream: - Import ordering standardization (genai imports grouped separately) - Minor formatting improvements (blank lines, function signatures) - Documentation comment additions for exported functions
3 participants
Summary
This PR introduces new functionality to enhance the user experience and addresses several bug fixes identified in the previous release.
Changes
Testing
Breaking Changes
None
Dependencies
Please review and approve these changes.