Skip to content

feat(governance): make an Awb 7:13 objection advisory committee expressible - #874

Merged
rubenvdlinde merged 8 commits into
developmentfrom
feat/advisory-committee-and-approval-routes
Aug 24, 2026
Merged

feat(governance): make an Awb 7:13 objection advisory committee expressible#874
rubenvdlinde merged 8 commits into
developmentfrom
feat/advisory-committee-and-approval-routes

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Makes a Dutch bezwaaradviescommissie expressible as a decidiq GovernanceBody, so dossiq can stop carrying a parallel committee schema. Enabler for a follow-up migration in dossiq — nothing in dossiq changes here.

The measured gap

Checked against the live schemas on both sides, not assumed:

dossiq field decidiq before now
active GovernanceBody.active (bool, default true)
quorum (int) quorumRule (string, a method) GovernanceBody.quorum (int, min 2) — both kept
jurisdiction GovernanceBody.jurisdiction
type BAC/VKK/VTH bodyType: advisory-body (lossy) GovernanceBody.statutoryBasis (free text)
members[].external Membership.independenceStatus (different axis) Membership.external (bool)

active is worth singling out: it is the one field dossiq's live code actually reads, throwing "Committee is archived and cannot accept new bezwaaren". A migration that dropped it would silently route objections to disbanded committees.

Every field is optional — required is unchanged on both schemas, so no stored object becomes invalid. A test asserts that.

The write seam

decidiq's cross-app API was read-only, so no app had a supported way to place a body here. governance-bodies becomes the one writable resource; tests prove meetings, decisions, votes, persons, minutes and motions all stay refused.

⚠️ Authorization delegates to OpenRegister's RBAC, because the obvious alternative isn't real: ApiController::SCOPE_MAP reads as an access control and is referenced by no method — no request has ever been checked against it. It's now labelled decorative so nobody adds an entry expecting it to gate something.

Two things this cost, both worth reading

A duplicate route name takes down the whole app. Registering api#write for POST and PUT collides on the route identifier — and it doesn't fail at the duplicate, it throws while the table is built. Measured: the entire /api/v1 surface answered 500, including endpoints this change never touches, while the app still reported itself enabled. Split into api#create / api#update, and re-verified that POST /api/v1/decisions still reaches the decision-hub handler.

PUT replaces, it does not patch. A partial body is refused naming the missing required properties. Safe direction, now documented rather than discovered.

Verification

  • suite 1197 green · phpcs 0 · phpmd 0 · phpstan [OK]
  • live-verified on a running instance: POST creates (201) with all four fields round-tripping, PUT updates (200), POST to a read-only resource → 404, partial PUT → 400 naming the missing fields
  • 18 new tests

🤖 Generated with Claude Code

…ssible

A bezwaaradviescommissie is a governance body in everything but where it lives.
decidiq already owns governance bodies — 17 seeded, `Membership` and `Post` for
the roster, `bodyType: advisory-body` already in the enum — so another app
keeping a parallel committee schema is duplication, not architecture.

Four fields closed the measured gap. Each stands for a specific thing the
existing model could not say, checked against the live schemas rather than
assumed:

  GovernanceBody.active         — whether the body may take new work. THE one
                                  field the consuming app's live code reads and
                                  throws on ("Committee is archived and cannot
                                  accept new bezwaaren"). Nothing here carried
                                  it; `x-openregister.active` is SCHEMA-level and
                                  says nothing about an individual body.
  GovernanceBody.quorum         — how MANY members, as a number. `quorumRule` is
                                  a calculation method ("majority") and cannot
                                  express a statutory minimum. Both stay; each
                                  description now names the other.
  GovernanceBody.jurisdiction   — territorial/subject-matter competence.
  GovernanceBody.statutoryBasis — the instrument the body sits under.
  Membership.external           — sits from OUTSIDE the administrative organ,
                                  which Awb 7:13(2) requires of the chair.
                                  `independenceStatus` cannot carry it: that is
                                  corporate-governance independence, a different
                                  question with a different answer set. Each
                                  description now says what it is NOT, and a test
                                  asserts they do — two fields that read alike and
                                  mean different things is how the wrong one gets
                                  used.

`quorum` has NO default, and a test asserts the ABSENCE. A default of 0 would
read as "no members are required for a valid sitting" — a confident wrong answer
where "not specified" is the truth.

`statutoryBasis` is free text, not a BAC/VKK/VTH enum. A closed vocabulary drawn
from one country's law cannot express the instruments behind bodies in another,
and a caller forced to choose from a list that does not fit will choose a wrong
value — which is worse, because it then reads as data.

THE WRITE SEAM was the part no schema field fixes. decidiq's cross-app API was
read-only, so no other app had a supported way to place a body here, and the
alternative is reaching into this register directly — which ADR-022 and ADR-066
forbid. `governance-bodies` becomes the ONE writable resource, allowlisted in
`RESOURCE_WRITABLE`, with tests proving meetings, decisions, votes, persons,
minutes and motions all stay refused. The wildcard route is what makes that
assertion necessary.

AUTHORIZATION IS DELEGATED TO OPENREGISTER'S RBAC, and the obvious alternative
was not available: `SCOPE_MAP` in this controller reads as an access control and
enforces NOTHING — it is declared and referenced by no method, so no request has
ever been checked against it. Specifying a `governance-bodies:write` scope would
have named a gate that does not exist. It is now labelled as decorative so the
next reader does not add an entry believing it will gate anything.

TWO ROUTES, TWO NAMES, and that cost a real failure to learn. Registering
`api#write` for both POST and PUT collides on the route IDENTIFIER, and the
collision does not surface as a duplicate-route warning — it throws while the
table is built and takes down EVERY route in the app. Measured on the running
instance: the whole /api/v1 surface answered 500, including endpoints this change
never touches, while the app still reported itself enabled. Verified fixed by
re-testing that `POST /api/v1/decisions` still reaches the decision-hub handler
rather than the generic writer.

LIVE-VERIFIED end to end, not only in the suite: POST creates a committee (201)
with all four fields round-tripping; PUT updates it (200); POST to a read-only
resource answers 404 "Unknown resource"; and PUT REPLACES rather than patches —
a partial body is refused naming the missing required properties, which is the
safe direction but is now documented rather than discovered.

Suite 1197 green, phpcs 0, phpmd 0, phpstan [OK].

Scope note: migrating another app's committees onto these bodies is that app's
change, not this one. This makes the target exist.
…advances it

`DecisionStage` modelled the stages of a decision's route and NOTHING in this
app ever wrote one. Measured: zero create/save/update of `decision-stage` across
lib/, six seeded rows, two readers (EIDASSignatureService,
DecisionIntegrationService), and a route tab whose own header declares "Posture:
read-only". A schema without an engine is a description of a capability, not the
capability.

This adds the three things missing around it:

  ApprovalRoute        the reusable TEMPLATE a route is instantiated from.
                       `DecisionStage` is bound to a Decision, so there was
                       nowhere to say "every collegeadvies travels these four
                       steps". ProcessTemplate is not that: it is a state
                       machine (states, transitions, guards), which answers what
                       states a decision may be IN, not who signs and in what
                       order.
  ApprovalAction       the APPEND-ONLY trail. A stage is one mutable row; a
                       route needs many actions against one step — an advice, a
                       return, a re-submission, a delegate acting under mandate.
                       The stages say where the route IS; the actions say what
                       happened, including the attempts a return undid.
  ApprovalRouteService the engine, and the point of the change.

DecisionStage grows additively: `stageType` += `endorsement` (parafering: a
sign-off that is not itself the decision), `outcome` += approved/endorsed/
returned/skipped, and `mandatory`. `required` is unchanged, so no stored stage
becomes invalid.

`returned` behaves unlike every other outcome and that is why it had to be new:
it RE-OPENS an earlier stage rather than ending this one. `rejected` and
`deferred` both end a stage and neither reopens one, so no existing value could
express "send it back to step 2".

FAIL-CLOSED, deliberately and with tests on the refusals rather than only the
happy path. An action by an actor the active stage does not name is refused; a
skip of a mandatory stage is refused; a return pointing forwards is refused; an
action on a finished route is refused. Each refusal test asserts that NOTHING
changed — no action row, no stage move. An engine that records an action and
does not move is the exact failure this replaces: the consuming app had one
route API whose every call returned 400 and one approval bridge that
short-circuited on a property nothing ever set, and both reported success.

Two details that would each have produced a quietly broken engine:

  A `role`-typed step does NOT write its role token into `assignedPerson`. That
  field means "this person", and a role name there would make every actor check
  compare a uid against a role and refuse everyone. Asserted.

  A return CLEARS the outcome of the stages it resets. A stage that is pending
  again while still showing an outcome reads as decided to every consumer that
  looks at the outcome rather than the status.

`mandatory` is COPIED onto the stage at instantiation rather than read from the
route at decision time — otherwise editing a template would change the rules
under a subject already travelling it. Same reason the steps themselves are
copied.

The controller takes the ACTOR FROM THE SESSION, never from the body. Reading it
from the request would let any caller sign off as anyone, which is the one thing
a sign-off route exists to prevent.

Persistence is split into ApprovalRouteStore so the engine holds only the rules.
That was prompted by phpmd (class complexity 58 vs a threshold of 55) and is the
better shape regardless.

l10n: the 15 schema strings this change introduces are translated in en/nl and
BUILT into l10n/*.js — the artifact the browser actually loads, which the
source-reading check cannot see.

⚠️ `check:schema-l10n` is RED ON DEVELOPMENT ITSELF: 43 strings above the
hardcoded baseline of 1675, before this branch existed. This change reduces it
to 29 by translating its own strings properly. The rest is being fixed on
`test/schema-l10n-ratchet`, which replaces the hardcoded number with a baseline
file plus an --update mode; duplicating that here would conflict with it.

Suite 1212 green, phpcs 0, phpmd 0, phpstan [OK].

The UI write affordances on DecisionRouteTab are deliberately deferred — the tab
is read-only today, and giving it verbs is a separate change now that the engine
it would drive exists and its shape is settled.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/decidiq @ eae3411

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-nav-ceiling
test-l10n
format
check-l10n-js
check-schema-l10n
composer ✅ 104/104
npm ✅ 555/555
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman
Playwright
Hydra gates

Quality workflow — 2026-08-24 09:40 UTC

Download the full PDF report from the workflow artifacts.

…ger uses

`RegulatorExportRenderer` probes for a document-app PDF service and falls back
when none is found. Both candidates named `OCA\Docudesk\Service\*`; the document
app renamed to `OCA\Filinq` with no compatibility alias, so every candidate
missed and the renderer silently took its fallback path.

The pattern here was already right — a candidate LIST rather than a pinned class
— which is why this was a quiet degradation rather than a crash. It just listed
one app's old name twice. A list is only resilient if it spans the rename.

Measured on a running instance:
  OCA\Docudesk\Service\PdfRenderService   MISSING
  OCA\Docudesk\Service\PdfService         MISSING
  OCA\Filinq\Service\PdfService           EXISTS
  OCA\Filinq\Service\PdfConversionService EXISTS

Note `PdfRenderService` has no Filinq counterpart at all — the class list moved
as well as the namespace, so a mechanical namespace swap would have produced two
candidates of which one still could not resolve.

The Docudesk entries stay until no supported install ships them; an unresolvable
candidate costs one class_exists() call.

Found by a fleet sweep after the same defect was confirmed in dossiq;
openregister (→ Keepiq) and softwarecatalog (→ Decidiq) carried it too.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/decidiq @ 2966cab

Check PHP Vue Security License Tests
lint ⏭️
phpcs ⏭️
phpmd ⏭️
psalm ⏭️
phpstan
phpmetrics ⏭️
eslint ⏭️
stylelint ⏭️
build ⏭️
composer ⏭️ ⏭️
npm ⏭️ ⏭️
app:check-code ⏭️
info.xml ⏭️
REUSE ⏭️
PHPUnit
Newman
Playwright
Hydra gates

Quality workflow — 2026-08-24 09:57 UTC

Download the full PDF report from the workflow artifacts.

… baseline

`check:schema-l10n` was failing this PR, and it was failing on `development`
too: 1718 uncovered against a baseline of 1675, i.e. 43 over, before this branch
existed. A schema string with no catalogue key renders in English inside an
otherwise Dutch form — so this is not a bookkeeping gate, it is 43 labels a
Dutch user reads in the wrong language.

The previous commit translated the 15 strings this branch INTRODUCES, taking it
from 43 to 29. That made the branch better than its base and still red. This
closes the rest: 56 schema property titles — the strings that render as form
labels, so the highest value per string — now at 1648, under the 1675 baseline.

Safe to translate BECAUSE THEY ARE TITLES. A property title is a display string;
translating a stored value is what breaks colour maps and filters. Nothing here
touches an enum value, a slug, or anything a filter compares against.

Some were already Dutch in the schema ("Aanlever-deadline",
"Achterbanraadpleging", "Afdoening evidence"). Those get an identity entry in
en.json and a cleaned Dutch form in nl.json rather than being left uncovered —
uncovered is uncovered whatever language the source happens to be in.

BUILT, not just catalogued: `npm run l10n:build` regenerates l10n/*.js, which is
the artefact the browser actually loads. A check that reads the JSON cannot see
whether the .js was rebuilt, and shipping the one without the other is how 8,137
translations once reached nobody.

Verified: check:schema-l10n exits 0, test:l10n OK, check:l10n-js up to date.

⚠️ I misread this earlier and said it was another session's branch to fix. That
branch (#859) MERGED on 2026-08-23 — it introduced the baseline FILE mechanism
this now satisfies. The remaining gap was ordinary untranslated debt, not a
missing mechanism, and waiting for it would have waited forever.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/decidiq @ 2f02d86

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-nav-ceiling
test-l10n
format
check-l10n-js
check-schema-l10n
composer ✅ 104/104
npm ✅ 555/555
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman
Playwright
Hydra gates

Quality workflow — 2026-08-24 10:15 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/decidiq @ 6f9bb63

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-nav-ceiling
test-l10n
format
check-l10n-js
check-schema-l10n
composer ✅ 104/104
npm ✅ 555/555
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman
Playwright
Hydra gates

Quality workflow — 2026-08-24 11:14 UTC

Download the full PDF report from the workflow artifacts.

rubenvdlinde and others added 3 commits August 24, 2026 18:24
…red icon

Four Hydra gates failed on this branch. All four were mine and all four were
right; one of them was a genuine security hole.

GATE-7 (no-admin-idor) — `ApprovalRouteController::instantiate` carried
`#[NoAdminRequired]` and no authorisation. It had an authentication preamble,
which the gate explicitly does not count and should not: "is anyone logged in"
was already settled by the attribute. The actual effect was that ANY signed-in
user could start a sign-off route against ANY subject, writing DecisionStage rows
onto somebody else's object.

Fixed with `assertSubjectAccessible()`, which asks the question that matters —
can this caller reach this subject — and delegates the answer to OpenRegister,
reading as the acting user so OR's register RBAC and multitenancy decide. A user
who cannot reach the subject gets nothing back, and nothing back is a refusal.
`subjectSchema` becomes required, because the check cannot be performed without
it. The gate no longer lists this method.

GATE-66 (openregister-dependency-shape) — `ApprovalRouteStore` fetched
ObjectService from the container by string. ADR-083 rule 1: a dependency
resolved that way is declared nowhere a reader or a gate can see it. Now injected
as a typed `ObjectServiceInterface`, which `Application::register()` already
aliases — an ALIAS, so it still resolves lazily and an instance without
OpenRegister still boots.

That change made a second thing true: `findAll()` is typed `: array`, so the
`is_array()` guard below it became unreachable. Removed rather than kept —
phpstan proved it dead, and it was caution against the untyped lookup that no
longer exists.

GATE-60 (icon-vocabulary) — `GestureTapButton` is not in the canonical
vocabulary, and ADR-077 rule 3 means it renders with NO icon at all, not a
fallback. Swapped for `ClipboardCheckOutline`, which is registered and reads
correctly for a recorded sign-off.

GATE-16 (spec-coverage) — `ApprovalRouteStore::save()` and `::findAll()` are new
public methods without `@spec`. Added.

TESTS follow the real contract now, and that mattered twice.
`ObjectServiceInterface::saveObject()`'s SECOND parameter is `?array $extend`,
not the register, and it returns an ENTITY rather than an array. A callback
shaped like the call site (which uses named arguments and skips `$extend`)
received the wrong values in the wrong order. The stateful fake is now reached
through a typed mock that mirrors the real signature — the type satisfies the
constructor and the state survives, which is what lets these tests assert the
route ADVANCING rather than merely that a call was made.

Suite 1212 green, phpcs 0, phpmd 0, phpstan [OK], psalm clean, l10n ratchet PASS.

Still failing locally and NOT mine: gate-7 lists 3 VotingController methods
(cast/proxy/revokeProxy) — pre-existing, and CI counts diff-scoped so they did
not block this PR. They are real findings and are being taken up separately.
…oval-routes' into feat/advisory-committee-and-approval-routes
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/decidiq @ 10ca597

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-nav-ceiling
test-l10n
format
check-l10n-js
check-schema-l10n
composer ✅ 104/104
npm ✅ 555/555
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman
Playwright
Hydra gates

Quality workflow — 2026-08-24 17:06 UTC

Download the full PDF report from the workflow artifacts.

…nothing

Found by running `occ upgrade` on a live instance, not by any test.

`MigrateLegacyTemplatesToDecisionTemplate` calls `saveObject()` with no system
identity. A migration executes during an upgrade, where there is no session — so
OpenRegister sees the actor as 'Anonymous' and refuses `create` on
DecisionTemplate. Every one of the 14 legacy templates failed:

  Repair warning: Failed to migrate process-template 20ac9a95-…:
    User 'Anonymous' does not have permission to 'create' objects in schema
    'DecisionTemplate'                                        (× 8 process)
  Repair warning: Failed to migrate vve-decision-template e2e93cb6-…:
    …same…                                                    (× 6 vve)

AND THE UPGRADE STILL SAID "Update successful". Each failure was reported with
`$output->warning()`, which does not fail an upgrade, and the step's own summary
line then read "0 migrated, 14 skipped" — a sentence that is true, undramatic,
and scrolls past. The legacy templates are still there and the unified ones were
never written, on every instance that has run this.

Wrapped in `runAsSystem()`. The wrap is around the WHOLE traversal rather than
each save, so the index build happens inside the same identity scope too;
that meant lifting the body into `migrateAll()`, which is the only reason
`run()` changed shape.

THE TEST FAKE NEEDED THE METHOD, and that is the interesting half. The suite was
green throughout — 16 tests asserting the mapping in detail — because its
anonymous ObjectService fake simply had no `runAsSystem()`, and nothing required
it to. A fake that does not model the contract cannot fail when production
violates it: the wrapper could be deleted tomorrow and these tests would still
pass while every real upgrade migrated nothing. The fake now implements it, so
removing the wrap breaks the suite.

16/16 green, suite 1219 green, phpcs 0, phpstan [OK].

Pre-existing debt, found while getting this branch's own CI green. Not related to
approval routes; fixed here because an upgrade that silently migrates nothing is
worse than one that fails loudly.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/decidiq @ 27ba41f

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-nav-ceiling
test-l10n
format
check-l10n-js
check-schema-l10n
composer ✅ 104/104
npm ✅ 555/555
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman
Playwright
Hydra gates

Quality workflow — 2026-08-24 18:05 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/decidiq @ 4938fce

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-nav-ceiling
test-l10n
format
check-l10n-js
check-schema-l10n
composer ✅ 104/104
npm ✅ 555/555
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman
Playwright
Hydra gates

Quality workflow — 2026-08-24 19:17 UTC

Download the full PDF report from the workflow artifacts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants