Add sonarqube for cuopt - static analysis #762
Conversation
rgsl888prabhu
added
non-breaking
📝 WalkthroughWalkthroughAdds SonarQube analysis infrastructure: new sonarqube/ docs, branch list, and an orchestration script for per-branch analysis; adds SonarQube project config; updates the CONTRIBUTING guide to reference a newer Conda CUDA YAML. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
✏️ Tip: You can disable this entire section by setting 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 |
rgsl888prabhu
changed the title
![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
🤖 Fix all issues with AI agents
In @.github/sonar-project.properties:
- Line 1: The SonarQube properties file sonar-project.properties is placed in
.github so the SonarSource action won't find it by default; either move
sonar-project.properties to the repository root or update the workflow step
using SonarSource/sonarqube-scan-action to set with: projectBaseDir: .github.
Also add standard properties (e.g., sonar.sources=.,
sonar.exclusions=**/node_modules/**,**/build/**,**/dist/**,**/*.min.js)
alongside sonar.projectKey to scope the analysis; update the workflow YAML or
the sonar-project.properties accordingly so the action locates and uses the
correct settings.
In @.github/workflows/build.yaml:
- Around line 192-195: The SonarQube scan action is looking in the repo root but
your sonar-project.properties is in .github/; either move
sonar-project.properties to the repository root (recommended) or update the
SonarSource/sonarqube-scan-action@v4 step to specify the custom projectBaseDir
pointing at ".github" (preserve existing env vars SONAR_TOKEN and
SONAR_HOST_URL) so the action can find the properties file.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/sonar-project.properties.github/workflows/build.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.12, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.11, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.10, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.10, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.11, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.13, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.13, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-sh-client / 13.1.0, 3.10, amd64, rockylinux8
- GitHub Check: checks / check-style
🔇 Additional comments (2)
.github/workflows/build.yaml (2)
185-191: LGTM on checkout configuration.Using
fetch-depth: 0for full git history is correct for SonarQube analysis, as it enables better blame information and new code detection.
196-199: No action required for this step. The quality gate action will automatically detect the SonarQube server URL from the scanner's report task file generated during the scan step. WhileSONAR_HOST_URLcan optionally be provided, it is not necessary for the action to function correctly with self-hosted SonarQube instances.Regarding version consistency: the quality gate action is at v1.2.0 (latest), which differs from the scan action's v4 release. These actions maintain separate version streams, so using different major versions is standard practice and not a concern.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/pr.yaml (1)
180-196: AddSONAR_HOST_URLto quality gate action for consistency and reliability.While the sonarqube-quality-gate-action can infer the SonarQube host URL from the scan report (report-task.txt), it's recommended to explicitly provide
SONAR_HOST_URLfor both the scan and quality gate steps, especially with self-hosted SonarQube instances. This ensures the quality gate can reliably contact the correct server without depending on the report file.Proposed fix
- uses: SonarSource/sonarqube-quality-gate-action@v1 timeout-minutes: 5 env: SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} + SONAR_HOST_URL: ${{ secrets.SONAR_HOST_URL }}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/sonar-project.properties.github/workflows/build.yaml.github/workflows/pr.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/build.yaml
- .github/sonar-project.properties
🔇 Additional comments (3)
.github/workflows/pr.yaml (3)
16-38: LGTM!Adding
sonarqube-analysisto thepr-builderneeds list correctly integrates static analysis into the PR workflow, ensuring quality gates must pass before the PR can merge.
205-213: LGTM!Using
secrets: inheritis a cleaner pattern for reusable workflows that simplifies secret management. This is consistent with other jobs in the workflow that already use this pattern (e.g.,conda-cpp-build,conda-python-build).
222-231: Consistent secret inheritance pattern applied.The
secrets: inheritdirective is correctly applied across multiple test jobs, maintaining consistency with the rest of the workflow.Also applies to: 277-285, 311-319
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @sonarqube/run-sonar-analysis.sh:
- Line 111: The script currently runs cd "$clone_dir" without checking for
failure; update the invocation of cd "$clone_dir" so that if the chdir fails you
log an error including the target path and exit non‑zero (e.g., check the
command’s exit status and call echo/process logger and exit 1) to prevent
subsequent commands from running in the wrong directory.
- Around line 130-157: The sonar-scanner invocation currently passes the secret
via command-line (-Dsonar.token='$SONAR_TOKEN') and pipes output to
/tmp/sonar_${safe_branch_name}.log which can expose the token; remove the token
from the command-line and instead export the secret into the environment (e.g.,
set SONAR_TOKEN in the shell before calling sonar-scanner) so sonar-scanner
reads it from the env, and ensure the pipeline does not echo command arguments
or sensitive values into /tmp/sonar_${safe_branch_name}.log (e.g., run
sonar-scanner without embedding secrets in args and, if necessary, filter the
tee output to redact any accidental token occurrences) — update the block around
the sonar-scanner invocation and the surrounding shell snippet that references
SONAR_TOKEN and branch/safe_branch_name accordingly.
🧹 Nitpick comments (4)
sonarqube/run-sonar-analysis.sh (3)
104-104: Consider sanitizing branch names to prevent command injection.The
$branchvariable is read from a file and used directly ingit clone --branch "$branch". While unlikely in a controlled environment, a maliciously crafted branch name insonar-branches.txtcould potentially cause issues. Consider validating branch names match expected patterns.🔧 Optional: Add branch name validation
# Trim whitespace and add to array branch=$(echo "$branch" | xargs) + # Validate branch name contains only safe characters + if [[ ! "$branch" =~ ^[a-zA-Z0-9._/-]+$ ]]; then + echo "WARNING: Skipping invalid branch name: $branch" + continue + fi branches+=("$branch")
159-163: Improve failure classification logic.The current logic checks for "Build failed" string in the build log, but if the build log doesn't exist or the string isn't found, it defaults to "sonar analysis failed". This could misclassify failures. Consider checking log existence and using more robust detection.
🔧 More robust failure detection
- if grep -q "Build failed" /tmp/build_${safe_branch_name}.log 2>/dev/null; then - failed_branches+=("$branch (build failed)") - else - failed_branches+=("$branch (sonar analysis failed)") - fi + if [ -f /tmp/build_${safe_branch_name}.log ] && grep -q "Build failed" /tmp/build_${safe_branch_name}.log; then + failed_branches+=("$branch (build failed)") + elif [ -f /tmp/sonar_${safe_branch_name}.log ] && grep -q "SonarQube analysis failed" /tmp/sonar_${safe_branch_name}.log; then + failed_branches+=("$branch (sonar analysis failed)") + else + failed_branches+=("$branch (unknown failure)") + fi
49-57: Consider preserving logs on failure before cleanup.The
trap cleanup EXITremoves$WORK_DIRon exit, but the logs are stored in/tmp/outside this directory, so they're preserved. However, it might be helpful to explicitly mention this or provide an option to preserve the working directory for debugging failed branches.sonarqube/README.md (1)
25-33: Consider documenting the optionalSONAR_HOST_URLvariable.The script supports a custom SonarQube server via
SONAR_HOST_URL, but the README only mentionsSONAR_TOKEN. Adding this information would help users with self-hosted SonarQube instances.📝 Suggested addition
### 2. Set Required Environment Variable The script requires authentication: ```bash export SONAR_TOKEN="your_token_here" + +# Optional: For self-hosted SonarQube (defaults to SonarCloud) +export SONAR_HOST_URL="https://your-sonarqube-server.com"Note: Contact the cuOpt team for token details.
</details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between f2b8381f81cf1031649da69a5d55161ce35305f8 and ac0a4dbe786c24cbc000ba5b1d32c5dd019af7d7. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `CONTRIBUTING.md` * `sonar-project.properties` * `sonarqube/README.md` * `sonarqube/run-sonar-analysis.sh` * `sonarqube/sonar-branches.txt` </details> <details> <summary>✅ Files skipped from review due to trivial changes (2)</summary> * sonarqube/sonar-branches.txt * sonar-project.properties </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 Shellcheck (0.11.0)</summary> <details> <summary>sonarqube/run-sonar-analysis.sh</summary> [warning] 111-111: Use 'cd ... || exit' or 'cd ... || return' in case cd fails. (SC2164) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)</summary> * GitHub Check: wheel-build-cuopt-sh-client / 13.1.0, 3.10, amd64, rockylinux8 * GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.10, arm64, rockylinux8 * GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.10, amd64, rockylinux8 * GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.11, amd64, rockylinux8 * GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.11, arm64, rockylinux8 * GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.13, amd64, rockylinux8 * GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.12, amd64, rockylinux8 * GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.12, arm64, rockylinux8 * GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.13, arm64, rockylinux8 </details> <details> <summary>🔇 Additional comments (5)</summary><blockquote> <details> <summary>sonarqube/run-sonar-analysis.sh (3)</summary><blockquote> `1-8`: **LGTM on script initialization.** Proper license header and sensible defaults for script directory detection and timestamped working directory. --- `9-28`: **Good security practice for token handling.** Requiring `SONAR_TOKEN` via environment variable (rather than hardcoding or accepting as argument) is the correct approach. The helpful error message with instructions is a nice touch. --- `184-219`: **LGTM on the summary and exit logic.** Clear summary output with success/failure tracking and appropriate exit codes. The cleanup via trap ensures temporary directories are removed even on failures. </blockquote></details> <details> <summary>sonarqube/README.md (1)</summary><blockquote> `1-60`: **Good documentation coverage.** The README provides clear instructions covering the essential workflow: configuring branches, setting credentials, and running the script. The script behavior section accurately reflects what the script does. </blockquote></details> <details> <summary>CONTRIBUTING.md (1)</summary><blockquote> `139-139`: **The conda environment file reference is correct and complete.** The CUDA version update from 130 to 131 is properly implemented. Both required environment files (`all_cuda-131_arch-aarch64.yaml` and `all_cuda-131_arch-x86_64.yaml`) exist in the repository, and all references to the old version have been removed. The change is consistently applied across CONTRIBUTING.md and the sonarqube integration scripts. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @sonarqube/run-sonar-analysis.sh:
- Around line 99-103: The script changes into "$clone_dir" without restoring the
previous working directory; wrap the cd by using pushd "$clone_dir" (instead of
cd) when entering the clone and ensure you call popd > /dev/null before every
continue path (e.g., where failed_branches+=(... ) followed by continue) and
also call popd > /dev/null just before the final rm -rf "$clone_dir" at the end
of the loop so the original cwd is restored even when the clone is removed.
🧹 Nitpick comments (2)
sonarqube/run-sonar-analysis.sh (2)
92-92: Log files in /tmp are not cleaned up.Log files (
/tmp/clone_*.log,/tmp/conda_create_*.log,/tmp/build_*.log,/tmp/sonar_*.log) are written outsideWORK_DIRand won't be cleaned up by the exit trap. Over time, these can accumulate.Proposed fix: Write logs to WORK_DIR
- if ! git clone --single-branch --branch "$branch" --depth 1 "$REPO_URL" "$clone_dir" 2>&1 | tee /tmp/clone_${safe_branch_name}.log; then + if ! git clone --single-branch --branch "$branch" --depth 1 "$REPO_URL" "$clone_dir" 2>&1 | tee "$WORK_DIR/clone_${safe_branch_name}.log"; thenApply similar changes to lines 112, 132, and 142 to use
$WORK_DIR/prefix for all log files.
139-145: Token handling in bash -c could break with special characters.If
SONAR_TOKENcontains single quotes or other shell metacharacters, the string interpolation into thebash -ccommand could cause syntax errors or unexpected behavior.Proposed fix: Export token and reference in subshell
+ # Export token for subshell access + export SONAR_TOKEN + if ! bash -c " set -e source \$(conda info --base)/etc/profile.d/conda.sh conda activate $conda_env_name ... # Run SonarQube analysis echo 'Running SonarQube analysis...' if ! sonar-scanner \ - -Dsonar.token='$SONAR_TOKEN' \ + -Dsonar.token=\"\$SONAR_TOKEN\" \ -Dsonar.branch.name='$branch' \ 2>&1 | tee /tmp/sonar_${safe_branch_name}.log; thenBy exporting the variable and referencing it with
\$SONAR_TOKEN(escaped to defer expansion to the subshell), the token is passed safely without string interpolation issues.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sonar-project.propertiessonarqube/run-sonar-analysis.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- sonar-project.properties
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: conda-cpp-build / 13.1.0, 3.10, amd64, rockylinux8
- GitHub Check: conda-cpp-build / 12.9.1, 3.10, arm64, rockylinux8
- GitHub Check: conda-cpp-build / 12.9.1, 3.10, amd64, rockylinux8
- GitHub Check: conda-cpp-build / 13.1.0, 3.10, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-server / 13.1.0, 3.13, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-server / 12.9.1, 3.13, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.11, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.10, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.11, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.13, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.13, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.12, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.10, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.12, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-sh-client / 13.1.0, 3.10, amd64, rockylinux8
🔇 Additional comments (3)
sonarqube/run-sonar-analysis.sh (3)
1-13: Well-structured script initialization.The script setup is clean with proper license headers, dynamic architecture detection via
uname -m, and configurable paths. The comment on line 10-11 providing available YAML options is helpful for maintainers.
37-45: Good use of cleanup trap.The trap on EXIT ensures the working directory is cleaned up regardless of how the script exits, which is a shell scripting best practice.
175-210: Clear summary and appropriate exit handling.The final summary with counts and lists of successful/failed branches provides good visibility. Exiting with non-zero status when any branch fails ensures CI will correctly report failures.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @sonarqube/run-sonar-analysis.sh:
- Around line 32-40: The cleanup() function currently only removes $WORK_DIR but
leaves log files in /tmp; update cleanup() (the cleanup function registered by
trap cleanup EXIT) to also remove the stale log patterns (/tmp/clone_*.log,
/tmp/conda_create_*.log, /tmp/build_*.log, /tmp/sonar_*.log) or alternatively
ensure all logs are written into $WORK_DIR so they are removed when $WORK_DIR is
deleted; make the change inside the cleanup() function and keep the trap cleanup
EXIT registration.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sonarqube/run-sonar-analysis.sh
🔇 Additional comments (5)
sonarqube/run-sonar-analysis.sh (5)
1-13: LGTM!Script setup is well-structured with proper license headers and architecture-aware environment configuration.
48-65: LGTM!The branch file parsing correctly handles comments, empty lines, and the edge case of files without a trailing newline.
81-91: LGTM!Shallow cloning with
--depth 1 --single-branchis efficient for CI analysis. Error handling for clone failures is appropriate.
100-168: LGTM!Good resource management with proper cleanup of conda environments and clone directories on both success and failure paths.
170-205: LGTM!Clear summary output with proper exit code for CI integration. The non-zero exit on any failure ensures CI pipelines will correctly detect analysis issues.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @sonarqube/run-sonar-analysis.sh:
- Around line 134-140: The script passes the token via the sonar-scanner command
using -Dsonar.token='$SONAR_TOKEN', exposing secrets in process listings; remove
the -Dsonar.token argument and allow sonar-scanner to read SONAR_TOKEN from the
environment (keep the rest of the invocation, piping to tee
/tmp/sonar_${safe_branch_name}.log unchanged) so the token is not visible in ps
output.
🧹 Nitpick comments (1)
sonarqube/run-sonar-analysis.sh (1)
1-4: Consider addingset -euo pipefailfor safer script execution.Adding strict mode helps catch unset variables and ensures pipeline failures are detected. This is particularly relevant given the use of
| teepatterns later in the script.Suggested improvement
#!/bin/bash # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 +set -euo pipefail
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sonarqube/run-sonar-analysis.sh
🔇 Additional comments (3)
sonarqube/run-sonar-analysis.sh (3)
32-40: LGTM!The cleanup trap pattern is well-implemented, and log files in
/tmp/appropriately survive the cleanup for post-mortem debugging.
48-65: LGTM!The branch parsing handles comments, empty lines, and the edge case of files without trailing newlines correctly.
170-205: LGTM!The summary output and exit code handling are well-structured, providing clear CI feedback.
rgsl888prabhu
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @sonarqube/cron-wrapper.sh:
- Around line 10-14: Update the hardcoded development repo and branch variables:
replace REPO_URL (currently pointing to rgsl888prabhu/cuopt_public.git) with the
official production repository URL and set REPO_BRANCH (currently
enable_sonar_cube_for_cuopt) to the production branch (e.g., main); ensure these
variables in the script are the canonical production values before merging so
the cron uses the correct repository and branch.
- Around line 118-128: The pipeline is incorrectly reporting the exit status of
run-sonar-analysis.sh because `$?` reflects the last command (tee); fix by
ensuring the real exit of the left-hand command is captured: either enable
pipefail at the top of the script (e.g., set -o pipefail) so `if
./sonarqube/run-sonar-analysis.sh 2>&1 | tee -a "$LOG_FILE"; then` evaluates the
producer's status, or after the pipeline read the producer's status from the
PIPESTATUS array (use EXIT_CODE=${PIPESTATUS[0]} and then test/exit based on
that) when logging failure for run-sonar-analysis.sh.
🧹 Nitpick comments (2)
sonarqube/run-sonar-analysis.sh (2)
108-114: Consider adding a fallback frommambatoconda.The script uses
mambadirectly, which may not be installed in all environments. Consider falling back tocondaifmambais unavailable.Proposed fix
+ # Use mamba if available, otherwise fall back to conda + if command -v mamba &> /dev/null; then + CONDA_CMD="mamba" + else + CONDA_CMD="conda" + fi + # Create conda environment - mamba env create -n "$conda_env_name" -f "$CONDA_ENV_FILE" 2>&1 | tee /tmp/conda_create_${safe_branch_name}.log + $CONDA_CMD env create -n "$conda_env_name" -f "$CONDA_ENV_FILE" 2>&1 | tee /tmp/conda_create_${safe_branch_name}.log
119-147: Complex subshell with string interpolation is fragile.The
bash -cwith embedded script string works but is difficult to maintain and debug. The mix of escaped and unescaped variables ($conda_env_namevs\$(python --version)) is error-prone.Consider extracting this into a separate helper script or using a heredoc with proper quoting for better maintainability.
Alternative approach using a heredoc
# Write a temporary script for the subshell cat > /tmp/build_and_analyze_${safe_branch_name}.sh << 'INNER_SCRIPT' #!/bin/bash set -e source $(conda info --base)/etc/profile.d/conda.sh conda activate "$1" echo "Conda environment activated: $1" echo "Python version: $(python --version)" echo 'Building project...' ./build.sh 2>&1 | tee "$2" if [ ${PIPESTATUS[0]} -ne 0 ]; then echo 'Build failed' exit 1 fi echo 'Running SonarQube analysis...' sonar-scanner -Dsonar.branch.name="$3" 2>&1 | tee "$4" if [ ${PIPESTATUS[0]} -ne 0 ]; then echo 'SonarQube analysis failed' exit 1 fi echo 'Build and analysis completed successfully' INNER_SCRIPT chmod +x /tmp/build_and_analyze_${safe_branch_name}.sh if ! /tmp/build_and_analyze_${safe_branch_name}.sh \ "$conda_env_name" \ "/tmp/build_${safe_branch_name}.log" \ "$branch" \ "/tmp/sonar_${safe_branch_name}.log"; then # ... error handling fi
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sonarqube/cron-wrapper.shsonarqube/run-sonar-analysis.sh
🔇 Additional comments (9)
sonarqube/cron-wrapper.sh (3)
1-8: LGTM on shebang, license, and error handling setup.The script header is well-structured with proper SPDX licensing and
set -efor fail-fast behavior.
35-40: SONAR_TOKEN validation is appropriate.Good practice to validate the required environment variable early and provide a helpful error message.
42-77: Repository update logic is well-structured.The clone/pull logic handles edge cases appropriately:
- Validates existing directory is a git repository
- Handles branch switching with proper error checking
- Cleans up and re-clones if the directory is corrupted
sonarqube/run-sonar-analysis.sh (6)
1-14: LGTM on script setup and architecture detection.Good use of
SCRIPT_DIRfor relative path resolution and dynamic architecture detection for the conda environment file.
23-24: Repository URL is correctly configured.Unlike the cron-wrapper.sh, this script correctly points to the main NVIDIA repository.
32-40: Cleanup trap is well-implemented.Good practice to register a cleanup function that removes the working directory on exit.
48-65: Branch file parsing handles edge cases well.Properly skips comments and empty lines, trims whitespace, and validates that at least one branch is configured.
174-209: Final summary and exit logic is comprehensive.Good reporting of successful and failed branches with appropriate exit codes.
85-92: Shallow clone is efficient for analysis.Using
--depth 1for shallow clones is appropriate since only the current state is needed for static analysis. Good use ofPIPESTATUSfor pipeline exit status here.
…u/cuopt_public into enable_sonar_cube_for_cuopt
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@sonarqube/run-sonar-analysis.sh`:
- Around line 145-173: The bash -c block currently interpolates unquoted
variables ($conda_env_name, $branch, safe_branch_name) and can be exploited via
quote/command injection; replace the inline bash -c string with a safe subshell
or heredoc that passes variables via exported environment variables (or uses a
single-quoted heredoc) so no user-controlled value is injected into the command
string, ensure all usages of safe_branch_name and LOG_DIR in the subshell are
quoted, and run sonar-scanner and ./build.sh with their outputs piped to tee as
before; additionally, add validation where branches are read by invoking git
check-ref-format --branch "$branch" (and reject/skip invalid branches early) to
prevent invalid or malicious branch names from entering the workflow.
🧹 Nitpick comments (1)
sonarqube/run-sonar-analysis.sh (1)
7-7: MakeWORK_DIRcollision‑safe for concurrent runs.Timestamp‑only paths can collide if the script is launched twice within the same second, which can mix logs and cleanup. Prefer
mktemp -d(or add$$) for uniqueness.💡 Suggested change
-WORK_DIR="/tmp/sonar-analysis-$(date +%Y%m%d-%H%M%S)" +WORK_DIR="$(mktemp -d -t sonar-analysis.XXXXXX)"Please verify
mktempis available on the target hosts (most Linux distros ship it by default).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sonarqube/run-sonar-analysis.sh
🧰 Additional context used
🪛 Shellcheck (0.11.0)
sonarqube/run-sonar-analysis.sh
[warning] 137-137: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 183-183: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 198-198: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.13, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.11, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.13, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.10, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.10, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.11, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.12, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.12, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-sh-client / 13.1.0, 3.10, amd64, rockylinux8
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if ! bash -c " | ||
| set -e | ||
| source \$(conda info --base)/etc/profile.d/conda.sh | ||
| conda activate $conda_env_name | ||
| echo 'Conda environment activated: $conda_env_name' | ||
| echo 'Python version:' \$(python --version) | ||
| # Build the project | ||
| echo 'Building project...' | ||
| ./build.sh 2>&1 | tee '$LOG_DIR/build_${safe_branch_name}.log' | ||
| if [ \${PIPESTATUS[0]} -ne 0 ]; then | ||
| echo 'Build failed' | ||
| exit 1 | ||
| fi | ||
| # Run SonarQube analysis | ||
| # Note: SONAR_TOKEN is read from environment automatically by sonar-scanner | ||
| echo 'Running SonarQube analysis...' | ||
| sonar-scanner \ | ||
| -Dsonar.branch.name='$branch' \ | ||
| 2>&1 | tee '$LOG_DIR/sonar_${safe_branch_name}.log' | ||
| if [ \${PIPESTATUS[0]} -ne 0 ]; then | ||
| echo 'SonarQube analysis failed' | ||
| exit 1 | ||
| fi | ||
| echo 'Build and analysis completed successfully' | ||
| "; then |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd sonarqube && ls -laRepository: NVIDIA/cuopt
Length of output: 370
🏁 Script executed:
cat -n run-sonar-analysis.sh | head -200Repository: NVIDIA/cuopt
Length of output: 109
🏁 Script executed:
# Check if sonar-branches.txt exists
find . -name "sonar-branches.txt" -o -name "*branches*" 2>/dev/nullRepository: NVIDIA/cuopt
Length of output: 86
🏁 Script executed:
cat -n sonarqube/run-sonar-analysis.sh | sed -n '140,180p'Repository: NVIDIA/cuopt
Length of output: 1648
🏁 Script executed:
cat sonarqube/sonar-branches.txtRepository: NVIDIA/cuopt
Length of output: 324
🏁 Script executed:
# Check how branches are read and processed in the script
cat -n sonarqube/run-sonar-analysis.sh | head -100Repository: NVIDIA/cuopt
Length of output: 3731
🏁 Script executed:
# Check how safe_branch_name is derived and if there's any sanitization
cat -n sonarqube/run-sonar-analysis.sh | sed -n '100,145p'Repository: NVIDIA/cuopt
Length of output: 2134
🏁 Script executed:
# Check conda activation and environment variable handling
cat -n sonarqube/run-sonar-analysis.sh | sed -n '1,40p'Repository: NVIDIA/cuopt
Length of output: 1730
Prevent command injection via unquoted variable interpolation in the bash -c block.
The current code interpolates $branch and $conda_env_name into a bash -c string without proper quoting. If a branch name contains special characters or quotes, it can break out of the command context and execute arbitrary code. The safe_branch_name substitution only replaces / with _; it does not prevent quote injection.
Additionally, branch names read from sonar-branches.txt are not validated. Consider validating branch names against git check-ref-format --branch to ensure only valid Git references are accepted.
The proposed subshell approach with proper variable quoting is the correct solution:
🔧 Safer subshell-based rewrite
- if ! bash -c "
- set -e
- source \$(conda info --base)/etc/profile.d/conda.sh
- conda activate $conda_env_name
+ if ! (
+ set -e
+ source "$(conda info --base)/etc/profile.d/conda.sh"
+ conda activate "$conda_env_name"
- echo 'Conda environment activated: $conda_env_name'
- echo 'Python version:' \$(python --version)
+ echo "Conda environment activated: $conda_env_name"
+ echo "Python version: $(python --version 2>&1)"
# Build the project
echo 'Building project...'
- ./build.sh 2>&1 | tee '$LOG_DIR/build_${safe_branch_name}.log'
+ ./build.sh 2>&1 | tee "$LOG_DIR/build_${safe_branch_name}.log"
if [ \${PIPESTATUS[0]} -ne 0 ]; then
echo 'Build failed'
exit 1
fi
# Run SonarQube analysis
# Note: SONAR_TOKEN is read from environment automatically by sonar-scanner
echo 'Running SonarQube analysis...'
sonar-scanner \
- -Dsonar.branch.name='$branch' \
- 2>&1 | tee '$LOG_DIR/sonar_${safe_branch_name}.log'
+ -Dsonar.branch.name="$branch" \
+ 2>&1 | tee "$LOG_DIR/sonar_${safe_branch_name}.log"
if [ \${PIPESTATUS[0]} -ne 0 ]; then
echo 'SonarQube analysis failed'
exit 1
fi
echo 'Build and analysis completed successfully'
- "; then
+ ); thenAlso add branch name validation in the loop (lines 73–82) using git check-ref-format --branch "$branch" to reject invalid Git references early.
🤖 Prompt for AI Agents
In `@sonarqube/run-sonar-analysis.sh` around lines 145 - 173, The bash -c block
currently interpolates unquoted variables ($conda_env_name, $branch,
safe_branch_name) and can be exploited via quote/command injection; replace the
inline bash -c string with a safe subshell or heredoc that passes variables via
exported environment variables (or uses a single-quoted heredoc) so no
user-controlled value is injected into the command string, ensure all usages of
safe_branch_name and LOG_DIR in the subshell are quoted, and run sonar-scanner
and ./build.sh with their outputs piped to tee as before; additionally, add
validation where branches are read by invoking git check-ref-format --branch
"$branch" (and reject/skip invalid branches early) to prevent invalid or
malicious branch names from entering the workflow.
…PIs (NVIDIA#781) Replace cudf Column and Buffers APIs with pylibcudf and public cuDF APIs ## Issue closes NVIDIA#762 579 Authors: - Ramakrishnap (https://github.com/rgsl888prabhu) Approvers: - Trevor McKay (https://github.com/tmckayus) URL: NVIDIA#781
1 participant
Description
This PR adds script to run sonarqube manually to create report.
Issue
closes #151
Checklist
Summary by CodeRabbit
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.