feat(openspec): platform capabilities catalog + three declarative-annotation deltas#1353
feat(openspec): platform capabilities catalog + three declarative-annotation deltas#1353rubenvdlinde wants to merge 5 commits intodevelopmentfrom
Conversation
Quality Report — ConductionNL/openregister @
|
| 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 05:54 UTC
Download the full PDF report from the workflow artifacts.
…ative-engines-adr024-027
…r CVEs Resolves PKSA-hznc-gbby-6w16 (CVE-2026-40296) and PKSA-jtdk-dcr5-f11n (CVE-2026-35453). Cascaded minor bumps to symfony/var-dumper, symfony/deprecation-contracts, symfony/polyfill-mbstring (all in-range).
rubenvdlinde
requested review from
Rem-Dam,
WilcoLouwerse,
bbrands02 and
rjzondervan
as code owners
|
🟢 Minor — Newman Integration Tests CI failure is expected false-positive for a spec-only PR The PR adds only openspec/ markdown files and a composer.lock bump. Newman tests exercise live API endpoints; they don't run against spec files. The failure is almost certainly a pre-existing flake or an environment issue unrelated to this PR's changes. No code was changed that could affect Newman test outcomes. Suggested: Confirm the Newman failure is pre-existing by checking whether the same failure appears on the development branch. If so, document it as a known flaky test and re-run. If it is new, investigate whether the composer.lock bump introduced a transitive dependency issue. |
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Strict-mode review — 7 blockers, 8 concerns, 3 minors.
Top blockers:
- 🔴 POST /transition endpoint: spec omits CSRF protect
- 🔴 ObjectTransitionedEvent for direct PATCH: action f
- 🔴 Aggregations RBAC requirement doesn't cover multi-
- 🔴 notifications-annotation threshold trigger creates
- 🔴 Five NC-app integration providers marked 'proposed
- … +1 more 🔴 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>
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
All 17 findings from the first review are resolved in 4c268e0 — 13 verified in the earlier re-review pass; the remaining 4 verified just now: composer.lock plugin-api-version, NoAdminRequired in spec body, time-bucket UTC semantics, throttle grammar. No new findings. The Newman Integration Tests CI failure is expected for a spec-only PR (no controllers exercised) and is not a required check on development (ruleset 14128349 has no required_status_checks). Approving.
Resolves three conflicts surfaced by development's catch-up of the three
declarative-annotation deltas (now archived under openspec/changes/archive/
as of 2026-04-29 / 2026-05-01) and the newer pilot-grade catalog status.
- openspec/changes/aggregations-and-calculations/{tasks.md,
specs/zoeken-filteren/spec.md}: accept dev's delete. The change has
been split into aggregations-annotation (archived 2026-04-29) and
aggregations-backend-native (in progress on dev). The PR's edits to
the bundled directory are superseded.
- openspec/platform-capabilities.md: keep dev's NC-providers block
(richer pilot status: implemented / partial / "Verified: ..." notes)
written by Wilco against the live implementation. The PR's earlier
proposed/implemented call for that section is dropped in favour of
the verified state.
Renames detected by the merge moved openspec/changes/{lifecycle-annotation,
notifications-annotation}/* into openspec/changes/archive/2026-04-29-*/
and carried this PR's review-fix edits along (uniqueness constraint,
StoppableEvent rejection contract, channel block table, etc.).
Those edits now live in the archive directory rather than active
specs — left as-is rather than reverted. If the live specs that
graduated from these changes need the same fixes, that is a
follow-up rather than a merge-resolution concern.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Re-review (Thorough): the fix commit 4c268e0 textually addressed the 13 prior blockers/concerns I marked resolved at 14:30 (✓), but it introduced 2 new contradictions between the new normative MUSTs and the existing implementation: Auth contract MUST NOT use @NoCSRFRequired — the shipped TransitionController has it; and setRejectionReason/getRejectionReason mandated on ObjectUpdatingEvent — the shipped class has setErrors/getErrors. Two non-blocking concerns: the new MUSTs are being added to archived spec directories so they don't propagate to the canonical openspec/specs/event-driven-architecture/spec.md, and composer.lock bundles 4 unrelated symfony bumps into a spec-only PR. (CI: 3 failing checks — License (composer), PHP Quality (phpcs), Vue Quality (eslint) — all informational; the development ruleset has no required status checks.)
Two blockers + two concerns from the 2026-05-08 review pass: 1. Blocker — CSRF: TransitionController::transition() no longer carries @NoCSRFRequired. The spec already says state-mutating POSTs MUST go through NC's standard CSRF middleware; the implementation now matches. 2. Blocker — rejection API: lifecycle spec + tasks now reference the shipped setErrors/getErrors API on ObjectUpdatingEvent instead of a new setRejectionReason/getRejectionReason pair. Renaming the publicly-exposed event API would break third-party listeners that already call setErrors(). 3. Concern — archive vs canonical: new openspec change directory lifecycle-notifications-amendments graduates the recent normative MUSTs (uniqueness constraint, auth contract, rejection-metadata contract, action-resolution rule, channel-block format, throttle window grammar, created/updated triggers) from the archived 2026-04-29-* change folders into ## ADDED Requirements blocks targeting the canonical event-driven-architecture and notificatie-engine specs. 4. Concern — composer.lock dep bumps: reverted. The symfony 6.4.x patch bumps belong in a separate chore(deps) PR, not this spec-only catalog PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
All 4 prior findings resolved in 0ed01171b and verified at HEAD: @NoCSRFRequired removed from TransitionController::transition(); spec reuses the existing setErrors/getErrors API on ObjectUpdatingEvent (no breaking rename); new active change directory openspec/changes/lifecycle-notifications-amendments/ graduates the new MUSTs from archive into canonical event-driven-architecture + notificatie-engine specs; composer.lock matches origin/development exactly. CI: 3 non-required checks still red (License (composer), phpcs, eslint) — informational; no required status checks on the development ruleset.
2 participants
Summary
Replace three earlier draft "engines" with three small deltas against already-implemented specs, and add a single source-of-truth platform capabilities catalog so apps + Specter + Hydra all see what the platform already provides.
What used to be over-scoped
I'd drafted three standalone "engines" (lifecycle / aggregation / notification). The audit was right to push back: OpenRegister already has the foundation (event-driven-architecture, webhook-payload-mapping, notificatie-engine, zoeken-filteren). The deltas in this PR are the small declarative shortcuts that sit ON TOP of those, not new subsystems.
Three deltas (each ~5 files: proposal + design + tasks + specs//spec.md)
lifecycle-annotationevent-driven-architecturex-openregister-lifecycleObjectUpdatingEventrejects invalid transitions. SugarPOST /transitionendpoint.ObjectTransitionedEventjoins the 39-event family.aggregations-and-calculationszoeken-filterenx-openregister-aggregations,x-openregister-calculationsnotifications-annotationnotificatie-enginex-openregister-notificationsEach delta uses OpenSpec's terse format: short proposal + design, numbered tasks, and
specs/<existing-spec>/spec.mdwith## ADDED Requirementsblocks containing GIVEN/WHEN/THEN scenarios — same shape asnotificatie-engine's existing change directory.Platform capabilities catalog
openspec/platform-capabilities.md— five buckets: schema annotations, NC-app integration providers, object interactions, infrastructure, frontend abstractions. Catalogs every capability with status + spec link + one-line description. Read by Specter context briefs + Hydra builder skills + the spec linter.Sequencing
Each delta is implementable independently. Suggested order:
lifecycle-annotation(smallest, validates the pattern, unlocks composition)aggregations-and-calculations(largest; the calculation evaluator is the new substantive code)notifications-annotation(thinnest; mostly reusesnotificatie-engine)Companion changes elsewhere:
hydra— ADR-024 / 025 / 026 / 027 proposals (fix 1-1-1970 date on published #202–Documentation Build Failed #205); spec linter extension (Documentation Build Failed #206)concurrentie-analyse— Phase 5 classifier extensions (Development to master #32 + [Changelog CI] Add Changelog for Version 0.1.18 #33)Cross-references
@conduction/nextcloud-vue— separate PR)🤖 Generated with Claude Code