Add AnalysisError type and wrap all analyzer error paths#4779
Add AnalysisError type and wrap all analyzer error paths#4779johnelliott wants to merge 3 commits intomainfrom
Conversation
Introduce a shared error type that provides structured metadata (analyzer type, operation, service, resource) for analysis failures. This allows the scanner to extract context from errors without depending on concrete types.
Batch A: Airbrake, Anthropic, Asana, DigitalOcean, DockerHub, ElevenLabs, Fastly, Groq, HuggingFace, Mailchimp, Mailgun, Mux, Netlify, Ngrok, Notion, OpenAI, Opsgenie, Posthog, Postman, Sendgrid, Sourcegraph. Wraps credential validation errors with operation "validate_credentials" and AnalyzePermissions errors with operation "analyze_permissions".
Batch B (OAuth/multi-credential): airtableoauth, airtablepat, datadog, dropbox, figma, launchdarkly, plaid Batch C (Complex): bitbucket, databricks, github, gitlab, jira, monday, planetscale, shopify, slack, square, stripe, twilio Batch D (Database): mysql, postgres (service: Database) Batch E (PrivateKey): privatekey (service: crypto)
|
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| return nil, err | ||
| return nil, analyzers.NewAnalysisError( | ||
| "Airbrake", "analyze_permissions", "API", "", err, | ||
| ) |
There was a problem hiding this comment.
Hardcoded analyzer type strings duplicate existing constants
Low Severity
Every NewAnalysisError call across all 43 analyzers passes a hardcoded string like "Airbrake", "GitHub", etc. for the analyzer type. These exact strings already exist in analyzerTypeStrings and are accessible via a.Type().String() from within each Analyze method. This duplication means a future analyzer addition or rename requires updating both the analyzerTypeStrings map and each NewAnalysisError call, with no compile-time check to catch a mismatch.
Additional Locations (1)
| if err != nil { | ||
| return nil, err | ||
| return nil, analyzers.NewAnalysisError( | ||
| "Airbrake", "analyze_permissions", "API", "", err, |
There was a problem hiding this comment.
Let's use a.Type().String() instead of the hardcoded Airbrake to eliminate the risk of inconsistencies and improve maintainability.
| token, ok := credInfo["token"] | ||
| if !ok { | ||
| return nil, errors.New("token not found in credInfo") | ||
| return nil, analyzers.NewAnalysisError("AirtablePat", "validate_credentials", "config", "", errors.New("token not found in credInfo")) |
There was a problem hiding this comment.
Could we define constants (or typed values) for validate_credentials, analyze_permissions, etc., instead of using raw strings? They’re used in several places, and this would help prevent easy typos (speaking for myself here 😄) and make things safer long-term.
3 participants
What this does
Adds a shared error type so analyzer failures carry structured metadata (analyzer type, operation, service). Currently only GCP errors have this; the other 44 analyzers return bare
errors.Newthat get stored as "unknown" in the database.How to review
Read one file, skip the other 44. There are only 2 things to look at:
1. The new error type (entire abstraction)
pkg/analyzer/analyzers/errors.go-- 51 lines, new file:2. The pattern every analyzer follows
Every analyzer diff looks like this (Airbrake shown):
The original error is wrapped, not replaced (
Unwrap()preserves the chain). Two operations are used:"validate_credentials"for bad input,"analyze_permissions"for API/DB failures.All 44 analyzer files are this same 1-3 line change. You can spot-check a few and skip the rest.
Part of a cross-repo change
errors.As()Test plan
go test ./pkg/analyzer/analyzers/...(includeserrors_test.go)go build ./pkg/analyzer/analyzers/...