feat: add ADK AI Chat agent sample #276
Conversation
Summary of ChangesHello @PierrickVoulet, 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 adds a new sample application that showcases the integration of a Google Chat bot with an ADK AI agent hosted on Vertex AI Agent Engine. The sample provides a functional framework for processing user messages, interacting with the AI agent, and presenting the agent's responses, including grounded information and markdown, directly within Google Chat. It serves as a quickstart guide for developers looking to build AI-powered Chat applications on Google Workspace. 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 adds a sample ADK AI Chat agent. The code is well-structured, but there are a few critical areas that need improvement for robustness and security. My review includes feedback on:
- Lack of error handling for API calls, which could lead to unhandled exceptions.
- A potential Cross-Site Scripting (XSS) vulnerability due to unsanitized HTML rendering.
- Fragile logic for parsing environment variables which could cause runtime errors.
I've provided suggestions to address these high-severity issues to make the sample more secure and reliable.
| function createMarkdownWidgets(markdown) { | ||
| if (!markdown) return []; | ||
| const textParagraph = CardService.newTextParagraph(); | ||
| textParagraph.setText(new showdown.Converter().makeHtml(markdown)); |
There was a problem hiding this comment.
The makeHtml function from the showdown library is used to convert Markdown to HTML. However, the output is not sanitized. Since the markdown content comes from an AI agent, it should be treated as untrusted input. This could potentially lead to a Cross-Site Scripting (XSS) vulnerability if the AI returns malicious HTML or JavaScript within the markdown. While Google Chat performs its own sanitization on card content, it is a security best practice to sanitize untrusted HTML content within your application. Consider using an HTML sanitization library or configuring showdown to sanitize its output if the version you are using supports it.
| // Request AI agent to answer the message | ||
| requestAgent(chatEvent.user.name, chatEvent.messagePayload.message) |
There was a problem hiding this comment.
The onMessage function, which is the entry point for chat messages, calls requestAgent directly. Any unhandled exceptions within requestAgent or the functions it calls (like network errors or parsing errors) will cause the script to terminate. This will result in a generic error message for the user in Google Chat, such as "App is not responding", which is not a good user experience. Wrapping the call to requestAgent in a try...catch block will handle potential errors gracefully and can provide a more informative error message to the user.
// Request AI agent to answer the message
try {
requestAgent(chatEvent.user.name, chatEvent.messagePayload.message);
} catch (e) {
console.error('Failed to process agent request: %s', e);
// Let the user know something went wrong.
createMessage({ text: "Sorry, I encountered an error and couldn't process your request." });
}| function downloadChatAttachment(attachmentName) { | ||
| const response = UrlFetchApp.fetch( | ||
| `https://chat.googleapis.com/v1/media/${attachmentName}?alt=media`, | ||
| { | ||
| method: 'get', | ||
| headers: { 'Authorization': `Bearer ${getCredentials().getAccessToken()}` }, | ||
| muteHttpExceptions: true | ||
| } | ||
| ); | ||
| return Utilities.base64Encode(response.getContent()); | ||
| } |
There was a problem hiding this comment.
The UrlFetchApp.fetch call has muteHttpExceptions: true, but there's no subsequent check of the HTTP response code. If downloading the attachment fails (e.g., due to permissions or a transient network issue), response.getContent() might return an empty or error-like content. Calling Utilities.base64Encode on this will not produce the expected result and could lead to downstream errors when the AI agent processes the malformed attachment data. You should check the response code after the fetch call and handle failures appropriately, for example by throwing an error that can be caught by the caller.
| function downloadChatAttachment(attachmentName) { | |
| const response = UrlFetchApp.fetch( | |
| `https://chat.googleapis.com/v1/media/${attachmentName}?alt=media`, | |
| { | |
| method: 'get', | |
| headers: { 'Authorization': `Bearer ${getCredentials().getAccessToken()}` }, | |
| muteHttpExceptions: true | |
| } | |
| ); | |
| return Utilities.base64Encode(response.getContent()); | |
| } | |
| function downloadChatAttachment(attachmentName) { | |
| const response = UrlFetchApp.fetch( | |
| `https://chat.googleapis.com/v1/media/${attachmentName}?alt=media`, | |
| { | |
| method: 'get', | |
| headers: { 'Authorization': `Bearer ${getCredentials().getAccessToken()}` }, | |
| muteHttpExceptions: true | |
| } | |
| ); | |
| if (response.getResponseCode() !== 200) { | |
| const errorText = response.getContentText(); | |
| console.error(`Failed to download attachment ${attachmentName}. Status: ${response.getResponseCode()}. Response: ${errorText}`); | |
| throw new Error(`Failed to download attachment: ${attachmentName}`); | |
| } | |
| return Utilities.base64Encode(response.getContent()); | |
| } |
| const LOCATION = PropertiesService.getScriptProperties().getProperty('LOCATION'); | ||
|
|
||
| // Get reasoning engine location | ||
| function getLocation() { | ||
| const parts = REASONING_ENGINE_RESOURCE_NAME.split('/'); | ||
| const locationIndex = parts.indexOf('locations') + 1; | ||
| return parts[locationIndex]; | ||
| } |
There was a problem hiding this comment.
The getLocation() function is fragile. It assumes REASONING_ENGINE_RESOURCE_NAME is always set and has a specific format. If the property is missing or has an unexpected format, this function will throw an error or return an incorrect value. Additionally, there is a LOCATION constant read from script properties on line 24 which is never used, while getLocation() re-parses the location from another property. This is redundant and confusing. You should consistently use one method to get the location. Using the LOCATION script property directly is simpler and more robust.
| const LOCATION = PropertiesService.getScriptProperties().getProperty('LOCATION'); | |
| // Get reasoning engine location | |
| function getLocation() { | |
| const parts = REASONING_ENGINE_RESOURCE_NAME.split('/'); | |
| const locationIndex = parts.indexOf('locations') + 1; | |
| return parts[locationIndex]; | |
| } | |
| const LOCATION = PropertiesService.getScriptProperties().getProperty('LOCATION'); | |
| // Get reasoning engine location | |
| function getLocation() { | |
| if (!LOCATION) { | |
| throw new Error('LOCATION script property must be set.'); | |
| } | |
| return LOCATION; | |
| } |
1 participant
No description provided.