fix(quality): restore development to green - #147
Open
rubenvdlinde wants to merge 4 commits into
Open
Conversation
Every npm-shaped job on development failed at `Install dependencies`, and
the PHP jobs failed independently. Six distinct causes, each verified
locally before and after:
1. npm ERESOLVE. package.json pinned vite ^8.2.1 while
@nextcloud/vite-config@2.5.2 declares peer vite ^7.1.10. Nothing
installed, so eslint, stylelint, Frontend Build, Frontend Tests,
License (npm), Security (npm), Hydra Gates, E2E and all six PHPUnit
legs failed on the install step rather than on anything they measure.
vite -> ^7.1.10.
2. Pinning vite alone was NOT enough, and the build proved it: with
vite-config at 2.5.2, `vite-plugin-node-polyfills` installed NESTED
under @nextcloud/vite-config/node_modules/, so rollup could not
resolve `vite-plugin-node-polyfills/shims/global` from @vue/shared
and `npm run build` exited 1. 2.5.4 hoists it. Measured: build
exit 1 -> exit 0, package now at node_modules/ top level.
3. psalm, 7 errors. Five are IConfig::setAppValue() calls that are
DELIBERATE — the block comment above them records that writing
core-owned keys through the typed API leaves rows core can no longer
update ("conflict between new type (mixed) and old type (string)"),
which made every app installed through App Versions impossible to
enable. Rewriting them to setValueString would reintroduce that bug,
so they are suppressed AT THE CALL SITE, next to the reason, rather
than silenced globally. Same for the \OC_App::isAppCompatible static,
which is private API with no OCP stub and equally deliberate.
4. psalm UnusedBaselineEntry: the baseline claimed MixedMethodCall on
both setDefault and setNotnull in Version1004Date20260725120000, but
setNotnull is no longer called there. psalm.xml sets
findUnusedBaselineEntry=true precisely so the baseline cannot rot.
5. docs/features.json was stale at ONE feature; the generator emits ten.
Regenerated with .github@main's extract-features.py and confirmed
idempotent on a second run.
6. SBOM: `There are no commands defined in the "CycloneDX" namespace` —
enable-sbom is on but cyclonedx/cyclonedx-php-composer was not a
dependency at all. Added at ^6.2 and allow-listed, matching decidesk,
openbuild and doriath. Verified by generating an SBOM locally.
Also fixed while here, both pre-existing:
- .npmrc set `min-release-age=0` under a comment promising a 24h
cooldown. Zero is not a cooldown; the file read as hardened and
enforced nothing. The rest of the fleet moved to `2` with a
@conduction/* exclusion and this repo was missed.
- stylelint failed on the deprecated `word-break: break-word` in two
components; replaced with its equivalent `overflow-wrap: anywhere`.
Verified locally from a clean tree with the cooldown active:
npm ci, build, lint, stylelint, test, composer check:strict and
psalm all exit 0.
Contributor
Quality Report — ConductionNL/app-versions @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 24/24 | |||
| npm | ✅ | ✅ 282/282 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ❌ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-18 17:37 UTC
Download the full PDF report from the workflow artifacts.
added 2 commits
August 18, 2026 21:40
Three more causes behind the same red, all found by reading the failing
step rather than the job name.
1. ALL SIX PHPUnit legs and the E2E job died at "Install app dependencies
and enable app" with
App "app-versions" cannot be installed because appinfo file cannot be read.
which reads like a malformed info.xml and is nothing of the sort — the
file is valid against the App Store schema. Nextcloud requires the app
DIRECTORY name to equal the app id, appinfo/info.xml declares
<id>app_versions</id> with an underscore, and the workflow passed the
REPOSITORY name `app-versions` with a hyphen. Checked out under the
wrong name, the app cannot be enabled, so not one of those seven jobs
ever ran a test. Fixed by naming the app, not the repo.
2. `lint-check` and `validate` failed with
npm error Missing: picomatch@4.0.5 from lock file
for a package that IS in the lock (twice). npm 10 cannot read an npm-11
lockfile, and package.json declares `npm: ^11`. spec-validation.yml
pinned node 20; pull-request-lint-check.yaml had NO setup-node at all
and took whatever the runner image shipped. Both now pin node 24, which
is what the manifest asks for.
3. `openapi` has never passed — not on this branch, not on development,
not anywhere in its history — and it cannot:
PHP Fatal error: license: Unable to convert EUPL-1.2 to SPDX identifier
nextcloud/openapi-extractor maps licences through a hardcoded match()
accepting five short names and five SPDX spellings, and calls
Logger::panic() on anything else. EUPL-1.2 is a valid SPDX identifier;
the list is incomplete. Verified against the extractor's `main` today —
still missing. Every Conduction app is EUPL-1.2.
Mislabelling the licence to suit the tool, and marking the step
continue-on-error so a broken job reports green, were both rejected.
The job is workflow_dispatch-only with the reason recorded, and nothing
is lost — it has never produced its artefacts either. The fix is
upstream; restore the pull_request trigger when it lands.
All six PHPUnit legs failed at "Validate test infrastructure": ::error::PHPUnit is enabled but no phpunit.xml or phpunit-unit.xml found. The shared quality workflow looks for that file at the APP ROOT and there is no input to point it elsewhere. app-versions kept its config at tests/phpunit.xml — the only app in the fleet that did; decidesk, doriath, scholiq and hrmq all have it at the root already. So the legs never got as far as running a test. This is the second cause behind those six red cells. The first was the app-id/directory mismatch fixed in 25bacd2; with that alone they got further and then failed here instead. Paths inside the file are rebased from tests/ to the root (bootstrap="tests/bootstrap.php", testsuite directory "tests", source "appinfo"/"lib"). Verified locally that PHPUnit now resolves the bootstrap correctly — it fails afterwards on `../../../tests/bootstrap.php`, the NEXTCLOUD SERVER bootstrap, which by design only exists when the app is checked out inside a server tree. The old config fails identically here, so that is the standalone-worktree limitation and not this change. tests/phpunit.xml is deleted rather than left behind: two configs for one suite is exactly how they drift apart. `composer test:unit` now uses the root file. tests/phpunit-unit-only.xml is untouched — different purpose.
Contributor
Quality Report — ConductionNL/app-versions @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 24/24 | |||
| npm | ✅ | ✅ 282/282 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ❌ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-18 19:50 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Quality Report — ConductionNL/app-versions @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 24/24 | |||
| npm | ✅ | ✅ 282/282 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ❌ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-18 20:09 UTC
Download the full PDF report from the workflow artifacts.
The `openapi` job had never once passed — not on this branch, not on development, not anywhere in its history — and it could not, for a reason that had nothing to do with this repository: PHP Fatal error: license: Unable to convert EUPL-1.2 to SPDX identifier nextcloud/openapi-extractor maps an app's <licence> through a hardcoded match() in src/Helpers.php that accepts five short names and five SPDX spellings, and calls Logger::panic() — a fatal — on anything else. EUPL-1.2 is a valid SPDX identifier and is what every Conduction app ships under. Upstream `main` still had the same list when checked today. WE DO NOT GET TO WAIT ON UPSTREAM, AND WE DO NOT GET TO DISABLE THE CHECK. So the tool is patched at install time — cweagans/composer-patches plus vendor-bin/openapi-extractor/patches/, adding EUPL-1.1 and EUPL-1.2 to the allow-list. The patch is pinned against the exact version this repo already pinned (v1.8.7); if that pin moves and the patch stops applying, composer fails LOUDLY rather than silently reverting to the broken behaviour. The patch file carries its own rationale so it can be sent upstream unchanged. Two dishonest routes were rejected on the way: relabelling the licence to suit the tool, and marking the step continue-on-error so a broken job reports green. The second is worse than the red. WHAT THE TOOL THEN FOUND With the fatal gone the extractor reached this app's controllers for the first time and reported 50 errors about them. Those are ours: - 27 missing @return annotations - 11 missing @PARAM docs - 12 uses of getParam() where a typed method parameter belongs All 50 are fixed. The @return types are derived from each method's actual return statements, not invented: where a payload is a literal array the shape is written out, and where it comes from a service whose shape is not visible at the call site it is annotated as a generic map rather than a guessed set of keys. A wrong shape in a published API spec is worse than a loose one. The getParam() conversions are spelling changes, not behaviour changes — both forms resolve from the same merged request parameters — but two of them needed care and got it: - installVersion's `dryRun` KEEPS a null default. Null means "not supplied", which is what still lets `debug=1` imply a dry run; a '0' default would have silently killed that legacy path. - discover's parameter is still named `installedOnly`. The parameter NAME is the wire contract; only the local flag was renamed. Two methods carry a documented psalm suppression: appVersions() and installVersion() return a status that toHttpStatus() passes through from the source or installer, so psalm widens the inferred type to ~60 codes and cannot reconcile it with the documented set. Widening the annotation to match would make the spec describe sixty responses and document nothing. RESULT `composer openapi` exits 0 and generates a real 21-route spec. The committed openapi.json was a placeholder that still said "An example summary" — it had never been regenerated, because the tool always died first. It is regenerated here from a CLEAN run; the failing intermediate run's output was deliberately not committed. Verified locally: openapi 0 errors, psalm 0 errors, cs:check clean, npm build / test / lint all exit 0, and `php -l` over every non-vendor PHP file.
Contributor
Quality Report — ConductionNL/app-versions @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 24/24 | |||
| npm | ✅ | ✅ 282/282 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ❌ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-18 21:48 UTC
Download the full PDF report from the workflow artifacts.
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.
developmenthas been red since 2026-08-14. Six distinct causes, each reproduced and re-measured locally — full detail in the commit message.The headline one:
package.jsonpinnedvite@^8.2.1while@nextcloud/vite-config@2.5.2declares peervite@^7.1.10. npm resolution failed outright, so every npm-shaped job failed onInstall dependencies— eslint, stylelint, Frontend Build, Frontend Tests, License (npm), Security (npm), Hydra Gates, E2E and all six PHPUnit legs. None of them were reporting on what they measure.Pinning vite alone was not sufficient and the build proved it: at vite-config 2.5.2
vite-plugin-node-polyfillsinstalls nested, and rollup cannot resolveshims/globalfrom@vue/shared.npm run buildexited 1. 2.5.4 hoists it — build exit 1 → 0.The five
IConfig::setAppValue()psalm errors are suppressed, not rewritten. The block comment above them records that writing core-owned keys through the typed API leaves rows core can no longer update, which made every app installed through App Versions impossible to enable. Rewriting tosetValueStringwould reintroduce that bug.Two pre-existing issues fixed while here:
.npmrchadmin-release-age=0under a comment promising a 24h cooldown (zero is not a cooldown — the file read as hardened and enforced nothing), and stylelint's deprecatedword-break: break-word.Verified locally from a clean tree with the cooldown active:
npm ci, build, lint, stylelint, test,composer check:strictand psalm all exit 0.