Skip to content
This repository was archived by the owner on May 29, 2026. It is now read-only.

feat(quorum-schema-declaration): implement spec (#151)#155

Closed
rubenvdlinde wants to merge 3 commits into
developmentfrom
feature/151/quorum-schema-declaration
Closed

feat(quorum-schema-declaration): implement spec (#151)#155
rubenvdlinde wants to merge 3 commits into
developmentfrom
feature/151/quorum-schema-declaration

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Closes #151

Summary

Auto-generated draft PR for OpenSpec change quorum-schema-declaration.
The Hydra builder ran the spec but could not run gh pr create itself
(Phase D+E credential strip — Claude has no GH_TOKEN by design).
The entrypoint detected commits on the feature branch with no PR and
created this draft so the reviewer + security + applier can proceed.

Spec Reference

Commits on this branch

Files changed

  • docs/data-model.md
  • lib/Settings/decidesk_register.json
  • openspec/architecture/adr-000-data-model.md
  • openspec/changes/quorum-schema-declaration/design.md
  • openspec/changes/quorum-schema-declaration/tasks.md
  • package-lock.json
  • tests/Integration/Meeting/QuorumDeclarativeTest.php
  • tests/unit/Controller/SettingsControllerTest.php

PR auto-created by Hydra builder entrypoint (hydra_ensure_pr_exists)
because Claude's session closed without running gh pr create.
Reviewer + applier follow as normal.

infoconductionnl and others added 3 commits May 7, 2026 20:44
Declares x-openregister-aggregations (totalParticipantCount,
presentParticipantCount) and x-openregister-calculations
(quorumPercentage, quorumMet) on the Meeting schema in
decidesk_register.json, making quorum status readable as derived
fields on every Meeting object without imperative service code.

Also adds attendanceStatus field to Participant schema (required for
the presentParticipantCount aggregation filter) and an integration test
that skips cleanly in the build container but asserts against a live
OpenRegister engine when DECIDESK_INTEGRATION=1.

Chain spec 1 of 3 (quorum-schema-declaration). Successors
quorum-guard-rewrite and quorum-service-deletion depend on this merging.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Code Review — Juan Claude van Damme

Result: PASS (1 merge conflict resolved, 0 unfixed, 0 blocking)

What was reviewed

PR #155 adds declarative quorum fields to the Meeting schema via decidesk_register.json (x-openregister-aggregations for totalParticipantCount / presentParticipantCount, x-openregister-calculations for quorumPercentage / quorumMet) and a corresponding integration test suite that properly soft-fails in non-provisioned environments.

Merge conflict resolved

tests/unit/Controller/SettingsControllerTest.php had a conflict between origin/development and the PR branch on the getUser() mock setup. Took the origin/development version — expects($this->once()) is the stricter and correct assertion; the PR's bare willReturn() lacked the call-count expectation. Committed as merge commit 84406ce.

Quality suite results

Check Result
hydra-gates (14 gates) ALL GREEN
phpcs (in-scope files) PASS
psalm (in-scope) PASS (errors on out-of-scope files only)
phpstan (in-scope) PASS (errors on out-of-scope files only)
phpunit 146 tests, 30 skipped, 0 failures
npm lint 0 errors, 49 warnings (all out-of-scope Vue files)

Out-of-scope inherited debt (informational)

The following pre-existing issues were found on unchanged files — they are NOT blockers for this PR but should be addressed in a follow-up cleanup PR:

  • lib/AppInfo/Application.php:187 — Psalm/PHPStan: AnalyticsController::__construct called with too few arguments (missing userSession, groupManager). Pre-existing.
  • lib/Controller/SettingsController.php:140 — PHPStan: AuthorizedAdminSetting param expects class-string<IDelegatedSettings> but string given.
  • lib/Service/ActionItemAnalyticsService.php:224,226 — PHPStan: always-true comparison from PHPDoc type.
  • PHPMD: Complexity warnings on ALVMinutesService, MeetingService, MinutesService, MotionService, VotingService, etc.
  • ESLint: 49 warnings across src/views/LiveMeeting.vue, MeetingDetail.vue, MotionDetail.vue (deprecated OC.generateUrl, component option ordering).

🤖 Changes Juan Claude van Damme applied

View full diff · 14 files changed, 295 insertions(+), 1 deletion(-)


$this->settingsService->expects($this->once())
->method('getSettings')
->willReturn($settings);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[fixed: merge conflict resolved] Chose origin/development version: expects($this->once())->method('getUser')->willReturn(...) over PR's bare method('getUser')->willReturn(...). The expects($this->once()) asserts the mock is called exactly once, which is the correct stricter form for this test.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Security Review — Clyde Barcode

Result: FAIL (1 fixed CRITICAL, 1 unfixed WARNING)

Count
Fixed 1
Unfixed blocking 1 (WARNING)
Informational (inherited debt) 1

Fixed

  • [CRITICAL] AnalyticsController DI missing userSession+groupManagerlib/AppInfo/Application.php:188. Factory omitted two required constructor params; PHP ArgumentCountError on instantiation meant all three analytics endpoints 500'd before auth/admin guards ran. Fixed by mirroring VotingBehaviourController factory pattern in same file.

Unfixed (blocking)

  • [WARNING] getMyItems() filters by display name — IDOR risklib/Controller/AnalyticsController.php:148 / lib/Service/ActionItemAnalyticsService.php:269. User display names are non-unique and user-controlled in Nextcloud; using them as an item filter allows impersonation and cross-user data access (OWASP A01:2021). Safe fix requires confirming whether assignee field stores UIDs or display names in existing data — out of bounded fix scope.

Inherited debt (informational, non-blocking)

  • npm audit: 37 CVEs in axios / dompurify / @nextcloud/* transitive deps. Pre-existed on origin/development; this PR makes no package.json changes. Recommend dedicated dep-bump PR.

See inline comments for per-finding detail.


🤖 Changes Clyde Barcode applied

View full diff · 14 files changed, 291 insertions(+), 1 deletion(-)

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

📍 lib/AppInfo/Application.php:188 (outside PR diff — line-comment API rejected, posted as issue comment)

[fixed: missing DI wiring restored admin/auth guards] Rule: CWE-287 / OWASP A07:2021 — AnalyticsController factory omitted required IUserSession and IGroupManager params; PHP ArgumentCountError on instantiation bypassed all auth checks. Added both params mirroring VotingBehaviourController factory pattern in same file.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

📍 lib/Controller/AnalyticsController.php:148 (outside PR diff — line-comment API rejected, posted as issue comment)

[unfixed: WARNING — escalated, data-schema investigation required] Rule: OWASP A01:2021 — getMyItems() passes $user->getDisplayName() as assignee filter. Display names are non-unique and user-controlled in Nextcloud; two users sharing a display name see each other's action items. Fix: change to $user->getUID() and verify assignee field stores UIDs in existing ActionItem records before applying.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Closing — wrapper bug (push to literal pr-branch instead of PR head ref) caused Juan's merge-conflict resolution and Clyde's CRITICAL DI fix to be lost. Fixed in hydra#239. Rebuilding from scratch with new wrapper to validate end-to-end pipeline.

@rubenvdlinde rubenvdlinde deleted the feature/151/quorum-schema-declaration branch May 8, 2026 11:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants