scorecard: restore comment, rename uploadSARIFResult, add error path tests by justaugustus · Pull Request #816 · ossf/allstar

justaugustus · 2026-04-14T22:10:04Z

Summary

Follow-up to #807. Addresses remaining review feedback:

Restore error handling comment — the original comment explaining why per-check errors are skipped (rather than aborting) was lost during the consolidation refactor. Restored to scorecard.go.
Rename uploadSARIFResult back to uploadSARIF — the old function no longer exists after the consolidation, so the "Result" suffix is redundant.
Add tests for new error handling paths — the consolidation changed error handling from break-on-error to skip-and-continue, but the new paths were untested:
- TestCheckUnknownCheckSkipped: verifies unknown checks are filtered out and remaining valid checks still execute and produce results
- TestCheckPerCheckErrorSkipped: verifies checks with CheckResult.Error are skipped while other checks still affect pass/notify
Archive OpenSpec proposal — move openspec/changes/consolidate-check-execution/ to openspec/changes/archive/2026-04-14-consolidate-check-execution/ with updated task completion status.

Test plan

All 20 tests pass (18 existing + 2 new)
go vet clean
go build clean

🤖 Generated with Claude Code

- Rename uploadSARIFResult back to uploadSARIF — the old function no longer exists, so the "Result" suffix is redundant - Add TestCheckUnknownCheckSkipped: verifies that unknown checks are filtered out and remaining valid checks still execute - Add TestCheckPerCheckErrorSkipped: verifies that checks with CheckResult.Error are skipped while other checks still produce results Addresses review feedback from #807. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Stephen Augustus <foo@auggie.dev>

Copilot

Pull request overview

Follow-up to the Scorecard check-execution consolidation, restoring rationale around per-check error skipping, simplifying the SARIF upload helper name, and extending tests + OpenSpec documentation to reflect the new control flow.

Changes:

Restores the explanatory comment for skipping per-check Scorecard errors (continue vs abort).
Renames uploadSARIFResult back to uploadSARIF and updates call sites/tests accordingly.
Adds two new tests for “unknown check” and “per-check error” skip paths; archives the related OpenSpec change set with updated status.

Reviewed changes

Copilot reviewed 5 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
pkg/policies/scorecard/scorecard_test.go	Adds new tests for unknown-check and per-check-error skip behavior.
pkg/policies/scorecard/scorecard.go	Restores comment explaining why per-check errors are logged + skipped; updates SARIF upload call to renamed helper.
pkg/policies/scorecard/sarif_test.go	Renames SARIF upload tests to match `uploadSARIF` and updates invocations.
pkg/policies/scorecard/sarif.go	Renames `uploadSARIFResult` to `uploadSARIF` and updates the doc comment.
openspec/changes/archive/2026-04-14-consolidate-check-execution/tasks.md	Updates archived task checklist/status to match final implementation and follow-up PR.
openspec/changes/archive/2026-04-14-consolidate-check-execution/proposal.md	Adds archived proposal describing the consolidation rationale and behavioral changes.
openspec/changes/archive/2026-04-14-consolidate-check-execution/design.md	Adds archived design notes for validation, batching, and error-handling changes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-14T23:16:33Z

+	s := NewScorecard()
+	res, err := s.Check(context.Background(), github.NewClient(nil), "", "")
+	if err != nil {
+		t.Fatalf("Unexpected error: %v", err)
+	}
+	if !res.Pass {
+		t.Error("Expected pass — unknown check should be skipped, valid check should still run")
+	}


This test doesn’t actually prove the valid check ran after encountering an unknown check. If the implementation regresses to break (so no checks run), pass will still default to true and the assertion will still pass. To make this cover the intended behavior, assert an observable effect from the valid check (e.g., have the valid check return a failing score below the threshold and expect res.Pass == false, or explicitly assert that scRun was called / called with only the valid check).

Copilot · 2026-04-14T23:16:33Z

+func TestCheckUnknownCheckSkipped(t *testing.T) {
+	configFetchConfig = func(_ context.Context, _ *github.Client, _, _, _ string,
+		ol config.ConfigLevel, out interface{},
+	) error {
+		if ol == config.OrgLevel {
+			oc := out.(*OrgConfig)
+			*oc = OrgConfig{
+				Checks:    []string{"nonexistent-check", "test"},
+				Threshold: 8,
+			}
+		}
+		return nil
+	}
+	configIsEnabled = func(_ context.Context, _ config.OrgOptConfig, _,
+		_ config.RepoOptConfig, _ *github.Client, _, _ string,
+	) (bool, error) {
+		return true, nil
+	}
+	scorecardGet = func(_ context.Context, _ string, _ bool,
+		_ http.RoundTripper,
+	) (*scorecard.ScClient, error) {
+		return &scorecard.ScClient{}, nil
+	}
+	checksAllChecks = checker.CheckNameToFnMap{"test": {}}
+	scRun = func(_ context.Context, _ clients.Repo, _ ...sc.Option) (sc.Result, error) {
+		return sc.Result{
+			Checks: []checker.CheckResult{{Name: "test", Score: 10}},
+		}, nil
+	}


This test mutates package-level function vars (configFetchConfig/configIsEnabled/scorecardGet/checksAllChecks/scRun) without restoring them, which can leak state into other tests and make failures order-dependent. Consider capturing the original values at the start of the test and restoring them with t.Cleanup (as done in sarif_test.go).

Copilot · 2026-04-14T23:16:33Z

+	s := NewScorecard()
+	res, err := s.Check(context.Background(), github.NewClient(nil), "", "")
+	if err != nil {
+		t.Fatalf("Unexpected error: %v", err)
+	}
+	if !res.Pass {
+		t.Error("Expected pass — errored check should be skipped, passing check should still count")
+	}


This test also doesn’t validate the intended “skip errored check but still let other checks affect pass/notify” behavior. If the code regresses to break on the errored check, the passing check would be skipped but pass would remain true, so this assertion would still pass. Make the non-errored check produce a failing score below threshold (and expect res.Pass == false), or otherwise assert that later check results were processed.

Copilot · 2026-04-14T23:16:33Z

+func TestCheckPerCheckErrorSkipped(t *testing.T) {
+	configFetchConfig = func(_ context.Context, _ *github.Client, _, _, _ string,
+		ol config.ConfigLevel, out interface{},
+	) error {
+		if ol == config.OrgLevel {
+			oc := out.(*OrgConfig)
+			*oc = OrgConfig{
+				Checks:    []string{"erroring", "passing"},
+				Threshold: 8,
+			}
+		}
+		return nil
+	}
+	configIsEnabled = func(_ context.Context, _ config.OrgOptConfig, _,
+		_ config.RepoOptConfig, _ *github.Client, _, _ string,
+	) (bool, error) {
+		return true, nil
+	}
+	scorecardGet = func(_ context.Context, _ string, _ bool,
+		_ http.RoundTripper,
+	) (*scorecard.ScClient, error) {
+		return &scorecard.ScClient{}, nil
+	}
+	checksAllChecks = checker.CheckNameToFnMap{"erroring": {}, "passing": {}}
+	scRun = func(_ context.Context, _ clients.Repo, _ ...sc.Option) (sc.Result, error) {
+		return sc.Result{
+			Checks: []checker.CheckResult{
+				{Name: "erroring", Error: fmt.Errorf("unsupported check")},
+				{Name: "passing", Score: 10},
+			},
+		}, nil
+	}


Like the previous test, this one overrides package-level function variables without restoring them. Please use t.Cleanup to restore the original values so test behavior doesn’t depend on execution order and future tests aren’t affected.

justaugustus · 2026-04-14T23:28:47Z

@copilot — Address the review comments you've left in this PR.

Agent-Logs-Url: https://github.com/ossf/allstar/sessions/db2b87bf-95ff-4444-8743-d64b627d1244 Co-authored-by: justaugustus <567897+justaugustus@users.noreply.github.com>

Copilot · 2026-04-14T23:35:00Z

@copilot — Address the review comments you've left in this PR.

Done in c01fcff. Fixed both new tests:

Added t.Cleanup to restore all mutated package-level vars (configFetchConfig, configIsEnabled, scorecardGet, checksAllChecks, scRun) in both TestCheckUnknownCheckSkipped and TestCheckPerCheckErrorSkipped
Strengthened the assertions: the valid/non-errored check now returns a score of 5 (below threshold 8), so the tests expect res.Pass == false. If the implementation regresses to break on the first skipped check, pass stays true (its initial value) and the test fails — making the regression detectable
Renamed "passing" to "failing" in TestCheckPerCheckErrorSkipped to better reflect the check's intended role in the test

justaugustus requested a review from a team as a code owner April 14, 2026 22:10

github-project-automation Bot added this to OpenSSF Scorecard Apr 14, 2026

dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Apr 14, 2026

justaugustus force-pushed the fix-sarif-naming-and-tests branch from 0e624ed to 80dfb96 Compare April 14, 2026 23:01

dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Apr 14, 2026

justaugustus changed the title ~~scorecard: rename uploadSARIFResult, add error path tests~~ scorecard: restore comment, rename uploadSARIFResult, add error path tests Apr 14, 2026

justaugustus force-pushed the fix-sarif-naming-and-tests branch from 80dfb96 to 551eaf4 Compare April 14, 2026 23:07

justaugustus requested a review from Copilot April 14, 2026 23:13

Copilot started reviewing on behalf of justaugustus April 14, 2026 23:14 View session

Copilot AI reviewed Apr 14, 2026

View reviewed changes

Copilot started work on behalf of justaugustus April 14, 2026 23:28 View session

scorecard: fix tests — add t.Cleanup and strengthen skip assertions

c01fcff

Agent-Logs-Url: https://github.com/ossf/allstar/sessions/db2b87bf-95ff-4444-8743-d64b627d1244 Co-authored-by: justaugustus <567897+justaugustus@users.noreply.github.com>

Copilot finished work on behalf of justaugustus April 14, 2026 23:35

jeffmendoza approved these changes Apr 15, 2026

View reviewed changes

jeffmendoza merged commit 7af507d into main Apr 15, 2026
7 checks passed

jeffmendoza deleted the fix-sarif-naming-and-tests branch April 15, 2026 00:40

github-project-automation Bot moved this to Done in OpenSSF Scorecard Apr 15, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scorecard: restore comment, rename uploadSARIFResult, add error path tests#816

scorecard: restore comment, rename uploadSARIFResult, add error path tests#816
jeffmendoza merged 2 commits intomainfrom
fix-sarif-naming-and-tests

justaugustus commented Apr 14, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 14, 2026

Uh oh!

Copilot AI Apr 14, 2026

Uh oh!

Copilot AI Apr 14, 2026

Uh oh!

Copilot AI Apr 14, 2026

Uh oh!

justaugustus commented Apr 14, 2026

Uh oh!

Copilot AI commented Apr 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

justaugustus commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

justaugustus commented Apr 14, 2026

Uh oh!

Copilot AI commented Apr 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

justaugustus commented Apr 14, 2026 •

edited

Loading