Repository navigation
Fix/repo sync river jobs - #148
Conversation
Implement Repository Sync Error Handling and Webhook ValidationOverviewThis change addresses Technical Highlights
Impact
|
![livereview-agent[bot]](https://avatars.githubusercontent.com/in/5380735?s=60&v=4){kind=small}
|
|
||
| // Guard against webhook-to-connector provider mismatch: e.g. a GitHub webhook | ||
| // sent to a GitLab connector endpoint (/github-hook/:gitlab_connector_id). | ||
| if connProvider, ok := auth.GetConnectorProviderFromContext(c); ok && connProvider != "" { |
There was a problem hiding this comment.
Severity: warning
Please check the IsProviderCompatible function for any potential mismatches.
Suggestions:
- Ensure
providers.IsProviderCompatiblehandles all expected provider types and compatibility rules correctly.
| Queues: config.RiverQueueConfig(), | ||
| Workers: workers, | ||
| CompletedJobRetentionPeriod: 30 * 24 * time.Hour, | ||
| CancelledJobRetentionPeriod: 30 * 24 * time.Hour, |
There was a problem hiding this comment.
Severity: warning
Consider reducing the CancelledJobRetentionPeriod to 7 days.
Suggestions:
- Ensure 7 days is sufficient for debugging cancelled jobs before deletion. This change directly addresses the bloat issue.
| return river.JobCancel(fmt.Errorf("fatal provider mismatch for repository %d: repo provider %q != connector %q", args.RepositoryID, info.Provider, info.ConnectorProvider)) | ||
| } | ||
|
|
||
| if strings.TrimSpace(info.token()) == "" || info.token() == "NA" { |
There was a problem hiding this comment.
Severity: warning
The magic string 'NA' is used for a missing token. Please confirm its reliability.
Suggestions:
- Define 'NA' as a named constant if it's a specific sentinel value.
- Consider if an empty string or nil token should be sufficient for this check.
| } | ||
| if err != nil { | ||
| var mismatchErr *pullrequests.ProviderMismatchError | ||
| if errors.As(err, &mismatchErr) || strings.Contains(err.Error(), "incompatible with connector") { |
There was a problem hiding this comment.
Severity: warning
The error string check is brittle. It would be preferable to check for a specific error type.
Suggestions:
- Ensure
pullrequests.ProviderMismatchErroris always returned or wrapped when this condition occurs. - Avoid parsing error messages for control flow; rely on error types.
| }, &river.InsertOpts{Queue: "repo_sync", MaxAttempts: 5}) | ||
| }, &river.InsertOpts{ | ||
| Queue: "repo_sync", | ||
| MaxAttempts: 3, |
There was a problem hiding this comment.
Severity: info
The MaxAttempts has been reduced from 5 to 3. Please confirm if this is sufficient.
Suggestions:
- Verify if 3 attempts are sufficient for transient repo sync issues.
- Consider if specific error types should trigger different retry policies.
| log.Printf("[ERROR] ScheduledReviewWorker: failed to load integration token %d for config %d: %v", cfg.ConnectorID, cfg.ID, err) | ||
| run.ErrorMessage = err.Error() | ||
| if err == sql.ErrNoRows { | ||
| _ = store.UpdateCheckpoint(ctx, cfg.ID, cfg.DefaultBranch.String, cfg.LastSyncedSHA.String, time.Now(), nextRunAt) |
There was a problem hiding this comment.
Severity: warning
The error returned by UpdateCheckpoint is being ignored. This could potentially mask checkpoint update failures.
Suggestions:
- Log errors from
store.UpdateCheckpointto ensure visibility if checkpoint updates fail.
| run.ErrorMessage = err.Error() | ||
| if providers.IsAuthError(err) { | ||
| log.Printf("[WARN] ScheduledReviewWorker: auth error resolving GitHub app token for config %d: %v; cancelling job", cfg.ID, err) | ||
| _ = store.UpdateCheckpoint(ctx, cfg.ID, cfg.DefaultBranch.String, cfg.LastSyncedSHA.String, time.Now(), nextRunAt) |
There was a problem hiding this comment.
Severity: warning
The error returned by UpdateCheckpoint is being ignored. This could potentially mask checkpoint update failures.
Suggestions:
- Log errors from
store.UpdateCheckpointto ensure visibility if checkpoint updates fail.
| if strings.TrimSpace(patToken) == "" { | ||
| run.ErrorMessage = "connector access token is missing" | ||
| log.Printf("[WARN] ScheduledReviewWorker: connector %d has empty access token; cancelling job", cfg.ConnectorID) | ||
| _ = store.UpdateCheckpoint(ctx, cfg.ID, cfg.DefaultBranch.String, cfg.LastSyncedSHA.String, time.Now(), nextRunAt) |
There was a problem hiding this comment.
Severity: warning
The error returned by UpdateCheckpoint is being ignored. This could potentially mask checkpoint update failures.
Suggestions:
- Log errors from
store.UpdateCheckpointto ensure visibility if checkpoint updates fail.
| run.ErrorMessage = err.Error() | ||
| if providers.IsAuthError(err) { | ||
| log.Printf("[WARN] ScheduledReviewWorker: auth error resolving default branch for %s (config %d): %v; cancelling job", cfg.ProjectFullName, cfg.ID, err) | ||
| _ = store.UpdateCheckpoint(ctx, cfg.ID, cfg.DefaultBranch.String, cfg.LastSyncedSHA.String, time.Now(), nextRunAt) |
There was a problem hiding this comment.
Severity: warning
The error returned by UpdateCheckpoint is being ignored. This could potentially mask checkpoint update failures.
Suggestions:
- Log errors from
store.UpdateCheckpointto ensure visibility if checkpoint updates fail.
| run.ErrorMessage = err.Error() | ||
| if providers.IsAuthError(err) { | ||
| log.Printf("[WARN] ScheduledReviewWorker: auth error resolving branch head for %s (config %d): %v; cancelling job", cfg.ProjectFullName, cfg.ID, err) | ||
| _ = store.UpdateCheckpoint(ctx, cfg.ID, branch, cfg.LastSyncedSHA.String, time.Now(), nextRunAt) |
There was a problem hiding this comment.
Severity: warning
The error returned by UpdateCheckpoint is being ignored. This could potentially mask checkpoint update failures.
Suggestions:
- Log errors from
store.UpdateCheckpointto ensure visibility if checkpoint updates fail.
| run.ErrorMessage = err.Error() | ||
| if providers.IsAuthError(err) { | ||
| log.Printf("[WARN] ScheduledReviewWorker: auth error resolving lookback commit for %s (config %d): %v; cancelling job", cfg.ProjectFullName, cfg.ID, err) | ||
| _ = store.UpdateCheckpoint(ctx, cfg.ID, branch, headSHA, time.Now(), nextRunAt) |
There was a problem hiding this comment.
Severity: warning
The error returned by UpdateCheckpoint is being ignored. This could potentially mask checkpoint update failures.
Suggestions:
- Log errors from
store.UpdateCheckpointto ensure visibility if checkpoint updates fail.
| run.ErrorMessage = err.Error() | ||
| if providers.IsAuthError(err) { | ||
| log.Printf("[WARN] ScheduledReviewWorker: auth error fetching compare diff for %s (config %d): %v; cancelling job", cfg.ProjectFullName, cfg.ID, err) | ||
| _ = store.UpdateCheckpoint(ctx, cfg.ID, branch, headSHA, time.Now(), nextRunAt) |
There was a problem hiding this comment.
Severity: warning
The error returned by UpdateCheckpoint is being ignored. This could potentially mask checkpoint update failures.
Suggestions:
- Log errors from
store.UpdateCheckpointto ensure visibility if checkpoint updates fail.
| if strings.Contains(msg, "rate limit") || strings.Contains(msg, "too many requests") { | ||
| return false | ||
| } | ||
| return strings.Contains(msg, "403") && ( |
There was a problem hiding this comment.
Severity: warning
Generic string matching for 'status' or 'http' is too broad and can misclassify non-403 errors.
Suggestions:
- Rely primarily on
AuthError.StatusCodeforIsForbiddenError. - Remove generic string checks like 'status' or 'http' if not an
AuthError.
| // - Bracketed status format (Azure DevOps, Gitea): "(401)" | ||
| has401 := strings.Contains(msg, "401") && ( | ||
| strings.Contains(msg, "unauthorized") || | ||
| strings.Contains(msg, "status") || |
There was a problem hiding this comment.
Severity: warning
Generic string matching for 'status' or 'http' is too broad and can misclassify non-401 errors.
Suggestions:
- Rely primarily on
AuthError.StatusCodeforIsAuthError. - Remove generic string checks like 'status' or 'http' if not an
AuthError.
| // - "403 forbidden", "status 403", "http 403", "(403)" | ||
| has403 := strings.Contains(msg, "403") && ( | ||
| strings.Contains(msg, "forbidden") || | ||
| strings.Contains(msg, "status") || |
There was a problem hiding this comment.
Severity: warning
Generic string matching for 'status' or 'http' is too broad and can misclassify non-403 errors.
Suggestions:
- Rely primarily on
AuthError.StatusCodeforIsAuthError. - Remove generic string checks like 'status' or 'http' if not an
AuthError.
| if r.ConnectorID > 0 { | ||
| var connProvider string | ||
| err := s.db.QueryRow(`SELECT provider FROM integration_tokens WHERE id = $1`, r.ConnectorID).Scan(&connProvider) | ||
| if err != nil && err != sql.ErrNoRows { |
There was a problem hiding this comment.
Severity: warning
When a database query error occurs on the connector ID, it is being handled with a general fmt.Errorf. It would be better to return a specific error or log the error instead.
Suggestions:
- Return a specific error type (e.g.,
ErrConnectorNotFound) ifsql.ErrNoRowsis encountered, or log the specific DB error. - Consider if
ConnectorIDnot found should be aProviderMismatchErroror a distinct error.
| if err != nil && err != sql.ErrNoRows { | ||
| return 0, fmt.Errorf("failed to query connector %d for provider validation: %w", r.ConnectorID, err) | ||
| } | ||
| if err == nil && connProvider != "" && !providers.IsProviderCompatible(r.Provider, connProvider) { |
There was a problem hiding this comment.
Severity: warning
If sql.ErrNoRows is returned for ConnectorID, connProvider becomes empty, and the compatibility check is skipped. Is this the intended behavior?
Suggestions:
- Clarify if a non-existent
ConnectorIDshould bypass the provider compatibility check. - Consider treating
sql.ErrNoRowsas an error indicating a missing connector, or explicitly handle it as a 'no provider to check against' scenario.
| if r.ConnectorID > 0 { | ||
| var connProvider string | ||
| err := s.db.QueryRow(`SELECT provider FROM integration_tokens WHERE id = $1`, r.ConnectorID).Scan(&connProvider) | ||
| if err != nil && err != sql.ErrNoRows { |
There was a problem hiding this comment.
Severity: warning
When a database query error occurs on the connector ID, it is being handled with a general fmt.Errorf. It would be better to return a specific error or log the error instead.
Suggestions:
- Return a specific error type (e.g.,
ErrConnectorNotFound) ifsql.ErrNoRowsis encountered, or log the specific DB error. - Consider if
ConnectorIDnot found should be aProviderMismatchErroror a distinct error.
| if err != nil && err != sql.ErrNoRows { | ||
| return 0, fmt.Errorf("failed to query connector %d for provider validation: %w", r.ConnectorID, err) | ||
| } | ||
| if err == nil && connProvider != "" && !providers.IsProviderCompatible(r.Provider, connProvider) { |
There was a problem hiding this comment.
Severity: warning
If sql.ErrNoRows is returned for ConnectorID, connProvider becomes empty, and the compatibility check is skipped. Is this the intended behavior?
Suggestions:
- Clarify if a non-existent
ConnectorIDshould bypass the provider compatibility check. - Consider treating
sql.ErrNoRowsas an error indicating a missing connector, or explicitly handle it as a 'no provider to check against' scenario.
| }) | ||
| if err == nil { | ||
| t.Fatal("expected error when upserting github repo with gitlab connector, got nil") | ||
| } |
There was a problem hiding this comment.
Severity: warning
When asserting an error, please assert the specific error type ProviderMismatchError instead of just checking err != nil.
Suggestions:
- Use
errors.Asorerrors.Isto assert that the returned error is specifically*ProviderMismatchErrorto ensure correct error propagation.
| FullName: "acme/github-repo", | ||
| Name: "github-repo", | ||
| }) | ||
| if err == nil { |
There was a problem hiding this comment.
Severity: warning
When asserting an error, please assert the specific error type ProviderMismatchError instead of just checking err != nil.
Suggestions:
- Use
errors.Asorerrors.Isto assert that the returned error is specifically*ProviderMismatchErrorto ensure correct error propagation.
| for (const repo of repos) { | ||
| if (repo.last_sync_status !== 'error' || !repo.last_sync_error) continue; | ||
| const err = repo.last_sync_error.toLowerCase(); | ||
| const isForbidden = |
There was a problem hiding this comment.
Severity: warning
A 403 (Forbidden) status code is often related to authentication. Please consider if isForbidden should classify as an auth kind.
Suggestions:
- Re-evaluate if 'forbidden' errors should be categorized as 'auth' for user action consistency.
- If distinct, ensure UI actions for 403 vs 401 are clear.
| const [connectors, repoResp] = await Promise.all([ | ||
| getConnectors(), | ||
| // Only pull error repos — targeted scan, not the full table. | ||
| getRepositories({ syncStatus: 'error', page: 1, perPage: 100 }), |
There was a problem hiding this comment.
Severity: warning
Only the first 100 error repositories are fetched, which may result in missed alerts if there are more errors.
Suggestions:
- Implement pagination to fetch all error repositories.
- Increase
perPagelimit if backend supports more than 100.
| }) | ||
| ); | ||
| }); | ||
| } catch { |
There was a problem hiding this comment.
Severity: info
Alerts are silently skipped when a fetch error occurs. Consider logging these errors for better debuggability.
Suggestions:
- Add a
console.erroror logging mechanism to capture fetch failures.
| }); | ||
| }; | ||
|
|
||
| const alerts = allAlerts.filter((a) => !dismissed.has(a.connector.id)); |
There was a problem hiding this comment.
Severity: warning
The dismissAlert function updates the local state, but it does not update the Redux notification state for ConnectorAlerts.
Suggestions:
- Dispatch
dismissNotificationforConnectorAlertswhendismissAlertis called, to keep Redux state consistent. - Ensure
ConnectorAlertBanneris rendered from Redux state if Redux is source of truth for all notifications.
| useEffect(() => { | ||
| const progress = dashboardData?.connector_setup_progress || []; | ||
| progress.forEach((connector) => { | ||
| dispatch( |
There was a problem hiding this comment.
Severity: info
A dedupeKey is missing for setup progress notifications. This could lead to duplicate notifications being added to Redux.
Suggestions:
- Add
dedupeKey: connectorNotificationId(connector.connector_id)toaddNotificationfor setup progress.
| useEffect(() => { | ||
| const progress = dashboardData?.connector_setup_progress || []; | ||
| progress.forEach((connector) => { | ||
| dispatch( |
There was a problem hiding this comment.
Severity: warning
A title is missing for setup progress notifications, which is inconsistent with how error alerts are displayed.
Suggestions:
- Add a
titleproperty to setup progress notifications for consistency and clarity. - Use
getPhaseMessageto generate a suitable title.
| useEffect(() => { | ||
| const progress = dashboardData?.connector_setup_progress || []; | ||
| progress.forEach((connector) => { | ||
| dispatch( |
There was a problem hiding this comment.
Severity: warning
The actions property is missing for setup progress notifications, which is inconsistent with how error alerts are displayed.
Suggestions:
- Add relevant
actions(e.g., 'View Details', 'Retry Setup') to setup progress notifications for better UX.
| connector.phase, | ||
| connector.connector_name, | ||
| connector.provider, | ||
| connector.total_projects, |
There was a problem hiding this comment.
Severity: critical
The definitions for getPhaseVariant and getPhaseMessage are missing.
Suggestions:
- Define
getPhaseVariantandgetPhaseMessagefunctions within this component or import them from a utility file.
| // URL: github.com uses api.github.com, while GitHub Enterprise instances serve | ||
| // their API under /api/v3 of the same host. | ||
| func apiBaseURL(baseURL string) string { | ||
| if baseURL == "" || baseURL == "https://github.com" { |
There was a problem hiding this comment.
Severity: info
TrimSpace has been added for robustness against leading/trailing whitespace.
| func apiBaseURL(baseURL string) string { | ||
| if baseURL == "" || baseURL == "https://github.com" { | ||
| u := strings.TrimRight(strings.TrimSpace(baseURL), "/") | ||
| if u == "" || u == "https://github.com" || u == "http://github.com" || u == "github.com" || u == "https://api.github.com" { |
There was a problem hiding this comment.
Severity: info
The canonical URL https://api.github.com is now explicitly handled.
| return "https://api.github.com" | ||
| } | ||
| return strings.TrimSuffix(baseURL, "/") + "/api/v3" | ||
| if strings.HasSuffix(u, "/api/v3") { |
There was a problem hiding this comment.
Severity: warning
The double appending of /api/v3 has been prevented, ensuring idempotency.
Suggestions:
- This check prevents
ghe.example.com/api/v3/api/v3.
| defer resp.Body.Close() | ||
| if resp.StatusCode != 200 { | ||
| body, _ := io.ReadAll(resp.Body) | ||
| if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden { |
There was a problem hiding this comment.
Severity: info
Authentication error handling has been standardized with a specific AuthError type.
| defer resp.Body.Close() | ||
| if resp.StatusCode != 200 { | ||
| body, _ := io.ReadAll(resp.Body) | ||
| if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden { |
There was a problem hiding this comment.
Severity: info
Authentication error handling has been standardized with a specific AuthError type.
| defer resp.Body.Close() | ||
| if resp.StatusCode != 200 { | ||
| body, _ := io.ReadAll(resp.Body) | ||
| if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden { |
There was a problem hiding this comment.
Severity: info
Authentication error handling has been standardized with a specific AuthError type.
| defer resp.Body.Close() | ||
| if resp.StatusCode != 200 { | ||
| body, _ := io.ReadAll(resp.Body) | ||
| if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden { |
There was a problem hiding this comment.
Severity: info
Authentication error handling has been standardized with a specific AuthError type.
| return nil, rlErr | ||
| } | ||
|
|
||
| if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden { |
There was a problem hiding this comment.
Severity: info
Authentication error handling has been standardized with a specific AuthError type.
| if resp.StatusCode != http.StatusOK { | ||
| body, _ := io.ReadAll(resp.Body) | ||
| return nil, fmt.Errorf("GitHub API request failed with status %d: %s", resp.StatusCode, string(body)) | ||
| return nil, fmt.Errorf("GitHub API request failed with status %d: %s", resp.StatusCode, providers.FormatAPIResponseBody(body)) |
There was a problem hiding this comment.
Severity: info
FormatAPIResponseBody is now used for consistent error messages.
| if resp.StatusCode != http.StatusOK { | ||
| body, _ := io.ReadAll(resp.Body) | ||
| return nil, fmt.Errorf("API request failed with status %d: %s", resp.StatusCode, string(body)) | ||
| return nil, fmt.Errorf("API request failed with status %d: %s", resp.StatusCode, providers.FormatAPIResponseBody(body)) |
There was a problem hiding this comment.
Severity: info
FormatAPIResponseBody is now used for consistent error messages.
| func NewHTTPClient(baseURL, token string) *GitLabHTTPClient { | ||
| return &GitLabHTTPClient{ | ||
| baseURL: fmt.Sprintf("%s/api/v4", baseURL), | ||
| baseURL: normalizeGitLabBaseURL(baseURL), |
There was a problem hiding this comment.
Severity: info
normalizeGitLabBaseURL is now used for consistent URL generation.
| defer resp.Body.Close() | ||
|
|
||
| // Check for errors | ||
| if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden { |
There was a problem hiding this comment.
Severity: info
Authentication error handling has been standardized with a specific AuthError type.
| if resp.StatusCode != http.StatusOK { | ||
| body, _ := io.ReadAll(resp.Body) | ||
| return nil, "", fmt.Errorf("API request failed with status %d: %s", resp.StatusCode, string(body)) | ||
| return nil, "", fmt.Errorf("API request failed with status %d: %s", resp.StatusCode, providers.FormatAPIResponseBody(body)) |
There was a problem hiding this comment.
Severity: info
FormatAPIResponseBody is now used for consistent error messages.
| "net/http" | ||
| "net/url" | ||
| "strconv" | ||
| "strings" |
There was a problem hiding this comment.
Severity: info
The unused strings import has been removed.
| params.Add("page", strconv.Itoa(page)) | ||
| params.Add("per_page", "100") | ||
| requestURL := fmt.Sprintf("%s/api/v4/projects?%s", strings.TrimSuffix(baseURL, "/"), params.Encode()) | ||
| requestURL := fmt.Sprintf("%s/projects?%s", normalizeGitLabBaseURL(baseURL), params.Encode()) |
There was a problem hiding this comment.
Severity: info
normalizeGitLabBaseURL is now used for consistent URL generation.
| if resp.StatusCode != http.StatusOK { | ||
| body, _ := io.ReadAll(resp.Body) | ||
| return nil, fmt.Errorf("GitLab API request failed with status %d: %s", resp.StatusCode, string(body)) | ||
| return nil, fmt.Errorf("GitLab API request failed with status %d: %s", resp.StatusCode, providers.FormatAPIResponseBody(body)) |
There was a problem hiding this comment.
Severity: info
FormatAPIResponseBody is now used for consistent error messages.
| return fmt.Sprintf("%s authentication failed: %s", e.Provider, e.Message) | ||
| } | ||
|
|
||
| func isHTTPStatusCode(msg, code, text string) bool { |
There was a problem hiding this comment.
Severity: warning
The regex for isHTTPStatusCode is more robust; the current string contains can have false positives (e.g., 'status 200 OK' containing 'status 20').
Suggestions:
- Use regular expressions for more precise matching of status codes, e.g.,
\bstatus\s*\d{3}\b. - Consider parsing HTTP status code from error string if possible, rather than generic
strings.Contains.
| return false | ||
| } | ||
| } | ||
| msg := strings.ToLower(err.Error()) |
There was a problem hiding this comment.
Severity: info
Lowercase err.Error() once, and then use the msg variable.
Suggestions:
- Assign
strings.ToLower(err.Error())tomsgonce at the beginning of the function to avoid repeated conversions.
| if strings.Contains(msg, "rate limit") || strings.Contains(msg, "too many requests") { | ||
| return false | ||
| } | ||
| return isHTTPStatusCode(msg, "403", "forbidden") |
There was a problem hiding this comment.
Severity: warning
String matching for 403 forbidden might have false positives; see the comment for isHTTPStatusCode.
Suggestions:
- Improve
isHTTPStatusCodeto use regex for more precise matching.
| // IsAuthError returns true if err or any error in its chain is an *AuthError, | ||
| // or contains authentication error indicators for the specified provider (or across all | ||
| // known providers if no provider is passed). | ||
| func IsAuthError(err error, providerOpt ...string) bool { |
There was a problem hiding this comment.
Severity: warning
The providerOpt ...string allows multiple providers, but the function only uses the first. Clarify the intent or change the signature.
Suggestions:
- Change
providerOpt ...stringtoprovider stringif only one provider is ever expected. - If multiple providers are intended, iterate through
providerOptand apply logic for each.
| } | ||
|
|
||
| // Common credential errors across all providers: | ||
| if strings.Contains(msg, "authentication failed") || strings.Contains(msg, "oauth token is invalid") { |
There was a problem hiding this comment.
Severity: warning
oauth token is invalid is very generic and could match non-authentication errors. Consider using more specific patterns.
Suggestions:
- Review logs/error messages for false positives.
- Refine pattern to be more specific, e.g.,
oauth: invalid tokenoroauth2: invalid token.
|
|
||
| // Provider-specific checks: | ||
| switch { | ||
| case strings.HasPrefix(provider, "github"): |
There was a problem hiding this comment.
Severity: warning
Provider-specific checks should handle providerOpt being empty or nil for broader matching.
Suggestions:
- Ensure
providervariable is correctly set to an empty string ifproviderOptis empty, sostrings.HasPrefixworks as expected for the default case.
| default: | ||
| // When no specific provider is passed, check across all known provider signatures: | ||
| authSignatures := []string{ | ||
| "bad credentials", |
There was a problem hiding this comment.
Severity: warning
There is a duplicate invalid token in authSignatures (lines 234, 235). Remove one.
Suggestions:
- Remove the duplicate entry
"invalid token"fromauthSignatures.
| default: | ||
| // When no specific provider is passed, check across all known provider signatures: | ||
| authSignatures := []string{ | ||
| "bad credentials", |
There was a problem hiding this comment.
Severity: warning
There is a duplicate token expired in authSignatures (lines 236, 238). Remove one.
Suggestions:
- Remove the duplicate entry
"token expired"fromauthSignatures.
| func IsProviderCompatible(repoProvider, connProvider string) bool { | ||
| repoNorm := strings.ToLower(repoProvider) | ||
| connNorm := strings.ToLower(connProvider) | ||
| if strings.HasPrefix(repoNorm, "github") && (strings.HasPrefix(connNorm, "github") || connNorm == "github_app") { |
There was a problem hiding this comment.
Severity: warning
connNorm == "github_app" should be strings.HasPrefix(connNorm, "github") to include github-enterprise.
Suggestions:
- Change
connNorm == "github_app"tostrings.HasPrefix(connNorm, "github")for consistency and to correctly matchgithub-enterprise.
| return "" | ||
| } | ||
| lower := strings.ToLower(s) | ||
| for _, tag := range []string{"<!doctype", "<html", "<head"} { |
There was a problem hiding this comment.
Severity: warning
HTML tag detection could be more robust; for example, by checking for the Content-Type: text/html header first.
Suggestions:
- If possible, pass HTTP response headers to
FormatAPIResponseBodyand checkContent-Type. - Consider using a more sophisticated HTML detection library if simple string checks become insufficient.
There was a problem hiding this comment.
❌ LiveReview policy: Not approved (blocked at Eligibility) · Review #13600
| Stage | Result | Detail |
|---|---|---|
| Eligibility | ❌ | author level 1 < required 2 |
| Static rules | ⏭️ | skipped (earlier stage blocked) |
| Risk score | ⏭️ | skipped (earlier stage blocked) |
| LLM classification | ⏭️ | skipped (earlier stage blocked) |
| Final decision | ❌ | eligibility: author level 1 < required 2 |
There was a problem hiding this comment.
⚠️ LiveReview policy: Force-approved
Force-approved by lince@hexmos.com despite a policy block.
Reason: Most of critical comments were critcal infromation about the changes. These risk i know it. So, I am approving it.
This override is recorded in LiveReview's audit log.
🔍 LiveReview
⚙️ Customize risk-based approval policy for this repository or your organization.
Summary
Fix
river_jobBloat:river.JobCancel) unauthenticated sync jobs on 401/403/missing token instead of retrying 5 times with large error traces.ReconciliationSweepWorkerso failing repositories are not re-enqueued every 15 minutes.ByArgs: true, 15m period) and reducedMaxAttemptsfrom 5 to 3 forrepo_pr_sync.Provider Compatibility & Error Classification:
Frontend Connector Alerts & Dismissal:
/dashboard,/reviews,/explore/repositories, and/git.connector-${kind}-${connectorId}).Agent Guidelines:
AGENTS.md.Linked Issue
Fixes production
river_jobtable bloat caused by infinite retry loops on unauthenticated repositories.Validation
go test -v ./internal/jobqueue -run "TestRepoPRSync|TestScheduledReview"go test -v ./internal/api -run "TestWebhookOrchestratorV2_RejectsProviderMismatch"npm test -- src/store/Notifications/__tests__/(14/14 tests passed)npm test -- src/components/reviews/diffviewer/reviewStreamState.test.ts(21/21 passed)npm test -- src/lib/blastRadius.test.ts(9/9 passed)npx tsc --noEmitpassed with 0 errors.Checklist
SECURITY.mdand updated related documentation if needed.Notes For Reviewers
master.LiveReview auto-approval decisions