fix(sonar): add --sonar-branch so attest sonar finds scans on non-main branches

[AlexKantor87]

Fixes#1116

What was wrong

GetProjectAnalysisFromRevision called api/project_analyses/search with only project. SonarQube defaults that endpoint to the project's main branch, so kosli attest sonar --sonar-project-key --sonar-revision could not find an analysis on any other branch. The search came back empty and the user was told the revision was wrong.

This is the same defect as #861, in the sibling code path that fix did not cover. #861's fix cannot simply be reused: sonarResults.Branch is only ever populated from a CE task response, and the revision path has no task — it is searching for the analysis by revision — so there is nothing to forward. The CLI cannot infer the branch here, so it has to be told.

What this does

• --sonar-branch supplies the branch on the project-key/revision path, and is forwarded to project_analyses/search. Unset, the request is byte-for-byte what it was, so this is backwards compatible.
• Mutually exclusive with --pull-request: a PR scan is not a branch scan, and the branch would otherwise be silently ignored.
• The "not found" error now says which branch was searched — or that only the main branch was, because no --sonar-branch was given. This is the part that cost the customer several days: an empty result from SonarQube is indistinguishable from a permissions problem, and their token and permissions were correct throughout.

The branch is set on sonarResults.Branch rather than threaded as a new argument, so both analyses lookups keep one mechanism, and branch now appears in the attestation payload for this path too. Where a CE task is found later in the flow it still wins — the flag supplies a name only, and the branch type can only come from the task.

The constructor decision

NewSonarConfig takes the branch as a ninth positional parameter rather than moving to an options struct. Nine positional params is overdue that refactor, but this fix is blocking a live customer, and changing an exported API for reasons unrelated to the defect belongs in its own PR where it cannot be conflated with a behaviour change. Noted as a follow-up.

Testing

Unit tests use the real responses from the customer instance in #1116 (release/uat, master never analysed). They cover: branch forwarded when set; nobranch parameter at all when unset or empty (asserting absence, not emptiness); the analysis found on a non-main branch when the main branch has none; a branch analysis with a different revision not returned; and the error text in both branch states. The two pre-existing #861 tests still pass.

Fuzzing.FuzzBranchParam pins two properties for any branch name: it round-trips to SonarQube byte-for-byte, and it cannot alter another query parameter — a branch called x&project=other must not change which project is searched. url.Values.Encode() already gives us both; the fuzz test is what keeps them. Verified by temporarily rewriting the URL build as string concatenation: seeds a&b=c, x&project=other, a#b and a?b all fail, so a later refactor that reintroduces the injection is caught. 1.1M executions clean over 60s.

Mutation testing with gremlins 0.6.0, scoped to internal/sonar:

	Killed	Lived	Not covered	Efficacy	Mutator coverage
main	43	23	46	65.15%	58.93%
this branch	70	33	14	67.96%	88.03%

Every mutant in the new branch handling is killed, including the one worth worrying about — negating Branch.Name != "" in the guard, which a happy-path-only test would miss. Mutator coverage rises 29 points because the new end-to-end test reaches code the package never executed before; the newly-visible survivors are pre-existing gaps in GetQualityGate and the CE wait loop, not new ones.

Two survivors adjacent to this change were killed: mutation testing showed a single-task fake let both halves of GetTaskID's match condition be negated unnoticed, so the test now serves a decoy task for a different analysis ahead of the real one and asserts the task ID, status and branch type that come back. Three survivors remain at sonar.go:615, in GetTaskID's pull-request match clause, which has no end-to-end test — a separate gap, noted rather than papered over.

Note the gremlins run needs --timeout-coefficient=60; at the default, most mutants time out spuriously on a loaded machine.

Not run locally:make test_integration. It needs KOSLI_API_TOKEN_PROD and KOSLI_SONAR_API_TOKEN, neither of which is available in my environment, and the setup script prompts interactively. Relying on CI for it. Two cmd/kosli golden strings are updated for the new error text — including case 113 in the SonarQube Server suite, whose golden was already stale (it predates an earlier reword of this message, and that suite only runs with SONARQUBE set).

make lint clean.

Customer impact

ADCB's pipeline currently works around this by listing branches and matching the revision itself, then attesting with kosli attest generic, which loses the native sonar attestation type. That workaround is marked REMOVE WHEN FIXED in their repo. They pin KOSLI_CLI_VERSION to major 2, so they pick this up automatically on release.

🤖 Generated with Claude Code

[claude]

Claude finished @AlexKantor87's task in 5m 41s —— View job

---

Review — round 3 (at 195f34ca)

• Static analysis (go build, go vet, go mod tidy)
• Read the current diff
• Verify prior review threads are resolved at HEAD
• Review new/changed code and tests
• Post review

Static analysis

Check	Result
go build ./...	clean
go vet ./...	clean (this compiles the test files too, so the new tests build)
go mod tidy + git diff --exit-code go.mod go.sum	clean

I could not run go test — the sandbox refused the command, so the sonar unit tests and the fuzz seeds are unverified by me. Everything below is from reading.

Prior threads — all resolved at HEAD

Thread	State
Missing empty-flag-audit-coverage.json entry	fixed (20cacfd9), plus spec.json and categories.json
Branch leaking into PR payloads	fixed — guard at sonar.go:219, results.Branch == nil asserted at sonar_branch_test.go:203
api/ce/activity branch scoping	answered with evidence (no branch param exists on that endpoint); correctly not forwarded
"Check the branch is correct" with no branch given	reworded, both goldens updated
Task erasing the user's branch	fixed at sonar.go:242, pinned by TestGetSonarResults_TaskWithoutBranch_KeepsSuppliedBranch
Warning landing in golden output ahead of case 27's error	fixed (3832c1c9), pinned by TestGetSonarResults_NoTaskThenFailure_DoesNotWarn
Case 20 coupled to SonarCloud's activity window	fixed (f44ff094) — analysisID != "" gate, pinned by TestGetSonarResults_PullRequestNoTask_DoesNotWarn; case 13 carries the note
analysesSearchURL duplication	fixed (201a2a95) — both lookups share it, GetProjectAnalysisFromAnalysisID is now a pure refactor
--pull-request help not naming the exclusion	fixed (195f34ca)

I checked the audit spec change specifically for the failure mode a new MuX'd flag could cause: flags_to_test now lists both pull-request and sonar-branch, but flag_values is consumed one flag at a time against a baseline that contains neither, so the probe never sends the forbidden pair. Same shape as the pre-existing sonar-revision/pull-request pair.

This round

Two nits, both posted inline, neither blocking:

• sonar.go:219 — the two branch guards spell the same-looking condition two ways (sc.pullRequest vs sonarResults.PullRequest). Both are correct for their position; only the second can be the results field. Spelling both the same way removes the invitation to unify them wrongly later.
• sonar.go:310 — the missing-task warning names task ID and status, but project.Name is also only set inside GetTaskID's match block, so the payload loses that too on this path. One more noun in the sentence, and an assertion in TestGetSonarResults_NoMatchingTask_Warns to hold it.

Nothing else found. In particular I checked and found no problem with: GetQualityGate (keyed on analysisId, branch-independent, so branch scoping cannot skew it); the Branch/Type JSON tags (both omitempty, so a name-only branch serialises cleanly); backwards compatibility (with the flag unset, analysesSearchURL emits exactly the pre-PR query on both paths, and TestGetProjectAnalysisFromRevision_EmptyBranchNameNotSent asserts absence rather than emptiness); and the new MuX ordering against case 30's golden (--sonar-revision defaults to the commit SHA without being Changed, so the revision/PR check passes first and the branch/PR check produces the expected message).

Worth saying

The analysesSearchURL extraction is the change that matters most beyond this bug: two copies of "scope the search to the branch" is what let #861 and #1116 be fixed six months apart, and the comment now records that reason at the one place it lives. The pairing of WithBranch and WithoutBranch against the same fake is good evidence discipline — it's what makes the passing test a claim about the fix rather than about a lenient stub. And the two corrections this round, on warning placement and on a comment that described intent rather than code, were both taken cleanly with the reasoning written down.
· branch sonar-branch-flag-1116

[claude]

Nit, consistency: this guard reads sc.pullRequest == "" while the sibling guard 23 lines below reads sonarResults.PullRequest == "". Both are correct — at this point sonarResults.PullRequest has only ever been assigned from sc.pullRequest (line 193-195), whereas after GetTaskID the results field can additionally have been set from the matched task, which is exactly why the later guard must read the results field. But two spellings of what looks like the same condition, in two blocks that exist for the same reason, invites someone to "unify" them in the wrong direction later.

Cheapest fix is to spell both the same way, since sonarResults.PullRequest is already populated here:

Suggested change

	ifsc.branch!=""&&sc.pullRequest=="" {
	ifsc.branch!=""&&sonarResults.PullRequest=="" {

[claude]

The warning slightly under-reports what the unmatched task costs. GetTaskID sets project.Name = task.ComponentName only inside the match block (sonar.go:662), so on this exact path the attestation also carries an empty project name — on the project-key path nothing else ever sets it. TestGetSonarResults_NoMatchingTask_Warns would show it: results.Project.Name == "" there, while the happy-path test gets "customer project".

Since the sentence exists to say what the payload is missing, naming the third thing costs nothing:

Suggested change

	logger.Warn("no SonarQube compute engine task was found for analysis %s of project %s: the attestation carries no task ID or scan status", analysisID, project.Key)
	logger.Warn("no SonarQube compute engine task was found for analysis %s of project %s: the attestation carries no task ID, scan status or project name", analysisID, project.Key)

Worth an assertion on results.Project.Name in that test either way, so the claim in the message stays tied to something.