feat: declarative annotation platform — lifecycle / aggregations / calculations / notifications by rubenvdlinde · Pull Request #1357 · ConductionNL/openregister

rubenvdlinde · 2026-04-29T08:52:55Z

Summary

End-to-end implementation of four declarative schema annotations + their runtime engines, plus the platform-capabilities catalog updates.

All four annotations were verified live in the dev container against decidesk's Meeting + ActionItem schemas (see commit messages for the smoke-test transcripts).

Annotations shipped

Annotation	Storage	Surfaces
`x-openregister-lifecycle`	`configuration['x-openregister-lifecycle']`	`POST /api/objects/{id}/transition`, `GET /api/objects/{id}/available-actions`, `ObjectTransitionedEvent`, schema-save validator, ObjectUpdating listener (StoppableEvent rejection), ObjectCreating listener (initial-state default), `LifecycleGuardInterface` for app-side authorisation
`x-openregister-aggregations`	`configuration['x-openregister-aggregations']`	`GET /api/objects/aggregations/{register}/{schema}/{name}` returns `{name, metric, field, value
`x-openregister-calculations`	`configuration['x-openregister-calculations']`	JSON-shaped expression AST (concat / if / arithmetic / comparison / date diff). `materialise: true` evaluates on save and persists the value; aggregations can target it. ObjectCreating + ObjectUpdating listeners. Cycle detection at schema-save.
`x-openregister-notifications`	`configuration['x-openregister-notifications']`	Subscribes to ObjectCreatedEvent / ObjectUpdatedEvent / ObjectTransitionedEvent. Trigger types: created / updated / transition (with optional action filter). Recipients: users (literal) / field (uid from object). Channel: nc-notification (via `INotificationManager` + bundled `INotifier`). `{{prop}}` interpolation in subject.

Cross-cutting changes

Schema::validateConfigurationArray: pass any x-openregister-* key through the configuration column; previously the allowlist silently dropped unknown keys.
SchemaMapper::cleanObject: each annotation runs its dedicated validator at schema-save and raises a clean error.
LifecycleGuardRegistry: tries the OR app container first, falls back to \OC::$server so cross-app guards (FQCN) autowire from the server container.
Routes for /api/objects/{id}/transition and /api/objects/{id}/available-actions precede the /api/objects/{register}/{schema} wildcard so they aren't grabbed as register=id, schema=transition.
TransitionController translates HookStoppedException to a structured 422 instead of a 500.

What's deferred

aggregations: backend-native aggregation (Postgres GROUP BY / Solr facets / Elasticsearch aggs) — currently PHP-side over MagicMapper.
calculations: virtual (materialise:false) calculations, @self.* property refs, occ rematerialise / validate commands.
notifications: Webhook auto-create, scheduled / threshold triggers, group / relation / object-acl / expression recipients, email / Talk / Activity channels.

The four change directories (lifecycle-annotation, aggregations-annotation, calculations-annotation, notifications-annotation) have been moved to openspec/changes/archive/2026-04-29-*.

Test plan

schema-save validators reject malformed annotations
lifecycle: draft→scheduled→opened→closed transitions, invalid transitions rejected, guard denial surfaces as 422
aggregations: count/groupBy/in/gte($startOfMonth) all return correct values
calculations: daysLate + isOverdue materialise on save; aggregations target the materialised fields
notifications: transition triggers fire to admin via INotificationManager; action filter rejects unrelated transitions
CI quality (PHPCS / PHPMD / Psalm / PHPStan) — expected to flag pre-existing issues; new code passes locally

Companion PR

Decidesk pilot adopting all four annotations: ConductionNL/decidesk@feature/json-manifest-pilot

github-actions · 2026-04-29T08:55:40Z

Quality Report — ConductionNL/openregister @ `793f435`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	❌
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
composer			✅	✅ 147/147
npm			✅	✅ 599/599
PHPUnit					⏭️
Newman					⏭️
Playwright					⏭️

Quality workflow — 2026-04-29 08:55 UTC

Download the full PDF report from the workflow artifacts.

New calculation: daysOpen = diffDays($now, @self.created) Materialises on every save thanks to OR's CalculationOnSaveListener injecting @self metadata at evaluation time (PR ConductionNL/openregister#1357). This is the canonical "time since creation" metric that previously required a hand-written analytics query — the listener does it once, materialises the integer, and aggregations can target it directly: avgDaysOpen: { metric: "avg", field: "daysOpen", filter: { taskStatus: { in: ["open","in-progress","overdue"] } } }

statusBadge is a materialise:false calculation: 'taskStatus' + ' (' + (isOverdue ? 'overdue' : 'on track') + ')'. Evaluated at response-render time only when caller passes _extend=calculations. Demonstrates the virtual-calculation path that landed in OR PR #1357. Companion: ConductionNL/openregister#1357

…larative aggregations Replaces the in-PHP iteration over every ActionItem (~50 lines per metric) with 4 calls to OR's AggregationRunner. ### Schema additions - daysToClose calculation (materialise:true): diffDays(completedAt, @self.created) - daysToClose property declaration (integer, populated by the calc listener) - avgDaysToClose aggregation (avg of daysToClose where taskStatus=completed) ### Service refactor - getSummary delegates to a thin private aggregate() helper that calls OCA\OpenRegister\Service\Aggregation\AggregationRunner directly. - Each metric (totalOpen, totalOverdue, completedThisMonth, avgDaysToClose) is now declared on the schema and computed by the platform — the service no longer iterates objects in PHP. ### Live verification ActionItemAnalyticsService::getSummary("2026-01-01","2026-12-31") → {"totalOpen":10,"totalOverdue":3,"completedThisMonth":4,"avgDaysToClose":0} avgDaysToClose is 0 today because newly-imported ActionItems don't have daysToClose materialised yet — once OR's \`occ openregister:rematerialise-calculations decidesk action-item\` command lands (class is shipped in OR PR #1357 but the <commands> block is gated on an upstream DI fix), existing items will pick up the materialised value on the next bulk pass. Companion: ConductionNL/openregister#1357

WilcoLouwerse · 2026-05-04T13:38:28Z

Heads-up on coordination with the retrofit / reverse-spec PRs landing in parallel:

retrofit: reverse-spec oas-generation (2 REQs / 2 methods) #1397 (oas-generation retrofit) adds @spec PHPDoc tags to the two existing methods on lib/Controller/OasController.php (generateAll and generate). This PR also modifies that file substantively, so whichever lands second will need a trivial rebase — please keep those @spec annotations intact when resolving.
If this PR adds new public methods to OasController (e.g. around the new error contract / additional endpoints), they're good candidates for a follow-up @spec annotation in a delta on retrofit: reverse-spec oas-generation (2 REQs / 2 methods) #1397 or a subsequent retrofit.
No functional dependency between this PR and retrofit: reverse-spec oas-generation (2 REQs / 2 methods) #1397 — purely a documentation-coordination note.
retrofit: reverse-spec actions (5 REQs / 9 methods) #1394 (actions retrofit, already merged) and retrofit: reverse-spec approval-workflow (5 REQs / 13 methods) #1400 (approval-workflow retrofit) have no file overlap with this PR.

…ration-2026-04 # Conflicts: # lib/Controller/OasController.php

WilcoLouwerse

Strict-mode review — scope-limited (PR is too large for exhaustive review). Reviewed: appinfo/routes.php, lib/Controller/TransitionController.php, lib/Controller/AggregationController.php, lib/Controller/DsarController.php, lib/Controller/NotificationHistoryController.php, lib/Controller/RealtimeController.php, lib/Controller/ScopesController.php, lib/Controller/UrnControll

Strict mode requires REQUEST_CHANGES on any 🔴 or 🟡. Please address blockers and concerns before merge.

Blockers (F03–F06): - F03 TransitionController/Engine — gate transition() with PermissionHandler::hasPermission(action='update', object) and availableActions() with action='read'. Adds NotAuthorizedException catch in the controller (403 Forbidden). Without this gate any authenticated user could advance any object's lifecycle. - F04 AggregationRunner — gate run() with hasPermission(action='list') on the schema. Adds 403 catch in AggregationController. Documents the per-object ACL leak that remains for schemas with object-acl / conditional rules; fix tracked alongside the aggregations-backend-native follow-up. - F05 AnnotationNotificationDispatcher — validate every recipient uid via IUserManager::userExists() (per-request cached). Covers the `users`, `field`, and `relation` recipient kinds so attacker- controlled object data cannot direct notifications at admin uids. - F06 LifecycleGuardRegistry / AnnotationNotificationDispatcher — drop every `\OC::$server` reference in lib/. Inject IServerContainer instead. Aligns with the ADR added in this same PR (docs/development-notes/AUDIT_2026-05-01.md). The Talk emit path also prefers the injected IConfig. Concerns (F07–F15): - F07 NotificationHistoryController — return 401 when unauthenticated; force `recipient = currentUid` filter for non-admins so the endpoint cannot dump every user's notification history. - F08 Dispatcher.interpolate() — htmlspecialchars-escape every substituted value (defence in depth before NC's own render layer). - F09 ScopesController.index() — add `@NoAdminRequired`. Endpoint was silently 401-ing for the very audience it's designed to serve. - F10 UrnController — add `@NoAdminRequired` to resolve / lookup / bulk. - F11 RealtimeController — return 401 when no user session, instead of the empty-stream masquerade. - F12 LifecycleValidationListener — document the trust contract: only fires on `ObjectUpdatingEvent`, so callers MUST go through `ObjectService::saveObject()` for lifecycle integrity. - F13 Schema::validateConfigurationArray — declare the `x-openregister-*` vocabulary in `ANNOTATION_VOCABULARY` and drop unknown keys at save time so `x-openregister-lifecycl` (typo) is caught early instead of round-tripping silently. - F14 CalculationEvaluator::formatDate — document the trust model (schema authors are admin-equivalent; format string is not allowlisted). - F15 AggregationRunner — cap PHP fallback at 10 000 rows (PHP_FALLBACK_ROW_CAP), surface `truncated: true` in the result. Native Postgres path is unaffected. F16 dompdf 3.1.5 — `composer audit` on production deps reports no known advisories. Single instantiation site at PdfReportWriter:69 (verified via grep). isRemoteEnabled=false / isPhpEnabled=false on that path — no other Dompdf instances added. F01 (split into four PRs) and F02 (no CI on head SHA — push trigger excludes `platform-integration-*`, pull_request types omit `synchronize`) are addressed in PR comment replies, not by code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@return

Resolves conflicts in: - lib/Service/Lifecycle/TransitionEngine.php — keep F03 RBAC gate (NotAuthorizedException → 403); take development's named-arg style for getLifecycleAnnotation() calls. - lib/Service/Aggregation/AggregationRunner.php — keep F15 PHP_FALLBACK_ROW_CAP + truncated flag; take development's broader constructor (OrganisationService, optional SearchBackendInterface); deduplicate the two RBAC gates added in parallel on each branch and upgrade development's RuntimeException-based gate to NotAuthorizedException for proper 403 mapping. - lib/Service/Notification/AnnotationNotificationDispatcher.php — keep F05 userExists() guard on extracted relation uids; take development's named-arg style; merge development's @param/@return docblock additions with the F08 escape-rationale and F06 IServerContainer-ADR rationale. - lib/Service/Calculation/CalculationEvaluator.php — keep F14 trust- model docblock; merge development's @param/@return tags. - lib/Controller/RealtimeController.php — keep F11 401 guards on events() and cursor(); take development's @param/@return docblock additions and the new org-scoped cursor implementation. - lib/Controller/UrnController.php — keep F10 @NoAdminRequired on all three methods; merge development's @param/@return docblock additions. - lib/Controller/NotificationHistoryController.php — accept development's wording in the 401 error body ("Authentication required"). Functional behaviour unchanged. - lib/Controller/OasController.php — accept development's spec path (`retrofit-2026-05-01-oas-generation`); the directory in the merged tree only exists under this name. php -l on every edited file is clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

WilcoLouwerse

2 blockers require fixes before merge — dompdf LGPL-2.1 not on license allowlist (the License/composer job stays red until .license-overrides.json gains an entry) and phpcs violations in F03/F04/F06 security-fix code (named-parameter rule on NotAuthorizedException() calls + missing docblocks for $serverContainer / $permissionHandler; ~30 errors total, most auto-fixable via phpcbf).

✅ All 14 prior security findings (F03–F16) verified intact post-merge: TransitionEngine RBAC gates, AggregationRunner hasPermission(action='list'), Dispatcher userExists() for users/field/relation kinds, IServerContainer injection, NotificationHistory 401 + recipient force, interpolate() htmlspecialchars, @NoAdminRequired on Scopes/Urn/Realtime, lifecycle/calculation/dispatcher trust-model docblocks, ANNOTATION_VOCABULARY allowlist, PHP_FALLBACK_ROW_CAP=10000 with truncated flag.

✅ F02 resolved — CI now runs on the merge commit; 3 jobs are red but the workflow trigger gap is closed.

ℹ️ The eslint failure on src/views/reports/ReportView.vue:405 (unused widget arg) is in development-branch code outside this PR's diff — coordinate with whoever owns that file rather than fixing here.

Addresses the phpcs failures flagged on the merge commit: - TransitionEngine.php — add docblock for $permissionHandler param and switch the two NotAuthorizedException throws to named-parameter form (`message: sprintf(...)`). - LifecycleGuardRegistry.php — add docblock for $serverContainer and re-align the now-three-param block. - AggregationRunner.php — switch the NotAuthorizedException throw to named-parameter form. The multi-line IF closing-paren style and equals-sign alignment were auto-fixed by phpcbf. - Schema.php — blank line after the if-closing brace inside validateConfigurationArray (auto-fixed by phpcbf). phpcs --standard=phpcs.xml now reports 0 errors across all four files. The 3 remaining warnings (line-length in computeMetric's match block) predate this PR and live in unaltered code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

WilcoLouwerse

1 blocker unchanged + 1 partially-addressed concern remain:

🔴 dompdf 3.1.5 LGPL-2.1 still not on the license allowlist — License (composer) job stays red; .license-overrides.json needs a dompdf entry.
🟡 phpcs only partially cleared — c34082e cleaned the 4 files I called out, but the same 535973c fix commit also left 21 phpcs errors in NotificationHistoryController (F07 docblock), RealtimeController (F11 docblock), TransitionController (F03 end-comment), and Dispatcher (F05 named-param + F06 docblock-shift + F08 control-flow style) — all in this PR's diff per ADR-020.

✅ All 14 prior security findings remain intact; the named-param + docblock style fixes that DID land (TransitionEngine / AggregationRunner / LifecycleGuardRegistry / Schema) read clean.

Round 2 of the c34082e phpcs cleanup, plus the dompdf license override flagged as the only remaining 🔴 in the latest CHANGES_REQUESTED review. License gate - .license-overrides.json — add `dompdf/dompdf` (LGPL-2.1) with the same rationale we use for smalot/pdfparser (LGPL family is allowlisted; the checker can't normalise the SPDX variant). The single instantiation site PdfReportWriter:69 keeps isRemoteEnabled / isPhpEnabled false. phpcs in F03/F05/F06/F07/F11 fix-commit files (the 21 errors flagged on 535973c that the c34082e pass missed) - NotificationHistoryController — add docblocks for $userSession + $groupManager (F07). - RealtimeController — add docblock for $userSession (F11). - TransitionController — phpcbf supplied the missing `//end try` end-comment (F03 long try block). - AnnotationNotificationDispatcher — add `$serverContainer` docblock (kept short to stay inside the 150-char hard limit), shift the five existing optional-param docblocks back into alignment, switch the three `userExists($u)` calls to named-parameter form (`uid: $u`), add the missing `@return bool` tag on userExists itself, and let phpcbf clean up the no-blank-line-after-control violations introduced alongside the F08 escape changes. phpcs in upstream-side files that the full-repo gate also fails on - PermissionHandler — add `@param Schema|null $schema` for the new optional cache-break parameter on buildPermissionCacheKey; phpcbf supplied the missing `//end try` end-comment. - AuditTrailController — add docblocks for $userSession + $groupManager. - ReportsController — add docblock for $logger; phpcbf added the missing `//end try` end-comment. - ActionsController — add docblocks for $userSession + $groupManager; capitalise an inline-comment first letter. - RegistersController — phpcbf restored two ternary spaces (auto-fix). `./vendor/bin/phpcs --standard=phpcs.xml` now reports 0 errors across the full lib/ tree (warnings only, all line-length on pre-existing match arms / long format strings — none introduced by F03–F16). `php -l` clean on every touched file. License-checker should now stop flagging dompdf as denied. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

WilcoLouwerse

1 new blocker — DispatcherTest not updated for F06's new 9th constructor arg, PHPUnit (PHP 8.3 / 8.4, NC stable32) fails 9× until the test passes a mocked IServerContainer at slot 9.

Both prior blockers are cleared: License (composer) — .license-overrides.json got the dompdf entry — and phpcs — all 21 errors in the F03/F05/F06/F07/F08/F11 fix code are gone.

ℹ️ Two pre-existing CI failures are not introduced by this PR but will block merge until someone fixes them: AuditTrailControllerTest:35 already passes 5 args for a 7-arg constructor on development (32× PHPUnit errors, surfaced now phpcs unblocks PHPUnit; needs a follow-up PR on development), and Newman shows 32 cascading assertion failures whose only between-commits change is whitespace/docblocks — most likely a flake or environment regression worth a re-run before assuming it's a real bug.

Resolutions per #1353 review thread: Blockers - aggregations-and-calculations/tasks.md: explicit ADR-028 gating note pointing at split PR #1354 — change MUST NOT merge in current bundled form. - lifecycle-annotation spec: add auth contract for POST /transition (NoAdminRequired, CSRF enforced, 401/403 codes + scenarios). - lifecycle-annotation spec: forbid duplicate (from,to) transition pairs; this makes ObjectTransitionedEvent.action deterministically resolvable on direct PATCH; new 422 code lifecycle-duplicate-from-to + scenario. - aggregations spec: new MUST clause for organisation/register multi-tenancy scoping on aggregate queries (closes the gap that PR #1357/#1419 hit) + cross-org and cross-register scenarios. - notifications-annotation/tasks.md: explicit prerequisite note (depends_on aggregations-and-calculations) — PlaceholderResolver + cache-invalidation event live there. - platform-capabilities.md: mail-sidebar status corrected to implemented; RBAC and multi-tenancy rows now reference the active specs (rbac-scopes, tenant-isolation-audit/lifecycle/quotas) with explicit archive notes for the legacy slugs; geo-metadata-kaart reverted to proposed (still in changes/, not yet graduated). Concerns - lifecycle spec: define StoppableEvent rejection-metadata contract (setRejectionReason / getRejectionReason on ObjectUpdatingEvent; ObjectService::saveObject translates stopped+reason into HTTP 422). Add prerequisite task 1.2a to extend the event class if missing. - notifications spec: normative channel block format table (per kind) + explicit prohibition on inline webhook URLs (SSRF); end-to-end annotation example. - notifications spec: scenarios for created and updated triggers, including only_if_changed field-diff semantics. - aggregations spec: computeOn vocabulary ('save' default; 'transition:<name>' explicitly cross-spec dependent on lifecycle-annotation, with fail-fast 422 if used without lifecycle). - platform-capabilities.md: mark Hydra spec linter and CI catalog enforcement as planned, not shipped. - three deltas/tasks.md: explicit CHANGELOG-entry obligation tasks (deferred to implementing PR; spec PRs document the obligation). - lifecycle spec: auth contract requirement now captures NoAdminRequired in the spec body, not only in tasks.md. Minors - aggregations spec: time-bucket UTC semantics + v2 deferral note. - notifications spec: throttle window grammar pinned to ^([1-9][0-9]*) per (second|minute|hour|day|week)$. Composer.lock plugin-api-version downgrade (concern #14): no spec edit; addressed in the inline review reply (CI Composer version is the canonical regenerator; the field is an environment artefact and the symfony bumps in the same hunk are the substantive change). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

F06 added `IServerContainer $serverContainer` as the 9th required constructor parameter on `AnnotationNotificationDispatcher`. The existing `makeDispatcher()` helper still passed the pre-F06 positional shape, so `null` landed at slot 9 and PHPUnit raised TypeError 9× across the suite (every test that touches makeDispatcher). Adds the IServerContainer mock to the test fixture: import, property declaration, setUp() initialisation, and the missing positional argument between $httpClient and the optional rateLimiter null. The mock's default-null `get()` matches the only call site (`resolveExpressionRecipients`) the dispatcher exercises, so no test logic changes are required. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

WilcoLouwerse

Re-review @ 902fd30 (Strict mode) — verifying the F03–F16 fix chain plus the merge-from-development. Verdict and finding overview in a follow-up review.

WilcoLouwerse

Re-review @ `902fd30` — Strict mode

The F03–F16 fix chain plus the development-merge resolution land cleanly for the original blockers — F03/F04 RBAC gates, F05 userExists guard, F06 \OC::$server removal, F07/F11 401 + scoping, F08 htmlspecialchars, F09–F10 @NoAdminRequired, F12–F14 trust-contract docblocks, F15 row cap + truncated, F16 dompdf license override, plus the constructor-TypeError TestFix at this HEAD all verify end-to-end. ✅

But 8 new findings surfaced during the verification pass — 1 🔴 blocker (a second-order regression in the same test file the TestFix touched), 5 🟡 concerns (Strict-mode blocking), 2 🟢 minors:

🔴 Blocker (1)

R01 — F05's userExists() guard breaks every emit-assertion test (no userExists mock stub → default false → all recipients dropped)

🟡 Concerns (5) — Strict-mode blocking

🟢 Minor (2)

Status of prior findings: 16/16 resolved (F02–F16 verified at HEAD; F01 acknowledged, deferred). All previously-flagged blockers are closed by the 5 fix commits.

CI: Newman API ✅ on this SHA, but the Code Quality run is mid-flight (a fresh workflow_dispatch superseded the previous one); PHPUnit / phpcs / license verdicts haven't reported on 902fd300 yet — note that R01 will turn that PHPUnit job red unless addressed.

Requesting changes — R01 must land, and the five 🟡 concerns block under Strict mode.

R01 — DispatcherTest userExists default-false dropped every recipient across nine emit-assertion tests. setUp() now stubs IUserManager's userExists via a willReturnCallback that returns true for every uid EXCEPT the literal `ghost-uid` sentinel. New testFieldRecipientDroppedWhenUidDoesNotExist() exercises the F05 drop branch explicitly so the guard has positive negative-path coverage. R02 — AggregationRunner::run() picked up a `bool $bypassRbac=false` parameter so non-controller callers can opt into the unauthenticated path with explicit reasoning. Threaded `bypassRbac: true` through: - ReportRenderService:180 (dashboard widgets — viewer's authoritative reason is dashboard-read, not the schema's `list` permission; dashboard's own RBAC gate already filtered the viewer at load). - AggregationThresholdListener:171 (write-event reaction — the write itself already passed RBAC; the listener fires on a system trigger independent of the active session). Controller path keeps the F04 verdict (default `false`). R03 — NotificationHistoryController had two functionally-idempotent non-admin recipient-force blocks. Dropped the post-resolve duplicate at lines 110-113; kept the early gate at 96-98. R04 — UrnController::bulk() inline comment now reflects the `@NoAdminRequired` reality post-F10 instead of the stale "effectively admin-only" claim from before the relaxation. R05 — AggregationRunner result envelope now carries `truncated` on every backend, not just the PHP fallback. Native Postgres always emits `truncated: false` (it aggregates the full set); search-backend path propagates from the engine result with `false` as the default. Clients can now branch on `result.truncated === true` regardless of which backend served the request. R06 — Dispatcher::userExists() no longer caches `\Throwable`-failed lookups as permanent `false`. Transient DB/LDAP/container hiccups log a warning + return false WITHOUT writing to the per-request cache, so the next call within the same request retries the lookup. Definitive verdicts (genuine `bool` returns) are still cached. R07 — Schema::validateConfigurationArray now collects unknown `x-openregister-*` keys onto the entity (private `$droppedAnnotationKeys` + `consumeDroppedAnnotationKeys()` getter) and SchemaMapper bridges them to a structured `LoggerInterface::warning()` after `cleanObject()`. Operators see "Dropped N unknown x-openregister-* key(s) on schema X: foo, bar. Typo?" in the NC logs immediately after a save with a typo — the original silent-drop behaviour now has an in-band signal. Couldn't inject a logger into the entity itself (NC Entity pattern + the F06 ADR bans \OC::$server / \OCP\Server static accessors), so the bridge stays in the mapper which already has DI plumbing. R08 — Pinned the F03/F04 → 403 contract with unit tests: - AggregationControllerTest::testReturnsForbiddenWhenRunnerDeniesAccess - new TransitionControllerTest with 6 cases (200/400/403 on transition, 403/404 on availableActions, 422 happy-path errors) A future try/catch reorder or wrapper-layer refactor that silently flips 403 → 404 (or 500) will trip these tests rather than land unnoticed. phpcs --standard=phpcs.xml --warning-severity=0 reports 0 errors across the full lib/ tree. Test files are out-of-phpcs-scope per phpcs.xml's `<file>lib</file>` declaration. php -l clean on every edited file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

WilcoLouwerse

Re-review of fd91a62. R01–R06 and R08 confirmed fixed by reading code. R07 partial-fix introduces a new 🔴 blocker — SchemaMapper's new required LoggerInterface 8th-arg constructor param wasn't propagated to the DI factory at Application.php:378-388 or SchemaMapperTest.php:45-53, so quality / PHPUnit (PHP 8.3 + 8.4) flipped to failure on this head SHA (ArgumentCountError). Two-line wiring update on each site clears it.

7 of 8 R-findings cleared; 1 partial-fix needs that wiring before the verdict can flip to APPROVE.

R07's SchemaMapper logger param landed without propagation to the two construction sites, flipping PHPUnit (PHP 8.3 / 8.4) red on fd91a62 with ArgumentCountError (8 expected, 7 passed): - Application.php DI factory: add `logger: $container->get('Psr\\Log\\LoggerInterface')` as the 8th named arg (matches the FQCN-string pattern used by every other logger-injected service in this file). - SchemaMapperTest::setUp(): mock LoggerInterface and pass as the 8th positional arg to the constructor. Closes the only remaining blocker on the re-review @ fd91a62. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

WilcoLouwerse

✅ Approved — all prior findings resolved

Verified state at HEAD 4b20f2d:

First review (2026-05-07 10:42, CHANGES_REQUESTED): 21 findings (6 🔴 / 10 🟡 / 5 🟢) — all confirmed resolved via inline replies in commit 535973c4.
Quality follow-ups: 21 phpcs failures cleared in 654fd2b9; License (composer) dompdf overrides cleared in 654fd2b9; CI surfaced after base merge 8ac3dec brought the workflow trigger back into scope (F02 closed).
DispatcherTest blocker: broken constructor signature fixed in 902fd300.
Re-review (2026-05-07 15:10, CHANGES_REQUESTED): R01 blocker + R02–R08 — all confirmed resolved in fd91a62e.
R07 follow-up blocker (LoggerInterface DI propagation) — fixed in 4b20f2d and verified: 8th constructor arg wired in Application.php DI factory + SchemaMapperTest::setUp.

CI: api-test-coverage green on this SHA. code-quality does not run here — F02's synchronize-trigger gap reproduces; recommend addressing in a follow-up PR (out of scope here).

Lifecycle, aggregations, calculations, and notifications annotation engines all ship with their security guards (RBAC, uid validation, htmlspecialchars escaping, fail-closed lifecycle guards) and the structural concerns (PR-size, monolithic registration) acknowledged with rationale.

WilcoLouwerse · 2026-05-08T10:57:05Z

PR #1357 — Full Review & Fix Summary

Final verdict: ✅ APPROVED at 2026-05-08 08:36 UTC (review #4250816414)

33 findings raised across 5 review rounds, all resolved across 8 fix commits.

Timeline

Time (UTC)	Event
2026-05-07 10:29–10:42	Review 1 (CHANGES_REQUESTED) — 21 findings: 6🔴 / 10🟡 / 5🟢
2026-05-07 11:28	Fix commit `535973c4` — F03–F16
2026-05-07 11:46	Merge base `8ac3dec` — brought CI workflow trigger back into scope (closes F02)
2026-05-07 12:04–12:05	2 new blockers surfaced once CI ran: License (dompdf) + 21 phpcs errors
2026-05-07 13:29	Fix commit `c34082ef` — partial phpcs
2026-05-07 13:57	Fix commit `654fd2b9` — phpcs cleared + license override
2026-05-07 14:16	New blocker — DispatcherTest broken by F06's 9th constructor arg
2026-05-07 14:33	Fix commit `902fd300` — test fix
2026-05-07 15:10	Review 2 (CHANGES_REQUESTED) — R01🔴 + R02–R08 (5🟡, 2🟢)
2026-05-07 17:04	Fix commit `fd91a62e` — R01–R08
2026-05-07 18:00	R07 follow-up — LoggerInterface DI propagation missing → new 🔴
2026-05-08 08:18	Fix commit `4b20f2d` — DI factory + test
2026-05-08 08:36	Final APPROVE

🔴 Blockers (12 total — all resolved)

Round 1: Security & operational (initial review)

ID	Finding	Fix
F01	PR too large to review safely (587 files reported) — should split into four single-mechanism deltas	Acknowledged: kept monolithic; four engines share `Application.php` registration so a clean split was non-trivial. Author committed to splitting future iterations.
F02	No CI check runs on head SHA — workflow `pull_request` trigger missing `synchronize` event	Root-caused & resolved: merging `development` (`8ac3dec`) brought the workflow trigger back into scope. Permanent fix to `code-quality.yml` deferred to follow-up.
F03	`TransitionController` — no per-object RBAC; any authenticated user could transition any object's lifecycle state	Fixed in `535973c4`: `TransitionEngine::transition()` now calls `PermissionHandler::hasPermission(action='update', objectId)`; throws `NotAuthorizedException` → 403.
F04	`AggregationController` — no object-level RBAC; auth users could aggregate across any schema	Fixed in `535973c4`: `AggregationRunner::run()` requires `PermissionHandler::hasPermission(action='list', schema)`.
F05	`AnnotationNotificationDispatcher` — `field` recipient kind accepted unvalidated `uid` from object data (attacker-controlled notification target)	Fixed in `535973c4`: `userExists()` (per-request cached) now wraps every uid added from object data; non-existent uids dropped before dispatch.
F06	`LifecycleGuardRegistry` + `AnnotationNotificationDispatcher` use `\OC::$server` (banned by ADR added in this PR)	Fixed in `535973c4`: refactored to `IServerContainer $serverContainer` constructor injection — falls back from OR app container to server container for cross-app FQCN guards.

Round 2: Quality blockers (surfaced once CI ran)

ID	Finding	Fix
License	`dompdf/dompdf v3.1.5` uses LGPL-2.1, not in composer license allowlist → `composer check:strict` red	Fixed in `654fd2b9`: added `dompdf/dompdf` entry to `.license-overrides.json` with same rationale as existing `sabre/*` exemptions.
phpcs	21 errors across 8 files — missing docblocks for new constructor params, named-parameter rule on `NotAuthorizedException()` calls	Fixed across `c34082ef` + `654fd2b9`: representative-example trap caught — first commit fixed only the 4 cited files; second commit cleared the rest after the full PR-diff scope was called out per ADR-020.

Round 3: Test breakage from constructor refactor

ID	Finding	Fix
F06-test	F06's 9th required `IServerContainer` constructor arg broke `AnnotationNotificationDispatcherTest` — PHPUnit fails 9× with `ArgumentCountError`	Fixed in `902fd300`: added `use OCP\IServerContainer;`, mocked it in `setUp()`, threaded as 9th positional in `makeDispatcher()` helper.

Round 4: Re-review blocker

ID	Finding	Fix
R01	`DispatcherTest` — F05's `userExists()` guard breaks every emit-assertion test; mock has no `userExists` stub, defaults to `false`, every recipient silently dropped	Fixed in `fd91a62e`: `setUp()` stubs `IUserManager::userExists` via `willReturnCallback(static fn(string $uid): bool => true)`.

Round 5: R07-introduced regression

ID	Finding	Fix
R07-followup	R07's fix added `LoggerInterface` as 8th required constructor param to `SchemaMapper` but didn't propagate to the DI factory or test → `ArgumentCountError` at runtime, PHPUnit (PHP 8.3/8.4) red	Fixed in `4b20f2d` (final commit): added `logger: $container->get('Psr\Log\LoggerInterface')` to `Application.php` DI factory + matching mock + 8th positional in `SchemaMapperTest::setUp()`.

🟡 Concerns (15 total — all resolved)

Round 1 (initial review)

ID	File	Finding	Fix
F07	`NotificationHistoryController`	`@NoAdminRequired` endpoint returned all users' notification history without per-user filter	Fixed `535973c4`: 401 when no session; force `filters['recipient'] = currentUid` for non-admins
F08	`AnnotationNotificationDispatcher::interpolate()`	`{{prop}}` values not HTML-escaped → XSS in notification subject	Fixed `535973c4`: `htmlspecialchars((string)$v, ENT_QUOTES, 'UTF-8')` on every substituted value
F09	`ScopesController::index()`	Missing `@NoAdminRequired` → silently 401s for all non-admins despite docblock claim	Fixed `535973c4`: added annotation
F10	`UrnController` (3 methods)	`@NoCSRFRequired` only, missing `@NoAdminRequired` → URN resolution silently rejected for non-admins	Fixed `535973c4`: added `@NoAdminRequired` to `resolve()`, `lookup()`, `bulk()`
F11	`RealtimeController`	`@NoCSRFRequired` only — anonymous callers got empty result instead of 401	Fixed `535973c4`: both `events()` and `cursor()` early-return 401 when `getUser()` is null
F12	`LifecycleValidationListener`	Only runs on `ObjectUpdatingEvent` — direct `SaveObject` calls bypass	Documented `535973c4`: explicit trust-contract docblock — lifecycle integrity depends on every mutation routing through the event
F13	`Schema::validateConfigurationArray`	Passed all `x-openregister-*` keys without shape validation at write time	Fixed `535973c4`: `ANNOTATION_VOCABULARY` constant lists 7 declared keys; unknowns logged & dropped
F14	`CalculationEvaluator::formatDate`	Accepts format string from schema annotations — PHP date format codes could leak metadata	Documented `535973c4`: trust-model docblock — schema authors are admin-equivalent
F15	`AggregationRunner` PHP-fallback	Fetched up to 100,000 objects per request → memory exhaustion	Fixed `535973c4`: `PHP_FALLBACK_ROW_CAP = 10000` (10× drop), `truncated` flag in result envelope
F16	`composer.json`	Added `dompdf/dompdf v3.1.5` — historical SSRF/XSS issues	Verified `535973c4`: `composer audit --no-dev` clean; hermetic config (`isRemoteEnabled=false`, `isPhpEnabled=false`) confirmed at the only instantiation site

Round 2 (re-review at 15:10)

ID	File	Finding	Fix
R02	`AggregationRunner`	F04's RBAC gate inside `run()` regresses non-controller callers — `ReportRenderService` widgets and `AggregationThresholdListener` would silently fail for actors lacking `list` permission	Fixed `fd91a62e`: added `bool $bypassRbac=false` parameter; threaded `bypassRbac: true` from system-actor call sites
R03	`NotificationHistoryController`	F07 `recipient = currentUid` block duplicated at lines 96–98 and 110–113	Fixed `fd91a62e`: dropped post-resolve duplicate
R04	`UrnController::bulk()`	Inline security comment stale & contradicts new `@NoAdminRequired`	Fixed `fd91a62e`: comment rewritten — "this route is reachable by every authenticated user"
R05	`AggregationRunner`	`truncated` flag only emitted on PHP-fallback path; native-Postgres and search-backend paths silently drop the signal	Fixed `fd91a62e`: `truncated` now appears in every backend's result envelope
R06	`AnnotationNotificationDispatcher::userExists()`	Caches `\Throwable`-failed lookups as permanent `false` for the rest of the request	Fixed `fd91a62e`: `catch (\Throwable)` arm logs at warning level with retry note; doesn't poison the cache
R07	`Schema::validateConfigurationArray`	F13 silently drops unknown `x-openregister-*` keys without a log entry — operators get no in-band signal	Fixed `fd91a62e` (introduced R07-followup blocker, then fully fixed in `4b20f2d`)

🟢 Minor / Positive observations (6)

ID	Observation
M1	Route ordering: `/api/objects/{id}/transition` and `/api/objects/aggregations/{register}/{schema}/{name}` correctly precede the `/api/objects/{register}/{schema}` wildcard
M2	dompdf hermetic configuration verified (`isRemoteEnabled=false`, `isPhpEnabled=false`) at the only instantiation site `PdfReportWriter:69`
M3	`CalculationEvaluator::divide()` and `modulo()` correctly catch division/modulo by zero
M4	`AggregationRunner::sanitizeColumnName()` strips non-alphanumeric characters → SQL injection via column names neutralised
M5	`LifecycleGuardInterface` contract is fail-closed: unregistered guard throws `RuntimeException`
R08	F03/F04 added 403 path on `NotAuthorizedException` but no test pinning the contract — fixed `fd91a62e` (added `AggregationControllerTest::testReturnsForbiddenWhenRunnerDenies` etc.)

What's deferred (per PR description, accepted)

Aggregations: backend-native aggregation (Postgres GROUP BY / Solr facets / Elasticsearch aggs) — currently PHP-side over MagicMapper.
Calculations: virtual (materialise:false), @self.* property refs, occ rematerialise / validate commands.
Notifications: webhook auto-create, scheduled/threshold triggers, group/relation/ACL recipients, email/Talk/Activity channels.

Final state

✅ All 33 findings resolved across 8 fix commits. Newman API tests green at HEAD 4b20f2d. code-quality CI doesn't run on this SHA due to the F02 trigger gap (separate follow-up). PR ready to merge pending the 1-approval requirement on the development branch ruleset.

Thanks @rubenvdlinde for the iterative discipline across the 5 review rounds — every blocker confirmed-fixed via inline thread, no shortcuts taken.

The development merge (PR #1357) added IUserSession/IGroupManager to AuditTrailController's constructor and changed two production behaviours without updating the affected tests. Bring the tests in line: - AuditTrailControllerTest: pass the two new constructor args; default setUp() mocks an authenticated admin so the requireAdmin() gate on export()/clearAll() lets the happy-path assertions through. Clears all 136 ArgumentCountError failures. - ObjectServiceTest + ObjectServiceDeepTest: setSchema() deliberately rethrows DoesNotExistException (so NC's dispatcher returns 404; wrapping in ValidationException would be a 500). Update the expected exception. - PermissionHandlerCacheTest::testConditionalRulesEvaluatePerObjectUuid: buildPermissionCacheKey() returns null for schemas with a match rule (verdict depends on mutable object data), so repeats are re-evaluated — 4 hasPermission() calls => 4 ConditionMatcher delegations, not 2. These three failures pre-date this PR (present on development's own CI); fixed here because the development merge pulled them into this branch.

rubenvdlinde mentioned this pull request Apr 29, 2026

feat: adopt OpenRegister declarative annotations (lifecycle / aggregations / calculations / notifications) ConductionNL/decidesk#141

Open

6 tasks

This was referenced Apr 29, 2026

feat: adopt x-openregister-lifecycle on 7 procest schemas ConductionNL/procest#306

Closed

feat: adopt x-openregister-lifecycle on 5 softwarecatalogus schemas ConductionNL/softwarecatalog#209

Open

Merge remote-tracking branch 'origin/development' into platform-integ…

d3b42c6

…ration-2026-04 # Conflicts: # lib/Controller/OasController.php

rubenvdlinde requested review from Rem-Dam, SudoThijn, WilcoLouwerse, bbrands02, remko48 and rjzondervan as code owners May 7, 2026 07:35

WilcoLouwerse mentioned this pull request May 7, 2026

Openstaande OpenRegister PR's verwerken ConductionNL/.github#42

Open

20 tasks