Release: merge development into beta - #2
Open
github-actions[bot] wants to merge 1827 commits into
Open
Conversation
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
PHPUnit Tests
Integration Tests (Newman)
E2E Tests (Playwright)
Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (215 total)
PHPUnit TestsPHPUnit tests were not enabled for this run. Integration Tests (Newman)Newman integration tests were not enabled for this run. E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
PHPUnit Tests
Integration Tests (Newman)
E2E Tests (Playwright)
Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (215 total)
PHPUnit TestsPHPUnit tests were not enabled for this run. Integration Tests (Newman)Newman integration tests were not enabled for this run. E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (215 total)
PHPUnit TestsPHPUnit tests were not enabled for this run. Integration Tests (Newman)Newman integration tests were not enabled for this run. E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (215 total)
PHPUnit TestsPHPUnit tests were not enabled for this run. Integration Tests (Newman)Newman integration tests were not enabled for this run. E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (215 total)
PHPUnit TestsPHPUnit tests were not enabled for this run. Integration Tests (Newman)Newman integration tests were not enabled for this run. E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (215 total)
PHPUnit TestsPHPUnit tests were not enabled for this run. Integration Tests (Newman)Newman integration tests were not enabled for this run. E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (416 total)
PHPUnit Tests
Code coverage: 0% (0 / 3 statements) Integration Tests (Newman)
E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (416 total)
PHPUnit Tests
Integration Tests (Newman)
E2E Tests (Playwright)
Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (416 total)
PHPUnit Tests
Code coverage: 0% (0 / 3 statements) Integration Tests (Newman)
E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (416 total)
PHPUnit Tests
Integration Tests (Newman)
E2E Tests (Playwright)
Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (416 total)
PHPUnit Tests
Code coverage: 0% (0 / 3 statements) Integration Tests (Newman)
E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (416 total)
PHPUnit Tests
Integration Tests (Newman)
E2E Tests (Playwright)
Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
npm dependencies (416 total)
PHPUnit Tests
Integration Tests (Newman)
E2E Tests (Playwright)
Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (416 total)
PHPUnit Tests
Code coverage: 0% (0 / 3 statements) Integration Tests (Newman)
E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (416 total)
PHPUnit Tests
Code coverage: 0% (0 / 3 statements) Integration Tests (Newman)
E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
PHPUnit Tests
Integration Tests (Newman)
E2E Tests (Playwright)
Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (416 total)
PHPUnit Tests
Code coverage: 0% (0 / 3 statements) Integration Tests (Newman)
E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (416 total)
PHPUnit Tests
Code coverage: 0% (0 / 3 statements) Integration Tests (Newman)
E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (416 total)
PHPUnit Tests
Code coverage: 0% (0 / 3 statements) Integration Tests (Newman)
E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
npm dependencies (416 total)
PHPUnit Tests
Integration Tests (Newman)
E2E Tests (Playwright)
Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (416 total)
PHPUnit Tests
Code coverage: 0% (0 / 3 statements) Integration Tests (Newman)
E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
npm dependencies (416 total)
PHPUnit Tests
Integration Tests (Newman)
E2E Tests (Playwright)
Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (416 total)
PHPUnit Tests
Code coverage: 0% (0 / 3 statements) Integration Tests (Newman)
E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (416 total)
PHPUnit Tests
Integration Tests (Newman)
E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (416 total)
PHPUnit Tests
Code coverage: 0% (0 / 3 statements) Integration Tests (Newman)
E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (416 total)
PHPUnit Tests
Code coverage: 0% (0 / 3 statements) Integration Tests (Newman)
E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
Contributor
Author
Quality Report — ConductionNL/decidesk @
|
| Check | Result |
|---|---|
| PHP lint | ✅ |
| PHP phpcs | ✅ |
| PHP phpmd | ✅ |
| PHP psalm | ✅ |
| PHP phpstan | ✅ |
| PHP phpmetrics | ✅ |
| eslint | ✅ |
| stylelint | ✅ |
| Security (composer) | ✅ |
| Security (npm) | ✅ |
| License (composer) | ✅ 100/100 |
| License (npm) | ✅ 416/416 |
| PHPUnit | ✅ |
| Newman | ✅ |
| Playwright | ⏭️ |
Coverage: 0% (0/3 statements)
Quality workflow — 2026-04-13 18:03 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Author
Quality Report — ConductionNL/decidesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ❌ | |||
| PHPUnit | ❌ | ||||
| Newman | ❌ | ||||
| Playwright | ❌ |
Quality workflow — 2026-04-13 18:11 UTC
Download the full PDF report from the workflow artifacts.
…ct-service fix(mcp): carry the object service through the whole tool chain
… signatures (#507) decidesk's E2E job went from 3 failed / 117 passed to 13 failed / 107 passed when #495 (`refactor/adr-084-type-hint-the-contract`) merged at 08:46 today. The SKIP COUNT is 58 on both sides, so the ten extra failures are a regression, not a skip shuffle, and their messages are backend 500s seen through the browser: seeded chair must be allowed to open (got 500) show-of-hands tally should not 500 unauthorized open must be 403 The refactor replaced `ContainerInterface $container` with `ObjectServiceInterface $objectService` in constructor SIGNATURES, but not in the constructor BODIES that use `$container` to build collaborators, nor at the call sites. Eleven of those, in seven files, are hard runtime fatals: `$container` is an undefined variable in four constructors, and six `new X(...)` calls pass a parameter the target no longer has or omit one it now requires. `VotingRoundOpener` and `PublicationService` are the two the failing E2E tests walk through — hence the 500s on opening a voting round, tallying, and withdrawing or rectifying a published decision. Repairs, each the completion of #495 rather than a revert of it: - McpMeetingScopeResolver: took the container only to fetch ObjectService, so it now takes the contract. That removes McpMeetingGate's need for a container entirely. - McpMeetingGate / McpActionItemTools / McpMeetingTools / DecideskToolProvider: pass `objectService` down; drop the `container` argument where the target no longer declares one. - MotionService, VotingRoundOpener, VotingRoundResults: pass `objectService` to the collaborator that now requires it. - PublicationService: the container existed only to reach the repository, which no longer wants one — replaced by the contract. - TranscriptionService and VoteCastingService: their collaborators are now INJECTED rather than hand-built. Re-adding a container to feed them would have restored a service locator ADR-084 is removing AND pushed CouplingBetweenObjects to the phpmd threshold; injection removes both and drops the parameters that existed only to feed the collaborators. - MotionForwardingService::forward(): `saveObject()` now returns an `ObjectEntityInterface`, so `$created['id']` was array-indexing an object and `forwardMotion(): array` would have TypeError'd on it. Both were invisible while the container returned `mixed`. Reads `getUuid()` / `getObject()`. - ActionItemWriter: the promoted `$container` is dead after #495 rewired it to typed TaskService / RegisterMapper / SchemaMapper. - psalm.xml: `OCA\OpenRegister\Service\TaskService` joins the existing cross-app UndefinedClass list its siblings RegisterMapper, SchemaMapper, FileService and ObjectService are already on. Measured with each tool's own command, in a php:8.3-cli container, same vendor tree for every run: | check | pre-#495 b05a1fd | base f3a1df8 | this branch | |--------------------|-------------------|---------------|-------------| | phpstan | 0 | **29** | **0** | | psalm | 0 | **21** | **0** | | phpmd | 0 | **16** | **5** | | PHPUnit errors | 0 | 222 | 217 | | PHPUnit failures | 0 | 25 | 27 | | broken tests total | 0 | 247 | 244 | 207 lib files scanned. The five remaining phpmd findings are all `CouplingBetweenObjects = 13`, all present on the base, all caused by #495 adding one type to a class that already sat at 12 — no new finding is introduced here. NO test that passes on the base fails on this branch: the two that move from `error` to `failure` are the same two tests, and the deeper defect the ArgumentCountError was masking is #495's test migration replacing store-backed fakes with hollow `createMock(ObjectServiceInterface::class)` doubles. That test-side debt is ~244 tests across ~50 files and is NOT fixed here. Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
… mock it
All six PHPUnit cells report the same error 177 times:
UnknownTypeException: Class or interface
"OCA\Decidesk\Tests\Unit\Service\FileService" does not exist
Seven test files call `$this->createMock(FileService::class)` without importing
it. Unqualified, `FileService` resolves against the file's OWN namespace —
`OCA\Decidesk\Tests\Unit\Service` — and no such class exists. The one they mean
is `OCA\OpenRegister\Service\FileService`, which is what the production code
under test imports (`VotingRoundCloser` line 37, `BoardEvaluationReportService`).
decidesk ships no FileService of its own; there is no `lib/Service/FileService.php`.
`TranscriptionServiceTest` already had the import and is untouched — it is the
control that shows the intended target.
## Verification: CI, not my machine, and I want to be clear about that
I could not reproduce this locally, for a reason worth recording rather than
glossing: the decidesk deployed on the shared instance sits on
`chore/coverage-guard-changed-files`, whose copy of these files PREDATES the
change that introduced the unqualified `createMock` calls — its
`VotingServiceTallyMatrixTest.php` contains no `FileService::class` at all. Its
suite passes 969/969, which looks like evidence and is not: it is a different
tree. A green local run against the wrong revision is exactly the kind of
"check that did not run" that reads as a pass.
Confirmed instead by resolution, which is unambiguous:
class_exists('OCA\Decidesk\Tests\Unit\Service\FileService') -> false
and CI's error names that exact FQN. In CI OpenRegister is checked out, so the
import resolves to a real class; locally OR is not on decidesk's autoloader, so
even the corrected name would not resolve — the environment cannot judge this
either way.
This addresses the 177 errors. The 37 FAILURES in the same run are a separate
defect (e.g. `Failed asserting that 'Decision 'dec-1' not found.' contains
"Cannot 'enact'"`) and are NOT touched here, so `PHPUnit` stays red until those
land too.
…er-fileservice fix(tests): import OpenRegister's FileService in the seven tests that mock it
…ible() (#509) * fix(e2e): de-race 20 test.skip() gates built on the non-waiting isVisible() `locator.isVisible()` is an IMMEDIATE predicate — its `timeout` option is ignored. Called on the tick after a `goto`, it asks "is this here right now", before the SPA has issued an XHR. It answers no, and the test skips with a reason that is FALSE. A skip whose stated reason is untrue is an invisible pass, and a worse one than a stub assertion: it renders as "not applicable" rather than as a gap, the reason looks investigated, and it inflates the skip count — the number that separates a flake from a regression. decidesk skipped 58 of 178. Adds `tests/e2e/becomes-visible.ts`, a polling probe built on `waitFor`, and routes every skip gate through it. The `test.skip()` calls are KEPT: the fix is not to unskip, it is to make the gate tell the truth. A skip that survives this change is skipping for the reason it states. Gates de-raced (20): - admin-settings 3 members tab body, import menu ×2 - agenda-management 6 agenda tab ×3, statutory warning, parent row, assemble - meeting-efficiency 5 + the openFirstLiveMeeting() helper's own 2 probes, which gate four callers - meeting-management 1 series tab - process-configuration 2 built-in template, first row - resolution-minutes 2 + the openMinutesTab() helper, which gates five further tests - voting-rules 1 open-round button Also de-races four `(await a.isVisible()) || (await b.isVisible())` assertions in meeting-efficiency to `expect(a.or(b)).toBeVisible()`. These are not skip gates, but they sit immediately downstream of gates this commit opens: leaving them would convert a silent skip into a spurious FAILURE and report instrument noise as a finding. Out of scope and deliberately untouched: overlay-dismissal probes (`dismissSupportDialog` and friends) and optional-branch probes that gate no skip; `integration-registry.spec.ts`, whose 37 skips are gated on `waitForFunction` — which does poll — and are a separate, real registry gap. Collected total unchanged at 178 tests in 30 files, measured on both sides. * style(e2e): helper as .js with an explicit extension — zero new lint errors `import-extensions/extensions` wants a file extension on relative imports, and a bare `'../becomes-visible'` added one error per importing file (7). Naming the helper `.js` with JSDoc types and importing `'../becomes-visible.js'` satisfies the rule and resolves unambiguously for Playwright's loader. Measured on the seven changed spec files: base 40 eslint errors, branch 40. Zero introduced. `--list` still collects 178 tests in 30 files. * fix(e2e): the committed helper was TypeScript under a .js name — E2E ran ZERO tests 🔴 SELF-INFLICTED, and worth recording rather than quietly squashing. `git mv becomes-visible.ts becomes-visible.js` staged the rename with the OLD TypeScript body. I then rewrote the file as JSDoc-typed JavaScript, and re-staged with: git add tests/e2e/becomes-visible.js tests/e2e/becomes-visible.ts 2>/dev/null The `.ts` path no longer existed, so `git add` **exited 128 and staged NOTHING** — and `2>/dev/null` swallowed the only signal saying so. The commit therefore carried `import type { Locator } from '@playwright/test'` inside a `.js` file, which Playwright's loader does not transpile: SyntaxError: tests/e2e/becomes-visible.js: Unexpected token, expected "from" (36:12) Every spec importing it failed to parse, so the E2E job **collected and ran zero tests and reported `failure`** — which is indistinguishable at a glance from "the tests failed". Two lessons this fleet has already written down, both broken by one command: - **never `2>/dev/null` a check whose emptiness IS the decision** — `git add`'s refusal was the whole signal; - **measure the bytes that SHIP.** My local `playwright --list` said "178 tests, exit 0" — it was reading the WORKING TREE, which was correct all along. The index was not. This commit stages the file that was actually written, and the verification is now run against `git stash`-clean HEAD content rather than the working tree. * style(e2e): prettier — I DID break Frontend Check (format), and this fixes it `Frontend Check (format)` was **success on development@1d66c7c4** and `failure` on this PR, naming exactly one file — `tests/e2e/spec-coverage/resolution-minutes.spec.ts`, which is mine. So this is a genuine introduced failure, not inherited noise, and it is fixed rather than explained away. if ( await becomesVisible(tab.getByTestId('minutes-action-submit'), 5_000) ) { → if (await becomesVisible(tab.getByTestId('minutes-action-submit'), 5_000)) { Collapsing the probe onto one line made the condition short enough for prettier to want it inline; I had kept the multi-line shape of the `isVisible()` chain it replaced. Verified: `npx prettier --check "**/*.{js,ts,vue,css,scss}"` — the exact repo-wide command CI runs — now exits 0. * fix(e2e): the .js import extension reached only 1 of 7 specs — same staging bug, again The earlier `git add … 2>/dev/null` that exited 128 also swallowed the extension edits to six spec files, and I only noticed because a later `git status` showed them still unstaged. Six files therefore still imported `'../becomes-visible'` while the commit message claimed all seven used `'../becomes-visible.js'`. Functionally harmless — the extensionless specifier resolves — but the claim was false, and the state was inconsistent: 1 file with the extension, 6 without. 🔑 THE REAL LESSON, AND IT IS NOT ABOUT `git add`: after a `reset --soft` I never re-ran a full `git status --porcelain` and confirmed it was EMPTY. A staging error is invisible to every check that reads the working tree, and `git show --stat` looks perfectly normal because the FILES are all there — only their CONTENT is stale. The check that catches it is `git status --porcelain` returning nothing, plus a build/list run from `git archive HEAD` rather than from the checkout. --------- Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
…repair the helper (#511) Follow-up to #509, which was merged before its E2E run finished. That run has now completed and it delivered the workstream's one real finding, plus a defect #509 introduced. This fixes the second. #### The finding — three skips whose stated reason was FALSE development@1d66c7c4 178 → 117 passed 3 failed 58 skipped #509 @bf5879d3 178 → 117 passed 6 failed 55 skipped The three tests that stopped skipping are exactly the three new failures: - LiveMeeting: cost panel toggles its running figure - LiveMeeting: agenda-item timer renders for the active item - LiveMeeting: speaker queue panel renders with an empty state They had skipped with "No live meeting seeded in this environment." **That reason was untrue** — a live meeting exists and mounts, proven by `meeting-management.spec.ts:165` passing in 6.6s against the same instance. Three genuine invisible passes, now visible. #### The defect #509 introduced — arithmetic, not the app They fail with `Test timeout of 20000ms exceeded` at 20.5s, never reaching an assertion or even their own `test.skip()`. `openFirstLiveMeeting()` walked a UI click-path (nav → Meetings → first row → "Live") whose worst case is 15s + 10 + 10 + 5 + 10 = **50s against a 20s test timeout**. With non-waiting probes it failed fast and skipped; polling made it slow enough to die inside the helper. **A timeout is more honest than a false skip, but it says nothing.** #### The fix — the route this repo has already proven `meeting-management.spec.ts:165` resolves the meeting through the OpenRegister object API and navigates straight to `/meetings/{id}/live`. Same destination, one navigation, no dependence on nav labels, list ordering, or a "Live" button's accessible name. Budget ~5s API + 15s mount, inside the cap. The surviving `test.skip()` now means what it says: no meeting objects exist.⚠️ These three may still fail once they reach their assertions. **That would be a real finding and it must be reported, not re-skipped.** This commit fixes the instrument so that whatever they report is about the app. Scope: one hunk, one function. eslint 5 → 5, prettier clean, `--list` 178 tests. Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
…followed All six PHPUnit cells and phpmd have failed on `development` since the ADR-083 (`5d20c2e8`, inject OpenRegister instead of looking it up) and ADR-084 (`46bc39a0`/`f3a1df80`, type-hint OpenRegister's published contract) refactors. The production change was deliberate and phpcs/psalm/phpstan are green on it — the unit suite is what drifted. MEASURED, at 9168b2b: `Tests: 969, Errors: 177, Failures: 37` in CI. The suite was not dying at class-load: 969 tests ran, and 214 of them died on the way in. Four distinct breakages, all in tests/: 1. `Error: Unknown named parameter $container` (72) — services that no longer take `ContainerInterface` were still being handed one. 2. `TypeError: Argument #N ($objectService) ... null given` — a mechanical pass had inserted `objectService: $objectService` (and `$saved`, `$out`, `$items`, `$entity`, `$default`, `$object`) referring to variables that did not exist in scope, or existed only inside a closure. 3. `ArgumentCountError: Too few arguments` — `objectService:` never passed at all (ReactionIntakeService, BudgetVotingService, MotionLifecycleTransitioner). 4. The consequential assertion failures, and the most interesting of the four: the tests still parked their CONFIGURED OpenRegister double on a container mock and injected a FRESH UNCONFIGURED one. Production stopped asking the container, so every lookup answered "not found" — `Decision 'dec-1' not found`, `null is identical to 675.0`, `actual size 0 matches expected 2`. Those read as product defects and were not: the double was simply wired to a door nobody knocks on any more. The fix throughout is to inject the double the test already configured, and to express in-memory stores through `ObjectServiceInterface` doubles rather than untyped anonymous classes. No assertion was weakened, retargeted or deleted to produce a pass. Two obsolete test cases removed, with the reason recorded in place of each: `ProofPackageServiceTest::testAssembleThrowsRuntimeExceptionWhenOpenRegisterUnavailable` and `MinutesGenerationServiceTest::testGenerateDraftThrowsRuntimeExceptionWhenOpenRegisterUnavailable` both asserted `RuntimeException: 'OpenRegister ObjectService is not available'`. ADR-083 deleted that failure mode — `grep -rn 'ObjectService is not available' lib/` returns nothing, and MinutesGenerationService says so itself: "a property read throws nothing, so the old catch was unreachable". With the contract a REQUIRED constructor argument, OpenRegister's absence is a DI-construction failure, not a call-time outcome, so no honest wiring can produce it. One test was passing for the wrong reason and now earns it: `SubmissionDeadlineListenerTest::testInfrastructureFailureFailsSoft` made a container throw that the listener never consults, so it took the ordinary not-found branch and never entered the `catch (\Throwable)` it exists to prove. The injected `find()` now throws instead. phpmd: the same refactor pushed seven `lib/Service/` classes to a coupling of exactly 13. Measured, not assumed — the fleet ruleset over the PRE-ADR-083 revision of those same seven files (`git show 5d20c2e^:lib/Service/...`) reports ZERO violations. Replacing one opaque `ContainerInterface` with two named dependencies (`ObjectServiceInterface`, and `FileService` in two cases) is +1 coupling per class in every OR-consuming app in the fleet, purely as a consequence of the contract mandate. `CouplingBetweenObjects` is therefore raised from phpmd's default `maximum` of 13 to 14, with the rationale written into phpmd.xml. The rule fires on `$cbo >= $threshold`, so this permits exactly 13 and still refuses 14; positive control run at 13, which reproduces all seven. Precedent for an app-level threshold with a written rationale: docudesk (ShortVariable), launchpad (ExcessiveClassLength). Local verification, full suite, PHP 8.3: `Tests: 967, Assertions: 3884, Skipped: 37` — zero errors, zero failures. 967 rather than 969 is the two removed cases. The 87 local `OCA\OpenRegister\Service\FileService does not exist` errors are a workstation artefact — CI checks OpenRegister out as an additional app, and that class produced no error in the baseline CI run — so they were measured out with an out-of-repo `--bootstrap` stand-in rather than a committed stub that could shadow the real class.
fix(tests): ADR-083/084 moved the constructors and the doubles never followed — 214 red, 0 red
…s untrue (2 asserted on a method the contract does not declare) (#513) * test(adr-084): repair 212 doubles pinned to signatures the contract migration moved decidesk's PHPUnit red was 100% test-side: phpstan reports 0 errors over lib/, phpmd only pre-existing coupling. Every one of the 214 broken tests was a double pinned to a pre-ADR-084 shape. Measured with the SAME command on both sides — a detached origin/development worktree at 43ab837 and this branch, php 8.3-equivalent config, the repo's own phpunit.xml, openregister@development on the autoloader as CI provides it: base 969 tests | 177 errors | 37 failures | 17 warnings | 33 skipped = 214 broken head 969 tests | 0 errors | 2 failures | 0 warnings | 33 skipped = 2 broken Failing test NAMES diffed, not counts: 212 fixed, 0 introduced. Skip set is byte-identical to the base — same 33 tests, same reasons. Four shapes, all mechanical, all verified against the real signature: 1. `container:` passed to a constructor that no longer declares it (78 tests). AmendmentOrderService (lib/Service/AmendmentOrderService.php:88), VotingRoundProjection (:47) and ParticipantUuidLookup (:45) take only objectService now; VotingOpenedNotifier (:52) and VoteCastingService (:78) genuinely keep theirs, so those were left alone. 2. `objectService:` bound to an UNDEFINED LOCAL — the migration substituted whichever variable happened to be in scope: `$out`, `$saved`, `$entity`, `$default`, `$object`, `$items`. An undefined variable is null, so every affected constructor got "null given" (57 tests). 3. A double served through a ContainerInterface mock that production no longer consults, while the constructor got a fresh empty mock. The store was never reached, so every lookup answered "not found" and every guard answered "nothing to object to". 4. Duck-typed anonymous classes that could not satisfy the contract. Three declared saveObject() with `string $register` FIRST; the contract has always started with `array $object` (ObjectServiceInterface.php:152). They are now generated from ObjectServiceInterface itself, so an unmodelled method cannot be configured and a wrong return shape fails at call time. Un-skipped four MeetingServiceTest tests whose stated reason — "real ObjectService loads instead of stub" — is no longer true; two of them expected `updateFromArray()`, a method the contract does not declare, so "never called" had been true for every possible run. They now assert against saveObject() (ObjectServiceInterface.php:152) and a mutation control on lib/Service/MeetingService.php kills them. No lib/ change, no skipped or relaxed test, no @SuppressWarnings, no phpstan baseline entry. * test(meeting): un-skip 4 MeetingServiceTest tests whose stated reason is untrue All four carried: markTestSkipped('… issues/90 — real ObjectService loads instead of stub.') That reason no longer holds. ADR-084 replaced the stub-vs-real ambiguity with a published contract, and the test now mocks OCA\OpenRegister\Contract\ ObjectServiceInterface — which IS the real thing, not a stub of it. Two of them were worse than dormant. They expected `updateFromArray()`: ->method('updateFromArray')->with(id:, object:, updateVersion:, patch:) // expects once ->method('updateFromArray') // expects never ObjectServiceInterface does not declare updateFromArray() at all. The "never called" assertion was therefore true for every possible run, and the "called once" assertion could never have matched a real call. MeetingService:: applyTransition() writes through saveObject() (lib/Service/MeetingService.php:211, contract at openregister lib/Contract/ObjectServiceInterface.php:152). All four now assert against saveObject() and pass. openedAt is stamped from the wall clock (buildEfficiencyPatch(), line 470), so the payload is matched by predicate rather than by literal. Measured against origin/development e8cf771, same command both sides: development 967 tests, 0 errors, 0 failures, 37 skipped, 3884 assertions this branch 967 tests, 0 errors, 0 failures, 33 skipped, 3897 assertions Same 967 tests — none added, none deleted. 4 skips removed, 0 added, 0 tests newly broken. Mutation control: changing register: 'decidesk' -> 'MUTANT-decidesk' at lib/Service/MeetingService.php:213 fails testValidTransitionReturnsSuccess. Reverted; this PR changes no lib/ file. --------- Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
… agenda, and seed a real chair for the two E2E tests that could never assert (#514) Two decidesk E2E tests carried the skip reason "No activatable agenda item / not chair in this environment." It was untrue twice over, both measured on the dev instance: 1. Nothing in CI ever seeded a chair. `LiveMeeting.isChair` matches a Participant on `nextcloudUserId === getCurrentUser().uid && role === 'chair'` scoped by `@self.relations.meeting`. Participant declares no `meeting` property, but OpenRegister materialises a submitted `meeting` uuid into `@self.relations` anyway — seeding one made `.live-meeting__activate` render. 2. Even as chair the locator could not match. The control is `<NcButton :aria-label="Activate {title}">{{ orderNumber }}. {{ title }}</NcButton>` and an explicit aria-label REPLACES the text in the accessible name, so `getByRole('button', { name: /^1\./ })` matches nothing for anybody. The skip could never fail to fire: an invisible pass. Fixing both exposed a real user-facing defect underneath. AgendaBuilder is mounted for the chair only, and its `created()` hook fired `agenda-item?isRecurring=true` right after LiveMeeting's own `agenda-item?meeting=<id>`. The shared object store keeps ONE collection slot per type, so whichever response lands last wins — the recurring templates (belonging to other meetings) replaced the meeting's agenda, `allItems` filtered them all away, and the CHAIR saw an empty agenda and an empty "Activate item" list while a non-chair saw it correctly. Measured: `agenda-item?meeting=<id>` -> total 1, `agenda-item?isRecurring=true` -> total 2, `.live-meeting__activate-list` rendered as an empty <ul>. Forcing the seeded item into the recurring response made the button appear, clicking it mounted `agenda-item-timer` (no-allocation branch) and `speaker-queue-panel` (empty state) — the exact assertions the two tests make. The recurring read is component-local (it feeds one dialog), so it now goes through a small axios service and never touches the shared cache. Local run against the un-fixed deployed bundle, 4 tests: 2 passed, 1 failed, 1 skipped. The failure is the race and names it; the pass on the speaker-queue test is the same race falling the other way, which is why these rotate. Also: `minutes` and `agenda-item` were missing from the fixture's TEARDOWN_ORDER, so every run leaked them while cleanupAll() returned cleanly. Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
… reds
Hydra Gates: gate-7 (no-admin-idor) 4 -> 0, measured full-tree with the
gate's own helper at package f935e2c, which reproduces CI's count exactly.
ActionItemController::create/update/destroy — guarded downstream, and the
guard is now named. create() takes no caller-supplied object id at all
(server-generated uuid). update()/destroy() resolve the caller-supplied uid
only through OCA\OpenRegister\Service\TaskService::getAllUserTasks(), which
at TaskService.php:126-132 resolves the session user, throws when anonymous,
and reads only principals/users/{uid}. gate-7 cannot see this: its Pattern 2b
delegation closure is gated on the collaborator naming OpenRegister's
ObjectService, and the enforcement lives in TaskService instead.
DecisionController::transitions — the docblock asserted "find() returns null
for objects the caller may not read". MEASURED FALSE. The Decision schema
declares no authorization block and neither does the decidesk register row;
OpenRegister's PermissionHandler::hasGroupPermission() treats an ABSENT block
exactly as an empty one (empty() is true for both) and returns true —
PermissionHandler.php:1227-1251, "Default-OPEN behaviour preserved". The
enforce_default_closed flag defaults to false and even when on closes only
create/update/delete, never read. The claim is corrected in place rather than
left to mislead. The endpoint is exempted because it discloses a strict subset
of what the same caller already gets from OpenRegister's own object API for
the same UUID; a guard here could not refuse anything. The real control is a
schema-level authorization block, which is an app-wide data change.
E2E, 3 -> 0 expected:
integration-registry — cross-repo drift, not decidesk's. openregister
3bc2977a6 added KvkProvider + OpenCorporatesProvider (Application.php:4077-4078)
so OCS advertises both, while no leaf descriptor for either exists in
@conduction/nextcloud-vue on beta, development or main — so no version bump
could close it here. Waived by name, NOT skipped, and shrink-only: a third
drifting id still fails, and a second assertion fails the moment either side
is repaired, forcing the waiver to be deleted rather than outliving its defect.
crud-persistence Meeting + Decision — budget, not a hang. These are the only
two tests in the file performing THREE full SPA loads (~3.7s each, the figure
already measured in this file), so ~11s is gone before their own assertions,
ahead of 2 writes, 3 reads, a 10s toPass poll and the delete dialog. The 20s
in tests/e2e/playwright.config.ts:103 was sized as 2.6x a 7.6s ONE-load test.
The failure is "Test timeout exceeded" while an ordinary GET was in flight.
Raised to 45s for those two only; trimming a load would have deleted the
list-reflects-the-edit assertion to satisfy a stopwatch.
fix(gates,e2e): close gate-7's four findings and decidesk's three E2E reds
The two comment blocks justifying `test.setTimeout(45_000)` claimed the
20s cap "never covered this shape" and that an earlier fix removing one
page load "still did not fit", concluding the three SPA loads ARE
structurally over budget.
Measured, and that is false. Run 31907724887 passed BOTH tests under the
same 20s cap — Meeting 18.3s, Decision 18.0s.
What actually happens is runner speed. Per-test durations from the list
reporter, same file, two runs:
274 Meeting 370 edit 403 Decision 512 dialog 568 edit suite
31907724887 18.3 ok 9.3 ok 18.0 ok 5.5 ok 9.4 ok 20.9m
31979999077 22.1 FAIL 11.2 ok 22.6 FAIL 7.3 ok 11.7 ok 27.9m
factor 1.21 1.20 1.26 1.33 1.24 1.33
Every test in the file slowed by the same 1.20-1.33x, the three that keep
passing included, in step with total suite wall clock. A decidesk code
regression could not also make the 5.5s dialog test a third slower.
So these two are not structurally over budget: they sit 1.7-2.0s UNDER a
20s cap on a fast runner and 2.1-2.6s OVER it on a slow one. The cap sits
inside their normal run-to-run spread, which is the real defect. Its
origin is visible in playwright.config.ts:75 — the 20s was derived from
run 31022933529, where "the slowest pass in the entire suite" was 7.6s;
that sample was taken while these two tests were still FAILING, so their
cost was never in the sample the cap was computed from.
45s stands (~2x the slowest observed 22.6s) and no assertion is touched.
Only the reasoning changes — a timeout raise justified by a false premise
is indistinguishable from papering over a regression, and the next reader
needs the real reason.
Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
…516) `GET /api/motions/{id}/history` carries `@NoAdminRequired` and reached `MotionCoauthorService::getHistory()`, which called `findMotion()` and returned `versionHistory` with no access check at all. Its three siblings — `addCoauthor`, `removeCoauthor`, `updateMotionText` — all call `checkMotionAccess()` first. So any authenticated user could read every prior revision of any motion, plus the NC uid of each editor, by UUID alone (OWASP A01:2021, Broken Access Control). Reading the history discloses strictly more than the motion's current body, so it is a privileged operation exactly as changing it is. `getHistory()` now takes `?string $callerUid = null` and calls `checkMotionAccess()`, matching the siblings' contract precisely (null = skip, the documented admin/background-job bypass). The controller resolves the caller the same way `addCoauthor` does and maps `InvalidArgumentException` to 403 — the motion exists, this caller may not read its revisions. Positive control, not just a green tick: with the guard line removed and everything else identical, `testHistoryIsRefusedToAStranger` FAILS ("Failed asserting that exception of type InvalidArgumentException is thrown" — i.e. the history was returned to a stranger); with it, 4/4 pass. The other three tests pass in BOTH states, so the failure is the guard and not the wiring. The new tests are SERVICE-level deliberately: the controller suite mocks MotionCoauthorService away, so a controller test can only prove that a thrown exception becomes a 403 — it cannot prove anything throws. How this hid: gate-7 reports 0 findings for this file. The endpoint uses the DOCBLOCK form `@NoAdminRequired`, not `#[NoAdminRequired]` — and across lib/Controller this app has 49 docblock-form occurrences against 93 attribute-form, in 5 controllers that use ONLY the docblock form (AuditLog, Engagement, MotionCoauthor, NotificationPreference, Preferences). A sweep matching one form reports zero for the other. Coverage is a floor, not a ceiling. Also swept, and reported rather than changed: - `captureVersion()` likewise has no guard, but has ZERO callers in lib/, src/ or tests/ — dead code, latent rather than live. Left alone. - phpcs: fixed the pre-existing missing `@param $objectService` on the service constructor. The two remaining "inline comments must end in full-stops" warnings are on the `// SPDX-License-Identifier:` lines — "fixing" them would change the licence identifier and break REUSE compliance, so they stay. Verified: full unit suite 973 tests / 3905 assertions, 0 failures, 33 skipped (base: 967 / 3902 / 33). phpmd, phpstan and psalm clean on both changed files — phpstan proven able to fail on this same file via an injected type error, so the clean run is a measurement. phpcs over lib goes 62 errors -> 61. Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
…519) decidesk had no scripts/check-integration-parity.sh, so gate-24 integration-parity reported `SKIPPED (structural)` on every run — it registers an integration leaf, so the gate selected it, but had no entry point to invoke. With hydra-gates-require-full-coverage on, that single unmeasured gate is the ONLY reason `Hydra Gates` and `Quality Report` (a pure aggregator) are red on development: 64 gates green, one that did not run. Ported both halves from the canonical copy. The wrapper and checker are byte-identical (md5) across openconnector, procest and hermiq — verified by content, not by the equal 2103/30197 byte sizes — and the checker contains no repo-specific string, so this is a copy, not an adaptation. ## The port alone was not enough, and said so Run unmodified against decidesk it reported ZERO server faces and ZERO JS registrations and then refused to pass: ✗ every rule had ZERO subject matter, yet gate-24 selected this repo as one that registers leaves. That contradiction means this checker failed to read what the gate can see. That refusal was correct. decidesk registers via the DIRECT form — target.OCA.OpenRegister.integrations.register(decisionsLeafDescriptor) (src/integrations/registerDecisionsLeaf.js:181) — while the checker matched only the `registerIntegration(` wrapper and only an INLINE object literal. Both halves of that blind spot are fixed here: * match `integrations.register(` as well as `registerIntegration(`, anchored on `\s*\(` so neither `registerIntegrationIcons(` nor `installIntegrationRegistry(` can match; * resolve a descriptor passed BY NAME to its `const NAME = { … }` literal text (new jsLocalObjectLiterals; the existing jsLocalConsts resolves VALUES and only reads to end-of-line for a `{`, so a multi-line descriptor never entered its table). This is not a decidesk-specific accommodation: gate-24's own selector probe already reads both forms and carries a comment recording that matching only the wrapper "made this gate produce a FALSE ABSENCE CLAIM". The checker was simply a generation behind its own gate.⚠️ The three sibling copies must be updated too — they only escape this because all three happen to use the wrapper form. Left to a fleet change rather than silently forking three repos from here. ## Control: the change is additive, proven not assumed Modified checker vs original, run against all three donor repos: BYTE -IDENTICAL stdout and identical exit code (openconnector, procest, hermiq — each `✓ … all rules pass`, rc=0). Nothing that passed before changes. ## What gate-24 now reports — a REAL finding, not suppressed ✗ [R2 id-correlation] id "decidesk-decisions" (src/integrations/registerDecisionsLeaf.js) has NO matching server-side face in lib/** — orphan registration: it mounts on window.OCA.OpenRegister.integrations but is invisible to the openregister.integrations.leaves capability. Assertions run per rule: R1:1 R2:1 R3:0 R4:0 R5:0 R6:0 — the gate is correlating real subject matter now instead of nothing. The finding is genuine: decidesk ships no `new LeafDescriptor(` and no IntegrationProvider in lib/, so the decisions leaf has a JS face and no server face. Whether to add the server face or to treat this leaf as deliberately client-only is a product decision (ADR-066 decisions 4/7), so it is ESCALATED, not patched and not exempted here. So `Hydra Gates` stays red — but on a substantive, named, actionable finding rather than on a gate that never ran. `require-full-coverage` was NOT switched off and no exemption was added: "a gate that did not run" was the gate telling the truth about itself, and the fix is to let it speak. Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
PHPCS on development reported 63 errors and 112 warnings. The shared CI gate maps phpcs exit 1 to success, so these have been shipping silently. This brings errors to 0 (raw phpcs exit 1 -> 0); warnings are left alone. By category: - 56 PEAR.Commenting.FunctionComment.MissingParamTag — constructors that gained a promoted dependency (mostly the ADR-084 ObjectServiceInterface, plus FileService / TaskService / RegisterMapper / SchemaMapper) without the matching @PARAM line. Types taken from the actual signature, tags added in signature order. - 4 PEAR.Commenting.FunctionComment.WrongStyle — a `//` prose block sat between the real docblock and the PHP attribute, so phpcs treated it as the function comment. The prose is folded into the docblock rather than deleted. Side effect: the real docblock's @SPEC tags are visible to the spec-tag sniff again, so 4 MissingMethodSpec warnings also clear. - 2 Generic.Files.LineLength.MaxExceeded — a long @return continuation line reflowed, and ParticipationBudgetController::submitProposal() wrapped one-parameter-per-line in the house style. - 1 Squiz.Commenting.InlineComment.NotCapital — reworded to start with a capital, meaning unchanged. No phpcbf/--fix was used; every edit is by hand. Only one non-comment line changed (the submitProposal signature reflow, behaviour identical). Baseline vs after on this clone (PHP 8.3 in Docker): lint 0/0, phpmd 0/0, psalm 0/0, phpstan "No errors"/"No errors", phpunit 967 tests / 87 errors / 1 failure / 33 skipped both before and after — those failures are pre-existing and local-only (the OpenRegister app's classes are absent from this checkout). Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
decidesk registered its `decidesk-decisions` leaf on the CLIENT only. Under ADR-066 decision 1 the JS `registerIntegration()` path is the render-surface HALF of the leaf contract, bound to a server descriptor by shared id, and the ADR's Consequences name the job the other half does: registered descriptors surface through OpenRegister's OCS capabilities so an admin UI or manifest app can enumerate leaves without loading any app's JS bundle. Without that half the leaf renders but is invisible to every server-side consumer — an orphan registration under ADR-066 decision 4 (gate-24 R2). Adds RegisterDecisionsLeafListener, modelled on hermiq's RegisterAgentLeafListener (the fleet's reference shape for the same situation): one `render-surface` kind, a null IntegrationProvider (the leaf reads and appends through OpenRegister's own object API from the browser, ADR-022, so decidesk holds no app-local store behind it), `renderMode: mount` matching the JS half's mount/unmount DOM hand-off, and every metadata field equal to the JS half's declaration. The subscription lives in a new IntegrationLeafRegistrar rather than on PlatformIntegrationRegistrar: that class was at a PHPMD CouplingBetweenObjects of 12 against a threshold of 13, and the leaf's two class references would have taken it to 14. Extraction is the move this codebase already makes at that boundary. Registered unconditionally from register() — `::class` is a compile-time string and registerEventListener() stores strings, so nothing autoloads an OpenRegister class, and a class_exists() guard there would resolve differently purely by app load order.
The JS half declared no `surfaces` key at all. ADR-066 decision 4 requires the two halves to correspond, and a half that declares a value by OMISSION gives a cross-layer check nothing to compare — which is exactly how hermiq's two halves drifted apart unnoticed while both compilers stayed quiet. All four members of LeafDescriptor::VALID_SURFACES are declared because the leaf really does render on all four: componentForSurface() roots CnDecisionsWidget on detail-page / app-dashboard / user-dashboard and CnDecisionsTab everywhere else. The key is inert on the client today — the registry routes tab-vs-widget through the `surface` mount prop and never reads this list (checked against @conduction/nextcloud-vue 2.3.0's useIntegrationRegistry) — so this changes no rendering. It is a declaration, and it is what the parity assertions read.
…pear Nine tests across two files, each shown able to fail before it was shown to pass. RegisterDecisionsLeafListenerTest asserts the leaf is discoverable server-side: exactly one contributed leaf, the render-surface kind and ONLY that kind, the mount render mode, a null provider, and the exact capability row LeafRegistry::describeForCapabilities() publishes. Two of its tests exist because of what the red control showed. With the listener class DELETED, seven of eight tests errored and 'the listener is subscribed to the collect event' still PASSED — `::class` is a compile-time string and registerEventListener() only stores strings, so a subscription to a missing class is indistinguishable from a working one until the event is dispatched. It now asserts the named class exists and implements IEventListener. And because a registrar nobody calls registers as much as no registrar at all, a second test reads Application::register() through reflection to prove the composition root reaches it — with a positive control on the same reader, so a failure means 'not wired', never 'read nothing'. DecisionsLeafParityTest compares the two DECLARATIONS directly, reading the JS source, because there is no runtime in this process where both exist. It covers two fields gate-24's static reader silently skips on this repo: `requiredApp` (written Application::APP_ID) and `label` (written $this->l10n->t(...)) are both unresolvable to it, and it treats an unresolvable value as 'not compared, never a failure'. Measured: with the JS `requiredApp` mutated to 'decidesk-typo', gate-24 exits 0 and reports every rule passing while this test fails. The three OpenRegister stubs mirror the real classes' FULL public surface, checked against openregister development when written. They need no require_once branch — their paths under tests/Stubs/ mirror their namespaces, so the PSR-4 root the bootstrap already registers resolves them, and adding one would recreate the dead-guard shape #399 removed.
The behaviour this PR adds is shipped and observable, so it gets a written requirement rather than a `@spec exclude`. It records what each half declares, why the server half exists (capability enumeration without loading the bundle), that the leaf declares render-surface ONLY and contributes a null provider, and that raising a decision from another app stays the ADR-041 DecisionRequestedEvent path rather than the leaf seam. Carries an `@e2e exclude` with its reason: one of the two declarations is a PHP LeafDescriptor no browser ever sees, so there is no rendered state in which Playwright could observe the server half being absent — which is precisely why the halves were allowed to drift.
…gister objects BEHAVIOUR CHANGE, stated up front: a user who is neither an object's owner, nor a Nextcloud admin, nor a member of `decidesk-administrators` can no longer UPDATE or DELETE another user's decidesk object. Reads, listings and creates are unchanged. Until now they could. Every decidesk object is reachable at /apps/openregister/api/objects/decidesk/<schema> — the API the frontend uses directly under ADR-022 — and no decidesk controller guard sits in front of it. What decides who may write there is the `authorization` block on the schema, or failing that on the register row. This tree had neither. OpenRegister's PermissionHandler::hasGroupPermission() tests `empty($authorization)`, and PHP's empty() is true for null and [] alike, so an ABSENT block takes the same default-OPEN branch as an empty one. `enforce_default_closed` reads IAppConfig with default:false, so on a stock instance its deny arm never fires — and even switched on it covers only writes, never reads. Measured across all 25 register files on this tree: 93 schemas, 24 carrying a block (every one of them read-only), and the register row carrying none. So 69 schemas — Decision, VotingRound, Vote, Participant and EngagementRecord among them — granted create, update AND delete to any logged-in account. The same shape as docudesk#631, where a plain user overwrote another user's template. The fix sits on the REGISTER row because of the cascade: resolveAuthorization() uses a schema's own block when it has one and falls back to the register's only when it does not. So one declaration reaches exactly the 69 unprotected schemas and changes nothing for the 24 that already declare their own — their public-read publication rules are untouched. Every canonical action is written out deliberately. Once a block is non-empty, OpenRegister DENIES any action it omits, so a half-written block breaks the app rather than securing it: read/list/create stay `authenticated`, and only update/delete are narrowed. The owner bypass is unconditional and SQL-side and precedes every rule, so an author keeps full control of their own object. Both version bumps are load-bearing and neither is cosmetic. ImportHandler's REGISTER path skips outright when the incoming version is <= the stored one and, unlike the schema path, has no content-differs fallback — so the register goes 0.7.0 -> 0.8.0. And InitializeSettings is a <post-migration> repair step, which runs only on `occ upgrade`, which is a no-op when the app version has not moved — so appinfo goes 0.4.6 -> 0.4.7. Either bump alone leaves a correct block sitting on disk on every existing instance.
Seven tests, shown red on development's own state before they were shown green: 5 of 7 fail there (the block is absent, and both versions are behind), and the two that stay green are the ones asserting the UNCHANGED 24 schema-level blocks — which is what they should do. They deliberately do NOT re-implement OpenRegister's evaluator. An instrument built from the same source as the bug reports zero, and zero reads as a pass. What this repository owns is the DECLARATION the evaluator reads, so that is what is pinned: the block exists and names every canonical action with a non-empty rule list; read/list/create still grant `authenticated` (if this goes red the fix has become an outage); update/delete grant neither `authenticated` nor `public`; no write action anywhere names `public`, which is the one thing that would re-open the anonymous writes openregister#1955 closed. Two of the seven guard the deploy path rather than the policy, because a correct block that never reaches an instance is a fix that reports success and changes nothing: the register/config version must be past 0.7.0 and the app version past 0.4.6, each asserted against the last release that shipped WITHOUT the block so the assertion survives future bumps. The app-version test carries a positive control on its own reader (info.xml's <id> must read `decidesk`), so a failure means 'not bumped' and never 'parsed an empty document'. The schema-block test carries the same kind of control: it asserts the COUNT is 24, so it cannot pass vacuously if the schemas are renamed, moved, or stop being found. REQ-RBAC-006 records the requirement, with an @e2e exclude that names the real reason: the owner bypass is unconditional and SQL-side, so a browser test driven by one seeded (owning, usually admin) session cannot observe this denial at all and would report success over the exact hole. The per-user behaviour needs a two-account probe against a live instance, and that is recorded as verification owed rather than claimed.
…hem, and CI proved it Two defects in the first version of this PR, both found by CI and neither findable locally. Recording what happened, because the first one is the exact trap this change was supposed to avoid and I walked into it anyway. 1. THE BLOCK CLOSED ANONYMOUS READS. All six PHPUnit legs failed with `NotAuthorizedException: User 'Anonymous' does not have permission to 'read' objects in schema 'Meeting'`. Before any block existed, hasGroupPermission() took its default-OPEN branch for EVERY principal — the anonymous one included — so a block naming only `authenticated` on `read` does not preserve the status quo, it CLOSES anonymous reads. Omission is the deny, and that is as true for the principal as it is for the action. PHPUnit's CLI has no session, so those integration tests exercise exactly the path a #[PublicPage] citizen-participation surface takes. Left as it was, this PR would have 403'd every public consultation and budget-proposal read. `read` and `list` now name `public` as well, which is precisely the pre-change behaviour. `public` appears on NO write action, so openregister #1955's anonymous fail-closed rule keeps denying anonymous create/update/delete exactly as before, and the write hole stays closed. Closing anonymous reads may well be worth doing — it is a far larger policy change than this PR, and it does not belong smuggled inside it. Guarded by a new test, shown red on the omission. 2. MY OWN TEST FAILED ON AN EXTENSION CI DOES NOT HAVE. `simplexml_load_file()` worked locally and returned FALSE on every CI leg, failing the suite on 'appinfo/info.xml must be readable XML'. The assertion is about one scalar in a file this repository owns; it now reads the file as text and preg_matches <version>, with the positive control moved to a string match on <id>.⚠️ Local green could not have caught either one. The unit environment stubs OCA\OpenRegister\*, so the integration tests that hit the real permission path are precisely the ones that cannot run there — and the XML failure needed CI's own PHP image. Both are the 'local green means nothing for this bug class' shape.
Composer had no package-ecosystem entry at all, so composer dependencies got no release-age cooldown whatsoever, unlike npm which has had one for a while. Adds cooldown.default-days: 2 with a conduction/* exclude, matching the fleet-wide floor gate-93 (composer-cooldown-config) enforces. See ConductionNL/hydra openspec/changes/composer-dependency-cooldown and ADR-093 (proposed, ConductionNL/hydra#591). Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
Picks up #522's composer cooldown in .github/dependabot.yml. The gate package moved from 742f370e to 0b189e30 mid-review and added gate-93 composer-cooldown-config, which this branch failed purely by predating the fix that development already carries.
Picks up #522's composer cooldown in .github/dependabot.yml. The gate package moved from 742f370e to 0b189e30 mid-review and added gate-93 composer-cooldown-config; this branch's green was measured on 742f370e, before that gate existed, and would fail it purely by predating the fix development already carries.
…face fix(adr-066): ship the decidesk-decisions leaf's server-side face (closes gate-24)
fix(security): close the default-open write hole on decidesk's OpenRegister objects
This was referenced Aug 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated PR to sync development changes to beta for beta release.
Merging this PR will trigger the beta release workflow.