chore(openspec): split aggregations-and-calculations into two single-mechanism specs by rubenvdlinde · Pull Request #1354 · ConductionNL/openregister

rubenvdlinde · 2026-04-29T07:41:04Z

Stacked on #1353. Targets feature/declarative-engines-adr024-027; auto-rebases when #1353 merges.

Summary

Per ADR-028 (hydra#208), one spec covers exactly one platform mechanism. The earlier aggregations-and-calculations spec bundled two distinct annotations (x-openregister-aggregations + x-openregister-calculations) in 35 tasks — flagged by check-spec-size.py as a bundle violation against my own work.

Split into two independent change directories, each ≤ 15 tasks, each extending zoeken-filteren as a delta:

Spec	Tasks	What
aggregations-annotation	13	count/sum/avg/min/max/count_distinct + groupBy + time-bucket. New endpoint + inline `_aggregate=`. Ships the shared PlaceholderResolver.
calculations-annotation	13	Typed computed fields with a small expression DSL (concat, if, arithmetic, comparison, date diff). Materialise at save (aggregatable, persisted) or compute on read (virtual). Cycle detection.

The two compose: an aggregation MAY target a materialised calculation field (e.g., avgDaysToClose: { metric: "avg", field: "daysFromCreatedToCompleted" }). They ship independently.

Demonstrates

This PR is the first concrete migration under ADR-028 — taking my own bundled spec, splitting it, and confirming the size linter goes green:

$ check-spec-size.py aggregations-annotation/ calculations-annotation/
# count=0

The reverse is the audit baseline:

$ check-spec-size.py p2-meeting-management-core-t1/    # decidesk legacy bundle
73 tasks (limit 15)
appears to modify 4 schemas (...)

🤖 Generated with Claude Code

…mechanism specs Per ADR-028 (hydra#208), one spec covers exactly one platform mechanism. The earlier aggregations-and-calculations spec bundled two distinct annotations (x-openregister-aggregations + x-openregister-calculations) in 35 tasks — flagged by check-spec-size.py as a bundle violation. Splits into two independent change directories, each ≤ 15 tasks, each extending zoeken-filteren as a delta: - aggregations-annotation/ — count/sum/avg/min/max/count_distinct + groupBy + time-bucket. New endpoint + inline _aggregate=. Adds the PlaceholderResolver as a shared service. 13 tasks. - calculations-annotation/ — typed computed fields with a small expression DSL (concat, if, arithmetic, comparison, date diff). Materialise at save (aggregatable, persisted) or compute on read (virtual). Cycle detection at schema-save time. 13 tasks. The two compose: an aggregation MAY target a materialised calculation field (e.g., `avgDaysToClose: { metric: "avg", field: "daysFromCreatedToCompleted" }`). They ship independently. Catalog updated to reference the two split changes instead of the bundle. Smoke test: $ check-spec-size.py aggregations-annotation/ calculations-annotation/ # count=0

WilcoLouwerse · 2026-05-07T10:41:51Z

+#### Scenario: Inline aggregations alongside results
+- WHEN a client GETs `/api/objects?register=decidesk&schema=action-item&taskStatus=open&_aggregate=totalOpen,totalOverdue&_limit=20`
+- THEN the response MUST contain `results: [...]` (paginated, max 20)
+- AND `aggregations: { totalOpen: { value: 42 }, totalOverdue: { value: 3 } }`


(adjusted from line 335 to nearest hunk line 74)

🔴 Blocker — Garbled scenario title in aggregations-annotation

The scenario is titled 'A to-shaped reference outside the enum is rejected'. The phrase 'to-shaped reference' is nonsensical — this scenario tests that an aggregation filter referencing a value not in a property's enum is rejected at schema-save. The scenario body is correct; only the title is broken. Likely a copy/paste artifact. Suggested fix: 'An out-of-enum filter value is rejected at schema-save' or similar.

WilcoLouwerse · 2026-05-07T10:41:54Z

+- [ ] 1.10 `occ openregister:validate-calculations <register> <schema>` — re-runs every calculation against existing objects and reports any whose stored value differs from the recomputed one.
+- [ ] 1.11 `occ openregister:rematerialise-calculations <register> <schema>` — bulk reprocesses every object's materialised calculations.
+- [ ] 1.12 Doc: `docs/annotations/x-openregister-calculations.md` — DSL reference (every function with examples), virtual vs materialised, idempotency guarantees, cycle rules.
+- [ ] 1.13 Update `openspec/platform-capabilities.md` with the `x-openregister-calculations` row.


(adjusted from line 539 to nearest hunk line 15)

🔴 Blocker — computeOn trigger-based recomputation has no implementation task

The spec.md requirement (line 470 of the diff, calculations-annotation/specs/zoeken-filteren/spec.md) explicitly declares computeOn: ["save", "transition:<name>"] as a supported field. However, none of the 13 tasks in calculations-annotation/tasks.md covers implementing the transition-triggered recomputation path. Task 1.6 only mentions ObjectCreatingEvent + ObjectUpdatingEvent (save-path). Without a task, the builder has no instruction to hook ObjectTransitionedEvent and conditionally rerun the evaluator for the matching computeOn entries. Either add a task or remove computeOn from the spec requirement.

WilcoLouwerse · 2026-05-07T10:41:57Z

+- [ ] 1.10 `occ openregister:validate-calculations <register> <schema>` — re-runs every calculation against existing objects and reports any whose stored value differs from the recomputed one.
+- [ ] 1.11 `occ openregister:rematerialise-calculations <register> <schema>` — bulk reprocesses every object's materialised calculations.
+- [ ] 1.12 Doc: `docs/annotations/x-openregister-calculations.md` — DSL reference (every function with examples), virtual vs materialised, idempotency guarantees, cycle rules.
+- [ ] 1.13 Update `openspec/platform-capabilities.md` with the `x-openregister-calculations` row.


(adjusted from line 539 to nearest hunk line 15)

🔴 Blocker — PlaceholderResolver dependency on aggregations-annotation not declared in calculations tasks

calculations-annotation/proposal.md and design.md state that the spec 'reuses the placeholder resolver shipped in aggregations-annotation'. However, no task in calculations-annotation/tasks.md mentions consuming lib/Service/Search/PlaceholderResolver.php or declares that aggregations-annotation must ship first. Without this, a builder implementing calculations-annotation in isolation will either re-implement the resolver (divergence) or fail (class not found). Task 1.1 (SchemaService validation) and the on-save listener both potentially need $now/$currentUser resolution if expressions use those placeholders. The dependency and the reuse contract must be explicit in at least one task.

WilcoLouwerse · 2026-05-07T10:42:00Z

+## Out of scope
+- External calls in expressions (no HTTP, no I/O — pure evaluation).
+- Aggregations over calculation outputs (covered by `aggregations-annotation`).
+- Materialised calculations whose inputs span multiple objects (no joins; one-object-at-a-time).


(adjusted from line 427 to nearest hunk line 20)

🔴 Blocker — Listener namespace inconsistency between the two split specs

aggregations-annotation places its listener at lib/EventListener/AggregationInvalidationListener.php (both design.md line 281 and task 1.9 line 404 are consistent). calculations-annotation places its listener at lib/Listener/CalculationOnSaveListener.php (design.md line 427 and task 1.6 line 544 are internally consistent). The two sibling specs, both targeting the same app codebase, use different namespace conventions (EventListener vs Listener). This will cause a real build inconsistency unless the app already has both directories. One spec must be corrected to use the canonical path. If the app follows Nextcloud conventions, lib/Listener/ is standard; lib/EventListener/ would be non-standard. The aggregations spec should change to lib/Listener/.

WilcoLouwerse · 2026-05-07T10:42:04Z

+- [ ] 1.10 `occ openregister:validate-calculations <register> <schema>` — re-runs every calculation against existing objects and reports any whose stored value differs from the recomputed one.
+- [ ] 1.11 `occ openregister:rematerialise-calculations <register> <schema>` — bulk reprocesses every object's materialised calculations.
+- [ ] 1.12 Doc: `docs/annotations/x-openregister-calculations.md` — DSL reference (every function with examples), virtual vs materialised, idempotency guarantees, cycle rules.
+- [ ] 1.13 Update `openspec/platform-capabilities.md` with the `x-openregister-calculations` row.


(adjusted from line 539 to nearest hunk line 15)

🟡 Concern — Cycle detection is redundantly specified in tasks 1.1 and 1.8

Task 1.1 (SchemaService validation) already includes 'cycle detection (sort calculations by input dependencies, reject cycles)'. Task 1.8 is entirely 'Schema-save cycle detection — sort calculations by input dependencies; reject cycles with { code: "calculation-cycle", path: [...] }' — a pure duplicate. This is not a correctness issue, but the builder may interpret them as two separate things to implement and produce duplicate code, or waste turns realising they are the same. Task 1.8 should be removed; the error code shape { code: "calculation-cycle", path: [...] } should be folded into task 1.1.

WilcoLouwerse · 2026-05-07T10:42:07Z

+#### Scenario: Inline aggregations alongside results
+- WHEN a client GETs `/api/objects?register=decidesk&schema=action-item&taskStatus=open&_aggregate=totalOpen,totalOverdue&_limit=20`
+- THEN the response MUST contain `results: [...]` (paginated, max 20)
+- AND `aggregations: { totalOpen: { value: 42 }, totalOverdue: { value: 3 } }`


(adjusted from line 384 to nearest hunk line 74)

🟡 Concern — Composition contract documented only in calculations-annotation, not in aggregations-annotation

The claim 'aggregation MAY target a materialised calculation field' is a two-way contract. The calculations-annotation spec.md documents it with a dedicated scenario ('An aggregation MAY target a materialised calculation field', diff line 487). The aggregations-annotation spec.md has no equivalent scenario or requirement statement about accepting a materialised calculation field as the target of a metric. A builder implementing aggregations-annotation in isolation has no instruction that the field param of an aggregation can point to a calculation field rather than a raw schema property, and schema-save validation (task 1.1) might reject such a reference. Add a brief composition note or scenario in aggregations-annotation spec.md.

WilcoLouwerse · 2026-05-07T10:42:10Z

+- [ ] 1.10 Unit tests: validator (every rule, pass + fail), placeholder resolver (every placeholder + offset), compiler (every backend, every metric, groupBy + bucket).
+- [ ] 1.11 Integration test: declare aggregations on a test schema; create / update / delete objects; verify counts respond correctly; verify cache invalidation fires.
+- [ ] 1.12 Doc: `docs/annotations/x-openregister-aggregations.md` + a worked example mirroring decidesk's ActionItem aggregations.
+- [ ] 1.13 Update `openspec/platform-capabilities.md` with the `x-openregister-aggregations` row.


(adjusted from line 408 to nearest hunk line 15)

🟡 Concern — Task 1.13 (platform-capabilities.md update) is already done in this PR

Both aggregations-annotation/tasks.md task 1.13 and calculations-annotation/tasks.md task 1.13 instruct the builder to update openspec/platform-capabilities.md. This PR already updates that file (diff lines 560-563). When the builder implements these specs it will find the row already correct and either skip silently or produce a no-op diff. This is not a blocking issue, but the tasks are pre-satisfied and may confuse the builder's progress tracking. Both tasks 1.13 should note '(done as part of spec split PR #1354)' or be removed.

WilcoLouwerse · 2026-05-07T10:42:13Z

@@ -0,0 +1,74 @@
+---


🟡 Concern — Stacked-PR drift risk — base is feature/declarative-engines-adr024-027

This PR is stacked on #1353 (base branch feature/declarative-engines-adr024-027). If #1353 is rebased, amended, or has its spec structure changed before this PR merges, this PR will need a rebase. The specs here reference zoeken-filteren as the implemented dependency, which comes from #1353's stack. No immediate action required, but the author and reviewer should be aware: merge #1353 first and update this branch's base to development before final merge.

WilcoLouwerse

Strict-mode review — 4 blockers, 4 concerns, 0 minors.

Top blockers:

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

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>

This was referenced May 7, 2026

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

Open

feat(openspec): platform capabilities catalog + three declarative-annotation deltas #1353

Merged

WilcoLouwerse reviewed May 7, 2026

View reviewed changes

WilcoLouwerse requested changes May 7, 2026

View reviewed changes

WilcoLouwerse merged commit 29fc82d into feature/declarative-engines-adr024-027 May 7, 2026
2 checks passed

WilcoLouwerse deleted the feature/split-aggregations-and-calculations branch May 7, 2026 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(openspec): split aggregations-and-calculations into two single-mechanism specs#1354

chore(openspec): split aggregations-and-calculations into two single-mechanism specs#1354
WilcoLouwerse merged 1 commit into
feature/declarative-engines-adr024-027from
feature/split-aggregations-and-calculations

rubenvdlinde commented Apr 29, 2026

Uh oh!

WilcoLouwerse May 7, 2026

Uh oh!

WilcoLouwerse May 7, 2026

Uh oh!

WilcoLouwerse May 7, 2026

Uh oh!

WilcoLouwerse May 7, 2026

Uh oh!

WilcoLouwerse May 7, 2026

Uh oh!

WilcoLouwerse May 7, 2026

Uh oh!

WilcoLouwerse May 7, 2026

Uh oh!

WilcoLouwerse May 7, 2026

Uh oh!

WilcoLouwerse left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

rubenvdlinde commented Apr 29, 2026

Summary

Demonstrates

Uh oh!

Choose a reason for hiding this comment

🔴 Blocker — Garbled scenario title in aggregations-annotation

Uh oh!

Choose a reason for hiding this comment

🔴 Blocker — computeOn trigger-based recomputation has no implementation task

Uh oh!

Choose a reason for hiding this comment

🔴 Blocker — PlaceholderResolver dependency on aggregations-annotation not declared in calculations tasks

Uh oh!

Choose a reason for hiding this comment

🔴 Blocker — Listener namespace inconsistency between the two split specs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WilcoLouwerse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants