fix(adr-066): ship the decidesk-decisions leaf's server-side face (closes gate-24) - #520
Merged
Merged
Conversation
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.
rubenvdlinde
requested review from
Rem-Dam,
SudoThijn,
WilcoLouwerse,
bbrands02,
remko48 and
rjzondervan
as code owners
August 17, 2026 07:37
Contributor
Quality Report — ConductionNL/decidesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| composer | ✅ | ✅ 104/104 | |||
| npm | ✅ | ✅ 532/532 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-17 08:15 UTC
Download the full PDF report from the workflow artifacts.
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.
Contributor
Quality Report — ConductionNL/decidesk @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| composer | ✅ | ✅ 104/104 | |||
| npm | ✅ | ✅ 532/532 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-17 11:42 UTC
Download the full PDF report from the workflow artifacts.
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
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.
What
Ships the server-side half of decidesk's
decidesk-decisionsintegration leaf, and pins it with tests.This closes
[gate-24] integration-parity, decidesk's last failing gate — but it is not why it was done. The finding was real: the leaf was registered on the client only, which under ADR-066 is half a contract.Why ADR-066 asks for this
ADR-066 decision 1 makes the JS
registerIntegration()path the render-surface half of the leaf contract, "bound to the server descriptor by sharedid". Its Consequences → Cross-app / governance section names the job the other half does:Decision 4 then requires the two halves to correspond under one id, and decision 7 requires the server descriptor's
renderModeto equal the JS registration's.decidesk had one JS registration (
decidesk-decisions,renderMode: 'mount', a completemount/unmountpair) and zero server faces —new LeafDescriptor(,IntegrationProviderandRegisterLeafProvidersEventall measured empty inlib/, and openregister ships no face for that id either. That is an orphan registration.Cutting the other way, stated plainly: nothing reads the capability today. The only references to
openregister.integrations.leavesin the whole fleet checkout are the writer (openregister/lib/Capabilities/IntegrationsCapability.php:114) andLeafRegistry::describeForCapabilities(). Rendering is driven entirely client-side, so the leaf renders today and this PR changes no rendering. What it adds is server-side discoverability — a genuine ADR-066 Decision 1/4 gap with no runtime symptom yet.The shape
Modelled on hermiq's
lib/Listener/RegisterAgentLeafListener.php, the fleet's reference for exactly this situation — same collect-event, same guarded-throw contract, adapted to decidesk's leaf:kinds: ['render-surface']and only that kind. Asserted, not left unstated.IntegrationProvider. The tab and widget read (and append) decisions through OpenRegister's own object API in the browser (ADR-022), so decidesk holds no app-local store behind this leaf.renderMode: mount, matching the JS half's DOM hand-off (openregister#2127).DecisionRequestedEventpath (REQ-DCDH-002), never the leaf seam — ADR-066 decision 2.Two things worth a reviewer's eye
1. The subscription is in a NEW registrar.
PlatformIntegrationRegistrarsat at a PHPMDCouplingBetweenObjectsof 12 against a threshold of 13; the leaf's two class references took it to 14 and turned a correct registration into a redphpmdjob. Extraction is the move this codebase already makes at that boundary — it is why the four existing registrars were split out ofApplicationin the first place, and those files' own docblocks say so.Applicationgoes 11 → 12.2.
surfacesis now declared on BOTH halves. The JS half declared nosurfaceskey at all. 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 while both compilers stayed quiet. All fourVALID_SURFACESmembers are declared because the leaf really does render on all four (componentForSurface()roots the widget on the three dashboard/detail surfaces and the tab everywhere else). The key is inert on the client —@conduction/nextcloud-vue2.3.0'suseIntegrationRegistrynever reads it; the registry routes tab-vs-widget through thesurfacemount prop — so this changes no rendering either.gate-24, before and after
Both sides measured with the same gate package
742f370e152b296acdca3289230a0119d8bb23b8(=ConductionNL/.github@main), the samenode_modules, the samevendor, the same runner invocation. The base is a sibling worktree ata9de4096, not a remembered number.a9de40967e3f2293run-hydra-gates.shexitFAIL — 1 parity violation(s)PASS1 GATE(S) FAILEDALL 62 APPLICABLE GATES GREEN — and all 62 of them ranThe base line reproduces CI run
95288055932exactly ([gate-24] integration-parity: FAIL — 1 parity violation(s),1 GATE(S) FAILED). The violation:Re-run with a delta base (
--base a9de4096, which activates the diff-scoped gates CI also runs):ALL 64 APPLICABLE GATES GREEN — and all 64 of them ran, exit 0 — including[gate-16] spec-coverage: PASS,[gate-46] spec-anchor-existence: PASSand[gate-47] security-change-has-tests: PASS.The gate was not weakened, exempted or retargeted, and no parity script was shaped around decidesk's configuration.
scripts/check-integration-parity.jsis byte-identical todevelopment—git difftouches it not at all.The tests, and their red control
Nine tests. Every one was shown able to fail before it was shown to pass.
Removing
lib/Listener/RegisterDecisionsLeafListener.phpentirely (tests kept):🔴 That control changed the test. On the first run, seven tests errored and "the listener is subscribed to the collect event" still PASSED over a listener class that did not exist.
::classis resolved by the compiler to a plain string andregisterEventListener()only stores strings, so a subscription to a missing class is indistinguishable from a working one right up until OpenRegister dispatches the event. The test now asserts the named class exists and implementsIEventListener— and it is red in the control above.Three further controls, each reverted:
ICON→ScaleBalanceIntegrationLeafRegistrarcall removed fromApplication::register()requiredApp→decidesk-typosurfaceskey removedThe last two rows are the point of
DecisionsLeafParityTest. gate-24's static reader cannot resolveApplication::APP_IDor$this->l10n->t(self::LABEL_SOURCE), and it treats an unresolvable value as "not compared, never a failure" — sorequiredAppandlabelare unguarded by the gate. Measured: withrequiredAppmutated, gate-24 exits 0 and prints "all rules pass". The test does not.(Root cause, reported not fixed — see below: the checker's
/class\s+(\w+)/regex matches the word "class" insideApplication.php's docblock sentence "Main application class for the Decidesk…", so it filesAPP_IDunderfor::APP_ID.)A registrar with no caller registers as much as no registrar at all, so
testTheCompositionRootRunsTheLeafRegistrarreadsApplication::register()through reflection — with a positive control on the same reader (new PlatformIntegrationRegistrar()must be found), so a failure means "not wired", never "read nothing".Everything else, measured
Run in a
php:8.3-clicontainer (the host has no PHP 8.3):phpunit -c phpunit.xml(full suite)Tests: 982, Assertions: 3766, Errors: 87, Failures: 1, Skipped: 33Tests: 973, Errors: 87, Failures: 1, Skipped: 33phpcs lib/phpmd(both rulesets)phpstan[OK] No errorspsalmNo errors found!eslint src/integrations/registerDecisionsLeaf.jsprettier --checknpm run test:l10nOK — every used translation key is presentOCA\OpenRegister\Service\FileServicemocks: CI checks outopenregister@developmentas an additional app and binds the real class. My change contributes exactly +9 tests / +57 assertions and moves neither number.Two things reported, deliberately NOT fixed here
1. gate-24's checker has a class-name blind spot.
collectServerFaces()builds itsClass::CONSTtable from the FIRST/class\s+(\w+)/match in each PHP file. Inlib/AppInfo/Application.phpthat match is the word "class" in the header docblock — verified: it returns"for"at offset 58, from "Main application class for the Decidesk Nextcloud app." SoApplication::APP_IDis never resolvable and R4 silently skipsrequiredAppon this repo. That sentence appears in most Conduction app bootstraps, so this is fleet-wide. Not patched here on purpose:scripts/check-integration-parity.jsis the generation the board proposes to vendor intohydra-gatesas-is (md58516176…), and forking it in one app would make the seed a moving target. It belongs inConductionNL/.github.2.
LeafDescriptor::toArray()dropsreferenceTypeandrequiresPermission.LeafRegistry::describeForCapabilities()publishestoArray()plus ausableflag, andtoArray()omits both fields. So the AD-18referenceTypemarker — the thing that tells a consumer which schema property renders this leaf's single-entity surface — is declared on the descriptor and never reaches the capability. An openregister question, not decidesk's; the stub mirrors the omission rather than helpfully filling it in, so no test here asserts a payload OpenRegister does not emit.Not in this PR
decidesk's OpenRegister
authorizationblocks. That is a separate, real security finding with a behaviour change of its own and it gets its own PR.