fix(ci): make the gates capable of both passing and failing - #9
Conversation
main was red, and had two opposite problems sitting next to each other. PSALM. psalm.xml suppressed NoEnvOutsideConfig for src/, and Psalm reported UnusedIssueHandlerSuppression because the issue is never raised — src/'s env() calls live in files literally named config.php, which the Laravel plugin already treats as config. So the suppression protected nothing and its own unusedness was the only error Psalm found. Removing it takes Psalm from exit 2 to "No errors found!" with the 1486 info-level issues unchanged. That was the whole Psalm half of the red. TESTS. tests.yml ran a bare `phpunit`, which includes the Module suite — src/**/*Test.php, 448 known failures, treated everywhere else as visible debt rather than a gate. So the workflow could never pass. It now runs --testsuite=Feature,Unit, which is exit 0 with 269 tests, and the Module suite gets its own job with continue-on-error so the number stays on screen and can be driven down. That mirrors .gitlab-ci.yml, which had already made exactly this split with the reasoning written out. fail-fast is off on the matrix. With it on, the first failing leg cancelled the others, so a break specific to one PHP version was indistinguishable from a break on all three. AND THE OTHER DIRECTION. ci.yml ran pint, phpstan and pest each with `|| true`. It was the one check in the repository that could not fail, next to a Tests workflow that could not pass. Removed, all three. Doing that surfaced something the `|| true` had been hiding: the Pest step passed --no-interaction, which is a PHPUnit flag. Pest answers `Unknown option "--no-interaction"` and exits 2. That step has never run the suite once — not "ran and was ignored", never ran. A check that swallows its exit code cannot tell you it is not the check you think it is. Verified locally, every gate: pint --test exit 0 phpstan analyse exit 0 psalm --show-info=false exit 0 (was 2) pest --testsuite=Feature,Unit exit 0 (was 2, on the bad flag) phpunit --testsuite=Feature,Unit exit 0 (was 1, via the debt suite) phpunit --testsuite=Module exit 1 — expected, now non-gating The 448 is not 448 bugs, and the debt job says so: ~114 are class-not-found for sibling packages' classes and cannot pass here at all, ~30 are one real defect (StorageUrlResolver handed a CdnUrlBuilder where it wants a BunnyStorageService), the rest ordinary assertion failures. Named there so the next person starts from the breakdown rather than the total. Left alone deliberately: ci.yml now overlaps static-analysis.yml on pint and phpstan, and tests.yml on the suite. Duplicated gates are harmless; lying ones are not. Consolidating them is a tidy-up, not this change. Co-Authored-By: Virgil <virgil@lethean.io>
|
Warning Review limit reached
Next review available in: 27 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
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 |
…gnosis undone I diagnosed Psalm from local output and got it wrong twice. Correcting both here rather than quietly. WHAT I DID WRONG. Locally Psalm reported one error — UnusedIssueHandlerSuppression for the NoEnvOutsideConfig suppression in psalm.xml — so I removed the suppression and saw exit 0. In CI that removal produced 183 NoEnvOutsideConfig errors in src/Core/config.php. The suppression was doing real work; it only looks unused locally, because the Laravel plugin resolves the config directory differently there. Reverted. WHAT CI WAS ACTUALLY FAILING ON, which I should have read first: 13 PublicModelAccessor errors, none of which appear locally at all. 6 src/Core/Seo/SeoMetadata.php 2 src/Core/Seo/Models/SeoScoreHistory.php 2 src/Core/Media/Image/ImageOptimization.php 2 src/Core/Config/Models/ConfigValue.php 1 src/Core/Cdn/Models/StorageOffload.php Eloquent accessors and mutators declared public. Laravel reaches them through getAttribute()/__get, so protected is what they should be, and public invites somebody to call getValueAttribute() directly and depend on it. Made protected, all 13. Checked first that nothing calls any of them directly — not in this package, and not in host.uk.com, which is the only consumer — so the visibility change costs nobody anything. All five classes extend Model, so the framework path is the only path. The lesson is the same one I flagged in another repo this morning and then walked into: local and CI disagreed, and I trusted the copy in front of me instead of opening the job log. Local Psalm reports 1 error / 1486 issues, CI reports 13 / 1155. On a tool with a Laravel plugin that bootstraps differently per environment, the CI log is the only source that speaks for CI. vendor/bin/pint --test pass vendor/bin/phpstan analyse no errors ./vendor/bin/pest --testsuite=Feature,Unit 268 passed, 0 failed — unchanged ./vendor/bin/pest --testsuite=Module 448 failed / 308 passed — unchanged Psalm itself cannot be verified from here, for the reason above. CI is the check. Co-Authored-By: Virgil <virgil@lethean.io>
CI verdict, and a correctionEvery gate is green. The one red is I had the Psalm diagnosis wrong, twiceThe description above says the Psalm half was an unused suppression in Removing that suppression produced 183 What CI was actually failing on — which I should have read before touching anything — was 13
Eloquent accessors declared Local Psalm reports 1 error / 1486 issues; CI reports 13 / 1155. On a tool whose Laravel plugin bootstraps differently per environment, the CI log is the only thing that speaks for CI — which is the same lesson I flagged in another repo this morning and then walked straight into. |
|
@coderabbitai review |
|
|
Correcting my own comment above: the file column of that table came out empty. I wrote the comment through a double-quoted shell string and zsh executed the backticked paths as commands instead of passing them through — the same mistake I made on an earlier PR today. The table should read:
13 total, all now |
1 participant
mainwas red, with two opposite problems sitting next to each other: a check that could not fail, beside a check that could not pass.Psalm — the suppression whose only effect was to be unused
psalm.xmlsuppressedNoEnvOutsideConfigforsrc/. Psalm reportedUnusedIssueHandlerSuppression, because the issue is never raised:src/'s 171env()calls live in files literally namedconfig.php, which the Laravel plugin already treats as config.So the suppression protected nothing, and its own unusedness was the only error Psalm found.
Tests — a workflow that could never be green
tests.ymlran a barephpunit, which includes theModulesuite (src/**/*Test.php) — 448 known failures, treated everywhere else as visible debt rather than a gate.phpunit(bare, what it ran)phpunit --testsuite=Feature,UnitIt now runs the gate suites, and the Module suite gets its own job with
continue-on-error: trueso the number stays visible and can be driven down. This mirrors.gitlab-ci.yml, which had already made exactly this split with its reasoning written out — the two CIs disagreed, and GitLab was right.fail-fastis now off on the matrix: with it on, the first failing leg cancelled the others, so a break specific to one PHP version looked identical to a break on all three. That is why the reported red leg kept moving between 8.3 and 8.4.And the other direction
ci.ymlran pint, phpstan and pest each with|| true. It was the one check in the repository that could not fail. Removed, all three.Doing that surfaced what the
|| truehad been hiding:--no-interactionis a PHPUnit flag; Pest rejects it. That step has never run the suite — not "ran and was ignored", never ran. A check that swallows its exit code cannot tell you it is not the check you think it is.Verified locally, every gate
The 448 is not 448 bugs
The debt job says so in its comment, so the next person starts from the breakdown rather than the total:
Core\Tenant\Models,Core\Agentic\Services) — cannot pass in this repository at allStorageUrlResolverhanded aCdnUrlBuilderwhere it wants aBunnyStorageServiceLeft alone deliberately
ci.ymlnow overlapsstatic-analysis.ymlon pint/phpstan andtests.ymlon the suite. Duplicated gates are harmless; lying ones are not. Consolidating them is a tidy-up, not this change.🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io