Fix all code#730
Fix all code#730samhiotisiddn-jpg wants to merge 3 commits intogoogleworkspace:chore/sync-skillsfrom
Conversation
This workflow runs tests and publishes a Node.js package to GitHub Packages upon release creation.
Fixed 11 clippy warnings, ran fmt, and passed all tests.
Add top-level agent command with various tools and update workspace dependencies.
|
googleworkspace-bot
added
area: auth
area: distribution
area: http
area: core
|
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, 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 powerful new AI agent capability to the Google Workspace CLI. By integrating with LLM providers, the CLI can now perform complex, multi-step tasks by leveraging a suite of tools, including direct Google Workspace API calls, Notion integration, Zapier automation, and browser-based actions. The implementation includes a robust configuration system, persistent memory management, and a flexible tool registry, significantly expanding the utility of the CLI for automated workflows. Highlights
Ignored Files
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. 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 terminal-based AI agent, gws agent, which supports multi-turn conversations using LLM providers like OpenRouter and Ollama. The implementation includes a tool registry with integrations for Google Workspace, Notion, Zapier, Supabase, and a headless browser, along with persistent memory options. Feedback highlights a resource leak in the browser tool where tabs are not closed, a bypassable recursion check in the self-hosted CLI tool, and potential query parsing issues in the Supabase tool due to overly permissive URL encoding.
| fn run_blocking( | ||
| browser: &headless_chrome::Browser, | ||
| action: &str, | ||
| args: &Value, | ||
| ) -> Result<String, ToolError> { | ||
| let tab = browser | ||
| .new_tab() | ||
| .map_err(|e| ToolError::runtime(format!("new_tab: {e}")))?; | ||
| match action { | ||
| "navigate" => { | ||
| let url = args | ||
| .get("url") | ||
| .and_then(|v| v.as_str()) | ||
| .ok_or_else(|| ToolError::runtime("missing 'url'"))?; | ||
| tab.navigate_to(url) | ||
| .map_err(|e| ToolError::runtime(format!("navigate: {e}")))?; | ||
| tab.wait_until_navigated() | ||
| .map_err(|e| ToolError::runtime(format!("wait: {e}")))?; | ||
| Ok(format!("navigated to {url}")) | ||
| } | ||
| "click" => { | ||
| let url = args.get("url").and_then(|v| v.as_str()); | ||
| let selector = args | ||
| .get("selector") | ||
| .and_then(|v| v.as_str()) | ||
| .ok_or_else(|| ToolError::runtime("missing 'selector'"))?; | ||
| if let Some(u) = url { | ||
| tab.navigate_to(u) | ||
| .map_err(|e| ToolError::runtime(format!("navigate: {e}")))?; | ||
| tab.wait_until_navigated() | ||
| .map_err(|e| ToolError::runtime(format!("wait: {e}")))?; | ||
| } | ||
| let el = tab | ||
| .wait_for_element(selector) | ||
| .map_err(|e| ToolError::runtime(format!("element: {e}")))?; | ||
| el.click() | ||
| .map_err(|e| ToolError::runtime(format!("click: {e}")))?; | ||
| Ok(format!("clicked {selector}")) | ||
| } | ||
| "text" => { | ||
| let url = args | ||
| .get("url") | ||
| .and_then(|v| v.as_str()) | ||
| .ok_or_else(|| ToolError::runtime("missing 'url'"))?; | ||
| tab.navigate_to(url) | ||
| .map_err(|e| ToolError::runtime(format!("navigate: {e}")))?; | ||
| tab.wait_until_navigated() | ||
| .map_err(|e| ToolError::runtime(format!("wait: {e}")))?; | ||
| let body = tab | ||
| .wait_for_element("body") | ||
| .map_err(|e| ToolError::runtime(format!("body: {e}")))?; | ||
| let text = body | ||
| .get_inner_text() | ||
| .map_err(|e| ToolError::runtime(format!("inner text: {e}")))?; | ||
| Ok(truncate(&text, 4000)) | ||
| } | ||
| other => Err(ToolError::runtime(format!( | ||
| "unknown browser action '{other}'" | ||
| ))), | ||
| } | ||
| } |
There was a problem hiding this comment.
The run_blocking function opens a new browser tab for every tool call but never closes it. Since the browser instance is reused across the entire agent session, this will lead to a significant accumulation of open tabs, consuming excessive memory and potentially causing the browser process to crash or become unresponsive during long sessions.
fn run_blocking(
browser: &headless_chrome::Browser,
action: &str,
args: &Value,
) -> Result<String, ToolError> {
let tab = browser
.new_tab()
.map_err(|e| ToolError::runtime(format!("new_tab: {e}")))?;
let res = match action {
"navigate" => {
let url = args
.get("url")
.and_then(|v| v.as_str())
.ok_or_else(|| ToolError::runtime("missing 'url'"))?;
tab.navigate_to(url)
.map_err(|e| ToolError::runtime(format!("navigate: {e}")))?;
tab.wait_until_navigated()
.map_err(|e| ToolError::runtime(format!("wait: {e}")))?;
Ok(format!("navigated to {url}"))
}
"click" => {
let url = args.get("url").and_then(|v| v.as_str());
let selector = args
.get("selector")
.and_then(|v| v.as_str())
.ok_or_else(|| ToolError::runtime("missing 'selector'"))?;
if let Some(u) = url {
tab.navigate_to(u)
.map_err(|e| ToolError::runtime(format!("navigate: {e}")))?;
tab.wait_until_navigated()
.map_err(|e| ToolError::runtime(format!("wait: {e}")))?;
}
let el = tab
.wait_for_element(selector)
.map_err(|e| ToolError::runtime(format!("element: {e}")))?;
el.click()
.map_err(|e| ToolError::runtime(format!("click: {e}")))?;
Ok(format!("clicked {selector}"))
}
"text" => {
let url = args
.get("url")
.and_then(|v| v.as_str())
.ok_or_else(|| ToolError::runtime("missing 'url'"))?;
tab.navigate_to(url)
.map_err(|e| ToolError::runtime(format!("navigate: {e}")))?;
tab.wait_until_navigated()
.map_err(|e| ToolError::runtime(format!("wait: {e}")))?;
let body = tab
.wait_for_element("body")
.map_err(|e| ToolError::runtime(format!("body: {e}")))?;
let text = body
.get_inner_text()
.map_err(|e| ToolError::runtime(format!("inner text: {e}")))?;
Ok(truncate(&text, 4000))
}
other => Err(ToolError::runtime(format!(
"unknown browser action '{other}'"
))),
};
let _ = tab.close();
res
}| argv.push(s.to_string()); | ||
| } | ||
| // Disallow recursive agent self-invocation to avoid runaway loops. | ||
| if argv.first().map(|s| s.as_str()) == Some("agent") { |
There was a problem hiding this comment.
The recursion check only inspects the first argument in the vector. This can be easily bypassed by an LLM using global flags (e.g., gws --api-version v1 agent ...), leading to infinite recursive agent loops and local resource exhaustion. The check should identify the "agent" command regardless of preceding flags.
| if argv.first().map(|s| s.as_str()) == Some("agent") { | |
| if argv.iter().find(|s| !s.starts_with('-')).map(|s| s.as_str()) == Some("agent") { |
| fn encode(s: &str) -> String { | ||
| let mut out = String::with_capacity(s.len()); | ||
| for b in s.bytes() { | ||
| if b.is_ascii_alphanumeric() || matches!(b, b'-' | b'_' | b'.' | b'~' | b',' | b':') { |
There was a problem hiding this comment.
The encode function includes , and : in its allow-list for PostgREST query values. However, PostgREST uses these characters as delimiters for operators (like in) and complex types (like ranges). Allowing them unencoded in the value part can lead to ambiguous or incorrect query parsing. Following standard percent-encoding for values is safer.
| if b.is_ascii_alphanumeric() || matches!(b, b'-' | b'_' | b'.' | b'~' | b',' | b':') { | |
| if b.is_ascii_alphanumeric() || matches!(b, b'-' | b'_' | b'.' | b'~') { |
googleworkspace-bot
force-pushed
the
chore/sync-skills
branch
19 times, most recently
from
06b6a7c to
1fc8f04
Compare
googleworkspace-bot
force-pushed
the
chore/sync-skills
branch
4 times, most recently
from
f9b6687 to
57d1bb2
Compare
2 participants
Description
Please include a summary of the change and which issue is fixed. If adding a new feature or command, please include the output of running it with
--dry-runto prove the JSON request body matches the Discovery Document schema.Dry Run Output:
// Paste --dry-run output here if applicableChecklist:
AGENTS.mdguidelines (no generatedgoogle-*crates).cargo fmt --allto format the code perfectly.cargo clippy -- -D warningsand resolved all warnings.pnpx changeset) to document my changes.