Conversation
…p and docs restructure
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 represents a significant architectural shift for the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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 represents a significant and impressive migration from a collection of shell scripts to a modern, modular Node.js CLI. The new architecture, built with @clack/prompts and vitest, greatly improves the user experience, maintainability, and testability of the project. The introduction of distinct commands (setup, append, verify, clean) and a layered Zsh configuration is a very well-thought-out design. My review includes a few minor suggestions for improving documentation clarity, robustness, and security.
Note: Security Review did not run due to the size of the PR.
|
@ChangeHow I've opened a new pull request, #11, to work on those changes. Once the pull request is ready, I'll request review from you. |
The `local/secrets.zsh` reference in `AGENTS.md` was ambiguous — it omitted the full installed path, inconsistent with how the zshrc templates source it and the layout described in `README.md`. ## Changes - **`AGENTS.md`**: Updated `local/secrets.zsh` → `~/.config/zsh/local/secrets.zsh` in the Local files section <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/ChangeHow/suitup/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ChangeHow <23733347+ChangeHow@users.noreply.github.com>
|
@ChangeHow I've opened a new pull request, #12, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@ChangeHow I've opened a new pull request, #13, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@copilot add a github action to current PR, it would run all the test cases in every PR and commit. |
|
@ChangeHow I've opened a new pull request, #14, to work on those changes. Once the pull request is ready, I'll request review from you. |
- [x] Investigate CI failure logs - [x] Fix: add `apt-get install zsh` step — tests use `zsh -n` for syntax checking but zsh is not pre-installed on ubuntu-latest <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/ChangeHow/suitup/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ChangeHow <23733347+ChangeHow@users.noreply.github.com>
|
@ChangeHow I've opened a new pull request, #15, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
/gemini review |
|
/gemini summary |
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the suitup tool from a collection of shell scripts to a modern, modular Node.js CLI, which is a significant improvement with clear separation of concerns into different commands (setup, append, verify, clean) and logical steps. While the new architecture, comprehensive Vitest test suite, and updated documentation are commendable, a critical command injection vulnerability was identified in the SSH key generation step. User-supplied input (email) is directly interpolated into a shell command string, which could allow for arbitrary code execution. This must be addressed by using environment variables or avoiding shell execution for this command. Additionally, please review the minor issues detailed in the specific comments.
| await runStream( | ||
| `ssh-keygen -t rsa -b 4096 -C "${email}" -f "${keyFile}" -N "$SSH_KEYGEN_PASSPHRASE"`, | ||
| { env: { ...process.env, SSH_KEYGEN_PASSPHRASE: passphrase } } | ||
| ); |
There was a problem hiding this comment.
The email variable, obtained from user input via p.text, is directly interpolated into a shell command string without sanitization. This command is then executed using runStream, which calls spawn("bash", ["-c", cmd]). An attacker can provide an input containing shell metacharacters (e.g., ;, $(...), or backticks) to execute arbitrary commands with the privileges of the user running the script.
To remediate this, pass the email address via an environment variable, similar to how the passphrase is handled. This prevents the shell from interpreting the content of the email as part of the command structure.
| await runStream( | |
| `ssh-keygen -t rsa -b 4096 -C "${email}" -f "${keyFile}" -N "$SSH_KEYGEN_PASSPHRASE"`, | |
| { env: { ...process.env, SSH_KEYGEN_PASSPHRASE: passphrase } } | |
| ); | |
| await runStream( | |
| `ssh-keygen -t rsa -b 4096 -C "$SSH_KEYGEN_EMAIL" -f "${keyFile}" -N "$SSH_KEYGEN_PASSPHRASE"`, | |
| { env: { ...process.env, SSH_KEYGEN_EMAIL: email, SSH_KEYGEN_PASSPHRASE: passphrase } } | |
| ); |
There was a problem hiding this comment.
@copilot valuable suggestion, solve it and add some test cases.
| export function projectRoot() { | ||
| return join(import.meta.dirname, "..", ".."); | ||
| } |
There was a problem hiding this comment.
The projectRoot function uses import.meta.dirname, which is not a standard property in Node.js ES modules and will be undefined at runtime, causing the function to fail if called. To correctly get the directory path, you should use dirname(fileURLToPath(import.meta.url)), as you've done in src/constants.js. Although this function appears to be unused currently, it's best to fix it to prevent future bugs.
You'll also need to add import { fileURLToPath } from "node:url"; at the top of the file.
| export function projectRoot() { | |
| return join(import.meta.dirname, "..", ".."); | |
| } | |
| export function projectRoot() { | |
| return join(dirname(fileURLToPath(import.meta.url)), "..", ".."); | |
| } |
There was a problem hiding this comment.
@copilot add a node version checker before running script and make sure user running node withnative ESM support.
Summary of ChangesThis pull request represents a significant architectural overhaul of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Activity
|
The `fzf-config` append block extracted FZF env vars from `tools.zsh` by
splitting on a comment string (`# Tool initialization`), silently
breaking if that comment ever changed. Additionally, the Powerlevel10k
theme was being loaded in the suitup stage instead of as the final step.
## Changes
- **`configs/shared/fzf.zsh`** — new standalone file holding only
`FZF_DEFAULT_COMMAND`, `FZF_CTRL_T_COMMAND`, and `FZF_CTRL_T_OPTS`
- **`configs/shared/tools.zsh`** — FZF vars removed; replaced with an
inline conditional source of `fzf.zsh` (avoids `source_if_exists`
dependency):
```zsh
[[ -f "${ZDOTDIR:-$HOME/.config/zsh}/shared/fzf.zsh" ]] && source
"${ZDOTDIR:-$HOME/.config/zsh}/shared/fzf.zsh"
```
- **`src/append.js`** — `fzf-config` block reads `fzf.zsh` directly via
`readFileSync` instead of parsing `tools.zsh`
- **`src/steps/zsh-config.js`** — `fzf.zsh` added to `sharedFiles` so
it's copied to `~/.config/zsh/shared/` during setup
- **`configs/zinit-plugins`** — removed `zinit light
romkatv/powerlevel10k` so p10k is no longer loaded in the suitup stage
(step 7)
- **`configs/shared/prompt.zsh`** — now loads the p10k theme as the very
last step (prompt stage, step 9), guarded with `(( ${+functions[zinit]}
))` so it is a no-op for OMZ setups where p10k is handled by OMZ itself
- **Tests** — 6 new cases in `append.test.js` assert `fzf.zsh` exists
standalone, contains only FZF vars (no cache helpers), and that the
appended block is idempotent; `configs.test.js` updated to assert p10k
is in `prompt.zsh` (not `zinit-plugins`) and `setup.test.js` asserts
`prompt.zsh` is sourced after `zinit-plugins` in the template loading
order
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/ChangeHow/suitup/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: ChangeHow <23733347+ChangeHow@users.noreply.github.com>
|
@ChangeHow I've opened a new pull request, #16, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@ChangeHow I've opened a new pull request, #17, to work on those changes. Once the pull request is ready, I'll request review from you. |
`cli.js` uses top-level `await` and native ESM, requiring Node ≥ 18.
There was no runtime guard against older versions, and `src/utils/fs.js`
had a silent bug (`import.meta.dirname` is non-standard and returns
`undefined`).
## Changes
- **`src/utils/node-version.js`** — new utility; checks
`process.version` against the Node 18 minimum and exits with a clear
upgrade message if not met
- **`src/cli.js`** — calls `checkNodeVersion()` before any setup logic
- **`package.json`** — adds `"engines": { "node": ">=18.0.0" }` for
tooling-level enforcement
- **`src/utils/fs.js`** — fixes `projectRoot()` to use
`dirname(fileURLToPath(import.meta.url))` instead of
`import.meta.dirname`
```js
// src/utils/node-version.js
export function checkNodeVersion() {
const major = parseInt(process.version.replace(/^v/, "").split(".")[0], 10);
if (major < 18) {
console.error(
`suitup requires Node.js 18 or later for native ESM support.\n` +
`You are running Node.js ${process.version}.\n` +
`Please upgrade: https://nodejs.org/en/download`
);
process.exit(1);
}
}
```
<!-- START COPILOT CODING AGENT TIPS -->
---
💬 We'd love your input! Share your thoughts on Copilot coding agent in
our [2 minute survey](https://gh.io/copilot-coding-agent-survey).
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: ChangeHow <23733347+ChangeHow@users.noreply.github.com>
Summary
setup,append,verify,clean) and step-based architecture.configs/plus Vitest coverage for setup flows, config integrity, and shell syntax.AGENTS.md.