feat: add Checks API, file contents, and Laravel 13 support#119
Open
jordanpartridge wants to merge 3 commits into
Open
feat: add Checks API, file contents, and Laravel 13 support#119jordanpartridge wants to merge 3 commits into
jordanpartridge wants to merge 3 commits into
Conversation
- Add ChecksResource with forRef(), combinedStatus(), allPassing()
- Add GetCheckRunsForRef and GetCombinedStatus requests
- Add FileResource::contents() and getContent() for reading files at a ref
- Add GetContents request for /repos/{owner}/{repo}/contents/{path}
- Bump illuminate/contracts to ^11.0||^12.0||^13.0
- 223 tests passing
📝 WalkthroughWalkthroughThis PR extends Laravel 13 compatibility in dependencies, adds a new Checks feature with related request and resource classes, enhances file operations with new methods, performs widespread import cleanups to use imported class names instead of fully-qualified references, adds PHPStan baseline configuration, and updates test assertions accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant Github as Github Class
participant ChecksResource as ChecksResource
participant Connector as Connector
participant GithubAPI as GitHub API
Client->>Github: checks()
Github->>ChecksResource: new ChecksResource($this)
alt forRef() flow
Client->>ChecksResource: forRef(owner, repo, ref)
ChecksResource->>Connector: send(GetCheckRunsForRef request)
Connector->>GithubAPI: GET /repos/{owner}/{repo}/commits/{ref}/check-runs
GithubAPI-->>Connector: check runs array
Connector-->>ChecksResource: decoded JSON array
ChecksResource-->>Client: array
end
alt allPassing() flow
Client->>ChecksResource: allPassing(owner, repo, ref)
ChecksResource->>ChecksResource: forRef(owner, repo, ref)
ChecksResource->>ChecksResource: iterate runs, validate completion & conclusion
ChecksResource-->>Client: boolean (all passed?)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
- Restore phpstan-baseline.neon deleted in #111 cleanup (CI depends on it) - Baseline 4 pre-existing GithubConnector errors - Fix FileResource::getContent() null coalescing on always-present keys
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
tests/AuthenticationTest.php (1)
172-172: Inconsistent class reference style.Line 172 uses a fully-qualified class name without importing
Githubat the top. For consistency with the rest of the file (which importsRepoDataand usesRepoData::class), either add an import forJordanPartridge\GithubClient\Githubor use the leading backslash\JordanPartridge\GithubClient\Github.♻️ Proposed fix: import Github class
use JordanPartridge\GithubClient\Facades\Github; +use JordanPartridge\GithubClient\Github as GithubClient; use Saloon\Http\Faking\MockClient; use Saloon\Http\Faking\MockResponse; use JordanPartridge\GithubClient\Data\Repos\RepoData;Then update line 172:
- $github = new JordanPartridge\GithubClient\Github($connector); + $github = new GithubClient($connector);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AuthenticationTest.php` at line 172, The test instantiates the Github client with a fully-qualified name (new JordanPartridge\GithubClient\Github($connector)) which is inconsistent with the file's import style; fix it by either adding a top-level import for JordanPartridge\GithubClient\Github (use JordanPartridge\GithubClient\Github;) and then instantiate via new Github($connector) or by normalizing to a leading-backslash fully-qualified reference (\JordanPartridge\GithubClient\Github) to match the rest of the file's style; update the instantiation at the line creating the $github variable accordingly.src/Resources/FileResource.php (1)
24-36: PHPDoc return type may be incomplete for directory paths.The
@returnannotation documents a single file's shape, but GitHub's Contents API returns an array of objects when$pathpoints to a directory. Consider documenting this or adding a note that this method is intended for files only.📝 Suggested documentation update
/** * Get the contents of a file at a specific ref (branch, tag, or SHA). + * + * Note: If the path is a directory, GitHub returns an array of entries instead. * * `@return` array{name: string, path: string, sha: string, size: int, content: string, encoding: string} */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Resources/FileResource.php` around lines 24 - 36, The PHPDoc for FileResource::contents currently documents only a single-file response shape but the GitHub Contents API can return an array of items when $path is a directory; update the docblock on the contents method to reflect both possibilities (e.g. union type for single file object and array of objects) or clearly state that this method is file-only, and mention the directory case in the description; reference the contents method and the GetContents call so the return type matches the response shape from new GetContents($owner, $repo, $path, $ref).tests/CommitResourceTest.php (1)
19-19: Minor inconsistency with import style.The file uses a fully-qualified class name without importing it. For consistency with the PR's pattern of using imports, consider importing the
Githubclass at the top.♻️ Proposed fix
Add import at top of file:
use JordanPartridge\GithubClient\Facades\Github; +use JordanPartridge\GithubClient\Github as GithubClient; use JordanPartridge\GithubClient\Resources\CommitResource;Then update line 19:
- $this->resource = new CommitResource(app(JordanPartridge\GithubClient\Github::class)); + $this->resource = new CommitResource(app(GithubClient::class));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/CommitResourceTest.php` at line 19, The test instantiates CommitResource with a fully-qualified class name; add a top-level import for JordanPartridge\GithubClient\Github (e.g., a use statement) and update the instantiation in the CommitResource constructor to reference Github::class instead of the FQCN to match the project's import style and keep consistency with other tests.src/Resources/ChecksResource.php (1)
15-22: Consider exposing pagination parameters to enable complete result retrieval.The
GetCheckRunsForRefrequest supportsperPageandpageparameters, but this method doesn't expose them. GitHub's API returns paginated results (default ~30 items), which limits downstream consumers who need to fetch all check runs.Consider adding optional pagination parameters or providing an iterator/collection method for complete retrieval:
♻️ Suggested change to support pagination
/** * Get check runs for a commit reference (SHA, branch, or tag). * * `@return` array{total_count: int, check_runs: array<int, array<string, mixed>>} */ - public function forRef(string $owner, string $repo, string $ref): array + public function forRef(string $owner, string $repo, string $ref, ?int $perPage = null, ?int $page = null): array { $response = $this->github()->connector()->send( - new GetCheckRunsForRef($owner, $repo, $ref), + new GetCheckRunsForRef($owner, $repo, $ref, $perPage, $page), ); return $response->json(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Resources/ChecksResource.php` around lines 15 - 22, The forRef method in ChecksResource currently calls GetCheckRunsForRef without pagination, returning only the default page; update forRef(string $owner, string $repo, string $ref, ?int $perPage = null, ?int $page = null): array to accept optional pagination parameters and forward them into the GetCheckRunsForRef request (or its constructor/setters) so callers can request specific pages, and additionally consider adding a helper allForRef or iterator on ChecksResource that repeatedly calls GetCheckRunsForRef until no more results to fetch all pages; reference the forRef method and GetCheckRunsForRef when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composer.json`:
- Line 31: Update the composer dependency constraints to properly signal Laravel
13 support: in composer.json amend the "orchestra/testbench" constraint to
include "^13.0" (so it becomes ^8.0||^9.0||^10.0||^11.0||^12.0||^13.0) and
tighten "spatie/laravel-package-tools" to " ^1.93|^2.0" (effectively ^1.93.0 to
ensure versions that support Laravel 13 are required).
In `@phpstan-baseline.neon`:
- Around line 1-21: Remove the three suppressed PHPStan errors from the baseline
and fix the root issues in the GithubConnector: ensure you reference/instantiate
the correct exception class
(JordanPartridge\\GithubClient\\Exceptions\\ResourceNotFoundException) or import
the actual existing exception instead of a non-existent class; update the
NetworkException instantiation to call its actual constructor signature (pass
the required $message string and any valid $code/$previous params, removing the
unknown $reason parameter); and fix the redundant strict comparison (replace the
always-false === check with a proper !== '' or adjust the variable type/logic).
After changes, re-run PHPStan and only keep genuine false-positives in the
baseline.
- Around line 13-16: The baseline suppresses PHPStan's warning about the
redundant check for `$this->token === ''` in the condition `if (! $this->token
|| $this->token === '')`; locate the usage of `$this->token` in the
GithubConnector class (e.g., the method containing that `if` check) and either
remove the redundant `=== ''` branch and delete the corresponding baseline entry
(lines referencing src/Connectors/GithubConnector.php and the Strict comparison
message), or if you intentionally kept the explicit empty-string check for
clarity, add an inline comment next to `if (! $this->token || $this->token ===
'')` explaining why both checks are present and keep the baseline suppression;
choose one of these two fixes and apply it consistently.
- Around line 8-11: The call to NetworkException in GithubConnector (the
instantiation in src/Connectors/GithubConnector.php) uses the wrong named
parameter "reason" and omits the required "message" parameter; update the
constructor call to use message: "Server error ({$status}): {$message}" (or
equivalent string) instead of reason:, keep operation: 'GitHub API request' and
previous: $senderException so the parameters match
NetworkException::__construct(string $operation, string $message, int $code = 0,
?\Throwable $previous = null).
- Around line 3-6: The baseline hides a fatal runtime error caused by
instantiating a non-existent ResourceNotFoundException in the GithubConnector
code; fix it by either adding the missing exception class (e.g., create class
ResourceNotFoundException extends \Exception in the appropriate namespace) or
change the instantiation to use an existing exception type that conveys "not
found" semantics (update the throw/new site in GithubConnector and its
use/import statements to reference the correct exception class, e.g., an
existing NotFoundException or GithubClient\Exceptions\SomeExistingException).
Ensure the chosen exception lives in the namespace imported by GithubConnector
(adjust use statements) and remove the phpstan baseline suppression for that
message.
In `@src/Github.php`:
- Around line 61-64: Add a matching `@method` annotation to the Github facade so
IDEs and static analyzers recognize the new checks() method: in
src/Facades/Github.php add the annotation "@method static
\JordanPartridge\GithubClient\Resources\ChecksResource checks()" alongside the
existing `@method` declarations for other resource methods, ensuring the facade
correctly documents the new checks() method defined in the Github class.
In `@src/Resources/ChecksResource.php`:
- Around line 43-62: allPassing() only inspects the first page returned by
forRef(), causing false positives when results are paginated; update the logic
to detect and handle pagination by either (a) using the response's total_count
vs count(check_runs) to detect truncation and throw an error, or (b) iterating
pages and aggregating all check runs before evaluating. Concretely, modify
either forRef() to fetch all pages (looping with page/per_page until collected)
or change allPassing() to call forRef() repeatedly (page++/per_page) and merge
all check_runs, then run the existing status/conclusion checks on the full
aggregated list; ensure you reference forRef() and allPassing() when
implementing the pagination loop or truncation check.
In `@src/Resources/FileResource.php`:
- Around line 41-50: The getContent method can fail when contents(...) returns a
directory array, is missing 'encoding'/'content' keys, or when base64_decode
returns false; update getContent to validate the response from
contents($owner,$repo,$path,$ref): if the return is an array of objects
(directory) or doesn't contain string 'content'/'encoding' keys, throw or return
a clear error/empty string, and when 'encoding' === 'base64' call base64_decode
and check for false—if decoding fails, throw a descriptive exception (or
fallback to an empty string) so the method always satisfies its string return
type; reference the getContent method and the contents(...) call to locate and
implement these defensive checks and error handling.
---
Nitpick comments:
In `@src/Resources/ChecksResource.php`:
- Around line 15-22: The forRef method in ChecksResource currently calls
GetCheckRunsForRef without pagination, returning only the default page; update
forRef(string $owner, string $repo, string $ref, ?int $perPage = null, ?int
$page = null): array to accept optional pagination parameters and forward them
into the GetCheckRunsForRef request (or its constructor/setters) so callers can
request specific pages, and additionally consider adding a helper allForRef or
iterator on ChecksResource that repeatedly calls GetCheckRunsForRef until no
more results to fetch all pages; reference the forRef method and
GetCheckRunsForRef when making the changes.
In `@src/Resources/FileResource.php`:
- Around line 24-36: The PHPDoc for FileResource::contents currently documents
only a single-file response shape but the GitHub Contents API can return an
array of items when $path is a directory; update the docblock on the contents
method to reflect both possibilities (e.g. union type for single file object and
array of objects) or clearly state that this method is file-only, and mention
the directory case in the description; reference the contents method and the
GetContents call so the return type matches the response shape from new
GetContents($owner, $repo, $path, $ref).
In `@tests/AuthenticationTest.php`:
- Line 172: The test instantiates the Github client with a fully-qualified name
(new JordanPartridge\GithubClient\Github($connector)) which is inconsistent with
the file's import style; fix it by either adding a top-level import for
JordanPartridge\GithubClient\Github (use JordanPartridge\GithubClient\Github;)
and then instantiate via new Github($connector) or by normalizing to a
leading-backslash fully-qualified reference
(\JordanPartridge\GithubClient\Github) to match the rest of the file's style;
update the instantiation at the line creating the $github variable accordingly.
In `@tests/CommitResourceTest.php`:
- Line 19: The test instantiates CommitResource with a fully-qualified class
name; add a top-level import for JordanPartridge\GithubClient\Github (e.g., a
use statement) and update the instantiation in the CommitResource constructor to
reference Github::class instead of the FQCN to match the project's import style
and keep consistency with other tests.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2dcaf429-86b0-4327-9d4d-557d0c78d13e
📒 Files selected for processing (18)
composer.jsonphpstan-baseline.neonsrc/Auth/AuthenticationStrategy.phpsrc/Commands/GithubClientCommand.phpsrc/Github.phpsrc/Requests/Checks/GetCheckRunsForRef.phpsrc/Requests/Checks/GetCombinedStatus.phpsrc/Requests/Files/GetContents.phpsrc/Resources/ChecksResource.phpsrc/Resources/FileResource.phpsrc/Resources/PullRequestResource.phptests/ArchTest.phptests/AuthenticationTest.phptests/CommitResourceTest.phptests/DTOPatternTest.phptests/PullRequestsTest.phptests/Unit/Data/RepoDataTest.phptests/Unit/ValueObjects/RepoTest.php
| "php": "^8.2|^8.3|^8.4", | ||
| "firebase/php-jwt": "^7.0", | ||
| "illuminate/contracts": "^11.0||^12.0", | ||
| "illuminate/contracts": "^11.0||^12.0||^13.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What version of orchestra/testbench is compatible with Laravel 13 (illuminate/* v13), and do spatie/laravel-package-tools v1.16 or v2.0 officially support Laravel 13?
💡 Result:
-
Laravel 13 (illuminate/ v13) + orchestra/testbench:* use orchestra/testbench
^11.0(11.x) — it requireslaravel/framework ^13.0.0. [1]
(Testbench Core is also on 11.x for Laravel 13.) [2] -
spatie/laravel-package-tools v1.16.x: does not officially support Laravel 13 —
1.16.0only allowsilluminate/contracts ^9.28|^10.0[3] and1.16.6only up to^11.0. [4] -
spatie/laravel-package-tools v2.0: there is no 2.x release on Packagist (current releases are 1.x; e.g. latest shown is
1.93.0). [5]
If you need Laravel 13 support,spatie/laravel-package-tools 1.93.0explicitly supportsilluminate/contracts ^13.0. [6]
Sources: [1] (packagist.org) [2] (packagist.org) [3] (packages.ecosyste.ms) [4] (packages.ecosyste.ms) [5] (packages.ecosyste.ms) [6] (packagist.org)
Citations:
- 1: https://packagist.org/packages/orchestra/testbench
- 2: https://packagist.org/packages/orchestra/testbench-core
- 3: https://packages.ecosyste.ms/registries/packagist.org/packages/spatie%2Flaravel-package-tools/versions/1.16.0
- 4: https://packages.ecosyste.ms/registries/packagist.org/packages/spatie%2Flaravel-package-tools/versions/1.16.6
- 5: https://packages.ecosyste.ms/registries/packagist.org/packages/spatie%2Flaravel-package-tools/versions
- 6: https://packagist.org/packages/spatie/laravel-package-tools
🏁 Script executed:
cat composer.jsonRepository: jordanpartridge/github-client
Length of output: 3286
Update orchestra/testbench and spatie/laravel-package-tools to properly support Laravel 13.
Line 31 claims support for illuminate/contracts ^13.0, but the dev and runtime dependency constraints are incomplete:
- orchestra/testbench (dev): currently
^8.0||^9.0||^10.0||^11.0||^12.0— testbench 12.x is incompatible with Laravel 13. Add^13.0to the constraint. - spatie/laravel-package-tools (runtime): currently
^1.16|^2.0— versions 1.16.0 through 1.92.x do not support Laravel 13 (only 1.93.0+). Tighten to^1.93|^2.0(noting that 2.x does not yet exist on Packagist, so effectively^1.93.0).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composer.json` at line 31, Update the composer dependency constraints to
properly signal Laravel 13 support: in composer.json amend the
"orchestra/testbench" constraint to include "^13.0" (so it becomes
^8.0||^9.0||^10.0||^11.0||^12.0||^13.0) and tighten
"spatie/laravel-package-tools" to " ^1.93|^2.0" (effectively ^1.93.0 to ensure
versions that support Laravel 13 are required).
Comment on lines
+1
to
+21
| parameters: | ||
| ignoreErrors: | ||
| - | ||
| message: "#^Instantiated class JordanPartridge\\\\GithubClient\\\\Exceptions\\\\ResourceNotFoundException not found\\.$#" | ||
| count: 1 | ||
| path: src/Connectors/GithubConnector.php | ||
|
|
||
| - | ||
| message: "#^Missing parameter \\$message \\(string\\) in call to JordanPartridge\\\\GithubClient\\\\Exceptions\\\\NetworkException constructor\\.$#" | ||
| count: 1 | ||
| path: src/Connectors/GithubConnector.php | ||
|
|
||
| - | ||
| message: "#^Strict comparison using \\=\\=\\= between non\\-falsy\\-string and '' will always evaluate to false\\.$#" | ||
| count: 1 | ||
| path: src/Connectors/GithubConnector.php | ||
|
|
||
| - | ||
| message: "#^Unknown parameter \\$reason in call to JordanPartridge\\\\GithubClient\\\\Exceptions\\\\NetworkException constructor\\.$#" | ||
| count: 1 | ||
| path: src/Connectors/GithubConnector.php |
There was a problem hiding this comment.
Reconsider using a baseline for actual bugs.
PHPStan baselines are designed for:
- Suppressing technical debt tracked for later resolution
- Ignoring false positives from static analysis
- Temporary suppressions during large refactorings
However, this baseline suppresses 3 critical bugs that will cause runtime failures:
- Instantiating a non-existent exception class (fatal error)
- Calling a constructor with wrong parameter names (TypeError)
These should be fixed immediately, not suppressed. Baseline files hide issues from static analysis, giving false confidence that the code is correct.
Recommendation: Fix the critical bugs first, then decide if any remaining suppressions (like the redundant comparison) warrant a baseline entry.
Would you like me to open a separate issue to track fixing these bugs?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@phpstan-baseline.neon` around lines 1 - 21, Remove the three suppressed
PHPStan errors from the baseline and fix the root issues in the GithubConnector:
ensure you reference/instantiate the correct exception class
(JordanPartridge\\GithubClient\\Exceptions\\ResourceNotFoundException) or import
the actual existing exception instead of a non-existent class; update the
NetworkException instantiation to call its actual constructor signature (pass
the required $message string and any valid $code/$previous params, removing the
unknown $reason parameter); and fix the redundant strict comparison (replace the
always-false === check with a proper !== '' or adjust the variable type/logic).
After changes, re-run PHPStan and only keep genuine false-positives in the
baseline.
Comment on lines
+3
to
+6
| - | ||
| message: "#^Instantiated class JordanPartridge\\\\GithubClient\\\\Exceptions\\\\ResourceNotFoundException not found\\.$#" | ||
| count: 1 | ||
| path: src/Connectors/GithubConnector.php |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for ResourceNotFoundException class definition
rg -n 'class ResourceNotFoundException' --type phpRepository: jordanpartridge/github-client
Length of output: 55
Do not suppress instantiation of non-existent ResourceNotFoundException class—fix the underlying bug.
The baseline suppresses a fatal error: "Instantiated class ResourceNotFoundException not found" in GithubConnector.php. This class does not exist in the codebase, so code attempting to instantiate it will fail at runtime.
Fix by either creating the ResourceNotFoundException class or using an existing exception class that matches the intended behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@phpstan-baseline.neon` around lines 3 - 6, The baseline hides a fatal runtime
error caused by instantiating a non-existent ResourceNotFoundException in the
GithubConnector code; fix it by either adding the missing exception class (e.g.,
create class ResourceNotFoundException extends \Exception in the appropriate
namespace) or change the instantiation to use an existing exception type that
conveys "not found" semantics (update the throw/new site in GithubConnector and
its use/import statements to reference the correct exception class, e.g., an
existing NotFoundException or GithubClient\Exceptions\SomeExistingException).
Ensure the chosen exception lives in the namespace imported by GithubConnector
(adjust use statements) and remove the phpstan baseline suppression for that
message.
Comment on lines
+8
to
+11
| - | ||
| message: "#^Missing parameter \\$message \\(string\\) in call to JordanPartridge\\\\GithubClient\\\\Exceptions\\\\NetworkException constructor\\.$#" | ||
| count: 1 | ||
| path: src/Connectors/GithubConnector.php |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find NetworkException constructor calls with 'reason:' parameter
rg -n -A3 'new NetworkException\(' src/Connectors/GithubConnector.phpRepository: jordanpartridge/github-client
Length of output: 301
🏁 Script executed:
#!/bin/bash
# Find NetworkException class definition
find . -name "*.php" -type f | xargs grep -l "class NetworkException"Repository: jordanpartridge/github-client
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Check the constructor of NetworkException
rg -A10 "class NetworkException" --type phpRepository: jordanpartridge/github-client
Length of output: 750
🏁 Script executed:
#!/bin/bash
# Find all NetworkException constructor calls
rg -n "new NetworkException\(" --type php -A4Repository: jordanpartridge/github-client
Length of output: 1114
Critical: Fix mismatched parameter name instead of suppressing the error.
Lines 8-11 and 18-21 in phpstan-baseline.neon suppress errors about incorrect NetworkException constructor calls. The constructor signature is:
public function __construct(
string $operation,
string $message,
int $code = 0,
?\Throwable $previous = null,
)However, src/Connectors/GithubConnector.php:148 calls it with the wrong parameter name:
new NetworkException(
operation: 'GitHub API request',
reason: "Server error ({$status}): {$message}", // ✗ wrong parameter name
previous: $senderException,
)This will cause a fatal TypeError at runtime: unknown parameter $reason and missing required parameter $message.
Fix in GithubConnector.php:
Corrected code
new NetworkException(
operation: 'GitHub API request',
- reason: "Server error ({$status}): {$message}",
+ message: "Server error ({$status}): {$message}",
previous: $senderException,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@phpstan-baseline.neon` around lines 8 - 11, The call to NetworkException in
GithubConnector (the instantiation in src/Connectors/GithubConnector.php) uses
the wrong named parameter "reason" and omits the required "message" parameter;
update the constructor call to use message: "Server error ({$status}):
{$message}" (or equivalent string) instead of reason:, keep operation: 'GitHub
API request' and previous: $senderException so the parameters match
NetworkException::__construct(string $operation, string $message, int $code = 0,
?\Throwable $previous = null).
Comment on lines
+13
to
+16
| - | ||
| message: "#^Strict comparison using \\=\\=\\= between non\\-falsy\\-string and '' will always evaluate to false\\.$#" | ||
| count: 1 | ||
| path: src/Connectors/GithubConnector.php |
There was a problem hiding this comment.
Verify if the redundant empty string check is intentional.
The baseline suppresses PHPStan's warning about $this->token === '' always evaluating to false after the falsy check. In the code:
if (! $this->token || $this->token === '')The empty string case is already covered by ! $this->token since empty strings are falsy. The explicit === '' check is redundant.
Options:
- If the redundancy is intentional for clarity, document it with a comment and keep the suppression.
- Otherwise, simplify to
if (! $this->token)and remove this baseline entry.
♻️ Suggested simplification
-if (! $this->token || $this->token === '') {
+if (! $this->token) {
return null;
}Then remove lines 13-16 from this baseline file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@phpstan-baseline.neon` around lines 13 - 16, The baseline suppresses
PHPStan's warning about the redundant check for `$this->token === ''` in the
condition `if (! $this->token || $this->token === '')`; locate the usage of
`$this->token` in the GithubConnector class (e.g., the method containing that
`if` check) and either remove the redundant `=== ''` branch and delete the
corresponding baseline entry (lines referencing
src/Connectors/GithubConnector.php and the Strict comparison message), or if you
intentionally kept the explicit empty-string check for clarity, add an inline
comment next to `if (! $this->token || $this->token === '')` explaining why both
checks are present and keep the baseline suppression; choose one of these two
fixes and apply it consistently.
Comment on lines
+61
to
+64
| public function checks(): ChecksResource | ||
| { | ||
| return new ChecksResource($this); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check current facade annotations and verify checks() is missing
echo "=== Current facade annotations ==="
rg -A 10 '@method' src/Facades/Github.php
echo ""
echo "=== Checking if checks() annotation exists ==="
rg 'checks\(\)' src/Facades/Github.php || echo "checks() annotation NOT found - needs to be added"Repository: jordanpartridge/github-client
Length of output: 932
Add missing @method annotation to the facade.
The new checks() method in src/Github.php requires a corresponding @method annotation in src/Facades/Github.php to enable IDE autocomplete and static analysis for Github::checks() calls.
Add this annotation with the other @method declarations:
`@method` static \JordanPartridge\GithubClient\Resources\ChecksResource checks()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Github.php` around lines 61 - 64, Add a matching `@method` annotation to
the Github facade so IDEs and static analyzers recognize the new checks()
method: in src/Facades/Github.php add the annotation "@method static
\JordanPartridge\GithubClient\Resources\ChecksResource checks()" alongside the
existing `@method` declarations for other resource methods, ensuring the facade
correctly documents the new checks() method defined in the Github class.
Comment on lines
+43
to
+62
| public function allPassing(string $owner, string $repo, string $ref): bool | ||
| { | ||
| $checkRuns = $this->forRef($owner, $repo, $ref); | ||
|
|
||
| if (empty($checkRuns['check_runs'])) { | ||
| return true; | ||
| } | ||
|
|
||
| foreach ($checkRuns['check_runs'] as $run) { | ||
| if ($run['status'] !== 'completed') { | ||
| return false; | ||
| } | ||
|
|
||
| if ($run['conclusion'] !== 'success' && $run['conclusion'] !== 'skipped') { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Pagination gap may cause false positives when many check runs exist.
allPassing() only evaluates the first page of results from forRef(). If a repository has more check runs than GitHub's default page size (~30), failing checks on subsequent pages won't be detected, causing the method to incorrectly return true.
Consider either:
- Fetching all pages before evaluating, or
- At minimum, comparing
total_countagainst the actualcheck_runscount to detect truncation
🐛 Option 1: Detect pagination truncation and throw
public function allPassing(string $owner, string $repo, string $ref): bool
{
$checkRuns = $this->forRef($owner, $repo, $ref);
+ // Guard against incomplete results due to pagination
+ if ($checkRuns['total_count'] > count($checkRuns['check_runs'])) {
+ throw new \RuntimeException(
+ "Check runs response is paginated ({$checkRuns['total_count']} total, " .
+ count($checkRuns['check_runs']) . " returned). Cannot reliably determine all passing."
+ );
+ }
+
if (empty($checkRuns['check_runs'])) {
return true;
}🐛 Option 2: Fetch all pages (more complete solution)
public function allPassing(string $owner, string $repo, string $ref): bool
{
- $checkRuns = $this->forRef($owner, $repo, $ref);
-
- if (empty($checkRuns['check_runs'])) {
- return true;
- }
+ $page = 1;
+ $perPage = 100; // Max allowed by GitHub API
+
+ do {
+ $response = $this->forRef($owner, $repo, $ref, $perPage, $page);
+ $checkRuns = $response['check_runs'];
+
+ foreach ($checkRuns as $run) {
+ if ($run['status'] !== 'completed') {
+ return false;
+ }
+
+ if ($run['conclusion'] !== 'success' && $run['conclusion'] !== 'skipped') {
+ return false;
+ }
+ }
+
+ $page++;
+ $hasMore = count($checkRuns) === $perPage;
+ } while ($hasMore);
- foreach ($checkRuns['check_runs'] as $run) {
- if ($run['status'] !== 'completed') {
- return false;
- }
-
- if ($run['conclusion'] !== 'success' && $run['conclusion'] !== 'skipped') {
- return false;
- }
- }
-
return true;
}📝 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.
Suggested change
| public function allPassing(string $owner, string $repo, string $ref): bool | |
| { | |
| $checkRuns = $this->forRef($owner, $repo, $ref); | |
| if (empty($checkRuns['check_runs'])) { | |
| return true; | |
| } | |
| foreach ($checkRuns['check_runs'] as $run) { | |
| if ($run['status'] !== 'completed') { | |
| return false; | |
| } | |
| if ($run['conclusion'] !== 'success' && $run['conclusion'] !== 'skipped') { | |
| return false; | |
| } | |
| } | |
| return true; | |
| } | |
| public function allPassing(string $owner, string $repo, string $ref): bool | |
| { | |
| $checkRuns = $this->forRef($owner, $repo, $ref); | |
| // Guard against incomplete results due to pagination | |
| if ($checkRuns['total_count'] > count($checkRuns['check_runs'])) { | |
| throw new \RuntimeException( | |
| "Check runs response is paginated ({$checkRuns['total_count']} total, " . | |
| count($checkRuns['check_runs']) . " returned). Cannot reliably determine all passing." | |
| ); | |
| } | |
| if (empty($checkRuns['check_runs'])) { | |
| return true; | |
| } | |
| foreach ($checkRuns['check_runs'] as $run) { | |
| if ($run['status'] !== 'completed') { | |
| return false; | |
| } | |
| if ($run['conclusion'] !== 'success' && $run['conclusion'] !== 'skipped') { | |
| return false; | |
| } | |
| } | |
| return true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Resources/ChecksResource.php` around lines 43 - 62, allPassing() only
inspects the first page returned by forRef(), causing false positives when
results are paginated; update the logic to detect and handle pagination by
either (a) using the response's total_count vs count(check_runs) to detect
truncation and throw an error, or (b) iterating pages and aggregating all check
runs before evaluating. Concretely, modify either forRef() to fetch all pages
(looping with page/per_page until collected) or change allPassing() to call
forRef() repeatedly (page++/per_page) and merge all check_runs, then run the
existing status/conclusion checks on the full aggregated list; ensure you
reference forRef() and allPassing() when implementing the pagination loop or
truncation check.
Comment on lines
+41
to
+50
| public function getContent(string $owner, string $repo, string $path, ?string $ref = null): string | ||
| { | ||
| $data = $this->contents($owner, $repo, $path, $ref); | ||
|
|
||
| if ($data['encoding'] === 'base64') { | ||
| return base64_decode($data['content']); | ||
| } | ||
|
|
||
| return $data['content']; | ||
| } |
There was a problem hiding this comment.
Potential runtime errors in getContent() method.
Several edge cases could cause issues:
-
Missing key access: If
$data['encoding']or$data['content']don't exist (e.g., when the path is a directory or submodule), this will throw an undefined array key error. -
base64_decodecan returnfalse: On malformed input,base64_decode()returnsfalse, but the method declares astringreturn type. -
Directory responses: GitHub's API returns an array of objects when the path is a directory, not a single file object.
🛡️ Proposed fix with defensive checks
public function getContent(string $owner, string $repo, string $path, ?string $ref = null): string
{
$data = $this->contents($owner, $repo, $path, $ref);
+ if (!isset($data['content'], $data['encoding'])) {
+ throw new \RuntimeException('Path does not point to a file or content is unavailable');
+ }
+
if ($data['encoding'] === 'base64') {
- return base64_decode($data['content']);
+ $decoded = base64_decode($data['content'], true);
+ if ($decoded === false) {
+ throw new \RuntimeException('Failed to decode base64 content');
+ }
+ return $decoded;
}
return $data['content'];
}📝 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.
Suggested change
| public function getContent(string $owner, string $repo, string $path, ?string $ref = null): string | |
| { | |
| $data = $this->contents($owner, $repo, $path, $ref); | |
| if ($data['encoding'] === 'base64') { | |
| return base64_decode($data['content']); | |
| } | |
| return $data['content']; | |
| } | |
| public function getContent(string $owner, string $repo, string $path, ?string $ref = null): string | |
| { | |
| $data = $this->contents($owner, $repo, $path, $ref); | |
| if (!isset($data['content'], $data['encoding'])) { | |
| throw new \RuntimeException('Path does not point to a file or content is unavailable'); | |
| } | |
| if ($data['encoding'] === 'base64') { | |
| $decoded = base64_decode($data['content'], true); | |
| if ($decoded === false) { | |
| throw new \RuntimeException('Failed to decode base64 content'); | |
| } | |
| return $decoded; | |
| } | |
| return $data['content']; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Resources/FileResource.php` around lines 41 - 50, The getContent method
can fail when contents(...) returns a directory array, is missing
'encoding'/'content' keys, or when base64_decode returns false; update
getContent to validate the response from contents($owner,$repo,$path,$ref): if
the return is an array of objects (directory) or doesn't contain string
'content'/'encoding' keys, throw or return a clear error/empty string, and when
'encoding' === 'base64' call base64_decode and check for false—if decoding
fails, throw a descriptive exception (or fallback to an empty string) so the
method always satisfies its string return type; reference the getContent method
and the contents(...) call to locate and implement these defensive checks and
error handling.
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
1 participant
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.
Summary
forRef(),combinedStatus(),allPassing()— query CI check runs and combined status for any commit refcontents()andgetContent()— read file contents at a specific ref (branch/tag/SHA), with auto base64 decodingilluminate/contractsto^11.0||^12.0||^13.0New files
src/Requests/Checks/GetCheckRunsForRef.phpsrc/Requests/Checks/GetCombinedStatus.phpsrc/Requests/Files/GetContents.phpsrc/Resources/ChecksResource.phpContext
These additions support
the-shit/review— a PR review toolkit that needs to read check status and file contents at specific refs to do structural analysis.Test plan
Summary by CodeRabbit
Release Notes
New Features
Chores