Skip to content

fix(github-scraper): error during security alerts must not fail the entire scan - #2042

Merged
moshloop merged 1 commit into
mainfrom
fix/github-err-handling
Mar 26, 2026
Merged

moshloop merged 1 commit into
mainfrom
fix/github-err-handling

Conversation

@adityathebe

@adityathebe adityathebe commented Mar 26, 2026 •

Copy link
Copy Markdown
Member

when client.GetSecretScanningAlerts returns 404, the entire scan would
be marked as failed.

Now, 404 isn't considered an error. It jsut means there's no alert.
And, any failure on codescan, dependabot, or secret scan doesn't dismiss
the findings from the other

Summary by CodeRabbit

Bug Fixes

  • Improved GitHub API error handling—missing resources (404 responses) are now treated as "no results" rather than errors
  • Enhanced data collection resilience—security alert and scorecard fetching now continues on partial failures, enabling more complete data retrieval instead of stopping at the first error

entire scan

when `client.GetSecretScanningAlerts` returns 404, the entire scan would
be marked as failed.

Now, 404 isn't considered an error. It jsut means there's no alert.
And, any failure on codescan, dependabot, or secret scan doesn't dismiss
the findings from the other
@adityathebe
adityathebe requested a review from moshloop March 26, 2026 05:17
@github-actions

github-actions Bot commented Mar 26, 2026 •

Copy link
Copy Markdown

Benchstat

Base: dd4264a99cfbb41c90749d0119917e6b1c89c9eb
Head: a1998c1f0f43ce77cc8492eb85bae5c43d6b7754

📊 1 minor regression(s) (all within 5% threshold)

Benchmark Base Head Change p-value
BenchSaveResultsUpdateChanged/N=1000-4 1.164 1.184 +1.67% 0.041
Full benchstat output
goos: linux
goarch: amd64
pkg: github.com/flanksource/config-db/bench
cpu: AMD EPYC 7763 64-Core Processor                
                                         │ bench-base.txt │          bench-head.txt           │
                                         │     sec/op     │   sec/op     vs base              │
BenchSaveResultsSeed/N=1000-4                 608.4m ± 8%   625.7m ± 7%       ~ (p=0.394 n=6)
BenchSaveResultsUpdateUnchanged/N=1000-4      706.9m ± 1%   714.6m ± 3%       ~ (p=0.132 n=6)
BenchSaveResultsUpdateChanged/N=1000-4         1.164 ± 1%    1.184 ± 2%  +1.67% (p=0.041 n=6)
geomean                                       794.1m        808.9m       +1.86%

                                         │ bench-base.txt │           bench-head.txt           │
                                         │      MB/s      │    MB/s     vs base                │
BenchSaveResultsSeed/N=1000-4                0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=6) ¹
BenchSaveResultsUpdateUnchanged/N=1000-4     0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=6) ¹
BenchSaveResultsUpdateChanged/N=1000-4       0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=6) ¹
geomean                                                 ²               +0.00%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                                         │ bench-base.txt │           bench-head.txt           │
                                         │      B/op      │     B/op      vs base              │
BenchSaveResultsSeed/N=1000-4                34.60Mi ± 0%   34.57Mi ± 0%       ~ (p=0.394 n=6)
BenchSaveResultsUpdateUnchanged/N=1000-4     24.76Mi ± 0%   24.77Mi ± 0%       ~ (p=0.818 n=6)
BenchSaveResultsUpdateChanged/N=1000-4       72.21Mi ± 0%   72.23Mi ± 0%       ~ (p=0.818 n=6)
geomean                                      39.55Mi        39.55Mi       +0.00%

                                         │ bench-base.txt │          bench-head.txt           │
                                         │   allocs/op    │  allocs/op   vs base              │
BenchSaveResultsSeed/N=1000-4                 454.1k ± 0%   454.0k ± 0%       ~ (p=0.485 n=6)
BenchSaveResultsUpdateUnchanged/N=1000-4      288.0k ± 0%   288.0k ± 0%       ~ (p=0.675 n=6)
BenchSaveResultsUpdateChanged/N=1000-4        856.6k ± 1%   856.6k ± 1%       ~ (p=0.937 n=6)
geomean                                       482.0k        482.0k       -0.00%

@coderabbitai

coderabbitai Bot commented Mar 26, 2026 •

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d1386231-7190-44e4-b7a0-b73dc6a4ae70

📥 Commits

Reviewing files that changed from the base of the PR and between dd4264a and a1998c1.

📒 Files selected for processing (3)
  • scrapers/github/client.go
  • scrapers/github/scraper.go
  • scrapers/github/security.go

Walkthrough

The changes improve error handling for GitHub API alert fetches by treating 404 responses as empty results rather than errors, accumulating multiple alert fetch failures instead of returning early, and ensuring security alert scraping failures don't skip subsequent repository processing.

Changes

Cohort / File(s) Summary
Alert Fetch Error Handling
scrapers/github/client.go, scrapers/github/security.go
Added isNotFound() helper to detect GitHub API 404 errors and modified alert fetch methods to return nil alerts for 404 responses. Updated scrapeSecurityAlerts to accumulate errors from all three alert types instead of returning on first failure, and assign successful fetches to structured alert fields.
Repository Processing Flow
scrapers/github/scraper.go
Removed continue statement after security alert scraping errors to allow subsequent processing, and changed OpenSSF scorecard error handling from logging via ctx.Warnf() to recording errors in results.Errorf().
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: error handling improvements for security alerts scanning that prevent a single alert fetch failure from failing the entire scan.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/github-err-handling
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch fix/github-err-handling

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@moshloop
moshloop merged commit 32e689b into main Mar 26, 2026
20 of 21 checks passed
@moshloop
moshloop deleted the fix/github-err-handling branch March 26, 2026 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants