NO-JIRA: Migrate from github3.py to PyGithub#71
Open
RadekManak wants to merge 1 commit into
Open
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@RadekManak: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RadekManak The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
openshift-ci
Bot
added
the
needs-rebase
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplace github3-py with PyGithub and migrate all GitHub interactions: dependency update, GitHub App installation token flow, repo/PR/content APIs updated to PyGithub methods, logging target adjusted, and tests/mocks adapted to PyGithub object shapes and behaviors. ChangesPyGithub migration
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant AppProv as GithubAppProvider
participant GHInt as GithubIntegration
participant PyGH as PyGithub Client
participant Repo as Repository
CLI->>AppProv: request authenticated client for repo
rect rgba(200,200,255,0.5)
AppProv->>GHInt: get_repo_installation(namespace, repo)
GHInt-->>AppProv: installation id
AppProv->>GHInt: get_access_token(installation id)
GHInt-->>AppProv: access token
AppProv->>PyGH: create Github client with token
end
AppProv-->>CLI: return Github client
CLI->>PyGH: get_repo("owner/name")
PyGH-->>CLI: Repo
CLI->>Repo: get_pulls(base=..., state=...)
Repo-->>CLI: list of PullRequests
CLI->>Repo: create_pull(head="owner:branch", base="branch", ...)
Repo-->>CLI: created PullRequest
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
RadekManak
force-pushed
the
claude/migrate-to-pygithub-011CUwBiQ43PocAz3kWfv4uY
branch
from
6ffce74 to
3638882
Compare
openshift-ci
Bot
removed
the
needs-rebase
Contributor
Author
|
/test all |
RadekManak
force-pushed
the
claude/migrate-to-pygithub-011CUwBiQ43PocAz3kWfv4uY
branch
3 times, most recently
from
1698e18 to
77d4ca3
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rebasebot/bot.py (1)
635-643:⚠️ Potential issue | 🟠 MajorGuard deleted PR head repos in the ART PR scan too.
PyGithub can surface
pull_request.head.repoasNonefor PRs whose source repository was deleted._is_pr_available()already guards this case at lines 691–693, but_cherrypick_art_pull_request()dereferences.nameand.html_urlunconditionally at lines 639–641, so a stale ART PR with a deleted head repository will crash the run withAttributeError.Suggested fix
for pull_request in dest_repo.get_pulls(state="open", base=dest.branch): if "consistent with ART" in pull_request.title and pull_request.user.login == "openshift-bot": logging.info(f"Found open ART image update pull requst: {pull_request.title}") remote = pull_request.head.repo + if remote is None: + logging.warning("Skipping ART PR with deleted head repository: %s", pull_request.html_url) + continue remote_name = remote.name if remote_name in gitwd.remotes: gitwd.remotes[remote_name].set_url(remote.html_url) else: gitwd.create_remote(remote_name, remote.html_url)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rebasebot/bot.py` around lines 635 - 643, The loop in _cherrypick_art_pull_request (scanning dest_repo.get_pulls) dereferences pull_request.head.repo unconditionally and will AttributeError if the PR's source repo was deleted; add a guard that checks if pull_request.head.repo is None (like _is_pr_available does) and skip/log the PR when head.repo is missing before using remote = pull_request.head.repo and accessing remote.name/remote.html_url so the gitwd remote create/set_url steps only run for non-deleted head repositories.
🧹 Nitpick comments (2)
rebasebot/github.py (1)
136-154: Make the token getters initialize themselves.
get_app_token()andget_cloner_token()now return the cached fields directly, so a freshGithubAppProvideryieldsNoneuntilgithub_app/github_cloner_apphas been accessed first.rebasebot/bot.pyhappens to do that today, but the public getters are now order-dependent even though they are annotated asstr.Possible fix
def get_app_token(self) -> str: """ Get app auth token :return: str """ if self.user_auth: return self.user_token + if self._app_token is None: + _ = self.github_app return self._app_token def get_cloner_token(self) -> str: """ Get cloner app auth token :return: str """ if self.user_auth: return self.user_token + if self._cloner_token is None: + _ = self.github_cloner_app return self._cloner_token🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rebasebot/github.py` around lines 136 - 154, The getters get_app_token and get_cloner_token should lazily initialize their cached tokens instead of returning None when a fresh GithubAppProvider hasn't accessed github_app/github_cloner_app yet; update get_app_token to check user_auth first (return user_token) and otherwise, if self._app_token is falsy, trigger the initializer by accessing or calling the provider's github_app property/method to populate self._app_token, then return it; do the analogous change in get_cloner_token using github_cloner_app and self._cloner_token so both getters always return a str after initialization.tests/test_bot.py (1)
195-230: Add a regression case for deleted head repositories.These tests were updated for
get_pulls(...), but they still don't exercise the newpr.head.repo is Nonebranch in_is_pr_available(). A dedicated case here would lock in the null-safety fix this PR is introducing.Suggested test shape
+ def test_is_pr_available_skips_deleted_head_repo(self, dest_repo, dest, rebase): + deleted_pr = MagicMock() + deleted_pr.head.repo = None + deleted_pr.html_url = "https://github.com/test-namespace/dest-repo/pull/1" + dest_repo.get_pulls.return_value = [deleted_pr] + + pr, pr_available = _is_pr_available(dest_repo, dest, rebase) + + assert pr is None + assert pr_available is False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_bot.py` around lines 195 - 230, Add a new unit test in tests/test_bot.py that exercises the branch where a pull request's head repo is deleted by creating a MagicMock gh_pr with gh_pr.head.repo set to None (and gh_pr.head.ref set to the rebase branch), have dest_repo.get_pulls return [gh_pr], call _is_pr_available(dest_repo, dest, rebase) and assert that it returns (None, False), and assert dest_repo.get_pulls was called with base="dest-branch", state="open" to lock in the null-safety behavior in _is_pr_available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@rebasebot/bot.py`:
- Around line 635-643: The loop in _cherrypick_art_pull_request (scanning
dest_repo.get_pulls) dereferences pull_request.head.repo unconditionally and
will AttributeError if the PR's source repo was deleted; add a guard that checks
if pull_request.head.repo is None (like _is_pr_available does) and skip/log the
PR when head.repo is missing before using remote = pull_request.head.repo and
accessing remote.name/remote.html_url so the gitwd remote create/set_url steps
only run for non-deleted head repositories.
---
Nitpick comments:
In `@rebasebot/github.py`:
- Around line 136-154: The getters get_app_token and get_cloner_token should
lazily initialize their cached tokens instead of returning None when a fresh
GithubAppProvider hasn't accessed github_app/github_cloner_app yet; update
get_app_token to check user_auth first (return user_token) and otherwise, if
self._app_token is falsy, trigger the initializer by accessing or calling the
provider's github_app property/method to populate self._app_token, then return
it; do the analogous change in get_cloner_token using github_cloner_app and
self._cloner_token so both getters always return a str after initialization.
In `@tests/test_bot.py`:
- Around line 195-230: Add a new unit test in tests/test_bot.py that exercises
the branch where a pull request's head repo is deleted by creating a MagicMock
gh_pr with gh_pr.head.repo set to None (and gh_pr.head.ref set to the rebase
branch), have dest_repo.get_pulls return [gh_pr], call
_is_pr_available(dest_repo, dest, rebase) and assert that it returns (None,
False), and assert dest_repo.get_pulls was called with base="dest-branch",
state="open" to lock in the null-safety behavior in _is_pr_available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 74091bab-e182-4576-9738-8233afb0b28b
📒 Files selected for processing (8)
pyproject.tomlrebasebot/bot.pyrebasebot/cli.pyrebasebot/github.pyrebasebot/lifecycle_hooks.pytests/conftest.pytests/test_bot.pytests/test_rebases.py
RadekManak
force-pushed
the
claude/migrate-to-pygithub-011CUwBiQ43PocAz3kWfv4uY
branch
from
77d4ca3 to
55019f2
Compare
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)
rebasebot/bot.py (1)
635-654:⚠️ Potential issue | 🟡 MinorMissing null-check for
pull_request.head.repoin ART cherry-pick.The code accesses
pull_request.head.repo.nameat line 639 without checking ifhead.repoisNone. The same null-safety pattern added in_is_pr_available(lines 691-693) should be applied here for consistency.If the ART bot's fork repository gets deleted while a PR is open, this would cause an
AttributeError.🛡️ Proposed fix
for pull_request in dest_repo.get_pulls(state="open", base=dest.branch): if "consistent with ART" in pull_request.title and pull_request.user.login == "openshift-bot": logging.info(f"Found open ART image update pull requst: {pull_request.title}") remote = pull_request.head.repo + if remote is None: + logging.warning(f"Skipping ART PR with deleted head repository: {pull_request.html_url}") + continue remote_name = remote.name🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rebasebot/bot.py` around lines 635 - 654, The loop handling ART PRs assumes pull_request.head.repo exists; add a null-check like in _is_pr_available to skip and log when pull_request.head.repo is None before accessing remote.name. Specifically, before assigning remote = pull_request.head.repo, test if pull_request.head.repo is None and continue (or log a warning) to avoid AttributeError; keep the rest of the flow that uses remote.name, gitwd.create_remote / gitwd.remotes[remote_name].set_url, gitwd.remotes[remote_name].fetch and the _safe_cherry_pick calls unchanged.
🧹 Nitpick comments (2)
tests/test_bot.py (1)
364-366: Duplicatesourceassignment (pre-existing issue).Line 364-365 has a duplicate
source = MagicMock(...)assignment. The same pattern appears at lines 401-402. While harmless (second assignment simply overwrites the first with identical values), this appears to be a copy-paste artifact.🧹 Remove duplicate lines
def test_jira_link(self): gitwd = MagicMock() gitwd.git.rev_parse.return_value = "abcdefg" pull_req = MagicMock() pull_req.title = "OCPCLOUD-2051: Merge " "https://github.com/kubernetes/cloud-provider-aws:master (b80e8ef) into master" - source = MagicMock(branch="my-feature", url="https://github.com/my/repo") source = MagicMock(branch="my-feature", url="https://github.com/my/repo") dest = MagicMock(branch="main")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_bot.py` around lines 364 - 366, Remove the duplicate redundant assignment of the test variable "source" (the identical line source = MagicMock(branch="my-feature", url="https://github.com/my/repo")) so only a single assignment remains; ensure the corresponding "dest = MagicMock(branch='main')" and other usages still refer to the one "source" instance; also remove the matching duplicate occurrence later in the file where the same identical "source" assignment was duplicated.rebasebot/github.py (1)
136-154: PotentialNonereturn from token getters when property not accessed.
get_app_token()andget_cloner_token()can returnNoneif the correspondinggithub_apporgithub_cloner_appcached property hasn't been accessed yet, since_app_tokenand_cloner_tokenare initialized toNone. The return type annotation indicatesstr, but this is only guaranteed after the cached properties are accessed.This works correctly in the current codebase because
_init_working_dirinbot.pyaccessesgithub_app_provider.get_app_token()aftergithub_app_provider.github_appis accessed inrun(). However, this implicit ordering dependency could cause subtle bugs if the code is refactored.💡 Consider documenting or enforcing access order
Option 1: Update return type to
str | Noneto reflect reality:- def get_app_token(self) -> str: + def get_app_token(self) -> str | None:Option 2: Access the cached property to ensure token is populated:
def get_app_token(self) -> str: if self.user_auth: return self.user_token + # Ensure github_app has been accessed to populate token + _ = self.github_app return self._app_token🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rebasebot/github.py` around lines 136 - 154, get_app_token() and get_cloner_token() may return None because _app_token/_cloner_token are initialized to None until the cached properties are accessed; ensure these getters always populate the token before returning by forcing the cached provider access when needed: in get_app_token() if not self.user_auth and self._app_token is None, call self.github_app to populate _app_token and then return it; similarly in get_cloner_token() if not self.user_auth and self._cloner_token is None, call self.github_cloner_app to populate _cloner_token and then return it so the annotated return type str is always satisfied (alternatively change annotations to Optional[str] if you prefer to allow None).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rebasebot/lifecycle_hooks.py`:
- Around line 237-241: _fetch_file_from_github currently assumes
Repository.get_contents(...) returns a single ContentFile but PyGithub may
return a list for directories; change the function to call
repo.get_contents(git_repo_path_to_script, ref=branch), store the result in a
local variable, check if isinstance(result, list) and if so raise a clear
ValueError (or TypeError) explaining a directory was provided and a file path is
required, otherwise return the single ContentFile; keep the function signature
returning ContentFile and update any callers or docstring if needed to document
that only file paths are accepted.
---
Outside diff comments:
In `@rebasebot/bot.py`:
- Around line 635-654: The loop handling ART PRs assumes pull_request.head.repo
exists; add a null-check like in _is_pr_available to skip and log when
pull_request.head.repo is None before accessing remote.name. Specifically,
before assigning remote = pull_request.head.repo, test if pull_request.head.repo
is None and continue (or log a warning) to avoid AttributeError; keep the rest
of the flow that uses remote.name, gitwd.create_remote /
gitwd.remotes[remote_name].set_url, gitwd.remotes[remote_name].fetch and the
_safe_cherry_pick calls unchanged.
---
Nitpick comments:
In `@rebasebot/github.py`:
- Around line 136-154: get_app_token() and get_cloner_token() may return None
because _app_token/_cloner_token are initialized to None until the cached
properties are accessed; ensure these getters always populate the token before
returning by forcing the cached provider access when needed: in get_app_token()
if not self.user_auth and self._app_token is None, call self.github_app to
populate _app_token and then return it; similarly in get_cloner_token() if not
self.user_auth and self._cloner_token is None, call self.github_cloner_app to
populate _cloner_token and then return it so the annotated return type str is
always satisfied (alternatively change annotations to Optional[str] if you
prefer to allow None).
In `@tests/test_bot.py`:
- Around line 364-366: Remove the duplicate redundant assignment of the test
variable "source" (the identical line source = MagicMock(branch="my-feature",
url="https://github.com/my/repo")) so only a single assignment remains; ensure
the corresponding "dest = MagicMock(branch='main')" and other usages still refer
to the one "source" instance; also remove the matching duplicate occurrence
later in the file where the same identical "source" assignment was duplicated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c188e249-9304-4ea1-925b-e74b3acca083
📒 Files selected for processing (8)
pyproject.tomlrebasebot/bot.pyrebasebot/cli.pyrebasebot/github.pyrebasebot/lifecycle_hooks.pytests/conftest.pytests/test_bot.pytests/test_rebases.py
✅ Files skipped from review due to trivial changes (3)
- pyproject.toml
- rebasebot/cli.py
- tests/conftest.py
RadekManak
force-pushed
the
claude/migrate-to-pygithub-011CUwBiQ43PocAz3kWfv4uY
branch
from
55019f2 to
9f3a475
Compare
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 (2)
rebasebot/github.py (2)
136-154:⚠️ Potential issue | 🟠 MajorToken getters can return
Nonebefore auth is initialized.In app mode these methods promise
str, but they just return_app_token/_cloner_token, which are only populated as a side effect of accessinggithub_app/github_cloner_app. A direct caller now getsNoneand will pass an invalid credential downstream. Either initialize lazily here or fail fast when the cache is still empty.Suggested fix
def get_app_token(self) -> str: """ Get app auth token :return: str """ if self.user_auth: return self.user_token - return self._app_token + if self._app_token is None: + _ = self.github_app + if self._app_token is None: + raise RuntimeError("App token is not initialized") + return self._app_token def get_cloner_token(self) -> str: """ Get cloner app auth token :return: str """ if self.user_auth: return self.user_token - return self._cloner_token + if self._cloner_token is None: + _ = self.github_cloner_app + if self._cloner_token is None: + raise RuntimeError("Cloner token is not initialized") + return self._cloner_token🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rebasebot/github.py` around lines 136 - 154, get_app_token and get_cloner_token can return None because _app_token/_cloner_token are populated only when github_app/github_cloner_app are accessed; update these getters to either lazily initialize the tokens by accessing self.github_app / self.github_cloner_app when the cached token is falsy, or raise a clear exception immediately if the token is still missing. Specifically, in get_app_token and get_cloner_token check if self.user_auth then return self.user_token; otherwise if self._app_token / self._cloner_token is falsy call the corresponding property (self.github_app or self.github_cloner_app) to force initialization and then return the token, and if it remains falsy raise a RuntimeError (or ValueError) describing that app/cloner token is unavailable.
107-124:⚠️ Potential issue | 🟠 MajorAdd validation for
user_tokenwhenuser_auth=Truein__init__.When
user_auth=True, thegithub_appandgithub_cloner_appproperties callAuth.Token(self.user_token)(line 227), which requires a non-None token. Currently,__init__allowsuser_auth=Truewithuser_token=None, causing the error to surface later during property access instead of failing fast at initialization.Suggested fix
self.user_auth = user_auth self.user_token = user_token + if self.user_auth and not self.user_token: + raise ValueError("user_token must be provided when user_auth=True") self._app_credentials = None self._cloner_app_credentials = None self._app_token = None self._cloner_token = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rebasebot/github.py` around lines 107 - 124, Add a guard in the __init__ of the GitHub client so that when user_auth is True but user_token is None it raises immediately (e.g., ValueError) instead of deferring to property access; specifically, validate the user_auth and user_token parameters in __init__ and raise a clear error if user_auth and not user_token, since github_app and github_cloner_app call Auth.Token(self.user_token).
🧹 Nitpick comments (1)
tests/test_bot.py (1)
195-230: Add coverage for deleted PR head repositories.The production path you just introduced in
rebasebot/bot.pyskips entries wherepr.head.repo is None, but this suite still doesn't exercise that branch. A regression there would not be caught by the current happy-path/empty-list tests.Suggested test shape
+ def test_is_pr_available_skips_deleted_head_repo(self, dest_repo, dest, rebase): + deleted_pr = MagicMock() + deleted_pr.head.repo = None + deleted_pr.html_url = "https://github.com/test-namespace/dest-repo/pull/1" + + matching_pr = MagicMock() + matching_pr.head.repo.full_name = "test-namespace/rebase-repo" + matching_pr.head.ref = rebase.branch + + dest_repo.get_pulls.return_value = [deleted_pr, matching_pr] + + pr, pr_available = _is_pr_available(dest_repo, dest, rebase) + + assert pr == matching_pr + assert pr_available is True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_bot.py` around lines 195 - 230, The tests do not cover the branch where a PR's head repo is deleted (pr.head.repo is None) which your _is_pr_available function explicitly skips; add a test that mocks dest_repo.get_pulls to return a MagicMock PR whose head.repo is None (and a differing state/open values as needed) and assert _is_pr_available returns (None, False) and that dest_repo.get_pulls was called with base="dest-branch", state="open"; reference the _is_pr_available function and the gh_pr.head.repo attribute to locate where to add this case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rebasebot/github.py`:
- Around line 156-188: The github_app and github_cloner_app cached_property
implementations currently fetch a one-hour installation token via
_github_login_app(self._app_credentials/_cloner_app_credentials), store it to
_app_token/_cloner_token and return a Github client that will go stale; replace
this pattern so the properties return a Github client built with PyGithub's
automatic installation-auth (e.g. use
Auth.AppAuth.get_installation_auth(installation_id) or
GithubIntegration.get_github_for_installation(installation_id)) instead of
exchanging and caching a raw token; update github_app and github_cloner_app to
call the integration-based factory (or build AppInstallationAuth) using the same
credential sources (_app_credentials and _cloner_app_credentials) and stop
persisting static tokens (_app_token/_cloner_token), while preserving the
user-auth branch that calls _get_github_user_logged_in_app().
---
Outside diff comments:
In `@rebasebot/github.py`:
- Around line 136-154: get_app_token and get_cloner_token can return None
because _app_token/_cloner_token are populated only when
github_app/github_cloner_app are accessed; update these getters to either lazily
initialize the tokens by accessing self.github_app / self.github_cloner_app when
the cached token is falsy, or raise a clear exception immediately if the token
is still missing. Specifically, in get_app_token and get_cloner_token check if
self.user_auth then return self.user_token; otherwise if self._app_token /
self._cloner_token is falsy call the corresponding property (self.github_app or
self.github_cloner_app) to force initialization and then return the token, and
if it remains falsy raise a RuntimeError (or ValueError) describing that
app/cloner token is unavailable.
- Around line 107-124: Add a guard in the __init__ of the GitHub client so that
when user_auth is True but user_token is None it raises immediately (e.g.,
ValueError) instead of deferring to property access; specifically, validate the
user_auth and user_token parameters in __init__ and raise a clear error if
user_auth and not user_token, since github_app and github_cloner_app call
Auth.Token(self.user_token).
---
Nitpick comments:
In `@tests/test_bot.py`:
- Around line 195-230: The tests do not cover the branch where a PR's head repo
is deleted (pr.head.repo is None) which your _is_pr_available function
explicitly skips; add a test that mocks dest_repo.get_pulls to return a
MagicMock PR whose head.repo is None (and a differing state/open values as
needed) and assert _is_pr_available returns (None, False) and that
dest_repo.get_pulls was called with base="dest-branch", state="open"; reference
the _is_pr_available function and the gh_pr.head.repo attribute to locate where
to add this case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ba9b5cf0-34bf-4e94-b82e-33d406cc495b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
pyproject.tomlrebasebot/bot.pyrebasebot/cli.pyrebasebot/github.pyrebasebot/lifecycle_hooks.pytests/conftest.pytests/test_bot.pytests/test_rebases.py
✅ Files skipped from review due to trivial changes (3)
- pyproject.toml
- rebasebot/cli.py
- rebasebot/bot.py
🚧 Files skipped from review as they are similar to previous changes (2)
- rebasebot/lifecycle_hooks.py
- tests/test_rebases.py
RadekManak
force-pushed
the
claude/migrate-to-pygithub-011CUwBiQ43PocAz3kWfv4uY
branch
from
9f3a475 to
d193bb5
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rebasebot/bot.py (1)
635-643:⚠️ Potential issue | 🟠 MajorGuard
pull_request.head.repobefore accessing it in ART PR cherry-picks.
_is_pr_available()already handles deleted head repositories with a guard at line 693, but_cherrypick_art_pull_request()dereferencespull_request.head.repounconditionally at line 638-639. A matching open ART PR with a deleted head repo will raiseAttributeErrorhere and crash the entire rebase workflow since this exception is not caught by the outer exception handler.Proposed fix
for pull_request in dest_repo.get_pulls(state="open", base=dest.branch): if "consistent with ART" in pull_request.title and pull_request.user.login == "openshift-bot": logging.info(f"Found open ART image update pull requst: {pull_request.title}") remote = pull_request.head.repo + if remote is None: + logging.warning("Skipping ART PR with deleted head repository: %s", pull_request.html_url) + continue remote_name = remote.name if remote_name in gitwd.remotes: gitwd.remotes[remote_name].set_url(remote.html_url)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rebasebot/bot.py` around lines 635 - 643, The loop in _cherrypick_art_pull_request unconditionally accesses pull_request.head.repo (variables: pull_request, .head, .repo, remote, remote_name, gitwd) which will raise AttributeError for deleted head repos; add a guard that checks if pull_request.head and pull_request.head.repo are truthy before using them, and if not, log a debug/info message and continue to the next pull_request; then only set remote = pull_request.head.repo and proceed to update or create the remote (gitwd.create_remote / gitwd.remotes[...] .set_url) when the repo is present.
♻️ Duplicate comments (1)
rebasebot/lifecycle_hooks.py (1)
236-238:⚠️ Potential issue | 🟡 MinorReject directory paths before treating the result as
ContentFile.
repo.get_contents(...)is polymorphic in PyGithub. If this hook path resolves to a directory, this helper returns alist, and_fetch_from_github_api()then blows up ondecoded_contentwith a generic wrapper instead of a clear validation error.Proposed fix
def _fetch_file_from_github(github, organization, name, branch, git_repo_path_to_script) -> ContentFile: repo = github.github_cloner_app.get_repo(f"{organization}/{name}") - return repo.get_contents(git_repo_path_to_script, ref=branch) + result = repo.get_contents(git_repo_path_to_script, ref=branch) + if isinstance(result, list): + raise ValueError(f"{git_repo_path_to_script} is a directory; expected a file path") + return resultIn PyGithub, what does `Repository.get_contents(path, ref=...)` return for a file path versus a directory path?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rebasebot/lifecycle_hooks.py` around lines 236 - 238, _repo.get_contents(path, ref=...) can return a list when the path is a directory, but the code in _fetch_file_from_github assumes a ContentFile and downstream _fetch_from_github_api accesses decoded_content; update _fetch_file_from_github to detect and reject directory results (e.g., if repo.get_contents(...) returns a list or not an instance of ContentFile) and raise a clear ValueError/TypeError indicating the hook path is a directory, so callers get a precise validation error instead of a generic failure when accessing decoded_content.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@rebasebot/bot.py`:
- Around line 635-643: The loop in _cherrypick_art_pull_request unconditionally
accesses pull_request.head.repo (variables: pull_request, .head, .repo, remote,
remote_name, gitwd) which will raise AttributeError for deleted head repos; add
a guard that checks if pull_request.head and pull_request.head.repo are truthy
before using them, and if not, log a debug/info message and continue to the next
pull_request; then only set remote = pull_request.head.repo and proceed to
update or create the remote (gitwd.create_remote / gitwd.remotes[...] .set_url)
when the repo is present.
---
Duplicate comments:
In `@rebasebot/lifecycle_hooks.py`:
- Around line 236-238: _repo.get_contents(path, ref=...) can return a list when
the path is a directory, but the code in _fetch_file_from_github assumes a
ContentFile and downstream _fetch_from_github_api accesses decoded_content;
update _fetch_file_from_github to detect and reject directory results (e.g., if
repo.get_contents(...) returns a list or not an instance of ContentFile) and
raise a clear ValueError/TypeError indicating the hook path is a directory, so
callers get a precise validation error instead of a generic failure when
accessing decoded_content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 76bd65ca-9b05-4404-a2c6-2c22bd1614a4
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
pyproject.tomlrebasebot/bot.pyrebasebot/cli.pyrebasebot/github.pyrebasebot/lifecycle_hooks.pytests/conftest.pytests/test_bot.pytests/test_github.pytests/test_rebases.py
✅ Files skipped from review due to trivial changes (2)
- rebasebot/cli.py
- tests/conftest.py
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- tests/test_bot.py
openshift-ci
Bot
added
the
needs-rebase
RadekManak
force-pushed
the
claude/migrate-to-pygithub-011CUwBiQ43PocAz3kWfv4uY
branch
from
d193bb5 to
32bd120
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rebasebot/github.py`:
- Line 25: The GITHUB_BRANCH_PATTERN constant in rebasebot/github.py is too
permissive (repo name may contain "/" and branch may be empty); update the regex
so the "name" group forbids "/" and ":" (e.g. use [^/:\s]+) and the "branch"
group requires at least one character (e.g. use .+ instead of .*), keeping the
"organization" group as-is; replace the current pattern value in
GITHUB_BRANCH_PATTERN accordingly to reject malformed/empty refs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: e933b667-ebcb-4407-b455-f714e810467c
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
pyproject.tomlrebasebot/bot.pyrebasebot/cli.pyrebasebot/github.pyrebasebot/lifecycle_hooks.pytests/conftest.pytests/test_bot.pytests/test_github.pytests/test_lifecycle_hooks.pytests/test_rebases.py
✅ Files skipped from review due to trivial changes (4)
- pyproject.toml
- tests/test_rebases.py
- rebasebot/cli.py
- tests/conftest.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_bot.py
- rebasebot/bot.py
| from github import Auth, Github, GithubIntegration, UnknownObjectException | ||
|
|
||
| logger = logging.getLogger() | ||
| GITHUB_BRANCH_PATTERN = re.compile(r"^(?P<organization>[^/]+)/(?P<name>[^:]+):(?P<branch>.*)$") |
There was a problem hiding this comment.
Tighten branch parser regex to reject malformed/empty refs.
Line 25 currently accepts repo names containing / and also allows empty branches (org/repo:), which can produce invalid GitHubBranch values downstream.
Suggested fix
-GITHUB_BRANCH_PATTERN = re.compile(r"^(?P<organization>[^/]+)/(?P<name>[^:]+):(?P<branch>.*)$")
+GITHUB_BRANCH_PATTERN = re.compile(
+ r"^(?P<organization>[^/:]+)/(?P<name>[^/:]+):(?P<branch>[^:]+)$"
+)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rebasebot/github.py` at line 25, The GITHUB_BRANCH_PATTERN constant in
rebasebot/github.py is too permissive (repo name may contain "/" and branch may
be empty); update the regex so the "name" group forbids "/" and ":" (e.g. use
[^/:\s]+) and the "branch" group requires at least one character (e.g. use .+
instead of .*), keeping the "organization" group as-is; replace the current
pattern value in GITHUB_BRANCH_PATTERN accordingly to reject malformed/empty
refs.
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
2 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.
Replace github3.py with PyGithub for GitHub API interactions. This library is better maintained.