fix: convert app_id to string before login_as_app_installation call#413
Merged
fix: convert app_id to string before login_as_app_installation call#413
Conversation
### What Wrapped `gh_app_id` with `str()` in the `login_as_app_installation` call in auth.py, and added a targeted test for integer app_id inputs. Existing tests were also tightened to assert on exact call arguments. ### Why When `gh_app_id` is passed as an integer, PyJWT raises a TypeError on the `iss` claim during JWT encoding because it expects a string. This surfaces at runtime when the environment variable is parsed as an int rather than str. ### Notes - Only `gh_app_id` is converted; `gh_app_installation_id` is left as-is since `login_as_app_installation` accepts it in its original form — reviewers should verify this is intentional - The existing tests previously used `assert_called_once()` without argument checks, so bugs like this were invisible; the tightened assertions now catch argument type mismatches Signed-off-by: jmeridth <jmeridth@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a GitHub App authentication runtime error by ensuring the App ID is passed as a string when authenticating via login_as_app_installation, and strengthens the unit tests to catch argument-type mismatches.
Changes:
- Cast
gh_app_idtostrinauth_to_github()before callinglogin_as_app_installation(...). - Tighten existing auth tests to assert exact
login_as_app_installation(...)call arguments and add a test for integerapp_id. - Add
.gitignoreentries for local Claude AI files and update the PR template checklist.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
auth.py |
Converts gh_app_id to string for the login_as_app_installation(...) call. |
test_auth.py |
Tightens call-argument assertions and adds coverage for integer gh_app_id. |
.gitignore |
Ignores .claude/*.local.* files. |
.github/pull_request_template.md |
Removes a checklist item from the PR template. |
Collaborator
|
@jmeridth Were we able to test this out? |
Collaborator
Author
Yes. Confirmed with the cleanowners fix I merged and deployed to my own org. This fix is necessitated all of our actions. |
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
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.
Pull Request
Proposed Changes
What
Wrapped
gh_app_idwithstr()in thelogin_as_app_installationcall in auth.py, and added a targeted test for integer app_id inputs. Existing tests were also tightened to assert on exact call arguments.Why
When
gh_app_idis passed as an integer, PyJWT raises a TypeError on theissclaim during JWT encoding because it expects a string. This surfaces at runtime when the environment variable is parsed as an int rather than str.Notes
gh_app_idis converted;gh_app_installation_idis left as-is sincelogin_as_app_installationaccepts it in its original form — reviewers should verify this is intentionalassert_called_once()without argument checks, so bugs like this were invisible; the tightened assertions now catch argument type mismatchesReadiness Checklist
Author/Contributor
make lintand fix any issues that you have introducedmake testand ensure you have test coverage for the lines you are introducing