S146 — make the Psalm gate real: it has never analysed a single file#572
Merged
Conversation
Checkpoint of interrupted work so it cannot be lost. NOT the final commit: the workflow guard is still present and the fix set is incomplete. This branch will be squash-merged so master still receives ONE atomic commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nalysis extensions The `psalm:` job in coding-standards.yml has existed since the first CI commit (b7c728c, 2026-05-15) but `vimeo/psalm` was never in composer.json, so `vendor/bin/psalm` never existed, its `if [ -f vendor/bin/psalm ]` guard was always false, and the job reported GREEN having analysed zero files. It was counted as real coverage in PR reviews. - Remove the `if [ -f vendor/bin/psalm ]` guard so the analysis step can fail. - Replace it with an explicit "Assert Psalm is actually installed" step, so a missing tool fails loudly instead of silently degrading back into a no-op. - Add an "Assert no psalm baseline file" step mirroring the PHPStan job's, so the gate can never be made green by a baseline. - Declare the analysis extensions. With `extensions: json` only, psalm reports 140 phantom errors on this tree (Swoole\Coroutine\Channel x34, Swoole\Coroutine x23, Socket x15, Swoole\Coroutine\Socket x6, ZipArchive x3, GdImage x3, Swoole\Coroutine\Http\Client, FFI, plus AF_INET/SOL_SOCKET/SOCK_DGRAM/ SO_RCVTIMEO/SO_SNDTIMEO/SOL_UDP) that are missing-extension artifacts, not defects. Base list copied from phpunit.yml's canonical project list. - README: document the PHP >= 8.3.16 minimum psalm 6.x enforces and the Docker workaround, so a contributor on an older dev PHP can still reproduce the gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ed with PHPStan L9 Verifying the Psalm work against the other gates found PHPStan level 9 had gone red on three of the earlier fixes. Both gates now pass on src/ with zero errors. - DashboardService::formatBytes(): the `?? 'TB'` added for Psalm's InvalidArrayOffset made PHPStan report nullCoalesce.offset, and removing it put Psalm's error back — the two contradict each other on the indexed form. Consume the unit list with array_shift() instead, so the label is a value that provably exists rather than an offset either tool has to bound. Verified byte-identical to the previous implementation on the boundary cases (0, 1023/1024/1025, 1024 TB saturation, PHP_INT_MAX, negatives) and 200k random inputs. - ChromaPrintFfi + WebSocketServer: folding `@phpstan-ignore-next-line` into the new multi-line @psalm-suppress docblock silently disarmed it — PHPStan resolves "next line" relative to the tag's OWN line, so it landed on a comment line and reported ignore.unmatchedLine while the real method.notFound / property.notFound errors came back. Use the trailing `@phpstan-ignore <identifier>` form, which binds to the statement, and note why in the docblock. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Documentation | 6 minor |
| CodeStyle | 2 minor |
🟢 Metrics 0 duplication
Metric Results Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #572 +/- ##
============================================
- Coverage 65.81% 65.81% -0.01%
- Complexity 21055 21058 +3
============================================
Files 657 657
Lines 66241 66237 -4
============================================
- Hits 43595 43591 -4
Misses 22646 22646 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
The defect
.github/workflows/coding-standards.ymlhas declared a job named "Psalm Static Analysis" since the first CI commit (b7c728ca, 2026-05-15). Its work step was wrapped inif [ -f vendor/bin/psalm ], andvimeo/psalmhas never been incomposer.json(git log -S psalm -- composer.json→ 0 commits). So the binary never existed, the condition was always false, and the job reported green having analysed zero files.It was counted among "all real gates green" when S122 merged. PHPUnit, PHPStan L9 and phpcs genuinely ran there, so S122 stands on the merits — but the gate set proved less than was claimed.
Why this finishes the rollout rather than deleting the job
b7c728caapplied the same defensiveif [ -f … ]guard to phpcs, phpstan, security-checker and psalm. For the first three the tool was later actually installed, which made their guards vacuously true. Psalm's never was. Nothing was removed — the rollout was abandoned mid-flight, so the additive fix is to complete it.Atomicity
Dependency + config + guard removal + fixes land in one squashed commit. Adding the dependency alone makes psalm abort with
Could not locate a config XML fileand turnsmasterred instantly.errorLevel 5 — measured, not guessed
Level 4 is the cliff: 348 of the 395 findings it adds over level 5 are
RedundantCondition/TypeDoesNotContainType/RedundantCast, which in this codebase are deliberateis_array()/is_string()guards on DB rows and decoded JSON. Deleting those to satisfy a linter would strip real runtime protection.No
psalm-baseline.xml, and no<issueHandlers>element inpsalm.xml. A baseline is the same "proves nothing" defect wearing a different hat. Two new assertion steps enforce this: one fails loudly if the binary is missing (so the gate cannot silently degrade to a no-op again), one fails if a baseline appears.Extensions are required, not cosmetic
The job declared
extensions: json. Measured on this tree with stockphp:8.3-cli: 140 errors, every one a missing-extension artifact (Swoole\Coroutine\Channel×34,Socket×15,AF_INET, …). A gate that is red for reasons unrelated to the code gets switched off — which is how the original no-op guard came to exist.Real defects this gate caught
AccessSchedule::timeToMinutes()returned a float from an: intmethod in adeclare(strict_types=1)file.isActiveAt()feeds itH:i:s, so for ~59 seconds of every 60 it threwTypeError. Live bug, now fixed.WebPortalRouter::tmdbApiKey()@included a path resolving above the repo root — unreachable, with@swallowing it andgetenv()masking the fallthrough. Path corrected to matchgetPlaybackPreferences(); the include is kept, not deleted.@phpstan-ignore-next-linefolded into a multi-line docblock is silently disarmed — PHPStan resolves "next line" from the tag's own line, landing on a comment. Replaced with the statement-bound// @phpstan-ignore <identifier>form.DashboardService::formatBytes()had the two analysers directly contradicting each other (Psalm demanded?? 'TB'; PHPStan called that same??nullCoalesce.offset). Resolved without a suppression, and verified byte-identical to the old output across 0 / 1023 / 1024 / 1025 / the TB saturation case /PHP_INT_MAX/ negatives / 200k random inputs.Suppressions: 9, all narrowly scoped with inline justification
FFI runtime bindings, Workerman dynamic properties (
websocketType,onWebSocketPong— both set by the protocol itself),&$refproperty references psalm declines to analyse, and threePDOStatement::fetch()returns where narrowing to the parent docblock would hidefalsefrom every caller. One candidate atAuthManager.php:449was rewritten rather than suppressed because the reference was not load-bearing.Other gates re-verified — none weakened
⚠ Psalm 6.5 requires PHP >= 8.3.16; the dev box runs 8.3.6. README documents the minimum and a CI-faithful Dockerfile so contributors can reproduce the gate locally.
🤖 Generated with Claude Code