Fix custom detector verification: add retry, configurable success ranges, and inconclusive state#4821
Conversation
![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.
| } | ||
|
|
||
| var httpClient = common.SaneHttpClient() | ||
| var httpClient = common.RetryableHTTPClient() |
There was a problem hiding this comment.
Retryable client may mask user-configured success status codes
Medium Severity
RetryableHTTPClient automatically retries 429 and 5xx responses (via ErrorPropagatedRetryPolicy), but users can now configure those same codes as success via successRanges. If a server returns e.g. 429 (user considers it "verified") and the retry yields 200 (not in their successRanges), isStatusInRanges returns false — producing a false negative. The two features introduced in this PR conflict with each other for retry-eligible status codes.
Additional Locations (1)
There was a problem hiding this comment.
Hmmm. Users would configure 429 and 5xx as success status codes?
There was a problem hiding this comment.
We shouldn’t make assumptions when offering a custom detector option. It needs to remain fully configurable, even for cases we might think aren’t necessary. If a retryable client is desired, it should also be exposed through configuration, with a clear warning that enabling it will retry on 429/500 status codes. Otherwise, users can stick with the simple client.
| } | ||
|
|
||
| // ClearVerificationError resets the verification error to nil. | ||
| func (r *Result) ClearVerificationError() { |
There was a problem hiding this comment.
I do not understand the purpose of this? Why would we want to add a verification error to the result and than remove it later? In which scenarios this will be useful?
There was a problem hiding this comment.
Successful verification should clear the prior error for a result.
2 participants
Summary
Fixes three issues with custom detector verification that caused false negatives and poor resilience against transient failures:
SaneHttpClienttoRetryableHTTPClient, giving custom detectors automatic retry with exponential backoff on 429 (rate-limit) and 5xx responses (3 retries).successRangesfield onVerifierConfig. Users can now specify status codes (e.g."403") or ranges (e.g."200-299") that count as verified. Defaults to200only when no ranges are configured (backward compatible). Validation viaValidateVerifyRangesis now called during detector initialization.SetVerificationError: Network failures and timeouts now callresult.SetVerificationError(err)instead of silently leavingVerified = false. This marks the result as "unknown" (inconclusive) rather than definitively unverified, preventing false negatives. A subsequent verify config that responds with a clear non-success status will clear the error viaClearVerificationError(). This matches the three-state model used by built-in detectors (Slack, SendGrid, OpenAI, etc.) and integrates with the existing--results=unknownfiltering in the engine.Changes
pkg/custom_detectors/custom_detectors.go: retryable client, success ranges check, verification error handling,isStatusInRangeshelperpkg/detectors/detectors.go: addedClearVerificationError()method onResultpkg/custom_detectors/custom_detectors_test.go: tests for success ranges, verification error on network failure, error clearing on definitive rejection, invalid range validationChecklist:
make test-community)?make lintthis requires golangci-lint)?Note
Medium Risk
Changes custom detector verification semantics and HTTP behavior (retries, configurable success status codes, and new inconclusive error state), which may affect verified/unverified classification and downstream filtering.
Overview
Custom detector webhook verification is now more resilient and expressive. Verification requests switch to
common.RetryableHTTPClient()and network failures/timeouts now set a verification error (inconclusive) instead of silently leaving results unverified.Verification success is now determined by configurable
successRanges(single codes like"403"or ranges like"200-299"), validated duringNewWebhookCustomRegexinit and implemented viaisStatusInRanges. AddsResult.ClearVerificationError()and logic to clear prior inconclusive errors when a later verifier gives a definitive non-success or success response, with new tests covering these behaviors and invalid ranges.Written by Cursor Bugbot for commit 625c881. This will update automatically on new commits. Configure here.