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

[AlexKantor87]

Fixes#1116. Replaces #1117, which grew well beyond the defect and was closed.

The defect

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.

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

The change

13 lines of production code:

• --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.
• Both analyses lookups now build that URL through one analysesSearchURL. Leaving the two copies in place is exactly how kosli attest sonar fails for non-main branches: missing branch param in project_analyses/search #861 was fixed on one path and left broken on the other for six months.
• Mutually exclusive with --pull-request, and --pull-request's help text names the new exclusion.
• The not-found error says what was searched. An empty result from SonarQube is indistinguishable from a permissions problem — that is what cost this customer several days while their token and permissions were correct throughout.

Data files: empty-flag-audit-coverage.json is regenerated (CI enforces it); the audit's spec.json gains the flag because audit.py refuses to start until every command-and-flag pair is covered; categories.json gains it as identity.

Deliberately not in this PR

The previous attempt included these; each was reviewed and dropped:

• Re-instating the user's branch after GetTaskID. It cannot affect the defect — the lookup has already returned, and GetQualityGate keys on analysisId. It only changes payload semantics in the sub-case where the matched CE task reports no branch. If the flag should be authoritative for the payload, that deserves its own issue.
  • Correction: an earlier version of this description also argued that re-instating would produce a shape that has never existed (branch.name with no branch.type). That argument was wrong, and review caught it. Setting the branch before the lookup — which this PR does, because that is what scopes the search — produces exactly that shape whenever no CE task matches, since GetTaskID only rewrites the branch inside its match block and type is omitempty. That is reachable: api/ce/activity needs administrative permission, so an ordinary CI token gets an error body that decodes to zero tasks. The behaviour is kept — the branch the user supplied is better payload data than nothing, and only a task can supply a type — and is now pinned by TestGetSonarResults_NoMatchingTask_KeepsSuppliedBranch so it is a deliberate choice rather than a side effect. The two remaining reasons for dropping the re-instate block stand on their own.
• A warning that --sonar-branch is ignored on the report-task.txt / --sonar-ce-task-url paths. It fires on the ordinary metadata path for anyone who sets KOSLI_SONAR_BRANCH in their environment, on a path the defect never touched, and logger.Warn output is compared as part of golden test output (cmd/kosli/testHelpers.go:69-71,112), which makes it a trap for the next test to pass the flag there. Note the related decision that is worth keeping: no mutual exclusion against --sonar-ce-task-url, because case 14 asserts that project-key-path flags are silently ignored when scanner metadata exists, so erroring on only the new flag would be inconsistent with its three siblings.
• A fuzz test. It was the only func Fuzz in the repository, and it exercised url.Values.Encode() — the standard library — on the premise that someone might later refactor to string concatenation. The two properties worth keeping are now four rows of an ordinary table test.

api/ce/activity is deliberately not scoped by branch: SonarQube's API takes no branch parameter there (it exists on api/ce/analysis_status), and its tasks carry branch per row, so it spans branches by design.

Tests

In internal/sonar/sonar_test.go, alongside the existing #861 pair and following the package's conventions (httptest, raw assertions, standalone functions):

• _PassesBranch — the branch is forwarded, the analysis is found
• _BranchParam — table: value arrives verbatim; an unset and an empty branch send no parameter at all rather than an empty one; a branch named x&project=other cannot alter the project parameter
• _NotFoundError — the error points at the flag when no branch was given, and names the branch when one was
• TestGetSonarResults_ProjectKeyPath_WithBranch / _WithoutBranch — end-to-end, and the failing half is what makes the passing half evidence about SonarQube rather than about a lenient fake
• _BranchIgnoredForPullRequest — no branch alongside a pull request
• The activity fake serves a decoy task first, so the CE task must be selected by analysis ID rather than by being the only one present
• CLI: case 30 for the mutual exclusion; goldens 16 and 113 updated for the new error text

Verified locally beyond the committed tests, with nothing added to the repo for it: fuzzing over the branch-name encoding (320k executions clean, then deleted) and mutation testing on internal/sonar with gremlins — every mutant on the new guards killed, package efficacy 65.2% → 68.6% and mutator coverage 58.9% → 90.5% against main. make lint clean.

Case 113's golden was already stale on main — it predates a reword of that error, and its suite only runs when SONARQUBE is set, so nothing caught the drift. Corrected here; flagging it so it does not read as scope creep.

Accepted with eyes open

MuXRequiredFlags keys on flag.Changed, and config/env binding applies values through cmd.Flags().Set (cmd/kosli/root.go:672), which marks a flag changed. So KOSLI_SONAR_BRANCH exported across a pipeline makes every --pull-request attestation in it fail, even where the flag was never typed. Kept as-is deliberately: it is byte-identical in shape to the --sonar-revision/--pull-request exclusion two lines above, and a pipeline attesting both branch and PR scans already hits that one via KOSLI_SONAR_REVISION. Consistency with the sibling beats a special case for the new flag.

TestGetSonarResults_BranchIgnoredForPullRequest currently proves the pull-request guard only by accident: the fake's ce/activity tasks carry no pullRequest, so nothing matches and GetTaskID never touches the branch. A more realistic fixture would make GetTaskID nil the branch itself, at which point the test would pass with the guard deleted. Known and accepted; fixing it properly means a new fixture and new assertions.

Follow-up worth its own issue

api/ce/activity requires administrative permission, so an ordinary Execute-Analysis + Browse CI token gets a 403. ActivityResponse is the only response struct in this file with no Errors field, and GetTaskID only returns an error when JSON decoding fails — so {"errors":[...]} decodes cleanly to zero tasks and every project-key run silently publishes with an empty taskId, status and project name. Strictly pre-existing and out of scope here, but it should not be lost.

Customer impact

ADCB's pipeline currently lists branches and matches the revision itself, then attests with kosli attest generic, losing 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 6s —— View job

---

Review — PR #1119 (--sonar-branch)

• Static analysis (go build, go vet, go mod tidy)
• Review production diff (internal/sonar, cmd/kosli)
• Review new tests from f18a14d8 / c013aff6
• Post review

Static analysis

Check	Result
go build ./...	clean
go vet ./...	clean (type-checks the new test files too)
go mod tidy → git status go.mod go.sum	no diff
go test ./internal/sonar	not run — blocked by the sandbox permission prompt in this environment, not by a failure. The package tests are self-contained (httptest, no local Kosli server), so they should be runnable in CI.

Verified against the code, not the description

The three commits since the last round hold up under checking:

• c013aff6 — I traced the claim end to end. Branch.Type is json:"type,omitempty" (sonar.go:50); GetTaskID writes Branch only inside if matched (sonar.go:615-628) and returns nil on no match. So {Name: …, Type: ""} survives, and TestGetSonarResults_NoMatchingTask_KeepsSuppliedBranch now pins both halves — TaskID == "" proving no task matched, then name-stands/type-empty. The omitMatchingTask switch keeps the decoy in place, so the fixture still exercises selection-by-analysis-ID.
• f18a14d8 — both tables are t.Run subtests with defer server.Close() inside the closure and the capture vars still scoped per-case. The injection row can no longer be disarmed by an earlier Fatal.
• 124e0d97 — ps=500 is a strict superset at offset 0, both loops break on first match (sonar.go:430-437, 448-454), and project_analyses/search is date-DESC, so the newest matching analysis is still the one selected on either path. No behaviour change beyond the wider window. Correspondingly, the error advice for the branch-known case no longer asserts the revision and branch are both wrong.

Also checked: GetQualityGate keys on analysisId (sonar.go:533), so setting Branch before the lookup cannot reach it; hack/empty-flag-audit/spec.json probes one flag at a time against the baseline set, so sonar-branch and pull-request never collide in the audit despite the new exclusion; goldens 16 and 113 match the format string byte for byte; the NewSonarConfig call in attestSonar.go:245 has the argument order right.

Findings

Two, both non-blocking, posted inline:

1. ps=500 has no test coverage (internal/sonar/sonar.go:168) — the one piece of new production behaviour here that nothing pins. One case-independent line inside _BranchParam's existing handler covers it.
2. NewSonarConfig is now eight positional strings (internal/sonar/sonar.go:139) — revision, pullRequest, branch adjacent and interchangeable to the compiler. Recorded, not requested; it belongs in its own commit.

Nothing else. I have no security or performance concerns: the branch reaches the URL through url.Values, which the injection row pins; ps=500 grows one response fivefold in the worst case and nothing in this path buffers unboundedly beyond what the decoder already does.

Worth saying

The two decisions carried over from the last round are the right ones and are better documented than most merged code. Pinning Type rather than Name in the payload assertion is the harder and more honest call — Name would have been tautological against this fixture, and a comment claiming otherwise is exactly the failure mode you set out to avoid. _ProjectKeyPath_WithoutBranch asserting --sonar-branch in the message, rather than the whole sentence, gets the durable half of the contract without adding a third rewording victim; and the pair-with-a-failing-half structure is what makes the passing half evidence about SonarQube rather than about the fake.

Deferring paging.total and the api/ce/activity permission/pagination problem to their own issue is correct scoping — both are the same class of defect (discarding SonarQube's own signal that its answer is incomplete) and neither is caused by this change. ps=500 is a fair interim narrowing given it is provably safe.
· branch 1116-sonar-branch

[AlexKantor87]

Review round addressed — all five threads replied to and resolved, in 124e0d97 and f18a14d8.

Taken: subtests for both parametric tables (the bare loops' t.Fatalf was ending the whole test, so an early failure silently disarmed the query-parameter-injection case); an assertion that the unscoped end-to-end test fails for its stated reason; the payload-branch assertion, pinned on Type since that is the half only a CE task can supply; and ps=500, with the error advice corrected so it no longer blames a revision and branch that may both be right.

Declined one: a comment noting that the CLI already blocks --sonar-branch with --pull-request. internal/sonar has one importer, so "the library must not depend on the CLI" describes a constraint that does not exist, and it would assert a fact about another package that nothing local enforces. Answered in the thread instead.

Two things recorded rather than fixed, so they are not lost:

1. TestGetSonarResults_BranchIgnoredForPullRequest currently proves the pull-request guard only by accident. The fake's ce/activity tasks carry no pullRequest field, so on that path nothing matches and GetTaskID never touches Branch — the assertion holds because of the guard. Make that fixture realistic and GetTaskID sets Branch = nil itself, at which point the test would pass with the guard deleted. Known and accepted; fixing it properly means a new fixture and new assertions.

2. ps=500 narrows the pagination window but does not close it. The response carries paging.total — the full pre-pagination count — and ProjectAnalyses decodes and discards it, so the CLI still cannot distinguish "no such revision" from "not in the window I searched". That is the same misdiagnosis class that cost this customer days. Filing separately, together with GetTaskID calling api/ce/activity first-page-only and returning a nil error when nothing matches, which publishes an attestation with an empty taskId and status and says nothing. Both are the CLI discarding SonarQube's own signal that its answer is incomplete.

Verified locally: go test, go test -race (CI does not run the race detector), make lint clean, and the empty-flag audit test. ps is asserted by no test and no golden; both existing goldens pin the no-branch advice, which is unchanged.