fix(quality): empty procest's phpstan baseline — 10 dead injected dependencies removed

[rubenvdlinde]

Why

composer check:strict was reporting green on procest's phpstan leg while 10 real phpstan errors were hidden by phpstan-baseline.neon. The gate's exit code was decoupled from the codebase's actual state — the classic dead-gate shape, where a check's suppression looks exactly like its success.

This PR fixes all 10 at source, deletes the baseline file, and removes it from phpstan.neon's includes:.

Before / after measurement

All runs in php:8.3-cli (host PHP is 8.2 and silently exits 255 on this repo, printing nothing).

#	Configuration	Exit	Result
1	development @ 70e1e06, baseline as shipped (14 entries)	0	[OK] No errors
2	Same code, baseline body replaced with an empty parameters:\n\tignoreErrors:	1	Found 10 errors
3	Baseline file deleted but includes: left in place	1	File '/app/phpstan-baseline.neon' is missing or is not readable.
4	This PR — 10 errors fixed at source, baseline + includes: both gone	0	[OK] No errors

Run 1 vs 2 is the whole point: the same code, analysed by the same tool, is green or red depending only on the suppression file. 10 of the 14 entries were load-bearing. The other 4 had already rotted into no-ops — the underlying code had been fixed and nobody pruned the file:

• AiController::$logger
• StufMessageBuilder::$logger
• WorkflowDefinitionService::$userSession
• the ParaferingAuditAppendOnlyValidator @implements generic-type entry (already covered by a documented ignoreErrors pattern in phpstan.neon itself, so it was double-suppressed)

Run 3 is the check that deletion is safe: a missing include fails loudly, it does not silently continue. So the includes: entry had to go too, and its absence cannot itself become a new silent pass.

Positive control on the final config. A baseline PR that leaves phpstan no-opping would be solving one dead gate by creating another. Before committing I added a deliberate unused promoted property to RoleGuard and re-ran the final phpstan.neon: exit 1, 524/524 files analysed, and the error raised was the exact rule these 14 entries had suppressed (Property ... is never read, only written). The canary was then reverted and exit 0 reconfirmed. The green in run 4 is bought by the source.

Case (a) / case (b) verdict per file

"Never read, only written" on an injected dependency has two causes: (a) leftover DI from a refactor, safe to remove; or (b) a check that was designed and never wired, where deleting the property hides the bug. Each of the 10 was read in full before touching it.

All 10 are case (a). None removes an enforcement path.

#	File	Property	Verdict and reasoning
1	lib/Controller/LhsController.php	$settingsService	(a) All three actions (recommend, override, lookup) authenticate via IUserSession and return 401 on null; override() additionally gates the manager role through IGroupManager::isAdmin(). SettingsService is an OpenRegister/config bridge with no authorization API — see note below.
2	lib/Controller/TemplateController.php	$logger	(a) All three methods check userSession->getUser() === null → 401. A logger is observability, never a check.
3	lib/Listener/BezwaarLifecycleListener.php	$settingsService	(a) Deliberate pure observer — its own comment states all state-machine work belongs to StatusTransitionService.execute() and there is intentionally no transition logic here. Nothing to enforce.
4	lib/Service/Bezwaar/AdvisoryCommitteeService.php	$transitions	(a) The one I looked hardest at, since routing through the FSM could have been the missing guard. It is not. The advice lifecycle is enforced locally and fail-closed: assertTransitionAllowed() reads ALLOWED_TRANSITIONS[$from] ?? [], so an unknown source state denies every target; plus a VALID_STATUSES whitelist, the Awb 7:13(3) independence guard on assigned → in-deliberation, and required advice fields with a fail-closed decidesk delegation on → advice-issued. $transitions was documented as an optional parent-case integration and no method ever advances the parent case. Reachability confirms it: transitionAdviceStatus() and assignToCommittee() have no external callers, so there is no unguarded HTTP path either way.
5	lib/Service/Bezwaar/BeroepService.php	$userSession	(a) StatusTransitionService::execute() resolves the actor itself (resolveUserId()) and runs assertTransitionAllowed() with it, and every write lands through OpenRegister, which stamps the acting user on its own audit trail. The caller does not need its own session. The class docblock claimed identity is "ALWAYS derived from IUserSession" — that was already untrue, and this PR corrects the prose. See finding 1 below: this whole class is unwired.
6	lib/Service/CaseDefinitionImportService.php	$appConfig	(a) The service reads no config at all and writes nothing to disk — it reads ZIP entries by name via getFromName()/getNameIndex(), never extractTo(), so there is no zip-slip path a config-driven limit would have guarded. See finding 2 below: importComponent() is a stub.
7	lib/Service/Pdok/PdokBagService.php	$settingsService	(a) A redundant second config accessor. The live path is $appConfig, read at 4 call sites for endpoint/source/TTL.
8	lib/Service/Pdok/PdokLocatieserverService.php	$settingsService	(a) Identical shape; $appConfig is read at 7 call sites.
9	lib/Service/TenantService.php	$settingsService	(a) The class resolves OpenRegister itself through $appManager + $container, a parallel path to SettingsService::getOpenRegisterClass(). Its one auth-flavoured method, isPlatformAdmin(), delegates to IGroupManager::isAdmin() and does not consult settings.
10	lib/Service/Transitions/RoleGuard.php	$settingsService	(a) evaluate() is complete: allowedRoles check, direct case-role match, Nextcloud group fallback, then falls through to passed: false — fail-closed. Nothing is missing.

A structural note that makes six of these verdicts cheap to trust: SettingsService's entire public API is OpenRegister resolution plus config get/set (isOpenRegisterAvailable, getObjectService, getOpenRegisterClass, getConfigValue, setConfigValue, reconcileSchemaConfig, …). It exposes no authorization, role, or permission method at all. An unused $settingsService therefore cannot be an unwired access check in any of these classes — the check it would have performed does not exist on the type.

Two findings this review surfaced (not fixed here — both are product decisions)

Neither is caused or hidden by this PR; both are reported because looking at the 10 properties is what exposed them.

1. BeroepService is ~590 lines of entirely unwired service. Its only mention anywhere outside its own file is a prose sentence in the register schema, lib/Settings/procest_register.json:4287: "Immutability after appellantFilingDate is enforced by BeroepService." That claim is false twice over — the class implements no such immutability check, and it has no callers, so nothing runs. A schema description is documentation, not enforcement. Deleting the class is a bigger call than this PR should make; flagging it instead.

2. CaseDefinitionImportService::importComponent() is a stub that reports success. It parses each component's JSON and then returns status: 'success', "Component '<x>' imported successfully" while persisting nothing — the body carries the comment "In a full implementation, this would create/update OpenRegister objects." importCaseDefinition() aggregates those into "Import completed successfully". The removed $appConfig was plainly the intended register/schema accessor for that unwritten persistence. Blast radius is limited: CaseDefinitionController declares no #[NoAdminRequired], so the endpoint is admin-only. Removing the unused property does not make this less visible — the explicit stub comment stays in the code, and hydra-gate-stub-scan targets exactly this shape.

Changes

• 10 dead promoted constructor properties removed, with their @param docblock lines and the use imports that became orphaned.
• tests/Unit/Service/Transitions/RoleGuardTest.php — the only construction site of any of these classes outside the DI container (2 × new RoleGuard(settingsService: …), named args). Every other class is autowired. Test count is unchanged at 1684.
• lib/Service/Bezwaar/BeroepService.php — class docblock corrected to describe how identity is actually resolved.
• lib/Service/CaseDefinitionImportService.php — also drops a pre-existing unused use OCA\Procest\AppInfo\Application;.
• phpstan-baseline.neon deleted; includes: removed from phpstan.neon and replaced with a comment saying why it must not come back.
• openspec/specs/quality-gates/spec.md — the requirement changes from "PHPStan baseline is documented and minimal" to "PHPStan runs with no baseline", mirroring the PHPMD requirement immediately above it, and adds a scenario asserting that emptying the suppression config leaves the result unchanged.
• README.md — corrected; it still advertised the baseline as a virtue.

Gate results (this branch, exit codes printed, stderr byte counts checked)

Gate	Command	Exit	Result
phpstan	php:8.3-cli … phpstan analyse --memory-limit=1G	0	[OK] No errors, 524/524 files, no baseline
phpunit	nextcloud:latest … phpunit --no-coverage	0	1684 tests, 5614 assertions, 0 failures, 5 skipped — identical to development
phpcs	php:8.3-cli … phpcs --standard=phpcs.xml	0	0 errors (pre-existing @spec warnings only, unchanged)
psalm	nextcloud:latest … psalm --threads=1 --no-cache	0	No errors found
phpmd	php:8.3-cli … phpmd lib text phpmd.xml	2	25 findings — unchanged, none in a file this PR touches

⚠️ Environment correction for whoever runs these next: psalm must be run in a container that has ext-zip. Under php:8.3-cli it exits 2 with 25 UndefinedClass: ZipArchive errors that are pure environment artefacts — I verified all 25 are ZipArchive and that the same commit is clean under nextcloud:latest. The same caveat already applies to phpunit. A php:8.3-cli psalm run would look like this PR broke 25 things; it did not.

Follow-ups worth an issue

• .github/CODEOWNERS:15 still has a phpstan-baseline.neon rule that now matches nothing. Left alone deliberately to keep CODEOWNERS out of a quality PR.
• The deleted includes: comment claimed the block is "byte-identical across the fleet" and that debt-free apps ship an empty baseline file. If that convention is real, the other apps should drop theirs too — an empty baseline file is a standing invitation to refill it, and this repo is the demonstration of what happens next.
• Findings 1 and 2 above (BeroepService dead class; CaseDefinitionImportService success-reporting stub).

[github-actions]

Quality Report — ConductionNL/procest @ f786c18

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	❌
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
composer			✅	✅ 100/100
npm			✅	✅ 550/550
PHPUnit					❌
Newman					⏭️
Playwright					⏭️

---

Quality workflow — 2026-08-04 01:12 UTC

Download the full PDF report from the workflow artifacts.