diff --git a/openspec/changes/evidence-upload/design.md b/openspec/changes/archive/2026-03-28-evidence-upload/design.md similarity index 100% rename from openspec/changes/evidence-upload/design.md rename to openspec/changes/archive/2026-03-28-evidence-upload/design.md diff --git a/openspec/changes/evidence-upload/proposal.md b/openspec/changes/archive/2026-03-28-evidence-upload/proposal.md similarity index 100% rename from openspec/changes/evidence-upload/proposal.md rename to openspec/changes/archive/2026-03-28-evidence-upload/proposal.md diff --git a/openspec/changes/evidence-upload/tasks.md b/openspec/changes/archive/2026-03-28-evidence-upload/tasks.md similarity index 100% rename from openspec/changes/evidence-upload/tasks.md rename to openspec/changes/archive/2026-03-28-evidence-upload/tasks.md diff --git a/openspec/changes/consolidate-check-execution/design.md b/openspec/changes/consolidate-check-execution/design.md new file mode 100644 index 00000000..e82b04af --- /dev/null +++ b/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) | diff --git a/openspec/changes/consolidate-check-execution/proposal.md b/openspec/changes/consolidate-check-execution/proposal.md new file mode 100644 index 00000000..7a0e9952 --- /dev/null +++ b/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) | diff --git a/openspec/changes/consolidate-check-execution/tasks.md b/openspec/changes/consolidate-check-execution/tasks.md new file mode 100644 index 00000000..c7761fdb --- /dev/null +++ b/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/` diff --git a/pkg/policies/scorecard/sarif.go b/pkg/policies/scorecard/sarif.go index dd99ed72..cef9d4b3 100644 --- a/pkg/policies/scorecard/sarif.go +++ b/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) } diff --git a/pkg/policies/scorecard/sarif_test.go b/pkg/policies/scorecard/sarif_test.go index 00670c7f..d35bae84 100644 --- a/pkg/policies/scorecard/sarif_test.go +++ b/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,12 +279,19 @@ 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) } @@ -302,8 +299,8 @@ func TestUploadSARIF(t *testing.T) { 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 { diff --git a/pkg/policies/scorecard/scorecard.go b/pkg/policies/scorecard/scorecard.go index 0846dca4..acaad598 100644 --- a/pkg/policies/scorecard/scorecard.go +++ b/pkg/policies/scorecard/scorecard.go @@ -166,68 +166,50 @@ func (b Scorecard) Check(ctx context.Context, c *github.Client, owner, return nil, err } - var notify string - pass := true - f := make(map[string][]string) - + // Filter out unknown checks before running. + var validChecks []string for _, n := range mc.Checks { - _, ok := checksAllChecks[n] - if !ok { + if _, ok := checksAllChecks[n]; !ok { log.Warn(). Str("org", owner). Str("repo", repo). Str("area", polName). Str("check", n). Msg("Unknown scorecard check specified.") - break + continue } + validChecks = append(validChecks, n) + } - // Run each check separately for now. - allRes, err := scRun(ctx, scc.ScRepo, - sc.WithRepoClient(scc.ScRepoClient), - sc.WithChecks([]string{n}), - ) - if err != nil { - log.Warn(). - Str("org", owner). - Str("repo", repo). - Str("area", polName). - Str("check", n). - Err(err). - Msg("Scorecard check errored.") - break - } - if len(allRes.Checks) != 1 { - log.Warn(). - Str("org", owner). - Str("repo", repo). - Str("area", polName). - Str("check", n). - Int("chk_len", len(allRes.Checks)). - Msg("Scorecard did not return expected checks.") - break - } - res := allRes.Checks[0] + // Run all checks at once. Scorecard executes them concurrently and + // captures per-check errors in CheckResult.Error rather than aborting. + allRes, err := scRun(ctx, scc.ScRepo, + sc.WithRepoClient(scc.ScRepoClient), + sc.WithChecks(validChecks), + ) + if err != nil { + return nil, err + } + + var notify string + pass := true + f := make(map[string][]string) + for _, res := range allRes.Checks { if res.Error != nil { - // We are not sure that all checks are safe to run inside Allstar, some - // might error, and we don't want to abort a whole org enforcement loop - // for an expected error. Just log the error and move on. If there are - // any results from a previous check, those can be returned, so just - // break from the loop here. log.Warn(). Str("org", owner). Str("repo", repo). Str("area", polName). - Str("check", n). + Str("check", res.Name). Err(res.Error). Msg("Scorecard check errored.") - break + continue } logs := convertLogs(res.Details) if len(logs) > 0 { - f[n] = logs + f[res.Name] = logs } if res.Score < mc.Threshold && res.Score != checker.InconclusiveResultScore { pass = false @@ -253,8 +235,8 @@ The score was %v, and the passing threshold is %v. } if mc.Upload.SARIF { - if err := uploadSARIF(ctx, c, owner, repo, mc.Checks, mc.Threshold, - scc.ScRepo, scc.ScRepoClient); err != nil { + if err := uploadSARIFResult(ctx, c, owner, repo, &allRes, + mc.Checks, mc.Threshold); err != nil { log.Warn(). Str("org", owner). Str("repo", repo).