Conversation
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
📝 WalkthroughWalkthroughMultiple changes add a ShellKind abstraction and update command invocation handling, adjust executor script-extension logic, introduce PowerShell-focused tests, and add a RISC-V release target to the CI release workflow. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/shell_kind.rs`:
- Around line 27-35: The From<&str> for ShellKind currently does case-sensitive
matching causing mixed-case shell names (e.g., "Cmd.exe", "PWSH.EXE") to map to
Other; update the impl for From<&str> (the From conversion for ShellKind) to
perform case-insensitive comparisons—either normalize the input string
(to_lowercase) before the match or use eq_ignore_ascii_case in each arm for
"cmd"/"cmd.exe", "powershell"/"powershell.exe"/"pwsh"/"pwsh.exe"—so
Windows-configured shell.command values are correctly classified as
ShellKind::Cmd or ShellKind::Powershell.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9f00a88-19de-476a-8c6f-e97cc49709cd
📒 Files selected for processing (7)
src/evaluator.rssrc/executor.rssrc/function.rssrc/lib.rssrc/shell_kind.rstests/backticks.rstests/functions.rs
| impl From<&str> for ShellKind { | ||
| fn from(command: &str) -> Self { | ||
| match command { | ||
| "cmd" | "cmd.exe" => Self::Cmd, | ||
| "powershell" | "powershell.exe" | "pwsh" | "pwsh.exe" => Self::Powershell, | ||
| _ => Self::Other, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Case-sensitive matching may cause incorrect shell classification on Windows.
The From<&str> implementation only matches exact lowercase names (cmd, powershell, pwsh, etc.). On Windows, users may specify shell commands with different casing (e.g., Cmd.exe, PWSH.EXE, PowerShell.exe). Per context in src/settings.rs:55-59, shell.command is used directly without normalization, so mixed-case configurations will fall through to Other, resulting in incorrect script extensions (no extension instead of .bat/.ps1).
Consider using case-insensitive matching:
♻️ Suggested fix using case-insensitive comparison
impl From<&str> for ShellKind {
fn from(command: &str) -> Self {
- match command {
- "cmd" | "cmd.exe" => Self::Cmd,
- "powershell" | "powershell.exe" | "pwsh" | "pwsh.exe" => Self::Powershell,
+ match command.to_ascii_lowercase().as_str() {
+ "cmd" | "cmd.exe" => Self::Cmd,
+ "powershell" | "powershell.exe" | "pwsh" | "pwsh.exe" => Self::Powershell,
_ => Self::Other,
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl From<&str> for ShellKind { | |
| fn from(command: &str) -> Self { | |
| match command { | |
| "cmd" | "cmd.exe" => Self::Cmd, | |
| "powershell" | "powershell.exe" | "pwsh" | "pwsh.exe" => Self::Powershell, | |
| _ => Self::Other, | |
| } | |
| } | |
| } | |
| impl From<&str> for ShellKind { | |
| fn from(command: &str) -> Self { | |
| match command.to_ascii_lowercase().as_str() { | |
| "cmd" | "cmd.exe" => Self::Cmd, | |
| "powershell" | "powershell.exe" | "pwsh" | "pwsh.exe" => Self::Powershell, | |
| _ => Self::Other, | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shell_kind.rs` around lines 27 - 35, The From<&str> for ShellKind
currently does case-sensitive matching causing mixed-case shell names (e.g.,
"Cmd.exe", "PWSH.EXE") to map to Other; update the impl for From<&str> (the From
conversion for ShellKind) to perform case-insensitive comparisons—either
normalize the input string (to_lowercase) before the match or use
eq_ignore_ascii_case in each arm for "cmd"/"cmd.exe",
"powershell"/"powershell.exe"/"pwsh"/"pwsh.exe"—so Windows-configured
shell.command values are correctly classified as ShellKind::Cmd or
ShellKind::Powershell.
Validate riscv64 cross-compilation following the loongarch64 pattern.
Summary by CodeRabbit
Chores
Refactor
Bug Fixes / Reliability
Tests