feat: Add GitHub App authentication support #109
Conversation
🤖 Agent Implementation CompleteThis PR was implemented by the github-app-architect agent. What's Included
Usage Example// Authenticate as installation
$github = Github::forInstallation(98848199);
// Now all operations are attributed to the app
$github->repos()->get('the-shit/chat');
$github->pullRequests()->create(...);Tested WithThis implementation powers the shit-agents bot that created this very comment! Implementation by github-app-architect agent |
|
Caution Review failedThe pull request is closed. WalkthroughAdds native GitHub App support: configuration keys, authentication strategy with connector injection and installation-token refresh, DTOs and request classes for installations, an InstallationsResource, and public Github methods to create/apply app-scoped clients and installation auth. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Client as Github (Facade)
participant Conn as GithubConnector
participant Auth as GitHubAppAuthentication
participant Req as CreateAccessToken
participant API as GitHub API
Dev->>Client: withApp(appId, privateKey, installationId)
Client->>Auth: instantiate GitHubAppAuthentication(appId, privateKey, installationId)
Client->>Conn: new GithubConnector(authStrategy=Auth)
Conn->>Auth: setConnector(connector)
Dev->>Client: installations()->list()
Client->>Conn: send ListInstallations / CreateAccessToken via connector
Conn->>Auth: request Authorization header
Auth->>Auth: if token missing/expired -> refreshInstallationToken()
Auth->>Req: build CreateAccessToken(installationId)
Auth->>Conn: makeApiRequest(Req)
Conn->>API: POST /app/installations/{id}/access_tokens (JWT auth)
API-->>Conn: Installation token response
Conn-->>Auth: response
Auth->>Auth: update installationToken & expiry
Auth-->>Conn: return "Bearer {token}"
Conn->>API: perform requested API call with Bearer token
API-->>Conn: installations data
Conn-->>Client: DTOs / data
Client-->>Dev: installations list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (11)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/Github.php (1)
191-212: Remove unnecessary string cast on Line 206.The
GitHubAppAuthenticationconstructor accepts?string $installationId, and casting aninttostringis unnecessary here since the value will be auto-cast.Apply this diff:
$auth = new GitHubAppAuthentication( appId: $appId, privateKey: $privateKey, - installationId: (string) $installationId, + installationId: $installationId, );src/Auth/GitHubAppAuthentication.php (1)
19-19: Consider type-hinting the connector property.Line 19 declares
private ?object $connector = null, but it should be typed asGithubConnector(or an interface if available) for better IDE support and type safety.-private ?object $connector = null; +private ?GithubConnector $connector = null;You'll need to add the import:
+use JordanPartridge\GithubClient\Connectors\GithubConnector;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
config/github-client.php(1 hunks)src/Auth/GitHubAppAuthentication.php(2 hunks)src/Connectors/GithubConnector.php(4 hunks)src/Data/Installations/InstallationData.php(1 hunks)src/Data/Installations/InstallationTokenData.php(1 hunks)src/Github.php(3 hunks)src/Requests/Installations/CreateAccessToken.php(1 hunks)src/Requests/Installations/GetInstallation.php(1 hunks)src/Requests/Installations/ListInstallations.php(1 hunks)src/Resources/InstallationsResource.php(1 hunks)tests/Feature/GitHubAppAuthenticationTest.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/Requests/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Request classes should be organized in
src/Requests/and handle actual API calls via Saloon, organized in subdirectories matching their resource counterparts
Files:
src/Requests/Installations/ListInstallations.phpsrc/Requests/Installations/CreateAccessToken.phpsrc/Requests/Installations/GetInstallation.php
src/Data/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
All API responses should be mapped to strongly-typed DTOs using
spatie/laravel-datapackage insrc/Data/directory
Files:
src/Data/Installations/InstallationTokenData.phpsrc/Data/Installations/InstallationData.php
src/Resources/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
All API endpoints should be organized as resources extending
BaseResourceinsrc/Resources/directory, with each resource mapping to a GitHub API entity
Files:
src/Resources/InstallationsResource.php
tests/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Tests must use Pest PHP with Laravel testing helpers, mock Saloon responses using
MockClient, and validate both success and failure scenarios
Files:
tests/Feature/GitHubAppAuthenticationTest.php
src/Auth/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Make authentication optional for public repositories and support GitHub CLI token reuse via
gh auth tokenwith better error messages
Files:
src/Auth/GitHubAppAuthentication.php
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: jordanpartridge/github-client PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T05:02:33.323Z
Learning: Applies to src/Auth/**/*.php : Make authentication optional for public repositories and support GitHub CLI token reuse via `gh auth token` with better error messages
📚 Learning: 2025-12-10T05:02:33.323Z
Learnt from: CR
Repo: jordanpartridge/github-client PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T05:02:33.323Z
Learning: Applies to src/Auth/**/*.php : Make authentication optional for public repositories and support GitHub CLI token reuse via `gh auth token` with better error messages
Applied to files:
src/Github.phpsrc/Connectors/GithubConnector.phptests/Feature/GitHubAppAuthenticationTest.phpsrc/Auth/GitHubAppAuthentication.php
📚 Learning: 2025-12-10T05:02:33.323Z
Learnt from: CR
Repo: jordanpartridge/github-client PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T05:02:33.323Z
Learning: Provide custom exceptions with context-specific error information: `ApiException` for API failures, `AuthenticationException` for auth issues, `RateLimitException` for rate limits, `ValidationException` for input validation, and `NetworkException` for connectivity issues
Applied to files:
src/Github.php
📚 Learning: 2024-11-02T00:18:45.602Z
Learnt from: jordanpartridge
Repo: jordanpartridge/github-client PR: 5
File: src/Resources/RepoResource.php:85-0
Timestamp: 2024-11-02T00:18:45.602Z
Learning: Input validation for parameters like `$per_page` and `$page` is handled in the request classes (e.g., `src/Requests/Repos/Repos.php`), so it's not necessary to duplicate this validation in the resource methods such as `RepoResource::all()`.
Applied to files:
src/Requests/Installations/ListInstallations.php
📚 Learning: 2024-11-01T05:08:11.744Z
Learnt from: jordanpartridge
Repo: jordanpartridge/github-client PR: 1
File: src/GithubConnector.php:12-16
Timestamp: 2024-11-01T05:08:11.744Z
Learning: When using token authentication in `GithubConnector` (PHP), and not utilizing OAuth2 flows, remove the `AuthorizationCodeGrant` trait to avoid unnecessary methods like `defaultOauthConfig()`.
Applied to files:
src/Connectors/GithubConnector.phptests/Feature/GitHubAppAuthenticationTest.phpsrc/Auth/GitHubAppAuthentication.php
📚 Learning: 2024-10-31T15:53:12.233Z
Learnt from: jordanpartridge
Repo: jordanpartridge/github-client PR: 1
File: config/github-client.php:4-4
Timestamp: 2024-10-31T15:53:12.233Z
Learning: The `TokenAuthenticator` class is provided by the Saloon package and does not need to be implemented within this codebase.
Applied to files:
src/Connectors/GithubConnector.php
📚 Learning: 2025-12-10T05:02:33.322Z
Learnt from: CR
Repo: jordanpartridge/github-client PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T05:02:33.322Z
Learning: Applies to src/Resources/**/*.php : All API endpoints should be organized as resources extending `BaseResource` in `src/Resources/` directory, with each resource mapping to a GitHub API entity
Applied to files:
src/Resources/InstallationsResource.php
🧬 Code graph analysis (5)
src/Github.php (3)
src/Auth/GitHubAppAuthentication.php (1)
GitHubAppAuthentication(13-216)src/Resources/InstallationsResource.php (1)
InstallationsResource(20-122)src/Connectors/GithubConnector.php (1)
GithubConnector(29-315)
src/Requests/Installations/ListInstallations.php (1)
src/Data/Installations/InstallationData.php (3)
InstallationData(7-50)__construct(9-19)fromArray(21-34)
src/Requests/Installations/CreateAccessToken.php (2)
src/Data/Installations/InstallationTokenData.php (3)
InstallationTokenData(7-45)__construct(9-14)fromArray(16-24)src/Requests/Installations/GetInstallation.php (3)
__construct(14-16)createDtoFromResponse(18-21)resolveEndpoint(23-26)
src/Connectors/GithubConnector.php (2)
src/Auth/GitHubAppAuthentication.php (7)
GitHubAppAuthentication(13-216)__construct(21-25)getType(57-60)setConnector(168-171)needsRefresh(62-69)refresh(71-76)getAuthorizationHeader(27-35)src/Auth/TokenAuthentication.php (5)
__construct(12-14)getType(37-40)needsRefresh(42-46)refresh(48-51)getAuthorizationHeader(16-19)
src/Requests/Installations/GetInstallation.php (1)
src/Data/Installations/InstallationData.php (3)
InstallationData(7-50)__construct(9-19)fromArray(21-34)
🪛 GitHub Actions: run-tests
src/Auth/GitHubAppAuthentication.php
[error] 124-124: Pest test run failed due to PHP memory exhaustion: Allowed memory size of 536870912 bytes exhausted (tried to allocate 12288 bytes) in /home/runner/work/github-client/github-client/src/Auth/GitHubAppAuthentication.php on line 124
🪛 GitHub Check: P8.2 - L11.* - prefer-stable
src/Auth/GitHubAppAuthentication.php
[failure] 124-124:
Allowed memory size of 536870912 bytes exhausted (tried to allocate 12288 bytes)
🪛 GitHub Check: phpstan (8.2)
src/Data/Installations/InstallationTokenData.php
[failure] 43-43:
Method JordanPartridge\GithubClient\Data\Installations\InstallationTokenData::expiresIn() should return int but returns float.
🪛 GitHub Check: phpstan (8.3)
src/Data/Installations/InstallationTokenData.php
[failure] 43-43:
Method JordanPartridge\GithubClient\Data\Installations\InstallationTokenData::expiresIn() should return int but returns float.
🪛 GitHub Check: phpstan (8.4)
src/Data/Installations/InstallationTokenData.php
[failure] 43-43:
Method JordanPartridge\GithubClient\Data\Installations\InstallationTokenData::expiresIn() should return int but returns float.
🪛 Gitleaks (8.30.0)
tests/Feature/GitHubAppAuthenticationTest.php
[high] 16-44: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 144-254: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🔇 Additional comments (19)
config/github-client.php (1)
68-89: LGTM! Well-documented GitHub App configuration.The new configuration section is well-structured with clear documentation and follows Laravel conventions for environment variable usage.
src/Requests/Installations/ListInstallations.php (2)
19-26: LGTM! Proper pagination validation.The per_page validation (1-100) aligns with GitHub API constraints and throws a clear exception for invalid inputs.
36-42: LGTM! Clean DTO transformation.The response transformation using
InstallationData::fromArray()provides strong typing and follows the established pattern in this codebase.src/Requests/Installations/GetInstallation.php (1)
10-27: LGTM! Clean and focused request class.The implementation is straightforward and follows the established patterns in the codebase for request classes and DTO transformation.
src/Github.php (1)
77-80: LGTM! Clean resource accessor.The
installations()method follows the established pattern for resource accessors in this class.src/Requests/Installations/CreateAccessToken.php (1)
12-46: LGTM! Well-structured token creation request.The implementation correctly:
- Filters optional parameters (repositories, permissions) from the request body
- Transforms the response into a strongly-typed
InstallationTokenDataDTO- Constructs the proper endpoint path
src/Data/Installations/InstallationData.php (1)
7-50: LGTM! Well-structured DTO with proper transformation methods.The implementation provides:
- Clear property definitions with appropriate types
- Safe deserialization via
fromArray()with null coalescing- Clean serialization via
toArray()that filters null values- Proper Carbon date handling
src/Connectors/GithubConnector.php (4)
5-6: LGTM!The new imports are necessary for the authentication strategy support and are properly namespaced.
35-61: LGTM! Well-designed authentication strategy integration.The constructor elegantly handles three authentication modes while maintaining backward compatibility. The connector injection into
GitHubAppAuthentication(lines 50-52) creates a controlled circular dependency that enables the strategy to refresh installation tokens—this pattern is necessary and correctly implemented.
86-110: LGTM! Clean authentication flow with proper token refresh.The method correctly prioritizes auth strategy when available, refreshes tokens as needed, and maintains backward compatibility with the existing token-based authentication fallback.
117-117: LGTM! Correct authentication state detection.The method now properly accounts for both authentication strategy and direct token authentication modes.
tests/Feature/GitHubAppAuthenticationTest.php (5)
13-46: Static analysis warning is a false positive.The private key at lines 16-44 is a test fixture, not a real credential. Using test keys in test files is standard practice and necessary for validating JWT generation and authentication flows.
48-138: LGTM! Comprehensive authentication test coverage.The tests validate credentials, error handling, JWT generation, installation tokens, refresh flows, and connector integration. Good use of
MockClientfollowing the Pest PHP guidelines.
140-247: LGTM! Thorough resource testing with proper mocking.Tests validate all primary operations (list, get, createAccessToken) with correct DTO mapping and pagination handling. The Link header mocking for pagination tests follows best practices.
249-288: LGTM! Helper method tests cover key scenarios.Tests validate both configuration-based and explicit credential instantiation patterns, plus proper error handling for missing configuration.
290-333: LGTM! Complete token data behavior validation.Tests properly validate expiry checks, time calculations, and serialization. The range check for
expiresIn()(1700-1900 seconds) appropriately accounts for test execution time.src/Resources/InstallationsResource.php (3)
20-39: LGTM! Clean resource implementation.The readonly class properly extends
BaseResourcefollowing coding guidelines, and thelist()method cleanly delegates to the request class with appropriate pagination parameters.
52-85: LGTM! Well-documented API operations.Both methods follow the established delegation pattern and include comprehensive documentation with GitHub API references. The optional parameters in
createAccessToken()provide good flexibility for token scoping.
96-121: LGTM! Robust pagination with safety limits.The automatic pagination correctly parses Link headers and accumulates results. The 100-page hard limit (lines 103-105) is a good safety measure, preventing runaway API calls while supporting up to 10,000 installations.
| public function expiresIn(): int | ||
| { | ||
| return Carbon::now()->diffInSeconds($this->expires_at, false); | ||
| } |
There was a problem hiding this comment.
Fix return type mismatch in expiresIn().
PHPStan reports that Carbon::diffInSeconds($this->expires_at, false) returns float|int, but the method signature declares int. This causes failures across PHP 8.2, 8.3, and 8.4.
Apply this diff to cast the result to int:
public function expiresIn(): int
{
- return Carbon::now()->diffInSeconds($this->expires_at, false);
+ return (int) Carbon::now()->diffInSeconds($this->expires_at, false);
}🧰 Tools
🪛 GitHub Check: phpstan (8.2)
[failure] 43-43:
Method JordanPartridge\GithubClient\Data\Installations\InstallationTokenData::expiresIn() should return int but returns float.
🪛 GitHub Check: phpstan (8.3)
[failure] 43-43:
Method JordanPartridge\GithubClient\Data\Installations\InstallationTokenData::expiresIn() should return int but returns float.
🪛 GitHub Check: phpstan (8.4)
[failure] 43-43:
Method JordanPartridge\GithubClient\Data\Installations\InstallationTokenData::expiresIn() should return int but returns float.
🤖 Prompt for AI Agents
In src/Data/Installations/InstallationTokenData.php around lines 41 to 44, the
expiresIn() method returns Carbon::now()->diffInSeconds($this->expires_at,
false) which PHPStan reports as float|int while the method signature declares
int; cast the result to int before returning (e.g. wrap the diffInSeconds call
with (int) or use intval()) so the return type matches the declared int and
PHPStan errors are resolved.
| private static function resolvePrivateKey(): ?string | ||
| { | ||
| // Try direct key first | ||
| $key = config('github-client.github_app.private_key'); | ||
| if ($key) { | ||
| return $key; | ||
| } | ||
|
|
||
| // Try file path | ||
| $path = config('github-client.github_app.private_key_path'); | ||
| if ($path && file_exists($path)) { | ||
| return file_get_contents($path); | ||
| } | ||
|
|
||
| // Try base_path for relative paths | ||
| if ($path && file_exists(base_path($path))) { | ||
| return file_get_contents(base_path($path)); | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
Add error handling for file operations.
Lines 252 and 257 use file_get_contents() without checking if the operation succeeds. If the file read fails, false will be returned and used as the private key, leading to cryptic authentication errors downstream.
Apply this diff to add proper error handling:
// Try file path
$path = config('github-client.github_app.private_key_path');
if ($path && file_exists($path)) {
- return file_get_contents($path);
+ $key = file_get_contents($path);
+ if ($key === false) {
+ throw new \RuntimeException("Failed to read private key from path: {$path}");
+ }
+ return $key;
}
// Try base_path for relative paths
if ($path && file_exists(base_path($path))) {
- return file_get_contents(base_path($path));
+ $key = file_get_contents(base_path($path));
+ if ($key === false) {
+ throw new \RuntimeException("Failed to read private key from path: " . base_path($path));
+ }
+ return $key;
}
return null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static function resolvePrivateKey(): ?string | |
| { | |
| // Try direct key first | |
| $key = config('github-client.github_app.private_key'); | |
| if ($key) { | |
| return $key; | |
| } | |
| // Try file path | |
| $path = config('github-client.github_app.private_key_path'); | |
| if ($path && file_exists($path)) { | |
| return file_get_contents($path); | |
| } | |
| // Try base_path for relative paths | |
| if ($path && file_exists(base_path($path))) { | |
| return file_get_contents(base_path($path)); | |
| } | |
| return null; | |
| } | |
| private static function resolvePrivateKey(): ?string | |
| { | |
| // Try direct key first | |
| $key = config('github-client.github_app.private_key'); | |
| if ($key) { | |
| return $key; | |
| } | |
| // Try file path | |
| $path = config('github-client.github_app.private_key_path'); | |
| if ($path && file_exists($path)) { | |
| $key = file_get_contents($path); | |
| if ($key === false) { | |
| throw new \RuntimeException("Failed to read private key from path: {$path}"); | |
| } | |
| return $key; | |
| } | |
| // Try base_path for relative paths | |
| if ($path && file_exists(base_path($path))) { | |
| $key = file_get_contents(base_path($path)); | |
| if ($key === false) { | |
| throw new \RuntimeException("Failed to read private key from path: " . base_path($path)); | |
| } | |
| return $key; | |
| } | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In src/Github.php around lines 241 to 261, the code reads private key files with
file_get_contents() but doesn't check for failure; if file_get_contents()
returns false the false value will be treated as the key. Update both
file_get_contents() calls to capture the result, check if the return === false,
and on failure either throw a clear exception (e.g. RuntimeException with path
and error) or log an error and return null; if successful, return the string
(optionally trim it). Ensure the file_exists checks remain and that no false
value is returned as the private key.
## New Features - `Github::forInstallation($id)` - Create client for installation - `Github::withApp($appId, $key, $installationId)` - Custom app auth - `InstallationsResource` - List installations, get tokens - Installation token auto-refresh with caching ## Files Added - src/Resources/InstallationsResource.php - src/Data/Installations/InstallationData.php - src/Data/Installations/InstallationTokenData.php - src/Requests/Installations/ListInstallations.php - src/Requests/Installations/GetInstallation.php - src/Requests/Installations/CreateAccessToken.php - tests/Feature/GitHubAppAuthenticationTest.php ## Config New `github_app` section in config/github-client.php: - app_id, private_key, private_key_path, installation_id Closes #106 Co-Authored-By: github-app-architect <github-app-architect@the-shit.bot>
jordanpartridge
force-pushed
the
feat/github-app-auth
branch
from
9f9a00a to
f66d204
Compare
Summary
Native GitHub App authentication for bot-attributed commits and higher rate limits.
New Features
Static Factory Methods
InstallationsResource
Config
Files Added (869 lines)
src/Resources/InstallationsResource.phpsrc/Data/Installations/InstallationData.phpsrc/Data/Installations/InstallationTokenData.phpsrc/Requests/Installations/*.phptests/Feature/GitHubAppAuthenticationTest.phpWhy GitHub Apps?
app-name[bot])Closes #106
Co-Authored-By: github-app-architect github-app-architect@the-shit.bot
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.