chore(release): Prepare v2.2.0 - #41
Conversation
chore(release): Merge v2.1.0 back into develop
feat(audit): Harden gates, exceptions base, and follow-ups
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
PR Summary by Qodochore(release): Prepare v2.2.0 (security hardening, resilience, docs)
AI Description
Diagram
High-Level Assessment
Files changed (67)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41 +/- ##
============================================
+ Coverage 98.72% 98.82% +0.10%
- Complexity 940 949 +9
============================================
Files 71 70 -1
Lines 2198 2219 +21
============================================
+ Hits 2170 2193 +23
+ Misses 28 26 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review by Qodo
1. StateStoreInterface API break
|
| composer require jooservices/client | ||
| ``` | ||
|
|
||
| Runtime dependencies include Guzzle (`^7.10 || ^8.0`), `jooservices/exceptions` (`^0.5`), Monolog, and `mongodb/mongodb` (`^2.0`). |
There was a problem hiding this comment.
1. Readme lists guzzle ^7.10 📘 Rule violation ⚙ Maintainability
The updated README states runtime support for Guzzle ^7.10 || ^8.0, which contradicts the compliance requirement that 2.0+ documentation specify Guzzle ^8.0 as the dependency requirement. This can mislead integrators about the required major version for 2.0+ usage and support expectations.
Agent Prompt
## Issue description
The README’s runtime dependency line documents Guzzle as `^7.10 || ^8.0`, but the compliance checklist requires 2.0+ documentation to specify Guzzle `^8.0`.
## Issue Context
This is a public-facing documentation statement that sets user expectations for supported/required dependency versions.
## Fix Focus Areas
- README.md[32-32]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| * @return bool true if this caller may send the probe; false if another probe is in flight | ||
| */ | ||
| public function tryClaimProbe(): bool; |
There was a problem hiding this comment.
2. Statestoreinterface api break 🐞 Bug ⚙ Maintainability
StateStoreInterface now requires tryClaimProbe(), which is a breaking change for any downstream custom state-store implementations and can cause fatal interface-mismatch errors on upgrade. Docs explicitly describe custom StateStoreInterface implementations as a supported scalability path, but this release is targeting v2.2.0 (minor).
Agent Prompt
### Issue description
A new required method (`tryClaimProbe()`) was added to the public `StateStoreInterface`, which breaks any consumer implementation of that interface.
### Issue Context
The docs encourage implementing custom `StateStoreInterface` backends for horizontal scaling, so this interface is part of the supported extension surface.
### Fix Focus Areas
- Revert the breaking interface change by removing `tryClaimProbe()` from `StateStoreInterface` and introducing an *optional* interface (e.g., `HalfOpenProbeStoreInterface`) that defines `tryClaimProbe()`.
- Update built-in stores (`InMemoryStateStore`, `Psr16StateStore`) to implement the optional interface.
- Update `CircuitBreakerMiddleware` to call `tryClaimProbe()` only when the resolved store implements the optional interface (or via `method_exists`), otherwise proceed without single-flight gating.
### Fix Focus Areas (code pointers)
- src/Resilience/Contracts/StateStoreInterface.php[7-26]
- src/Middleware/CircuitBreakerMiddleware.php[28-40]
- src/Resilience/Storage/InMemoryStateStore.php[9-93]
- src/Resilience/Storage/Psr16StateStore.php[17-118]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| $state['probeInFlight'] = true; | ||
| $this->writeState($state); | ||
|
|
||
| return true; |
There was a problem hiding this comment.
3. Probe claim lacks expiry 🐞 Bug ☼ Reliability
Psr16StateStore::tryClaimProbe() persists a probeInFlight flag with no lease/expiry; if the worker that claimed the probe crashes or is killed before reporting success/failure, subsequent half-open attempts can be rejected indefinitely until cache eviction/manual reset. CircuitBreakerMiddleware rejects requests when tryClaimProbe() fails and does so before entering its try/catch, so callers don’t execute any automatic cleanup path.
Agent Prompt
### Issue description
`probeInFlight` can become a stale lock in shared caches: it is written with no lease/expiry and is only cleared on success/failure paths that won’t run if the claiming process terminates.
### Issue Context
`CircuitBreakerMiddleware` blocks half-open calls when `tryClaimProbe()` returns false, which makes a stale `probeInFlight=true` effectively wedge the circuit-breaker in practice.
### Fix Focus Areas
- Store a `probeClaimedAt` timestamp alongside `probeInFlight`.
- In `tryClaimProbe()`, if `probeInFlight` is true but `probeClaimedAt` is older than a configured/max lease duration, clear it and allow a new claim.
- Introduce a configurable probe lease duration (e.g., on `CircuitBreakerConfig`) or derive a safe default (e.g., based on request timeout/deadline) so the stale-probe window is bounded.
- Ensure both `Psr16StateStore` and `InMemoryStateStore` follow the same stale-claim semantics.
### Fix Focus Areas (code pointers)
- src/Middleware/CircuitBreakerMiddleware.php[28-67]
- src/Resilience/Storage/Psr16StateStore.php[58-157]
- src/Resilience/Storage/InMemoryStateStore.php[11-93]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| '''^coverage/''', # Local coverage output | ||
| '''^\.phpdoc/cache/''', # PHPDoc cache | ||
| '''\.txt$''', # Text files in tests | ||
| '''^tests/fixtures/''', # Test fixtures |
There was a problem hiding this comment.
4. Gitleaks ignores all .txt 🐞 Bug ⛨ Security
The gitleaks allowlist matches any path ending in .txt, excluding those files repo-wide from secret scanning and potentially letting real secrets slip through on push/scheduled/manual scans. The secret-scanning workflow runs gitleaks detect using this configuration (base-branch config on PRs, current branch config on pushes/schedule), so the reduced coverage affects non-PR events.
Agent Prompt
### Issue description
The allowlist pattern `\.txt$` is global, which exempts *all* `.txt` files from scanning.
### Issue Context
The CI workflow executes `gitleaks detect --config=.../gitleaks.toml`, so the allowlist is applied in CI (notably for push/schedule/workflow_dispatch, and PRs use the trusted base-branch config).
### Fix Focus Areas
- Replace `'''\.txt$'''` with a narrower path-scoped pattern (e.g., `^tests/fixtures/.*\.txt$` or the specific directories where `.txt` files are intentionally safe).
- If the intent is “text files in tests”, encode that intent in the regex/path allowlist instead of file extension alone.
### Fix Focus Areas (code pointers)
- .gitleaks.toml[14-23]
- .github/workflows/secret-scanning.yml[32-65]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Summary
developafter audit hardening landed (feat(audit): Harden gates, exceptions base, and follow-ups #40)Release flow next steps
masterv2.2.0frommaster(triggers release workflow + Packagist)masterback intodevelopTest plan
composer checkcomposer civ2.2.0