Skip to content

ci: gate every PR on tests, lint, and static analysis - #147

Merged
markshust merged 5 commits into
developfrom
feature/ci-gate-and-phpstan-fixes
Jul 26, 2026
Merged

ci: gate every PR on tests, lint, and static analysis#147
markshust merged 5 commits into
developfrom
feature/ci-gate-and-phpstan-fixes

Conversation

@markshust

@markshust markshust commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

Two problems, one root cause. packages/core carried 12 PHPStan errors and php-cs-fixer had drifted on 129 files, because this repo had no PR CI of any kind — the only workflows were auto-label, deploy-docs, readme-package-check, split, and sync-issue-templates. "develop is green" rested entirely on whoever last remembered to run the tools by hand, and pr-review-process.md's checklist never even listed PHPStan.

This fixes everything that was red and adds the gate that stops it recurring.

The 12 PHPStan errors were never failing tests

No test could have caught any of them — PHP tolerates every one at runtime. Extra arguments to a userland method are silently discarded; initInterception() exists on the trait so the call resolves; docblocks are invisible to the interpreter. The code worked. Its declared types lied about it.

Fix What was wrong
PluginInterceptor Passed a 3rd argument to generateInterfaceWrapper() / generateConcreteSubclass(), which take 2. Dead arguments removed.
PluginInterceptedInterface Declared only getPluginTarget(), yet PluginInterceptor calls initInterception() through that type. Declared on the interface where the contract belongs.
ModuleManifest::$singletons Annotated array<string, …> while list-style entries use int keys — exactly what BindingRegistry::registerModule() branches on via is_int(). The annotation made PHPStan call a live branch impossible. Behaviour unchanged, already covered by BindingRegistryTest.
Command, CommandDefinition $aliases missing list<string>
InterceptorClassGenerator collectInterfaceMethods() missing its ReflectionClass generic
Input array_values() on a value that is already a list

Provenance: six arrived via PR #15 (the PluginProxy → generated-interceptor rewrite, 2026-04-05), the rest from the January CLI batches and a February routing fix. All went through normal review.

One error is ignored rather than fixed: trait.unused on PluginInterception. Every runtime consumer is an eval-generated class, so no source-visible use exists for PHPStan to find. It is a limitation of analysing generated code, not a defect — ignored narrowly by identifier and path, with the reason recorded in phpstan.neon. This is the repo's first PHPStan ignore, so it deserves a look. The alternative is replacing the trait with a collaborator object, which the trait's own docblock already flags as desirable ("a deliberate exception to the No Traits code standard") — worth its own PR, not this one.

The gate

Three jobs on every pull request, with no paths: filter so nothing can route around it:

Job Runs
Tests composer test
Lint phpcs + php-cs-fixer --dry-run
Static analysis composer phpstan

Also adds composer phpstan and composer ci (the whole gate in one command), and puts PHPStan into the review checklist that omitted it.

integration-destructive is deliberately not in the gate: it deletes vendor/ and composer.lock, runs composer update, and re-runs the entire suite in a subprocess, so it measures install integrity rather than behaviour and is order-dependent under --parallel. It now runs nightly instead of never, and bin/release.sh still runs it before any tag is cut.

The 142-file style commit

dd46d89 is mechanical only — every hunk is composer cs:fix output, no behaviour or test expectations touched. It is isolated in its own commit so the real changes stay readable.

Worth knowing: the two formatters disagreed on 12 files — phpcbf's multi-line reformatting produced output php-cs-fixer wanted to revert. cs:fix runs php-cs-fixer first and phpcbf second, so phpcbf wins and one pass converges. That ordering is now load-bearing for the Lint job rather than a convenience.

Test plan

  • composer ci exits 0 — verified as a real exit code, not through a pipe
  • Each command independently: phpcs 0, php-cs-fixer 0, phpstan 0, pest 0
  • composer test — 6884 passed (up from 6871)
  • tests/CiWorkflowTest.php — 10 tests asserting the gate has no paths: escape hatch, pins PHP 8.5 in every job, pins actions to a major version, keeps the destructive group on a schedule, and excludes the unparseable fixtures
  • packages/core/tests/Unit/Plugin/PluginInterceptedInterfaceTest.php — pins the initInterception() contract against the trait's signature

Note on the fixture exclusions

php-cs-fixer lints before fixing and exits 4 when any file will not parse, even with zero files to change. Three fixtures are invalid PHP on purpose — the config and codeindexer packages test what happens when the tooling meets a broken file — so the Lint job would have failed on arrival, permanently, on a condition nobody could fix without deleting the test cases. Both linters now exclude them.

What the gate caught on its first run

Three tests passed locally and failed in CI — they had only ever run on one machine:

  • ratelimiter/RenameReferencesTest and skeleton/PackageStructureTest hardcoded /opt/homebrew/Cellar/php/8.5.1_2/bin/php, with a /opt/homebrew/bin/composer fallback. pr-review-process.md forbids exactly this — "no hardcoded paths or environment-specific values, use PHP_BINARY" — but nothing enforced it. Now PHP_BINARY, and the composer lookup skips instead of guessing.
  • devserver/PidFileTest built its process group via a string proc_open command, which runs through /bin/sh, so the reported pid is the shell's rather than PHP's. macOS sh execs in place and the pids coincide; Ubuntu's dash does not, so posix_setsid() created the group under a pid the assertions never inspected. Switched to the array form, which execs directly, plus a pcntl/posix guard.

That last one is a genuine portability bug in the test, not a CI quirk — it would have misled anyone debugging devserver on Linux.

Follow-ups this surfaced

Not fixed here, deliberately:

  • PHPStan covers 1 of 70 packages. phpstan.neon analyses only packages/core/src. Across packages/*/src at the same level there are 262 errors.
  • Core tests are excluded from PHPStan and carry 212 errors at level 6.

markshust and others added 4 commits July 26, 2026 11:19
None of these were failing tests, and no test could have caught them — PHP
tolerates every one at runtime. Extra arguments to a userland method are silently
discarded, initInterception() exists on the trait so the call resolves, and
docblocks are invisible to the interpreter. Only static analysis sees them, and
nothing ran it.

Six came in through PR #15 (the PluginProxy → generated-interceptor rewrite) on
2026-04-05; the rest date to the January CLI batches and a February routing fix.
They survived review because the PR checklist in pr-review-process.md never
listed PHPStan.

- PluginInterceptor passed a third argument to generateInterfaceWrapper() and
  generateConcreteSubclass(), which take two. Dead arguments, removed.
- PluginInterceptedInterface declared only getPluginTarget(), yet PluginInterceptor
  calls initInterception() through that type. Declared it on the interface, where
  the contract belongs — the PluginInterception trait already satisfies it.
- ModuleManifest::$singletons was annotated array<string, …> while list-style
  entries use int keys, which is exactly what BindingRegistry::registerModule()
  branches on via is_int(). The code was right and the annotation lied, making
  PHPStan call a live branch impossible. Behaviour is unchanged and already
  covered by BindingRegistryTest.
- Command and CommandDefinition gained list<string> for $aliases;
  collectInterfaceMethods() gained its ReflectionClass generic; Input dropped an
  array_values() call on a value that is already a list.

The one remaining error, trait.unused on PluginInterception, is a limitation
rather than a defect: every runtime consumer is an eval-generated class, so no
source-visible `use` exists. Ignored narrowly by identifier and path, with the
reason recorded in phpstan.neon.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was no PR CI in this repo at all — the only workflows were auto-label,
deploy-docs, readme-package-check, split, and sync-issue-templates. Nothing ran
tests, phpcs, php-cs-fixer, or PHPStan on a pull request, so "develop is green"
rested entirely on whoever last remembered to run them by hand. That is how core
carried twelve PHPStan errors for months, and how php-cs-fixer drifted on 129
files.

Add a CI workflow with three jobs — Tests, Lint, Static analysis — triggered on
every pull request with no paths: filter, so no PR can route around the gate.
Add composer phpstan and composer ci so the gate is one command locally, and add
both to the PR review checklist that omitted PHPStan.

The integration-destructive group is deliberately excluded from the gate: it
deletes vendor/ and composer.lock, runs composer update, and re-runs the whole
suite in a subprocess, so it measures install integrity rather than behaviour and
is order-dependent under --parallel. It now runs nightly instead of never, and
bin/release.sh still runs it before any tag is cut.

phpcs.xml excludes the deliberately-unparseable codeindexer fixture. It cannot be
valid PHP by construction — that is the test case — and phpcs aborts on the file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Mechanical only — no behaviour, no logic, no test expectations changed. Produced
entirely by `composer cs:fix`; every hunk is one of the two formatters' own
output.

php-cs-fixer was failing a dry run on 129 files on develop, and phpcbf on 40
across 15. Neither was ever enforced repo-wide: the review checklist asks for
both, but only "on touched files", so drift accumulated everywhere nobody
happened to edit. The CI gate added in the previous commit checks the whole
repo, which cannot go green while that backlog exists.

The two tools also disagreed on twelve files — phpcbf's multi-line reformatting
produced output php-cs-fixer wanted to change back. `composer cs:fix` runs
php-cs-fixer first and phpcbf second, so phpcbf wins, and running it once
converges: a second pass reports zero for both. That ordering is now load-bearing
for the Lint job, not just a convenience.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
php-cs-fixer lints every file before fixing and exits 4 when one will not parse,
even with zero files to change. Three fixtures are invalid PHP on purpose — the
config and codeindexer packages test what happens when the tooling is handed a
broken file — so the Lint job would have failed on arrival, permanently, on a
condition no one could fix without deleting the test cases.

Caught by running the gate end to end and checking the real exit code. Piping the
command through grep, as I had been, reports grep's status and hides this
completely.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@markshust markshust added the enhancement New feature or request label Jul 26, 2026
@github-actions github-actions Bot added ci CI/CD pipeline and automation changes and removed enhancement New feature or request labels Jul 26, 2026
Three tests passed only on the author's machine. The gate caught all three on its
first run, which is the argument for the gate.

- ratelimiter and skeleton hardcoded /opt/homebrew/Cellar/php/8.5.1_2/bin/php,
  and ratelimiter fell back to /opt/homebrew/bin/composer. pr-review-process.md
  forbids exactly this ("no hardcoded paths or environment-specific values — use
  PHP_BINARY"), but nothing enforced it. Both now use PHP_BINARY; the composer
  lookup skips rather than guessing a Homebrew path.
- PidFileTest built its process group through a string proc_open command, which
  runs via /bin/sh, so the reported pid belongs to the shell rather than to PHP.
  macOS sh execs in place and the pids coincide; Ubuntu's dash does not, so
  posix_setsid() created the group under a pid the assertions never inspected.
  Switched to the array form, which execs directly, and added a pcntl/posix guard.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added ci CI/CD pipeline and automation changes and removed ci CI/CD pipeline and automation changes labels Jul 26, 2026
@markshust
markshust merged commit 4f86b49 into develop Jul 26, 2026
4 checks passed
@markshust
markshust deleted the feature/ci-gate-and-phpstan-fixes branch July 26, 2026 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci CI/CD pipeline and automation changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant