Fix user list error handling.

[ggreer]

The old code didn't handle transient errors from github like 502, causing syncs to fail and start over.

Summary by CodeRabbit

• Bug Fixes
  • Improved error handling for user list operations. The system now gracefully handles not-found errors with a fallback mechanism and warning notification, while other errors are reported more consistently.

[coderabbitai]

📝 Walkthrough

Walkthrough

Modified error handling in the List function to treat not-found errors as recoverable by logging and falling back to the original user, while replacing rate-limit-specific error handling with a generalized error wrapping approach.

Changes

Cohort / File(s)	Summary
Error handling refactoring pkg/connector/user.go	Replaced rate-limit error handling with not-found error detection and recovery logic. Not-found errors now trigger a warning log and fallback to the original user, while other errors are delegated to wrapGitHubError for consistent error wrapping.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 When users vanish without a trace,
We log a hop and keep the pace,
No rate-limit walls block our way,
Just kindly fallback, come what may! 🌟

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix user list error handling' is directly related to the main change—it addresses error handling in the user list flow to handle transient GitHub errors.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch ggreer/error-handling

---

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

[github-actions]

Claude finished @ggreer's task in 2m 2s —— View job

---

PR Review: Fix user list error handling

Issues: 0

Breaking Changes

None.

Issues

None found. The refactoring correctly replaces inline error handling with the centralized wrapGitHubError helper, which provides:

• Rate limit handling (helpers.go:317-332) — covers RateLimitError, AbuseRateLimitError, and response-based 429/403 detection. The old inline isRatelimited check is now handled inside wrapGitHubError.
• Transient error handling (helpers.go:334-337) — 502, 503, 504 responses are now wrapped as codes.Unavailable, allowing the SDK to retry instead of failing the sync. This is the key fix described in the PR.
• Nil response safety — both isNotFoundError and wrapGitHubError check for resp == nil before accessing fields.
• Graceful degradation preserved — 404 still falls back to the original user object with a warning log, which is correct for this undocumented API.

The log level change from Error to Warn for the 404 case is also appropriate since this is expected behavior, not an error condition.

Documentation

Up to date — no capabilities, config, or credential changes.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/connector/user.go`:
- Around line 156-160: The code currently returns an error for any non-404
response in the GetByID handling (calling wrapGitHubError), which aborts sync on
transient GitHub failures; add a helper isTransientGitHubError(resp
*github.Response) that returns true for 502, 503, 504 and, in the block where
isNotFoundError(res) is checked, treat transient responses the same as not-found
by assigning u = user instead of returning; only call wrapGitHubError and return
for truly non-recoverable responses. Ensure you reference the existing
isNotFoundError(res) and wrapGitHubError(err, res, "...") symbols when making
the change.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3098cae1-5797-418d-81ea-ad7236c53fef

📥 Commits

Reviewing files that changed from the base of the PR and between a907de3 and 1b24b5e.

📒 Files selected for processing (1)

• pkg/connector/user.go

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Treat transient GetByID errors as recoverable too

At Line 160, non-404 errors still abort the sync. That means transient GitHub failures (e.g., 502/503/504) can still restart the operation, which conflicts with this PR’s objective. Consider falling back to u = user for transient HTTP errors as well, and only returning on non-recoverable failures.

Proposed fix

 		u, res, err := o.client.Users.GetByID(ctx, user.GetID())
 		if err != nil {
 			// This undocumented API can return 404 for some users. If this fails it means we won't get some of their details like email
-			if isNotFoundError(res) {
+			if isNotFoundError(res) || isTransientGitHubError(res) {
 				l.Warn("error fetching user by id", zap.Error(err), zap.Int64("user_id", user.GetID()))
 				u = user
 			} else {
 				return nil, nil, wrapGitHubError(err, res, "github-connector: failed to get user by id")
 			}
 		}

func isTransientGitHubError(resp *github.Response) bool {
	if resp == nil {
		return false
	}
	switch resp.StatusCode {
	case 502, 503, 504:
		return true
	default:
		return false
	}
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/user.go` around lines 156 - 160, The code currently returns an
error for any non-404 response in the GetByID handling (calling
wrapGitHubError), which aborts sync on transient GitHub failures; add a helper
isTransientGitHubError(resp *github.Response) that returns true for 502, 503,
504 and, in the block where isNotFoundError(res) is checked, treat transient
responses the same as not-found by assigning u = user instead of returning; only
call wrapGitHubError and return for truly non-recoverable responses. Ensure you
reference the existing isNotFoundError(res) and wrapGitHubError(err, res, "...")
symbols when making the change.

[ggreer]

wrapGitHubError() returns codes.Unavailable for 502/503/etc, which causes syncer to retry the List() call.

[coderabbitai]

@ggreer, thanks for the clarification! That makes sense — since wrapGitHubError() maps 502/503/504 responses to codes.Unavailable, the syncer will automatically retry the List() call rather than treating it as a fatal failure or restarting the entire sync from scratch. The current approach is correct and the concern I raised doesn't apply here.

---

✏️ Learnings added

Learnt from: ggreer
Repo: ConductorOne/baton-github PR: 127
File: pkg/connector/user.go:156-160
Timestamp: 2026-03-05T21:28:54.578Z
Learning: In the `baton-github` connector (`pkg/connector/user.go`), `wrapGitHubError()` returns `codes.Unavailable` for transient HTTP errors (502, 503, 504 etc.), which causes the syncer to retry the `List()` call rather than aborting or restarting the full sync. This means returning `wrapGitHubError(err, res, ...)` for non-404 errors in `GetByID` is intentional and correct — transient errors are already handled via the retry mechanism.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Incorrect assumptions