feat: allow --sonar-revision with --pull-request to validate analysis commit by mbevc1 · Pull Request #1193 · kosli-dev/cli

mbevc1 · 2026-09-15T21:05:26Z

What does this PR change and why?

This PR allows users to specify both --pull-request and --sonar-revision together when attesting SonarQube pull request scans. Previously, these flags were mutually exclusive.

Why this matters: SonarQube keeps only the latest analysis of a pull request. If a scan for the current push hasn't finished processing, the API returns the previous push's analysis. This change lets users validate that the attested analysis is actually of the commit they expect by providing --sonar-revision. If the pull request's analysis is of a different commit, the command fails with a clear error message.

Changes

cmd/kosli/attestSonar.go
- Removed the mutual exclusivity check between --sonar-revision and --pull-request
- Added revisionExplicit field to track whether the user explicitly provided --sonar-revision
- When a pull request is specified without an explicit revision, the revision is cleared before passing to SonarConfig (allowing the API to return whatever analysis exists)
- Updated help text and added a new example showing the pull request + revision pattern
internal/sonar/sonar.go
- Added validation: when both pullRequest and revision are provided, verify that the returned analysis matches the expected revision
- Returns a descriptive error if the pull request's analysis is of a different commit, suggesting the scan may still be processing
cmd/kosli/attestSonar_test.go
- Added prRevision field to test suite to capture the actual commit analyzed by the PR scan
- Added getPRAnalysisRevision() helper to fetch the PR's analyzed commit from SonarCloud
- Updated test case #25: changed from expecting an error to a successful attestation when revision matches the PR's analyzed commit
- Added test case Use cucumber/ci-environment to handle multiple CI environments #31: validates that providing a mismatched revision fails with the expected error message
internal/sonar/sonar_test.go
- Added /api/ce/task endpoint handler to the fake SonarQube server
- Added TestGetSonarResults_PullRequestRevision() covering three scenarios:
  - Matching revision succeeds
  - Mismatched revision fails with appropriate error
  - No revision provided skips the check
cmd/kosli/root.go
- Updated sonarRevisionFlag help text to clarify the new behavior with pull requests

Fixes #1192

Checklist

Docs are autogenerated from CLI help — help text updated with new example and clarification
Helm chart — not applicable
Terraform provider — not applicable

claude · 2026-09-15T21:06:14Z

Claude finished @mbevc1's task in 1m 41s —— View job

Follow-up review: status of prior findings

Reviewed commit 1fc3bc48 ("fix(sonar): address review feedback on the pull-request revision check") — the only new commit since the last review.

Fetch existing review comments/findings
Diff BEFORE..AFTER — BEFORE was force-pushed away, so reviewed git show 1fc3bc48 directly (the only new commit)
Check status of each prior finding
Close resolved inline review threads — all 4 were already marked resolved on GitHub, no action needed
Flag any new bugs/regressions — none found
Post summary comment

Finding	Status
`internal/sonar/sonar.go` — comment scope, trailing space, one-space indent, redundant third `%s`	Resolved
`internal/sonar/sonar_test.go` — CE-task variant didn't pin the `sc.pullRequest == ""` case	Resolved — new `TestGetSonarResults_PullRequestRevision_DiscoveredWithoutFlag` covers it
`cmd/kosli/attestSonar.go` — comment wording/missing rationale	Resolved
`cmd/kosli/attestSonar_test.go` — test name overpromised	Resolved — renamed to "25 can attest a pull request scan alongside `--sonar-revision`"

No new bugs, security issues, or regressions found in the changed lines of 1fc3bc48. 0 new inline comments.

Note: I was unable to run go build/go test in this environment (commands required approval that wasn't available), so this review is based on static reading of the diff and surrounding code, not an executed test run.

Full details posted as a separate summary comment.

claude · 2026-09-15T21:15:01Z

Follow-up review: status of prior findings

Checked git diff for the pushed commits (BEFORE 0157bbfd was force-pushed away at 21:12Z; the resulting tree at c27ec6ab is byte-identical to what existed when these review comments were posted at 21:10Z — i.e. no code changed in response to the feedback below).

Location	Status
`internal/sonar/sonar.go:264` (comment wording, trailing space, triple `%s` interpolation, scope of the `sc.pullRequest != ""` guard)	Not addressed — line is unchanged from the reviewed version.
`internal/sonar/sonar_test.go:416` (CE-task variant still passes `revPullRequest`, doesn't pin the `sc.pullRequest == ""` case)	Not addressed — both sub-tests still pass `revPullRequest`.
`cmd/kosli/attestSonar.go:263` (comment wording, missing rationale for why non-PR path needs the default)	Not addressed — comment text unchanged.
`cmd/kosli/attestSonar_test.go:236` (test name overpromises what the smoke test checks)	Not addressed — name unchanged.

No new inline comments: since the pushed commits didn't change any lines relative to what was already reviewed, there's nothing new to flag within scope.

View job run

SonarQube holds one analysis per pull request, the latest one. Until the scan for the current push is processed, that is the previous push's analysis, and it was being attested against the new commit. When the caller names a revision alongside the pull request, the pull request's analysed commit must now match it. Nothing is checked when no revision is given, so existing callers are unaffected. Refs #1192

The two flags were mutually exclusive, so nothing could ask for the pull request's analysed commit to be checked. They are now accepted together, and the pull request's analysis must then be of the given revision. Only a revision the user set (flag, env var or config) is passed on for a pull-request scan. The flag defaults to the CI commit, which nobody asked to have checked, so existing pull-request attestations are unchanged. Refs #1192

Refs #1192

- Note in the comment that only a revision passed alongside --pull-request is checked; on the report-task.txt/--sonar-ce-task-url paths without that flag, sc.revision is the defaulted CI commit, not an expectation to verify. - Pin that gap with a dedicated test: when the pull request is discovered from the CE task response rather than --pull-request, the check does not fire. - Fix the error message's stray trailing space and one-space indent, and drop the redundant third interpolation of the expected revision. - Rename test case 25 to not overstate what it checks: getPRAnalysisRevision reads the expected SHA from the same endpoint the production code reads the actual SHA from, so it is a smoke test for the flag combination, not an independent assertion on the revision.

claude · 2026-09-15T21:25:16Z

Follow-up review: status of prior findings

Reviewed commit 1fc3bc48 ("fix(sonar): address review feedback on the pull-request revision check"), the only new commit since the last review.

Finding	Status
`internal/sonar/sonar.go` — comment scope, trailing space, one-space indent, redundant third `%s`	Resolved — comment now notes the check only applies to a revision passed alongside `--pull-request`; message whitespace and the extra interpolation are fixed.
`internal/sonar/sonar_test.go` — CE-task variant didn't pin the `sc.pullRequest == ""` case	Resolved — new `TestGetSonarResults_PullRequestRevision_DiscoveredWithoutFlag` covers the case where the PR is discovered from the CE task response and the check correctly does not fire.
`cmd/kosli/attestSonar.go` — comment wording/missing rationale	Resolved — comment now explains why the non-PR path keeps the defaulted revision.
`cmd/kosli/attestSonar_test.go` — test name overpromised	Resolved — renamed to "25 can attest a pull request scan alongside `--sonar-revision`".

No new bugs, security issues, or regressions found in the changed lines of 1fc3bc48. All four previously opened inline threads are already marked resolved on GitHub — 0 new inline comments.

View job run

@AlexKantor87

kosli-cli 2.42.0 Created-by: HarmonybrewBot Commit-by: HarmonybrewBot Merged-by: HarmonybrewBot Description: Created by `brew bump` --- Created with `brew bump-formula-pr`.<details> <summary>release notes</summary> <pre># New features - `kosli snapshot s3`: added `--download-concurrency` and `--download-budget` flags to control parallel downloads and peak temporary disk usage when fingerprinting S3 buckets. - `kosli attest sonar`: `--sonar-revision` can now be used together with `--pull-request`; when provided, the command fails if the pull request's latest analysis is not of the specified revision. # Improvements - `kosli snapshot s3`: S3 object keys are no longer used as local file names; each object is downloaded to a temporary file, hashed, and removed, so any valid S3 key can be fingerprinted on any OS. - `kosli get flow` and `kosli list flows`: the `Visibility` field is no longer shown in table output (it is a legacy field with no effect on access). - `kosli list flows`: fixed a panic when a flow has no tags.  ## What's Changed * ci(kosli): attest SBOMs with the sbom type by @AlexKantor87 in kosli-dev/cli#1186 * feat(snapshot s3): fingerprint S3 buckets from a virtual tree by @mbevc1 in kosli-dev/cli#1180 * fix(ci): wait for staging to settle before picking a server image by @AlexKantor87 in kosli-dev/cli#1190 * feat(snapshot s3): download objects in parallel within a count and byte budget by @mbevc1 in kosli-dev/cli#1191 * feat: allow --sonar-revision with --pull-request to validate analysis commit by @mbevc1 in kosli-dev/cli#1193 * chore(list-flows): drop the VISIBILITY column from the table output by @dangrondahl in kosli-dev/cli#1187 * chore(get-flow): drop the Visibility row from the table output by @dangrondahl in kosli-dev/cli#1188 * fix(snapshot k8s): do not abort the snapshot when a Running pod has an empty imageID by @dangrondahl in kosli-dev/cli#1195 * test(fingerprint): cover OCI capture cleanliness against a fake registry by @mbevc1 in kosli-dev/cli#1197 **Full Changelog**: kosli-dev/cli@v2.41.0...v2.42.0 </pre> <p>View the full release notes at <a href="https://github.com/kosli-dev/cli/releases/tag/v2.42.0">https://github.com/kosli-dev/cli/releases/tag/v2.42.0</a>.</p> </details> <hr> See merge request: Harmonybrew/homebrew-core!20398

github-actions Bot added feat go Pull requests that update go code labels Sep 15, 2026

claude Bot reviewed Sep 15, 2026

View reviewed changes

Comment thread internal/sonar/sonar.go

claude Bot reviewed Sep 15, 2026

View reviewed changes

Comment thread internal/sonar/sonar_test.go

claude Bot reviewed Sep 15, 2026

View reviewed changes

Comment thread cmd/kosli/attestSonar.go Outdated

claude Bot reviewed Sep 15, 2026

View reviewed changes

Comment thread cmd/kosli/attestSonar_test.go Outdated

mbevc1 force-pushed the 20260915_fix_sonar branch from 0157bbf to c27ec6a Compare September 15, 2026 21:12

mbevc1 added 4 commits September 15, 2026 22:22

docs(attest sonar): describe --sonar-revision with --pull-request

bba9718

Refs #1192

mbevc1 force-pushed the 20260915_fix_sonar branch from c0181f4 to 1fc3bc4 Compare September 15, 2026 21:22

mbevc1 enabled auto-merge (squash) September 15, 2026 21:26

AlexKantor87 approved these changes Sep 16, 2026

View reviewed changes

mbevc1 merged commit 0928559 into main Sep 16, 2026
23 checks passed

mbevc1 deleted the 20260915_fix_sonar branch September 16, 2026 04:44

BrewTestBot mentioned this pull request Sep 16, 2026

kosli-cli 2.42.0 Homebrew/homebrew-core#311437

Merged

Navigation Menu

feat: allow --sonar-revision with --pull-request to validate analysis commit - #1193

feat: allow --sonar-revision with --pull-request to validate analysis commit#1193
mbevc1 merged 4 commits into
mainfrom
20260915_fix_sonar

mbevc1 commented Sep 15, 2026 •

edited

Loading

Uh oh!

claude Bot commented Sep 15, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot commented Sep 15, 2026

Uh oh!

claude Bot commented Sep 15, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mbevc1 commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR change and why?

Changes

Checklist

Uh oh!

claude Bot commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Follow-up review: status of prior findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot commented Sep 15, 2026

Follow-up review: status of prior findings

Uh oh!

claude Bot commented Sep 15, 2026

Follow-up review: status of prior findings

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mbevc1 commented Sep 15, 2026 •

edited

Loading

claude Bot commented Sep 15, 2026 •

edited

Loading