Skip to content

chore(github): upgrade PyGithub to 2.x and migrate off deprecated auth classes - #3240

Merged
IsmaelMartinez merged 2 commits into
The-PR-Agent:mainfrom
utsab345:feature/upgrade-pygithub-3193
Sep 10, 2026
Merged

IsmaelMartinez merged 2 commits into
The-PR-Agent:mainfrom
utsab345:feature/upgrade-pygithub-3193

Conversation

@utsab345

@utsab345 utsab345 commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

Description

Implements #3193: moves the PyGithub pin from ==1.59.* to >=2.10,<3 and migrates the two call sites (plus the import) that use APIs PyGithub deprecated back in the 1.59 era.

  1. Migrated call sites. Per the issue scope, the change touches exactly the three locations already identified:
    • pr_agent/git_providers/github_provider.py:14 drops the AppAuthentication import (no longer referenced after the migration).
    • GithubIntegration(integration_id=..., private_key=..., base_url=...) now passes auth=Auth.AppAuth(app_id=..., private_key=...) on the app-JWT path, with base_url unchanged.
    • AppAuthentication(app_id=..., private_key=..., installation_id=...) now uses Auth.AppInstallationAuth(Auth.AppAuth(...), installation_id=...) on the installation-token path, so provider.auth._app_auth stays the wrapped AppAuth that test_github_app_auth.py relies on for JWT creation.
  2. Pin. PyGithub>=2.10,<3; the lockfile diff is limited to the PyGithub version/hashes.
  3. Framing. This is deprecation cleanup ahead of PyGithub 3.x, not a fix for anything currently broken. Both deprecated forms still work on 2.10, but they raise DeprecationWarning from the app-auth path.
  4. Auth migration pinned by tests. The GithubIntegration stubs in test_review_persistent_comment_authorship.py now capture and assert that the new auth kwarg is an Auth.AppAuth, so the migration cannot silently regress to the deprecated constructor.
  5. Pacing and retries are a choice, not a surprise. PyGithub 2.x turns on request pacing and automatic retries by default (0.25s between requests, 1s between writes, 10 retries), behaviour 1.59 did not have. The [github] settings now mirror the 1.59 behaviour (seconds_between_requests = 0, seconds_between_writes = 0, api_retries = 0) and _get_github_client passes them to the Github(...) constructor explicitly, so the upgrade stays behaviour neutral unless an operator opts in. The app-auth docstring in test_github_app_auth.py and the str() comment in the provider were refreshed to match 2.x behaviour: PyGithub#3272 landed in 2.7.0, so the cast is now a harmless guarantee rather than the load-bearing requirement it was on 1.59.

Why this is the right replacement shape

The Auth.AppAuth swap alone would be correct at the GithubIntegration site (the app JWT needed for GET /app), but the deployment path issues an installation-scoped token, so it needs Auth.AppInstallationAuth wrapping Auth.AppAuth. Auth.AppAuth alone would not work there, a correction to the issue's original suggestion.

Verification

  1. tests/unittest/test_github_app_auth.py, tests/unittest/test_review_persistent_comment_authorship.py and tests/unittest/test_github_provider_import.py pass on the 2.10.0 pin.
  2. Running those tests with -W error::DeprecationWarning:github produces no github-module deprecation warnings, confirming the old auth path is gone from our call sites.
  3. Full unit suite: 4452 passed, 1 skipped, 1 xfailed. The baseline on main is 4450; the two new tests cover the client pacing defaults (no pacing, no retries) and the configurable overrides flowing into the constructor. The app-related e2e path still requires the GitHub App credentials as noted in the issue.

closes #3193

…h classes

Moves the PyGithub pin from ==1.59.* to >=2.10,<3 and migrates the two call
sites (plus the import) that used APIs deprecated since 1.59:

- GithubIntegration(integration_id=..., private_key=...) -> the app-JWT path
  now passes auth=Auth.AppAuth(...), which also feeds GET /app for the bot slug.
- AppAuthentication(app_id=..., private_key=..., installation_id=...) -> the
  installation-token path now uses Auth.AppInstallationAuth wrapping an
  Auth.AppAuth, so provider.auth._app_auth stays the wrapped AppAuth that
  tests rely on for JWT creation.

Deprecation cleanup ahead of PyGithub 3.x rather than a fix for anything
currently broken; the GitHub App unit coverage passes with no github-module
DeprecationWarnings and the full unit suite is unchanged (4450 passed).

Refs: The-PR-Agent#3193

@IsmaelMartinez IsmaelMartinez left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new auth path holds up against real PyGithub 2.10.0, so this is close. One ask here, two inline.

Nothing asserts the new auth= kwarg. The GithubIntegration stubs in test_review_persistent_comment_authorship.py, at lines 371 and 393, are def integration(**kwargs), so they pass either way: I reverted the call site to the old integration_id= form and all twelve tests in that file stayed green. Capturing and asserting the kwarg would pin the migration.

Comment thread pr_agent/git_providers/github_provider.py
Comment thread pyproject.toml
Pin the PyGithub 2.x migration in the unit tests the review asked for and
make the 2.x pacing and retry defaults an explicit, configurable choice so
the upgrade stays behaviour-neutral against the 1.59 baseline.
@qodo-code-review

qodo-code-review Bot commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

Code Review by Qodo

Grey Divider

Sorry, something went wrong

We weren't able to complete the code review on our side. Please try again manually by commenting /agentic_review on this PR.

Grey Divider

Qodo Logo

@utsab345

Copy link
Copy Markdown
Contributor Author

Pushed 291c8f9 addressing the review:

  • The GithubIntegration stubs in test_review_persistent_comment_authorship.py now capture and assert the auth kwarg is an Auth.AppAuth, so the migration is pinned by tests (the two stubs were **kwargs, so either form passed before).
  • The docstring in test_github_app_auth.py is updated: PyGithub#3272 landed in 2.7.0, so the 2.10 pin already ships the native integer to string iss conversion, and the str() cast is now a harmless guarantee. The provider comment at github_provider.py was refreshed to match.
  • On pacing and retries: PyGithub 2.x defaults to 0.25s between requests, 1s between writes and GithubRetry(total=10), none of which 1.59 had. The client now applies explicit [github] settings that mirror the 1.59 behaviour (seconds_between_requests = 0, seconds_between_writes = 0, api_retries = 0), so the upgrade stays behaviour neutral unless an operator opts in. Two tests cover both the defaults and the overrides reaching the Github(...) constructor.

Full unit suite: 4452 passed, 1 skipped, 1 xfailed (4450 on main plus the two new tests).

@IsmaelMartinez IsmaelMartinez left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pinning the auth kwarg in the stubs rather than only changing the call site. Approving.

Reverting the call site to integration_id= now turns two of the twelve tests in that file red, where all twelve stayed green before, so the migration is held in place.

On pacing I re-measured against real PyGithub 2.10.0: stock defaults come out at 0.25s, 1.0s and GithubRetry(total=10), and the client this branch builds comes out at 0, 0 and None. Neutral against 1.59, with the opt-in documented in the section. Worth naming what the zero default gives up: retry=None declines 2.x's secondary rate limit backoff on writes, and pr-agent only retries _get_diff_files.

Suite 4458 against a 4456 baseline on today's main.

@IsmaelMartinez
IsmaelMartinez merged commit 847dcbb into The-PR-Agent:main Sep 10, 2026
5 checks passed
IsmaelMartinez added a commit to PeterDaveHello/pr-agent that referenced this pull request Sep 10, 2026
The generated page now carries the three [github] pacing keys added in
The-PR-Agent#3240 and the two comment corrections from this branch, and the key-count
guard moves from 292 to 295 to match.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
IsmaelMartinez added a commit to MostafaHassanzadeh/pr-agent that referenced this pull request Sep 11, 2026
PyGithub 2.x adds a "url" string to every GET dict, so get_languages() on GitHub
carried a value that sort_files_by_main_languages and get_main_pr_language could not
rank, and every /describe, /review and /improve on GitHub failed since The-PR-Agent#3240.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J7XRE8KVG5XdDd2PNwwy42
(cherry picked from commit 2edacb7fa04d440c47ca764b1c77e51fd22c08d7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Upgrade PyGithub from 1.59.* to 2.x and migrate off the deprecated auth classes

2 participants