Skip to content

Conversation

@zhuangqh
Copy link

@zhuangqh zhuangqh commented Dec 19, 2025

bash grammer {1..60} is not supported in wsl default shell

Summary by CodeRabbit

  • Chores
    • Updated internal scripts and test utilities to explicitly use Bash for command execution, ensuring consistent behavior across different system environments.

✏️ Tip: You can customize this high-level summary in your review settings.

bash grammer {1..60} is not supported in wsl default shell

Signed-off-by: zhuangqh <[email protected]>
@CLAassistant
Copy link

CLAassistant commented Dec 19, 2025

CLA assistant check
All committers have signed the CLA.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 19, 2025

Walkthrough

Subprocess.run calls are explicitly configured to use Bash as the shell executable in two files by adding the executable="/bin/bash" parameter. This ensures Bash is used regardless of the system's default shell environment.

Changes

Cohort / File(s) Summary
Bash executable specification
scripts/run_eval_setup.py, tests/llm/utils/commands.py
Added executable="/bin/bash" parameter to subprocess.run calls to explicitly specify Bash as the shell interpreter

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly relates to the main change—adding explicit Bash executable specification to fix shell incompatibility on WSL by ensuring Bash is used instead of the system default shell.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
tests/llm/utils/commands.py (1)

42-49: Consider adding executable="/bin/bash" to other subprocess calls for consistency.

While these kubectl diagnostic commands may not currently use bash-specific syntax, adding executable="/bin/bash" would ensure consistency and prevent similar compatibility issues in the future if these commands are modified.

🔎 Proposed changes for consistency

For line 42-49:

         pod_status_result = subprocess.run(
             diagnostic_cmd,
             shell=True,
+            executable="/bin/bash",
             capture_output=True,
             text=True,
             timeout=5,
             cwd=test_case.folder,
         )

For line 63-70:

         ns_result = subprocess.run(
             ns_cmd,
             shell=True,
+            executable="/bin/bash",
             capture_output=True,
             text=True,
             timeout=5,
             cwd=test_case.folder,
         )

For line 80-87:

                     events_result = subprocess.run(
                         events_cmd,
                         shell=True,
+                        executable="/bin/bash",
                         capture_output=True,
                         text=True,
                         timeout=5,
                         cwd=test_case.folder,
                     )

Also applies to: 63-70, 80-87

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3f1d3a3 and 740b7c1.

📒 Files selected for processing (2)
  • scripts/run_eval_setup.py (1 hunks)
  • tests/llm/utils/commands.py (1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.8)
scripts/run_eval_setup.py

43-43: subprocess call with shell=True identified, security issue

(S602)

🔇 Additional comments (2)
tests/llm/utils/commands.py (1)

160-160: LGTM! Explicit bash execution fixes WSL compatibility.

This change correctly addresses the issue where bash-specific syntax (like brace expansion {1..60}) fails when the system default shell is not bash. The addition of executable="/bin/bash" ensures bash is used regardless of the default shell on WSL or other systems.

scripts/run_eval_setup.py (1)

43-43: LGTM! Change ensures bash execution for test commands.

The addition of executable="/bin/bash" correctly addresses the WSL compatibility issue by ensuring bash-specific syntax in test commands executes properly.

Regarding the static analysis warning (S602) about shell=True: This is acceptable in this context since the commands come from test YAML files (trusted sources) rather than user input, and this is a development/test script rather than production code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants