Conversation
- Pin clawhub@0.7.0 for supply-chain safety - Add concurrency group to prevent parallel publishes - Add PR dry-run validation - Guard against missing CLAWHUB_TOKEN secret - Add changeset file
…json support - Add +pull: download project files to local directory with filename sanitization - Add +open: open script editor in browser - Add +run: execute functions with cloud-platform scope and contextual error hints (extracts GCP project number from OAuth client ID for setup guidance) - Add +logs: view execution logs via processes.listScriptProcesses - Add .clasp.json support: auto-resolve scriptId/rootDir for all helpers - Add clasp_config.rs module with path traversal validation - Add validate_script_filename() for safe file writes during +pull - Regenerate skills for new helper commands
🦋 Changeset detectedLatest commit: b5c8ef8 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 |
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 significantly expands the Google Apps Script integration within the Highlights
Changelog
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. 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 is a significant feature addition, replacing clasp functionality by adding +pull, +open, +run, and +logs helper commands for Google Apps Script. It also introduces support for reading project configuration from .clasp.json, which is a great step for compatibility. The implementation includes robust security measures, such as validation against path traversal when pulling files and handling untrusted input from configuration files.
My review focuses on a couple of areas for improvement:
- Performance: I've pointed out an inefficiency where the
.clasp.jsonconfiguration file is read and parsed multiple times within a single command execution. - Correctness: I've identified an overly strict validation rule for filenames that could prevent pulling valid Apps Script projects.
Overall, this is a well-structured and security-conscious implementation. The new helper commands and contextual error hints will greatly improve the developer experience for Apps Script users.
| if let Ok(Some(config)) = load_clasp_config() { | ||
| if let Some(ref root_dir) = config.root_dir { | ||
| return crate::validate::validate_safe_dir_path(root_dir); | ||
| } | ||
| } |
There was a problem hiding this comment.
This function calls load_clasp_config() again, which can lead to redundant file I/O and JSON parsing if it was already called by resolve_script_id() within the same command handler. This is inefficient.
Consider refactoring resolve_dir and resolve_script_id to accept an optional ClaspConfig argument. The caller (e.g., handle_push in script.rs) could then load the configuration once and pass it to both functions.
Example refactoring:
// In clasp_config.rs
pub fn resolve_dir(explicit_dir: Option<&String>, config: Option<&ClaspConfig>) -> Result<PathBuf, GwsError> {
if let Some(dir) = explicit_dir {
return crate::validate::validate_safe_dir_path(dir);
}
if let Some(config) = config {
if let Some(ref root_dir) = config.root_dir {
return crate::validate::validate_safe_dir_path(root_dir);
}
}
std::env::current_dir()
.map_err(|e| GwsError::Validation(format!("Failed to determine current directory: {e}")))
}
// In script.rs handle_push()
async fn handle_push(...) -> Result<(), GwsError> {
let config = clasp_config::load_clasp_config()?;
let script_id = clasp_config::resolve_script_id(matches.get_one::<String>("script"), config.as_ref())?;
let dir = clasp_config::resolve_dir(matches.get_one::<String>("dir"), config.as_ref())?;
// ...
}This would improve performance and make the data flow more explicit.
| if !name | ||
| .chars() | ||
| .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') | ||
| { | ||
| return Err(GwsError::Validation(format!( | ||
| "Script file name contains invalid characters (allowed: a-z, A-Z, 0-9, _, -): {name}" | ||
| ))); | ||
| } |
There was a problem hiding this comment.
The allowlist for script filenames is overly restrictive by disallowing dots (.). Apps Script file names can legitimately contain dots (e.g., my.utils.js would have the name my.utils from the API). This restriction could prevent pulling valid projects that use this common naming convention for namespacing.
The existing checks for path traversal (..), directory separators (/, \), and hidden files (names starting with .) are sufficient for security. I suggest allowing dots within the filename.
| if !name | |
| .chars() | |
| .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') | |
| { | |
| return Err(GwsError::Validation(format!( | |
| "Script file name contains invalid characters (allowed: a-z, A-Z, 0-9, _, -): {name}" | |
| ))); | |
| } | |
| if !name | |
| .chars() | |
| .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '.') | |
| { | |
| return Err(GwsError::Validation(format!( | |
| "Script file name contains invalid characters (allowed: a-z, A-Z, 0-9, _, -, .): {name}" | |
| ))); | |
| } |
jpoehnelt
added
area: skills
cla: yes
2 participants
(extracts GCP project number from OAuth client ID for setup guidance)