fix(binary-installer): correct grep -q usage to avoid duplicate PATH / duplicate PATH entries#13410
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
All contributors have signed the CLA ✍️ ✅ |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces command-substitution checks using Changes
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
|
I have read the CLA Document and I hereby sign the CLA |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
binary-installer/install.sh (1)
72-90:⚠️ Potential issue | 🟠 MajorRemove the duplicated
ifon line 72; it breaks fallback control flow.The double condition (
if [[ -r $SHELL_CONFIG ]]; then if [[ -r $SHELL_CONFIG ]]; then) causes theelseon line 77 to bind to the innerifinstead of the outer one. When.bash_profileis unreadable, the entireelseblock (lines 78–88) that handles fallback to.bash_loginand.profileis skipped, breaking the intended cascade through shell startup files.Proposed fix
SHELL_CONFIG=$HOME/.bash_profile - if [[ -r $SHELL_CONFIG ]]; then if [[ -r $SHELL_CONFIG ]]; then + if [[ -r $SHELL_CONFIG ]]; then if ! grep -q '.serverless/bin' "$SHELL_CONFIG"; then add_to_path $SHELL_CONFIG fi else SHELL_CONFIG=$HOME/.bash_login🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@binary-installer/install.sh` around lines 72 - 90, Remove the duplicated nested "if [[ -r $SHELL_CONFIG ]]" so the fallback else branch correctly pairs with the outer test; locate the block using the SHELL_CONFIG variable and add_to_path function references, delete the inner redundant if that starts the duplicate check, and ensure the remaining if/else cascade checks $HOME/.bash_profile then falls back to $HOME/.bash_login and $HOME/.profile while calling add_to_path when a target file does not already contain '.serverless/bin'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@binary-installer/install.sh`:
- Around line 63-69: The script uses unquoted SHELL_CONFIG in assignments,
conditional tests and when calling add_to_path which can break on spaces or glob
chars; update all occurrences to quote the variable (e.g., use "SHELL_CONFIG" in
assignments, "[ ! -r "$SHELL_CONFIG" ]" and "grep -q '.serverless/bin'
"$SHELL_CONFIG"" tests, and call add_to_path "$SHELL_CONFIG") so every reference
to SHELL_CONFIG is wrapped in double quotes; ensure you update every use in the
if/else blocks and any other occurrences in the file (references: SHELL_CONFIG,
add_to_path, grep -q).
---
Outside diff comments:
In `@binary-installer/install.sh`:
- Around line 72-90: Remove the duplicated nested "if [[ -r $SHELL_CONFIG ]]" so
the fallback else branch correctly pairs with the outer test; locate the block
using the SHELL_CONFIG variable and add_to_path function references, delete the
inner redundant if that starts the duplicate check, and ensure the remaining
if/else cascade checks $HOME/.bash_profile then falls back to $HOME/.bash_login
and $HOME/.profile while calling add_to_path when a target file does not already
contain '.serverless/bin'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 892ffadd-d22e-48b4-8c0e-572ca6c0453d
📒 Files selected for processing (1)
binary-installer/install.sh
binary-installer/install.sh
Outdated
| if [ ! -r $SHELL_CONFIG ] || ! grep -q '.serverless/bin' "$SHELL_CONFIG"; then | ||
| add_to_path $SHELL_CONFIG | ||
| fi | ||
| else | ||
| SHELL_CONFIG=$HOME/.bashrc | ||
| if [ ! -r $SHELL_CONFIG ] || (! `grep -q '.serverless/bin' $SHELL_CONFIG`); then | ||
| if [ ! -r $SHELL_CONFIG ] || ! grep -q '.serverless/bin' "$SHELL_CONFIG"; then | ||
| add_to_path $SHELL_CONFIG |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify unquoted SHELL_CONFIG occurrences in file tests and add_to_path calls.
rg -nP '\[\s*! -r \$SHELL_CONFIG\s*\]|add_to_path \$SHELL_CONFIG' binary-installer/install.shRepository: serverless/serverless
Length of output: 495
🏁 Script executed:
# Check the file around the affected lines to understand context
cat -n binary-installer/install.sh | sed -n '55,95p'Repository: serverless/serverless
Length of output: 1556
🏁 Script executed:
# Check the shebang and overall script compatibility
head -5 binary-installer/install.shRepository: serverless/serverless
Length of output: 136
🏁 Script executed:
# Find the add_to_path function definition to understand its parameter handling
rg -n 'add_to_path' binary-installer/install.sh -A 3 | head -30Repository: serverless/serverless
Length of output: 737
Quote $SHELL_CONFIG in variable assignments, tests, and function calls.
Unquoted $SHELL_CONFIG can cause word-splitting or pathname expansion if the path contains spaces or special characters. This occurs at:
- Lines 62, 67, 71, 78, 84: Variable assignments
- Lines 63, 68, 85: Conditional tests
- Lines 64, 69, 75, 81, 86: Function calls to
add_to_path
Quote the assignments and use the variable quoted in all contexts for consistency and correctness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@binary-installer/install.sh` around lines 63 - 69, The script uses unquoted
SHELL_CONFIG in assignments, conditional tests and when calling add_to_path
which can break on spaces or glob chars; update all occurrences to quote the
variable (e.g., use "SHELL_CONFIG" in assignments, "[ ! -r "$SHELL_CONFIG" ]"
and "grep -q '.serverless/bin' "$SHELL_CONFIG"" tests, and call add_to_path
"$SHELL_CONFIG") so every reference to SHELL_CONFIG is wrapped in double quotes;
ensure you update every use in the if/else blocks and any other occurrences in
the file (references: SHELL_CONFIG, add_to_path, grep -q).
2 participants
Problem
The installer script incorrectly checks for
.serverless/binin shell configuration files using command substitution with
grep -q.grep -qreturns an exit status but produces no output, so usingcommand substitution (
$()or backticks) always results in an emptystring. This causes the condition to always evaluate true and results
in duplicate PATH entries being appended on repeated installs.
Solution
Check the exit status of
grep -qdirectly instead of using commandsubstitution. Also added proper quoting for
$SHELL_CONFIG.Result
The installer now correctly avoids adding duplicate
.serverless/binentries to shell configuration files.
Fixes #13394
Summary by CodeRabbit