Fix all code#730
Fix all code#730samhiotisiddn-jpg wants to merge 4 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.
🦋 Changeset detectedLatest commit: 526433e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a terminal AI agent (gws agent) to the Google Workspace CLI, featuring a modular tool system, conversational memory backends, and support for multiple LLM providers. The implementation includes integrations for Notion, Zapier, Supabase, and a headless browser. Review feedback identifies several critical issues that need to be addressed: a resource leak in the browser tool where tabs are not closed on error, a flawed recursion check in the GWS tool, and incorrect argument extraction when global flags are used. Furthermore, security vulnerabilities were identified in the URL encoding logic across the Notion, Supabase, and memory modules, where unencoded dots could lead to path traversal or query injection attacks.
|
|
||
| // Handle the `agent` command (terminal AI agent with tool use) | ||
| if first_arg == "agent" { | ||
| let agent_args: Vec<String> = args.iter().skip(2).cloned().collect(); |
There was a problem hiding this comment.
The extraction of agent_args by skipping a fixed number of arguments (skip(2)) is incorrect when global flags (like --api-version) are provided before the agent command. This results in global flags and the agent command itself being passed as arguments to the agent handler, which misinterprets them as part of the prompt. A more robust approach is to find the position of the agent command in the argument vector.
| let agent_args: Vec<String> = args.iter().skip(2).cloned().collect(); | |
| let agent_idx = args.iter().position(|a| a == "agent").unwrap_or(1); | |
| let agent_args: Vec<String> = args.iter().skip(agent_idx + 1).cloned().collect(); |
| // Disallow recursive agent self-invocation to avoid runaway loops. | ||
| // Check all arguments, not just the first, since the LLM can use global | ||
| // flags before the command (e.g., `gws --api-version v1 agent ...`). | ||
| if argv.iter().find(|s| !s.starts_with('-')).map(|s| s.as_str()) == Some("agent") { |
There was a problem hiding this comment.
The recursion check fails to detect the agent command if global flags with values are used before it (e.g., gws --api-version v1 agent). The find logic returns the first non-flag argument, which in this case would be the flag's value (v1), causing the check to miss the agent command and potentially allowing infinite recursive loops. Checking if any argument matches "agent" is a safer approach for this sandbox restriction.
| if argv.iter().find(|s| !s.starts_with('-')).map(|s| s.as_str()) == Some("agent") { | |
| if argv.iter().any(|s| s == "agent") { |
| fn percent_encode_path(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'~') { |
There was a problem hiding this comment.
Allowing the dot character (.) in percent_encode_path introduces a path traversal vulnerability. Since this function is used to construct API paths (e.g., /pages/{page_id}), an attacker or a misbehaving model could provide .. as a page_id to access unintended API endpoints. The dot character should be percent-encoded for safety in path segments.
| if b.is_ascii_alphanumeric() || matches!(b, b'-' | b'_' | b'.' | b'~') { | |
| if b.is_ascii_alphanumeric() || matches!(b, b'-' | b'_' | b'~') { |
| 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'~') { |
There was a problem hiding this comment.
The encode function should not allow the dot character (.) as it is a reserved delimiter in PostgREST filter syntax (column=op.value). Allowing it unencoded in values can lead to query injection or misinterpretation of filters by the backend. Reserved characters in PostgREST values must be percent-encoded.
| if b.is_ascii_alphanumeric() || matches!(b, b'-' | b'_' | b'.' | b'~') { | |
| if b.is_ascii_alphanumeric() || matches!(b, b'-' | b'_' | b'~') { |
| 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}")))?; | ||
| let _ = tab.close(); | ||
| 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}")))?; | ||
| let _ = tab.close(); | ||
| 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}")))?; | ||
| let _ = tab.close(); | ||
| Ok(truncate(&text, 4000)) | ||
| } | ||
| other => { | ||
| let _ = tab.close(); | ||
| Err(ToolError::runtime(format!( | ||
| "unknown browser action '{other}'" | ||
| ))) | ||
| } | ||
| } |
There was a problem hiding this comment.
The browser tool leaks tabs when an error occurs during an operation. The tab.close() call is only reached at the end of successful execution paths. If any operation (like navigate_to or wait_for_element) returns an error via the ? operator, the tab handle is dropped without being closed in the browser process, leading to memory accumulation over time. The tab should be closed regardless of the operation's outcome.
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| fn urlencoding(input: &str) -> String { | ||
| let mut out = String::with_capacity(input.len()); | ||
| for b in input.bytes() { | ||
| if b.is_ascii_alphanumeric() || matches!(b, b'-' | b'_' | b'.' | b'~') { |
There was a problem hiding this comment.
The urlencoding function allows the dot character (.), which is a reserved delimiter in PostgREST filter syntax. Since this function is used to encode conversation_id for Supabase queries, it should percent-encode dots to prevent potential query misinterpretation or injection if a custom ID contains dots.
| if b.is_ascii_alphanumeric() || matches!(b, b'-' | b'_' | b'.' | b'~') { | |
| if b.is_ascii_alphanumeric() || matches!(b, b'-' | b'_' | b'~') { |
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.