scorecard: consolidate to single scRun() call

openspec/changes/consolidate-check-execution/design.md

@@ -0,0 +1,58 @@
+# Design: Consolidate Scorecard Check Execution
+
+## Check() refactor
+
+### Check name validation
+
+Move validation before `scRun()`. Filter unknown checks, log warnings,
+and pass only valid checks to `scRun()`.
+
+```go
+var validChecks []string
+for _, n := range mc.Checks {
+    if _, ok := checksAllChecks[n]; !ok {
+        log.Warn()...Msg("Unknown scorecard check specified.")
+        continue  // skip instead of break
+    }
+    validChecks = append(validChecks, n)
+}
+```
+
+### Single scRun() call
+
+```go
+allRes, err := scRun(ctx, scc.ScRepo,
+    sc.WithRepoClient(scc.ScRepoClient),
+    sc.WithChecks(validChecks),
+)
+if err != nil {
+    return nil, err
+}
+```
+
+### Issue text from Result.Checks
+
+Iterate `allRes.Checks` with the same threshold comparison and notify
+text building logic. Per-check errors are logged and skipped instead of
+breaking the loop.
+
+### SARIF from same result
+
+Pass `&allRes` directly to the refactored upload function instead of
+calling `uploadSARIF()` which would re-run `scRun()`.
+
+## uploadSARIF refactor
+
+Rename to `uploadSARIFResult()`. Accept `*sc.Result` instead of running
+checks internally. Remove `scRepo` and `scRepoClient` parameters.
+
+Change detection (commit SHA comparison) remains in this function — it
+gates the upload step, not the scan.
+
+## Error handling
+
+| Scenario | Before | After |
+|----------|--------|-------|
+| Unknown check name | `break` | `continue` (skip, run remaining) |
+| `scRun()` error | `break` | `return nil, err` (infrastructure failure) |
+| `CheckResult.Error` | `break` | `continue` (skip, run remaining) |

openspec/changes/consolidate-check-execution/proposal.md

@@ -0,0 +1,93 @@
+# Proposal: Consolidate Scorecard Check Execution
+
+## Summary
+
+Replace the per-check loop + separate SARIF batch run with a single
+`sc.Run()` call. Currently, when `upload.sarif: true` is configured,
+every Scorecard check runs **N+1 times** — N times individually for issue
+text, then once more as a batch for SARIF and results output. This refactor
+eliminates the duplication.
+
+## Motivation
+
+The dual execution path was introduced as a pragmatic choice during the
+evidence-upload feature (see `openspec/changes/archive/2026-03-28-evidence-upload/`):
+the existing per-check loop was preserved to avoid risk, and SARIF generation
+was added as a second `scRun()` call. Now that the feature is tested and
+merged, the duplication should be removed.
+
+### Cost of duplication
+
+- **CPU/I/O**: Each check downloads and analyzes the repository tarball.
+  Running checks N+1 times means N+1x the compute cost.
+- **API calls**: Each `scRun()` call consumes GitHub API quota for repository
+  metadata, branch protection, workflow files, etc.
+- **Latency**: A full org scan of 10 repos with 20 checks takes ~10 minutes.
+  Eliminating the duplicate run could reduce this significantly.
+
+### Why consolidation is safe
+
+Investigation of Scorecard's `Run()` function (v5.4.0) confirms:
+
+- All checks run concurrently in separate goroutines via `runEnabledChecks()`
+- Per-check errors are captured in `CheckResult.Error`, not as a top-level
+  return error
+- `Result.Checks` always contains all results — passing, failing, and errored
+- The top-level `error` from `Run()` is only for infrastructure failures
+  (repo clone, client init)
+- The per-check loop's break-on-error behavior is actually more fragile
+  than batch execution — it loses results for subsequent checks
+
+## Current state
+
+```
+for _, n := range mc.Checks:        ← N sequential scRun() calls
+    scRun(WithChecks([n]))
+    validate result, build notify text
+
+if mc.Upload.SARIF:
+    uploadSARIF()                    ← 1 additional batch scRun()
+        scRun(WithChecks(mc.Checks))
+        resultToSARIF()
+        collectResult()
+        uploadToCodeScanning()
+```
+
+## Proposed state
+
+```
+scRun(WithChecks(validChecks))       ← 1 batch scRun() call
+
+for _, res := range result.Checks:   ← iterate for issue text
+    build notify text (same logic)
+
+if mc.Upload.SARIF:
+    uploadSARIFResult(&result)       ← use same result
+        resultToSARIF()
+        collectResult()
+        uploadToCodeScanning()
+```
+
+## Scope
+
+### In scope
+
+- Consolidate `Check()` to a single `scRun()` call
+- Refactor `uploadSARIF()` to accept a `*sc.Result`
+- Improve error handling: skip errored checks instead of breaking
+- Update tests
+
+### Out of scope
+
+- Change detection repositioning (remains scoped to SARIF upload)
+- Config model changes
+- New features
+
+## Behavioral changes
+
+| Behavior | Before | After |
+|----------|--------|-------|
+| Check execution count | N+1 | 1 |
+| Unknown check name | Breaks loop, loses subsequent | Filtered before Run(), remaining execute |
+| Per-check error | Breaks loop, loses subsequent | Skipped, remaining still produce results |
+| Check execution order | Sequential | Concurrent (Scorecard runs all in parallel) |

openspec/changes/consolidate-check-execution/tasks.md

@@ -0,0 +1,28 @@
+# Tasks: Consolidate Scorecard Check Execution
+
+## Phase 1: OpenSpec and archive
+
+- [x] 1.1 Archive `openspec/changes/evidence-upload/` to
+      `openspec/changes/archive/2026-03-28-evidence-upload/`
+- [x] 1.2 Create proposal, design, and tasks for this change
+
+## Phase 2: Refactor Check() and uploadSARIF()
+
+- [x] 2.1 Refactor `Check()` to use a single `scRun()` call with all checks
+- [x] 2.2 Refactor `uploadSARIF()` to accept `*sc.Result` (renamed to
+      `uploadSARIFResult()`)
+- [x] 2.3 Update error handling: skip instead of break for unknown checks
+      and per-check errors
+
+## Phase 3: Update tests
+
+- [x] 3.1 Update `scorecard_test.go` — existing TestCheck works as-is
+      (mock already returns batch result)
+- [x] 3.2 Update `sarif_test.go` — renamed tests to match
+      `uploadSARIFResult()`, removed internal `scRun` mock (no longer needed)
+
+## Verification
+
+- [x] `go test -v ./pkg/policies/scorecard/...` (18 tests pass)
+- [x] `go vet ./pkg/policies/scorecard/... ./cmd/allstar/`
+- [x] `go build ./cmd/allstar/`

pkg/policies/scorecard/sarif.go

@@ -214,21 +214,19 @@ func compressAndEncode(data []byte) (string, error) {
 	return base64.StdEncoding.EncodeToString(buf.Bytes()), nil
 }
 
-// uploadSARIF generates SARIF for the configured checks and uploads
-// it to GitHub's Code Scanning API, skipping the upload if the SARIF content
-// has not changed since the last upload.
-func uploadSARIF(
+// uploadSARIFResult uploads SARIF and collects results from an existing
+// sc.Result, skipping the upload if the repo HEAD hasn't changed since
+// the last upload.
+func uploadSARIFResult(
 	ctx context.Context,
 	c *github.Client,
 	owner, repo string,
+	result *sc.Result,
 	checkNames []string,
 	threshold int,
-	scRepo clients.Repo,
-	scRepoClient clients.RepoClient,
 ) error {
 	// Change detection: skip upload if the repo HEAD hasn't changed
-	// since the last upload. This avoids redundant uploads when Allstar
-	// runs on a 5-minute cycle and the repo hasn't been updated.
+	// since the last upload.
 	ref, commitSHA, err := getDefaultBranchRefFunc(ctx, c, owner, repo)
 	if err != nil {
 		return fmt.Errorf("getting branch info: %w", err)
@@ -249,25 +247,12 @@ func uploadSARIF(
 		return nil
 	}
 
-	// Run all checks at once to get a complete Result.
-	runOpts := []sc.Option{
-		sc.WithChecks(checkNames),
-	}
-	if scRepoClient != nil {
-		runOpts = append(runOpts, sc.WithRepoClient(scRepoClient))
-	}
-
-	result, err := scRun(ctx, scRepo, runOpts...)
-	if err != nil {
-		return fmt.Errorf("scorecard run for SARIF: %w", err)
-	}
-
 	// Collect JSON v2 result for results file output.
-	collectResult(&result)
+	collectResult(result)
 
 	// Generate SARIF from the result.
 	var buf bytes.Buffer
-	if err := resultToSARIF(&result, checkNames, threshold, &buf); err != nil {
+	if err := resultToSARIF(result, checkNames, threshold, &buf); err != nil {
 		return fmt.Errorf("generating SARIF: %w", err)
 	}
 

pkg/policies/scorecard/sarif_test.go

@@ -256,25 +256,15 @@ func TestUploadToCodeScanningAPIError(t *testing.T) {
 	}
 }
 
-func TestUploadSARIF(t *testing.T) {
-	origScRun := scRun
+func TestUploadSARIFResult(t *testing.T) {
 	origUpload := codeScanningUploadFunc
 	origGetRef := getDefaultBranchRefFunc
 	t.Cleanup(func() {
-		scRun = origScRun
 		codeScanningUploadFunc = origUpload
 		getDefaultBranchRefFunc = origGetRef
 		clearSARIFHashes()
 	})
 
-	scRun = func(_ context.Context, _ clients.Repo, _ ...sc.Option) (sc.Result, error) {
-		return sc.Result{
-			Repo: sc.RepoInfo{Name: "github.com/test/repo"},
-			Checks: []checker.CheckResult{
-				{Name: "Binary-Artifacts", Score: 10},
-			},
-		}, nil
-	}
 	getDefaultBranchRefFunc = func(_ context.Context, _ *github.Client,
 		_, _ string,
 	) (ref, sha string, err error) {
@@ -289,21 +279,28 @@ func TestUploadSARIF(t *testing.T) {
 		return &github.SarifID{ID: github.Ptr("test-id")}, nil, nil
 	}
 
+	result := &sc.Result{
+		Repo: sc.RepoInfo{Name: "github.com/test/repo"},
+		Checks: []checker.CheckResult{
+			{Name: "Binary-Artifacts", Score: 10},
+		},
+	}
+
 	ctx := context.Background()
 	c := github.NewClient(nil)
 	checks := []string{"Binary-Artifacts"}
 
 	// First call should upload.
-	err := uploadSARIF(ctx, c, "testorg", "testrepo", checks, 8, nil, nil)
+	err := uploadSARIFResult(ctx, c, "testorg", "testrepo", result, checks, 8)
 	if err != nil {
 		t.Fatalf("First upload failed: %v", err)
 	}
 	if uploadCount != 1 {
 		t.Errorf("Expected 1 upload, got %d", uploadCount)
 	}
 
-	// Second call with same result should skip (change detection).
-	err = uploadSARIF(ctx, c, "testorg", "testrepo", checks, 8, nil, nil)
+	// Second call with same commit SHA should skip (change detection).
+	err = uploadSARIFResult(ctx, c, "testorg", "testrepo", result, checks, 8)
 	if err != nil {
 		t.Fatalf("Second call failed: %v", err)
 	}
@@ -312,32 +309,20 @@ func TestUploadSARIF(t *testing.T) {
 	}
 }
 
-func TestUploadSARIFNewCommit(t *testing.T) {
-	origScRun := scRun
+func TestUploadSARIFResultNewCommit(t *testing.T) {
 	origUpload := codeScanningUploadFunc
 	origGetRef := getDefaultBranchRefFunc
 	t.Cleanup(func() {
-		scRun = origScRun
 		codeScanningUploadFunc = origUpload
 		getDefaultBranchRefFunc = origGetRef
 		clearSARIFHashes()
 	})
 
-	scRun = func(_ context.Context, _ clients.Repo, _ ...sc.Option) (sc.Result, error) {
-		return sc.Result{
-			Repo: sc.RepoInfo{Name: "github.com/test/repo"},
-			Checks: []checker.CheckResult{
-				{Name: "Binary-Artifacts", Score: 10},
-			},
-		}, nil
-	}
-
 	callNum := 0
 	getDefaultBranchRefFunc = func(_ context.Context, _ *github.Client,
 		_, _ string,
 	) (ref, sha string, err error) {
 		callNum++
-		// Different commit SHA each call — simulates a new push
 		return "refs/heads/main", fmt.Sprintf("sha-%d", callNum), nil
 	}
 
@@ -349,16 +334,23 @@ func TestUploadSARIFNewCommit(t *testing.T) {
 		return &github.SarifID{ID: github.Ptr("test-id")}, nil, nil
 	}
 
+	result := &sc.Result{
+		Repo: sc.RepoInfo{Name: "github.com/test/repo"},
+		Checks: []checker.CheckResult{
+			{Name: "Binary-Artifacts", Score: 10},
+		},
+	}
+
 	ctx := context.Background()
 	c := github.NewClient(nil)
 	checks := []string{"Binary-Artifacts"}
 
 	// First call uploads.
-	if err := uploadSARIF(ctx, c, "testorg", "testrepo", checks, 8, nil, nil); err != nil {
+	if err := uploadSARIFResult(ctx, c, "testorg", "testrepo", result, checks, 8); err != nil {
 		t.Fatalf("First upload failed: %v", err)
 	}
 	// Second call should also upload because the commit SHA changed.
-	if err := uploadSARIF(ctx, c, "testorg", "testrepo", checks, 8, nil, nil); err != nil {
+	if err := uploadSARIFResult(ctx, c, "testorg", "testrepo", result, checks, 8); err != nil {
 		t.Fatalf("Second upload failed: %v", err)
 	}
 	if uploadCount != 2 {