feat(sonarcloud): decouple sonar.python.version from the build version - #291
Conversation
``-Dsonar.python.version`` was wired to the ``python-version`` input, which also feeds ``actions/setup-python``. Because setup-python accepts exactly one version, a caller could never declare a support RANGE to the analyzer, only the single version CI happens to build with. That is not cosmetic. Sonar gates version-specific rules on ALL declared versions: ``PythonVersionUtils.areSourcePythonVersionsGreaterOrEqualThan`` is an ``allMatch`` over the parsed set. Declaring one modern version therefore opens every version gate, and the analyzer raises rules for syntax the project cannot legally use. Observed on cyo-adventure: 5 PEP 695 findings (``python:S6794`` x4, ``python:S6796`` x1) against a codebase whose ``requires-python`` is ``>=3.11`` and whose compatibility matrix still runs a 3.11 leg, where ``type X = ...`` is a SyntaxError. Both checks call that helper with ``Version.V_312``. The findings were on their way to being accepted by hand as false positives; they are neither false nor positives, just the answer to a question nobody meant to ask. Adds an optional ``sonar-python-version`` input for the source support range (``'3.11,3.12,3.13,3.14'``), defaulting to empty and falling back to ``python-version``, so every existing caller is byte-identical. Callers whose ``requires-python`` is wider than their CI version should set it. - YAML parses; the new input registers with ``default: ''``, ``required: false`` - actionlint: no new findings at the changed lines (the SC2086/SC2129 reports are pre-existing, in ``run:`` blocks this commit does not touch) - The value lands in the scan action's ``with: args:``, not a ``run:`` block, so it introduces no shell interpolation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 35 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe reusable workflow separates the Python interpreter used for setup and scanning from the Python versions supported by the source. It validates the supported-version list, passes it to SonarCloud, and documents the configuration. ChangesSonarCloud Python version configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant WorkflowInput
participant VersionResolution
participant SonarCloudScan
WorkflowInput->>VersionResolution: provide Python version inputs
VersionResolution->>VersionResolution: resolve and validate source versions
VersionResolution->>SonarCloudScan: pass sonar.python.version
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the reusable SonarCloud workflow to allow callers to declare the Python version range their source code supports independently from the single Python version used to run CI and the analysis. This avoids SonarPython enabling version-gated rules for syntax not actually supported by the project.
Changes:
- Added an optional
sonar-python-versioninput (default empty) to represent the supported source version range. - Updated the Sonar scan arguments so
-Dsonar.python.versionusessonar-python-versionwhen provided, otherwise falls back topython-version.
![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 @.github/workflows/python-sonarcloud.yml:
- Around line 71-90: Update the workflow documentation for the inputs around
python-version: describe python-version as the single version used by
actions/setup-python, document the sonar-python-version comma-separated
source-version input, and state that an empty sonar-python-version falls back to
python-version.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f2eb86f5-18c9-49c5-845e-0aa620a25882
📒 Files selected for processing (1)
.github/workflows/python-sonarcloud.yml
PR ReviewPREMISE QUESTION: the premise is sound and the lever is the right one, but the change ships without its caller-facing docs, leaves an identically-defective sibling template unfixed, and has one unguarded input-format footgun. Gates: 34 CI checks all SUCCESS or SKIPPED, SonarCloud quality gate passed (0 new issues, 0 hotspots), Qlty green. Copilot: overview only, zero findings. CodeRabbit: 1 actionable comment (finding 1 below). Findings: 0 Critical, 3 Important, 4 Suggested, 3 Informational. Verified cleanThe load-bearing technical claims check out against primary sources, worth stating plainly:
Important1. The caller-facing inputs table omits the new input and still carries the description string this PR replaced. 2. A value in SonarSource's own documented format silently breaks the scan. 3. The gallery starter workflow hardcodes the same value, so this is an incomplete remediation of its own stated problem. Suggested and Informational (summary)
Recommended action
SonarQube: 0 issues and 0 hotspots queued for auto-fix. Copilot review was requested by the org ruleset and submitted with no findings. 🤖 Generated with Claude Code |
The scan step passes sonar.python.version inside a folded block scalar (args: >), where YAML joins lines with spaces and the scanner then splits the result on whitespace. A value containing a space became two scanner arguments and the version list was silently truncated. SonarSource's own documentation writes the property as "3.11, 3.12", so a caller copying the vendor format hit that misparse with no validation and no warning. Resolve the value in a bash step instead: strip whitespace so both spellings work, then reject anything that is not a comma-separated list of MAJOR.MINOR versions. The regex doubles as the GITHUB_OUTPUT shape guard (PR #234), since a newline in a caller value could otherwise append extra key=value lines and forge unrelated step outputs. Both inputs reach the run: body through env:, never through template expansion. Also narrow the description example from 3.14 to 3.13 so it matches this repo's python-versions:supported marker and cannot trip the policy gate in scripts/check-python-versions.sh. Verified: 11 guard cases (vendor spacing, messy whitespace, empty fallback, both-empty, patch version, range operator, command substitution, trailing comma, newline injection) behave as designed; actionlint rc=0; yamllint adds no new warnings; qlty clean.
The inputs table still described python-version as "Python version for analysis" and did not list sonar-python-version at all, so callers had no way to learn the input exists or why they would set it. CodeRabbit flagged the same gap on PR #291. Add the missing row, retitle python-version to say it is the single actions/setup-python version, and add a "Declaring supported source versions" section covering the allMatch gating rule that makes the input necessary, the accepted format, and the inverse hazard: declaring versions below the real requires-python floor silently suppresses rules that should fire. Also correct the sonar-project.properties example, which showed a single version and did not mention that the workflow always passes -Dsonar.python.version on the command line, so a sonar.python.version key in that file is ignored.
The standalone starter workflow invokes the scanner directly and hardcodes -Dsonar.python.version=3.12, so it carries the same defect the reusable workflow just fixed: adopters whose requires-python is wider than 3.12 get version-gated rules raised against syntax they cannot use. The template has no workflow_call inputs to widen, so document the adjustment at the value itself, including the folded-scalar constraint that each value stay space-free.
Fixes appliedThree signed commits pushed (
Why the guard step exists
The step now normalizes whitespace (both spellings work) and rejects anything that is not a comma-separated list of Verification
Two items deliberately left out
Generated with Claude Code |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- PR diff summary ---'
git diff --stat origin/main...HEAD
echo
echo '--- Changed documentation sections ---'
sed -n '50,95p' docs/workflows/python-sonarcloud.md
sed -n '185,245p' docs/workflows/python-sonarcloud.md
echo
echo '--- Changed workflow section ---'
sed -n '390,465p' .github/workflows/python-sonarcloud.yml
echo
echo '--- Relevant diff ---'
git diff --unified=35 origin/main...HEAD -- \
docs/workflows/python-sonarcloud.md \
.github/workflows/python-sonarcloud.yml \
workflow-templates/python-sonarcloud.ymlLength of output: 32755
The workflow also normalizes documented comma spacing, validates the version list before it writes This addresses my caller-documentation finding. ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
docs/workflows/python-sonarcloud.md:118
- Docs describe stripping only surrounding whitespace, but the workflow implementation strips all whitespace from the value. Update the wording to reflect the actual behavior so callers understand that any spaces, tabs, or newlines are removed before validation.
**Format.** Comma-separated `MAJOR.MINOR` values. Surrounding whitespace is tolerated and
stripped, so both `3.11,3.12` and SonarSource's own documented `3.11, 3.12` spacing work.
.github/workflows/python-sonarcloud.yml:82
- The description says whitespace is stripped only around commas, but the resolver step removes all whitespace characters (VERSION="${RAW//[[:space:]]/}"). Consider updating the input description to match the actual behavior.
Whitespace around commas is stripped, so SonarSource's own documented
"3.11, 3.12" spacing is accepted; any other shape fails the job with an
explicit message instead of reaching the scanner malformed.
…commas Copilot's re-review flagged that the docs promised whitespace was stripped "around commas" while the resolver stripped whitespace everywhere. The claim was wrong in the caller's favour: stripping first meant "3 .11" was silently normalized to "3.11" instead of being reported as the typo it is, so a mistyped value could quietly analyze against a version the caller never declared. Validate before stripping, with whitespace permitted only where it carries no meaning: around the commas and at the ends. SonarSource's documented "3.11, 3.12" spacing still passes; "3 .11", "3. 11", "3.1 1", and a bare space used as a separator now fail with the same explicit message as any other malformed shape. The GITHUB_OUTPUT shape guard is unaffected. The accepted set is still only digits, dots, commas, and whitespace, and stripping the whitespace leaves no newline, no '=', and no shell metacharacter. Verified: 18 cases, extracted from the workflow body itself so the test cannot drift from the shipped code. 8 pass cases (including tab and newline as inter-comma whitespace) and 10 reject cases (the 4 new whitespace-inside- version cases, patch version, range operator, command substitution, trailing comma, both-empty, newline injection). actionlint findings are byte-identical to base at 46; yamllint line-length warnings unchanged at 5; qlty clean; check-python-versions.sh rc=0.
Follow-up: Copilot's suppressed finding, and a correctionCopilot was right, and the fix went into the implementationCopilot's re-review reported "no new comments" but listed 2 suppressed ones. Both were the That gap was in the caller's favour in the worst way. Because the old guard stripped first Resolved in if [[ ! "$RAW" =~ ^[[:space:]]*[0-9]+\.[0-9]+([[:space:]]*,[[:space:]]*[0-9]+\.[0-9]+)*[[:space:]]*$ ]]; then
...
fi
VERSION="${RAW//[[:space:]]/}"Newly rejected, previously silently normalized: The Correction: my earlier actionlint claim was wrongAn earlier comment on this PR states Re-measured properly, by diffing normalized output instead of trusting an exit code:
Byte-identical after stripping line numbers, and none of the 46 falls in the new step's Verification on the new head
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/workflows/python-sonarcloud.yml:438
- When
sonar-python-versionis empty, this step validatespython-versionas MAJOR.MINOR only. That makespython-version: '3.12.1'(valid foractions/setup-python) fail the job even though setup succeeds. If the intent is backward compatibility andpython-versioncontinues to feed setup-python, consider normalizing an optional patch component when falling back topython-version, while keeping strict validation for the explicitsonar-python-versionlist.
if [ -n "$SONAR_PYTHON_VERSION" ]; then
RAW="$SONAR_PYTHON_VERSION"
FROM_INPUT="sonar-python-version"
else
RAW="$PYTHON_VERSION"
FROM_INPUT="python-version (sonar-python-version was empty)"
fi
Copilot caught a backward-compatibility break I introduced in c150019. The strict MAJOR.MINOR list validation was applied to both inputs, but the two have different contracts. sonar-python-version is a sonar.python.version list and nothing else. python-version feeds actions/setup-python, whose accepted syntax is far wider: '3.12.1', '3.13.0-rc.1', '>=3.11', and 'pypy3.10' are all legal there and all analyzed fine before this step existed. Validating them against a Sonar list shape hard-failed the job for callers who never opted into sonar.python.version at all. Split the paths. The explicit input keeps strict validation. The fallback extracts the leading MAJOR.MINOR, since that is all sonar.python.version accepts, and only fails when no minor version can be derived ('3.x', 'pypy', empty), where the message points at sonar-python-version rather than guessing a version the caller never declared. A narrowed value is logged as narrowed, so the normalization is never silent, and the fallback now also emits a notice that a single build version may be too narrow. The GITHUB_OUTPUT shape guard survives the split: both branches assign VERSION from digits and dots only, either a whitespace-stripped match of the strict list regex or two numeric BASH_REMATCH groups. Verified: 26 cases, extracted from the workflow body so the test cannot drift from shipped code. 16 explicit-input cases (6 accept including vendor spacing, tab and newline; 10 reject including the injection and whitespace-inside-version cases) and 10 fallback cases (6 accept including the 3.12.1 regression Copilot found, 4 reject). Rejects write nothing to GITHUB_OUTPUT. pre-commit clean, qlty clean, check-python-versions.sh rc=0, actionlint 46 findings matching base with none in the step's line range.
Copilot's patch-version finding was right, and it was a break I introducedCopilot's latest review again reads "no new comments" with 1 suppressed, and the suppressed
Correct. I applied one validation shape to two inputs that have different contracts. Fixed in
So The Verification26 cases, extracted from the workflow body by script so the test cannot drift from shipped Note on the previous run
|
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.github/workflows/python-sonarcloud.yml:448
sonar-python-versionis documented as having whitespace at the ends stripped, but the script treats a whitespace-only value as non-empty and then fails validation. This makes an accidental value like' 'behave differently than empty, even though stripping would yield empty and should fall back topython-version. Consider treating the whitespace-stripped value as the emptiness check, while still validating the original string to keep rejecting whitespace inside a version ("3 .11").
if [ -n "$SONAR_PYTHON_VERSION" ]; then
# The explicit input is a sonar.python.version list and nothing else, so hold it to
# exactly that shape. Validate BEFORE stripping, so whitespace is tolerated only
# where it is meaningless: around the commas and at the ends. That accepts
# SonarSource's documented "3.11, 3.12" spacing while still rejecting whitespace
.github/workflows/python-sonarcloud.yml:463
- The PR description claims the default empty
sonar-python-versionpath produces a byte-identical scanner invocation by passinginputs.python-versionthrough unchanged. The implementation now normalizespython-versionto a leadingMAJOR.MINORforsonar.python.version, which changes the argument for callers using values like3.12.1,>=3.11, orpypy3.10(even though that normalization is likely desirable). Consider updating the PR description to reflect the normalization behavior.
if [[ "$PYTHON_VERSION" =~ ([0-9]+)\.([0-9]+) ]]; then
VERSION="${BASH_REMATCH[1]}.${BASH_REMATCH[2]}"
else
sonar.python.version was 3.12, the single version CI builds with, while requires-python is ">=3.11,<3.15". SonarPython gates version-specific rules on ALL declared versions (PythonVersionUtils.areSourcePythonVersionsGreaterOrEqualThan is allMatch), so declaring only 3.12 made every 3.12-gated rule fire against code that must still run on 3.11. That produced 2 live false positives today: S6794 and S6796 urging PEP 695 "type X = ..." aliases, which are a SyntaxError on 3.11. Declaring the full range makes allMatch(>=3.12) false, so the rules correctly stay silent until the floor moves. Sets the new sonar-python-version input (ByronWilliamsCPA/.github#291) and bumps the uses: pin to the commit that introduces it, since the input does not exist in the previously pinned revision and passing an undefined input fails the reusable workflow at startup. Also syncs sonar-project.properties. CI overrides that key via -D, so it does not affect the pipeline, but SonarLint in the IDE reads it and would otherwise keep raising the same false positives locally that CI no longer reports. python-version stays 3.12: it feeds actions/setup-python and is the version the project builds and runs tests with, which is a separate concern from the versions the source must remain compatible with. Pre-commit: all hooks pass except pydoclint, which fails identically on untouched main (DOC105/109/110 in fuzz/*.py). This change touches no Python, so it is scoped out with SKIP=pydoclint rather than bypassed.
sonar.python.version was 3.12, the single version CI builds with, while requires-python is ">=3.11,<3.15". SonarPython gates version-specific rules on ALL declared versions (PythonVersionUtils.areSourcePythonVersionsGreaterOrEqualThan is allMatch), so declaring only 3.12 made every 3.12-gated rule fire against code that must still run on 3.11. That produced 2 live false positives today: S6794 and S6796 urging PEP 695 "type X = ..." aliases, which are a SyntaxError on 3.11. Declaring the full range makes allMatch(>=3.12) false, so the rules correctly stay silent until the floor moves. Sets the new sonar-python-version input (ByronWilliamsCPA/.github#291) and bumps the uses: pin to the commit that introduces it, since the input does not exist in the previously pinned revision and passing an undefined input fails the reusable workflow at startup. Also syncs sonar-project.properties. CI overrides that key via -D, so it does not affect the pipeline, but SonarLint in the IDE reads it and would otherwise keep raising the same false positives locally that CI no longer reports. python-version stays 3.12: it feeds actions/setup-python and is the version the project builds and runs tests with, which is a separate concern from the versions the source must remain compatible with. Pre-commit: all hooks pass except pydoclint, which fails identically on untouched main (DOC105/109/110 in fuzz/*.py). This change touches no Python, so it is scoped out with SKIP=pydoclint rather than bypassed.
Problem
-Dsonar.python.versionwas wired to thepython-versioninput, which also feedsactions/setup-python. setup-python accepts exactly one version, so a caller could never declare a support range to the analyzer, only the single version CI happens to build with.That is not cosmetic. Sonar gates version-specific rules on all declared versions:
Declaring one modern version therefore opens every version gate, and the analyzer raises rules for syntax the project cannot legally use.
Observed impact
On
cyo-adventure, 5 PEP 695 findings (python:S6794x4,python:S6796x1) against a codebase whoserequires-pythonis>=3.11and whose compatibility matrix still runs a 3.11 leg, wheretype X = ...is aSyntaxError. Both checks (TypeAliasAnnotationCheck,GenericFunctionTypeParameterCheck) call that helper withVersion.V_312, and both rules carry the documented exception:Those 5 were on their way to being accepted by hand as false positives. They are neither false nor positives, just the answer to a question nobody meant to ask.
This is latent for every org repo whose
requires-pythonis wider than its CI Python version, and it distorts every version-gated rule, not only PEP 695.Change
Adds an optional
sonar-python-versioninput for the source support range, defaulting to empty and falling back topython-version:-Dsonar.python.version=${{ inputs.sonar-python-version != '' && inputs.sonar-python-version || inputs.python-version }}Callers whose
requires-pythonis wider than their CI version should set it, e.g.sonar-python-version: '3.11,3.12,3.13,3.14'.Compatibility
Backward compatible. With the empty default the expression resolves to
inputs.python-version, so every existing caller produces a byte-identical scanner invocation. No caller is required to change.Verification
default: '',required: falseactionlint: no new findings at the changed lines (the SC2086/SC2129 reports are pre-existing, inrun:blocks this PR does not touch)with: args:, not arun:block, so it introduces no shell interpolationGenerated with Claude Code
Summary by CodeRabbit
Enhancements
Documentation