fix(github-api): handle authentication errors in add_api_users_to_auto_verified_and_merged_users #984
Conversation
…o_verified_and_merged_users Previously, if a GitHub API token failed authentication with a 401 error, the GithubException was not caught, causing the server to crash. Added try/except block to catch GithubException and log a warning instead of crashing, allowing the server to continue with other valid tokens.
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
Walkthroughprocess() now awaits the moved initializer add_api_users_to_auto_verified_and_merged_users(), which asynchronously checks each (api, token) pair via asyncio.to_thread for rate limits and API user, logs token-scoped warnings on errors or low quota, and appends valid API user logins to auto-verified lists. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
|
/build-and-push-container |
|
New container for ghcr.io/myk-org/github-webhook-server:pr-984 published |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…fix/api-auth-error-handling
|
/build-and-push-container |
|
New container for ghcr.io/myk-org/github-webhook-server:pr-984 published |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@webhook_server/libs/github_api.py`:
- Around line 607-615: Replace the use of self.logger.error in the except block
that catches GithubException when calling _api.get_user() with
self.logger.exception so the traceback is automatically recorded; keep the
existing message context (including self.log_prefix and token_suffix) and the
continue behavior, leaving the surrounding logic that appends _api_user to
self.auto_verified_and_merged_users unchanged.
myakove-bot
added
commented-rnetser
commented-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
labels
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@webhook_server/libs/github_api.py`:
- Around line 596-615: In add_api_users_to_auto_verified_and_merged_users,
accessing _api.rate_limiting[-1] can raise GithubException for invalid tokens;
wrap the rate_limiting check in a try/except that catches GithubException (same
as the existing except used for _api.get_user()), log a clear message including
token_suffix and self.log_prefix, and continue to the next token on exception so
initialization doesn't crash; ensure you still perform the existing check when
rate_limiting is readable.
Address CodeRabbit review comment: wrap _api.rate_limiting[-1] access in try/except to handle GithubException on invalid tokens. Also fix logger.exception to logger.warning for non-exception context.
myakove-bot
added
size/S
and removed
commented-rnetser
commented-coderabbitai[bot]
changes-requested-coderabbitai[bot]
size/XS
labels
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@webhook_server/libs/github_api.py`:
- Around line 596-624: The method
add_api_users_to_auto_verified_and_merged_users performs blocking PyGithub calls
(_api.rate_limiting and _api.get_user()) and must be converted to an async
initialization or use asyncio.to_thread wrappers: make
add_api_users_to_auto_verified_and_merged_users async (or add an async variant
called from process()), and for each (_api, _token) call await
asyncio.to_thread(lambda: _api.rate_limiting) and await
asyncio.to_thread(lambda: _api.get_user()) (or equivalent callables) before
appending to self.auto_verified_and_merged_users; ensure any callers invoke the
new async method from an async context (or run it once during startup) so the
event loop is not blocked.
Address CodeRabbit review: convert blocking PyGithub calls (_api.rate_limiting and _api.get_user()) to async using asyncio.to_thread() to prevent blocking the event loop. - Convert add_api_users_to_auto_verified_and_merged_users to async - Move call from __init__ to process() (async context) - Wrap blocking PyGithub calls with asyncio.to_thread() - Update test to handle async method
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webhook_server/tests/test_github_api.py (1)
624-694: LOW: UseTEST_GITHUB_TOKENin this test.This avoids accidental drift and keeps all test tokens consistent and centrally managed. As per coding guidelines: Test files should use AsyncMock, Mock, and patch for mocking - use TEST_GITHUB_TOKEN constant for test tokens.
✅ Suggested fix
- mock_get_api.return_value = (Mock(), "token", "apiuser") + mock_get_api.return_value = (Mock(), TEST_GITHUB_TOKEN, "apiuser") @@ - mock_get_apis.return_value = [(mock_api, "token")] + mock_get_apis.return_value = [(mock_api, TEST_GITHUB_TOKEN)]
🤖 Fix all issues with AI agents
In `@webhook_server/libs/github_api.py`:
- Around line 599-627: The function
add_api_users_to_auto_verified_and_merged_users currently iterates
apis_and_tokens sequentially; refactor so each (_api, _token) is processed
concurrently via asyncio.gather over an async helper coroutine that (1) computes
token_suffix, (2) uses await asyncio.to_thread(...) to read api.rate_limiting
and api.get_user().login, (3) applies the same checks on rate_limit_remaining ==
60 and logs warnings/exceptions (using the same log messages and
self.log_prefix), and (4) returns the _api_user or None; after gather, extend
self.auto_verified_and_merged_users with non-None results while preserving
per-token skip behavior and exception logging.
Address CodeRabbit review comments: - Refactor add_api_users_to_auto_verified_and_merged_users to process all tokens concurrently using asyncio.gather instead of sequential loop - Use TEST_GITHUB_TOKEN constant in test for consistency
myakove-bot
added
size/M
commented-rnetser
commented-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
size/S
labels
|
Regarding the outside-diff-range comment about using Done - updated test to use |
2 participants
User description
Summary
GithubExceptionwhen calling_api.get_user().logininadd_api_users_to_auto_verified_and_merged_users()methodProblem
When a GitHub API token fails authentication with a 401 error, the server crashed because
GithubExceptionwas not being caught. The existing rate limit check (== 60) only catches one type of invalid token scenario.Solution
Added exception handling similar to the pattern used in
get_api_with_highest_rate_limit()inhelpers.py. Now authentication failures are logged as warnings and processing continues with the next token.Test plan
PR Type
Bug fix
Description
Add try/except block to catch GithubException in add_api_users_to_auto_verified_and_merged_users()
Include last 4 characters of token in log messages for debugging
Prevent server crash on authentication failures by continuing with next token
Diagram Walkthrough
flowchart LR A["API Token Processing"] --> B{"Rate Limit == 60?"} B -->|Yes| C["Log Warning with Token Suffix"] C --> D["Skip to Next Token"] B -->|No| E["Try get_user().login"] E --> F{"GithubException?"} F -->|Yes| G["Log Warning & Skip"] F -->|No| H["Add User to List"] G --> D H --> DFile Walkthrough
github_api.py
Add exception handling for GitHub API authenticationwebhook_server/libs/github_api.py
_to_tokento capture token valuetoken_suffixfor logging_api.get_user().logincall in try/except block to catchGithubException
processing
Summary by CodeRabbit
Bug Fixes
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.