Skip to content

feat: implement issue #378 — SonarCloud: shell script hygiene - #393

Merged
don-petry merged 2 commits into
mainfrom
dev-lead/issue-378-20260714-1820
Jul 14, 2026
Merged

don-petry merged 2 commits into
mainfrom
dev-lead/issue-378-20260714-1820

Conversation

@don-petry

@don-petry don-petry commented Jul 14, 2026 •

Copy link
Copy Markdown
Contributor

Closes #378

Implemented by dev-lead agent. Please review.

Summary by CodeRabbit

  • Tests
    • Improved test assertion reliability by ensuring the helper reports success consistently when checks pass.

@don-petry
don-petry requested a review from a team as a code owner July 14, 2026 18:21
Copilot AI review requested due to automatic review settings July 14, 2026 18:21
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@coderabbitai

coderabbitai Bot commented Jul 14, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 090d05a9-def1-464f-a4e8-94b215f62cc3

📥 Commits

Reviewing files that changed from the base of the PR and between d0c0c2b and 8d17a67.

📒 Files selected for processing (1)
  • tests/scripts/apply-repo-settings.test.sh

📝 Walkthrough

Walkthrough

The shell test’s assert_contains helper now explicitly returns status 0 after its checks.

Changes

Shell test hygiene

Layer / File(s) Summary
Assertion helper return status
tests/scripts/apply-repo-settings.test.sh
assert_contains explicitly returns success after completing its checks.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Possibly related issues

  • petry-projects/TalkTerm#349 — Addresses the same SonarCloud requirement for explicit shell-function return statements.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Linked Issues check ❓ Inconclusive The change adds the explicit return required by shelldre:S7682 and stays within scope, but SonarCloud and CI results are not shown. Provide the SonarCloud and CI results confirming the finding count is zero and behavior remains unchanged.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the SonarCloud shell-script hygiene fix tied to issue #378.
Out of Scope Changes check ✅ Passed The only change is the targeted helper return, with no unrelated edits visible in the summary.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch dev-lead/issue-378-20260714-1820

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.

❤️ Share

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

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request adds an explicit return 0 to the end of the assert_contains function in tests/scripts/apply-repo-settings.test.sh. There are no review comments, and I have no feedback to provide.

@don-petry

Copy link
Copy Markdown
Contributor Author

Dev-Lead — review-changes (no-changes)

No changes were needed for this PR.

@don-petry
don-petry enabled auto-merge (squash) July 14, 2026 18:23

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Resolves SonarCloud rule shelldre:S7682 in the shell test harness by making the function’s return behavior explicit, without changing the test’s pass/fail logic (which is driven by the global fail flag and final exit "$fail").

Changes:

  • Add an explicit return 0 at the end of assert_contains() to satisfy SonarCloud S7682.

@donpetry-bot donpetry-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Automated review — APPROVED ✓

Risk: LOW
Reviewed commit: 1e3aca5fa3ae92edb4068a69f5f967d53ac2ac3f
Review mode: triage-approved (single reviewer)

Summary

One-line change adding an explicit 'return 0' at the end of assert_contains() in tests/scripts/apply-repo-settings.test.sh to resolve SonarCloud rule shelldre:S7682. Behavior-neutral: both branches of the function already terminated with zero-status commands (echo / fail=1), and test pass/fail is driven by the global 'fail' flag and final 'exit $fail'. Triage assessment (low-risk) confirmed.

Linked issue analysis

Closes #378 (SonarCloud: shell script hygiene). The issue reports exactly one MAJOR finding (shelldre:S7682) in tests/scripts/apply-repo-settings.test.sh, asking for an explicit return statement. This PR adds it as a real fix (no NOSONAR), with no behavior change. SonarCloud quality gate on this PR passed with 0 new issues, satisfying the acceptance criteria.

Findings

No findings.

  • Security: none — test-only shell script, no secrets, no auth/crypto/CI-workflow changes. Secret-scanning MCP tool unavailable in this run; gitleaks CI check passed.
  • Correctness: 'return 0' does not mask assertion failures — the failure branch sets the global 'fail' flag, and the script exits with 'exit $fail'. The function's return value was already 0 in both branches.
  • Maintainability: change matches existing style of the test harness.

CI status

All validation checks green: CI (TypeScript, Go, gitleaks), CodeQL (actions/go/javascript-typescript), SonarCloud quality gate passed (0 new issues, 0 security hotspots), dependency audits (govulncheck, pnpm audit) passed, AgentShield passed. Two CANCELLED entries (dev-lead dispatch/ci-relay) are agent-orchestration jobs from a superseded run — a later dev-lead dispatch run succeeded; not code CI. Branch is BEHIND main but MERGEABLE.


Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.

@sonarqubecloud

Copy link
Copy Markdown

@don-petry
don-petry merged commit c343975 into main Jul 14, 2026
23 of 24 checks passed
@don-petry
don-petry deleted the dev-lead/issue-378-20260714-1820 branch July 14, 2026 18:47
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.

SonarCloud: shell script hygiene

3 participants