Repository navigation
Add pre-PR checks to cut SonarCloud and Sourcery review rounds - #95
Merged
Merged
Conversation
Most PRs have needed several push-and-review rounds for findings that could have been caught before the PR existed. Add a "Before opening a PR" section to AGENTS.md and a review checklist built from the Sonar issues and Sourcery threads on every merged PR (#28-#94): the Sonar rules this repo keeps hitting and the kinds of defects Sourcery keeps finding. The SonarQube CLI's code analysis returns 403 on this plan, so the local Sonar step uses the Gradle scanner's branch analysis instead. Ignore the .sonar/ cache it leaves behind. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewer's GuideThis documentation-only PR introduces a repeatable pre-PR quality workflow: contributors use a repository-derived SonarCloud/Sourcery checklist, run a local short-lived SonarCloud analysis, obtain an independent cross-vendor review, and batch fixes before pushing. It also ignores the scanner cache generated by that workflow. Flow diagram for the pre-PR quality workflowflowchart LR
Change["Prepare change"] --> Checklist["Review diff against checklist"]
Checklist --> Sonar["Run local SonarCloud branch analysis"]
Sonar --> Issues["Read and fix open SonarCloud issues"]
Issues --> Review["Obtain independent cross-vendor review"]
Review --> Fixes["Fix real findings"]
Fixes --> Push["Push one batched review round"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 7 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="AGENTS.md" line_range="32" />
<code_context>
+- Local Sonar analysis: `./gradlew build sonar --no-parallel -Dsonar.branch.name=<branch>` (`--no-parallel` as in CI). It uploads a short-lived branch analysis to SonarCloud with the PR gate's rules and needs a token in `systemProp.sonar.token` in `~/.gradle/gradle.properties` (never read or print that file). Then read the branch's open issues (`https://sonarcloud.io/api/issues/search?componentKeys=jircd_jircd-server&branch=<branch>&resolved=false`) and fix them before opening the PR. The SonarQube CLI's `sonar analyze` only scans secrets on this plan; its code analysis returns 403.
</code_context>
<issue_to_address>
**Local scan exposes Sonar token**
When a maintainer runs the documented scan on a branch containing untrusted Gradle build logic while the Sonar token is configured, gradle executes the checked-out project’s build logic while `build sonar` runs; malicious build logic can read the configured `systemProp.sonar.token` and send it off-host, exposing the token and its SonarCloud permissions.
Warn against running the token-bearing scan on untrusted code, and use an isolated, least-privileged token only for trusted branches.
</issue_to_address>
### Comment 2
<location path="AGENTS.md" line_range="32" />
<code_context>
+- Local Sonar analysis: `./gradlew build sonar --no-parallel -Dsonar.branch.name=<branch>` (`--no-parallel` as in CI). It uploads a short-lived branch analysis to SonarCloud with the PR gate's rules and needs a token in `systemProp.sonar.token` in `~/.gradle/gradle.properties` (never read or print that file). Then read the branch's open issues (`https://sonarcloud.io/api/issues/search?componentKeys=jircd_jircd-server&branch=<branch>&resolved=false`) and fix them before opening the PR. The SonarQube CLI's `sonar analyze` only scans secrets on this plan; its code analysis returns 403.
</code_context>
<issue_to_address>
**Branch name executes shell commands**
When a maintainer substitutes an attacker-controlled Git branch name containing shell metacharacters for `<branch>` and runs the documented command, the shell expands command substitutions in the unquoted `-Dsonar.branch.name=<branch>` argument before Gradle starts, so a malicious branch name can execute arbitrary commands as the maintainer.
Pass the branch name as data through a safely expanded shell variable instead of interpolating it into the command text.
</issue_to_address>
### Comment 3
<location path=".github/REVIEW_CHECKLIST.md" line_range="48" />
<code_context>
+- **Validation edge cases.** Treat empty, null, negative, zero, duplicate and out-of-range values explicitly: an empty parameter, an explicit `null` YAML section, a null bind address (which must not mean "all interfaces"), a negative limit, a duplicate CAP offer, an empty CAP REQ. Validate every configuration source, including reload and constructors. [#37, #39, #61, #67, #79, #80, #87]
</code_context>
<issue_to_address>
**Null bind-address guidance contradicts the API**
When a contributor applies this checklist item to the public listener API, contributors applying this guidance to `ServerConfiguration.Listener` treat a null `bindAddress` as invalid or non-wildcard, contradicting its documented contract and changing which interfaces the listener binds.
Clarify that an explicitly null YAML value is rejected, while a null `Listener.bindAddress` retains its documented wildcard meaning.
</issue_to_address>
### Comment 4
<location path="AGENTS.md" line_range="32" />
<code_context>
+- Local Sonar analysis: `./gradlew build sonar --no-parallel -Dsonar.branch.name=<branch>` (`--no-parallel` as in CI). It uploads a short-lived branch analysis to SonarCloud with the PR gate's rules and needs a token in `systemProp.sonar.token` in `~/.gradle/gradle.properties` (never read or print that file). Then read the branch's open issues (`https://sonarcloud.io/api/issues/search?componentKeys=jircd_jircd-server&branch=<branch>&resolved=false`) and fix them before opening the PR. The SonarQube CLI's `sonar analyze` only scans secrets on this plan; its code analysis returns 403.
</code_context>
<issue_to_address>
**Sonar findings are missed**
When the branch has more than 100 open Sonar issues, the issues-search request returns only its first 100 results, so later findings are omitted and unresolved issues reach the PR.
Request and inspect every page of the issues-search results before treating the review as complete.
</issue_to_address>
### Comment 5
<location path=".github/REVIEW_CHECKLIST.md" line_range="3" />
<code_context>
+# Pre-PR review checklist
+
+SonarCloud and Sourcery review every PR. This list collects what they have flagged on this repo's merged PRs (#28–#94), so a change can be checked against it before the PR opens. Numbers in brackets are example PRs.
+
+## SonarCloud rules this repo keeps hitting
</code_context>
<issue_to_address>
**Forks lack Sonar analysis**
When a pull request comes from a fork, `build.yml` skips SonarCloud analysis when `SONAR_TOKEN` is unavailable, while the checklist promises analysis for every PR and `AGENTS.md` tells authors to wait for its report. The local alternative also requires a Sonar token, leaving fork contributors without a way to satisfy the stated check.
Revise the guidance to state when Sonar analysis is unavailable and provide an actionable alternative for fork contributors.
Also at `AGENTS.md:29-34`.
</issue_to_address>
### Comment 6
<location path=".github/REVIEW_CHECKLIST.md" line_range="54" />
<code_context>
+- **Public API compatibility.** Changing a public constructor or record breaks callers. The project is unreleased (0.1.0-SNAPSHOT), so breaks are allowed, but say so in the PR body to save a review thread. [#39, #52, #63, #82, #83, #84, #87]
</code_context>
<issue_to_address>
**API breaks skip required migration guidance**
When a change intentionally breaks a public API, the checklist says unreleased status allows public API breaks and asks only for a PR-body note, so a break can ship without the migration path and version bump the constitution requires; callers then lack the documented transition.
Align the checklist with the constitution by requiring a migration path and a version bump for public API breaks, even while the project is unreleased.
</issue_to_address>
### Comment 7
<location path="AGENTS.md" line_range="32" />
<code_context>
+- Local Sonar analysis: `./gradlew build sonar --no-parallel -Dsonar.branch.name=<branch>` (`--no-parallel` as in CI). It uploads a short-lived branch analysis to SonarCloud with the PR gate's rules and needs a token in `systemProp.sonar.token` in `~/.gradle/gradle.properties` (never read or print that file). Then read the branch's open issues (`https://sonarcloud.io/api/issues/search?componentKeys=jircd_jircd-server&branch=<branch>&resolved=false`) and fix them before opening the PR. The SonarQube CLI's `sonar analyze` only scans secrets on this plan; its code analysis returns 403.
</code_context>
<issue_to_address>
**Sonar results can be stale**
When the issues endpoint is queried before SonarCloud finishes processing the uploaded report, `./gradlew sonar` can finish after uploading the report while SonarCloud is still processing it. Querying `/api/issues/search` before processing completes returns the branch's previous results, so a clean or stale response can be mistaken for the new analysis.
Wait for the analysis task to complete successfully and verify its commit before querying branch issues.
</issue_to_address>Sourcery assessment
Approval pending. 7 findings to address first.
Blocking findings: AGENTS.md:32, AGENTS.md:32, .github/REVIEW_CHECKLIST.md:48, AGENTS.md:32, .github/REVIEW_CHECKLIST.md:3, and 2 more
Review follow-ups: - Run the token-bearing scan only on trusted worktree branches, since the build executes the branch's Gradle logic with the token present. - Take the branch name from `git branch --show-current`, quoted, instead of a hand-substituted placeholder. - Wait for SonarCloud to process the analysis (sonar.qualitygate.wait) so the issue query doesn't read the previous one, and page the query. - Say when SonarCloud doesn't run (no SONAR_TOKEN, fork PRs). - A missing bind address binds every interface where the contract says so, as ServerConfiguration.Listener documents; the checklist had it backwards. - Public API breaks follow the constitution's migration-path rule. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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
1 participant
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.
Why
Most PRs have needed several push-and-review rounds: SonarCloud raised 132 issues across 34 rules on merged PRs #28–#94, and Sourcery opened 133 review threads on #36–#94. Many fall into recurring patterns that can be caught before the PR exists. This adds a pre-PR step so each PR needs at most one follow-up round.
Changes
AGENTS.md: new "Before opening a PR" section../gradlew build sonar --no-parallel -Dsonar.branch.name=<branch>uploads a short-lived branch analysis with the PR gate's rules, then read the branch's open issues. The SonarQube CLI'ssonar analyzecan't be used for this: its code analysis (Vortex) returns 403 on this plan, so it only scans secrets..github/REVIEW_CHECKLIST.md: built from every merged PR's Sonar issues and Sourcery threads..gitignore: ignore the.sonar/cache the scanner leaves behind.The project is unreleased, so the checklist suggests stating intentional public API breaks in the PR body rather than keeping old constructors.
Testing
./gradlew build sonar --no-parallel -Dsonar.branch.name=docs/pre-pr-checkslocally: build passed, and SonarCloud listsdocs/pre-pr-checksas a short-lived branch with the gate OK and 0 issues.🤖 Generated with Claude Code
Summary by Sourcery
Introduce pre-PR quality checks to catch recurring SonarCloud and Sourcery findings before pull requests are opened.
New Features:
Enhancements:
Documentation:
Chores: