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

cmd/kosli/attestSonar.go

@@ -18,16 +18,17 @@ type SonarAttestationPayload struct {
 
 type attestSonarOptions struct {
 	*CommonAttestationOptions
-	apiToken    string
-	workingDir  string
-	ceTaskURL   string
-	projectKey  string
-	serverURL   string
-	revision    string
-	pullRequest string
-	branch      string
-	maxWait     int
-	payload     SonarAttestationPayload
+	apiToken         string
+	workingDir       string
+	ceTaskURL        string
+	projectKey       string
+	serverURL        string
+	revision         string
+	pullRequest      string
+	branch           string
+	maxWait          int
+	payload          SonarAttestationPayload
+	revisionExplicit bool
 }
 
 const attestSonarShortDesc = `Report a SonarQube attestation to an artifact or a trail in a Kosli flow.  `
@@ -52,6 +53,8 @@ or have overridden the revision in SonarQube via parameters to the Sonar scanner
 If the scan ran on a branch other than the project's main branch in SonarQube, also provide the branch name using the ^--sonar-branch^ flag.
 SonarQube only searches the project's main branch unless told otherwise, so without this flag the scan cannot be found.
 For pull request scans: provide the pull-request ID using the ^--pull-request^ flag instead of the revision.
+SonarQube keeps only the latest analysis of a pull request, so if the scan for the current push has not finished, that is the previous push's analysis.
+To make sure the attested analysis is of the commit you expect, also pass ^--sonar-revision^: the command then fails if the pull request's analysis is of another commit.
 Kosli then finds the scan results for the specified project key and revision or pull-request ID.
 
 3. Providing the CE task URL directly via ^--sonar-ce-task-url^. The CE task URL can be found in the ^report-task.txt^ file
@@ -132,6 +135,18 @@ kosli attest sonar \
 	--api-token yourAPIToken \
 	--org yourOrgName \
 
+# report a SonarQube Cloud attestation about a trail for a pull request scan, failing unless the analysis is of the given commit:
+kosli attest sonar \
+	--name yourAttestationName \
+	--flow yourFlowName \
+	--trail yourTrailName \
+	--sonar-api-token yourSonarAPIToken \
+	--sonar-project-key yourSonarProjectKey \
+	--pull-request yourPullRequestID \
+	--sonar-revision yourSonarRevision \
+	--api-token yourAPIToken \
+	--org yourOrgName \
+
 # report a SonarQube Cloud attestation about a trail with an attachment using SonarQube's metadata, waiting for up to 300 seconds for the results to be available:
 kosli attest sonar \
 	--name yourAttestationName \
@@ -188,11 +203,6 @@ func newAttestSonarCmd(out io.Writer) *cobra.Command {
 				return err
 			}
 
-			err = MuXRequiredFlags(cmd, []string{"sonar-revision", "pull-request"}, false)
-			if err != nil {
-				return err
-			}
-
 			err = MuXRequiredFlags(cmd, []string{"sonar-branch", "pull-request"}, false)
 			if err != nil {
 				return err
@@ -209,6 +219,7 @@ func newAttestSonarCmd(out io.Writer) *cobra.Command {
 		RunE: func(cmd *cobra.Command, args []string) error {
 			o.repoURLExplicit = cmd.Flags().Changed("repo-url")
 			o.repoNameExplicit = cmd.Flags().Changed("repository")
+			o.revisionExplicit = cmd.Flags().Changed("sonar-revision")
 			return o.run(args)
 		},
 	}
@@ -244,7 +255,16 @@ 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.branch, o.maxWait)
+	// --sonar-revision defaults to the CI commit, so only a revision the user
+	// gave explicitly is checked against the pull request's analysed commit
+	// (#1192). The non-PR path still needs the default: it looks the analysis
+	// up by revision.
+	revision := o.revision
+	if o.pullRequest != "" && !o.revisionExplicit {
+		revision = ""
+	}
+
+	sc := sonar.NewSonarConfig(o.apiToken, o.workingDir, o.ceTaskURL, o.projectKey, o.serverURL, revision, o.pullRequest, o.branch, o.maxWait)
 
 	o.payload.SonarResults, err = sc.GetSonarResults(logger)
 	if err != nil {

cmd/kosli/attestSonar_test.go

@@ -45,6 +45,7 @@ type AttestSonarCommandTestSuite struct {
 	prScannerWorkDir    string
 	prKey               string
 	prCETaskURL         string
+	prRevision          string
 	suite.Suite
 	defaultKosliArguments string
 }
@@ -76,6 +77,7 @@ func (suite *AttestSonarCommandTestSuite) SetupTest() {
 
 	suite.mainScannerWorkDir, suite.mainCETaskURL, suite.mainRevision = downloadMainScanData(suite.T())
 	suite.prScannerWorkDir, suite.prKey, suite.prCETaskURL = downloadPRScanData(suite.T())
+	suite.prRevision = getPRAnalysisRevision(suite.T(), suite.prKey)
 }
 
 func (suite *AttestSonarQubeCommandTestSuite) SetupTest() {
@@ -229,10 +231,9 @@ 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",
 		},
 		{
-			wantError: true,
-			name:      "25 can't provide both revision 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-revision xxx --pull-request 5 %s", suite.defaultKosliArguments),
-			golden:    "Error: only one of --sonar-revision, --pull-request is allowed\n",
+			name:   "25 can attest a pull request scan alongside --sonar-revision",
+			cmd:    fmt.Sprintf("attest sonar --name cli.foo --commit HEAD --origin-url http://www.example.com --sonar-project-key cyber-dojo_differ --pull-request %s --sonar-revision %s %s", suite.prKey, suite.prRevision, suite.defaultKosliArguments),
+			golden: "sonar attestation 'foo' is reported to trail: test-123\n",
 		},
 		{
 			name:   "26 can attest sonar for a pull request scan using --sonar-ce-task-url",
@@ -263,6 +264,12 @@ func (suite *AttestSonarCommandTestSuite) TestAttestSonarCmd() {
 			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:      "31 attesting a pull request scan with --sonar-revision of a commit it did not analyse fails",
+			cmd:       fmt.Sprintf("attest sonar --name cli.foo --commit HEAD --origin-url http://www.example.com --sonar-project-key cyber-dojo_differ --pull-request %s --sonar-revision 0000000000000000000000000000000000000000 %s", suite.prKey, suite.defaultKosliArguments),
+			golden:    fmt.Sprintf("Error: analysis for pull request %s of project cyber-dojo_differ is of revision %s, not 0000000000000000000000000000000000000000.\nThe scan for that revision may still be being processed by SonarQube, try again later.\nOtherwise check the revision is correct\n", suite.prKey, suite.prRevision),
+		},
 	}
 
 	runTestCmd(suite.T(), tests)
@@ -435,6 +442,51 @@ func getLatestAnalysisRevision(t *testing.T) string {
 	return result.Analyses[0].Revision
 }
 
+// getPRAnalysisRevision fetches the commit SonarCloud analysed for the given pull
+// request of cyber-dojo_differ. project_analyses/search does not list PR analyses.
+func getPRAnalysisRevision(t *testing.T, prKey string) string {
+	t.Helper()
+
+	sonarToken := os.Getenv("KOSLI_SONAR_API_TOKEN")
+	httpClient := &http.Client{}
+
+	req, err := http.NewRequest("GET",
+		"https://sonarcloud.io/api/project_pull_requests/list?project=cyber-dojo_differ", nil)
+	if err != nil {
+		t.Fatalf("failed to create pull requests list request: %v", err)
+	}
+	req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", sonarToken))
+
+	resp, err := httpClient.Do(req)
+	if err != nil {
+		t.Fatalf("failed to fetch pull requests from SonarCloud: %v", err)
+	}
+	defer func() { _ = resp.Body.Close() }()
+
+	if resp.StatusCode != http.StatusOK {
+		t.Fatalf("SonarCloud pull requests API returned status %d", resp.StatusCode)
+	}
+
+	var result struct {
+		PullRequests []struct {
+			Key    string `json:"key"`
+			Commit struct {
+				SHA string `json:"sha"`
+			} `json:"commit"`
+		} `json:"pullRequests"`
+	}
+	if err := json.NewDecoder(resp.Body).Decode(&result); err != nil {
+		t.Fatalf("failed to decode pull requests response: %v", err)
+	}
+	for _, pr := range result.PullRequests {
+		if pr.Key == prKey {
+			return pr.Commit.SHA
+		}
+	}
+	t.Fatalf("pull request %s not found for cyber-dojo_differ on SonarCloud", prKey)
+	return ""
+}
+
 // downloadPRScanData downloads the report-task.txt from the latest
 // SonarCloud PR scan of cyber-dojo_differ.
 // Returns the directory containing report-task.txt, the PR key, and the CE task URL.

cmd/kosli/root.go

@@ -315,8 +315,8 @@ Paths the list already matches stay excluded whatever is later added there, so k
 	sonarWorkingDirFlag             = "[conditional] The base directory of the repo scanned by SonarQube. Only required if you have overridden the default in the Sonar scanner or you are running the CLI locally in a separate folder from the repo."
 	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 or --sonar-branch."
+	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. With --pull-request, optional: when given, the pull request's analysis must be of this revision or the command fails."
+	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-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."

internal/sonar/sonar.go

@@ -257,6 +257,13 @@ func (sc *SonarConfig) GetSonarResults(logger *log.Logger) (*SonarResults, error
 		if err != nil {
 			return nil, err
 		}
+		// SonarQube keeps only a pull request's latest analysis, which is the
+		// previous push's until the current scan is processed (#1192). Only a
+		// revision the user passed alongside --pull-request is checked; on the
+		// other paths sc.revision is the defaulted commit, not an expectation.
+		if sc.pullRequest != "" && sc.revision != "" && sonarResults.Revision != sc.revision {
+			return nil, fmt.Errorf("analysis for pull request %s of project %s is of revision %s, not %s.\nThe scan for that revision may still be being processed by SonarQube, try again later.\nOtherwise check the revision is correct", sc.pullRequest, project.Key, sonarResults.Revision, sc.revision)
+		}
 	}
 
 	//Get the quality gate status from the qualitygates/project_status API

internal/sonar/sonar_test.go

@@ -167,6 +167,11 @@ func (f *fakeSonarProject) handler() http.HandlerFunc {
 					Commit: sonar.PRCommit{SHA: revRevision},
 				}},
 			})
+		case "/api/ce/task":
+			_ = json.NewEncoder(w).Encode(sonar.TaskResponse{Task: sonar.Task{
+				TaskID: revTaskID, AnalysisID: revAnalysisKey, Status: "SUCCESS", ComponentKey: revProjectKey,
+				ComponentName: "customer project", PullRequest: revPullRequest,
+			}})
 		case "/api/qualitygates/project_status":
 			_ = json.NewEncoder(w).Encode(sonar.QualityGateResponse{
 				ProjectStatus: sonar.ProjectStatus{Status: "OK", Conditions: []sonar.Conditions{}},
@@ -379,6 +384,84 @@ func TestGetSonarResults_BranchIgnoredForPullRequest(t *testing.T) {
 	}
 }
 
+// TestGetSonarResults_PullRequestRevision is the #1192 check: SonarQube keeps only a
+// pull request's latest analysis, so a revision named alongside the pull request
+// must be the analysed commit, and no revision means no check. Both variants pass
+// --pull-request explicitly, discovering the scan via --sonar-project-key or via
+// --sonar-ce-task-url; see TestGetSonarResults_PullRequestRevision_DiscoveredWithoutFlag
+// for the case where the pull request is discovered without that flag.
+func TestGetSonarResults_PullRequestRevision(t *testing.T) {
+	const otherRevision = "0000000000000000000000000000000000000000"
+
+	cases := []struct {
+		name     string
+		revision string
+		wantErr  bool
+	}{
+		{name: "matching revision is attested", revision: revRevision},
+		{name: "other revision fails", revision: otherRevision, wantErr: true},
+		{name: "no revision is not checked", revision: ""},
+	}
+
+	for _, c := range cases {
+		for _, path := range []string{"project key", "CE task"} {
+			t.Run(path+": "+c.name, func(t *testing.T) {
+				fake := &fakeSonarProject{}
+				srv := httptest.NewServer(fake.handler())
+				defer srv.Close()
+
+				var sc *sonar.SonarConfig
+				if path == "CE task" {
+					sc = sonar.NewSonarConfig("tok", t.TempDir(), srv.URL+"/api/ce/task?id="+revTaskID, "", "", c.revision, revPullRequest, "", 5)
+				} else {
+					sc = sonar.NewSonarConfig("tok", t.TempDir(), "", revProjectKey, srv.URL, c.revision, revPullRequest, "", 5)
+				}
+				results, err := sc.GetSonarResults(discardLogger())
+
+				if !c.wantErr {
+					if err != nil {
+						t.Fatalf("expected the pull-request scan to be attested, got error: %v", err)
+					}
+					if results.Revision != revRevision {
+						t.Errorf("expected the analysed revision %q in the results, got %q", revRevision, results.Revision)
+					}
+					return
+				}
+				if err == nil {
+					t.Fatalf("expected an error: pull request %s was analysed at %s, not %s", revPullRequest, revRevision, otherRevision)
+				}
+				for _, want := range []string{revPullRequest, revRevision, otherRevision} {
+					if !strings.Contains(err.Error(), want) {
+						t.Errorf("expected %q in the error, got: %v", want, err)
+					}
+				}
+			})
+		}
+	}
+}
+
+// TestGetSonarResults_PullRequestRevision_DiscoveredWithoutFlag pins the gap the
+// comment above notes: when a CE task's own response names the pull request
+// (--sonar-ce-task-url or report-task.txt used without --pull-request), sc.pullRequest
+// is empty and the #1192 check does not fire, even for a revision that does not
+// match — the same staleness the check exists to catch elsewhere.
+func TestGetSonarResults_PullRequestRevision_DiscoveredWithoutFlag(t *testing.T) {
+	const otherRevision = "0000000000000000000000000000000000000000"
+
+	fake := &fakeSonarProject{}
+	srv := httptest.NewServer(fake.handler())
+	defer srv.Close()
+
+	sc := sonar.NewSonarConfig("tok", t.TempDir(), srv.URL+"/api/ce/task?id="+revTaskID, "", "", otherRevision, "", "", 5)
+	results, err := sc.GetSonarResults(discardLogger())
+	if err != nil {
+		t.Fatalf("expected no check without --pull-request, got error: %v", err)
+	}
+	if results.Revision != revRevision {
+		t.Errorf("expected the analysed revision %q in the results, got %q", revRevision, results.Revision)
+	}
+}
+
 // TestGetSonarResults_NoMatchingTask_KeepsSuppliedBranch pins the payload when
 // api/ce/activity holds no task for the analysis. That is reachable in production:
 // the endpoint needs administrative permission, so an ordinary CI token gets an