fix(quality): phpcs errors must fail the gate; warnings still pass [DRAFT — do not merge]

[rubenvdlinde]

🚧 DRAFT — DO NOT MERGE

.github@main is consumed live by every repo in the fleet, so merging this flips phpcs enforcement everywhere the instant it lands. It must not merge until the existing error debt is cleared, and the flip is Ruben's call. Debt-clearing PRs are listed at the bottom.

---

The defect

.github/workflows/quality.yml, the phpcs case:

# Exit code 0 = clean, 1 = warnings only, 2 = errors found.
RC=0
composer phpcs || RC=$?
if [ "$RC" -eq 1 ]; then
  echo "::warning::PHPCS found warnings but no errors — passing."
  exit 0
fi
exit $RC

That comment is false for this fleet's phpcs configuration. Established empirically — by reading php_codesniffer 3.13.6's own Runner.php and by running the real thing in a PHP 8.3 container, not from docs:

Runner::run() computes $return = totalErrors + totalWarnings, then subtracts warnings because the shared hydra-gates ruleset sets <config name="ignore_warnings_on_exit" value="1"/>. Runner::runPHPCS() then maps that count:

exit	actual meaning
0	no ERRORS. Warnings never reach the exit code at all.
1	ERRORS present, and NONE of the violations are phpcbf-fixable.
2	ERRORS present, at least one is phpcbf-fixable.
3	phpcs could not run (config/tokenizer failure).

Exit 1 never meant "warnings only". It meant "errors, none auto-fixable" — and the gate mapped every one of them to success.

Why it is near-total rather than occasional

The shared ruleset deliberately removed every auto-fixable formatting sniff (its own header documents why: 111,747 of 111,932 findings were the ruleset arguing with php-cs-fixer). That makes exit 2 nearly unreachable, so exit 1 becomes the ordinary way for errors to present — and the special case swallows essentially the entire fleet's error debt.

Two individually reasonable decisions — purge fixable sniffs; treat warnings as non-blocking — that are jointly catastrophic. Confirmed by the data below: no repo returned exit 2. Not one.

---

Measurement — all 18 core repos, development, 2026-08-17

Method: latest quality / PHP Quality (phpcs) check-run per repo, restricted to push-event check suites (the standing development → beta PR attaches rows to the same sha). Job log read via the API, Actions' echoed step source stripped first (^\S+ \x1b\[36;1m), then the authoritative per-file FOUND n ERRORS AND m WARNINGS summary lines summed.

⚠️ grep -c ERROR over a raw CI log overcounts badly — every individual violation row contains the word, and the log contains the gate's own script text echoed by Actions. Counted by the narrow identifier instead. Cross-checked with a second, independent count over | ERROR | violation rows: the two methods agree exactly on all 12 affected repos.

repo	check-run	ERRORS	warnings	printed "no errors — passing"
opencatalogi	success	401	116	yes
pipelinq	success	143	604	yes
decidesk	success	63	112	yes
procest	success	21	505	yes
openregister	success	20	0	yes
nldesign	success	17	3	yes
hermiq	success	16	42	yes
scholiq	success	10	112	yes
openbuild	success	10	98	yes
doriath	success	3	785	yes
launchpad	success	2	470	yes
openconnector	success	1	286	yes
docudesk	success	0	235	no
softwarecatalog	success	0	105	no
zaakafhandelapp	success	0	939	no
shillinq	success	0	539	no
portaliq	success	0	19	no
larpingapp	success	0	0	no

707 errors across 12 repos, every one of them reported green.

The last column is the control: the "passing" line appears exactly when errors exist and never when they don't — 12/12 and 6/6, no exceptions. That correlation is what proves the mechanism rather than merely correlating with it.

Exit-code semantics, measured directly

fixture	errors	warnings	raw phpcs exit
hermiq @ development	16	42	1
docudesk @ development	0	235	0
synthetic (one long-array-syntax error)	1	2	2

The third row is a positive control proving exit 2 is reachable at all — otherwise "no repo returned 2" would be unfalsifiable. Fixable count on all 12 affected repos: 0.

---

The fix

Pass the exit code straight through. Warnings staying non-blocking needs no special case, because phpcs already returns 0 for them — so this is simultaneously the correct rule and the simpler one.

--runtime-set ignore_warnings_on_exit 1 is appended so that "warnings do not fail the build" holds by construction at the gate rather than by inheritance. Today it rests on one line in one vendored ruleset: all 18 repos reference vendor/conduction/hydra-gates/quality-config/phpcs.xml and none sets the flag itself — too thin a thread to hang an enforcement flip on. (Verified that composer forwards -- <args> to the script's last command, and that the flag leaves both fixtures' verdicts unchanged.)

Behaviour change is exactly one cell

Verified with a shell control run against both the old and the new logic:

phpcs exit	old step exit	new step exit
0	0	0
1	0 ← the hole	1 ← fixed
2	2	2
3	3	3

Errors now fail. Warnings still pass. Nothing else moves.

---

Merge order

This must land after the debt is clear. Debt-clearing PRs (one per repo, base development, errors only — warnings deliberately untouched):

• hermiq — fix(phpcs): clear all 16 PHPCS errors hermiq#345 (16 → 0)
• decidesk — fix(phpcs): clear 63 PHPCS errors decidesk#518 (63 → 0)
• others in flight; this PR should be re-checked against a fresh sweep before it is un-drafted.

Recommended: re-run the 18-repo sweep immediately before merging and confirm all 18 read 0 errors. A repo that regains an error between now and then would go red on its next push.

---

This is one of three defects of the same shape in this file

#	where	what it does
#483 (this)	the phpcs case	measures correctly, then discards the verdict — exit 1 → success
#484	sibling-app composer install … 2>/dev/null || true	discards the measurement, reports success
#355	sibling-app app:enable … || echo "::warning::…"	discards the measurement, reports success (draft since 2026-08-11, never landed)

Three places where the shared workflow reports success over a failure it had already observed.

This PR is the only one of the three with a debt precondition. #484 and #355 do not turn any existing green cell red — they only stop a failed step from reporting success, so they can be sequenced independently and sooner. This one flips enforcement and must wait for the 707 errors to clear.

---

⚠️ SEQUENCING RISK — clearing phpcs debt can trip the COVERAGE RATCHET

Found while clearing hermiq's 16 errors: the fix went green on phpcs and red on the coverage ratchet, from the guard step that runs after a fully passing suite (OK (1738 tests, 5376 assertions)).

Changed files, head:    68.73%  (523/761 statements)
Changed files, base:    69.21%  (517/747 statements)
FAIL: coverage of the files this change touches dropped by 0.48%.

+14 statements, only +6 covered. All 8 uncovered ones were measured with local pcov against the merge base, and each is a mechanical consequence of a sniff this gate will make mandatory:

cause	required by	effect
a call reformatted to named arguments over 7 lines	CustomSniffs.Functions.NamedParameters	1 uncovered line → 7 uncovered statements (+6)
2 ternaries expanded to if	Squiz.PHP.DisallowInlineIf	1 covered statement → covered + uncovered branch each (+2)

The first row is the one to note: a pure formatting change multiplied one uncovered statement by seven. Statement count is not invariant under reformatting. And 396 of the 707 fleet-wide errors are the named-parameters sniff — every one is a call site that may go multi-line.

This is a third inversion of the ratchet alongside deletions and decomposition (DECISION-1, .github#480): it now also taxes phpcs remediation.

So the two gates are in direct opposition, and this PR makes one of them mandatory. Recommended before un-drafting:

1. Land the per-repo debt PRs first (in progress).
2. Confirm .github#480's asymmetric rule normalises statements added by reformatting, not only deletion/decomposition — otherwise a repo can be unable to satisfy both gates at once.
3. Re-run the 18-repo sweep and confirm all read 0 errors.

hermiq was resolved without filler tests — the coverage was bought back with one real test of a genuinely untested safety property. That is the pattern to copy, but it is effort the flip should budget for rather than discover.

---

✅ THE DEBT IS CLEARED — complete 18-repo table

All 12 affected repos now have an open PR against development taking them to 0 phpcs errors. Every "after" figure below is verified by re-reading that PR's own CI log with the same instrument used for the baseline sweep — not from any implementer's self-report.

repo	default br.	errors BEFORE	errors AFTER	warnings left	phpcs exit	PR
opencatalogi	main	401	0	111	1 → 0	#903
pipelinq	development	143	0	595	1 → 0	#1169
decidesk	development	63	0	108	1 → 0	#518
procest	main	21	0	492	1 → 0	#869
openregister	main	20	0	0	1 → 0	#2540
nldesign	main	17	0	0	1 → 0	#354
hermiq	development	16	0	39	1 → 0	#345
scholiq	development	10	0	112	1 → 0	#423
openbuild	development	10	0	98	1 → 0	#233
doriath	development	3	0	782	1 → 0	#254
launchpad	main	2	0	468	1 → 0	#133
openconnector	main	1	0	286	1 → 0	#1279
docudesk	main	0	0	235	0	—
softwarecatalog	main	0	0	105	0	—
zaakafhandelapp	main	0	0	939	0	—
shillinq	development	0	0	539	0	—
portaliq	development	0	0	19	0	—
larpingapp	development	0	0	0	0	—

707 → 0.

The control holds in both directions. The ::warning::PHPCS found warnings but no errors line appeared on exactly the 12 repos with errors and none of the 6 without; it is now absent from all 12. A clean control that shows the marker appearing only when errors exist, and disappearing when they are fixed.

How the debt was cleared

No phpcbf, no --fix, anywhere. That is not merely policy — for these repos it was a no-op by construction: exit 1 means totalFixable === 0, so all 707 violations were non-auto-fixable. Also zero phpcs:ignore, zero <exclude> rules, zero ruleset or baseline edits, and no scripted code edits. Each PR carries a per-tool check:strict baseline-vs-after and PHPUnit compared by failing-test name, not just totals.

Error mix: 396 NamedParameters, 176 MissingParamTag, 58 WrongStyle, 32 LineLength, ~28 ParamNameNoMatch, 9 DisallowInlineIf, plus a handful of others.

Two things the cleanup discovered that bear on this PR

1. The @spec coverage gate has been under-reporting. Fixing WrongStyle removed ~33 SpecTag warnings across the fleet — because a stray // comment between docblock and function was hiding real @spec tags from the sniff. Proven at source (opencatalogi's SearchController::index() carries two @spec tags the sniff called missing), and confirmed by two independent instruments: grep -c '@spec' byte-identical before/after, and launchpad's separate lint:spec-annotations script agreeing. A comment in the wrong position made a different gate blind, and that gate reported the blindness as debt against the code.

2. cancelled is not a pass, and it nearly entered this table as one. Two PRs carried completed/cancelled phpcs rows from push runs superseded when the PR opened. A cancelled job's log is truncated, so parsing it yields a zero error count indistinguishable from a clean run. Worse, a still-running row has completed_at = null, which sorts before a finished one — so "take the latest row" selected the stale run. Both holes were closed only because live data forced them.

Recommended merge order

1. ✅ Debt cleared — merge the 12 PRs above.
2. Confirm .github#480 handles statements added by reformatting (see the sequencing risk above — 396 of 707 errors are the named-parameters sniff, whose fix can trip the coverage ratchet).
3. Re-run the 18-repo sweep and confirm all read 0, then un-draft this PR.

Step 3 is not ceremony: a repo that regains a single error between now and the flip goes red on its next push.