feat(quality): the last four Nextcloud checks — app:check-code, REUSE, multi-DB PHPUnit - #385
Merged
Merged
Conversation
…loud checks Nextcloud runs five checks this fleet did not. #383 added the first (info.xml against the App Store XSD). This adds two more; multi-database PHPUnit is the remaining one and is a larger change to the phpunit job, tracked separately. occ app:check-code ------------------ Nextcloud's own tool for private and deprecated API usage. Our closest equivalent was ONE hand-written PHPCS sniff covering ONE symbol — \OC::$server, removed in NC 34 — written by hand precisely because static analysis could not see the removal while nextcloud/ocp sat a major below the declared min-version. app:check-code is the general form of that sniff: it knows the whole private surface of the server it ships with, needs no per-symbol maintenance, and moves forward when Nextcloud does. The server is fetched at the version the app DECLARES, anchored to the <nextcloud> element. Unanchored, the pattern returns 8 for thirteen fleet apps (from <php min-version="8.3"/>) and a plausible-but-wrong 28 for a fourteenth. REUSE ----- Nextcloud runs reuse.yml on every app. We had a PHPCS sniff checking @license and @copyright in PHP docblocks — a proxy for one file type that says nothing about images, JSON, YAML, or the LICENSES/ directory REUSE requires. It also closes a loop the coding-standard migration opened: the SPDX InvalidEndChar exception in the shared ruleset exists because a trailing full stop turns a machine-parsed SPDX expression into an invalid one. That exception is only worth having if something actually parses those expressions. BOTH ARE NON-BLOCKING BY DEFAULT, and that is a decision rather than timidity. Every app here reaches into server internals somewhere — OCA\DAV, OC_App and Doctrine\DBAL are all ignored by name in phpstan-base.neon — and only 2 of the 4 apps sampled ship a REUSE.toml at all. A gate that is red on arrival is a gate nobody turns on: that is how openregister ended up with an E2E job that had never once succeeded. Each carries a `*-blocking` input to flip per app once its findings are worked down. app:check-code carries a positive control. occ exits 0 on paths that inspect nothing — an app directory it cannot see, a command that bailed on the environment check — so the step asserts the log contains a recognisable verdict before treating any exit code as evidence. A silent exit 0 must not read as "no private API usage".
…check Nextcloud ships phpunit-mariadb, phpunit-mysql, phpunit-oci and phpunit-sqlite as separate workflow templates, because database-portability bugs are invisible on a single backend. This fleet ran PostgreSQL only. The `database` input already accepted sqlite, pgsql and mysql, but the phpunit job wired exactly one of them: the postgres service was keyed off `inputs.database`, and the install block had a pgsql branch with everything else falling through to a bare `--database <x>` that works for sqlite and cannot work for mysql — no host, no port, no credentials, and no service container to connect to. So "mysql" was a value the input accepted and the job could not honour. WHAT CHANGED - `database-test-matrix`, a JSON array, joins the phpunit matrix beside php-version and nextcloud-ref. - A mysql:8.4 service sits beside postgres:16. Both are keyed off matrix.database rather than inputs.database, and a service whose image evaluates to an empty string is not started — which is how one job definition serves several backends. - The install block is a case statement with real host/port/credentials per backend, and an explicit failure on an unknown one. - pdo_mysql and mysqli join the extension list unconditionally. Conditional extensions would mean a mysql leg silently falling through and testing sqlite while its name said mysql. INERT UNTIL AN APP OPTS IN. The default is an empty array, which collapses to `[inputs.database]` — today's dimensions exactly. Both sides of that comparison go through fromJSON on purpose: a string test against '[]' accepts '[ ]' and '[""]' and then resolves to an EMPTY MATRIX VECTOR, and an empty vector does not skip a job, it deletes it from the run with no row, no annotation and no trace. Measured on this repo's own self-test for frontend-checks: 19 jobs instead of 21. Verified here against all three spellings of empty plus a real three-backend array. POSITIVE CONTROL IN THE JOB. `maintenance:install` can exit 0 having fallen back to another backend, and a suite passing on sqlite while its leg is labelled mysql is worse than no leg at all — it is a green cell asserting something about a database it never touched. The step reads `occ config:system:get dbtype` back and fails if it does not match the leg's own name. That completes the five checks Nextcloud runs and we did not: info.xml XSD (#383), app:check-code and REUSE (this branch), integrity:sign-app (#375), and multi-database PHPUnit.
#383 (info.xml XSD) and this branch (app:check-code, REUSE, multi-database PHPUnit) both insert jobs immediately before the Vue Quality group, so git saw four conflicts that are all pure additions rather than competing edits. Resolved by keeping both sides. Verified after resolving: 20 jobs, no duplicate job names, YAML parses, and all four of info-xml / app-check-code / reuse / phpunit are present. A keep-both resolution that silently dropped one side would look identical to a clean merge in the diff, so the job list is checked rather than assumed.
rubenvdlinde
pushed a commit
that referenced
this pull request
Aug 12, 2026
…efault it off This job cannot pass in any repo in this fleet, and merging it into the Quality Report's `needs:` would have hard-failed every PHP app on a check that no longer exists. MEASURED, not inferred. Extracting the registered App-namespace commands from `core/register_command.php` at each tag — same expression, same file, only the tag varying: v20.0.0 Disable Enable GetPath Install ListApps Remove Update + CheckCode v34.0.0 Disable Enable GetPath Install ListApps Remove Update CheckCode appearing at v20 is the control: the extraction CAN produce it, so its absence at v34 is a result rather than a failed lookup. Corroborated independently against a second artefact — `core/Command/App/CheckCode.php` answers 200 at v20 and v21 and 404 at v25, v31, v32, v33 and v34, while `core/Command/App/ListApps.php` answers 200 at every one of those tags, so the 404s are a removal and not a wrong path. Every app here declares a Nextcloud min-version of 31, 32 or 34, and the job fetches the server at the version the app DECLARES. So occ would be asked for a command around eleven majors dead, print no recognisable verdict, and trip the job's own positive control — whose `exit 1` is UNCONDITIONAL. It sits above the check-code-blocking branch and is not covered by it, so "non-blocking by default" was never true of this job's failure path. The positive control behaved exactly as designed: it refused to report an absence of private API usage from a run that inspected nothing. The job is what is wrong, not the control. LEFT WIRED AND STILL IN `needs:`. A skipped job's result is `skipped`, not `failure`, so this blocks nothing while it is off, and the wiring stays correct for whoever rebuilds the check on tooling that still exists. Deleting it would quietly re-open the gap #385 was written to close. check-code-blocking is untouched. Nobody asked for it to move, and it is irrelevant while the job does not run.
rubenvdlinde
added a commit
that referenced
this pull request
Aug 12, 2026
… and disable the dead app:check-code (#389) TWO DEFECTS, plus a third found on the way in. 1. quality.yml produced ZERO JOBS fleet-wide since 11:26:16Z. #385 merged from a branch written before #383; its hunks anchored on what had become info-xml's Checkout step, and the clean textual merge spliced two jobs into a third — app-check-code lost its steps:, reuse swallowed info-xml's body, a stray sparse-checkout landed inside a run: block, and the final upload-artifact ended up with two name: keys. Every core app consumes this file at @main, so both apps that pushed after 11:26 got a run with no jobs at all (openconnector 31592994170, larpingapp 31592027242). An unresolvable reusable workflow never goes red on its own; only this repo's resolve probe noticed. Note for future linting: PyYAML's safe_load parses the broken file happily and reports twenty jobs, so a generic YAML lint would have called it clean. 2. None of the three Nextcloud legs was in the Quality Report's needs:, which is the only meaningful required check across the fleet. Now 16 -> 19. That is three jobs, not five: multi-database PHPUnit is a matrix dimension of phpunit and was already covered, and integrity:sign-app lives in release.yml, which has no Quality Report to gate. 3. occ app:check-code WAS REMOVED FROM NEXTCLOUD AFTER v20, so that job cannot pass anywhere here. Extracting registered App-namespace commands from core/register_command.php, same expression, only the tag varying: v20 yields CheckCode alongside the others, v34 yields the others without it — v20 is the control proving the extraction can produce the token. Corroborated on a second artefact: core/Command/App/CheckCode.php is 200 at v20/v21 and 404 at v25 through v34, while ListApps.php is 200 at every one of those tags. Every app here declares min-version 31+, and the job's positive control carries an unconditional exit 1 that sits above the check-code-blocking branch — so merging it into needs: would have hard-failed all 18 PHP apps on an impossible check. enable-check-code now defaults false, with the job left wired and still in needs: (a skipped result is skipped, not failure) so the wiring is right when someone rebuilds the check. The positive control was correct throughout: it refused to certify a run that inspected nothing. BLAST RADIUS, re-measured rather than quoted. info-xml has no blocking flag and gates on arrival. Against every repo's live development tip: 6 pass, 12 fail — not #383's 5/13. The difference is larpingapp, reordered by efa6aead at 11:28:48Z; that one file validates FAIL at the old ref and PASS at the new one. Eleven of the twelve are xs:sequence element order; the twelfth is an over-length summary. All are a manifest edit, not a code change. That work is not filed anywhere yet. reuse and info-xml are both proven to work — reuse passes in this PR's own resolve probe, and info-xml produced a real schema error on openregister run 31591669849 that a local validator reproduces byte for byte. Only app-check-code was dead. From needs: all three looked identical. Also corrected: the queue was not held by a red guard. Six of the seven open PRs carry a Shared-workflow guard pass from BEFORE the break — stale greens that would flip red on any re-run. Only #378 is measurably blocked, by both defects. check-code-blocking and reuse-blocking are untouched. Neither was asked to move.
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.
Nextcloud runs five checks we did not. #383 added the first (
info.xmlXSD). This adds two more. Multi-database PHPUnit is the remaining one — a larger change to thephpunitjob, tracked separately.occ app:check-codeNextcloud's own tool for private/deprecated API usage. Our closest equivalent was one hand-written PHPCS sniff covering one symbol (
\OC::$server), written by hand precisely because static analysis couldn't see the removal whilenextcloud/ocpsat a major below the declaredmin-version.app:check-codeis the general form: it knows the whole private surface of the server it ships with and moves forward when Nextcloud does.The server is fetched at the version the app declares, anchored to the
<nextcloud>element — unanchored, that pattern returns8for thirteen fleet apps and a plausible-but-wrong28for a fourteenth.REUSE
Nextcloud runs
reuse.ymlon every app. We had a PHPCS sniff checking@license/@copyrightin PHP docblocks — a proxy for one file type that says nothing about images, JSON, YAML, or theLICENSES/directory REUSE requires. It also closes a loop: the SPDXInvalidEndCharexception in the shared ruleset is only worth having if something actually parses those expressions.Both non-blocking by default — a decision, not timidity
Every app reaches into server internals somewhere (
OCA\DAV,OC_App,Doctrine\DBALare all ignored by name inphpstan-base.neon), and only 2 of 4 apps sampled ship aREUSE.toml. A gate that is red on arrival is a gate nobody turns on — that is how openregister ended up with an E2E job that had never once succeeded. Each has a*-blockinginput to flip per app.app:check-codecarries a positive control:occexits 0 on paths that inspect nothing, so the step asserts the log holds a recognisable verdict before trusting any exit code.quality.yml, as does #383. Merge #383 first; I'll rebase if they conflict.