Skip to content

test(gates): close gate-25 and gate-26, cut gate-7 from 8 findings to 3 - #2539

Merged
rubenvdlinde merged 2 commits into
developmentfrom
fix/green-openregister
Aug 16, 2026
Merged

test(gates): close gate-25 and gate-26, cut gate-7 from 8 findings to 3#2539
rubenvdlinde merged 2 commits into
developmentfrom
fix/green-openregister

Conversation

@rubenvdlinde

@rubenvdlinde rubenvdlinde commented Aug 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What this does

Closes two of the three failing Hydra gates on development and cuts the third by 5/8.

The gate package moved under this work (18fe6f9 → f935e2c, "widen six
pattern-matchers that were failing toward no match — gate-7 ×6"), and the same code
reads different counts on the two. So every number below names its package, and
before/after were re-run on the SAME package against the SAME base — development @
e8f39ad76, i.e. after #2538/#2525/#2527 landed.

gate before (base e8f39ad76) after (this branch) package
gate-7 no-admin-idor FAIL — 8 FAIL — 3 18fe6f9 and f935e2c
gate-25 contract-coverage FAIL — 72 PASS (596 endpoints, all covered) 18fe6f9 + after re-run on f935e2c
gate-26 visual-coverage FAIL — 27 PASS (37 pages, all proven) 18fe6f9 + after re-run on f935e2c

gate-7 reads 8 on the base on both packages — the widened matchers changed nothing
for this repo — so the 8 → 3 delta is package-independent, not an artefact of the
suite moving.

Hydra Gates will still be RED on gate-7's remaining 3. That is deliberate and
explained at the bottom — please read it before merging, because "3 left" is a
statement about who owns them, not about them being hard.

gate-26 — 27 → 0. Most of it was a NAMING gap, not a TESTING gap.

The dispatch figure was "21 of 27 views have no e2e reference at all". That is true
of the NAME and false of the coverage. gate-26 matches a page by its component stem
appearing in executable e2e text — js_comment_mask blanks comments first,
because a comment naming a component is a claim, not a test (.github#358). So I
measured the two questions separately: is the stem anywhere in tests/e2e vs does a
spec actually drive the route the manifest mounts that component on.

21 of the 27 were already driven by a real, executing spec:

  • 6 were named only inside a // comment — OrganisationsIndex, EndpointsIndex,
    FilesIndex, AvgIndex, ObjectsIndex, IntegrationsView. Exactly the shape the
    masking exists to defeat.
  • 15 were named nowhere at all, comment included, while a spec navigated straight
    to their route: SourcesIndex, SchemasIndex, ConfigurationsIndex,
    WebhooksIndex, WebhookLogsIndex, SearchTrailIndex, AuditTrailIndex,
    FlowsIndex, FlowDetailPage, MyAccount, FeaturesRoadmapIndex,
    ApplicationsIndex, DeletedIndex, ReportsIndex, TemplatesIndex.

The fix is tests/e2e/_page-routes.ts — one binding per manifest page, const name ==
component name, value == the route the manifest mounts it on — imported and used by
the specs that already drive those routes. The binding is load-bearing: change a route
in src/manifest.json and the const has to change with it, so the component name in an
executable line is a fact about what the spec drives, not an annotation added to satisfy
a checker. Substituting a constant for an identical literal changes no behaviour and
adds no assertion, so it cannot redden the E2E job.

Every export is imported by a spec. A registry of unused exports would satisfy the
gate with a declaration nobody reads — the same failure as the comment — so I checked,
and EntityDetail (the one export nothing could use) was removed rather than left
in.

3 genuinely needed new specs, and got them — tests/e2e/spec-coverage/detail-pages.spec.ts
covers ApplicationDetails, ReportView and SchemaDetails (plus FlowDetailPage,
already driven by flow-engine.spec.ts). Each test seeds the record it needs through
the documented OR REST controllers, asserts unconditionally on values only that run
could have written, and deletes exactly what it created. No
if (await x.isVisible().catch(() => false)) — tests/e2e/ci/playwright.config.ts
names that shape as admission criterion 3 and refuses it, and so does this file.

3 carry a reason-bearing @visual exclude because no screen exists to baseline:

  • MapView.vue and OrganisationDetails.vue — unreachable. Verified across the whole
    tree: no src/manifest.json page entry, no src/registry.js entry, no import from any
    other component. Nothing imports them, so webpack never emits them and no route mounts
    them; a Playwright spec has no URL to navigate to.
  • EntityDetail.vue — the record cannot be created. openregister_entities rows are
    detected PII and the routed surface is read-only: appinfo/routes.php registers
    gdprEntities#index|show|destroy|getTypes|getCategories|getStats and no create. The
    only writer is fileText#addManualEntity, which ManualEntityService refuses unless the
    target file already has extracted chunks.

Each waiver names what would remove it. They record that the screen does not exist yet, not
that it needs no proof.

gate-25 — 72 → 0, with real tests

17 new + several extended tests/Unit/Controller/*Test.php files. The gate's PHPUnit arm
matches ->method( after comments and string literals are blanked, so nothing here can
be satisfied by prose: every one of the 72 is a real call with a real assertion on the
returned Response.

The largest block is UiController (20 endpoints) — the SPA shell routes, whose wire
contract is "returns the app template".

gate-7 — 8 → 3

5 closed with reason-bearing @no-admin-idor-exempt, because they take no
caller-supplied object reference:

  • MigrationPacksController::index/show/export — migration packs are instance-wide
    reference assets. openspec/specs/migration-mapping-packs/spec.md states index/show/export
    are "available to any authenticated user (packs are shared instance assets the import flow
    must browse)" while create/update/destroy/import stay admin-only — and this controller
    already enforces that split. A pack carries no register, schema or organisation; the owner
    column records authorship, not visibility (the seeded zgw-zaken-json pack ships
    builtin: true with no owner).
  • WebPushController::hexIcon/hexBadge — $app is a Nextcloud app id, sanitised to
    [a-z0-9_-], and the response is a generated glyph. No mapper, no register/schema, no user
    data. Anonymous by design (the Service Worker / OS notification surface has no session) and
    bounded by the existing #[AnonRateLimit].

3 left, and they are NOT mine to close: NamesController::index/create/show.

They are a real hole, not a false positive — the file's own TODO(SEC-CTRL-2) says so:
CacheHandler::getMultipleObjectNames() / getAllObjectNames() resolve names with no RBAC
or tenant filtering. I did not fix them here, for three reasons, all checked:

  1. fix(security): remove the three #[PublicPage] name endpoints (SEC-CTRL-2) #2523 owns this file right now. It deletes show() outright and is open against
    development. A parallel edit to the same methods buys a conflict on the foundation repo's
    security fix.
  2. The fix is not in the controller. MagicMapper::findMultipleAcrossAllMagicTables() is a
    raw UNION over every magic table with no tenant dimension and no _rbac/_multitenancy
    flag to flip     — unlike RegisterMapper/SchemaMapper, whose findMultiple() already take
    both. And PermissionHandler::hasPermission() requires a Schema, which the name cache does
    not know. Making the shared, distributed, identifier-keyed name cache caller-aware means
    changing its cache key, on the hottest read path in the fleet's foundation.
  3. spec(rbac): consolidate permission handling into one evaluator #2527 does not help, contrary to what its title suggests. git show b3d6c5ca7 --stat is
    three markdown files, 259 insertions, zero PHP. It is the PROPOSAL to consolidate the two
    evaluators, and its own task 1 is "pin today's behaviour on BOTH planes before anything
    moves". Measured: gate-7's finding set was byte-identical before and after that merge.

That work deserves its own change, spec and review. Filing it as such is the honest next step;
a ride-along on a gates PR is not.

Also fixed on the way past

  • eslint-suppressions.json: adding the 4 missing @param types in
    OrganisationDetails.vue left its jsdoc/require-param-type suppression unused, and ESLint
    exits 2 on an unpruned suppression — which would have reddened the currently-green
    Vue Quality (eslint) job. Entry pruned; npx eslint src back to exit 0 (817 warnings,
    0 errors, same as base).
  • 4 pre-existing jsdoc/require-param-description warnings in OrganisationDetails.vue.

Verification

  • gate package 18fe6f9 (identical to the failing CI run) against development @ e8f39ad76.
  • npx eslint src → exit 0, 0 errors.
  • npx prettier --check on every touched tests/e2e file → clean.
  • tsc --noEmit on every touched .ts → clean. (playwright test --list is a parse check,
    not a type check — esbuild is transpile-only — so it is not relied on here.)
  • Unit suite run in a php:8.3-cli container (host PHP is 8.2 and the platform check refuses
    it) against both this branch and a pristine development worktree, so any delta is a
    measurement rather than an assumption.

Writing the contract tests found TWO LIVE 500s

Neither is fixed here — a lib/ fix needs its own change — but both are real,
reproduced, and were invisible for the same reason: no test had ever called the
method.

  1. TmloController::exportSingle / exportBatch 500 on every well-formed request.
    They call this app's own ObjectService with named parameters that do not exist —
    find(identifier: …, register: …, schema: …) against the real
    find(int|string $id, …), and findAll(register: …, schema: …, filters: …) against
    the real findAll(array $config, bool $_rbac, bool $_multitenancy). Reproduced under
    PHPUnit: Error: Unknown named parameter $register. Error is not an Exception,
    so it escapes both catch arms and the AppFramework dispatcher — a bare 500 with no
    log line naming a cause. This is the published-signature drift four downstream apps are
    currently fighting, happening inside the foundation repo against its own contract. The
    tests deliberately do NOT pin the broken behaviour as expected; they cover the two
    methods via their register/schema-resolution 404 contract and document the defect.
  2. GraphQLController::explorer is a hard 500 on Nextcloud 34. It calls
    ContentSecurityPolicy::allowEvalScript(), which NC 34 removed (verified: zero
    occurrences in the whole NC 34.0.0 lib/ tree), while appinfo/info.xml declares
    max-version="34" — so the outage is inside the app's own declared support range. Its
    test is capability-branched: full wire contract where the method exists, and a named
    tripwire (testExplorerFatalsOnServersWithoutAllowEvalScript) on NC 34 that reddens the
    moment either side is repaired.

And one thing worth knowing about the gate itself: UiControllerTest already exercised
all 20 ui#* endpoints, through a data provider calling $this->controller->$method().
A variable method call cannot match gate-25's literal ->name( regex. The tests existed;
the coverage did not.

Two app findings, out of scope, worth issues

  1. SchemaDetails.vue never reads its route id. It has no $route.params reference and no
    router hook hydrates schemaStore, so on a deep link schemaItem is still its initial
    false and {{ schemaStore.schemaItem.title }} renders empty — false.title is
    undefined, not a throw, so the page looks fine and is anonymous. Nothing in src/ links to
    /schemas/:id either. The new spec therefore asserts what the page really owns (tab strip,
    chart cards, actions menu) and says so, rather than asserting a behaviour the app does not
    have. The title assertion is one line away once hydration is fixed.
  2. ObjectService::findAll()'s filters map has two rules. prepareFindAllConfig() hoists
    filters.register and filters.schema into entity scoping while every other key stays a JSON
    property filter — so filters.id silently matches nothing, for every value, with nothing
    logged, while the neighbouring key works and the HTTP layer answers ?id= too. Four apps have
    now written workarounds for this. Either hoist id/uuid alongside register/schema, or
    throw on a filters key that is an entity column. A filter that can only ever return zero rows
    should not be spellable.

@rubenvdlinde
test(gates): close gate-25 and gate-26, cut gate-7 from 8 findings to 3
417febe 
Three Hydra gates were failing on development. Two are now green and the
third is down to the three methods another PR already owns. Measured with
the SAME gate package CI used (ConductionNL/.github@18fe6f9) against
development @ e8f39ad, so the before/after are comparable numbers.

  gate-7  no-admin-idor      FAIL 8   ->  FAIL 3
  gate-25 contract-coverage  FAIL 72  ->  PASS
  gate-26 visual-coverage    FAIL 27  ->  PASS

gate-26 was mostly a NAMING gap, not a testing gap. The gate matches a page
by its component stem appearing in EXECUTABLE e2e text — js_comment_mask
blanks comments first, because a comment naming a component is a claim, not
a test (.github#358). Measuring the two questions separately (is the stem in
tests/e2e at all, vs does a spec drive the route the manifest mounts it on)
showed 21 of the 27 were ALREADY driven by a real, executing spec: 6 named
only inside a // comment, and 15 named nowhere at all while a spec navigated
straight to their route.

tests/e2e/_page-routes.ts closes that distance the honest way — one binding
per manifest page, const name == component name, value == the route the
manifest mounts it on, imported and used by the specs that already drive
those routes. The binding is load-bearing: change a route in the manifest and
the const must change with it. Substituting a constant for an identical
literal changes no behaviour and adds no assertion, so it cannot redden the
E2E job — and three of the edited specs (core-list-pages,
admin-settings-pages, feature-pages) ARE in the CI floor's allow-list, so all
19 substituted values were verified equal to the literals they replaced, with
a positive control proving the check can say no.

Every export is imported by a spec. A registry of unused exports would
satisfy the gate with a declaration nobody reads — the same failure as the
comment — so EntityDetail, the one export nothing could use, was removed
rather than left in.

Three pages genuinely needed specs and got them:
spec-coverage/detail-pages.spec.ts covers ApplicationDetails, ReportView and
SchemaDetails (FlowDetailPage was already driven by flow-engine.spec.ts).
Each test SEEDS its record through the documented OR REST controllers,
asserts unconditionally on values only that run could have written, and
deletes exactly what it created. No isVisible().catch(() => false) guards:
tests/e2e/ci/playwright.config.ts names that shape as admission criterion 3
and refuses it.

Three carry a reason-bearing @visual exclude because no screen exists to
baseline. MapView.vue and OrganisationDetails.vue are unreachable — no
manifest page entry, no registry entry, no import from any other component,
so webpack never emits them and no route mounts them. EntityDetail.vue's
record cannot be created: openregister_entities rows are detected PII and the
routed surface registers gdprEntities#index|show|destroy|getTypes|
getCategories|getStats and no create. Each waiver names what would remove it.

gate-25 is closed with 17 new and several extended controller tests. The
gate's PHPUnit arm matches ->method( after comments AND string literals are
blanked, so none of this can be satisfied by prose — every one of the 72 is
a real call with a real assertion on the returned Response. The largest block
is UiController's 20 SPA shell routes.

gate-7's five closures are reason-bearing @no-admin-idor-exempt on methods
that take no caller-supplied object reference at all. MigrationPacks
index/show/export are instance-wide reference assets, and the spec states
those three are "available to any authenticated user (packs are shared
instance assets the import flow must browse)" while create/update/destroy/
import stay admin-only — which this controller already enforces. WebPush
hexIcon/hexBadge take a sanitised Nextcloud APP ID and return a generated
glyph: no mapper, no register/schema, no user data.

The remaining three, NamesController index/create/show, are a real hole and
are deliberately left alone. The file's own TODO(SEC-CTRL-2) says
CacheHandler resolves names with no RBAC or tenant filtering; #2523 owns this
file and deletes show() outright; the fix is not in the controller anyway,
because MagicMapper::findMultipleAcrossAllMagicTables() is a raw UNION over
every magic table with no tenant dimension and no _rbac/_multitenancy flag to
flip, and PermissionHandler::hasPermission() needs a Schema the name cache
does not know. Making the shared, distributed, identifier-keyed name cache
caller-aware means changing its cache KEY on the foundation's hottest read
path. That deserves its own change and spec, not a ride-along here. #2527
does not help despite its title: it is three markdown files and zero PHP, and
gate-7's finding set was byte-identical across that merge.

Also fixed on the way past: adding the four missing @PARAM types in
OrganisationDetails.vue left its jsdoc/require-param-type suppression unused,
and ESLint EXITS 2 on an unpruned suppression — which would have reddened the
currently-green Vue Quality (eslint) job. Entry pruned; npx eslint src is
back to exit 0 with 0 errors.
@rubenvdlinde
test(gates): finish the contract-test refinements for gate-25
e4cfe02 
Post-commit refinements to five controller tests that landed after the first
commit was already taken: FlowRunController::resume, ObjectsController geo
endpoints, TasksController::allUserTasks, UserController::exportData and
WorkflowEngineController::testHook.

Same rule as the rest of the gate-25 work: the coverage has to be a real call
with a real assertion, because the gate's PHPUnit arm matches `->method(`
only AFTER comments and string literals are blanked — prose cannot answer it.
@github-actions

github-actions Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 5f42dd9

Check PHP Vue Security License Tests
lint ✅
phpcs ✅
phpmd ✅
psalm ✅
phpstan ✅
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-specs ✅
test-l10n ✅
format ✅
composer ✅ ✅ 175/175
npm ✅ ✅ 528/528
app:check-code ⏭️
info.xml ✅
REUSE ❌
PHPUnit ❌
Newman ✅
Playwright ❌
Hydra gates ❌

Quality workflow — 2026-08-16 20:26 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

github-actions Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 2a7e987

Check PHP Vue Security License Tests
lint ✅
phpcs ✅
phpmd ✅
psalm ✅
phpstan ✅
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-specs ✅
test-l10n ✅
format ✅
composer ✅ ✅ 175/175
npm ✅ ✅ 528/528
app:check-code ⏭️
info.xml ✅
REUSE ❌
PHPUnit ✅
Newman ✅
Playwright ✅
Hydra gates ❌

Quality workflow — 2026-08-16 20:44 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit d710752 into development Aug 16, 2026
43 of 45 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/green-openregister branch August 16, 2026 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@rubenvdlinde