fix(ci): make the gates capable of both passing and failing (#9)

* fix(ci): make the gates capable of both passing and failing

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>

* fix(psalm): the 13 errors CI was actually failing on, and a wrong diagnosis 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>

Author: Snider

.github/workflows/ci.yml

@@ -17,9 +17,19 @@ jobs:
           coverage: none
       - name: Composer install
         run: composer install --prefer-dist --no-interaction --no-progress
+      # Without `|| true`. Every one of these swallowed its exit code, so this
+      # job reported success whatever happened — the only check in the repository
+      # that could not fail, sitting next to a Tests workflow that could not
+      # pass. Neither told anyone anything.
+      #
+      # Pest is scoped to the gate suites for the same reason tests.yml is: the
+      # Module suite is tracked debt, reported by its own non-gating job.
       - name: Pint
-        run: vendor/bin/pint --test || true
+        run: vendor/bin/pint --test
       - name: PHPStan
-        run: vendor/bin/phpstan analyse --no-progress || true
+        run: vendor/bin/phpstan analyse --no-progress
+      # No --no-interaction: that is a PHPUnit flag, and Pest rejects it with
+      # `Unknown option` and exit 2. The `|| true` had been hiding a broken
+      # invocation, not merely failing tests — this step has never run the suite.
       - name: Pest
-        run: vendor/bin/pest --no-coverage --no-interaction || true
+        run: vendor/bin/pest --testsuite=Feature,Unit --no-coverage

.github/workflows/tests.yml

@@ -11,7 +11,10 @@ jobs:
     runs-on: ubuntu-latest
 
     strategy:
-      fail-fast: true
+      # One leg failing should not hide the others. With fail-fast the first
+      # failure cancels the rest, so a PHP-version-specific break looks
+      # identical to a break on every version.
+      fail-fast: false
       matrix:
         php: [8.2, 8.3, 8.4]
         laravel: [12.*]
@@ -36,8 +39,14 @@ jobs:
           composer require "laravel/framework:${LARAVEL_VERSION}" --no-interaction --no-update
           composer update --prefer-dist --no-interaction --no-progress
 
+      # The gate suites only. A bare `phpunit` also runs the Module suite —
+      # src/**/*Test.php — which carries known failures and is treated as
+      # visible debt rather than a gate (see .gitlab-ci.yml's module-suite-debt
+      # job, and the module-debt job below). Including it here meant this
+      # workflow could never be green, so it stopped meaning anything: the same
+      # rot as a check that always passes, arrived at from the other side.
       - name: Execute tests with coverage
-        run: vendor/bin/phpunit --coverage-clover=coverage.xml
+        run: vendor/bin/phpunit --testsuite=Feature,Unit --coverage-clover=coverage.xml
 
       - name: Upload coverage to Codecov
         if: matrix.php == '8.3'
@@ -46,3 +55,40 @@ jobs:
           files: ./coverage.xml
           fail_ci_if_error: false
           verbose: true
+
+  # The Module suite — src/**/*Test.php — reported, never gating.
+  #
+  # It carries known failures, and the number is worth seeing rather than
+  # hiding: 448 at the time of writing, and they are not 448 bugs. Roughly 114
+  # are class-not-found for classes owned by sibling packages (Core\Tenant\Models,
+  # Core\Agentic\Services) — tests that cannot pass in this repository at all;
+  # about 30 are one real defect, StorageUrlResolver being handed a CdnUrlBuilder
+  # where it wants a BunnyStorageService; the rest are ordinary assertion and
+  # status-code failures.
+  #
+  # continue-on-error rather than `|| true`, and the difference matters: this
+  # way the step's own result is visible in the run, so the count can be driven
+  # down. A step that swallows its exit code teaches nobody anything.
+  #
+  # When it reaches zero, fold --testsuite=Module into the job above and delete
+  # this one. Mirrors .gitlab-ci.yml's module-suite-debt job.
+  module-debt:
+    runs-on: ubuntu-latest
+    name: Module suite (reported, not gating)
+    continue-on-error: true
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v4
+
+      - name: Setup PHP
+        uses: shivammathur/setup-php@v2
+        with:
+          php-version: '8.4'
+          extensions: dom, curl, libxml, mbstring, zip
+          coverage: none
+
+      - name: Install dependencies
+        run: composer install --prefer-dist --no-interaction --no-progress
+
+      - name: Execute the Module suite
+        run: vendor/bin/phpunit --testsuite=Module

src/Core/Cdn/Models/StorageOffload.php

@@ -150,7 +150,7 @@ public function scopeForDisk($query, string $disk)
     /**
      * Get human-readable file size.
      */
-    public function getFileSizeHumanAttribute(): string
+    protected function getFileSizeHumanAttribute(): string
     {
         $bytes = $this->file_size ?? 0;
         $units = ['B', 'KB', 'MB', 'GB', 'TB'];

src/Core/Config/Models/ConfigValue.php

@@ -64,7 +64,7 @@ class ConfigValue extends Model
     /**
      * Get the value attribute with automatic decryption for sensitive keys.
      */
-    public function getValueAttribute(mixed $value): mixed
+    protected function getValueAttribute(mixed $value): mixed
     {
         if ($value === null) {
             return null;
@@ -91,7 +91,7 @@ public function getValueAttribute(mixed $value): mixed
     /**
      * Set the value attribute with automatic encryption for sensitive keys.
      */
-    public function setValueAttribute(mixed $value): void
+    protected function setValueAttribute(mixed $value): void
     {
         // Check if the key is sensitive (need to load it if not already)
         $key = $this->relationLoaded('key')

src/Core/Media/Image/ImageOptimization.php

@@ -100,7 +100,7 @@ public function scopeForWorkspace($query, ?Model $workspace)
      *
      * Example: "45% saved (120KB → 66KB)"
      */
-    public function getSavingsHumanAttribute(): string
+    protected function getSavingsHumanAttribute(): string
     {
         // Format with appropriate unit
         $original = $this->formatBytes($this->original_size);
@@ -117,7 +117,7 @@ public function getSavingsHumanAttribute(): string
     /**
      * Get human-readable size saved.
      */
-    public function getSizeSavedHumanAttribute(): string
+    protected function getSizeSavedHumanAttribute(): string
     {
         $saved = $this->original_size - $this->optimized_size;
 

src/Core/Seo/Models/SeoScoreHistory.php

@@ -108,7 +108,7 @@ public function seoMetadata(): BelongsTo
     /**
      * Get the score color for UI display.
      */
-    public function getScoreColorAttribute(): string
+    protected function getScoreColorAttribute(): string
     {
         return match (true) {
             $this->score >= 80 => 'green',
@@ -120,7 +120,7 @@ public function getScoreColorAttribute(): string
     /**
      * Get the issue count.
      */
-    public function getIssueCountAttribute(): int
+    protected function getIssueCountAttribute(): int
     {
         return count($this->issues ?? []);
     }

src/Core/Seo/SeoMetadata.php

@@ -97,7 +97,7 @@ class SeoMetadata extends Model
      *
      * @return array<string, mixed>|null
      */
-    public function getSchemaMarkupAttribute(): ?array
+    protected function getSchemaMarkupAttribute(): ?array
     {
         if ($this->schemaMarkupLoaded) {
             return $this->parsedSchemaMarkup;
@@ -132,7 +132,7 @@ public function getSchemaMarkupAttribute(): ?array
      *
      * @param  array<string, mixed>|string|null  $value
      */
-    public function setSchemaMarkupAttribute(array|string|null $value): void
+    protected function setSchemaMarkupAttribute(array|string|null $value): void
     {
         // Reset the lazy loading cache
         $this->parsedSchemaMarkup = null;
@@ -183,7 +183,7 @@ public function seoable(): MorphTo
      *
      * Uses JSON_HEX_TAG to prevent XSS via </script> in content.
      */
-    public function getJsonLdAttribute(): string
+    protected function getJsonLdAttribute(): string
     {
         if (empty($this->schema_markup)) {
             return '';
@@ -197,7 +197,7 @@ public function getJsonLdAttribute(): string
     /**
      * Generate all meta tags as HTML.
      */
-    public function getMetaTagsAttribute(): string
+    protected function getMetaTagsAttribute(): string
     {
         $tags = [];
 
@@ -241,7 +241,7 @@ public function getMetaTagsAttribute(): string
     /**
      * Get SEO score colour for UI display.
      */
-    public function getScoreColorAttribute(): string
+    protected function getScoreColorAttribute(): string
     {
         if ($this->seo_score === null) {
             return 'zinc';
@@ -265,7 +265,7 @@ public function hasIssues(): bool
     /**
      * Get the count of issues.
      */
-    public function getIssueCountAttribute(): int
+    protected function getIssueCountAttribute(): int
     {
         return count($this->seo_issues ?? []);
     }