add --force flag and positional <env> argument to vulnerabilities ins…#23
Closed
ortizjeronimo wants to merge 2 commits into
Closed
add --force flag and positional <env> argument to vulnerabilities ins…#23ortizjeronimo wants to merge 2 commits into
ortizjeronimo wants to merge 2 commits into
Conversation
agustingroh
approved these changes
Sep 17, 2025
📝 WalkthroughWalkthroughThe bash script Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/env-setup.sh (1)
40-43: Invalid flags are silently treated as environment names.The
*case captures any argument, including misspelled flags like--froceor unknown options like--debug. This could lead to confusing behavior where a typo creates an unexpected service name.Consider validating that non-flag arguments don't start with
-:🛡️ Proposed fix to validate positional arguments
*) + if [[ "$1" == -* ]]; then + echo "Unknown option: $1" + show_help + fi ENVIRONMENT="$1" shift ;;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/env-setup.sh` around lines 40 - 43, The case '*' branch currently assigns any argument to ENVIRONMENT (ENVIRONMENT="$1"; shift) which treats unknown flags or typos like "--froce" as valid environments; update the '*' handler to validate the positional argument: if "$1" begins with '-' print a clear error message and exit non‑zero, otherwise assign ENVIRONMENT="$1" and shift; change the case pattern or add an explicit check in the '*' branch so flags are rejected instead of silently accepted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/env-setup.sh`:
- Around line 104-105: The chmod command on
/usr/local/bin/scanoss-vulnerabilities-api.sh lacks error handling; mirror the
cp error check by testing chmod's exit status and exiting with a non‑zero code
on failure: after running chmod +x "$DEST" (or the existing
/usr/local/bin/scanoss-vulnerabilities-api.sh reference), check its return value
and call echo "chmod failed" and exit 1 if it fails; locate the chmod invocation
in scripts/env-setup.sh and use the same error-handling pattern used for the cp
command that references $SCRIPT_DIR/scanoss-vulnerabilities-api.sh.
- Around line 131-138: The current curl redirection to "$CONF_DIR/$CONF" can
truncate or create a partial config on failure; update the download logic (both
the FORCE_INSTALL branch and the interactive branch where read -p is used) to
download into a temporary file (e.g., "$CONF_DIR/$CONF.tmp" or using mktemp),
verify curl exit status and non-empty content, then atomically move the temp
file to "$CONF_DIR/$CONF" (mv) only on success; if the download fails, remove
the temp file and emit a clear warning and do not overwrite the existing
"$CONF_DIR/$CONF" (and optionally exit with non-zero when FORCE_INSTALL is set).
---
Nitpick comments:
In `@scripts/env-setup.sh`:
- Around line 40-43: The case '*' branch currently assigns any argument to
ENVIRONMENT (ENVIRONMENT="$1"; shift) which treats unknown flags or typos like
"--froce" as valid environments; update the '*' handler to validate the
positional argument: if "$1" begins with '-' print a clear error message and
exit non‑zero, otherwise assign ENVIRONMENT="$1" and shift; change the case
pattern or add an explicit check in the '*' branch so flags are rejected instead
of silently accepted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+104
to
+105
| cp "$SCRIPT_DIR/scanoss-vulnerabilities-api.sh" /usr/local/bin || { echo "startup script copy failed"; exit 1; } | ||
| chmod +x /usr/local/bin/scanoss-vulnerabilities-api.sh |
There was a problem hiding this comment.
Inconsistent error handling on chmod.
Line 104 has proper error handling, but line 105's chmod has no error handling. If the permission change fails, the startup script may not be executable, causing service start failures later.
🔧 Proposed fix to add consistent error handling
cp "$SCRIPT_DIR/scanoss-vulnerabilities-api.sh" /usr/local/bin || { echo "startup script copy failed"; exit 1; }
-chmod +x /usr/local/bin/scanoss-vulnerabilities-api.sh
+chmod +x /usr/local/bin/scanoss-vulnerabilities-api.sh || { echo "chmod on startup script failed"; exit 1; }📝 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.
Suggested change
| cp "$SCRIPT_DIR/scanoss-vulnerabilities-api.sh" /usr/local/bin || { echo "startup script copy failed"; exit 1; } | |
| chmod +x /usr/local/bin/scanoss-vulnerabilities-api.sh | |
| cp "$SCRIPT_DIR/scanoss-vulnerabilities-api.sh" /usr/local/bin || { echo "startup script copy failed"; exit 1; } | |
| chmod +x /usr/local/bin/scanoss-vulnerabilities-api.sh || { echo "chmod on startup script failed"; exit 1; } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/env-setup.sh` around lines 104 - 105, The chmod command on
/usr/local/bin/scanoss-vulnerabilities-api.sh lacks error handling; mirror the
cp error check by testing chmod's exit status and exiting with a non‑zero code
on failure: after running chmod +x "$DEST" (or the existing
/usr/local/bin/scanoss-vulnerabilities-api.sh reference), check its return value
and call echo "chmod failed" and exit 1 if it fails; locate the chmod invocation
in scripts/env-setup.sh and use the same error-handling pattern used for the cp
command that references $SCRIPT_DIR/scanoss-vulnerabilities-api.sh.
Comment on lines
+131
to
+138
| if [ "$FORCE_INSTALL" -eq 1 ]; then | ||
| echo "[FORCE] Downloading default config to $CONF_DIR/$CONF" | ||
| curl -s "$CONF_DOWNLOAD" > "$CONF_DIR/$CONF" || echo "Warning: curl download failed" | ||
| else | ||
| read -p "Download sample $CONF (y/n) [y]? " -n 1 -r | ||
| echo | ||
| if [[ ! $REPLY =~ ^[Nn]$ ]]; then | ||
| curl -s "$CONF_DOWNLOAD" > "$CONF_DIR/$CONF" || echo "Warning: curl download failed" |
There was a problem hiding this comment.
Curl failure may leave an empty or partial config file.
When curl fails, the output redirection (>) still creates/truncates the target file. This could leave an empty or corrupted config at $CONF_DIR/$CONF, which may cause the service to fail on startup with a confusing error.
Consider writing to a temp file first and moving on success:
🛡️ Proposed fix to avoid corrupted config on download failure
if [ "$FORCE_INSTALL" -eq 1 ]; then
echo "[FORCE] Downloading default config to $CONF_DIR/$CONF"
- curl -s "$CONF_DOWNLOAD" > "$CONF_DIR/$CONF" || echo "Warning: curl download failed"
+ if curl -sf "$CONF_DOWNLOAD" -o "$CONF_DIR/$CONF.tmp"; then
+ mv "$CONF_DIR/$CONF.tmp" "$CONF_DIR/$CONF"
+ else
+ rm -f "$CONF_DIR/$CONF.tmp"
+ echo "Warning: curl download failed"
+ fi
else
read -p "Download sample $CONF (y/n) [y]? " -n 1 -r
echo
if [[ ! $REPLY =~ ^[Nn]$ ]]; then
- curl -s "$CONF_DOWNLOAD" > "$CONF_DIR/$CONF" || echo "Warning: curl download failed"
+ if curl -sf "$CONF_DOWNLOAD" -o "$CONF_DIR/$CONF.tmp"; then
+ mv "$CONF_DIR/$CONF.tmp" "$CONF_DIR/$CONF"
+ else
+ rm -f "$CONF_DIR/$CONF.tmp"
+ echo "Warning: curl download failed"
+ fi
else📝 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.
Suggested change
| if [ "$FORCE_INSTALL" -eq 1 ]; then | |
| echo "[FORCE] Downloading default config to $CONF_DIR/$CONF" | |
| curl -s "$CONF_DOWNLOAD" > "$CONF_DIR/$CONF" || echo "Warning: curl download failed" | |
| else | |
| read -p "Download sample $CONF (y/n) [y]? " -n 1 -r | |
| echo | |
| if [[ ! $REPLY =~ ^[Nn]$ ]]; then | |
| curl -s "$CONF_DOWNLOAD" > "$CONF_DIR/$CONF" || echo "Warning: curl download failed" | |
| if [ "$FORCE_INSTALL" -eq 1 ]; then | |
| echo "[FORCE] Downloading default config to $CONF_DIR/$CONF" | |
| if curl -sf "$CONF_DOWNLOAD" -o "$CONF_DIR/$CONF.tmp"; then | |
| mv "$CONF_DIR/$CONF.tmp" "$CONF_DIR/$CONF" | |
| else | |
| rm -f "$CONF_DIR/$CONF.tmp" | |
| echo "Warning: curl download failed" | |
| fi | |
| else | |
| read -p "Download sample $CONF (y/n) [y]? " -n 1 -r | |
| echo | |
| if [[ ! $REPLY =~ ^[Nn]$ ]]; then | |
| if curl -sf "$CONF_DOWNLOAD" -o "$CONF_DIR/$CONF.tmp"; then | |
| mv "$CONF_DIR/$CONF.tmp" "$CONF_DIR/$CONF" | |
| else | |
| rm -f "$CONF_DIR/$CONF.tmp" | |
| echo "Warning: curl download failed" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/env-setup.sh` around lines 131 - 138, The current curl redirection to
"$CONF_DIR/$CONF" can truncate or create a partial config on failure; update the
download logic (both the FORCE_INSTALL branch and the interactive branch where
read -p is used) to download into a temporary file (e.g., "$CONF_DIR/$CONF.tmp"
or using mktemp), verify curl exit status and non-empty content, then atomically
move the temp file to "$CONF_DIR/$CONF" (mv) only on success; if the download
fails, remove the temp file and emit a clear warning and do not overwrite the
existing "$CONF_DIR/$CONF" (and optionally exit with non-zero when FORCE_INSTALL
is set).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added support for -f | --force flag:
Enables fully non-interactive installs.
Auto-accepts installation without user prompts.
Skips overwriting config if it already exists.
Downloads default config automatically if missing.
Summary by CodeRabbit
New Features
--forceflag to enable automated installation without interactive prompts, improving setup efficiency for scripted environments.Improvements