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.
Collaborator
|
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. |
WalkthroughConverted add_api_users_to_auto_verified_and_merged_users from synchronous to asynchronous, moved its initialization out of the constructor into process(), and implemented concurrent token validation using an async check_token helper with asyncio.gather. Tests updated to await the new async method. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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:
|
||||||||||||
Collaborator
Author
|
/build-and-push-container |
Collaborator
|
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
Collaborator
Author
|
/build-and-push-container |
Collaborator
|
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.
Collaborator
|
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. |
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@webhook_server/libs/github_api.py`:
- Around line 619-648: The current check_token coroutine only catches
GithubException which lets other exceptions (e.g., ConnectionError, OSError)
escape and cause asyncio.gather to cancel siblings; update check_token to catch
Exception (replace both "except GithubException as ex" clauses with "except
Exception as ex") so any error is logged and the function returns None,
preserving the intended graceful-skip behavior when called via asyncio.gather
over apis_and_tokens; alternatively you can pass return_exceptions=True to
asyncio.gather and filter non-str(str) results, but the preferred minimal change
is broadening the exception handlers inside check_token while keeping existing
log messages and return None semantics.
- Around line 640-644: The log call inside the GithubException handler
redundantly injects the exception object into the f-string; update the except
block handling GithubException so the self.logger.exception(...) call only
contains the descriptive message (e.g., using self.log_prefix and token_suffix)
and does not format in {ex}, leaving the exception and traceback to be attached
automatically; keep the current return None behavior and target the except block
that catches GithubException and calls self.logger.exception with
self.log_prefix and token_suffix.
…ithubException asyncio.to_thread can raise ConnectionError, OSError, etc. in addition to GithubException. Since asyncio.gather cancels sibling tasks on the first unhandled exception, a network failure on one token would prevent all other tokens from being validated. Broadening to Exception ensures each token check is fully isolated.
Code Review by Qodo
1. Token suffix logged
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@webhook_server/libs/github_api.py`:
- Around line 640-644: The log message in the except block for get API user is
redundantly interpolating the exception into logger.exception; remove the
explicit "{ex}" from the formatted string so logger.exception(self.log_prefix
... token_suffix) is called with only the contextual message (it will
automatically include the exception and traceback), e.g., update the call to
logger.exception(...) in the except block that references self.log_prefix and
token_suffix and keep the subsequent return None unchanged.
Collaborator
Author
|
/build-and-push-container |
Collaborator
|
New container for ghcr.io/myk-org/github-webhook-server:pr-984 published |
Collaborator
Author
|
/verified |
Collaborator
|
/lgtm |
Collaborator
|
Successfully removed PR tag: ghcr.io/myk-org/github-webhook-server:pr-984. |
Collaborator
|
New container for ghcr.io/myk-org/github-webhook-server:latest published |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Performance Improvements
Reliability
Tests