Skip to content

fix: add instanceof RequestException check in ChecksClient.logError()#54

Open
jordanpartridge wants to merge 1 commit intomasterfrom
fix/checks-client-type-safety
Open

fix: add instanceof RequestException check in ChecksClient.logError()#54
jordanpartridge wants to merge 1 commit intomasterfrom
fix/checks-client-type-safety

Conversation

@jordanpartridge
Copy link
Contributor

@jordanpartridge jordanpartridge commented Mar 5, 2026

Summary

  • logError() accepts GuzzleException but called hasResponse() which only exists on RequestException — added instanceof RequestException guard for type safety
  • Ran ./vendor/bin/pint — includes style fixes across 5 files

Test plan

  • All 27 existing ChecksClient tests pass
  • Pint formatting applied

Closes #51

Summary by CodeRabbit

  • Bug Fixes

    • Improved error handling to prevent potential issues when processing request failures.
  • Refactor

    • Minor code syntax and formatting adjustments for consistency.

logError() accepted GuzzleException but called hasResponse() which only
exists on RequestException. Added instanceof guard for type safety.

Includes pint style fixes.

Closes #51
@coderabbitai
Copy link

coderabbitai bot commented Mar 5, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 26106def-ec93-4679-86d0-9222e867fa3a

📥 Commits

Reviewing files that changed from the base of the PR and between 5691a60 and 4a28bcc.

📒 Files selected for processing (5)
  • app/Commands/CertifyCommand.php
  • app/GitHub/ChecksClient.php
  • app/Services/PromptAssembler.php
  • app/Transformers/PhpStanPromptTransformer.php
  • app/Transformers/TestFailurePromptTransformer.php

📝 Walkthrough

Walkthrough

The pull request refactors syntax across multiple PHP files, primarily converting object instantiation from parentheses to parameter-less syntax. Additionally, it adds type safety to the ChecksClient.logError method by guarding against undefined method calls on unspecific exception types, and adjusts string concatenation formatting throughout.

Changes

Cohort / File(s) Summary
Syntax Normalization
app/Commands/CertifyCommand.php, app/Services/PromptAssembler.php
Standardized object instantiation syntax by removing unnecessary parentheses from class constructors. Removed unused CheckResult import.
Type Safety Fix
app/GitHub/ChecksClient.php
Added instanceof check for RequestException before calling hasResponse() to prevent type errors on GuzzleException interface.
String Concatenation Formatting
app/Services/PromptAssembler.php, app/Transformers/PhpStanPromptTransformer.php, app/Transformers/TestFailurePromptTransformer.php
Removed spaces around concatenation operators in multi-line string compositions for consistency.
Documentation Cleanup
app/Transformers/TestFailurePromptTransformer.php
Removed unused parameter docblock entries from private method extractTestCases.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A hop through syntax, clean and neat,
Type guards now standing strong and fleet,
No parentheses left behind,
Code formatted, refined.
Safety checks make hearts content! ✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The PR includes out-of-scope formatting changes across multiple files (CertifyCommand, PromptAssembler, PhpStanPromptTransformer, TestFailurePromptTransformer) unrelated to the type safety fix. Consider separating formatting/style fixes (applied by Pint) into a separate PR to keep the fix focused on issue #51's type safety requirements.
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the primary change: adding an instanceof check for RequestException in ChecksClient.logError(), which is the main fix in this PR.
Linked Issues check ✅ Passed The PR fully implements the required fix from issue #51: adding instanceof RequestException guard before hasResponse() calls, with all existing tests passing and PHPStan compliance.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/checks-client-type-safety

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link

github-actions bot commented Mar 5, 2026

📊 Coverage Report

Metric Coverage Threshold Status
Lines 68.2% 100%

Files Below Threshold

File Coverage Uncovered Lines
app/Commands/AnalyzeCommand.php 0% 22, 23, 25, 26, 28... (+51 more)
app/Commands/CheckCommand.php 0% 30, 31, 33, 34, 37... (+91 more)
app/Transformers/PhpStanPromptTransformer.php 3.6% 49, 51, 52, 53, 54... (+76 more)
app/Transformers/TestFailurePromptTransformer.php 11.7% 47, 68, 69, 71, 72... (+101 more)
app/Services/PromptAssembler.php 88.9% 55, 56, 57, 58, 120
app/GitHub/ChecksClient.php 89.1% 223, 224, 227, 228, 231... (+8 more)

🏆 Synapse Sentinel Gate

@github-actions
Copy link

github-actions bot commented Mar 5, 2026

🔧 Synapse Sentinel: 2 checks need attention

The following issues must be resolved before this PR can be merged:


All tests passed.---

Security Audit


Review the output and fix any issues.

Quick Reference:

  • PHPStan errors → Fix type mismatches first, then missing types
  • Test failures → Read the assertion message, trace expected vs actual
  • Style issues → Run composer format to auto-fix

🤖 Generated by Synapse Sentinel - View Run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix ChecksClient.logError() type safety (hasResponse on GuzzleException)

1 participant