fix(observability): declare the health checks the endpoint reports - #671
Merged
Conversation
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.
#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.
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 22:11 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
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.
#666 wired
/api/healthand/api/metricsto OpenRegister's generic controllers, which was the binding larpinq was missing. This adds the thing those controllers read.HealthCheckExecutoriterates the manifest'sobservabilitydescriptors. With noobservabilityblock it iterates an empty list, so/api/healthanswers 200 with an emptychecksobject — it would reportokwith the database down, which is the one thing a health endpoint must never do.Two checks, matching shillinq's:
database(critical) andopenregister(degraded). Metrics stay the implicitlarpinq_info/larpinq_upgauges the engine adds.Verified against a live instance, while an equivalent binding was in place:
Those are exactly the two checks this block declares — that response is what the block buys.
Also: a CodeQL high finding.
settings-roadmap.spec.tsbuilds its URL pattern with a slash-only escape, which CodeQL reports asjs/incomplete-sanitization— it leaves.,?,+,(and backslash live in the pattern. Nothing is exploitable, since every route in that file is a literal, but a route containing a.would silently match more than it names. Widened to the full metacharacter set.On the separate prettier commit:
src/manifest.jsonwas never prettier-clean, andFrontend 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. Kept as its own commit, and the JSON parses to an identical structure before and after.This replaces #665, which I have closed. That PR bound the routes via
AppHost\Bootstrap::register(); #666 landed a hand-rolled alias doing the same job while it was open, so only this part is still additive.