fix(routes): stop routing health and metrics larpinq cannot serve, and clear development - #666
Merged
Merged
Conversation
…d clear development larpinq's development has been red all day on three checks. Two of them are mine, from #651/#653.⚠️ gate-14 (route-reachability): "2 unrouted method(s) or wrong-target route(s)". The gate was RIGHT, and the finding is a latent fatal. #653's class_exists() fallback reproduces Routes::standard()'s output locally so larpinq still routes when OpenRegister is absent — and it copied `metrics#index` and `health#index` along with the rest. larpinq ships neither controller: lib/Controller holds Characters, Dashboard, Events, Preferences, Settings and Setup, and nothing else. Those two work on a normal instance because the branch ABOVE the fallback calls Routes::standard(), and OpenRegister's AppHost aliases the generic Metrics/Health controllers onto larpinq's conventional class names. Verified: /apps/larpinq/api/health answers 200 application/json and /api/metrics answers 200 Prometheus text. But the fallback runs ONLY when OpenRegister is absent, and then nothing aliases anything — so those two routes would have pointed at classes that do not exist, and a request to either would fatal rather than 404. Removed, with the reasoning written where the next person will copy the block. gate-25 (contract-coverage): `dashboard#catchAll` is a new public endpoint with no test. Covered in DashboardControllerTest, asserting equality with page() rather than merely "returns a response" — catchAll() exists only to delegate, and a delegation that quietly rendered the wrong template would give every deep link a blank page while still answering 200. Same test now guards zaakafhandelapp#619 and openregister#3303. Also aligned the fallback's catch-all with Routes::standard()'s `(?!api/)` lookahead. Without it a bare `.+` matches slashes and swallows unmatched `api/...` paths, answering the SPA shell at HTTP 200 — that is what ate all seventeen of zaakafhandelapp's ZGW resource routes (#619) and it would have behaved identically here whenever the fallback was in use. Third check, and NOT mine: prettier failed on tests/e2e/spec-coverage/settings-roadmap.spec.ts, last touched by #663. One statement that fits on a line after the hash was removed from it. Fixed here rather than left, since it keeps development red for everyone. php -l clean, prettier --check on the FULL glob clean, eslint 0 errors, DashboardControllerTest 5/5. The suite reports 5 errors locally, all `Class "Symfony\Component\HttpFoundation\HeaderUtils" not found` in the Characters/Events PDF download tests — a dev dependency missing from this checkout, present in CI, and untouched by this change.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| await expect(page).toHaveURL( | ||
| new RegExp(`${route.replace(/\//g, '\\/')}$`), | ||
| ) | ||
| await expect(page).toHaveURL(new RegExp(`${route.replace(/\//g, '\\/')}$`)) |
Contributor
Quality Report — ConductionNL/larpinq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-l10n-js | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| composer | ✅ | ✅ 105/105 | |||
| npm | ✅ | ✅ 525/525 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-09-01 19:47 UTC
Download the full PDF report from the workflow artifacts.
added 2 commits
September 1, 2026 22:02
…an see them Follow-up on the same branch. gate-14 (route-reachability) went on failing with the identical "2 unrouted method(s) or wrong-target route(s)" after the routes were removed from the fallback, and the reason turned out to be two separate things.⚠️ FIRST, THE GATE WAS READING MY COMMENT. I removed the two routes and explained the removal in a comment that spelled their slugs out (`<controller>` + `#` + `<method>`). gate-14 matches that shape anywhere in appinfo/routes.php, comments included, so it kept reporting both — pointing at controller files that do not exist — long after the routes were gone. The comment is reworded and now warns about exactly this. SECOND, AND THE REAL FINDING. With the comment cleaned the gate still reported both, because it knows Routes::standard()'s canonical set and attributes those routes to any app that calls it. It exempts them only when the app shows, in its own code, that it adopts the generic behind them: either `\OCA\OpenRegister\AppHost\Bootstrap::register()`, or a registerService() of the generic controller in the same file that names it. larpinq did neither. Its /api/health and /api/metrics answer 200 on a normal instance purely because the AppHost stands in under larpinq's conventional class names — a real dependency that nothing in this repository stated. So it is stated now: Application::register() registers GenericHealthController and GenericMetricsController under OCA\Larpinq\Controller\{Health,Metrics}Controller, each behind a class_exists() guard so an instance without OpenRegister degrades instead of fataling — the same discipline as the autoloader prelude above it. Registered individually rather than adopting Bootstrap::register() wholesale, which would also alias dashboard, settings, preferences, repair steps and sections onto generics that do NOT match the controllers larpinq actually ships. Same reasoning and the same shape as shillinq's registerAppHostGenerics(). Gate run on this tree: gate-14 PASS, gate-25 PASS (was FAIL, FAIL). Two gates still fail LOCALLY — gate-22 and gate-53, both on `/pages/25/type` — because the vendored hydra-gates schema here predates the `flow` page type. CI reported neither, so its schema is newer; not touched. php -l clean, phpcs 0 errors, DashboardControllerTest 5/5.
phpmd's ExcessiveMethodLength fires at 100 lines and register() landed on exactly 100 with the two registrations inline. Extracted to its own method, which is where procest and shillinq both put theirs — the gates push this decomposition, and the previous commit's comment already pointed at shillinq's registerAppHostGenerics() as the model. No behaviour change. phpmd clean, phpcs 0 errors, gate-14 and gate-25 still PASS, DashboardControllerTest 5/5.
Contributor
Quality Report — ConductionNL/larpinq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ❌ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-l10n-js | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| composer | ✅ | ✅ 105/105 | |||
| npm | ✅ | ✅ 525/525 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ❌ | ||||
| Newman | ❌ | ||||
| Playwright | ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-01 20:14 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Quality Report — ConductionNL/larpinq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-l10n-js | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| composer | ✅ | ✅ 105/105 | |||
| npm | ✅ | ✅ 525/525 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ❌ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-01 20:28 UTC
Download the full PDF report from the workflow artifacts.
This was referenced Sep 1, 2026
rubenvdlinde
added a commit
that referenced
this pull request
Sep 1, 2026
…ctory (#669) phpunit.xml collects ./tests/unit. A second ./tests/Unit existed beside it, holding Service/DemoDataServiceTest.php. On a case-sensitive filesystem those are different directories, so that file was never collected and its 7 tests have never run in CI. Nothing about the file was wrong, which is why this went unnoticed: it is green the moment it is collected. Collected tests go from 239 to 246. Found while working #667 (closed, superseded by #666); this is the one finding there that #666 does not cover. Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
This was referenced Sep 1, 2026
rubenvdlinde
added a commit
that referenced
this pull request
Sep 1, 2026
) * style(manifest): run prettier over src/manifest.json Formatting only: the JSON parses to an identical structure before and after. Its own commit because it is large and mechanical. The file was never prettier-clean, and Frontend Check (format) is scoped to changed FILES rather than changed lines, so any edit to the manifest fails that job until it is formatted once. * fix(observability): declare the health checks the endpoint reports #666 wired /api/health and /api/metrics to OpenRegister's generic controllers, which was the missing binding. This adds what they read. HealthCheckExecutor iterates the manifest's observability descriptors. With no observability block it iterates an empty list, so /api/health answers 200 with an empty `checks` object — it would report `ok` with the database down, which is the one thing a health endpoint must never do. Two checks, matching shillinq's: `database` (critical) and `openregister` (degraded). Metrics stay the implicit larpinq_info and larpinq_up gauges the engine adds. Also widens the URL escape in settings-roadmap.spec.ts. CodeQL reports the slash-only slash-escape as js/incomplete-sanitization, high: it leaves `.`, `?`, `+`, `(` and backslash live in the pattern. Nothing is exploitable — every route in that file is a literal — but a route containing a `.` would silently match more than it names. Verified against a live instance while the binding was in place: /api/health 200 returning exactly these two checks, both ok, and /api/metrics 401 anonymous.
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
2 participants
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.
fix(routes): stop routing health and metrics larpinq cannot serve, and clear development
larpinq's development has been red all day on three checks. Two of them are
mine, from #651/#653.
route(s)". The gate was RIGHT, and the finding is a latent fatal. #653's
class_exists() fallback reproduces Routes::standard()'s output locally so
larpinq still routes when OpenRegister is absent — and it copied
metrics#indexandhealth#indexalong with the rest. larpinq shipsneither controller: lib/Controller holds Characters, Dashboard, Events,
Preferences, Settings and Setup, and nothing else.
Those two work on a normal instance because the branch ABOVE the fallback
calls Routes::standard(), and OpenRegister's AppHost aliases the generic
Metrics/Health controllers onto larpinq's conventional class names.
Verified: /apps/larpinq/api/health answers 200 application/json and
/api/metrics answers 200 Prometheus text. But the fallback runs ONLY when
OpenRegister is absent, and then nothing aliases anything — so those two
routes would have pointed at classes that do not exist, and a request to
either would fatal rather than 404. Removed, with the reasoning written
where the next person will copy the block.
gate-25 (contract-coverage):
dashboard#catchAllis a new public endpointwith no test. Covered in DashboardControllerTest, asserting equality with
page() rather than merely "returns a response" — catchAll() exists only to
delegate, and a delegation that quietly rendered the wrong template would
give every deep link a blank page while still answering 200. Same test now
guards zaakafhandelapp#619 and openregister#3303.
Also aligned the fallback's catch-all with Routes::standard()'s
(?!api/)lookahead. Without it a bare
.+matches slashes and swallows unmatchedapi/...paths, answering the SPA shell at HTTP 200 — that is what ate allseventeen of zaakafhandelapp's ZGW resource routes (#619) and it would have
behaved identically here whenever the fallback was in use.
Third check, and NOT mine: prettier failed on
tests/e2e/spec-coverage/settings-roadmap.spec.ts, last touched by #663.
One statement that fits on a line after the hash was removed from it.
Fixed here rather than left, since it keeps development red for everyone.
php -l clean, prettier --check on the FULL glob clean, eslint 0 errors,
DashboardControllerTest 5/5. The suite reports 5 errors locally, all
Class "Symfony\Component\HttpFoundation\HeaderUtils" not foundin theCharacters/Events PDF download tests — a dev dependency missing from this
checkout, present in CI, and untouched by this change.