-
Notifications
You must be signed in to change notification settings - Fork 1.2k
refactor(tools): refactor all Tools to return ToolboxError #2403
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @duwenxin99, 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 refactors the error handling strategy across the tools module. By introducing a new, structured Highlights
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request significantly refactors error handling across all tools by introducing the ToolboxError interface, standardizing structured error reporting and consistently applying changes across all Invoke methods. A critical systemic security issue was identified: resource ID parameters (e.g., project, location, cluster) are passed to the underlying GCP API without validation. This vulnerability allows for API path manipulation via path traversal, potentially leading to unauthorized access, especially when the toolbox uses its own service account credentials (ADC). Strict regex validation is recommended for all such parameters. Additionally, the review includes suggestions for improving parameter validation by checking for empty strings, addresses a bug in NewAgentError usage, and proposes improvements for the new ProecessGcpError function, including a typo and a recommendation for a better default error type.
internal/tools/alloydb/alloydbwaitforoperation/alloydbwaitforoperation.go
Outdated
Show resolved
Hide resolved
internal/tools/alloydb/alloydbwaitforoperation/alloydbwaitforoperation.go
Show resolved
Hide resolved
internal/tools/alloydb/alloydbwaitforoperation/alloydbwaitforoperation.go
Show resolved
Hide resolved
internal/tools/cloudsqlmssql/cloudsqlmssqlcreateinstance/cloudsqlmssqlcreateinstance.go
Show resolved
Hide resolved
70777b9 to
e9184c5
Compare
e9184c5 to
08bc740
Compare
| source, err := tools.GetCompatibleSource[compatibleSource](resourceMgr, t.Source, t.Name, t.Type) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, util.NewClientServerError("source used is not compatible with the tool", http.StatusInternalServerError, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it is probably a naive question, but what is the rationale to have different error types tied to detailed reasons for such errors?
I imagine maybe there are automated handling based on error type? Maybe there are metrics built upon that? The trade-off, obviously, is that the developers will have to
- be familiar with all these detailed error types, or
- copy/paste what existed, or
- write their own if the meaning/reason, based on individual developers interpretation, does not already exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are only two new error types - AgentError and ClientServerError. The main rationale to distinguish between them is to return 200 OK status code for all agent errors (with error message in the response body) to allow the agent to self-correct. Therefore, detailed error message for AgentError allows LLM to understand the cause of error; for ClientServerError, it allows developers to debug more easily.
This refactor does increase the learning curve for the contributors, but I will add a more detailed guide in the documentation to make sure the categorization and expectations are clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.
But interestingly, why do you want to return 200 for errors? Isn't it conventional to return 4xx-5xx for errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I am a little confused about the difference between those 2 errors...
Say ClientServerError is for developers, does it mean that the error cannot be resolved automatically? In the code above, the error is used in incompatible source. Is it possible to update the source type using prompt?
Another error example NewAgentError, it is used for missing password. This is cannot be resolve automatically, right?
I guess I must have missed something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The source type used is defined in the server configuration yaml file so that can only changed by the developers.
In this case, the password is passed in as a tool invocation parameter, then it should be an AgentError because the agent provides these params.
955633b to
cfbd82b
Compare
46ac26b to
e1ae3a2
Compare
util.ProcessGcpErrorfor GCP toolsutil.ProcessGeneralErrorfor all other tools.Error categorization in Invoke():