feat: change RetryClassifier default to opt-in, update tests - #2
Merged
Conversation
#1 - RetryClassifier: default transientClasses changed from [\RuntimeException::class] to [] to avoid retrying Flysystem's UnableToXxx permanent exceptions which also extend RuntimeException - RetryClassifierTest: replace old default-retries-RuntimeException tests with testDefaultClassifierRetiesNothing() - RetryAdapterTest: add runtimeAdapter() helper; tests that expect RuntimeException retries now explicitly configure the classifier - ROADMAP: mark phases 0-4 complete, document known limitations
…unity files #1 - README: correct RetryClassifier default — nothing retried by default (opt-in) - Add CONTRIBUTING.md, CODE_OF_CONDUCT.md, SECURITY.md, HISTORY.md - Add .github/workflows/ci.yml (PHP 8.2–8.4 matrix, PHPUnit + PHPStan) - Add .github/dependabot.yml, pull_request_template.md, ISSUE_TEMPLATE/
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughWalkthroughRetryClassifier constructor is changed to assign the provided transient classes array directly, removing the implicit ChangesRetryClassifier opt-in behavior change
GitHub and project infrastructure
Estimated code review effort[2] (Simple) | ~10 minutes Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Tip: You can configure your own custom pre-merge checks in the settings. Finishing TouchesGenerate unit tests (beta)
Simplify code
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/RetryAdapterTest.php (1)
63-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
maxAttemptstests no longer exercise retry behaviorAfter the default classifier switched to opt-in, both tests now use a non-retryable
RuntimeExceptionpath, so they no longer validate exhaustion/max-attempt semantics as named. UseruntimeAdapter()(or pass an explicit classifier) in these tests to keep their intent intact.Proposed fix
@@ - self::adapter($inner, maxAttempts: 2)->write('a.txt', 'hi', new Config()); + self::runtimeAdapter($inner, maxAttempts: 2)->write('a.txt', 'hi', new Config()); @@ - self::adapter($inner, maxAttempts: 1)->write('a.txt', '', new Config()); + self::runtimeAdapter($inner, maxAttempts: 1)->write('a.txt', '', new Config());Also applies to: 166-177
Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/RetryAdapterTest.php` around lines 63 - 71, The testExhaustsMaxAttemptsAndRethrows test no longer validates retry behavior because RuntimeException is no longer retryable by default after the classifier switched to opt-in mode. To restore the intended test behavior, replace the self::adapter call with runtimeAdapter() or pass an explicit classifier parameter that configures RuntimeException as a retryable exception. Apply the same fix to the other affected test mentioned (around lines 166-177) to ensure both tests properly exercise the max-attempt exhaustion and retry semantics they are designed to validate..github/workflows/ci.yml (1)
1-34:⚠️ Potential issue | 🟠 MajorAdd explicit least-privilege token permissions to the workflow.
No
permissions:block is configured, allowing the workflow to run with default GitHub Actions token permissions that exceed requirements for checkout, install, test, and static analysis operations.Proposed fix
name: CI on: push: branches: [main] pull_request: branches: [main] +permissions: + contents: read + jobs: test: runs-on: ubuntu-latestPrompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 1 - 34, Add explicit least-privilege token permissions to the workflow by inserting a `permissions:` block at the top level (after the `on:` section and before the `jobs:` section). Set `contents: read` as the only required permission, since the workflow only performs checkout operations and subsequent local analysis tasks (composer install, phpunit, and phpstan) that do not require additional token permissions. This ensures the workflow follows the principle of least privilege by restricting the GitHub Actions token to only the minimal access needed.Source: Linters/SAST tools
Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 16-17: Replace the movable version tags in the GitHub Actions
workflow with immutable full 40-character commit SHA hashes. For the
`actions/checkout@v4` action on line 16 and `shivammathur/setup-php@v2` action
on line 17 (and any other instances on lines 27-28), replace the `@v4` and `@v2`
tags with their corresponding full commit SHAs to ensure the exact version is
always used and prevent supply-chain risks from tag retagging or action
modifications.
- Line 16: The workflow has two `actions/checkout@v4` steps that persist
credentials by default, which unnecessarily expands the attack surface for a
read-only workflow that only performs checks without pushing code. Add the
`persist-credentials: false` parameter to both `actions/checkout@v4` calls at
lines 16 and 27 to disable credential persistence and improve security posture.
In `@CONTRIBUTING.md`:
- Around line 4-9: The CONTRIBUTING.md file references running composer test on
line 4, but this script is not defined in the composer.json file. To fix this,
either add a test script entry to the scripts section of composer.json that runs
the explicit commands vendor/bin/phpunit and vendor/bin/phpstan analyse src
tests --level=8, or update the CONTRIBUTING.md line 4 to replace the composer
test reference with these explicit commands so contributors understand exactly
what commands to run.
In `@ROADMAP.md`:
- Around line 14-16: Update AGENTS.md to reflect the new opt-in default
transient class list behavior. Find all references in AGENTS.md that mention the
default transient class list being `[\RuntimeException::class]` and update them
to document that the new default is `[]` (empty, opt-in). Ensure any examples or
explanations about RetryClassifier initialization in AGENTS.md clarify that
users must now explicitly pass the desired transient exception classes as
constructor arguments, consistent with how ROADMAP.md documents this change.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 1-34: Add explicit least-privilege token permissions to the
workflow by inserting a `permissions:` block at the top level (after the `on:`
section and before the `jobs:` section). Set `contents: read` as the only
required permission, since the workflow only performs checkout operations and
subsequent local analysis tasks (composer install, phpunit, and phpstan) that do
not require additional token permissions. This ensures the workflow follows the
principle of least privilege by restricting the GitHub Actions token to only the
minimal access needed.
In `@tests/RetryAdapterTest.php`:
- Around line 63-71: The testExhaustsMaxAttemptsAndRethrows test no longer
validates retry behavior because RuntimeException is no longer retryable by
default after the classifier switched to opt-in mode. To restore the intended
test behavior, replace the self::adapter call with runtimeAdapter() or pass an
explicit classifier parameter that configures RuntimeException as a retryable
exception. Apply the same fix to the other affected test mentioned (around lines
166-177) to ensure both tests properly exercise the max-attempt exhaustion and
retry semantics they are designed to validate.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f34f6c0e-1cef-415e-ad2e-eb516108b481
Files selected for processing (14)
.github/ISSUE_TEMPLATE/bug.yml.github/ISSUE_TEMPLATE/feature.yml.github/dependabot.yml.github/pull_request_template.md.github/workflows/ci.ymlCODE_OF_CONDUCT.mdCONTRIBUTING.mdHISTORY.mdREADME.mdROADMAP.mdSECURITY.mdsrc/RetryClassifier.phptests/RetryAdapterTest.phptests/RetryClassifierTest.php
There was a problem hiding this comment.
2 issues found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="CONTRIBUTING.md">
<violation number="1" location="CONTRIBUTING.md:8">
P3: This instruction points contributors to `composer test`, but the documented build/test flow here uses explicit PHPUnit and PHPStan commands. Align this step with executable commands (or add the script) to avoid a broken onboarding path.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ermissions, composer scripts #1 - testExhaustsMaxAttemptsAndRethrows: use runtimeAdapter() so RuntimeException is actually retried and exhaustion is tested - testMaxAttemptsOfOneNeverRetries: same fix - .github/workflows/ci.yml: add permissions: contents: read, persist-credentials: false on both checkout steps - composer.json: add scripts.test and scripts.phpstan so `composer test` works as documented - AGENTS.md: correct RetryClassifier default description to [] (opt-in)
There was a problem hiding this comment.
Actionable comments posted: 1
Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/RetryPolicy.php`:
- Line 20: The code on line 20 in the RetryPolicy.php file uses
Random\Randomizer::nextFloat() which requires PHP 8.3+, but the project requires
PHP 8.2+ compatibility. To fix this, replace the nextFloat() call with an
equivalent implementation using mt_rand() that generates a random float value
compatible with PHP 8.2, maintaining the same jitter calculation logic.
Alternatively, if dropping PHP 8.2 support is acceptable, update the PHP version
requirement to 8.3+ in the coding guidelines documentation and ROADMAP.md.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
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
RetryClassifierdefault$transientClasseschanged from[\RuntimeException::class]to[]— consumers must now opt in to retry specific exception classesUnableToXxxexceptions (which also extendRuntimeException)testDefaultClassifierRetiesNothing()replaces old default-retry tests;RetryAdapterTestgainsruntimeAdapter()helper for tests that explicitly wantRuntimeExceptionretriesTest plan
php vendor/bin/phpunit— 20 tests, all greenCloses #1
Summary by CodeRabbit