feat: implement issue #349 — SonarCloud: shell script hygiene - #396
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe repository settings script centralizes the ChangesShell script hygiene
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a readonly MISSING constant in the test script to replace hardcoded strings, adds explicit return statements to test helper functions, and appends a bare return statement to the apply_security_and_analysis function. The feedback recommends removing the redundant bare return statement in the Bash script, as it is unnecessary and can be misleading regarding the function's exit status.
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #349 by applying SonarCloud “shell script hygiene” fixes to the repo settings scripts/tests, aiming to reduce findings without changing runtime behavior.
Changes:
- Added a
MISSINGconstant inscripts/apply-repo-settings.test.shand reused it across assertions to remove repeated string literals. - Added explicit
return 0statements to the test helper functions (pass,fail,assert_eq) to satisfy SonarCloud’s “explicit return” rule. - Added an explicit
returnat the end ofapply_security_and_analysis()inscripts/apply-repo-settings.sh(preserving propagation of thegh apiexit status).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| scripts/apply-repo-settings.test.sh | Introduces a shared MISSING constant and adds explicit returns in helper functions to satisfy SonarCloud rules. |
| scripts/apply-repo-settings.sh | Adds an explicit end-of-function return for SonarCloud shell hygiene compliance. |
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #396 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: LOW
Reviewed commit: 9175ad4df9227d339549515a7c3afd83a9da9e09
Review mode: triage-approved (single reviewer)
Summary
Small, mechanical shell-hygiene PR (2 files, +14/−6) resolving SonarCloud findings from issue #349. Adds explicit return statements (rule S7682) to pass, fail, and assert_eq in scripts/apply-repo-settings.test.sh and to apply_security_and_analysis in scripts/apply-repo-settings.sh, and introduces a readonly MISSING constant (rule S1192) replacing six repeated 'missing' literals. No behavior change.
Linked issue analysis
Issue #349 lists 4× S7682 and 1× S1192 across exactly these two files. The diff addresses all five findings: four explicit returns and one constant extraction. Acceptance criteria met — SonarCloud quality gate passed on this PR with 0 new issues, no NOSONAR suppressions used, and no behavior change: the bare return at the end of apply_security_and_analysis preserves the gh api exit status, and return 0 in fail() is added after the failure counter increment. Verified the sourced script never assigns a MISSING variable, so the readonly declaration before source cannot conflict.
Findings
No blocking findings.
- Secret scan:
run_secret_scanningMCP tool not available in this environment; gitleaks CI check passed (SUCCESS). - Bot reviews (gemini-code-assist, copilot) are COMMENTED, non-blocking. Gemini suggested removing the bare
returninapply-repo-settings.shas redundant — declining to act on it, since removing it would reintroduce the S7682 finding this PR exists to fix, and the barereturncorrectly propagates the last command's exit status.
CI status
All substantive checks green: gitleaks, CodeQL (actions + python), SonarCloud (quality gate passed, 0 new issues), AgentShield, dependency-audit, PR Review Agent. Two CANCELLED dev-lead check runs (dispatch, ci-relay) belong to a superseded Dev-Lead Agent run; a later run of the same check names completed SUCCESS/SKIPPED. Mergeable; merge state BLOCKED only pending required review.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #396 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/apply-repo-settings.test.sh`:
- Around line 10-13: Move the MISSING constant from apply-repo-settings.test.sh
into apply-repo-settings.sh, then replace the repeated "missing" literals in
auto_trigger_status and pr_quality_merge_methods_status with that shared
constant. Update the tests to consume the sourced production value instead of
defining their own copy.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: edd12c4a-8ae5-426d-bd38-e158748031c5
📒 Files selected for processing (2)
scripts/apply-repo-settings.shscripts/apply-repo-settings.test.sh
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: LOW
Reviewed commit: 83f3b0361a295164b7bb55a8fdf50bffdf9d4392
Review mode: triage-approved (single reviewer)
Summary
Mechanical shell-hygiene PR (2 files, +19/−13) resolving SonarCloud findings from issue #349. Introduces a readonly MISSING constant in scripts/apply-repo-settings.sh (rule S1192, replacing six 'missing' literals — jq expressions receive it via --arg) and adds explicit return statements to pass/fail/assert_eq in the test script and apply_security_and_analysis in the production script (rule S7682). No behavior change. Since the prior approved SHA (9175ad4, dismissed on push), the branch merged main and added one fix commit moving MISSING into the production script per CodeRabbit's review comment, which CodeRabbit confirmed as addressed (thread resolved).
Linked issue analysis
Closes #349 (SonarCloud: shell script hygiene — 4× S7682 explicit-return, 1× S1192 missing-constant). The diff addresses exactly these findings in exactly the two affected files, and the SonarCloud / SonarCloud Code Analysis checks pass on this PR, confirming the quality gate. Acceptance criteria (real fixes, no NOSONAR suppressions, no behavior change, CI green) are met.
Findings
- No security-relevant changes: no auth/secrets/crypto/migrations/workflow edits (the dev-lead.yml delta in the compare range came from merging main, not this PR — changedFiles confirms only the two shell scripts).
- Secret scan: run_secret_scanning MCP tool not available in this environment; gitleaks CI check passed and the diff contains no secret-like content.
- One unresolved review thread remains: a low-priority gemini-code-assist nitpick claiming the bare 'return' in apply_security_and_analysis is redundant. The author replied with sound rationale — removing it would reintroduce the S7682 violation this PR exists to fix, and bare 'return' correctly propagates the gh api exit status. Answered, intentionally not acted on; not blocking.
- Minor non-blocking nit: the test-file comment says constants are 'exported' by the production script — they are readonly and consumed via sourcing, not exported. Cosmetic only.
- Note: branch is BEHIND main (mergeable; squash-merge ruleset will handle it).
CI status
All validation checks green: SonarCloud, SonarCloud Code Analysis, CodeQL (actions + python), gitleaks secret scan, AgentShield, pr-auto-review, review/review, CodeRabbit. Dependency-audit ecosystem jobs skipped (no matching ecosystems). Two dev-lead orchestration relay jobs show CANCELLED — these are agent-dispatch plumbing, not code validation.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
codeant-ai
Bot
added
size:S
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
codeant-ai
Bot
added
size:S
|
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
Dev-Lead — fix-reviews (no-changes)Agent reasoning |
User description
Closes #349
Implemented by dev-lead agent. Please review.
Summary by CodeRabbit
Bug Fixes
Tests
CodeAnt-AI Description
Standardize repository settings checks and finish updates without unintended follow-up processing
What Changed
Impact
✅ Consistent missing-status reporting✅ No extra processing after settings updates✅ More reliable shell test runs💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.