From 9dd1d773de4668e1141ed99a956397f2cc1eef15 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 20 Aug 2026 19:14:57 +0100 Subject: [PATCH 01/13] fix(sonar): forward branch to project_analyses/search on the revision path SonarQube's project_analyses/search endpoint defaults to the project's main branch. GetProjectAnalysisFromRevision never sent a 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. Forward sonarResults.Branch the same way #861 did. Nothing populates Branch on this path yet, so behaviour is unchanged until the --sonar-branch flag lands. Tests use the real responses from the customer instance in #1116, and pin the backwards-compatible contract: no branch param at all when no branch is known. Refs #1116 Co-Authored-By: Claude Opus 5 --- internal/sonar/sonar.go | 9 +- internal/sonar/sonar_test.go | 178 +++++++++++++++++++++++++++++++++++ 2 files changed, 186 insertions(+), 1 deletion(-) diff --git a/internal/sonar/sonar.go b/internal/sonar/sonar.go index 6676404fb..25af2a174 100644 --- a/internal/sonar/sonar.go +++ b/internal/sonar/sonar.go @@ -380,7 +380,14 @@ func GetCETaskData(httpClient *http.Client, project *Project, sonarResults *Sona func GetProjectAnalysisFromRevision(httpClient *http.Client, sonarResults *SonarResults, project *Project, revision string, logger *log.Logger) (string, error) { var analysisID string - projectAnalysesURL, err := sonarURL(sonarResults.ServerUrl, "api/project_analyses/search", url.Values{"project": {project.Key}}) + // Forward branch to search analyses on non-default branches (#1116). SonarQube + // defaults this endpoint to the project's main branch, so without it a scan on + // any other branch is invisible here. + params := url.Values{"project": {project.Key}} + if sonarResults.Branch != nil && sonarResults.Branch.Name != "" { + params.Set("branch", sonarResults.Branch.Name) + } + projectAnalysesURL, err := sonarURL(sonarResults.ServerUrl, "api/project_analyses/search", params) if err != nil { return "", err } diff --git a/internal/sonar/sonar_test.go b/internal/sonar/sonar_test.go index 4135a7c9a..94cf28c4a 100644 --- a/internal/sonar/sonar_test.go +++ b/internal/sonar/sonar_test.go @@ -101,3 +101,181 @@ func TestGetProjectAnalysisFromAnalysisID_NoBranch(t *testing.T) { t.Errorf("expected no branch param when Branch is nil, got branch=%q", receivedBranch) } } + +// The fixtures below are real responses from the customer instance in issue #1116. +// The project's main branch (master) has never been analysed; the analysis lives on +// release/uat. The slash in the branch name is deliberate — see FuzzBranchParam. +const ( + revProjectKey = "customer-project" + revMainBranch = "master" + revFeatureBranch = "release/uat" + revAnalysisKey = "AaAeAfTdP27JeOuKOycd" + revAnalysisDate = "2026-08-20T11:09:48+0400" + revRevision = "8700f236fe2fd6c3e2dc5bf33c7e5f3aa8fd3dee" +) + +// branchScopedAnalysesServer serves api/project_analyses/search the way SonarQube +// does for the issue #1116 project: unscoped (or scoped to master) the result is +// empty, and the analysis is only returned when the search is scoped to +// release/uat. It records the branch parameter of the last request. +func branchScopedAnalysesServer(t *testing.T, received *string, present *bool) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/project_analyses/search" { + http.NotFound(w, r) + return + } + *received = r.URL.Query().Get("branch") + _, *present = r.URL.Query()["branch"] + + resp := sonar.ProjectAnalyses{Analyses: []sonar.Analysis{}} + if *received == revFeatureBranch { + resp.Analyses = []sonar.Analysis{ + {Key: revAnalysisKey, Date: revAnalysisDate, Revision: revRevision}, + } + } + _ = json.NewEncoder(w).Encode(resp) + })) +} + +// TestGetProjectAnalysisFromRevision_PassesBranch is the issue #1116 regression: +// when a branch is known, GetProjectAnalysisFromRevision must scope the search to +// it, otherwise SonarQube only searches the main branch and the analysis is never +// found. This is the same defect as #861 in the sibling code path. +func TestGetProjectAnalysisFromRevision_PassesBranch(t *testing.T) { + var receivedBranch string + var branchParamPresent bool + server := branchScopedAnalysesServer(t, &receivedBranch, &branchParamPresent) + defer server.Close() + + sonarResults := &sonar.SonarResults{ + ServerUrl: server.URL, + Branch: &sonar.Branch{Name: revFeatureBranch}, + } + project := &sonar.Project{Key: revProjectKey} + + analysisID, err := sonar.GetProjectAnalysisFromRevision(http.DefaultClient, sonarResults, project, revRevision, discardLogger()) + if err != nil { + t.Fatalf("GetProjectAnalysisFromRevision returned error: %v", err) + } + if receivedBranch != revFeatureBranch { + t.Errorf("expected branch=%q to be forwarded to SonarQube, got %q", revFeatureBranch, receivedBranch) + } + if analysisID != revAnalysisKey { + t.Errorf("expected analysis ID %q, got %q", revAnalysisKey, analysisID) + } + if sonarResults.AnalysedAt != revAnalysisDate { + t.Errorf("expected AnalysedAt=%q, got %q", revAnalysisDate, sonarResults.AnalysedAt) + } +} + +// TestGetProjectAnalysisFromRevision_NoBranch pins the backwards-compatible +// behaviour: with no branch known, the branch parameter must be absent from the +// request, not present and empty — an empty branch is not the same query to +// SonarQube as no branch at all. +func TestGetProjectAnalysisFromRevision_NoBranch(t *testing.T) { + var receivedBranch string + var branchParamPresent bool + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + receivedBranch = r.URL.Query().Get("branch") + _, branchParamPresent = r.URL.Query()["branch"] + resp := sonar.ProjectAnalyses{ + Analyses: []sonar.Analysis{ + {Key: revAnalysisKey, Date: revAnalysisDate, Revision: revRevision}, + }, + } + _ = json.NewEncoder(w).Encode(resp) + })) + defer server.Close() + + sonarResults := &sonar.SonarResults{ServerUrl: server.URL} + project := &sonar.Project{Key: revProjectKey} + + if _, err := sonar.GetProjectAnalysisFromRevision(http.DefaultClient, sonarResults, project, revRevision, discardLogger()); err != nil { + t.Fatalf("GetProjectAnalysisFromRevision returned error: %v", err) + } + if branchParamPresent { + t.Errorf("expected no branch param when Branch is nil, got branch=%q", receivedBranch) + } +} + +// TestGetProjectAnalysisFromRevision_EmptyBranchNameNotSent covers the same +// contract for a branch that is set but empty (the flag defaults to ""): still no +// branch parameter. +func TestGetProjectAnalysisFromRevision_EmptyBranchNameNotSent(t *testing.T) { + var receivedBranch string + var branchParamPresent bool + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + receivedBranch = r.URL.Query().Get("branch") + _, branchParamPresent = r.URL.Query()["branch"] + resp := sonar.ProjectAnalyses{ + Analyses: []sonar.Analysis{ + {Key: revAnalysisKey, Date: revAnalysisDate, Revision: revRevision}, + }, + } + _ = json.NewEncoder(w).Encode(resp) + })) + defer server.Close() + + sonarResults := &sonar.SonarResults{ServerUrl: server.URL, Branch: &sonar.Branch{}} + project := &sonar.Project{Key: revProjectKey} + + if _, err := sonar.GetProjectAnalysisFromRevision(http.DefaultClient, sonarResults, project, revRevision, discardLogger()); err != nil { + t.Fatalf("GetProjectAnalysisFromRevision returned error: %v", err) + } + if branchParamPresent { + t.Errorf("expected no branch param for an empty branch name, got branch=%q", receivedBranch) + } +} + +// TestGetProjectAnalysisFromRevision_MainBranchHasNoAnalysis reproduces the +// customer's failure exactly: without the branch, the search comes back empty and +// the command fails even though the analysis exists on release/uat. +func TestGetProjectAnalysisFromRevision_MainBranchHasNoAnalysis(t *testing.T) { + var receivedBranch string + var branchParamPresent bool + server := branchScopedAnalysesServer(t, &receivedBranch, &branchParamPresent) + defer server.Close() + + project := &sonar.Project{Key: revProjectKey} + + // Scoped to the main branch: nothing there, so this must fail. + mainResults := &sonar.SonarResults{ServerUrl: server.URL, Branch: &sonar.Branch{Name: revMainBranch}} + if _, err := sonar.GetProjectAnalysisFromRevision(http.DefaultClient, mainResults, project, revRevision, discardLogger()); err == nil { + t.Fatal("expected an error when the main branch has no analysis for the revision") + } + + // Scoped to the branch the scan actually ran on: found. + branchResults := &sonar.SonarResults{ServerUrl: server.URL, Branch: &sonar.Branch{Name: revFeatureBranch}} + analysisID, err := sonar.GetProjectAnalysisFromRevision(http.DefaultClient, branchResults, project, revRevision, discardLogger()) + if err != nil { + t.Fatalf("expected the analysis to be found on %s, got error: %v", revFeatureBranch, err) + } + if analysisID != revAnalysisKey { + t.Errorf("expected analysis ID %q, got %q", revAnalysisKey, analysisID) + } +} + +// TestGetProjectAnalysisFromRevision_WrongRevisionOnBranch guards against the +// branch scoping loosening the revision match: an analysis on the right branch +// with a different revision is not the one we asked for. +func TestGetProjectAnalysisFromRevision_WrongRevisionOnBranch(t *testing.T) { + var receivedBranch string + var branchParamPresent bool + server := branchScopedAnalysesServer(t, &receivedBranch, &branchParamPresent) + defer server.Close() + + sonarResults := &sonar.SonarResults{ + ServerUrl: server.URL, + Branch: &sonar.Branch{Name: revFeatureBranch}, + } + project := &sonar.Project{Key: revProjectKey} + + _, err := sonar.GetProjectAnalysisFromRevision(http.DefaultClient, sonarResults, project, "0000000000000000000000000000000000000000", discardLogger()) + if err == nil { + t.Fatal("expected an error when no analysis on the branch matches the revision") + } + if sonarResults.AnalysedAt != "" { + t.Errorf("expected AnalysedAt to stay empty on no match, got %q", sonarResults.AnalysedAt) + } +} From a33520858fc3769fb80591c54134b8c7b33908c8 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 20 Aug 2026 19:19:30 +0100 Subject: [PATCH 02/13] feat(sonar): add --sonar-branch for scans on non-main branches The project-key/revision path has no CE task to read the branch from, so the branch can only come from the user. --sonar-branch supplies it and is forwarded to project_analyses/search, which makes `kosli attest sonar --sonar-project-key` work for a scan on any branch. Unset, nothing changes. Mutually exclusive with --pull-request: a PR scan is not a branch scan, and the branch would be silently ignored. NewSonarConfig takes the branch as a ninth positional parameter rather than moving to an options struct. The constructor is overdue that refactor, but this fix is blocking a customer and an exported-API change unrelated to the defect belongs in its own PR, where it cannot be conflated with a behaviour change. Refs #1116 Co-Authored-By: Claude Opus 5 --- cmd/kosli/attestSonar.go | 23 ++++- cmd/kosli/attestSonar_test.go | 6 ++ cmd/kosli/root.go | 1 + internal/sonar/sonar.go | 10 +- internal/sonar/sonar_auth_test.go | 16 ++-- internal/sonar/sonar_branch_test.go | 142 ++++++++++++++++++++++++++++ 6 files changed, 188 insertions(+), 10 deletions(-) create mode 100644 internal/sonar/sonar_branch_test.go diff --git a/cmd/kosli/attestSonar.go b/cmd/kosli/attestSonar.go index edc4d186b..212ad825c 100644 --- a/cmd/kosli/attestSonar.go +++ b/cmd/kosli/attestSonar.go @@ -25,6 +25,7 @@ type attestSonarOptions struct { serverURL string revision string pullRequest string + branch string maxWait int payload SonarAttestationPayload } @@ -48,6 +49,8 @@ exponential backoff between retries. Once the results are available they are att 2. Providing the Sonar project key and either the revision or the pull-request ID of the scan (plus the SonarQube server URL if relevant). For branch scans: if running the Kosli CLI in some CI/CD pipeline, the revision is defaulted to the commit SHA. If you are running the command locally, or have overriden the revision in SonarQube via parameters to the Sonar scanner, you can provide the correct revision using the ^--sonar-revision^ flag. +If the scan ran on a branch other than the project's main branch in SonarQube, you must also provide the branch name using the ^--sonar-branch^ flag: +SonarQube only searches the main branch unless it is told otherwise, so without it the scan cannot be found. For pull request scans: provide the pull-request ID using the ^--pull-request^ flag instead of the revision. Kosli then finds the scan results for the specified project key and revision or pull-request ID. @@ -104,6 +107,18 @@ kosli attest sonar \ --api-token yourAPIToken \ --org yourOrgName \ +# report a SonarQube Cloud attestation about a trail using key/revision for a scan on a non-main branch: +kosli attest sonar \ + --name yourAttestationName \ + --flow yourFlowName \ + --trail yourTrailName \ + --sonar-api-token yourSonarAPIToken \ + --sonar-project-key yourSonarProjectKey \ + --sonar-revision yourSonarRevision \ + --sonar-branch yourSonarBranchName \ + --api-token yourAPIToken \ + --org yourOrgName \ + # report a SonarQube Cloud attestation about a trail for a pull request scan using key/pull-request: kosli attest sonar \ --name yourAttestationName \ @@ -176,6 +191,11 @@ func newAttestSonarCmd(out io.Writer) *cobra.Command { return err } + err = MuXRequiredFlags(cmd, []string{"sonar-branch", "pull-request"}, false) + if err != nil { + return err + } + err = ValidateAttestationArtifactArg(args, o.fingerprintOptions.artifactType, o.payload.ArtifactFingerprint) if err != nil { return ErrorBeforePrintingUsage(cmd, err.Error()) @@ -199,6 +219,7 @@ func newAttestSonarCmd(out io.Writer) *cobra.Command { cmd.Flags().StringVar(&o.serverURL, "sonar-server-url", "https://sonarcloud.io", sonarServerURLFlag) cmd.Flags().StringVar(&o.revision, "sonar-revision", o.commitSHA, sonarRevisionFlag) cmd.Flags().StringVar(&o.pullRequest, "pull-request", "", sonarPRFlag) + cmd.Flags().StringVar(&o.branch, "sonar-branch", "", sonarBranchFlag) cmd.Flags().StringVar(&o.ceTaskURL, "sonar-ce-task-url", "", sonarCETaskURLFlag) cmd.Flags().IntVar(&o.maxWait, "max-wait", 30, sonarMaxWaitFlag) @@ -221,7 +242,7 @@ func (o *attestSonarOptions) run(args []string) error { return err } - sc := sonar.NewSonarConfig(o.apiToken, o.workingDir, o.ceTaskURL, o.projectKey, o.serverURL, o.revision, o.pullRequest, o.maxWait) + sc := sonar.NewSonarConfig(o.apiToken, o.workingDir, o.ceTaskURL, o.projectKey, o.serverURL, o.revision, o.pullRequest, o.branch, o.maxWait) o.payload.SonarResults, err = sc.GetSonarResults(logger) if err != nil { diff --git a/cmd/kosli/attestSonar_test.go b/cmd/kosli/attestSonar_test.go index 7cd8d5577..ac4872c54 100644 --- a/cmd/kosli/attestSonar_test.go +++ b/cmd/kosli/attestSonar_test.go @@ -251,6 +251,12 @@ func (suite *AttestSonarCommandTestSuite) TestAttestSonarCmd() { cmd: fmt.Sprintf("attest sonar --name cli.foo --commit HEAD --origin-url http://www.example.com --sonar-ce-task-url https://sonarcloud.io/api/ce/task?id=AZERk4uWpzGpahwkB9ac %s", suite.defaultKosliArguments), golden: "Error: No activity found for task 'AZERk4uWpzGpahwkB9ac' on https://sonarcloud.io. \nSonarQube may be experiencing problems, please check https://status.sonarqube.com/ and try again later. \nOtherwise if you are attesting an older scan, the snapshot may have been deleted by SonarQube\n", }, + { + wantError: true, + name: "30 can't provide both sonar-branch and pull-request", + cmd: fmt.Sprintf("attest sonar --name cli.foo --commit HEAD --origin-url http://www.example.com --sonar-project-key cyber-dojo_differ --sonar-branch release/uat --pull-request 5 %s", suite.defaultKosliArguments), + golden: "Error: only one of --sonar-branch, --pull-request is allowed\n", + }, { wantError: true, name: "29 fails when --name has invalid dot format", diff --git a/cmd/kosli/root.go b/cmd/kosli/root.go index 144650b3d..db33b771b 100644 --- a/cmd/kosli/root.go +++ b/cmd/kosli/root.go @@ -302,6 +302,7 @@ The ^.kosli_ignore^ will be treated as part of the artifact like any other file, sonarServerURLFlag = "[conditional] The URL of your SonarQube server. Only required if you are using SonarQube Server and not using SonarQube's metadata file to get scan results." sonarRevisionFlag = "[conditional] The revision of the SonarQube project. Only required if you want to use the project key/revision to get the scan results rather than using Sonar's metadata file and you have overridden the default revision, or you aren't using a CI. Defaults to the value of the git commit flag. Cannot be used with --pull-request." sonarPRFlag = "[conditional] The ID of the pull-request. Only required if you want to use the project key/pull-request to get the scan results rather than using Sonar's metadata file. Cannot be used with --sonar-revision." + sonarBranchFlag = "[conditional] The name of the branch the SonarQube scan ran on. Only required if you are using the project key/revision to get the scan results and the scan ran on a branch other than the project's main branch in SonarQube. Cannot be used with --pull-request." sonarMaxWaitFlag = "[optional] Allow the command to wait and retry fetching the scan results from SonarQube, up to the maximum number of seconds provided, with exponential backoff. Useful when using SonarQube's metadata file to retrieve and attest scans that take a long time to process . Defaults to 30 seconds." sonarCETaskURLFlag = "[conditional] The URL of the SonarQube CE task. Can be used instead of --sonar-working-dir when the report-task.txt file is not accessible, e.g. due to container isolation in CI/CD pipelines." logicalEnvFlag = "[required] The logical environment." diff --git a/internal/sonar/sonar.go b/internal/sonar/sonar.go index 25af2a174..14f732489 100644 --- a/internal/sonar/sonar.go +++ b/internal/sonar/sonar.go @@ -22,6 +22,7 @@ type SonarConfig struct { projectKey string serverURL string pullRequest string + branch string maxWait int } @@ -135,7 +136,7 @@ type Error struct { Msg string `json:"msg"` } -func NewSonarConfig(apiToken, workingDir, ceTaskUrl, projectKey, serverURL, revision, pullRequest string, maxWait int) *SonarConfig { +func NewSonarConfig(apiToken, workingDir, ceTaskUrl, projectKey, serverURL, revision, pullRequest, branch string, maxWait int) *SonarConfig { return &SonarConfig{ APIToken: apiToken, WorkingDir: workingDir, @@ -144,6 +145,7 @@ func NewSonarConfig(apiToken, workingDir, ceTaskUrl, projectKey, serverURL, revi projectKey: projectKey, serverURL: serverURL, pullRequest: pullRequest, + branch: branch, maxWait: maxWait, } } @@ -197,6 +199,12 @@ func (sc *SonarConfig) GetSonarResults(logger *log.Logger) (*SonarResults, error project.Key = sc.projectKey sonarResults.ServerUrl = sc.serverURL sonarResults.Revision = sc.revision + // On this path there is no CE task to read the branch from, so the branch + // can only come from the user (#1116). Set it on the results, which is the + // one mechanism both analyses lookups use to scope their search. + if sc.branch != "" { + sonarResults.Branch = &Branch{Name: sc.branch} + } project.Url, err = sonarURL(sonarResults.ServerUrl, "dashboard", url.Values{"id": {project.Key}}) if err != nil { return nil, err diff --git a/internal/sonar/sonar_auth_test.go b/internal/sonar/sonar_auth_test.go index 607e11e54..6b0daeafc 100644 --- a/internal/sonar/sonar_auth_test.go +++ b/internal/sonar/sonar_auth_test.go @@ -123,7 +123,7 @@ func TestGetSonarResults_Pre10Server_FallsBackToBasic(t *testing.T) { srv := httptest.NewServer(fake.handler()) defer srv.Close() - sc := sonar.NewSonarConfig("tok", t.TempDir(), srv.URL+"/api/ce/task?id=AYx", "", "", "", "", 5) + sc := sonar.NewSonarConfig("tok", t.TempDir(), srv.URL+"/api/ce/task?id=AYx", "", "", "", "", "", 5) res, err := sc.GetSonarResults(discardLogger()) if err != nil { t.Fatalf("expected success against pre-10 server via Basic fallback, got error: %v", err) @@ -158,7 +158,7 @@ func TestGetSonarResults_BearerServer_NoBasicSent(t *testing.T) { srv := httptest.NewServer(fake.handler()) defer srv.Close() - sc := sonar.NewSonarConfig("tok", t.TempDir(), srv.URL+"/api/ce/task?id=AYx", "", "", "", "", 5) + sc := sonar.NewSonarConfig("tok", t.TempDir(), srv.URL+"/api/ce/task?id=AYx", "", "", "", "", "", 5) res, err := sc.GetSonarResults(discardLogger()) if err != nil { t.Fatalf("expected success against a Bearer-capable server, got error: %v", err) @@ -182,7 +182,7 @@ func TestGetSonarResults_InvalidToken_TriesBothThenErrors(t *testing.T) { srv := httptest.NewServer(fake.handler()) defer srv.Close() - sc := sonar.NewSonarConfig("tok", t.TempDir(), srv.URL+"/api/ce/task?id=AYx", "", "", "", "", 5) + sc := sonar.NewSonarConfig("tok", t.TempDir(), srv.URL+"/api/ce/task?id=AYx", "", "", "", "", "", 5) _, err := sc.GetSonarResults(discardLogger()) if err == nil { t.Fatal("expected an error when neither auth scheme is accepted") @@ -212,7 +212,7 @@ func TestGetSonarResults_ServerError_NoFallback(t *testing.T) { srv := httptest.NewServer(fake.handler()) defer srv.Close() - sc := sonar.NewSonarConfig("tok", t.TempDir(), srv.URL+"/api/ce/task?id=AYx", "", "", "", "", 5) + sc := sonar.NewSonarConfig("tok", t.TempDir(), srv.URL+"/api/ce/task?id=AYx", "", "", "", "", "", 5) _, err := sc.GetSonarResults(discardLogger()) if err == nil { t.Fatal("expected an error on HTTP 503") @@ -237,7 +237,7 @@ func TestGetSonarResults_StructuredForbidden_NoFallback(t *testing.T) { srv := httptest.NewServer(fake.handler()) defer srv.Close() - sc := sonar.NewSonarConfig("tok", t.TempDir(), srv.URL+"/api/ce/task?id=AYx", "", "", "", "", 5) + sc := sonar.NewSonarConfig("tok", t.TempDir(), srv.URL+"/api/ce/task?id=AYx", "", "", "", "", "", 5) _, err := sc.GetSonarResults(discardLogger()) if err == nil { t.Fatal("expected an error on a structured 403") @@ -260,7 +260,7 @@ func TestGetSonarResults_TokenWhitespaceTrimmed(t *testing.T) { srv := httptest.NewServer(fake.handler()) defer srv.Close() - sc := sonar.NewSonarConfig("tok\n", t.TempDir(), srv.URL+"/api/ce/task?id=AYx", "", "", "", "", 5) + sc := sonar.NewSonarConfig("tok\n", t.TempDir(), srv.URL+"/api/ce/task?id=AYx", "", "", "", "", "", 5) res, err := sc.GetSonarResults(discardLogger()) if err != nil { t.Fatalf("expected success with a trimmed token, got error: %v", err) @@ -286,7 +286,7 @@ func TestGetSonarResults_Pre10PollLoop_ResolvesSchemeOncePerRun(t *testing.T) { srv := httptest.NewServer(fake.handler()) defer srv.Close() - sc := sonar.NewSonarConfig("tok", t.TempDir(), srv.URL+"/api/ce/task?id=AYx", "", "", "", "", 3) + sc := sonar.NewSonarConfig("tok", t.TempDir(), srv.URL+"/api/ce/task?id=AYx", "", "", "", "", "", 3) res, err := sc.GetSonarResults(discardLogger()) if err != nil { t.Fatalf("expected success after a PENDING poll, got error: %v", err) @@ -307,7 +307,7 @@ func TestGetSonarResults_Forbidden_NonJSON_RendersActualStatus(t *testing.T) { srv := httptest.NewServer(fake.handler()) defer srv.Close() - sc := sonar.NewSonarConfig("tok", t.TempDir(), srv.URL+"/api/ce/task?id=AYx", "", "", "", "", 5) + sc := sonar.NewSonarConfig("tok", t.TempDir(), srv.URL+"/api/ce/task?id=AYx", "", "", "", "", "", 5) _, err := sc.GetSonarResults(discardLogger()) if err == nil { t.Fatal("expected an error on a 403 with a non-JSON body") diff --git a/internal/sonar/sonar_branch_test.go b/internal/sonar/sonar_branch_test.go new file mode 100644 index 000000000..7f3329e2c --- /dev/null +++ b/internal/sonar/sonar_branch_test.go @@ -0,0 +1,142 @@ +package sonar_test + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "sync" + "testing" + + "github.com/kosli-dev/cli/internal/sonar" +) + +// fakeBranchSonar is a SonarQube stand-in for the project-key/revision path of +// issue #1116. It models the customer's project: the analysis for the revision +// exists only on release/uat, so api/project_analyses/search returns it only when +// the request is scoped to that branch — exactly as SonarQube behaves. +type fakeBranchSonar struct { + mu sync.Mutex + searchBranches []string // branch param of each project_analyses/search request + searchProjects []string // project param of each project_analyses/search request + taskBranch string // branch reported on the ce/activity task + taskBranchType string +} + +func (f *fakeBranchSonar) handler() http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/project_analyses/search": + branch := r.URL.Query().Get("branch") + f.mu.Lock() + f.searchBranches = append(f.searchBranches, branch) + f.searchProjects = append(f.searchProjects, r.URL.Query().Get("project")) + f.mu.Unlock() + + resp := sonar.ProjectAnalyses{Analyses: []sonar.Analysis{}} + if branch == revFeatureBranch { + resp.Analyses = []sonar.Analysis{ + {Key: revAnalysisKey, Date: revAnalysisDate, Revision: revRevision}, + } + } + _ = json.NewEncoder(w).Encode(resp) + case "/api/ce/activity": + _ = json.NewEncoder(w).Encode(sonar.ActivityResponse{ + Tasks: []sonar.Task{{ + TaskID: "AaAeAfTdP27JeOuKOyce", + ComponentName: "customer project", + ComponentKey: revProjectKey, + AnalysisID: revAnalysisKey, + Status: "SUCCESS", + Branch: f.taskBranch, + BranchType: f.taskBranchType, + }}, + }) + case "/api/qualitygates/project_status": + _ = json.NewEncoder(w).Encode(sonar.QualityGateResponse{ + ProjectStatus: sonar.ProjectStatus{Status: "OK", Conditions: []sonar.Conditions{}}, + }) + default: + http.NotFound(w, r) + } + } +} + +func (f *fakeBranchSonar) searches() ([]string, []string) { + f.mu.Lock() + defer f.mu.Unlock() + return append([]string(nil), f.searchBranches...), append([]string(nil), f.searchProjects...) +} + +// TestGetSonarResults_ProjectKeyPath_WithBranch is the end-to-end wiring for +// --sonar-branch: given the project key, the revision and the branch, the whole +// project-key path succeeds against a project whose analysis is not on the main +// branch, and the branch reaches the attestation payload. +func TestGetSonarResults_ProjectKeyPath_WithBranch(t *testing.T) { + fake := &fakeBranchSonar{taskBranch: revFeatureBranch, taskBranchType: "LONG"} + srv := httptest.NewServer(fake.handler()) + defer srv.Close() + + sc := sonar.NewSonarConfig("tok", t.TempDir(), "", revProjectKey, srv.URL, revRevision, "", revFeatureBranch, 5) + results, err := sc.GetSonarResults(discardLogger()) + if err != nil { + t.Fatalf("expected the scan on %s to be found, got error: %v", revFeatureBranch, err) + } + + branches, projects := fake.searches() + if len(branches) != 1 || branches[0] != revFeatureBranch { + t.Errorf("expected one search scoped to branch %q, got %v", revFeatureBranch, branches) + } + if len(projects) != 1 || projects[0] != revProjectKey { + t.Errorf("expected project %q on the search, got %v", revProjectKey, projects) + } + if results.Branch == nil || results.Branch.Name != revFeatureBranch { + t.Errorf("expected branch %q in the attestation payload, got %+v", revFeatureBranch, results.Branch) + } + if results.AnalysedAt != revAnalysisDate { + t.Errorf("expected AnalysedAt=%q, got %q", revAnalysisDate, results.AnalysedAt) + } + if results.Revision != revRevision { + t.Errorf("expected Revision=%q, got %q", revRevision, results.Revision) + } + if results.QualityGate == nil || results.QualityGate.Status != "OK" { + t.Fatalf("expected quality gate OK, got %+v", results.QualityGate) + } +} + +// TestGetSonarResults_ProjectKeyPath_WithoutBranch is the other half of the same +// contract: against the same server, omitting the branch still fails. That is what +// makes the test above evidence that the flag is what fixes #1116, rather than the +// fake being lenient. +func TestGetSonarResults_ProjectKeyPath_WithoutBranch(t *testing.T) { + fake := &fakeBranchSonar{taskBranch: revFeatureBranch, taskBranchType: "LONG"} + srv := httptest.NewServer(fake.handler()) + defer srv.Close() + + sc := sonar.NewSonarConfig("tok", t.TempDir(), "", revProjectKey, srv.URL, revRevision, "", "", 5) + if _, err := sc.GetSonarResults(discardLogger()); err == nil { + t.Fatal("expected the unscoped search to fail, as it does for the customer in #1116") + } + + branches, _ := fake.searches() + if len(branches) != 1 || branches[0] != "" { + t.Errorf("expected one unscoped search, got %v", branches) + } +} + +// TestGetSonarResults_BranchIgnoredForPullRequest documents that a PR scan is not a +// branch scan: with --pull-request the branch is not used to scope any search. +// The two flags are mutually exclusive at the CLI, so this only pins the library. +func TestGetSonarResults_BranchIgnoredForPullRequest(t *testing.T) { + fake := &fakeBranchSonar{} + srv := httptest.NewServer(fake.handler()) + defer srv.Close() + + sc := sonar.NewSonarConfig("tok", t.TempDir(), "", revProjectKey, srv.URL, "", "42", revFeatureBranch, 5) + // The PR lookup is not served by this fake, so this fails; what matters is that + // no branch-scoped analyses search was made on the way. + _, _ = sc.GetSonarResults(discardLogger()) + + if branches, _ := fake.searches(); len(branches) != 0 { + t.Errorf("expected no project_analyses/search on the pull-request path, got %v", branches) + } +} From c9ba63e09b3509805a64761da444bcc150e739be Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 20 Aug 2026 19:20:56 +0100 Subject: [PATCH 03/13] fix(sonar): say which branch was searched when no analysis is found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An empty result from project_analyses/search is indistinguishable from a permissions problem. In #1116 that cost the customer several days chasing token types and Browse permissions while their token was correct throughout: the analysis was on another branch and the search only ever looked at the main one. Name the scope in the error — the branch that was searched, or that only the main branch was, because no --sonar-branch was given. Golden for the SonarQube Server suite's case 113 was already stale (it predates an earlier reword of this message and that suite only runs with SONARQUBE set); brought in line with the message the code now produces. Refs #1116 Co-Authored-By: Claude Opus 5 --- cmd/kosli/attestSonar_test.go | 4 +-- internal/sonar/sonar.go | 8 +++++- internal/sonar/sonar_test.go | 50 +++++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/cmd/kosli/attestSonar_test.go b/cmd/kosli/attestSonar_test.go index ac4872c54..31a4f1235 100644 --- a/cmd/kosli/attestSonar_test.go +++ b/cmd/kosli/attestSonar_test.go @@ -182,7 +182,7 @@ func (suite *AttestSonarCommandTestSuite) TestAttestSonarCmd() { wantError: true, name: "16 if incorrect revision given (or the scan for the given revision has been deleted by SonarCloud)", cmd: fmt.Sprintf("attest sonar --name cli.foo --commit HEAD --origin-url http://www.example.com --sonar-project-key cyber-dojo_differ --sonar-revision b4d1053f2aac18c9fb4b9a289a8289199c932e12 %s", suite.defaultKosliArguments), - golden: "Error: analysis for revision b4d1053f2aac18c9fb4b9a289a8289199c932e12 of project cyber-dojo_differ not found. Check the revision is correct. \nThe scan may still be being processed by SonarQube, try again later.\n Otherwise if you are attesting an older scan, the snapshot may also have been deleted by SonarQube\n", + golden: "Error: analysis for revision b4d1053f2aac18c9fb4b9a289a8289199c932e12 of project cyber-dojo_differ not found: only the project's main branch was searched, because no --sonar-branch was given. Check the revision and the branch are correct. \nThe scan may still be being processed by SonarQube, try again later.\n Otherwise if you are attesting an older scan, the snapshot may also have been deleted by SonarQube\n", }, { wantError: true, @@ -345,7 +345,7 @@ func (suite *AttestSonarQubeCommandTestSuite) TestAttestSonarQubeCmd() { wantError: true, name: "113 if incorrect revision given, give an error", cmd: fmt.Sprintf("attest sonar --name cli.foo --commit HEAD --origin-url http://www.example.com --sonar-server-url http://localhost:9000 --sonar-project-key test5 --sonar-revision 8e6f9489e5f2ddf8e719b503e374975e8b607fd2 %s", suite.defaultKosliArguments), - golden: "Error: analysis for revision 8e6f9489e5f2ddf8e719b503e374975e8b607fd2 of project test5 not found. Check the revision is correct. Snapshot may also have been deleted by SonarQube\n", + golden: "Error: analysis for revision 8e6f9489e5f2ddf8e719b503e374975e8b607fd2 of project test5 not found: only the project's main branch was searched, because no --sonar-branch was given. Check the revision and the branch are correct. \nThe scan may still be being processed by SonarQube, try again later.\n Otherwise if you are attesting an older scan, the snapshot may also have been deleted by SonarQube\n", }, { wantError: true, diff --git a/internal/sonar/sonar.go b/internal/sonar/sonar.go index 14f732489..b16d16b58 100644 --- a/internal/sonar/sonar.go +++ b/internal/sonar/sonar.go @@ -433,7 +433,13 @@ func GetProjectAnalysisFromRevision(httpClient *http.Client, sonarResults *Sonar } if sonarResults.AnalysedAt == "" { - return "", fmt.Errorf("analysis for revision %s of project %s not found. Check the revision is correct. \nThe scan may still be being processed by SonarQube, try again later.\n Otherwise if you are attesting an older scan, the snapshot may also have been deleted by SonarQube", revision, project.Key) + // An empty result reads like a permissions problem, so say which branch was + // actually searched: unscoped, SonarQube only searches the main branch (#1116). + scope := "only the project's main branch was searched, because no --sonar-branch was given" + if sonarResults.Branch != nil && sonarResults.Branch.Name != "" { + scope = fmt.Sprintf("branch %s was searched", sonarResults.Branch.Name) + } + return "", fmt.Errorf("analysis for revision %s of project %s not found: %s. Check the revision and the branch are correct. \nThe scan may still be being processed by SonarQube, try again later.\n Otherwise if you are attesting an older scan, the snapshot may also have been deleted by SonarQube", revision, project.Key, scope) } return analysisID, nil diff --git a/internal/sonar/sonar_test.go b/internal/sonar/sonar_test.go index 94cf28c4a..3a2cc08f2 100644 --- a/internal/sonar/sonar_test.go +++ b/internal/sonar/sonar_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" "github.com/kosli-dev/cli/internal/sonar" @@ -279,3 +280,52 @@ func TestGetProjectAnalysisFromRevision_WrongRevisionOnBranch(t *testing.T) { t.Errorf("expected AnalysedAt to stay empty on no match, got %q", sonarResults.AnalysedAt) } } + +// TestGetProjectAnalysisFromRevision_NoBranchErrorSuggestsFlag covers the other +// half of #1116: an empty search result is indistinguishable from a permissions +// problem, which is what cost the customer several days. When no branch was given, +// the error has to say that only the main branch was searched. +func TestGetProjectAnalysisFromRevision_NoBranchErrorSuggestsFlag(t *testing.T) { + var receivedBranch string + var branchParamPresent bool + server := branchScopedAnalysesServer(t, &receivedBranch, &branchParamPresent) + defer server.Close() + + sonarResults := &sonar.SonarResults{ServerUrl: server.URL} + project := &sonar.Project{Key: revProjectKey} + + _, err := sonar.GetProjectAnalysisFromRevision(http.DefaultClient, sonarResults, project, revRevision, discardLogger()) + if err == nil { + t.Fatal("expected an error when the main branch has no analysis for the revision") + } + if !strings.Contains(err.Error(), "--sonar-branch") { + t.Errorf("expected the error to point at --sonar-branch, got: %v", err) + } + if !strings.Contains(err.Error(), "main branch") { + t.Errorf("expected the error to say only the main branch was searched, got: %v", err) + } +} + +// TestGetProjectAnalysisFromRevision_BranchErrorNamesBranch is the same contract +// once a branch has been supplied: the error must name the branch that was +// searched, and must not suggest a flag that is already in use. +func TestGetProjectAnalysisFromRevision_BranchErrorNamesBranch(t *testing.T) { + var receivedBranch string + var branchParamPresent bool + server := branchScopedAnalysesServer(t, &receivedBranch, &branchParamPresent) + defer server.Close() + + sonarResults := &sonar.SonarResults{ServerUrl: server.URL, Branch: &sonar.Branch{Name: revMainBranch}} + project := &sonar.Project{Key: revProjectKey} + + _, err := sonar.GetProjectAnalysisFromRevision(http.DefaultClient, sonarResults, project, revRevision, discardLogger()) + if err == nil { + t.Fatal("expected an error when the searched branch has no analysis for the revision") + } + if !strings.Contains(err.Error(), revMainBranch) { + t.Errorf("expected the error to name branch %q, got: %v", revMainBranch, err) + } + if strings.Contains(err.Error(), "--sonar-branch") { + t.Errorf("expected no --sonar-branch suggestion when a branch was given, got: %v", err) + } +} From bf38a53d4f3c1ad91436fa42de651ebde2944b6a Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 20 Aug 2026 19:24:31 +0100 Subject: [PATCH 04/13] test(sonar): fuzz branch-name encoding on project_analyses/search MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Branch names routinely contain characters that are special in a query string, and the #1116 branch (release/uat) is one of them. 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. Refs #1116 Co-Authored-By: Claude Opus 5 --- internal/sonar/sonar_fuzz_test.go | 102 ++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) create mode 100644 internal/sonar/sonar_fuzz_test.go diff --git a/internal/sonar/sonar_fuzz_test.go b/internal/sonar/sonar_fuzz_test.go new file mode 100644 index 000000000..276db89da --- /dev/null +++ b/internal/sonar/sonar_fuzz_test.go @@ -0,0 +1,102 @@ +package sonar_test + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + + "github.com/kosli-dev/cli/internal/sonar" +) + +// FuzzBranchParam checks two properties of branch handling on the +// project_analyses/search request, for any branch name at all: +// +// 1. round-trip — the branch SonarQube receives is byte-for-byte the branch the +// user supplied. Branch names routinely contain characters that are special in +// a query string (release/uat, feature/ABC-123, names with spaces). +// 2. no query-parameter injection — a branch name cannot alter another parameter. +// A branch called "x&project=other" must not change which project is searched. +// +// url.Values.Encode() gives us both. The point of the fuzz test is to keep them: +// a later refactor to string concatenation would reintroduce the injection, and +// this is what would catch it. +func FuzzBranchParam(f *testing.F) { + seeds := []string{ + "release/uat", // the branch from #1116 — a slash is the common case + "feature/ABC-123_x", // typical Jira-derived branch name + "main", // plain + "master", // + "a b", // space + "a&b=c", // separator and assignment + "x&project=other", // the injection attempt this test exists for + "a#b", // fragment + "a?b", // query start + "a%2Fb", // already percent-encoded: must not be double-decoded + "a+b", // plus, which decodes to a space if mishandled + "ünïcøde", // non-ASCII + "", // unset + strings.Repeat("longer", 200), + } + for _, s := range seeds { + f.Add(s) + } + + // One server for the whole run, not one per input: a fuzz worker executes + // thousands of inputs, and a server each exhausts the ephemeral port range. + var ( + mu sync.Mutex + gotBranch string + gotProject string + branchPresent bool + ) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + q := r.URL.Query() + mu.Lock() + gotBranch = q.Get("branch") + gotProject = q.Get("project") + _, branchPresent = q["branch"] + mu.Unlock() + _ = json.NewEncoder(w).Encode(sonar.ProjectAnalyses{ + Analyses: []sonar.Analysis{ + {Key: revAnalysisKey, Date: revAnalysisDate, Revision: revRevision}, + }, + }) + })) + defer server.Close() + + f.Fuzz(func(t *testing.T, branch string) { + mu.Lock() + gotBranch, gotProject, branchPresent = "", "", false + mu.Unlock() + + sonarResults := &sonar.SonarResults{ServerUrl: server.URL, Branch: &sonar.Branch{Name: branch}} + project := &sonar.Project{Key: revProjectKey} + + if _, err := sonar.GetProjectAnalysisFromRevision(http.DefaultClient, sonarResults, project, revRevision, discardLogger()); err != nil { + t.Fatalf("branch %q: unexpected error: %v", branch, err) + } + + mu.Lock() + sentBranch, sentProject, sawBranch := gotBranch, gotProject, branchPresent + mu.Unlock() + + if sentProject != revProjectKey { + t.Errorf("branch %q altered the project parameter: got %q, want %q", branch, sentProject, revProjectKey) + } + if branch == "" { + if sawBranch { + t.Errorf("empty branch must not be sent at all, got branch=%q", sentBranch) + } + return + } + if !sawBranch { + t.Fatalf("branch %q was not sent", branch) + } + if sentBranch != branch { + t.Errorf("branch was not round-tripped: sent %q, SonarQube received %q", branch, sentBranch) + } + }) +} From 092bbe21f48edf956a834ffd5e9860e8f7a9c88a Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 20 Aug 2026 19:33:50 +0100 Subject: [PATCH 05/13] test(sonar): select the CE task by analysis ID, not by being the only one The activity list spans every branch of a project, so the end-to-end 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. Mutation testing showed the single-task fake let both halves of GetTaskID's match condition be negated without any test noticing; both mutants are now killed. Also documents the CE task winning over the flag: --sonar-branch supplies a branch name only, and the branch type can only come from the task. Refs #1116 Co-Authored-By: Claude Opus 5 --- internal/sonar/sonar_branch_test.go | 43 +++++++++++++++++++++++------ internal/sonar/sonar_test.go | 1 + 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/internal/sonar/sonar_branch_test.go b/internal/sonar/sonar_branch_test.go index 7f3329e2c..343fc2d38 100644 --- a/internal/sonar/sonar_branch_test.go +++ b/internal/sonar/sonar_branch_test.go @@ -40,16 +40,30 @@ func (f *fakeBranchSonar) handler() http.HandlerFunc { } _ = json.NewEncoder(w).Encode(resp) case "/api/ce/activity": + // The activity list spans every branch of the project, so it also holds + // tasks for other analyses. The decoy comes first: the task we want has to + // be selected by analysis ID, not by being the only one there. _ = json.NewEncoder(w).Encode(sonar.ActivityResponse{ - Tasks: []sonar.Task{{ - TaskID: "AaAeAfTdP27JeOuKOyce", - ComponentName: "customer project", - ComponentKey: revProjectKey, - AnalysisID: revAnalysisKey, - Status: "SUCCESS", - Branch: f.taskBranch, - BranchType: f.taskBranchType, - }}, + Tasks: []sonar.Task{ + { + TaskID: "DECOY", + ComponentName: "customer project", + ComponentKey: revProjectKey, + AnalysisID: "SOME_OTHER_ANALYSIS", + Status: "FAILED", + Branch: revMainBranch, + BranchType: "LONG", + }, + { + TaskID: revTaskID, + ComponentName: "customer project", + ComponentKey: revProjectKey, + AnalysisID: revAnalysisKey, + Status: "SUCCESS", + Branch: f.taskBranch, + BranchType: f.taskBranchType, + }, + }, }) case "/api/qualitygates/project_status": _ = json.NewEncoder(w).Encode(sonar.QualityGateResponse{ @@ -101,6 +115,17 @@ func TestGetSonarResults_ProjectKeyPath_WithBranch(t *testing.T) { if results.QualityGate == nil || results.QualityGate.Status != "OK" { t.Fatalf("expected quality gate OK, got %+v", results.QualityGate) } + if results.TaskID != revTaskID { + t.Errorf("expected the task for analysis %s, got TaskID %q", revAnalysisKey, results.TaskID) + } + if results.Status != "SUCCESS" { + t.Errorf("expected status SUCCESS from the matched task, got %q", results.Status) + } + // The CE task is authoritative once found, and it is the only source of the + // branch type — the flag supplies a name only. + if results.Branch.Type != "LONG" { + t.Errorf("expected branch type from the CE task, got %q", results.Branch.Type) + } } // TestGetSonarResults_ProjectKeyPath_WithoutBranch is the other half of the same diff --git a/internal/sonar/sonar_test.go b/internal/sonar/sonar_test.go index 3a2cc08f2..adc7f5736 100644 --- a/internal/sonar/sonar_test.go +++ b/internal/sonar/sonar_test.go @@ -111,6 +111,7 @@ const ( revMainBranch = "master" revFeatureBranch = "release/uat" revAnalysisKey = "AaAeAfTdP27JeOuKOycd" + revTaskID = "AaAeAfTdP27JeOuKOyce" revAnalysisDate = "2026-08-20T11:09:48+0400" revRevision = "8700f236fe2fd6c3e2dc5bf33c7e5f3aa8fd3dee" ) From 20cacfd97a0f86cfed2264a02a10bbfd0fc9d768 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 20 Aug 2026 20:02:58 +0100 Subject: [PATCH 06/13] test(sonar): extend the empty-flag audit to cover --sonar-branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI's TestEmptyFlagAuditCoversEveryCommandAndFlag caught the new flag as unaudited, which is what that test is for. Regenerated the coverage file with UPDATE_AUDIT_COVERAGE=1 as it documents (one line added, no churn), and added the flag to spec.json's attest sonar entry — audit.py hard-stops until the spec covers every combination, so without it the tool would refuse to run for the next person. The audit itself needs a local server and has not been re-run, so results.tsv is one row short of the current CLI until someone runs it. Refs #1116 Co-Authored-By: Claude Opus 5 --- cmd/kosli/testdata/empty-flag-audit-coverage.json | 1 + hack/empty-flag-audit/spec.json | 2 ++ 2 files changed, 3 insertions(+) diff --git a/cmd/kosli/testdata/empty-flag-audit-coverage.json b/cmd/kosli/testdata/empty-flag-audit-coverage.json index 73765f822..160179dea 100644 --- a/cmd/kosli/testdata/empty-flag-audit-coverage.json +++ b/cmd/kosli/testdata/empty-flag-audit-coverage.json @@ -459,6 +459,7 @@ "repo-url": "string", "repository": "string", "sonar-api-token": "string", + "sonar-branch": "string", "sonar-ce-task-url": "string", "sonar-project-key": "string", "sonar-revision": "string", diff --git a/hack/empty-flag-audit/spec.json b/hack/empty-flag-audit/spec.json index f828471e4..03586c647 100644 --- a/hack/empty-flag-audit/spec.json +++ b/hack/empty-flag-audit/spec.json @@ -1177,6 +1177,7 @@ "repo-url", "repository", "sonar-api-token", + "sonar-branch", "sonar-ce-task-url", "sonar-project-key", "sonar-revision", @@ -1211,6 +1212,7 @@ "repo-url": "http://example.com", "repository": "probe-repository", "sonar-api-token": "probe-sonar-api-token", + "sonar-branch": "probe-sonar-branch", "sonar-ce-task-url": "probe-sonar-ce-task-url", "sonar-project-key": "probe-sonar-project-key", "sonar-revision": "probe-sonar-revision", From c50a9c5c8dd192ce9fe2d06156b20575d372b168 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 20 Aug 2026 20:30:56 +0100 Subject: [PATCH 07/13] fix(sonar): keep the supplied branch out of PR payloads and out of silence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four points from review, each red-first: - A branch is no longer set alongside a pull request. The CLI blocks that flag combination, but the library could still produce it, because the branch is only cleared when GetTaskID matches a task and api/ce/activity is a bounded recent-activity list that need not contain one. - The supplied branch now survives a task that reports no branch. SonarQube omits it for main-branch tasks and older self-hosted Servers omit it more widely, so a run could use --sonar-branch to find the analysis and still publish an attestation without it. The task stays authoritative for the type. - GetTaskID warns when no task matched, instead of leaving TaskID and Status empty without a word — the same silent gap this PR exists to remove. - --sonar-branch warns when it is ignored, on the report-task.txt and --sonar-ce-task-url paths where the branch comes from the scan task. No mutual exclusion with --sonar-ce-task-url: this command already ignores --sonar-project-key, --sonar-revision and --sonar-server-url when scanner metadata exists (case 14), so erroring on only the new flag would be inconsistent. The not-found error no longer asks the user to check a branch they never gave. api/ce/activity is deliberately not scoped by branch: SonarQube's own client sends no such parameter there (branch exists on api/ce/analysis_status), and its tasks carry branch per row, so it spans branches by design. Forwarding one would have been an unknown parameter for SonarQube to reject. Mutation testing: all seven mutants on the new guards killed; package efficacy 67.96% -> 74.34%, mutator coverage 88.03% -> 91.13%. Refs #1116 Co-Authored-By: Claude Opus 5 --- cmd/kosli/attestSonar_test.go | 16 +-- internal/sonar/sonar.go | 37 +++++- internal/sonar/sonar_branch_test.go | 170 +++++++++++++++++++++++----- internal/sonar/sonar_test.go | 1 + 4 files changed, 185 insertions(+), 39 deletions(-) diff --git a/cmd/kosli/attestSonar_test.go b/cmd/kosli/attestSonar_test.go index 31a4f1235..1a05440bb 100644 --- a/cmd/kosli/attestSonar_test.go +++ b/cmd/kosli/attestSonar_test.go @@ -182,7 +182,7 @@ func (suite *AttestSonarCommandTestSuite) TestAttestSonarCmd() { wantError: true, name: "16 if incorrect revision given (or the scan for the given revision has been deleted by SonarCloud)", cmd: fmt.Sprintf("attest sonar --name cli.foo --commit HEAD --origin-url http://www.example.com --sonar-project-key cyber-dojo_differ --sonar-revision b4d1053f2aac18c9fb4b9a289a8289199c932e12 %s", suite.defaultKosliArguments), - golden: "Error: analysis for revision b4d1053f2aac18c9fb4b9a289a8289199c932e12 of project cyber-dojo_differ not found: only the project's main branch was searched, because no --sonar-branch was given. Check the revision and the branch are correct. \nThe scan may still be being processed by SonarQube, try again later.\n Otherwise if you are attesting an older scan, the snapshot may also have been deleted by SonarQube\n", + golden: "Error: analysis for revision b4d1053f2aac18c9fb4b9a289a8289199c932e12 of project cyber-dojo_differ not found: only the project's main branch was searched, because no --sonar-branch was given. Check the revision is correct, and pass --sonar-branch if the scan ran on another branch. \nThe scan may still be being processed by SonarQube, try again later.\n Otherwise if you are attesting an older scan, the snapshot may also have been deleted by SonarQube\n", }, { wantError: true, @@ -251,18 +251,18 @@ func (suite *AttestSonarCommandTestSuite) TestAttestSonarCmd() { cmd: fmt.Sprintf("attest sonar --name cli.foo --commit HEAD --origin-url http://www.example.com --sonar-ce-task-url https://sonarcloud.io/api/ce/task?id=AZERk4uWpzGpahwkB9ac %s", suite.defaultKosliArguments), golden: "Error: No activity found for task 'AZERk4uWpzGpahwkB9ac' on https://sonarcloud.io. \nSonarQube may be experiencing problems, please check https://status.sonarqube.com/ and try again later. \nOtherwise if you are attesting an older scan, the snapshot may have been deleted by SonarQube\n", }, - { - wantError: true, - name: "30 can't provide both sonar-branch and pull-request", - cmd: fmt.Sprintf("attest sonar --name cli.foo --commit HEAD --origin-url http://www.example.com --sonar-project-key cyber-dojo_differ --sonar-branch release/uat --pull-request 5 %s", suite.defaultKosliArguments), - golden: "Error: only one of --sonar-branch, --pull-request is allowed\n", - }, { wantError: true, name: "29 fails when --name has invalid dot format", cmd: fmt.Sprintf("attest sonar --name .foo %s", suite.defaultKosliArguments), golden: "Error: failed to parse attestation name: invalid attestation name format: .foo\n", }, + { + wantError: true, + name: "30 can't provide both sonar-branch and pull-request", + cmd: fmt.Sprintf("attest sonar --name cli.foo --commit HEAD --origin-url http://www.example.com --sonar-project-key cyber-dojo_differ --sonar-branch release/uat --pull-request 5 %s", suite.defaultKosliArguments), + golden: "Error: only one of --sonar-branch, --pull-request is allowed\n", + }, } runTestCmd(suite.T(), tests) @@ -345,7 +345,7 @@ func (suite *AttestSonarQubeCommandTestSuite) TestAttestSonarQubeCmd() { wantError: true, name: "113 if incorrect revision given, give an error", cmd: fmt.Sprintf("attest sonar --name cli.foo --commit HEAD --origin-url http://www.example.com --sonar-server-url http://localhost:9000 --sonar-project-key test5 --sonar-revision 8e6f9489e5f2ddf8e719b503e374975e8b607fd2 %s", suite.defaultKosliArguments), - golden: "Error: analysis for revision 8e6f9489e5f2ddf8e719b503e374975e8b607fd2 of project test5 not found: only the project's main branch was searched, because no --sonar-branch was given. Check the revision and the branch are correct. \nThe scan may still be being processed by SonarQube, try again later.\n Otherwise if you are attesting an older scan, the snapshot may also have been deleted by SonarQube\n", + golden: "Error: analysis for revision 8e6f9489e5f2ddf8e719b503e374975e8b607fd2 of project test5 not found: only the project's main branch was searched, because no --sonar-branch was given. Check the revision is correct, and pass --sonar-branch if the scan ran on another branch. \nThe scan may still be being processed by SonarQube, try again later.\n Otherwise if you are attesting an older scan, the snapshot may also have been deleted by SonarQube\n", }, { wantError: true, diff --git a/internal/sonar/sonar.go b/internal/sonar/sonar.go index b16d16b58..ab7cf4a02 100644 --- a/internal/sonar/sonar.go +++ b/internal/sonar/sonar.go @@ -201,8 +201,9 @@ func (sc *SonarConfig) GetSonarResults(logger *log.Logger) (*SonarResults, error sonarResults.Revision = sc.revision // On this path there is no CE task to read the branch from, so the branch // can only come from the user (#1116). Set it on the results, which is the - // one mechanism both analyses lookups use to scope their search. - if sc.branch != "" { + // one mechanism both analyses lookups use to scope their search. A pull + // request scan is not a branch scan, so the branch is not carried there. + if sc.branch != "" && sc.pullRequest == "" { sonarResults.Branch = &Branch{Name: sc.branch} } project.Url, err = sonarURL(sonarResults.ServerUrl, "dashboard", url.Values{"id": {project.Key}}) @@ -220,10 +221,25 @@ func (sc *SonarConfig) GetSonarResults(logger *log.Logger) (*SonarResults, error if err != nil { return nil, err } + // GetTaskID rewrites the branch from the matched task, and clears it when + // the task reports none — which SonarQube does for main-branch tasks, and + // older self-hosted Servers do more widely. The task is authoritative for + // the branch type, but it must not delete the branch the user gave us to + // find the analysis with (#1116). + if sc.branch != "" && sonarResults.PullRequest == "" && + (sonarResults.Branch == nil || sonarResults.Branch.Name == "") { + sonarResults.Branch = &Branch{Name: sc.branch} + } } } if analysisID == "" && sc.CETaskUrl != "" { + // Here the scan is identified by report-task.txt or --sonar-ce-task-url, and + // the branch is read from the scan task, so a supplied branch reaches nothing. + // Say so rather than ignoring it in silence. + if sc.branch != "" { + logger.Warn("--sonar-branch is ignored when the scan is identified by report-task.txt or --sonar-ce-task-url: the branch is read from the scan task") + } //Get the analysis ID, status, project name and branch data from the ceTaskURL (ce API) analysisID, err = GetCETaskData(httpClient, project, sonarResults, sc.CETaskUrl, sc.maxWait, logger) if err != nil { @@ -436,10 +452,12 @@ func GetProjectAnalysisFromRevision(httpClient *http.Client, sonarResults *Sonar // An empty result reads like a permissions problem, so say which branch was // actually searched: unscoped, SonarQube only searches the main branch (#1116). scope := "only the project's main branch was searched, because no --sonar-branch was given" + advice := "Check the revision is correct, and pass --sonar-branch if the scan ran on another branch." if sonarResults.Branch != nil && sonarResults.Branch.Name != "" { scope = fmt.Sprintf("branch %s was searched", sonarResults.Branch.Name) + advice = "Check the revision and the branch are correct." } - return "", fmt.Errorf("analysis for revision %s of project %s not found: %s. Check the revision and the branch are correct. \nThe scan may still be being processed by SonarQube, try again later.\n Otherwise if you are attesting an older scan, the snapshot may also have been deleted by SonarQube", revision, project.Key, scope) + return "", fmt.Errorf("analysis for revision %s of project %s not found: %s. %s \nThe scan may still be being processed by SonarQube, try again later.\n Otherwise if you are attesting an older scan, the snapshot may also have been deleted by SonarQube", revision, project.Key, scope, advice) } return analysisID, nil @@ -609,11 +627,13 @@ func GetTaskID(httpClient *http.Client, sonarResults *SonarResults, project *Pro return sonarResponseError(CEActivityResponse.StatusCode) } + matchedTask := false for t := range CEActivityData.Tasks { task := CEActivityData.Tasks[t] matched := (analysisID != "" && task.AnalysisID == analysisID) || (analysisID == "" && sonarResults.PullRequest != "" && task.PullRequest == sonarResults.PullRequest) if matched { + matchedTask = true sonarResults.TaskID = task.TaskID sonarResults.Status = task.Status project.Name = task.ComponentName @@ -631,5 +651,16 @@ func GetTaskID(httpClient *http.Client, sonarResults *SonarResults, project *Pro } } + // api/ce/activity is a bounded recent-activity list and takes no branch + // parameter, so the task may simply not be there. Without this the attestation + // is published with no task ID and no status, and nothing says why. + if !matchedTask { + sought := fmt.Sprintf("analysis %s", analysisID) + if analysisID == "" { + sought = fmt.Sprintf("pull request %s", sonarResults.PullRequest) + } + logger.Warn("no SonarQube compute engine task found for %s of project %s: the attestation will carry no task ID or scan status", sought, project.Key) + } + return nil } diff --git a/internal/sonar/sonar_branch_test.go b/internal/sonar/sonar_branch_test.go index 343fc2d38..ad595f22e 100644 --- a/internal/sonar/sonar_branch_test.go +++ b/internal/sonar/sonar_branch_test.go @@ -1,15 +1,25 @@ package sonar_test import ( + "bytes" "encoding/json" "net/http" "net/http/httptest" + "strings" "sync" "testing" + "github.com/kosli-dev/cli/internal/logger" "github.com/kosli-dev/cli/internal/sonar" ) +// bufferLogger returns a logger whose warnings can be asserted on. Warn writes to +// the error stream. +func bufferLogger() (*logger.Logger, *bytes.Buffer) { + stderr := &bytes.Buffer{} + return logger.NewLogger(&bytes.Buffer{}, stderr, false), stderr +} + // fakeBranchSonar is a SonarQube stand-in for the project-key/revision path of // issue #1116. It models the customer's project: the analysis for the revision // exists only on release/uat, so api/project_analyses/search returns it only when @@ -20,6 +30,9 @@ type fakeBranchSonar struct { searchProjects []string // project param of each project_analyses/search request taskBranch string // branch reported on the ce/activity task taskBranchType string + // omitMatchingTask serves activity without the task for our analysis, as a + // bounded recent-activity list eventually does. + omitMatchingTask bool } func (f *fakeBranchSonar) handler() http.HandlerFunc { @@ -43,27 +56,37 @@ func (f *fakeBranchSonar) handler() http.HandlerFunc { // The activity list spans every branch of the project, so it also holds // tasks for other analyses. The decoy comes first: the task we want has to // be selected by analysis ID, not by being the only one there. - _ = json.NewEncoder(w).Encode(sonar.ActivityResponse{ - Tasks: []sonar.Task{ - { - TaskID: "DECOY", - ComponentName: "customer project", - ComponentKey: revProjectKey, - AnalysisID: "SOME_OTHER_ANALYSIS", - Status: "FAILED", - Branch: revMainBranch, - BranchType: "LONG", - }, - { - TaskID: revTaskID, - ComponentName: "customer project", - ComponentKey: revProjectKey, - AnalysisID: revAnalysisKey, - Status: "SUCCESS", - Branch: f.taskBranch, - BranchType: f.taskBranchType, - }, + tasks := []sonar.Task{ + { + TaskID: "DECOY", + ComponentName: "customer project", + ComponentKey: revProjectKey, + AnalysisID: "SOME_OTHER_ANALYSIS", + Status: "FAILED", + Branch: revMainBranch, + BranchType: "LONG", }, + } + if !f.omitMatchingTask { + tasks = append(tasks, sonar.Task{ + TaskID: revTaskID, + ComponentName: "customer project", + ComponentKey: revProjectKey, + AnalysisID: revAnalysisKey, + Status: "SUCCESS", + Branch: f.taskBranch, + BranchType: f.taskBranchType, + }) + } + _ = json.NewEncoder(w).Encode(sonar.ActivityResponse{Tasks: tasks}) + case "/api/project_pull_requests/list": + _ = json.NewEncoder(w).Encode(sonar.PullRequestsResponse{ + PullRequests: []sonar.PullRequestInfo{{ + Key: revPullRequest, + Branch: "feature/whatever", + AnalysisDate: revAnalysisDate, + Commit: sonar.PRCommit{SHA: revRevision}, + }}, }) case "/api/qualitygates/project_status": _ = json.NewEncoder(w).Encode(sonar.QualityGateResponse{ @@ -90,11 +113,17 @@ func TestGetSonarResults_ProjectKeyPath_WithBranch(t *testing.T) { srv := httptest.NewServer(fake.handler()) defer srv.Close() + log, stderr := bufferLogger() sc := sonar.NewSonarConfig("tok", t.TempDir(), "", revProjectKey, srv.URL, revRevision, "", revFeatureBranch, 5) - results, err := sc.GetSonarResults(discardLogger()) + results, err := sc.GetSonarResults(log) if err != nil { t.Fatalf("expected the scan on %s to be found, got error: %v", revFeatureBranch, err) } + // The happy path warns about nothing: neither an unmatched task nor an ignored + // flag, both of which are warned about elsewhere. + if stderr.Len() != 0 { + t.Errorf("expected no warnings on the happy path, got stderr: %q", stderr.String()) + } branches, projects := fake.searches() if len(branches) != 1 || branches[0] != revFeatureBranch { @@ -148,20 +177,105 @@ func TestGetSonarResults_ProjectKeyPath_WithoutBranch(t *testing.T) { } } -// TestGetSonarResults_BranchIgnoredForPullRequest documents that a PR scan is not a -// branch scan: with --pull-request the branch is not used to scope any search. -// The two flags are mutually exclusive at the CLI, so this only pins the library. +// TestGetSonarResults_BranchIgnoredForPullRequest is the invalid state the guard +// exists to prevent: a PR scan is not a branch scan, so an attestation must not +// carry both. The CLI blocks the flag combination, but nothing stopped the +// library from producing it — and the CE task only clears the branch when it +// matches a task, which api/ce/activity (a bounded recent-activity list) need +// not contain. func TestGetSonarResults_BranchIgnoredForPullRequest(t *testing.T) { fake := &fakeBranchSonar{} srv := httptest.NewServer(fake.handler()) defer srv.Close() - sc := sonar.NewSonarConfig("tok", t.TempDir(), "", revProjectKey, srv.URL, "", "42", revFeatureBranch, 5) - // The PR lookup is not served by this fake, so this fails; what matters is that - // no branch-scoped analyses search was made on the way. - _, _ = sc.GetSonarResults(discardLogger()) + sc := sonar.NewSonarConfig("tok", t.TempDir(), "", revProjectKey, srv.URL, "", revPullRequest, revFeatureBranch, 5) + results, err := sc.GetSonarResults(discardLogger()) + if err != nil { + t.Fatalf("expected the pull-request scan to be found, got error: %v", err) + } if branches, _ := fake.searches(); len(branches) != 0 { t.Errorf("expected no project_analyses/search on the pull-request path, got %v", branches) } + if results.PullRequest != revPullRequest { + t.Errorf("expected pull request %q, got %q", revPullRequest, results.PullRequest) + } + if results.Branch != nil { + t.Errorf("expected no branch alongside a pull request, got %+v", results.Branch) + } +} + +// TestGetSonarResults_TaskWithoutBranch_KeepsSuppliedBranch pins which side wins +// when the CE task reports no branch at all — SonarQube omits it for main-branch +// tasks, and older self-hosted Servers omit it more widely. The task is +// authoritative for the branch type, but it must not delete the branch the user +// gave us to find the analysis with. +func TestGetSonarResults_TaskWithoutBranch_KeepsSuppliedBranch(t *testing.T) { + fake := &fakeBranchSonar{taskBranch: "", taskBranchType: ""} + srv := httptest.NewServer(fake.handler()) + defer srv.Close() + + sc := sonar.NewSonarConfig("tok", t.TempDir(), "", revProjectKey, srv.URL, revRevision, "", revFeatureBranch, 5) + results, err := sc.GetSonarResults(discardLogger()) + if err != nil { + t.Fatalf("expected the scan to be found, got error: %v", err) + } + + if results.Branch == nil || results.Branch.Name != revFeatureBranch { + t.Fatalf("expected the supplied branch %q to survive a task that reports none, got %+v", revFeatureBranch, results.Branch) + } + if results.Branch.Type != "" { + t.Errorf("expected no branch type when the task reports none, got %q", results.Branch.Type) + } + if results.TaskID != revTaskID { + t.Errorf("expected the task to still be matched, got TaskID %q", results.TaskID) + } +} + +// TestGetSonarResults_NoMatchingTask_Warns covers the silent gap: api/ce/activity +// cannot be scoped to a branch and is a bounded recent-activity list, so it may +// simply not hold the task. The attestation is then published with no task ID +// and no scan status, which used to happen without a word. +func TestGetSonarResults_NoMatchingTask_Warns(t *testing.T) { + fake := &fakeBranchSonar{taskBranch: revFeatureBranch, taskBranchType: "LONG", omitMatchingTask: true} + srv := httptest.NewServer(fake.handler()) + defer srv.Close() + + log, stderr := bufferLogger() + sc := sonar.NewSonarConfig("tok", t.TempDir(), "", revProjectKey, srv.URL, revRevision, "", revFeatureBranch, 5) + results, err := sc.GetSonarResults(log) + if err != nil { + t.Fatalf("expected the scan to be found, got error: %v", err) + } + + if results.TaskID != "" { + t.Errorf("expected no task ID when no task matched, got %q", results.TaskID) + } + if !strings.Contains(stderr.String(), "no SonarQube compute engine task") { + t.Errorf("expected a warning that no task matched, got stderr: %q", stderr.String()) + } + // The branch still has to survive: it is what found the analysis. + if results.Branch == nil || results.Branch.Name != revFeatureBranch { + t.Errorf("expected the supplied branch to survive, got %+v", results.Branch) + } +} + +// TestGetSonarResults_BranchIgnoredOnCETaskPath_Warns covers the flag being a +// no-op: identified by report-task.txt or --sonar-ce-task-url, the branch comes +// from the scan task and --sonar-branch reaches nothing. Silently ignoring it is +// the same class of unexplained outcome this flag exists to remove. +func TestGetSonarResults_BranchIgnoredOnCETaskPath_Warns(t *testing.T) { + fake := &fakeSonar{acceptsBearer: true, acceptsBasic: true} + srv := httptest.NewServer(fake.handler()) + defer srv.Close() + + log, stderr := bufferLogger() + sc := sonar.NewSonarConfig("tok", t.TempDir(), srv.URL+"/api/ce/task?id=AYx", "", "", "", "", revFeatureBranch, 5) + if _, err := sc.GetSonarResults(log); err != nil { + t.Fatalf("expected the CE task path to succeed, got error: %v", err) + } + + if !strings.Contains(stderr.String(), "--sonar-branch is ignored") { + t.Errorf("expected a warning that --sonar-branch is ignored on this path, got stderr: %q", stderr.String()) + } } diff --git a/internal/sonar/sonar_test.go b/internal/sonar/sonar_test.go index adc7f5736..fc1dbb73f 100644 --- a/internal/sonar/sonar_test.go +++ b/internal/sonar/sonar_test.go @@ -112,6 +112,7 @@ const ( revFeatureBranch = "release/uat" revAnalysisKey = "AaAeAfTdP27JeOuKOycd" revTaskID = "AaAeAfTdP27JeOuKOyce" + revPullRequest = "42" revAnalysisDate = "2026-08-20T11:09:48+0400" revRevision = "8700f236fe2fd6c3e2dc5bf33c7e5f3aa8fd3dee" ) From 3832c1c991b24552e1b3e53eb21424756c35b689 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 20 Aug 2026 20:44:14 +0100 Subject: [PATCH 08/13] fix(sonar): warn about the payload, not about the lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught the warning added in c50a9c5c firing ahead of an error: case 27 gives a pull-request ID that does not exist, so no compute engine task matches, and the run printed "the attestation will carry no task ID" immediately before "pull request 1 not found" — noise ahead of a better message, about an attestation that was never going to be published. Moved it to the successful return, conditioned on the payload actually having no task ID. Errors return before it, so it can no longer precede one, and it now describes what is being returned rather than what one lookup did. The failing case is pinned as a unit test, so the next person does not need CI to find it. Refs #1116 Co-Authored-By: Claude Opus 5 --- internal/sonar/sonar.go | 22 +++++++++------------- internal/sonar/sonar_branch_test.go | 21 +++++++++++++++++++++ 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/internal/sonar/sonar.go b/internal/sonar/sonar.go index ab7cf4a02..2f3bc1006 100644 --- a/internal/sonar/sonar.go +++ b/internal/sonar/sonar.go @@ -273,6 +273,15 @@ func (sc *SonarConfig) GetSonarResults(logger *log.Logger) (*SonarResults, error sonarResults.Project = *project sonarResults.QualityGate = qualityGate + // No task ID means api/ce/activity held no task for this scan: it is a bounded + // recent-activity list and takes no branch parameter, so an older scan can age + // out of it. The attestation is still valid, it just says less — and nothing + // used to say why. Warned here, on the way out, so it describes the payload + // being returned rather than appearing ahead of an error the caller reports. + if sonarResults.TaskID == "" { + logger.Warn("no SonarQube compute engine task was found for this scan of project %s: the attestation carries no task ID or scan status", project.Key) + } + return sonarResults, nil } @@ -627,13 +636,11 @@ func GetTaskID(httpClient *http.Client, sonarResults *SonarResults, project *Pro return sonarResponseError(CEActivityResponse.StatusCode) } - matchedTask := false for t := range CEActivityData.Tasks { task := CEActivityData.Tasks[t] matched := (analysisID != "" && task.AnalysisID == analysisID) || (analysisID == "" && sonarResults.PullRequest != "" && task.PullRequest == sonarResults.PullRequest) if matched { - matchedTask = true sonarResults.TaskID = task.TaskID sonarResults.Status = task.Status project.Name = task.ComponentName @@ -651,16 +658,5 @@ func GetTaskID(httpClient *http.Client, sonarResults *SonarResults, project *Pro } } - // api/ce/activity is a bounded recent-activity list and takes no branch - // parameter, so the task may simply not be there. Without this the attestation - // is published with no task ID and no status, and nothing says why. - if !matchedTask { - sought := fmt.Sprintf("analysis %s", analysisID) - if analysisID == "" { - sought = fmt.Sprintf("pull request %s", sonarResults.PullRequest) - } - logger.Warn("no SonarQube compute engine task found for %s of project %s: the attestation will carry no task ID or scan status", sought, project.Key) - } - return nil } diff --git a/internal/sonar/sonar_branch_test.go b/internal/sonar/sonar_branch_test.go index ad595f22e..7b507095a 100644 --- a/internal/sonar/sonar_branch_test.go +++ b/internal/sonar/sonar_branch_test.go @@ -279,3 +279,24 @@ func TestGetSonarResults_BranchIgnoredOnCETaskPath_Warns(t *testing.T) { t.Errorf("expected a warning that --sonar-branch is ignored on this path, got stderr: %q", stderr.String()) } } + +// TestGetSonarResults_NoTaskThenFailure_DoesNotWarn is the case CI caught: when +// the lookup goes on to fail, the command already says something specific and +// true, and a warning about an attestation that is never published is noise +// ahead of a better message. The warning describes the payload we are about to +// return, so it must not appear on a path that returns an error instead. +func TestGetSonarResults_NoTaskThenFailure_DoesNotWarn(t *testing.T) { + fake := &fakeBranchSonar{omitMatchingTask: true} + srv := httptest.NewServer(fake.handler()) + defer srv.Close() + + log, stderr := bufferLogger() + sc := sonar.NewSonarConfig("tok", t.TempDir(), "", revProjectKey, srv.URL, "", "99", "", 5) + if _, err := sc.GetSonarResults(log); err == nil { + t.Fatal("expected an error for a pull request that does not exist") + } + + if stderr.Len() != 0 { + t.Errorf("expected no warning ahead of the error, got stderr: %q", stderr.String()) + } +} From 201a2a950e1024b41027694ac3aa8509577a906b Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 20 Aug 2026 20:55:36 +0100 Subject: [PATCH 09/13] refactor(sonar): one place for the branch-scoping rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both analyses lookups had their own copy of "scope the search to the branch when we have one", added six months apart by the fixes for #861 and #1116. That duplication is why the second issue existed: the rule was fixed on one path and left broken on the other. Extracted analysesSearchURL so there is one copy to get right, and so FuzzBranchParam pins the encoding for both paths. Behaviour-preserving: no test changed, and mutation testing still kills both guard mutants, now at one site instead of two. Also from review: the comment above the branch-fallback said the task was authoritative for the branch *type*, which undersold it — the task's branch wins outright when it reports one. Comment now says what the code does. And categories.json gains sonar-branch as "identity", alongside sonar-project-key, sonar-revision and pull-request; report.py tolerates its absence, so this only keeps the generated table from reading as though nobody considered the flag. Refs #1116 Co-Authored-By: Claude Opus 5 --- hack/empty-flag-audit/categories.json | 1 + internal/sonar/sonar.go | 39 ++++++++++++++------------- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/hack/empty-flag-audit/categories.json b/hack/empty-flag-audit/categories.json index ee0679783..a5848b47d 100644 --- a/hack/empty-flag-audit/categories.json +++ b/hack/empty-flag-audit/categories.json @@ -136,6 +136,7 @@ "show-input": "output", "show-unchanged": "output", "sonar-api-token": "credentials", + "sonar-branch": "identity", "sonar-ce-task-url": "location", "sonar-project-key": "identity", "sonar-revision": "identity", diff --git a/internal/sonar/sonar.go b/internal/sonar/sonar.go index 2f3bc1006..570dbf0b2 100644 --- a/internal/sonar/sonar.go +++ b/internal/sonar/sonar.go @@ -160,6 +160,19 @@ func sonarURL(serverURL, apiPath string, params url.Values) (string, error) { return u.String(), nil } +// analysesSearchURL builds a project_analyses/search URL, scoped to the branch +// when there is one: SonarQube otherwise searches only the project's main branch, +// so an analysis on any other branch is invisible (#861, #1116). Both lookups +// share this so the rule cannot be fixed on one path and left broken on the other, +// which is how those two issues came to be six months apart. +func analysesSearchURL(sonarResults *SonarResults, project *Project) (string, error) { + params := url.Values{"project": {project.Key}} + if sonarResults.Branch != nil && sonarResults.Branch.Name != "" { + params.Set("branch", sonarResults.Branch.Name) + } + return sonarURL(sonarResults.ServerUrl, "api/project_analyses/search", params) +} + func (sc *SonarConfig) GetSonarResults(logger *log.Logger) (*SonarResults, error) { var analysisID string var err error @@ -221,11 +234,11 @@ func (sc *SonarConfig) GetSonarResults(logger *log.Logger) (*SonarResults, error if err != nil { return nil, err } - // GetTaskID rewrites the branch from the matched task, and clears it when - // the task reports none — which SonarQube does for main-branch tasks, and - // older self-hosted Servers do more widely. The task is authoritative for - // the branch type, but it must not delete the branch the user gave us to - // find the analysis with (#1116). + // The task's own branch wins when it reports one — it is the scan's own + // record, reached via an analysis ID the branch-scoped search returned. + // But it must not delete the branch the user gave us when it reports none, + // which SonarQube does for main-branch tasks and older self-hosted Servers + // do more widely (#1116). if sc.branch != "" && sonarResults.PullRequest == "" && (sonarResults.Branch == nil || sonarResults.Branch.Name == "") { sonarResults.Branch = &Branch{Name: sc.branch} @@ -413,14 +426,7 @@ func GetCETaskData(httpClient *http.Client, project *Project, sonarResults *Sona func GetProjectAnalysisFromRevision(httpClient *http.Client, sonarResults *SonarResults, project *Project, revision string, logger *log.Logger) (string, error) { var analysisID string - // Forward branch to search analyses on non-default branches (#1116). SonarQube - // defaults this endpoint to the project's main branch, so without it a scan on - // any other branch is invisible here. - params := url.Values{"project": {project.Key}} - if sonarResults.Branch != nil && sonarResults.Branch.Name != "" { - params.Set("branch", sonarResults.Branch.Name) - } - projectAnalysesURL, err := sonarURL(sonarResults.ServerUrl, "api/project_analyses/search", params) + projectAnalysesURL, err := analysesSearchURL(sonarResults, project) if err != nil { return "", err } @@ -473,12 +479,7 @@ func GetProjectAnalysisFromRevision(httpClient *http.Client, sonarResults *Sonar } func GetProjectAnalysisFromAnalysisID(httpClient *http.Client, sonarResults *SonarResults, project *Project, analysisID string) error { - // Forward branch to find analyses on non-default branches (#861). - params := url.Values{"project": {project.Key}} - if sonarResults.Branch != nil && sonarResults.Branch.Name != "" { - params.Set("branch", sonarResults.Branch.Name) - } - projectAnalysesURL, err := sonarURL(sonarResults.ServerUrl, "api/project_analyses/search", params) + projectAnalysesURL, err := analysesSearchURL(sonarResults, project) if err != nil { return err } From f44ff094347109ee99f0f2f578f290e170f438a0 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 20 Aug 2026 21:06:43 +0100 Subject: [PATCH 10/13] fix(sonar): warn about a missing task only when we have an analysis ID MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review changed my mind on the scope of this warning. With an analysis ID in hand, SonarQube has just returned that analysis, so its compute engine task missing from api/ce/activity is surprising and worth saying. On the pull-request path there is no analysis ID and the match can only be on the PR key, which a recent-first, page-bounded list drops as a matter of course — so the warning was firing on the ordinary case, and by the comment above it that was the documented expectation. It also tied case 20 to SonarQube's activity window, since the warning is compared as part of the golden. Case 13 still is, and now says so. Rejected the alternative of demoting it to Debug: off by default means the one person who needs it is the one who will not see it. Also from review: a word on why reaching the CE-path block is what makes the ignored-flag warning true, since readFile populating sc.CETaskUrl is not visible from there. Refs #1116 Co-Authored-By: Claude Opus 5 --- cmd/kosli/attestSonar_test.go | 4 ++++ internal/sonar/sonar.go | 23 ++++++++++++++++------- internal/sonar/sonar_branch_test.go | 25 +++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 7 deletions(-) diff --git a/cmd/kosli/attestSonar_test.go b/cmd/kosli/attestSonar_test.go index 1a05440bb..efba7abf9 100644 --- a/cmd/kosli/attestSonar_test.go +++ b/cmd/kosli/attestSonar_test.go @@ -163,6 +163,10 @@ func (suite *AttestSonarCommandTestSuite) TestAttestSonarCmd() { golden: "Error: open .scannerwork/report-task.txt: no such file or directory. Check your working directory is set correctly. Alternatively provide the project key and either revision or pull-request ID for the scan to attest\n", }, { + // Depends on SonarCloud still holding a compute engine task for the latest + // analysis: the CLI warns when it does not, and that warning is compared + // as part of the golden. getLatestAnalysisRevision picks the newest + // analysis, so its task is at the top of api/ce/activity. name: "13 can retrieve scan results using project key and revision and attest them", cmd: fmt.Sprintf("attest sonar --name cli.foo --commit HEAD --origin-url http://www.example.com --sonar-project-key cyber-dojo_differ --sonar-revision %s %s", suite.mainRevision, suite.defaultKosliArguments), golden: "sonar attestation 'foo' is reported to trail: test-123\n", diff --git a/internal/sonar/sonar.go b/internal/sonar/sonar.go index 570dbf0b2..cf81629c7 100644 --- a/internal/sonar/sonar.go +++ b/internal/sonar/sonar.go @@ -250,6 +250,11 @@ func (sc *SonarConfig) GetSonarResults(logger *log.Logger) (*SonarResults, error // Here the scan is identified by report-task.txt or --sonar-ce-task-url, and // the branch is read from the scan task, so a supplied branch reaches nothing. // Say so rather than ignoring it in silence. + // + // Reaching this block is what makes that true: readFile puts the file's + // ceTaskUrl on sc.CETaskUrl, so both ways of naming a scan by its task arrive + // here, and an empty analysisID means the project-key path did not already + // resolve one. if sc.branch != "" { logger.Warn("--sonar-branch is ignored when the scan is identified by report-task.txt or --sonar-ce-task-url: the branch is read from the scan task") } @@ -286,13 +291,17 @@ func (sc *SonarConfig) GetSonarResults(logger *log.Logger) (*SonarResults, error sonarResults.Project = *project sonarResults.QualityGate = qualityGate - // No task ID means api/ce/activity held no task for this scan: it is a bounded - // recent-activity list and takes no branch parameter, so an older scan can age - // out of it. The attestation is still valid, it just says less — and nothing - // used to say why. Warned here, on the way out, so it describes the payload - // being returned rather than appearing ahead of an error the caller reports. - if sonarResults.TaskID == "" { - logger.Warn("no SonarQube compute engine task was found for this scan of project %s: the attestation carries no task ID or scan status", project.Key) + // No task ID means api/ce/activity held no task for this scan. Warned here, on + // the way out, so it describes the payload being returned rather than appearing + // ahead of an error the caller reports. + // + // Only when we have an analysis ID: SonarQube has just returned that analysis, + // so its task missing from the activity list is surprising and worth saying. On + // the pull-request path there is no analysis ID and the match can only be on the + // PR key, which a recent-first, page-bounded list drops as a matter of course — + // warning there would be noise about the ordinary case. + if analysisID != "" && sonarResults.TaskID == "" { + 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) } return sonarResults, nil diff --git a/internal/sonar/sonar_branch_test.go b/internal/sonar/sonar_branch_test.go index 7b507095a..dfaaed9f2 100644 --- a/internal/sonar/sonar_branch_test.go +++ b/internal/sonar/sonar_branch_test.go @@ -300,3 +300,28 @@ func TestGetSonarResults_NoTaskThenFailure_DoesNotWarn(t *testing.T) { t.Errorf("expected no warning ahead of the error, got stderr: %q", stderr.String()) } } + +// TestGetSonarResults_PullRequestNoTask_DoesNotWarn pins the scope of the +// missing-task warning. On the pull-request path GetTaskID can only match on the +// PR key, and api/ce/activity is recent-first and page-bounded, so a PR task +// ageing out is ordinary rather than surprising. Warning there would be noise, +// and would tie a passing test to SonarQube's activity window. +func TestGetSonarResults_PullRequestNoTask_DoesNotWarn(t *testing.T) { + fake := &fakeBranchSonar{} + srv := httptest.NewServer(fake.handler()) + defer srv.Close() + + log, stderr := bufferLogger() + sc := sonar.NewSonarConfig("tok", t.TempDir(), "", revProjectKey, srv.URL, "", revPullRequest, "", 5) + results, err := sc.GetSonarResults(log) + if err != nil { + t.Fatalf("expected the pull-request scan to be found, got error: %v", err) + } + + if results.TaskID != "" { + t.Fatalf("expected no task to have matched on the pull-request path, got %q", results.TaskID) + } + if stderr.Len() != 0 { + t.Errorf("expected no warning for an ordinary pull-request task miss, got stderr: %q", stderr.String()) + } +} From 195f34cac1d7b684d90d1ae9cdf6310ea64df1a5 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 20 Aug 2026 21:35:18 +0100 Subject: [PATCH 11/13] docs(sonar): --pull-request help names the --sonar-branch exclusion --sonar-branch documented the mutual exclusion from its side and --pull-request did not, so a user reading --help from the --pull-request entry found out by hitting the error. A help string I made wrong, missed in review replies rather than declined. Also records why the ignored-flag warning is deliberately emitted before the lookups that can fail, where the missing-task warning is not: "the flag you passed is ignored" is true whether or not the run succeeds, while a warning that describes the payload would be false on a run that never publishes one. Refs #1116 Co-Authored-By: Claude Opus 5 --- cmd/kosli/root.go | 2 +- internal/sonar/sonar.go | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/cmd/kosli/root.go b/cmd/kosli/root.go index db33b771b..0c8f89610 100644 --- a/cmd/kosli/root.go +++ b/cmd/kosli/root.go @@ -301,7 +301,7 @@ The ^.kosli_ignore^ will be treated as part of the artifact like any other file, sonarProjectKeyFlag = "[conditional] The project key of the SonarQube project. Only required if you want to use the project key/revision/pull-request to get the scan results rather than using Sonar's metadata file." sonarServerURLFlag = "[conditional] The URL of your SonarQube server. Only required if you are using SonarQube Server and not using SonarQube's metadata file to get scan results." sonarRevisionFlag = "[conditional] The revision of the SonarQube project. Only required if you want to use the project key/revision to get the scan results rather than using Sonar's metadata file and you have overridden the default revision, or you aren't using a CI. Defaults to the value of the git commit flag. Cannot be used with --pull-request." - sonarPRFlag = "[conditional] The ID of the pull-request. Only required if you want to use the project key/pull-request to get the scan results rather than using Sonar's metadata file. Cannot be used with --sonar-revision." + sonarPRFlag = "[conditional] The ID of the pull-request. Only required if you want to use the project key/pull-request to get the scan results rather than using Sonar's metadata file. Cannot be used with --sonar-revision or --sonar-branch." sonarBranchFlag = "[conditional] The name of the branch the SonarQube scan ran on. Only required if you are using the project key/revision to get the scan results and the scan ran on a branch other than the project's main branch in SonarQube. Cannot be used with --pull-request." sonarMaxWaitFlag = "[optional] Allow the command to wait and retry fetching the scan results from SonarQube, up to the maximum number of seconds provided, with exponential backoff. Useful when using SonarQube's metadata file to retrieve and attest scans that take a long time to process . Defaults to 30 seconds." sonarCETaskURLFlag = "[conditional] The URL of the SonarQube CE task. Can be used instead of --sonar-working-dir when the report-task.txt file is not accessible, e.g. due to container isolation in CI/CD pipelines." diff --git a/internal/sonar/sonar.go b/internal/sonar/sonar.go index cf81629c7..7b38a5f2b 100644 --- a/internal/sonar/sonar.go +++ b/internal/sonar/sonar.go @@ -255,6 +255,12 @@ func (sc *SonarConfig) GetSonarResults(logger *log.Logger) (*SonarResults, error // ceTaskUrl on sc.CETaskUrl, so both ways of naming a scan by its task arrive // here, and an empty analysisID means the project-key path did not already // resolve one. + // + // Unlike the missing-task warning at the end of this function, this one is + // deliberately emitted before the lookups that can fail. "The flag you passed + // is ignored" is true whether or not the run then succeeds, and is worth + // saying either way; the other warning describes a payload, so it would be a + // false statement on a run that never publishes one. if sc.branch != "" { logger.Warn("--sonar-branch is ignored when the scan is identified by report-task.txt or --sonar-ce-task-url: the branch is read from the scan task") } From 216a4379a3a2424be71c28ce43729037125184ef Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Fri, 21 Aug 2026 06:18:21 +0100 Subject: [PATCH 12/13] fix(sonar): name the project name the unmatched task also costs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The missing-task warning listed task ID and status. project.Name is assigned only inside GetTaskID's match block, and nothing else sets it on the project-key path, so the payload loses that too. Both tests now assert it from their own side, so the claim in the message stays tied to something. Also one spelling for one condition: the pull-request guard read sc.pullRequest while its sibling twenty lines down reads sonarResults.PullRequest. Both were correct, and the difference is load-bearing in the later one — which is exactly why the earlier one reading differently invites unifying it the wrong way. Refs #1116 Co-Authored-By: Claude Opus 5 --- internal/sonar/sonar.go | 4 ++-- internal/sonar/sonar_branch_test.go | 8 ++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/internal/sonar/sonar.go b/internal/sonar/sonar.go index 7b38a5f2b..d0a8b68ec 100644 --- a/internal/sonar/sonar.go +++ b/internal/sonar/sonar.go @@ -216,7 +216,7 @@ func (sc *SonarConfig) GetSonarResults(logger *log.Logger) (*SonarResults, error // can only come from the user (#1116). Set it on the results, which is the // one mechanism both analyses lookups use to scope their search. A pull // request scan is not a branch scan, so the branch is not carried there. - if sc.branch != "" && sc.pullRequest == "" { + if sc.branch != "" && sonarResults.PullRequest == "" { sonarResults.Branch = &Branch{Name: sc.branch} } project.Url, err = sonarURL(sonarResults.ServerUrl, "dashboard", url.Values{"id": {project.Key}}) @@ -307,7 +307,7 @@ func (sc *SonarConfig) GetSonarResults(logger *log.Logger) (*SonarResults, error // PR key, which a recent-first, page-bounded list drops as a matter of course — // warning there would be noise about the ordinary case. if analysisID != "" && sonarResults.TaskID == "" { - 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) } return sonarResults, nil diff --git a/internal/sonar/sonar_branch_test.go b/internal/sonar/sonar_branch_test.go index dfaaed9f2..e57c2d046 100644 --- a/internal/sonar/sonar_branch_test.go +++ b/internal/sonar/sonar_branch_test.go @@ -147,6 +147,9 @@ func TestGetSonarResults_ProjectKeyPath_WithBranch(t *testing.T) { if results.TaskID != revTaskID { t.Errorf("expected the task for analysis %s, got TaskID %q", revAnalysisKey, results.TaskID) } + if results.Project.Name != "customer project" { + t.Errorf("expected the project name from the matched task, got %q", results.Project.Name) + } if results.Status != "SUCCESS" { t.Errorf("expected status SUCCESS from the matched task, got %q", results.Status) } @@ -254,6 +257,11 @@ func TestGetSonarResults_NoMatchingTask_Warns(t *testing.T) { if !strings.Contains(stderr.String(), "no SonarQube compute engine task") { t.Errorf("expected a warning that no task matched, got stderr: %q", stderr.String()) } + // The project name comes from the task too, so the warning names it: keep the + // message tied to something the test checks. + if results.Project.Name != "" { + t.Errorf("expected no project name when no task matched, got %q", results.Project.Name) + } // The branch still has to survive: it is what found the analysis. if results.Branch == nil || results.Branch.Name != revFeatureBranch { t.Errorf("expected the supplied branch to survive, got %+v", results.Branch) From bbfc33fbf5fa9c547bb592a37b98a366f0dcad86 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Fri, 21 Aug 2026 08:49:09 +0100 Subject: [PATCH 13/13] revert(sonar): drop the missing-task warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removing this rather than fixing it a fourth time. It reports pre-existing behaviour — GetTaskID has always returned silently when api/ce/activity holds no task for a scan — and nothing in #1116 asks for it. I added it from a review suggestion, and it has since cost four of this branch's twelve commits: wrong place (fired ahead of a clear error and broke case 27 in CI), wrong scope (fired on the ordinary pull-request miss), wrong claim (omitted the project name), plus a standing coupling of case 13's golden to SonarQube's activity window, because warnings are compared as part of golden output. None of that was about the defect. Every one of those was about the diagnostic. Kept: the --sonar-branch-is-ignored warning, which is about the new flag rather than pre-existing behaviour, and cannot reach a golden because no integration test passes the flag on a CE-task path. The property worth keeping from its tests survives as TestGetSonarResults_NoMatchingTask_KeepsSuppliedBranch: when no task matches, the branch the user supplied must still be in the payload, since it is what found the analysis. That test now asserts no output too, so leaving the payload gap unreported stays a recorded decision rather than an oversight. Also records why the two pull-request guards read the same field for different reasons: after GetTaskID the results field can additionally have been set from the matched task. Refs #1116 Co-Authored-By: Claude Opus 5 --- cmd/kosli/attestSonar_test.go | 4 -- internal/sonar/sonar.go | 16 ++----- internal/sonar/sonar_branch_test.go | 68 +++++------------------------ 3 files changed, 15 insertions(+), 73 deletions(-) diff --git a/cmd/kosli/attestSonar_test.go b/cmd/kosli/attestSonar_test.go index efba7abf9..1a05440bb 100644 --- a/cmd/kosli/attestSonar_test.go +++ b/cmd/kosli/attestSonar_test.go @@ -163,10 +163,6 @@ func (suite *AttestSonarCommandTestSuite) TestAttestSonarCmd() { golden: "Error: open .scannerwork/report-task.txt: no such file or directory. Check your working directory is set correctly. Alternatively provide the project key and either revision or pull-request ID for the scan to attest\n", }, { - // Depends on SonarCloud still holding a compute engine task for the latest - // analysis: the CLI warns when it does not, and that warning is compared - // as part of the golden. getLatestAnalysisRevision picks the newest - // analysis, so its task is at the top of api/ce/activity. name: "13 can retrieve scan results using project key and revision and attest them", cmd: fmt.Sprintf("attest sonar --name cli.foo --commit HEAD --origin-url http://www.example.com --sonar-project-key cyber-dojo_differ --sonar-revision %s %s", suite.mainRevision, suite.defaultKosliArguments), golden: "sonar attestation 'foo' is reported to trail: test-123\n", diff --git a/internal/sonar/sonar.go b/internal/sonar/sonar.go index d0a8b68ec..403fcc087 100644 --- a/internal/sonar/sonar.go +++ b/internal/sonar/sonar.go @@ -239,6 +239,9 @@ func (sc *SonarConfig) GetSonarResults(logger *log.Logger) (*SonarResults, error // But it must not delete the branch the user gave us when it reports none, // which SonarQube does for main-branch tasks and older self-hosted Servers // do more widely (#1116). + // + // Unlike the guard above, PullRequest here can also have been set from the + // matched task, so this must read the results field rather than the config. if sc.branch != "" && sonarResults.PullRequest == "" && (sonarResults.Branch == nil || sonarResults.Branch.Name == "") { sonarResults.Branch = &Branch{Name: sc.branch} @@ -297,19 +300,6 @@ func (sc *SonarConfig) GetSonarResults(logger *log.Logger) (*SonarResults, error sonarResults.Project = *project sonarResults.QualityGate = qualityGate - // No task ID means api/ce/activity held no task for this scan. Warned here, on - // the way out, so it describes the payload being returned rather than appearing - // ahead of an error the caller reports. - // - // Only when we have an analysis ID: SonarQube has just returned that analysis, - // so its task missing from the activity list is surprising and worth saying. On - // the pull-request path there is no analysis ID and the match can only be on the - // PR key, which a recent-first, page-bounded list drops as a matter of course — - // warning there would be noise about the ordinary case. - if analysisID != "" && sonarResults.TaskID == "" { - 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) - } - return sonarResults, nil } diff --git a/internal/sonar/sonar_branch_test.go b/internal/sonar/sonar_branch_test.go index e57c2d046..d43ab1cf5 100644 --- a/internal/sonar/sonar_branch_test.go +++ b/internal/sonar/sonar_branch_test.go @@ -235,11 +235,15 @@ func TestGetSonarResults_TaskWithoutBranch_KeepsSuppliedBranch(t *testing.T) { } } -// TestGetSonarResults_NoMatchingTask_Warns covers the silent gap: api/ce/activity -// cannot be scoped to a branch and is a bounded recent-activity list, so it may -// simply not hold the task. The attestation is then published with no task ID -// and no scan status, which used to happen without a word. -func TestGetSonarResults_NoMatchingTask_Warns(t *testing.T) { +// TestGetSonarResults_NoMatchingTask_KeepsSuppliedBranch covers the case where +// api/ce/activity holds no task for the scan: it cannot be scoped to a branch and +// is a bounded recent-activity list, so an older scan's task may be gone. The +// branch still has to survive, because it is what found the analysis. +// +// The payload's task ID, status and project name are all empty here, which is +// pre-existing behaviour on this path and deliberately left alone: reporting it +// is not part of #1116. Asserting no output keeps that decision explicit. +func TestGetSonarResults_NoMatchingTask_KeepsSuppliedBranch(t *testing.T) { fake := &fakeBranchSonar{taskBranch: revFeatureBranch, taskBranchType: "LONG", omitMatchingTask: true} srv := httptest.NewServer(fake.handler()) defer srv.Close() @@ -254,14 +258,12 @@ func TestGetSonarResults_NoMatchingTask_Warns(t *testing.T) { if results.TaskID != "" { t.Errorf("expected no task ID when no task matched, got %q", results.TaskID) } - if !strings.Contains(stderr.String(), "no SonarQube compute engine task") { - t.Errorf("expected a warning that no task matched, got stderr: %q", stderr.String()) - } - // The project name comes from the task too, so the warning names it: keep the - // message tied to something the test checks. if results.Project.Name != "" { t.Errorf("expected no project name when no task matched, got %q", results.Project.Name) } + if stderr.Len() != 0 { + t.Errorf("expected no output about a pre-existing payload gap, got stderr: %q", stderr.String()) + } // The branch still has to survive: it is what found the analysis. if results.Branch == nil || results.Branch.Name != revFeatureBranch { t.Errorf("expected the supplied branch to survive, got %+v", results.Branch) @@ -287,49 +289,3 @@ func TestGetSonarResults_BranchIgnoredOnCETaskPath_Warns(t *testing.T) { t.Errorf("expected a warning that --sonar-branch is ignored on this path, got stderr: %q", stderr.String()) } } - -// TestGetSonarResults_NoTaskThenFailure_DoesNotWarn is the case CI caught: when -// the lookup goes on to fail, the command already says something specific and -// true, and a warning about an attestation that is never published is noise -// ahead of a better message. The warning describes the payload we are about to -// return, so it must not appear on a path that returns an error instead. -func TestGetSonarResults_NoTaskThenFailure_DoesNotWarn(t *testing.T) { - fake := &fakeBranchSonar{omitMatchingTask: true} - srv := httptest.NewServer(fake.handler()) - defer srv.Close() - - log, stderr := bufferLogger() - sc := sonar.NewSonarConfig("tok", t.TempDir(), "", revProjectKey, srv.URL, "", "99", "", 5) - if _, err := sc.GetSonarResults(log); err == nil { - t.Fatal("expected an error for a pull request that does not exist") - } - - if stderr.Len() != 0 { - t.Errorf("expected no warning ahead of the error, got stderr: %q", stderr.String()) - } -} - -// TestGetSonarResults_PullRequestNoTask_DoesNotWarn pins the scope of the -// missing-task warning. On the pull-request path GetTaskID can only match on the -// PR key, and api/ce/activity is recent-first and page-bounded, so a PR task -// ageing out is ordinary rather than surprising. Warning there would be noise, -// and would tie a passing test to SonarQube's activity window. -func TestGetSonarResults_PullRequestNoTask_DoesNotWarn(t *testing.T) { - fake := &fakeBranchSonar{} - srv := httptest.NewServer(fake.handler()) - defer srv.Close() - - log, stderr := bufferLogger() - sc := sonar.NewSonarConfig("tok", t.TempDir(), "", revProjectKey, srv.URL, "", revPullRequest, "", 5) - results, err := sc.GetSonarResults(log) - if err != nil { - t.Fatalf("expected the pull-request scan to be found, got error: %v", err) - } - - if results.TaskID != "" { - t.Fatalf("expected no task to have matched on the pull-request path, got %q", results.TaskID) - } - if stderr.Len() != 0 { - t.Errorf("expected no warning for an ordinary pull-request task miss, got stderr: %q", stderr.String()) - } -}