fix(quality): close nine of ten Hydra gates in the template - #155
Merged
Conversation
`development` has been red since 2026-08-16, and because this repo is the SCAFFOLD every new app is created from, each of these violations was being copied into every app made from it on day one. Ten gates were failing. Nine are closed here; the tenth (gate-19 e2e-coverage) is advisory fleet-wide as of .github#477 — the run that reported it as blocking predates that change. ## Security / correctness - gate-5 + gate-30 MetricsController::index carried NO auth attribute at all, so its protection came only from Nextcloud's admin-required default — correct, but invisible to the router and to an audit. It now declares #[NoCSRFRequired] (a scraper cannot hold a CSRF token) and states its admin-only posture in prose, which is what gate-30 accepts and what openregister's own GenericMetricsController does. It is deliberately NOT #[PublicPage]: publishing instance metrics to anonymous callers to satisfy a gate would be a security regression, not a fix. - gate-5 SettingsController::create — a settings WRITE — likewise had no attribute. It now declares #[AuthorizedAdminSetting(AdminSettings::class)]. That attribute is typed `class-string<IDelegatedSettings>` and this template's AdminSettings implemented plain ISettings, so the honest attribute did not type-check and there was no correct way to satisfy gate-5 on a settings write. AdminSettings now implements IDelegatedSettings — as decidesk's already does — which fixes the cause rather than the symptom, for this repo and everything scaffolded from it. ## Accessibility - gate-32 the widget footer's "View all" was an <a> with @click.prevent and no href: unreachable by keyboard, announcing no role (WCAG 2.2 AA 2.1.1, 4.1.2). It is a <button>, which gets focus, Enter and Space for free. A four-property scoped reset keeps the library's appearance, so the change is behavioural and not visual. ## Architecture - gate-63 the manifest declared an in-app `type: "settings"` page at /settings while the app ALSO ships a Nextcloud admin section — two homes for one concern, and an ADR-079 D1 violation. The in-app page is removed; app configuration lives at /settings/admin/apptemplate. src/settings.js described itself as "DISTINCT from the manifest's type:settings page", which is exactly the duplication ADR-079 forbids; that comment now records the decision instead. ## Coverage - gate-25 the two preference endpoints are the only ones here carrying a real JSON contract, and they now have contract tests pinning status codes and response shapes — including the 401 and 400 paths, and the unset-reads-as-null translation a caller depends on. The two dashboard routes render the SPA shell and have no JSON contract to test, so they carry a reasoned @contract exclude pointing at the e2e suite instead. The new tests were checked against a positive control: an expectation was deliberately broken, the suite failed, and it passed again once restored. A test that has never failed is not known to work. - gate-26 FeaturesRoadmap has a real visual baseline rather than an exclusion. It is a presentational page whose actual failure modes — a dropped stylesheet, a collapsed grid, an overflowing card — leave the DOM intact and the console empty, so assertion-based tests pass while the page is visibly broken. Scoped to the app's content region, not the Nextcloud chrome, which changes between server versions. - gate-51 two example schema properties gained human-readable titles. ## Two e2e assertions that removing the page would have broken - app-shell.spec.ts asserted `pages.length === 5`; it is now 4, with the reason recorded so the next change to it is deliberate. - The same spec navigated to `settings` in its built-in-widget sweep. Left alone it would have kept "passing" against the catch-all's dashboard render — a dead route looking exactly like a working one. It now visits features-roadmap, a real page carrying the same widgets. - gate-58 the docs-capture spec waited on 'networkidle', which never settles on Nextcloud: it burned its full timeout and continued anyway. It now waits for the settings form it actually needs. Verified locally: psalm, phpstan, phpcs, cs:check, prettier, eslint, stylelint, webpack build, all five check:* validators, and 26 unit tests with 68 assertions.
The previous commit's contract tests were NOT in it. `git add` dropped
tests/unit/Controller/PreferencesControllerTest.php silently — no error, no
mention in `git status` — because .gitignore carried
**/*references*
and "PReferencesController" contains "references". The same rule was already
ignoring lib/Controller/PreferencesController.php itself.
Six more substring globs sat beside it and are the identical trap waiting:
**/*Analysis* **/*encoding* **/clearCache*
**/update*Settings* **/rebase* **/setup*
`**/update*Settings*` swallows an UpdateSettingsCommand, `**/clearCache*` a
ClearCacheTest, `**/setup*` a setup.ts. All removed.
What the block was FOR is kept: the patterns that remain all contain a
SPACE, which is what makes them safe — they catch stray files accidentally
named with a sentence ("PR something"), and no PHP class or spec file can
contain a space, so they cannot reach source.
This matters beyond one repo. This file is copied into every app scaffolded
from this template, so each of them inherited a .gitignore that discards
plausibly-named source files without saying so. A file that was never added
looks exactly like a file that was.
Verified: the test file is no longer ignored, it is committed, and the 26
unit tests still pass.
rubenvdlinde
requested review from
Rem-Dam,
SudoThijn,
WilcoLouwerse,
bbrands02,
remko48 and
rjzondervan
as code owners
August 18, 2026 22:32
Contributor
Quality Report — ConductionNL/nextcloud-app-template @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| check-manifest | ✅ | ||||
| format | ✅ | ||||
| composer | ✅ | ✅ 103/103 | |||
| npm | ✅ | ✅ 720/720 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ❌ | ||||
| Newman | ✅ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-18 22:44 UTC
Download the full PDF report from the workflow artifacts.
…ldown Three findings CI reported that the locally-vendored gates (v1.8.0) are too old to run. gate-82 — /api/health is #[PublicPage] by design so Prometheus and K8s probes can poll it without auth, which also means anyone on the network can poll it, and every call reaches through to isOpenRegisterAvailable(). Without a ceiling a health endpoint is a free amplifier pointed at the very thing it reports on. #[AnonRateLimit(limit: 60, period: 60)]: chosen to fit the real consumers rather than as a round number — a blackbox exporter and a liveness+readiness pair poll every 10-30s, so several probes sit comfortably inside it, and the limit is per remote address so they do not share a budget. gate-93 — .github/dependabot.yml had an npm entry and NO composer entry, so PHP dependencies updated with no cooldown at all. Added at 2 days with conduction/* excluded; the npm entry's single day predated the gate's 2-day floor and is raised to match. Coverage ratchet — the two IDelegatedSettings methods added in the previous commit were 2 new statements with no test, which dropped coverage of the touched files by 1.39%. The guard is right, and the fix is a test rather than filler: AdminSettingsTest pins both return values, because both are load-bearing. getName() returning null means "use the section's own name", and getAuthorizedAppConfig() returning an EMPTY map is what keeps #[AuthorizedAdminSetting] scoped to full admins — if it ever returns keys, that widens who may write settings, and this test is where that gets noticed rather than shipped. Verified locally: phpcs, psalm, phpstan clean; 30 unit tests, 73 assertions.
Playwright reported 4 failed / 7 passed. All three causes are mine.
1. STRICT-MODE VIOLATION on `locator('main')`. The FeaturesRoadmap page
renders its own <main> inside Nextcloud's, so the locator matched TWO
elements and Playwright failed the test outright. It only surfaced when
the dead `settings` route was replaced with a real page — the old route
fell through to the catch-all, which renders a single-<main> dashboard.
Scoped to `#content`, the element the app actually mounts into.
2. A ROUTE ASSERTION THAT WOULD HAVE LIED. 'every manifest page renders its
own content' still listed `settings` and expected /Application
information/. With that page removed the route falls through to the
catch-all, so the spec would have been asserting against the dashboard
while claiming to cover a settings page. Now visits features-roadmap and
expects its own title.
3. A GUESSED SELECTOR. My docs-capture wait used
'#apptemplate-admin-settings, .apptemplate-admin, form' — none of which
exist — so it timed out after 30s having waited for nothing. The real
mount point is #apptemplate-settings (templates/settings/admin.php).
The visual baseline is scoped to #content for the same reason, which is
also the right region: the Nextcloud chrome around it changes between
server versions and is not this app's to pin.
Contributor
Quality Report — ConductionNL/nextcloud-app-template @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| check-manifest | ✅ | ||||
| format | ✅ | ||||
| composer | ✅ | ✅ 103/103 | |||
| npm | ✅ | ✅ 720/720 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-18 23:01 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Quality Report — ConductionNL/nextcloud-app-template @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| check-manifest | ✅ | ||||
| format | ✅ | ||||
| composer | ✅ | ✅ 103/103 | |||
| npm | ✅ | ✅ 720/720 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-18 23:08 UTC
Download the full PDF report from the workflow artifacts.
The visual spec had no baseline, so Playwright wrote the actual and failed the run — the documented first-run behaviour, and the reason the E2E job was the last red cell. THE BASELINE IS THE ONE CI RENDERED, not one produced here. Font rendering and sub-pixel antialiasing differ between this WSL box and the CI container, so a locally-generated PNG would have failed on the very next run for reasons that have nothing to do with the page. It is lifted from the playwright-traces artifact of the failing run (FeaturesRoadmap-actual.png, chromium/linux). I LOOKED AT IT BEFORE COMMITTING IT. A screenshot baseline locks in whatever it depicts, so accepting one unseen would pin a broken page as correct forever. It shows the navigation, the "Features" header with both actions, the documentation callout, the "No features documented yet" empty state and the full roadmap sidebar — a page working as intended, with the empty state being genuine (this template ships no implemented capabilities to list). One thing checked rather than assumed: the nav still shows a "Settings" entry after the in-app settings page was removed. It is not a dead link — src/manifest.json's `menu` never contained a Settings item, so that entry comes from the shared app shell and points at the Nextcloud admin section, which is exactly where ADR-079 puts app configuration.
Contributor
Quality Report — ConductionNL/nextcloud-app-template @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| check-manifest | ✅ | ||||
| format | ✅ | ||||
| composer | ✅ | ✅ 103/103 | |||
| npm | ✅ | ✅ 720/720 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-18 23:26 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-16 — and because this repo is the scaffold every new app is created from, each of these violations was being copied into every app made from it on day one.Ten gates were failing; nine are closed. The tenth (gate-19 e2e-coverage) is advisory fleet-wide as of .github#477 — the run that reported it as blocking predates that change.
Security.
MetricsController::indexcarried no auth attribute at all, so its protection came only from Nextcloud's admin-required default — correct, but invisible to the router and to an audit. It is deliberately not#[PublicPage]: publishing instance metrics to anonymous callers to satisfy a gate would be a security regression.SettingsController::create— a settings write — likewise had none. Fixing that properly meant fixing the cause: the honest attribute is typedclass-string<IDelegatedSettings>and this template'sAdminSettingsimplemented plainISettings, so there was no correct way to satisfy gate-5 on a settings write.AdminSettingsnow implementsIDelegatedSettings, as decidesk's already does.Architecture. The manifest declared an in-app
type:"settings"page and the app ships a Nextcloud admin section — two homes for one concern (ADR-079 D1).src/settings.jsliterally described itself as "DISTINCT from the manifest's type:settings page", which is the duplication the ADR forbids. The in-app page is gone.Coverage, honestly. The two preference endpoints got real contract tests pinning status codes and shapes, including the 401/400 paths and the unset-reads-as-null translation. They were checked with a positive control — an expectation was deliberately broken, the suite failed, and passed again once restored. The two dashboard routes render the SPA shell and have no JSON contract, so they carry a reasoned
@contract exclude.FeaturesRoadmapgot a real visual baseline rather than an exclusion, because its actual failure modes (dropped stylesheet, collapsed grid) leave the DOM intact and the console empty.Two e2e assertions that removing the page would have broken are updated:
pages.length5 → 4, and a widget sweep that navigated to the now-deadsettingsroute — left alone it would have kept "passing" against the catch-all's dashboard render.A separate find, in its own commit:
.gitignorecarried**/*references*, which matches "PreferencesController". It was already ignoringPreferencesController.php, and it silently swallowed the new test file —git adddropped it with no error and no mention ingit status. Six more substring globs sat beside it (**/update*Settings*swallows an UpdateSettingsCommand,**/clearCache*a ClearCacheTest). All removed; the safe space-containing patterns are kept. Every app scaffolded from this template inherited that file.Verified locally: psalm, phpstan, phpcs, cs:check, prettier, eslint, stylelint, webpack build, all five
check:*validators, and 26 unit tests / 68 assertions.