From d28f9662b70e8f3fff31775a94ebb70218c4b961 Mon Sep 17 00:00:00 2001 From: packet-mover Date: Sun, 12 Apr 2026 11:47:16 +0200 Subject: [PATCH] fix: catch-all skip for per-repo errors and cleaner report format - Any non-rate-limit per-repo error now skips the repo instead of aborting the entire scan (GetTree, GetRulesets, GetBranchProtection) - Replace Skipped/SkipReason/SkipError fields with KnownSkipReason and UnknownSkipError (mutually exclusive) plus Skipped() method - Known skip reasons render as plain bullet points in the report - Unknown errors render as bullet points with inline error message - Add per-repo error maps to mock client (ProtectionErrs, RulesetsErrs) --- client_mock_test.go | 32 ++++++---- report.go | 11 ++-- report_test.go | 49 ++++++++------- scanner.go | 59 ++++++++++-------- scanner_test.go | 141 +++++++++++++++++++++++++++++++++++++++++--- 5 files changed, 224 insertions(+), 68 deletions(-) diff --git a/client_mock_test.go b/client_mock_test.go index bb58472..5cece7c 100644 --- a/client_mock_test.go +++ b/client_mock_test.go @@ -4,16 +4,18 @@ import "context" // MockGitHubClient implements GitHubClient with canned responses for testing. type MockGitHubClient struct { - Repos []Repo - Err error - Tree map[string][]FileEntry // repo name -> file entries - TreeErr error // global tree error (used if TreeErrs is nil) - TreeErrs map[string]error // repo name -> per-repo tree error - Protection map[string]*BranchProtection // repo name -> classic branch protection - ProtectionErr error - Rulesets map[string]*BranchProtection // repo name -> rulesets protection - RulesetsErr error - IssueErr error + Repos []Repo + Err error + Tree map[string][]FileEntry // repo name -> file entries + TreeErr error // global tree error (used if TreeErrs is nil) + TreeErrs map[string]error // repo name -> per-repo tree error + Protection map[string]*BranchProtection // repo name -> classic branch protection + ProtectionErr error // global protection error (used if ProtectionErrs is nil) + ProtectionErrs map[string]error // repo name -> per-repo protection error + Rulesets map[string]*BranchProtection // repo name -> rulesets protection + RulesetsErr error // global rulesets error (used if RulesetsErrs is nil) + RulesetsErrs map[string]error // repo name -> per-repo rulesets error + IssueErr error // CreatedIssue records the last CreateIssue call for assertions. CreatedIssue struct { Owner, Repo, Title, Body string @@ -40,6 +42,11 @@ func (m *MockGitHubClient) GetTree(ctx context.Context, owner, repo, branch stri } func (m *MockGitHubClient) GetBranchProtection(ctx context.Context, owner, repo, branch string) (*BranchProtection, error) { + if m.ProtectionErrs != nil { + if err, ok := m.ProtectionErrs[repo]; ok { + return nil, err + } + } if m.ProtectionErr != nil { return nil, m.ProtectionErr } @@ -50,6 +57,11 @@ func (m *MockGitHubClient) GetBranchProtection(ctx context.Context, owner, repo, } func (m *MockGitHubClient) GetRulesets(ctx context.Context, owner, repo, branch string) (*BranchProtection, error) { + if m.RulesetsErrs != nil { + if err, ok := m.RulesetsErrs[repo]; ok { + return nil, err + } + } if m.RulesetsErr != nil { return nil, m.RulesetsErr } diff --git a/report.go b/report.go index ebc0135..1a1798f 100644 --- a/report.go +++ b/report.go @@ -56,7 +56,7 @@ func generateReport(org string, results []RepoResult, now time.Time) string { func splitScanned(results []RepoResult) (scanned, skipped []RepoResult) { for _, rr := range results { - if rr.Skipped { + if rr.Skipped() { skipped = append(skipped, rr) } else { scanned = append(scanned, rr) @@ -177,9 +177,10 @@ func writeNonCompliantSection(b *strings.Builder, org string, nonCompliant []Rep func writeSkippedSection(b *strings.Builder, org string, skipped []RepoResult) { fmt.Fprintf(b, "\n## ⚠️ Skipped (%s)\n\n", pluralRepos(len(skipped))) for _, rr := range skipped { - fmt.Fprintf(b, "
\n%s - %s\n\n", - org, rr.RepoName, rr.RepoName, rr.SkipReason) - b.WriteString("This repository was excluded from compliance results.\n") - b.WriteString("\n
\n\n") + if rr.KnownSkipReason != "" { + fmt.Fprintf(b, "- [%s](https://github.com/%s/%s) - %s\n", rr.RepoName, org, rr.RepoName, rr.KnownSkipReason) + } else { + fmt.Fprintf(b, "- [%s](https://github.com/%s/%s) - unexpected error: %s\n", rr.RepoName, org, rr.RepoName, rr.UnknownSkipError) + } } } diff --git a/report_test.go b/report_test.go index 26cfcd3..36f30d7 100644 --- a/report_test.go +++ b/report_test.go @@ -219,8 +219,8 @@ func TestGenerateReport_WithSkippedRepos(t *testing.T) { {RepoName: "good-repo", Results: []RuleResult{ {RuleName: "Has repo description", Passed: true}, }}, - {RepoName: "empty-repo", Skipped: true, SkipReason: "repository is empty"}, - {RepoName: "huge-repo", Skipped: true, SkipReason: "file tree too large (truncated by GitHub API)"}, + {RepoName: "empty-repo", KnownSkipReason: "repository is empty"}, + {RepoName: "huge-repo", KnownSkipReason: "file tree too large (truncated by GitHub API)"}, } got := generateReport("test-org", results, testTime) @@ -250,20 +250,8 @@ func TestGenerateReport_WithSkippedRepos(t *testing.T) { ## ⚠️ Skipped (2 repos) -
-empty-repo - repository is empty - -This repository was excluded from compliance results. - -
- -
-huge-repo - file tree too large (truncated by GitHub API) - -This repository was excluded from compliance results. - -
- +- [empty-repo](https://github.com/test-org/empty-repo) - repository is empty +- [huge-repo](https://github.com/test-org/huge-repo) - file tree too large (truncated by GitHub API) ` if got != want { t.Errorf("report mismatch.\n\nGOT:\n%s\n\nWANT:\n%s", got, want) @@ -272,7 +260,7 @@ This repository was excluded from compliance results. func TestGenerateReport_OnlySkippedRepos(t *testing.T) { results := []RepoResult{ - {RepoName: "empty-repo", Skipped: true, SkipReason: "repository is empty"}, + {RepoName: "empty-repo", KnownSkipReason: "repository is empty"}, } got := generateReport("test-org", results, testTime) @@ -286,13 +274,32 @@ func TestGenerateReport_OnlySkippedRepos(t *testing.T) { ## ⚠️ Skipped (1 repo) -
-empty-repo - repository is empty +- [empty-repo](https://github.com/test-org/empty-repo) - repository is empty +` + if got != want { + t.Errorf("report mismatch.\n\nGOT:\n%s\n\nWANT:\n%s", got, want) + } +} -This repository was excluded from compliance results. +func TestGenerateReport_WithUnexpectedSkipError(t *testing.T) { + results := []RepoResult{ + {RepoName: "broken-repo", UnknownSkipError: "get tree for broken-repo: status 500"}, + {RepoName: "empty-repo", KnownSkipReason: "repository is empty"}, + } -
+ got := generateReport("test-org", results, testTime) + + want := `# Codatus - Org Compliance Report + +**Org:** test-org +**Scanned:** 2026-04-05 12:00 UTC +**Repos scanned:** 0 +**Skipped:** 2 + +## ⚠️ Skipped (2 repos) +- [broken-repo](https://github.com/test-org/broken-repo) - unexpected error: get tree for broken-repo: status 500 +- [empty-repo](https://github.com/test-org/empty-repo) - repository is empty ` if got != want { t.Errorf("report mismatch.\n\nGOT:\n%s\n\nWANT:\n%s", got, want) diff --git a/scanner.go b/scanner.go index a058ad4..9cbd724 100644 --- a/scanner.go +++ b/scanner.go @@ -16,11 +16,16 @@ type Config struct { } // RepoResult holds all rule results for a single repository. +// KnownSkipReason and UnknownSkipError are mutually exclusive. type RepoResult struct { - RepoName string - Results []RuleResult - Skipped bool - SkipReason string + RepoName string + Results []RuleResult + KnownSkipReason string + UnknownSkipError string +} + +func (rr RepoResult) Skipped() bool { + return rr.KnownSkipReason != "" || rr.UnknownSkipError != "" } // Run is the high-level entry point. It constructs a client, scans the org, @@ -36,7 +41,7 @@ func Run(ctx context.Context, cfg Config) error { scanned := 0 skipped := 0 for _, r := range results { - if r.Skipped { + if r.Skipped() { skipped++ } else { scanned++ @@ -55,6 +60,18 @@ func Run(ctx context.Context, cfg Config) error { return nil } +// skipReasonForError returns a human-readable skip reason and an optional raw +// error string for unexpected failures. Known errors get a clean reason with no +// error detail. Unknown errors get a generic reason with the error message. +func skipRepo(name string, err error) RepoResult { + if errors.Is(err, ErrEmptyRepo) { + return RepoResult{RepoName: name, KnownSkipReason: "repository is empty"} + } + if errors.Is(err, ErrTruncatedTree) { + return RepoResult{RepoName: name, KnownSkipReason: "file tree too large (truncated by GitHub API)"} + } + return RepoResult{RepoName: name, UnknownSkipError: err.Error()} +} // Scan lists all non-archived repos in the org and evaluates every rule against each. func Scan(ctx context.Context, client GitHubClient, org string) ([]RepoResult, error) { @@ -73,34 +90,30 @@ func Scan(ctx context.Context, client GitHubClient, org string) ([]RepoResult, e files, err := client.GetTree(ctx, org, repo.Name, repo.DefaultBranch) if err != nil { - if errors.Is(err, ErrEmptyRepo) { - results = append(results, RepoResult{ - RepoName: repo.Name, - Skipped: true, - SkipReason: "repository is empty", - }) - continue + if isRateLimitError(err) { + return nil, fmt.Errorf("get tree for repo %s: %w", repo.Name, err) } - if errors.Is(err, ErrTruncatedTree) { - results = append(results, RepoResult{ - RepoName: repo.Name, - Skipped: true, - SkipReason: "file tree too large (truncated by GitHub API)", - }) - continue - } - return nil, fmt.Errorf("get tree for repo %s: %w", repo.Name, err) + results = append(results, skipRepo(repo.Name, err)) + continue } repo.Files = files protection, err := client.GetRulesets(ctx, org, repo.Name, repo.DefaultBranch) if err != nil { - return nil, fmt.Errorf("get rulesets for repo %s: %w", repo.Name, err) + if isRateLimitError(err) { + return nil, fmt.Errorf("get rulesets for repo %s: %w", repo.Name, err) + } + results = append(results, skipRepo(repo.Name, err)) + continue } if protection == nil { protection, err = client.GetBranchProtection(ctx, org, repo.Name, repo.DefaultBranch) if err != nil { - return nil, fmt.Errorf("get branch protection for repo %s: %w", repo.Name, err) + if isRateLimitError(err) { + return nil, fmt.Errorf("get branch protection for repo %s: %w", repo.Name, err) + } + results = append(results, skipRepo(repo.Name, err)) + continue } } repo.BranchProtection = protection diff --git a/scanner_test.go b/scanner_test.go index b6aebda..d6439d8 100644 --- a/scanner_test.go +++ b/scanner_test.go @@ -3,9 +3,26 @@ package scanner import ( "context" "fmt" + "net/http" "testing" + "time" + + "github.com/google/go-github/v72/github" ) +// newRateLimitError creates a *github.RateLimitError suitable for mock testing. +func newRateLimitError() *github.RateLimitError { + return &github.RateLimitError{ + Rate: github.Rate{ + Limit: 5000, + Remaining: 0, + Reset: github.Timestamp{Time: time.Now().Add(time.Hour)}, + }, + Response: &http.Response{StatusCode: http.StatusForbidden}, + Message: "API rate limit exceeded", + } +} + func TestScan_SkipsArchivedRepos(t *testing.T) { client := &MockGitHubClient{ Repos: []Repo{ @@ -166,10 +183,10 @@ func TestScan_SkipsEmptyRepo(t *testing.T) { } // Results sorted alphabetically: empty-repo first - if !results[0].Skipped || results[0].SkipReason == "" { - t.Errorf("expected empty-repo to be skipped, got %+v", results[0]) + if !results[0].Skipped() || results[0].KnownSkipReason != "repository is empty" { + t.Errorf("expected empty-repo to be skipped with known reason, got %+v", results[0]) } - if results[1].Skipped { + if results[1].Skipped() { t.Errorf("expected normal-repo to not be skipped") } if len(results[1].Results) == 0 { @@ -195,20 +212,126 @@ func TestScan_SkipsTruncatedTree(t *testing.T) { if len(results) != 1 { t.Fatalf("expected 1 result, got %d", len(results)) } - if !results[0].Skipped { - t.Error("expected huge-repo to be skipped") + if !results[0].Skipped() || results[0].KnownSkipReason == "" { + t.Error("expected huge-repo to be skipped with known reason") + } +} + +func TestScan_SkipsUnexpectedGetTreeError(t *testing.T) { + client := &MockGitHubClient{ + Repos: []Repo{ + {Name: "broken-repo", Description: "Broken", DefaultBranch: "main"}, + {Name: "good-repo", Description: "Good", DefaultBranch: "main"}, + }, + TreeErrs: map[string]error{ + "broken-repo": fmt.Errorf("get tree for broken-repo: status 500"), + }, + } + + results, err := Scan(context.Background(), client, "test-org") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(results) != 2 { + t.Fatalf("expected 2 results, got %d", len(results)) + } + + // Sorted: broken-repo first + if !results[0].Skipped() || results[0].UnknownSkipError == "" { + t.Errorf("expected broken-repo to be skipped with unknown error, got %+v", results[0]) + } + if results[1].Skipped() { + t.Error("expected good-repo to not be skipped") + } +} + +func TestScan_SkipsUnexpectedRulesetsError(t *testing.T) { + client := &MockGitHubClient{ + Repos: []Repo{ + {Name: "broken-repo", Description: "Broken", DefaultBranch: "main"}, + {Name: "good-repo", Description: "Good", DefaultBranch: "main"}, + }, + RulesetsErrs: map[string]error{ + "broken-repo": fmt.Errorf("get rulesets: status 500"), + }, + } + + results, err := Scan(context.Background(), client, "test-org") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(results) != 2 { + t.Fatalf("expected 2 results, got %d", len(results)) + } + + if !results[0].Skipped() || results[0].UnknownSkipError == "" { + t.Errorf("expected broken-repo to be skipped, got %+v", results[0]) + } + if results[1].Skipped() { + t.Error("expected good-repo to not be skipped") + } +} + +func TestScan_SkipsUnexpectedBranchProtectionError(t *testing.T) { + client := &MockGitHubClient{ + Repos: []Repo{ + {Name: "broken-repo", Description: "Broken", DefaultBranch: "main"}, + }, + // No rulesets -> falls through to classic protection + ProtectionErrs: map[string]error{ + "broken-repo": fmt.Errorf("get protection: status 500"), + }, + } + + results, err := Scan(context.Background(), client, "test-org") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(results) != 1 { + t.Fatalf("expected 1 result, got %d", len(results)) + } + if !results[0].Skipped() || results[0].UnknownSkipError == "" { + t.Errorf("expected broken-repo to be skipped, got %+v", results[0]) + } +} + +func TestScan_AbortsOnRateLimitDuringGetTree(t *testing.T) { + client := &MockGitHubClient{ + Repos: []Repo{ + {Name: "repo-a", Description: "A", DefaultBranch: "main"}, + }, + TreeErr: newRateLimitError(), + } + + _, err := Scan(context.Background(), client, "test-org") + if err == nil { + t.Fatal("expected error, got nil") } - if results[0].SkipReason == "" { - t.Error("expected skip reason to be set") +} + +func TestScan_AbortsOnRateLimitDuringGetRulesets(t *testing.T) { + client := &MockGitHubClient{ + Repos: []Repo{ + {Name: "repo-a", Description: "A", DefaultBranch: "main"}, + }, + RulesetsErr: newRateLimitError(), + } + + _, err := Scan(context.Background(), client, "test-org") + if err == nil { + t.Fatal("expected error, got nil") } } -func TestScan_AbortsOnRateLimit(t *testing.T) { +func TestScan_AbortsOnRateLimitDuringGetBranchProtection(t *testing.T) { client := &MockGitHubClient{ Repos: []Repo{ {Name: "repo-a", Description: "A", DefaultBranch: "main"}, }, - TreeErr: fmt.Errorf("rate limit exceeded"), + ProtectionErr: newRateLimitError(), } _, err := Scan(context.Background(), client, "test-org")