fix(cdn): stop the lower layer implementing the upper layer's interface - #10
Merged
Conversation
BunnyStorageService imported two types from dappcore/service — the
HealthCheckable interface it implemented, and HealthCheckResult, which its
healthCheck() body constructed. Neither exists in this package. php requires
laravel/framework, pennant and livewire, and nothing else.
So this package had production source that could not be loaded in this package.
It worked only because every real consumer installs dappcore/service alongside
it, which is why host.uk.com is fine and nobody has felt this.
Adding the dependency was not available: dappcore/service requires
dappcore/php: *, so it would close a loop. The direction composer already
declares is service-depends-on-php, and php reaching up into Core\Service\* was
against that grain. So the health report moves to the layer that is entitled to
build it.
WHAT MOVES, and it is a relocation rather than a removal: healthCheck() goes to a
decorator in dappcore/service, which composes this service and implements
HealthCheckable there. That repo has no session yet and the work is held; until
it lands, health reporting of this service is absent with a named destination.
WHAT STAYS: the zone probing, which never depended on Core\Service at all.
checkZoneHealth() keeps returning its {success, latency_ms, error} array and
isReachable() keeps answering the boolean question. checkZoneHealth() is now
public, because the decorator composes rather than extends — a protected probe
would have made the relocation cost exactly the latency and error detail that
makes a health report worth reading.
Safe to carry with no consumer noticing, checked rather than assumed: nothing in
host.uk.com's app or vendor tree calls healthCheck() on this service, and there
is no `instanceof HealthCheckable` anywhere in it. The only references were this
file and documentation inside dappcore/service.
ALSO HERE, and it is why the numbers move: CdnIntegrationTest built
StorageUrlResolver with its arguments wrong —
__construct(protected BunnyStorageService $bunnyStorage, ?CdnUrlBuilder $urlBuilder = null)
new StorageUrlResolver($this->urlBuilder)
passing the optional second argument into the required first slot and omitting
the required one, so all 30 cases died in setUp on one TypeError. Thirty
failures, one line. Fixed by resolving BunnyStorageService from the container,
which is how Core\Cdn\Boot wires it in production.
That fix was pointless on its own — corrected, the tests failed one line later on
the missing interface — which is why both halves are in one change.
./vendor/bin/pest --testsuite=Feature,Unit 268 passed, 0 failed — unchanged
./vendor/bin/pest --testsuite=Module 448 -> 426 failed, 308 -> 330 passed
CdnIntegrationTest alone 30 failed / 0 assertions
-> 8 failed / 22 passed / 52 assertions
vendor/bin/pint --test pass
vendor/bin/phpstan analyse no errors
The 8 that remain are real assertions against a constructible object rather than
a constructor error: a missing config_resolved table in the test database, a
singular/plural category path, a ?id= versus ?v= query parameter, and four
straightforward expectation mismatches. Each is now a visible disagreement about
behaviour instead of 30 identical crashes hiding one line.
Co-Authored-By: Virgil <virgil@lethean.io>
📝 WalkthroughWalkthroughBunnyStorageService now provides public per-zone health probes instead of aggregate health reporting. Core Psalm suppressions were removed. The CDN integration test now constructs StorageUrlResolver with the required services. ChangesCDN health probe refactor
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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 |
…w removed CI's Psalm went red on this branch with UnusedIssueHandlerSuppression: Suppressed issue type "MissingDependency" for /src/Core/Cdn/ was not thrown which is the fix reporting itself. The suppression's own comment said what it was for — "Pending Core\Service module — referenced by Cdn BunnyStorageService" — so it was config-level acknowledgement of the upward dependency this branch removes, pending indefinitely. With the dependency gone there is nothing left to suppress, and Psalm treats a suppression that never fires as an error in its own right. Read from the CI job log rather than a local run, deliberately: local and CI Psalm disagree in this repository — the Laravel plugin resolves differently — and I got that wrong earlier today by trusting the copy in front of me. CI is the authority for what CI thinks is unused, and here CI is the one calling it unused. Co-Authored-By: Virgil <virgil@lethean.io>
The previous commit's regex matched backwards from the wrong comment. Both blocks
I meant to touch begin "Pending Core\Service module", and the pattern anchored on
the first one — inside the UndefinedClass handler — then ran to the closing tag of
the second, taking two unrelated referencedClass entries and a pair of closing
tags with it. psalm.xml became invalid XML, and I pushed it.
Restored and redone against the tags themselves rather than a shared comment
prefix. What goes is exactly:
- referencedClass Core\Service\Contracts\HealthCheckable
- referencedClass Core\Service\HealthCheckResult
- the MissingDependency handler suppressing src/Core/Cdn
all three of which existed for the upward dependency this branch removes. What
stays is what I should never have touched: the Core\Front\Client\Boot and
Illuminate\Foundation\Auth\User entries, which have nothing to do with it.
Fixed forward rather than rewritten — the broken commit is on the remote and
stays there, with this on top.
python3 -c "xml.etree.ElementTree.parse('psalm.xml')" valid
./vendor/bin/pest --testsuite=Feature,Unit 268 passed, 0 failed
vendor/bin/pint --test pass
Co-Authored-By: Virgil <virgil@lethean.io>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/Core/Cdn/Services/BunnyStorageService.php`:
- Around line 567-570: Update checkZoneHealth to validate that zone is exactly
public or private before selecting the storage client; reject unsupported values
such as misspellings instead of falling through to the public client, while
preserving the existing client-selection behavior for valid zones.
🪄 Autofix
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: 9237abb1-6365-4a25-b468-01b9655aa886
📒 Files selected for processing (3)
psalm.xmlsrc/Core/Cdn/Services/BunnyStorageService.phpsrc/Core/Tests/Feature/CdnIntegrationTest.php
💤 Files with no reviewable changes (1)
- psalm.xml
Comment on lines
567
to
+570
| * @param string $zone 'public' or 'private' | ||
| * @return array{success: bool, latency_ms: float, error?: string} | ||
| */ | ||
| protected function checkZoneHealth(string $zone): array | ||
| public function checkZoneHealth(string $zone): array |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject invalid storage-zone names.
Line 575 selects the public client for every value except private. A caller that passes publci can receive a successful public-zone probe and report the wrong zone as healthy. Reject unsupported values before selecting a client.
Proposed fix
public function checkZoneHealth(string $zone): array
{
+ if (! in_array($zone, ['public', 'private'], true)) {
+ return [
+ 'success' => false,
+ 'latency_ms' => 0.0,
+ 'error' => 'Invalid storage zone',
+ ];
+ }
+
$startTime = microtime(true);🤖 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 `@src/Core/Cdn/Services/BunnyStorageService.php` around lines 567 - 570, Update
checkZoneHealth to validate that zone is exactly public or private before
selecting the storage client; reject unsupported values such as misspellings
instead of falling through to the public client, while preserving the existing
client-selection behavior for valid zones.
Snider
added a commit
that referenced
this pull request
Aug 8, 2026
The eight that survived #10 were real disagreements rather than one crash worn eight times, so each got decided on its own evidence. Six were the test being wrong, one was the harness, and one was the code. THE CODE ONE. CdnUrlBuilder::buildSignedUrlBase() is typed to return string and passed config('cdn.bunny.private.pull_zone') straight into str_starts_with(). Unconfigured, that is null, and str_starts_with(null, …) is a TypeError on PHP 8 — so signing with a token set and no pull zone did not fail politely, it threw from inside the URL builder. Only reachable once a token exists, which is why no test had ever got there: the test that would have was setting the wrong config key and taking the empty-token early return instead. Cast and defaulted. Also code, and a consistency defect rather than a crash: withVersion() emitted `id=` while Core\Helpers\Cdn::versioned() — the helper applications actually call from templates — emits `v=`. The same package cache-busted the same assets under two parameter names depending on which door you came in by. Nothing required `id`: no CDN documentation here mentions it, every docblock names the purpose and not the parameter, and its only appearance in the history is an unrelated Rector pass. Now `v`, consistent with the helper and with the test. One deploy's worth of cache misses, which is what a version parameter is for. THE HARNESS ONE. cdn.paths was absent, because the base TestCase registers only LifecycleEventProvider and nothing merges the package's cdn config. pathPrefix() falls back to the raw category name, so 'avatar' stayed 'avatar' where the config maps it to 'avatars' — the test was right all along. Set that one key. Deliberately one key and not the file: merging the whole config wholesale overrode the test disks configured below it and took the file from 6 failures to 9. I did that first and reverted it. Two more needed the Config package's migrations. BunnyStorageService reads its zone credentials through ConfigService, which is a database query against config_resolved, so isConfigured() died on "no such table" rather than on anything it was testing. THE STALE TESTS, four of them, each pinning something the code has never done: signed() set cdn.signing_key and cdn.token_lifetime; the implementation reads cdn.bunny.private.token, and neither of those keys is read anywhere urls() asserted cdn_url / origin_url; the contract is cdn / origin, which is what allUrls() documents too copy() passed the destination path as the source bucket and two disk names after it — every argument after the first in the wrong parameter, written against a signature this method has not had size() used UploadedFile::fake()->create('test.txt', 50), which reports getSize() 51200 and writes zero real bytes. Measured, not guessed: create_getSize 51200 / create_realBytes 0, against image_getSize 695 / image_realBytes 695. So the stored file was empty and size() answered 0 correctly — the assertion was measuring the fixture. Now createWithContent with known content, asserting the exact size rather than "more than nothing". CdnIntegrationTest 8 failed / 22 passed -> 30 passed, 0 failed Module suite 426 -> 418 failed, 330 -> 338 passed gate suites 268 passed, 0 failed — unchanged pint, phpstan clean
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.
BunnyStorageServiceimported two types fromdappcore/service— theHealthCheckableinterface it implemented, andHealthCheckResult, which itshealthCheck()body constructed. Neither exists in this package. php requires laravel/framework, pennant and livewire, and nothing else.So this package had production source that could not be loaded in this package. It worked only because every real consumer installs
dappcore/servicealongside it — which is why host.uk.com is fine and nobody has felt this.Why not just add the dependency
dappcore/servicerequiresdappcore/php: *. Adding it here closes a loop.The direction composer already declares is service depends on php, so php reaching up into
Core\Service\*was against the grain the ecosystem itself defines. The health report moves to the layer entitled to build it.This is a relocation, not a removal
healthCheck()goes to a decorator indappcore/service, which composes this service and implementsHealthCheckablethere. That repo has no session yet and the work is held — until it lands, health reporting of this service is absent with a named destination.What stays is the part that never depended on
Core\Serviceat all:checkZoneHealth(string $zone){success, latency_ms, error?}isReachable(string $zone = 'any')boolcheckZoneHealth()is now public, because the decorator composes rather than extends. A protected probe would have made the relocation cost exactly the latency and error detail that makes a health report worth reading.Safe to carry with no consumer noticing — checked, not assumed
app/orvendor/callshealthCheck()on this serviceinstanceof HealthCheckableanywhere in that treedappcore/serviceAlso here, and it is why the numbers move
CdnIntegrationTestbuiltStorageUrlResolverwith its arguments wrong:It passed the optional second argument into the required first slot and omitted the required one, so all 30 cases died in setUp on one TypeError. Thirty failures, one line. Fixed by resolving
BunnyStorageServicefrom the container, which is howCore\Cdn\Bootwires it in production.That fix was pointless alone — corrected, the tests failed one line later on the missing interface — which is why both halves are in one change.
Receipts
The 8 that remain
Real assertions against a constructible object, rather than one constructor error wearing thirty hats:
config_resolvedtable in the test databaseavatar/where the test expectsavatars/?id=v123where the test expectsv=v123Each is now a visible disagreement about behaviour. Not fixed here — that is the next bucket, and each needs deciding on its merits rather than sweeping.
🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io
Summary by CodeRabbit
Bug Fixes
Tests