fix: surface rest data setup errors from Setup - #471
Conversation
Signed-off-by: Satarupa22-SD <satarupa2212@gmail.com>
jmeridth
left a comment
jmeridth
left a comment
There was a problem hiding this comment.
🤖 (summary) Good fix for #42, and the design nailed the part I was most worried about: the joined error is caught and Warn-logged in payload.go rather than propagating to the orchestrator, so scans still degrade gracefully instead of aborting, and every per-domain error-flag contract survives (I traced all consumers). errors.Join with per-domain prefixes is the right shape. Two findings inline; the first matters because it is the PR's whole purpose.
| wg.Wait() | ||
| return nil | ||
|
|
||
| return errors.Join( |
There was a problem hiding this comment.
🤖 (medium) The propagation this PR exists to add is not pinned by any test: I applied two mutations in a temp tree (the vuln-reporting fetcher swallowing real API errors again, and Setup discarding the errors.Join result and returning nil) and the full suite passed both times. The changed tests only assert NoError on success and expected-absence rows. One test that injects a failing fetch and asserts Setup returns a non-nil error naming that domain would lock the behavior in.
| } | ||
| msg := err.Error() | ||
| for _, code := range codes { | ||
| if strings.Contains(msg, fmt.Sprintf("%d", code)) { |
There was a problem hiding this comment.
🤖 (low, non-blocking) Matching "403"/"404" as substrings of the whole error text can misclassify: MakeApiCall and the retry wrapper embed the full endpoint URL in the message, so a repo or owner named "404" (these exist on GitHub) turns a real outage into an "expected absence" and silently drops it from the joined warn log. Matching against a typed status (or anchoring on "status code: 404" style substrings) avoids it. Verdicts are unaffected either way, hence non-blocking.
2 participants
Fixes #42