fix: add shell: true for Windows .cmd execution (ACS-365) #1340
Conversation
Summary of ChangesHello @StillKnotKnown, 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 addresses a specific problem encountered during the Windows packaging process, 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
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdded Windows-only shell metacharacter detection and exported validation (SHELL_METACHARACTERS, validateArgs); integrated validateArgs into runCommand with shell enabled on Windows; added a .d.ts declaration and comprehensive unit tests; guarded main invocation and exported helpers for testing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
StillKnotKnown
added
bug
![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 addresses a packaging failure on Windows by enabling the shell option for spawnSync when executing .cmd files. The fix is well-reasoned and follows existing patterns in the codebase. I've added one suggestion for a minor refactoring to further simplify the code, taking advantage of the new shell execution context.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| stdio: 'inherit', | ||
| shell: isWindows(), | ||
| }); | ||
|
|
||
| if (result.error) { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
StillKnotKnown
force-pushed
the
stillknotknown/acs-365-windows-packaging-fails-with-spawnsync-electron-vite-cmd-einval
branch
from
bdb5831 to
222d08d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/frontend/scripts/package-with-python.cjs`:
- Around line 16-42: Add unit tests for the security gate around validateArgs:
write tests that mock isWindows() to true to exercise the Windows branch and
verify that validateArgs throws for inputs containing metacharacters from
SHELL_METACHARACTERS (test at least a representative subset and
one-per-character or a sample like '&', '|', '>', '<') and that it does not
throw for safe inputs (e.g., alphanumeric, paths, flags). Also add a test with
isWindows() mocked to false to ensure validateArgs returns/does nothing on
non-Windows. Reference validateArgs, SHELL_METACHARACTERS and isWindows when
locating the code to test and assert thrown Error contains the offending
character and argument. Ensure tests are lightweight, deterministic, and
included in the project test suite.
| /** | ||
| * Shell metacharacters that could enable command injection when shell: true is used on Windows. | ||
| * These characters have special meaning in cmd.exe and could be used to inject arbitrary commands. | ||
| */ | ||
| const SHELL_METACHARACTERS = ['&', '|', '>', '<', '^', '%', ';', '$', '`', '\n', '\r']; | ||
|
|
||
| /** | ||
| * Validate that arguments don't contain shell metacharacters on Windows. | ||
| * When shell: true is used, cmd.exe interprets metacharacters which could lead to command injection. | ||
| * | ||
| * @param {string[]} commandArgs - Arguments to validate | ||
| * @throws {Error} If any argument contains dangerous shell metacharacters on Windows | ||
| */ | ||
| function validateArgs(commandArgs) { | ||
| if (!isWindows()) return; // Only validate on Windows where shell: true is used | ||
|
|
||
| for (const arg of commandArgs) { | ||
| for (const char of SHELL_METACHARACTERS) { | ||
| if (arg.includes(char)) { | ||
| throw new Error( | ||
| `Security: Argument contains shell metacharacter '${char}' which could enable command injection. ` + | ||
| `Argument: "${arg}"` | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add targeted tests for validateArgs to lock down the security gate.
This new filter is security‑sensitive; lightweight unit tests covering allowed args and a few blocked metacharacters will prevent regressions.
🤖 Prompt for AI Agents
In `@apps/frontend/scripts/package-with-python.cjs` around lines 16 - 42, Add unit
tests for the security gate around validateArgs: write tests that mock
isWindows() to true to exercise the Windows branch and verify that validateArgs
throws for inputs containing metacharacters from SHELL_METACHARACTERS (test at
least a representative subset and one-per-character or a sample like '&', '|',
'>', '<') and that it does not throw for safe inputs (e.g., alphanumeric, paths,
flags). Also add a test with isWindows() mocked to false to ensure validateArgs
returns/does nothing on non-Windows. Reference validateArgs,
SHELL_METACHARACTERS and isWindows when locating the code to test and assert
thrown Error contains the offending character and argument. Ensure tests are
lightweight, deterministic, and included in the project test suite.
222d08d to
2c4c5bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/frontend/scripts/package-with-python.cjs`:
- Around line 16-21: Update the SHELL_METACHARACTERS constant to include
additional cmd.exe metacharacters '(' , ')' and '!' so the array in
apps/frontend/scripts/package-with-python.cjs more fully covers Windows command
injection vectors; locate the SHELL_METACHARACTERS declaration and add these
three characters to the list alongside the existing entries.
In `@apps/frontend/src/main/__tests__/package-with-python.test.ts`:
- Around line 9-11: The `@ts-expect-error` comment above the import of
validateArgs and SHELL_METACHARACTERS is outdated because
package-with-python.d.ts now provides types; either remove the `@ts-expect-error`
line if TypeScript no longer raises an error for that import, or replace its
message with an accurate explanation (e.g., "bundler module resolution edge
case" or similar) if the suppression is still required—inspect the import of
validateArgs and SHELL_METACHARACTERS and update the comment accordingly.
♻️ Duplicate comments (1)
apps/frontend/scripts/package-with-python.cjs (1)
122-142: The command injection concern has been addressed.The
validateArgscall beforespawnSyncmitigates the shell metacharacter injection risk flagged in the previous review. The error handling forresult.errorand non-zero exit status is a good addition.
StillKnotKnown
force-pushed
the
stillknotknown/acs-365-windows-packaging-fails-with-spawnsync-electron-vite-cmd-einval
branch
from
2c4c5bd to
51b55ed
Compare
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
🟠 Needs revision - 1 issue(s) require attention.
1 issue(s) must be addressed (0 required, 1 recommended), 1 suggestions
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | Low | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- Medium: 1 issue(s)
- Low: 1 issue(s)
Generated by Auto Claude PR Review
Findings (2 selected of 2 total)
🟡 [9125993e3330] [MEDIUM] Incomplete shell metacharacter list missing parentheses, quotes, and exclamation mark
📁 apps/frontend/scripts/package-with-python.cjs:20
The SHELL_METACHARACTERS list is missing characters that have special meaning in Windows cmd.exe: parentheses () for command grouping, double quotes " for argument boundary manipulation, square/curly brackets []{}, and ! for delayed expansion. The codebase has a more complete pattern at apps/frontend/src/main/platform/index.ts:294 using /[;&|\${}[]<>!"^]/. While the current list matches mcp-handlers.ts`, it should use the more comprehensive security pattern for defense-in-depth.
Suggested fix:
Update to include all dangerous characters: const SHELL_METACHARACTERS = ['&', '|', '>', '<', '^', '%', ';', '$', '`', '\n', '\r', '(', ')', '[', ']', '{', '}', '"', '!'];
🔵 [e8e72b0b3e37] [LOW] validateArgs may crash on non-string arguments
📁 apps/frontend/scripts/package-with-python.cjs:32
The validateArgs function calls arg.includes() without verifying arg is a string. If null or undefined is passed in the array, TypeError will be thrown. While current usage with process.argv.slice(2) always produces strings, defensive coding should handle this edge case.
Suggested fix:
Add type check at start of loop: `if (typeof arg !== 'string') continue;` or throw a descriptive error for invalid argument types.
This review was generated by Auto Claude.
StillKnotKnown
force-pushed
the
stillknotknown/acs-365-windows-packaging-fails-with-spawnsync-electron-vite-cmd-einval
branch
from
51b55ed to
5f54fd7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/__tests__/package-with-python.test.ts`:
- Around line 138-140: The test "should allow paths with backslashes" hardcodes
a Windows system path; update the test to construct a Windows-style path instead
of using "C:\Program Files\…". Replace the literal string with a constructed
path using Node's path utilities (e.g., path.win32.join or building from
components) so the test still validates backslashes but avoids hardcoded system
locations; update the expectation around validateArgs(...) accordingly in the
test case named 'should allow paths with backslashes'.
5f54fd7 to
56e16d5
Compare
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 4 new issue(s) found in recent changes.
Resolution Status
- ✅ Resolved: 2 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 4 new findings in recent changes
Verdict
CI Status: ⏳ 1 check pending - Cannot approve merge until CI completes.
Resolution Status:
- ✅ Finding 9125993e3330 (medium - incomplete metacharacter list): RESOLVED - All missing characters (parentheses, brackets, braces, exclamation mark, double quotes) now included in SHELL_METACHARACTERS array
- ✅ Finding e8e72b0b3e37 (low - non-string crash): RESOLVED - Defensive type check added that throws TypeError for non-string arguments before calling includes()
New Findings:
- 4 LOW severity suggestions (quality improvements, documentation polish) - none blocking
Verdict Reasoning:
Once CI passes, this PR will be READY_TO_MERGE. All previous findings have been properly addressed with comprehensive fixes including unit tests. The new LOW severity findings are optional polish items that can be addressed post-merge. Excellent work addressing the security concerns with thorough implementation and test coverage.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (4 selected of 4 total)
🔵 [NEW-001] [LOW] Consider adding single quote to SHELL_METACHARACTERS
📁 apps/frontend/scripts/package-with-python.cjs:30
The list includes double quote but omits single quote. While cmd.exe doesn't treat single quotes as shell metacharacters for command injection, some programs interpret them specially. This is a minor consistency consideration, not a security gap.
Suggested fix:
Consider adding single quote if desired for completeness, but not required for cmd.exe security.
🔵 [NEW-002] [LOW] Test type assertions for non-string tests could be more explicit
📁 apps/frontend/src/main/__tests__/package-with-python.test.ts:96
Tests for non-string arguments (null, undefined, number, object) pass values that TypeScript would normally reject. Consider adding @ts-expect-error comments or explicit type assertions for clarity.
Suggested fix:
Add // @ts-expect-error - Testing runtime validation of invalid input before each non-string test
🔵 [NEW-003] [LOW] Consider documenting Windows-only validation rationale in runCommand
📁 apps/frontend/scripts/package-with-python.cjs:142
The validateArgs check only runs on Windows because shell: true is Windows-only. While correct, the asymmetry could benefit from an inline comment explaining the relationship.
Suggested fix:
Add comment: // Note: validateArgs only validates on Windows because shell: true is only used on Windows
🔵 [NEW-004] [LOW] Type declaration uses readonly but implementation is mutable
📁 apps/frontend/scripts/package-with-python.d.ts:4
The .d.ts declares SHELL_METACHARACTERS as 'readonly string[]' but the .cjs implementation is a regular mutable array. Minor type/implementation mismatch.
Suggested fix:
Either use Object.freeze() in implementation or change type to string[]
This review was generated by Auto Claude.
On Windows, spawnSync cannot execute .cmd files directly without a shell context. This adds shell: isWindows() to the spawnSync options in runCommand() to properly execute electron-vite.cmd and electron-builder.cmd during packaging. Additionally, adds argument validation to prevent potential command injection via shell metacharacters when shell: true is used on Windows. When using shell: true, cmd.exe interprets certain characters (& | > < ^ % ; $ $`) as special operators, which could enable command injection if present in user- controlled arguments. The validateArgs() function checks for these metacharacters on Windows and throws an error if any are found, following the same security pattern used in apps/frontend/src/main/ipc-handlers/mcp-handlers.ts. This follows the existing pattern used throughout the codebase for Windows .cmd file execution (env-utils.ts, mcp-handlers.ts). Fixes ACS-365
StillKnotKnown
force-pushed
the
stillknotknown/acs-365-windows-packaging-fails-with-spawnsync-electron-vite-cmd-einval
branch
from
56e16d5 to
9a31d0f
Compare
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
✅ Auto Claude Review - APPROVED
Status: Ready to Merge
Summary: ## ✅ Follow-up Review: Ready To Merge
✅ Ready to merge - All checks passing and findings addressed.
Resolution Status
- ✅ Resolved: 4 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 0 new findings in recent changes
Verdict
All 4 previous LOW severity findings have been resolved: (1) NEW-001 - single quote exclusion now documented with rationale, (2) NEW-002 - @ts-expect-error added for TypeScript import clarity, (3) NEW-003 - Windows-only validation rationale documented inline, (4) NEW-004 - Object.freeze() now aligns implementation with readonly type declaration. CI Status: ✅ All 19 checks passing. No new commits since last review means no new issues to evaluate. No contributor comments requiring response. The PR is ready to merge.
Review Process
Agents invoked: resolution-verifier
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
This automated review found no blocking issues. The PR can be safely merged.
Generated by Auto Claude
StillKnotKnown
deleted the
stillknotknown/acs-365-windows-packaging-fails-with-spawnsync-electron-vite-cmd-einval
branch
…ndyMik90#1340) On Windows, spawnSync cannot execute .cmd files directly without a shell context. This adds shell: isWindows() to the spawnSync options in runCommand() to properly execute electron-vite.cmd and electron-builder.cmd during packaging. Additionally, adds argument validation to prevent potential command injection via shell metacharacters when shell: true is used on Windows. When using shell: true, cmd.exe interprets certain characters (& | > < ^ % ; $ $`) as special operators, which could enable command injection if present in user- controlled arguments. The validateArgs() function checks for these metacharacters on Windows and throws an error if any are found, following the same security pattern used in apps/frontend/src/main/ipc-handlers/mcp-handlers.ts. This follows the existing pattern used throughout the codebase for Windows .cmd file execution (env-utils.ts, mcp-handlers.ts). Fixes ACS-365 Co-authored-by: StillKnotKnown <[email protected]>
2 participants
Summary
Fixes Windows packaging failure by adding
shell: truewhen executing.cmdfiles in the package script.Problem
On Windows,
npm run package:winfails withError: spawnSync electron-vite.cmd EINVALbecause Node.js'sspawnSynccannot execute.cmdfiles directly without a shell context.Solution
Adds
shell: isWindows()to thespawnSyncoptions inrunCommand()function. This follows the existing pattern used throughout the codebase for Windows.cmdfile execution (seeenv-utils.ts,mcp-handlers.ts).Type of Change
Area
Checklist
Refs: ACS-365
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.