feat: add mk template for scaffolding project structures#40
Conversation
|
@greptile |
|
/gemini review |
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 significant new feature to the 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. 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 useful mk template command for scaffolding new projects. The implementation is straightforward, but there are a few areas for improvement. My main concerns are around input sanitization for the project name, which can lead to broken file content or a Cross-Site Scripting vulnerability in the node and web templates respectively. Additionally, the test coverage should be expanded to include all template types and verify all generated files to make the feature more robust. I've added specific suggestions to address these points.
utility.sh
Outdated
| node) | ||
| _u7_exec mkdir -p "$name/src" "$name/test" | ||
| echo "# $name" > "$name/README.md" | ||
| echo '{"name": "'"$name"'", "version": "0.1.0", "main": "src/index.js"}' > "$name/package.json" |
There was a problem hiding this comment.
The project name is directly embedded into the package.json string. If the name contains a double quote ("), it will result in invalid JSON. Consider escaping the name to prevent this.
| echo '{"name": "'"$name"'", "version": "0.1.0", "main": "src/index.js"}' > "$name/package.json" | |
| echo '{"name": "'"${name//\"/\\\"}"'", "version": "0.1.0", "main": "src/index.js"}' > "$name/package.json" |
utility.sh
Outdated
| ;; | ||
| web) | ||
| _u7_exec mkdir -p "$name/css" "$name/js" | ||
| echo "<!DOCTYPE html><html><head><title>$name</title><link rel=\"stylesheet\" href=\"css/style.css\"></head><body><h1>$name</h1><script src=\"js/main.js\"></script></body></html>" > "$name/index.html" |
There was a problem hiding this comment.
The project name is directly embedded into the HTML content without escaping. This creates a Cross-Site Scripting (XSS) vulnerability. If a project name contains HTML tags (e.g., <script>...), they will be rendered by the browser.
You should HTML-escape the $name variable before using it. A simple approach in bash is to replace special characters:
local name_html_escaped="$name"
name_html_escaped="${name_html_escaped//&/&}"
name_html_escaped="${name_html_escaped//</<}"
name_html_escaped="${name_html_escaped//>/>}"
# ... and then use $name_html_escaped in the echo command.
utility.sh
Outdated
| touch "$name/css/style.css" "$name/js/main.js" | ||
| echo "Created Web project: $name" | ||
| ;; | ||
| *) echo "Usage: u7 mk template <python|node|bash|web> <project-name>" ; return 1 ;; |
There was a problem hiding this comment.
The usage message here is identical to the one on line 533. This duplication can make maintenance harder. To improve clarity, consider making this error message more specific to an invalid template type, distinguishing it from the missing-argument error.
| *) echo "Usage: u7 mk template <python|node|bash|web> <project-name>" ; return 1 ;; | |
| *) echo "Error: Invalid template type '$tmpl'. Must be one of python, node, bash, web." ; return 1 ;; |
test.sh
Outdated
| # Test: mk template python | ||
| cd "$TEST_DIR" | ||
| u7 mk template python myapp >/dev/null 2>&1 | ||
| if [[ -f "myapp/src/main.py" && -f "myapp/README.md" && -f "myapp/tests/__init__.py" ]]; then |
There was a problem hiding this comment.
This test for the Python template is incomplete. The scaffolding logic creates src/__init__.py, but this check doesn't verify its existence. Adding this check will make the test more thorough and prevent future regressions.
| if [[ -f "myapp/src/main.py" && -f "myapp/README.md" && -f "myapp/tests/__init__.py" ]]; then | |
| if [[ -f "myapp/src/main.py" && -f "myapp/README.md" && -f "myapp/tests/__init__.py" && -f "myapp/src/__init__.py" ]]; then |
|
|
||
| # Test: mk template node | ||
| u7 mk template node mynode >/dev/null 2>&1 | ||
| if [[ -f "mynode/package.json" && -f "mynode/src/index.js" ]]; then |
There was a problem hiding this comment.
This test for the Node.js template is incomplete. The scaffolding logic also creates a README.md file and a test directory, but this test doesn't verify their existence. Expanding the check will make the test more robust.
| if [[ -f "mynode/package.json" && -f "mynode/src/index.js" ]]; then | |
| if [[ -f "mynode/package.json" && -f "mynode/src/index.js" && -f "mynode/README.md" && -d "mynode/test" ]]; then |
Greptile SummaryThis PR adds a Key observations:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["u7 mk template tmpl name"] --> B{tmpl or name empty?}
B -- yes --> C["echo Usage; return 1"]
B -- no --> D{name is alphanumeric?}
D -- no --> E["Error: alphanumerics only; return 1"]
D -- yes --> F{directory already exists?}
F -- yes --> G["Error: already exists; return 1"]
F -- no --> H{DRY_RUN == 1?}
H -- yes --> I["echo dry-run message; return 0"]
H -- no --> J{template type?}
J -- python --> K["mkdir src/ tests/ \n README.md, main.py, __init__.py x2"]
J -- node --> L["_u7_require jq \n mkdir src/ test/ \n package.json via jq \n index.js, README.md"]
J -- bash --> M["mkdir name/ \n README.md \n printf main.sh \n chmod +x"]
J -- web --> N["mkdir css/ js/ \n index.html \n touch style.css, main.js"]
J -- other --> O["Error: Unknown template; return 1"]
Prompt To Fix All With AIThis is a comment left during a code review.
Path: test.sh
Line: 872-879
Comment:
**Web template test missing file-level assertions**
The test verifies that `css` and `js` are directories but never checks that `css/style.css` or `js/main.js` were actually created. The template uses `touch "$name/css/style.css" "$name/js/main.js"`, so a regression that removes those `touch` calls would still let this test pass.
```suggestion
u7 mk template web myweb >/dev/null 2>&1
if [[ -f "myweb/index.html" && -f "myweb/css/style.css" && -f "myweb/js/main.js" ]]; then
echo -e "${GREEN}✓${NC} mk template web works"
((PASSED++))
else
echo -e "${RED}✗${NC} mk template web failed"
((FAILED++))
fi
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: utility.sh
Line: 536-538
Comment:
**Regex permits names starting with `-`**
The pattern `^[a-zA-Z0-9_-]+$` allows project names that begin with a hyphen (e.g., `-myapp`, `--help`). Although most path-based operations are safe once a `/` is present in the full path, some downstream tools called with a bare `$name` (e.g., tools that don't use `--`) could misinterpret it as a flag. Anchoring the first character to an alphanumeric avoids the ambiguity:
```suggestion
if [[ ! "$name" =~ ^[a-zA-Z0-9][a-zA-Z0-9_-]*$ ]]; then
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: utility.sh
Line: 573-574
Comment:
**HTML escaping is now dead code**
The `<` → `<` and `>` → `>` substitutions on these two lines can never trigger because the input validation on line 536 (`^[a-zA-Z0-9_-]+$`) already rejects any name containing `<`, `>`, or `&`. The `escaped_name` variable is identical to `$name` in every reachable code path, so the variable and both assignments can be removed:
```suggestion
echo "<!DOCTYPE html><html><head><title>$name</title><link rel=\"stylesheet\" href=\"css/style.css\"></head><body><h1>$name</h1><script src=\"js/main.js\"></script></body></html>" > "$name/index.html"
```
Removing the dead escaping also makes it immediately obvious that the name is safe to embed directly, thanks to the earlier validation guard.
How can I resolve this? If you propose a fix, please make it concise.Reviews (6): Last reviewed commit: "fix: validate project name to alphanumer..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a useful mk template command for scaffolding new projects. The implementation is a good start, but there are a couple of critical issues with how project names are handled when generating files, which can lead to corrupted or invalid files if the name contains special characters. I've left comments on the specific lines in utility.sh. Additionally, the test suite for this new feature is incomplete; it's missing tests for the bash and web templates, and the existing tests could be more thorough. Please see my comment in test.sh for details.
utility.sh
Outdated
| node) | ||
| _u7_exec mkdir -p "$name/src" "$name/test" | ||
| echo "# $name" > "$name/README.md" | ||
| echo '{"name": "'"$name"'", "version": "0.1.0", "main": "src/index.js"}' > "$name/package.json" |
There was a problem hiding this comment.
The project name is not escaped when being inserted into package.json. If the project name contains a double quote, it will result in an invalid JSON file. You should use a tool like jq to safely generate the JSON. Since jq seems to be a dependency for other parts of the script, you could use it like this: jq -n --arg name "$name" '{name: $name, version: "0.1.0", main: "src/index.js"}' > "$name/package.json"
utility.sh
Outdated
| ;; | ||
| web) | ||
| _u7_exec mkdir -p "$name/css" "$name/js" | ||
| echo "<!DOCTYPE html><html><head><title>$name</title><link rel=\"stylesheet\" href=\"css/style.css\"></head><body><h1>$name</h1><script src=\"js/main.js\"></script></body></html>" > "$name/index.html" |
There was a problem hiding this comment.
The project name is not escaped when being inserted into index.html. If the project name contains HTML special characters like < or >, it will result in an invalid or malformed HTML file. The project name should be HTML-escaped before being embedded in the title and body. You can use sed for basic escaping, for example: escaped_name=$(printf '%s' "$name" | sed 's/&/\&/g; s/</\</g; s/>/\>/g;')
| # Test: mk template python | ||
| cd "$TEST_DIR" | ||
| u7 mk template python myapp >/dev/null 2>&1 | ||
| if [[ -f "myapp/src/main.py" && -f "myapp/README.md" && -f "myapp/tests/__init__.py" ]]; then | ||
| echo -e "${GREEN}✓${NC} mk template python works" | ||
| ((PASSED++)) | ||
| else | ||
| echo -e "${RED}✗${NC} mk template python failed" | ||
| ((FAILED++)) | ||
| fi | ||
|
|
||
| # Test: mk template node | ||
| u7 mk template node mynode >/dev/null 2>&1 | ||
| if [[ -f "mynode/package.json" && -f "mynode/src/index.js" ]]; then | ||
| echo -e "${GREEN}✓${NC} mk template node works" | ||
| ((PASSED++)) | ||
| else | ||
| echo -e "${RED}✗${NC} mk template node failed" | ||
| ((FAILED++)) | ||
| fi | ||
|
|
||
| # Test: mk template requires args | ||
| result=$(u7 mk template 2>&1) | ||
| assert_contains "mk template requires args" "Usage:" "$result" |
There was a problem hiding this comment.
The test coverage for the new mk template feature is incomplete and could be improved:
- Tests for the
bashandwebtemplates are missing. - The existing tests for
pythonandnodeonly check for file existence. They should also verify file contents to ensure the templates are generated correctly (e.g., check for valid JSON inpackage.json, check for shebangs in scripts). - There are no tests for project names containing spaces or special characters, which would have helped catch the injection bugs in
utility.sh.
|
@greptile |
|
@greptile |
| local escaped_name="${name//</<}" | ||
| escaped_name="${escaped_name//>/>}" | ||
| echo "<!DOCTYPE html><html><head><title>$escaped_name</title><link rel=\"stylesheet\" href=\"css/style.css\"></head><body><h1>$escaped_name</h1><script src=\"js/main.js\"></script></body></html>" > "$name/index.html" |
There was a problem hiding this comment.
& not escaped in HTML template (incomplete fix)
The previous fix added < → < and > → > escaping, but & → & is still missing. This means a project name like foo & bar will produce:
<title>foo & bar</title>
<h1>foo & bar</h1>A bare & not followed by a valid entity reference is technically invalid HTML and can cause browsers to issue parse warnings. Additionally, the & substitution must happen first — if it were added after the current </> substitutions, the < and > sequences already written would get double-encoded into &lt; / &gt;.
| local escaped_name="${name//</<}" | |
| escaped_name="${escaped_name//>/>}" | |
| echo "<!DOCTYPE html><html><head><title>$escaped_name</title><link rel=\"stylesheet\" href=\"css/style.css\"></head><body><h1>$escaped_name</h1><script src=\"js/main.js\"></script></body></html>" > "$name/index.html" | |
| local escaped_name="${name//&/&}" | |
| escaped_name="${escaped_name//</<}" | |
| escaped_name="${escaped_name//>/>}" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: utility.sh
Line: 569-571
Comment:
**`&` not escaped in HTML template (incomplete fix)**
The previous fix added `<` → `<` and `>` → `>` escaping, but `&` → `&` is still missing. This means a project name like `foo & bar` will produce:
```html
<title>foo & bar</title>
<h1>foo & bar</h1>
```
A bare `&` not followed by a valid entity reference is technically invalid HTML and can cause browsers to issue parse warnings. Additionally, the `&` substitution **must happen first** — if it were added after the current `<`/`>` substitutions, the `<` and `>` sequences already written would get double-encoded into `&lt;` / `&gt;`.
```suggestion
local escaped_name="${name//&/&}"
escaped_name="${escaped_name//</<}"
escaped_name="${escaped_name//>/>}"
```
How can I resolve this? If you propose a fix, please make it concise.
|
@greptile |
|
@greptile |
… add src/__init__.py test
|
@greptile |
| if [[ ! "$name" =~ ^[a-zA-Z0-9_-]+$ ]]; then | ||
| echo "Error: project name must contain only alphanumerics, hyphens, and underscores" | ||
| return 1 |
There was a problem hiding this comment.
-
The pattern ^[a-zA-Z0-9_-]+$ allows project names that begin with a hyphen (e.g., -myapp, --help). Although most path-based operations are safe once a / is present in the full path, some downstream tools called with a bare $name (e.g., tools that don't use --) could misinterpret it as a flag. Anchoring the first character to an alphanumeric avoids the ambiguity:
| if [[ ! "$name" =~ ^[a-zA-Z0-9_-]+$ ]]; then | |
| echo "Error: project name must contain only alphanumerics, hyphens, and underscores" | |
| return 1 | |
| if [[ ! "$name" =~ ^[a-zA-Z0-9][a-zA-Z0-9_-]*$ ]]; then |
Prompt To Fix With AI
This is a comment left during a code review.
Path: utility.sh
Line: 536-538
Comment:
**Regex permits names starting with `-`**
The pattern `^[a-zA-Z0-9_-]+$` allows project names that begin with a hyphen (e.g., `-myapp`, `--help`). Although most path-based operations are safe once a `/` is present in the full path, some downstream tools called with a bare `$name` (e.g., tools that don't use `--`) could misinterpret it as a flag. Anchoring the first character to an alphanumeric avoids the ambiguity:
```suggestion
if [[ ! "$name" =~ ^[a-zA-Z0-9][a-zA-Z0-9_-]*$ ]]; then
```
How can I resolve this? If you propose a fix, please make it concise.| local escaped_name="${name//</<}" | ||
| escaped_name="${escaped_name//>/>}" |
There was a problem hiding this comment.
The < → < and > → > substitutions on these two lines can never trigger because the input validation on line 536 (^[a-zA-Z0-9_-]+$) already rejects any name containing <, >, or &. The escaped_name variable is identical to $name in every reachable code path, so the variable and both assignments can be removed:
| local escaped_name="${name//</<}" | |
| escaped_name="${escaped_name//>/>}" | |
| echo "<!DOCTYPE html><html><head><title>$name</title><link rel=\"stylesheet\" href=\"css/style.css\"></head><body><h1>$name</h1><script src=\"js/main.js\"></script></body></html>" > "$name/index.html" |
Removing the dead escaping also makes it immediately obvious that the name is safe to embed directly, thanks to the earlier validation guard.
Prompt To Fix With AI
This is a comment left during a code review.
Path: utility.sh
Line: 573-574
Comment:
**HTML escaping is now dead code**
The `<` → `<` and `>` → `>` substitutions on these two lines can never trigger because the input validation on line 536 (`^[a-zA-Z0-9_-]+$`) already rejects any name containing `<`, `>`, or `&`. The `escaped_name` variable is identical to `$name` in every reachable code path, so the variable and both assignments can be removed:
```suggestion
echo "<!DOCTYPE html><html><head><title>$name</title><link rel=\"stylesheet\" href=\"css/style.css\"></head><body><h1>$name</h1><script src=\"js/main.js\"></script></body></html>" > "$name/index.html"
```
Removing the dead escaping also makes it immediately obvious that the name is safe to embed directly, thanks to the earlier validation guard.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Summary
mk template python <name>— creates src/, tests/, main.py, READMEmk template node <name>— creates src/, test/, package.json, index.jsmk template bash <name>— creates main.sh with shebang and set flagsmk template web <name>— creates index.html, css/, js/Test plan
u7 mk template python myappcreates correct structureu7 mk template node mynodecreates package.jsonu7 mk templatewithout args shows usage