Skip to content

fix(apphost): implement the health and metrics endpoints larpinq already routed - #667

Closed
rubenvdlinde wants to merge 3 commits into
developmentfrom
fix/format-settings-roadmap-spec
Closed

fix(apphost): implement the health and metrics endpoints larpinq already routed#667
rubenvdlinde wants to merge 3 commits into
developmentfrom
fix/format-settings-roadmap-spec

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

What

larpinq's development was red on three jobs: Frontend Check (format), Hydra Gates, and the Quality Report that aggregates them. This clears all three.

The two dead routes

\OCA\OpenRegister\AppHost\Routes::standard() builds larpinq's route table and emits health#index and metrics#index for every adopter. larpinq has routed both since it adopted the builder and implemented neither, so /api/health and /api/metrics resolved to a controller class that does not exist. gate-14 route-reachability named them controller-class-not-found.

Ported from pipelinq, which carries the same pair. The constraint in its header travelled with it and is worth restating: neither class may name an OpenRegister symbol in any position the autoloader resolves. Nextcloud reflects every file in lib/Controller/ while matching a route, so a single unresolvable parent makes every larpinq route return 500, not only the one being matched. The collaborators are pulled from the container by FQCN string at dispatch time instead.

Behaviour follows ADR-006. Health answers HTTP 200 degraded when the engine is absent, because a probe that fails when its dependency is missing tells monitoring nothing about the app it was meant to report on. Metrics degrades to 503 rather than 500, and stays admin-only through the deliberate absence of #[NoAdminRequired].

openspec/specs/apphost-adoption/spec.md is new and owns those properties. larpinq had no home for them, and apphost-adoption is the capability name the fleet already uses.

The catch-all had no contract test

gate-25 contract-coverage flagged dashboard#catchAll on /{path}. That route is what makes a deep link survive a reload, and it is the route this app lacked when it was the only hash-mode app in the fleet whose sub-paths returned 404.

The assertion that matters is not that it answers. It is that it answers with the same shell as page(). A catch-all returning anything else would still route, still return 200, and still leave every deep link broken.

Tests that could never run

phpunit.xml collects ./tests/unit. A second ./tests/Unit existed alongside it, and on a case-sensitive filesystem the uppercase directory is simply not collected, so DemoDataServiceTest had never executed. It is moved into tests/unit/Service. Collected tests go from 238 to 251: seven recovered, plus six new.

Formatting

The route-matching change in #663 left a call wrapped across three lines where prettier wants one, which is what reddened Frontend Check (format).

Verification, locally

  • hydra-gates --base origin/development: gate-14 PASS, gate-25 PASS, 0 FAIL (both reproduced red first, from the same runner)
  • phpcs --standard=phpcs.xml: exit 0, no errors and no warnings
  • psalm: exit 0. phpstan analyse lib: no errors
  • phpunit --testsuite "Unit Tests": 250 tests. The 5 errors are pre-existing and environmental, all in EventsControllerTest on a missing Symfony\Component\HttpFoundation\HeaderUtils in this local vendor tree; larpinq's PHPUnit job is green in CI and untouched by this change
  • prettier --check and eslint: clean; playwright test --list collects 198 tests

Conduction Release Bot added 2 commits September 1, 2026 22:20
The route-matching change in #663 left this call wrapped across three lines
where prettier wants one, which reddened Frontend Check (format) on
development. No behaviour change.
…ady routed

Routes::standard() emits health#index and metrics#index for every AppHost
adopter, so larpinq has routed both since it adopted the builder and
implemented neither. Both URLs resolved to a controller class that does not
exist. gate-14 route-reachability named them controller-class-not-found.

Ported from pipelinq, which carries the same pair, including the constraint in
its header: neither class may name an OpenRegister symbol in any position the
autoloader resolves. Nextcloud reflects every file in lib/Controller/ while
matching a route, so one unresolvable parent makes EVERY larpinq route 500,
not just the one being matched. The collaborators are pulled from the
container by FQCN string at dispatch time instead.

Health answers HTTP 200 degraded when the engine is absent, because a probe
that fails when its dependency is missing tells monitoring nothing about the
app. Metrics degrades to 503 rather than 500 and stays admin-only through the
deliberate absence of NoAdminRequired.

openspec/specs/apphost-adoption/spec.md is new and owns all three properties.
larpinq had no home for them; the fleet convention is this capability name.

Also fixes gate-25 contract-coverage: dashboard#catchAll had no contract test.
The assertion that matters is not that it answers, but that it answers with
the SAME shell as page(), since a catch-all returning anything else still
routes, still returns 200, and still leaves every deep link broken.

TESTS THAT COULD NEVER RUN. phpunit.xml collects ./tests/unit, and a second
./tests/Unit existed alongside it. On a case-sensitive filesystem the
uppercase directory is simply not collected, so DemoDataServiceTest had never
executed. Moved into tests/unit/Service; the suite goes from 238 collected
tests to 251.
Comment thread tests/e2e/spec-coverage/settings-roadmap.spec.ts Fixed
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/larpinq @ 713a028

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:41 UTC

Download the full PDF report from the workflow artifacts.

…ed regex

CodeQL flagged this as a high-severity js/incomplete-sanitization: the regex
escaped only '/', leaving backslashes and every other metacharacter live. It is
also a latent false pass, since '.' matching any character would let a redirect
to a similar-looking path satisfy the assertion.

Reading URL.pathname and calling endsWith says exactly what the comment above
it already claimed, with nothing to escape.
@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Closing: superseded by #666, which is right where this is wrong.

I implemented HealthController and MetricsController on the premise that larpinq routed two endpoints it could not serve. #666 verified the part I did not: on a normal instance OpenRegister's AppHost aliases its generic Metrics/Health controllers onto larpinq's conventional class names, and /apps/larpinq/api/health already answers 200 application/json. The routes only pointed at nothing inside the OpenRegister-absent fallback, which #666 removes.

Writing the leaf classes would have made it worse, not better: a leaf controller in lib/Controller/ SUPPRESSES the generic one the AppHost aliases, so this PR would have replaced two working endpoints with two that degrade whenever the engine is not resolvable.

#666 also covers the other two things this touched, the settings-roadmap URL assertion and a catchAll contract test.

One finding here is not in #666 and I am re-opening it on its own: phpunit.xml collects ./tests/unit, and ./tests/Unit/Service/DemoDataServiceTest.php still exists beside it. On a case-sensitive filesystem that directory is never collected, so those 7 tests have never run.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants