fix(model): use user shell for ccusage command execution#128
Conversation
Changed npx/bunx command execution to use the user's shell environment instead of minimal Go environment. This ensures proper PATH and environment variables are available for node package managers. - Added getUserShell() to detect user's shell from $SHELL with fallbacks - Added shellEscapeArgs() for safe shell argument escaping - Modified exec.CommandContext to use shell -c for command execution Fixes #127 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Le He <AnnatarHe@users.noreply.github.com>
Summary of ChangesHello @AnnatarHe, 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 resolves an issue where 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. 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
|
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![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 correctly identifies the need to execute commands within a user's shell to ensure the proper environment is loaded. However, the current implementation has significant cross-platform issues that need to be addressed. The argument escaping logic is specific to POSIX shells and will fail on Windows, introducing both functional bugs and potential security vulnerabilities. Additionally, the way commands are constructed and executed is not compatible with Windows shells like PowerShell or cmd.exe. I have provided specific feedback on how to make this implementation more robust and secure across different operating systems.
| // shellEscapeArgs joins arguments with spaces and escapes them for safe shell execution | ||
| func shellEscapeArgs(args []string) string { | ||
| escaped := make([]string, len(args)) | ||
| for i, arg := range args { | ||
| // Simple shell escaping: wrap in single quotes and escape single quotes | ||
| escaped[i] = "'" + strings.ReplaceAll(arg, "'", "'\"'\"'") + "'" | ||
| } | ||
| return strings.Join(escaped, " ") | ||
| } |
There was a problem hiding this comment.
This shell escaping implementation is specific to POSIX-compliant shells (like sh or bash) and is not compatible with Windows shells. The getUserShell function can return powershell or cmd.exe on Windows, both of which have different and complex quoting rules.
Using this POSIX-specific escaping on Windows will lead to command execution failures for arguments containing spaces or special characters. More critically, it can introduce command injection vulnerabilities.
This function must be made platform-aware. You should check runtime.GOOS and implement escaping logic appropriate for the target shell. For example, PowerShell escapes single quotes within a single-quoted string by doubling them (''). A fully robust, cross-shell implementation is non-trivial and may require refactoring to pass the detected shell type into this function.
| cmdStr := bunxPath + " " + shellEscapeArgs(args) | ||
| cmd = exec.CommandContext(ctx, shell, "-c", cmdStr) | ||
| slog.Debug("Using bunx to collect ccusage data", "shell", shell) | ||
| } else { | ||
| // Fall back to npx with --yes flag to auto-accept prompts | ||
| npxArgs := append([]string{"--yes"}, args...) | ||
| cmd = exec.CommandContext(ctx, npxPath, npxArgs...) | ||
| slog.Debug("Using npx to collect ccusage data") | ||
| cmdStr := npxPath + " " + shellEscapeArgs(npxArgs) | ||
| cmd = exec.CommandContext(ctx, shell, "-c", cmdStr) |
There was a problem hiding this comment.
The command construction and execution logic has a couple of issues that will cause it to fail on Windows and on any OS if paths contain spaces:
- Unescaped Command Path: The command path (
bunxPathornpxPath) is not escaped. If the path contains spaces (which is common on Windows, e.g.,C:\Program Files\...), the shell will misinterpret the command, leading to execution failure. - Incorrect Shell Flag: The
-cflag is hardcoded to pass the command string to the shell. This is incorrect for PowerShell, which expects-Command, and forcmd.exe, which expects/c. SincegetUserShellcan return any of these on Windows, command execution will fail.
To fix this, you should treat the executable path as the first argument to be escaped along with the other arguments. You also need to use the correct flag for the detected shell. A good approach would be to modify getUserShell to return both the shell path and the appropriate flag.
This issue applies to both the bunx and npx command construction.
1 participant
Changed npx/bunx command execution to use the user's shell environment instead of minimal Go environment. This ensures proper PATH and environment variables are available for node package managers.
Fixes #127
🤖 Generated with Claude Code