feat: full quality gate v2 — PHPStan, Pint, Rector, Publish Guard#73
Open
jordanpartridge wants to merge 5 commits into
Open
feat: full quality gate v2 — PHPStan, Pint, Rector, Publish Guard#73jordanpartridge wants to merge 5 commits into
jordanpartridge wants to merge 5 commits into
Conversation
…tify Wire 4 new checks into CertifyCommand alongside existing Tests, Security, and Syntax: - PhpStanAnalyzer: configurable level (--phpstan-level, default 5), JSON output parsing, graceful skip if not installed - PintFormatter: --test mode, JSON output, reports files needing formatting - RectorAnalyzer: --dry-run mode, graceful skip if no rector.php config - PublishGuard: scans for .env files (excludes .testing/.example), .map files, large binaries (>1MB) All checks follow existing CheckInterface pattern with pass/fail/skip semantics. New action.yml input: phpstan-level (default 5, set -1 to skip). Certify now runs 7 checks: Tests → Pint → PHPStan → Rector → Security → Syntax → Publish
📝 WalkthroughWalkthroughAdds four new checks (PhpStanAnalyzer, PintFormatter, RectorAnalyzer, PublishGuard), integrates them into the certify pipeline and GitHub Action via a new Changes
Sequence Diagram(s)sequenceDiagram
participant Certify as CertifyCommand
participant PhpStan as PhpStanAnalyzer
participant Pint as PintFormatter
participant Rector as RectorAnalyzer
participant Publish as PublishGuard
participant Runner as ProcessRunner
Certify->>PhpStan: run(workingDirectory, level)
PhpStan->>Runner: execute php .../gate certify --phpstan-level
Runner-->>PhpStan: JSON output / exitCode
PhpStan-->>Certify: CheckResult
Certify->>Pint: run(workingDirectory)
Pint->>Runner: execute vendor/bin/pint --test --format=json
Runner-->>Pint: JSON output / exitCode
Pint-->>Certify: CheckResult
Certify->>Rector: run(workingDirectory)
Rector->>Runner: execute vendor/bin/rector --dry-run
Runner-->>Rector: output / exitCode
Rector-->>Certify: CheckResult
Certify->>Publish: run(workingDirectory)
Publish->>Runner: execute git ls-files / inspect files
Runner-->>Publish: file listings
Publish-->>Certify: CheckResult
Certify->>Certify: aggregate & report results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.46)/app/Branding.php ... [truncated 95241 characters] ... dor/nesbot/carbon/src/Carbon/Carbon.php PHPStan process crashed because it reached configured PHP memory limit: 8192M 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 |
📊 Coverage Report
Files Below Threshold
🏆 Synapse Sentinel Gate |
…d JSON structures Fix pre-existing pint style issues across gate codebase (4 files). Guard PintFormatter::run() against missing 'path' key in JSON output since different pint versions may structure the files array differently.
🔧 Synapse Sentinel: 3 checks need attentionThe following issues must be resolved before this PR can be merged: Test Failures (1 total)Fix these failing tests: 1. CertifyCommand → handle → it handles multiple failed checks 0.19sFAIL at Fix: Review the test expectation vs actual behavior. Check the tested code logic. Pint StyleReview the output and fix any issues.Security AuditReview the output and fix any issues.Quick Reference:
🤖 Generated by Synapse Sentinel - View Run |
🔧 Synapse Sentinel: 2 checks need attentionThe following issues must be resolved before this PR can be merged: Test Failures (1 total)Fix these failing tests: 1. CertifyCommand → handle → it returns failure when any check fails 0.12sFAIL at Fix: Review the test expectation vs actual behavior. Check the tested code logic. Pint StyleReview the output and fix any issues.Quick Reference:
🤖 Generated by Synapse Sentinel - View Run |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
app/Commands/CertifyCommand.php (1)
223-225: Compact aliases are now inconsistent across new checks.Consider adding short names for
PHPStanandRectoras well, so compact output remains uniformly concise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Commands/CertifyCommand.php` around lines 223 - 225, Add compact aliases for the PHPStan and Rector checks in the same mapping where 'Pint Style' => 'Pint' and 'Publish Guard' => 'Publish' are defined: add entries like 'PHPStan' => 'Stan' and 'Rector' => 'Rect' (or other preferred short labels) so the compact output remains uniformly concise; update the associative array that performs this name mapping to include these two new key=>value pairs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Checks/PhpStanAnalyzer.php`:
- Around line 60-63: In PhpStanAnalyzer (inside the loop that builds $details
from $data['files']), guard the message fields before concatenation: check
$msg['line'] and $msg['message'] and use safe fallbacks (e.g., $msg['line'] ?? 0
and $msg['message'] ?? '') or skip entries that lack both, then build the detail
string (currently done with basename($file).':'.$msg['line'].' —
'.$msg['message']) using those guarded values so notices are avoided; update the
foreach that populates $details accordingly.
- Around line 22-34: The run method in PhpStanAnalyzer currently always executes
PHPStan even when configured with phpstan-level=-1 and doesn't validate the
level; update PhpStanAnalyzer::run to first check if $this->level === -1 and
immediately return CheckResult::pass('PHPStan skipped via configuration') to
implement the skip behavior, then validate that $this->level is an integer
within the allowed range (e.g., minimum and maximum allowed PHPStan levels
configured for your project) and return a CheckResult::fail with a clear message
if it's out of range before constructing and running the process; only then
build the $binary command and call $this->processRunner->run as before.
In `@app/Checks/PublishGuard.php`:
- Around line 21-63: The PublishGuard::run currently checks for .env, .map and
large files but lacks secret/artifact detection; update run to also scan tracked
files for common secret filenames/extensions (e.g., files ending with .key,
.pem, .crt, .p12, .jks, .pfx or names like id_rsa, private_key, secret, token)
and to search file contents for high-confidence secret patterns (e.g., base64
API keys, AWS keys, private key headers like "-----BEGIN PRIVATE KEY-----", or
strings like "API_KEY", "SECRET=", "PRIVATE_KEY"). Use the existing git ls-files
result ($lsResult->output) to iterate tracked files, skip binaries by size/type
if needed, read files safely (check file_exists and filesize) and add violations
to $violations with clear messages (e.g., "Tracked secret file: {file}" or
"Secret pattern in: {file}"). Ensure this logic is placed alongside the
large-file loop in PublishGuard::run so secrets are reported together.
- Around line 26-54: The checks call $this->processRunner->run (producing
$envResult, $mapResult, $lsResult) and then parse ->output without verifying the
command succeeded; if git fails the guard can incorrectly pass. After each run()
call (for $envResult, $mapResult and $lsResult) check the returned result for
failure (e.g. non-zero exit code or an isSuccessful()/successful flag) and if it
failed, treat it as a violation (or throw) to "fail closed" — add a violation
entry like "git ls-files failed: {stderr or exit code}" or throw a
PublishGuardException so the publish is blocked; ensure you reference
$envResult->errorOutput / $mapResult->errorOutput / $lsResult->errorOutput (or
->exitCode) when constructing the message.
In `@app/Checks/RectorAnalyzer.php`:
- Around line 57-59: The failure message in RectorAnalyzer uses
max($changeCount, 1) which forces a minimum of 1 even when no diffs were found;
change the logic in the return call that invokes CheckResult::fail to use the
actual $changeCount (not max(...,1)) and interpolate that value into the message
and pluralization, and ensure the array_slice($details, 0, 20) remains
unchanged; this will ensure the reported file count reflects the real number of
diffs found.
---
Nitpick comments:
In `@app/Commands/CertifyCommand.php`:
- Around line 223-225: Add compact aliases for the PHPStan and Rector checks in
the same mapping where 'Pint Style' => 'Pint' and 'Publish Guard' => 'Publish'
are defined: add entries like 'PHPStan' => 'Stan' and 'Rector' => 'Rect' (or
other preferred short labels) so the compact output remains uniformly concise;
update the associative array that performs this name mapping to include these
two new key=>value pairs.
🪄 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: cb4e8e5c-74a0-41d8-90f8-400761c074b9
📒 Files selected for processing (10)
action.ymlapp/Checks/PhpStanAnalyzer.phpapp/Checks/PintFormatter.phpapp/Checks/PublishGuard.phpapp/Checks/RectorAnalyzer.phpapp/Commands/CertifyCommand.phpapp/GitHub/ChecksClient.phpapp/Services/PromptAssembler.phpapp/Transformers/PhpStanPromptTransformer.phpapp/Transformers/TestFailurePromptTransformer.php
Comment on lines
+22
to
+34
| public function run(string $workingDirectory): CheckResult | ||
| { | ||
| $binary = $workingDirectory.'/vendor/bin/phpstan'; | ||
|
|
||
| if (! file_exists($binary)) { | ||
| return CheckResult::pass('PHPStan not installed — skipped'); | ||
| } | ||
|
|
||
| $result = $this->processRunner->run( | ||
| [$binary, 'analyse', '--no-progress', '--error-format=json', "--level={$this->level}", '--memory-limit=512M'], | ||
| $workingDirectory, | ||
| timeout: 300, | ||
| ); |
There was a problem hiding this comment.
phpstan-level=-1 skip behavior is not implemented.
The analyzer always runs and forwards the level, so a configured skip value still invokes PHPStan. Add an explicit skip branch and validate allowed range before execution.
Suggested fix
public function run(string $workingDirectory): CheckResult
{
+ if ($this->level === -1) {
+ return CheckResult::pass('PHPStan skipped (level -1)');
+ }
+
+ if ($this->level < 0 || $this->level > 9) {
+ return CheckResult::fail(
+ "Invalid PHPStan level: {$this->level}. Expected -1 or 0-9."
+ );
+ }
+
$binary = $workingDirectory.'/vendor/bin/phpstan';📝 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 run(string $workingDirectory): CheckResult | |
| { | |
| $binary = $workingDirectory.'/vendor/bin/phpstan'; | |
| if (! file_exists($binary)) { | |
| return CheckResult::pass('PHPStan not installed — skipped'); | |
| } | |
| $result = $this->processRunner->run( | |
| [$binary, 'analyse', '--no-progress', '--error-format=json', "--level={$this->level}", '--memory-limit=512M'], | |
| $workingDirectory, | |
| timeout: 300, | |
| ); | |
| public function run(string $workingDirectory): CheckResult | |
| { | |
| if ($this->level === -1) { | |
| return CheckResult::pass('PHPStan skipped (level -1)'); | |
| } | |
| if ($this->level < 0 || $this->level > 9) { | |
| return CheckResult::fail( | |
| "Invalid PHPStan level: {$this->level}. Expected -1 or 0-9." | |
| ); | |
| } | |
| $binary = $workingDirectory.'/vendor/bin/phpstan'; | |
| if (! file_exists($binary)) { | |
| return CheckResult::pass('PHPStan not installed — skipped'); | |
| } | |
| $result = $this->processRunner->run( | |
| [$binary, 'analyse', '--no-progress', '--error-format=json', "--level={$this->level}", '--memory-limit=512M'], | |
| $workingDirectory, | |
| timeout: 300, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Checks/PhpStanAnalyzer.php` around lines 22 - 34, The run method in
PhpStanAnalyzer currently always executes PHPStan even when configured with
phpstan-level=-1 and doesn't validate the level; update PhpStanAnalyzer::run to
first check if $this->level === -1 and immediately return
CheckResult::pass('PHPStan skipped via configuration') to implement the skip
behavior, then validate that $this->level is an integer within the allowed range
(e.g., minimum and maximum allowed PHPStan levels configured for your project)
and return a CheckResult::fail with a clear message if it's out of range before
constructing and running the process; only then build the $binary command and
call $this->processRunner->run as before.
Comment on lines
+60
to
+63
| foreach ($data['files'] ?? [] as $file => $fileData) { | ||
| foreach ($fileData['messages'] ?? [] as $msg) { | ||
| $details[] = basename($file).':'.$msg['line'].' — '.$msg['message']; | ||
| } |
There was a problem hiding this comment.
Guard message fields before building detail lines.
Line 62 assumes line and message always exist. Use fallbacks to avoid notices on unexpected payloads.
Suggested fix
foreach ($data['files'] ?? [] as $file => $fileData) {
foreach ($fileData['messages'] ?? [] as $msg) {
- $details[] = basename($file).':'.$msg['line'].' — '.$msg['message'];
+ $line = $msg['line'] ?? '?';
+ $message = $msg['message'] ?? 'Unknown PHPStan error';
+ $details[] = basename($file).':'.$line.' — '.$message;
}
}📝 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
| foreach ($data['files'] ?? [] as $file => $fileData) { | |
| foreach ($fileData['messages'] ?? [] as $msg) { | |
| $details[] = basename($file).':'.$msg['line'].' — '.$msg['message']; | |
| } | |
| foreach ($data['files'] ?? [] as $file => $fileData) { | |
| foreach ($fileData['messages'] ?? [] as $msg) { | |
| $line = $msg['line'] ?? '?'; | |
| $message = $msg['message'] ?? 'Unknown PHPStan error'; | |
| $details[] = basename($file).':'.$line.' — '.$message; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Checks/PhpStanAnalyzer.php` around lines 60 - 63, In PhpStanAnalyzer
(inside the loop that builds $details from $data['files']), guard the message
fields before concatenation: check $msg['line'] and $msg['message'] and use safe
fallbacks (e.g., $msg['line'] ?? 0 and $msg['message'] ?? '') or skip entries
that lack both, then build the detail string (currently done with
basename($file).':'.$msg['line'].' — '.$msg['message']) using those guarded
values so notices are avoided; update the foreach that populates $details
accordingly.
Comment on lines
+21
to
+63
| public function run(string $workingDirectory): CheckResult | ||
| { | ||
| $violations = []; | ||
|
|
||
| // Check for .env files (excluding .env.testing and .env.example) | ||
| $envResult = $this->processRunner->run( | ||
| ['git', 'ls-files', '*.env', '.env*'], | ||
| $workingDirectory, | ||
| timeout: 10, | ||
| ); | ||
|
|
||
| foreach (array_filter(explode("\n", trim($envResult->output))) as $file) { | ||
| if (! str_ends_with($file, '.env.testing') && ! str_ends_with($file, '.env.example')) { | ||
| $violations[] = "Tracked .env file: {$file}"; | ||
| } | ||
| } | ||
|
|
||
| // Check for source map files | ||
| $mapResult = $this->processRunner->run( | ||
| ['git', 'ls-files', '*.map'], | ||
| $workingDirectory, | ||
| timeout: 10, | ||
| ); | ||
|
|
||
| foreach (array_filter(explode("\n", trim($mapResult->output))) as $file) { | ||
| $violations[] = "Tracked .map file: {$file}"; | ||
| } | ||
|
|
||
| // Check for large files (> 1MB) | ||
| $lsResult = $this->processRunner->run( | ||
| ['git', 'ls-files'], | ||
| $workingDirectory, | ||
| timeout: 10, | ||
| ); | ||
|
|
||
| foreach (array_filter(explode("\n", trim($lsResult->output))) as $file) { | ||
| $path = $workingDirectory.'/'.$file; | ||
| if (file_exists($path) && filesize($path) > 1_048_576) { | ||
| $size = round(filesize($path) / 1_048_576, 1); | ||
| $violations[] = "Large file ({$size}MB): {$file}"; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Secrets check is missing from Publish Guard.
This implementation enforces .env, .map, and size checks, but there is no explicit secret-artifact detection (e.g., key/cert files), which is part of the stated quality gate scope.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Checks/PublishGuard.php` around lines 21 - 63, The PublishGuard::run
currently checks for .env, .map and large files but lacks secret/artifact
detection; update run to also scan tracked files for common secret
filenames/extensions (e.g., files ending with .key, .pem, .crt, .p12, .jks, .pfx
or names like id_rsa, private_key, secret, token) and to search file contents
for high-confidence secret patterns (e.g., base64 API keys, AWS keys, private
key headers like "-----BEGIN PRIVATE KEY-----", or strings like "API_KEY",
"SECRET=", "PRIVATE_KEY"). Use the existing git ls-files result
($lsResult->output) to iterate tracked files, skip binaries by size/type if
needed, read files safely (check file_exists and filesize) and add violations to
$violations with clear messages (e.g., "Tracked secret file: {file}" or "Secret
pattern in: {file}"). Ensure this logic is placed alongside the large-file loop
in PublishGuard::run so secrets are reported together.
Comment on lines
+26
to
+54
| $envResult = $this->processRunner->run( | ||
| ['git', 'ls-files', '*.env', '.env*'], | ||
| $workingDirectory, | ||
| timeout: 10, | ||
| ); | ||
|
|
||
| foreach (array_filter(explode("\n", trim($envResult->output))) as $file) { | ||
| if (! str_ends_with($file, '.env.testing') && ! str_ends_with($file, '.env.example')) { | ||
| $violations[] = "Tracked .env file: {$file}"; | ||
| } | ||
| } | ||
|
|
||
| // Check for source map files | ||
| $mapResult = $this->processRunner->run( | ||
| ['git', 'ls-files', '*.map'], | ||
| $workingDirectory, | ||
| timeout: 10, | ||
| ); | ||
|
|
||
| foreach (array_filter(explode("\n", trim($mapResult->output))) as $file) { | ||
| $violations[] = "Tracked .map file: {$file}"; | ||
| } | ||
|
|
||
| // Check for large files (> 1MB) | ||
| $lsResult = $this->processRunner->run( | ||
| ['git', 'ls-files'], | ||
| $workingDirectory, | ||
| timeout: 10, | ||
| ); |
There was a problem hiding this comment.
Fail closed when git ls-files fails.
Lines 26–54 parse command output without checking command success. A failed git call can bypass the guard and incorrectly report pass.
Suggested fix
$envResult = $this->processRunner->run(
['git', 'ls-files', '*.env', '.env*'],
$workingDirectory,
timeout: 10,
);
+ if (! $envResult->successful) {
+ return CheckResult::fail(
+ 'Publish Guard failed to inspect tracked .env files',
+ [trim($envResult->output)],
+ $envResult->output,
+ );
+ }
@@
$mapResult = $this->processRunner->run(
['git', 'ls-files', '*.map'],
$workingDirectory,
timeout: 10,
);
+ if (! $mapResult->successful) {
+ return CheckResult::fail(
+ 'Publish Guard failed to inspect tracked .map files',
+ [trim($mapResult->output)],
+ $mapResult->output,
+ );
+ }
@@
$lsResult = $this->processRunner->run(
['git', 'ls-files'],
$workingDirectory,
timeout: 10,
);
+ if (! $lsResult->successful) {
+ return CheckResult::fail(
+ 'Publish Guard failed to inspect tracked files',
+ [trim($lsResult->output)],
+ $lsResult->output,
+ );
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Checks/PublishGuard.php` around lines 26 - 54, The checks call
$this->processRunner->run (producing $envResult, $mapResult, $lsResult) and then
parse ->output without verifying the command succeeded; if git fails the guard
can incorrectly pass. After each run() call (for $envResult, $mapResult and
$lsResult) check the returned result for failure (e.g. non-zero exit code or an
isSuccessful()/successful flag) and if it failed, treat it as a violation (or
throw) to "fail closed" — add a violation entry like "git ls-files failed:
{stderr or exit code}" or throw a PublishGuardException so the publish is
blocked; ensure you reference $envResult->errorOutput / $mapResult->errorOutput
/ $lsResult->errorOutput (or ->exitCode) when constructing the message.
Comment on lines
+57
to
+59
| return CheckResult::fail( | ||
| 'Rector found '.max($changeCount, 1).' file(s) needing refactoring', | ||
| array_slice($details, 0, 20), |
There was a problem hiding this comment.
Failure count/message can be incorrect when no diffs are detected.
Lines 57–59 force at least 1 changed file even when parsing finds zero diff markers, which can misreport tooling/config failures as refactoring suggestions.
Suggested fix
- return CheckResult::fail(
- 'Rector found '.max($changeCount, 1).' file(s) needing refactoring',
- array_slice($details, 0, 20),
- $result->output,
- );
+ $message = $changeCount > 0
+ ? "Rector found {$changeCount} file(s) needing refactoring"
+ : 'Rector failed to run cleanly; see raw output';
+
+ return CheckResult::fail(
+ $message,
+ array_slice($details, 0, 20),
+ $result->output,
+ );📝 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
| return CheckResult::fail( | |
| 'Rector found '.max($changeCount, 1).' file(s) needing refactoring', | |
| array_slice($details, 0, 20), | |
| $message = $changeCount > 0 | |
| ? "Rector found {$changeCount} file(s) needing refactoring" | |
| : 'Rector failed to run cleanly; see raw output'; | |
| return CheckResult::fail( | |
| $message, | |
| array_slice($details, 0, 20), | |
| $result->output, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Checks/RectorAnalyzer.php` around lines 57 - 59, The failure message in
RectorAnalyzer uses max($changeCount, 1) which forces a minimum of 1 even when
no diffs were found; change the logic in the return call that invokes
CheckResult::fail to use the actual $changeCount (not max(...,1)) and
interpolate that value into the message and pluralization, and ensure the
array_slice($details, 0, 20) remains unchanged; this will ensure the reported
file count reflects the real number of diffs found.
🔧 Synapse Sentinel: 2 checks need attentionThe following issues must be resolved before this PR can be merged: All tests passed.--- Pint StyleReview the output and fix any issues.Quick Reference:
🤖 Generated by Synapse Sentinel - View Run |
🔧 Synapse Sentinel: 2 checks need attentionThe following issues must be resolved before this PR can be merged: All tests passed.--- Pint StyleReview the output and fix any issues.Quick Reference:
🤖 Generated by Synapse Sentinel - View Run |
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)
tests/Unit/Commands/CertifyCommandTest.php (1)
59-67:⚠️ Potential issue | 🟠 MajorActionable-prompt path is still not exercised in these failure tests.
You added an extra mocked
Response(201)for actionable prompt, but these cases instantiateChecksClientwithout a PR number.postActionablePrompt()short-circuits when PR number is null, so the new mocked response is unused and the actionable-post flow remains untested.Suggested fix
- $checksClient = new ChecksClient('token', $httpClient, 'owner/repo', 'sha123'); + $checksClient = new ChecksClient('token', $httpClient, 'owner/repo', 'sha123', 1);Apply this to each failure-path test updated with the extra actionable prompt response.
Also applies to: 81-88, 166-174, 226-233, 253-261, 316-324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Commands/CertifyCommandTest.php` around lines 59 - 67, The tests add an extra mocked Response(201) intended for the actionable-prompt path but instantiate ChecksClient without a PR number so postActionablePrompt() short-circuits and never uses that mock; update the test instances that include the extra mock to construct ChecksClient with a PR number (e.g., pass 'owner/repo', 'sha123', and a PR number) so postActionablePrompt() runs and consumes the actionable-prompt Response(201); apply the same change to the other affected tests mentioned (lines ~81-88, 166-174, 226-233, 253-261, 316-324) to exercise the actionable-post flow.
🤖 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 `@tests/Unit/Commands/CertifyCommandTest.php`:
- Around line 59-67: The tests add an extra mocked Response(201) intended for
the actionable-prompt path but instantiate ChecksClient without a PR number so
postActionablePrompt() short-circuits and never uses that mock; update the test
instances that include the extra mock to construct ChecksClient with a PR number
(e.g., pass 'owner/repo', 'sha123', and a PR number) so postActionablePrompt()
runs and consumes the actionable-prompt Response(201); apply the same change to
the other affected tests mentioned (lines ~81-88, 166-174, 226-233, 253-261,
316-324) to exercise the actionable-post flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98875206-1a65-4b62-916e-ea945b535bc4
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
app/Checks/TestRunner.phpapp/Commands/CertifyCommand.phpbootstrap/app.phpconfig/app.phpconfig/commands.phptests/Unit/Checks/PestSyntaxValidatorTest.phptests/Unit/Checks/SecurityScannerTest.phptests/Unit/Checks/TestRunnerTest.phptests/Unit/Commands/CertifyCommandTest.php
✅ Files skipped from review due to trivial changes (6)
- bootstrap/app.php
- config/app.php
- tests/Unit/Checks/PestSyntaxValidatorTest.php
- tests/Unit/Checks/SecurityScannerTest.php
- config/commands.php
- app/Checks/TestRunner.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Commands/CertifyCommand.php
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
Upgrades
certifyfrom 3 checks to 7 — making gate the universal quality standard across the ecosystem.New checks
New action input
Certify order
Tests → Pint → PHPStan → Rector → Security → Syntax → Publish
Backward compatible
Consumer usage after merge
Summary by CodeRabbit
New Features
Tests
Chores