fix(approval-routes): repair the parafering seam and unshadow the registry endpoint - #1107
Merged
Merged
Conversation
…istry endpoint Three HIGH defects from the rig re-proof. Unlabeled steps wedge every cross-app route (defect 2). The decision-stage schema requires a label, instantiate() wrote '' for steps without one, OpenRegister stored NULL, and the patch recording the FIRST sign-off then 400'd — dossiq's held routes carry no step labels, so the cross-app case always hit it. The schema keeps its requirement (the route timeline displays the label); instantiate() now derives one from the stage type and step number via the shared mapper (labelOf()). The ordering bug the 400 exposed is fixed too: record() writes the stage FIRST and appends the action row after, so a refused stage write no longer leaves an orphan action row per retry — pinned by a no-orphan test on exactly the legacy NULL-label shape. An idempotent post-migration repair step (RepairDecisionStageLabels, runAsSystem, fail-soft) backfills derived labels onto existing NULL-label stages; the orphan action rows are deliberately KEPT — they are the audit record of what the signer did. Conclusions were never announced (defect 3). provenanceOf() resolved the route with findAll(['id' => ...]), and a top-level id filter matches NOTHING in OpenRegister (identity lives in @self), so sourceApp resolved empty, every conclusion read as 'internal route' and dossiq never heard one. The store gains find() (get-by-uuid, RBAC-scoped), and the sweep of the whole class fixed three sites: the announcer's provenance, assertSubjectAccessible() (which refused every valid subject), and MandateDirectory::resolve() (which silently passed every LOCAL mandate as external, skipping the effective/window/delegate checks). AuditLogService's ['uuid' => ...] filter is NOT this class: it goes to OR's AuditTrailMapper where uuid is a real column. Every stateful test fake now returns zero rows for top-level id/uuid filters, like live OR — a fake that resolves the dead form agrees with the caller and cannot fail. The registry endpoint shipped dead (defect 5). decisionTypes#index (GET /api/v1/decision-types) was declared below the api#index wildcard (GET /api/v1/{resource}); Nextcloud matches in declaration order, so the wildcard answered 404 'Unknown resource' and the picker silently fell back to its 13 shipped types. The literal route now precedes the wildcard, and a new contract test loads the real route table and asserts EVERY literal route precedes every wildcard that matches it (verified red on the pre-fix table, naming exactly this route; no other literal is shadowed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rubenvdlinde
requested review from
WilcoLouwerse,
bbrands02 and
rjzondervan
as code owners
September 3, 2026 05:54
Contributor
Quality Report — ConductionNL/decidiq @
|
| 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 | ✅ | ✅ 537/537 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-03 05:59 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.
Fixes the three HIGH defects the rig re-proof found on decidiq's side of the parafering seam, plus the dead registry endpoint.
Defect 2 — routes from unlabeled steps could never advance
The decision-stage schema requires
label;instantiate()wrote''for steps without one, OpenRegister stored NULL, and the patch that records every advance re-validates the whole stage — so the FIRST sign-off 400'd with "Property 'label' should be type 'string' but is 'null'". dossiq's held routes carry no step labels, so the cross-app case always hit it.Label semantics chosen: the schema keeps its requirement — the route timeline displays the label, so a stage without one is not a valid stage (the DC#1101 intent).
instantiate()now derives a fallback from the stage type and step number (Endorsement (step 2)) via the one sharedApprovalRouteStepMapper::labelOf(), so the engine and the repair below cannot drift.Ordering fix:
record()now writes the stage FIRST and appends the action row after, on both the completing and the returned path. A refused stage write can no longer leave an orphan action row per retry; the no-orphan property is pinned by a test on exactly the legacy shape that produced the orphans (a stored NULL-label stage).Repair:
RepairDecisionStageLabels(post-migration, runAsSystem, fail-soft, idempotent) backfills derived labels onto existing NULL-label stages viaDecisionStageLabelRepair. The orphan approval-action rows are deliberately KEPT: they record what the signer actually did, and a repair that deletes sign-off rows edits history. A duplicate verb in the trail is honest; a missing one is not.Defect 3 — conclusions never announced (dead id-filter class)
provenanceOf()resolved the route withfindAll(['id' => $routeId])— a top-level id filter matches NOTHING in OpenRegister (identity lives in@self), sosourceAppresolved empty, every conclusion read as "internal route, no producer waiting", and dossiq never heard one.RegisterObjectStoregainsfind()(get-by-uuid through OR's contract, RBAC-scoped). The sweep oflib/for the class found 3 live sites, all fixed:ApprovalRouteConclusionAnnouncer::provenanceOf()— the reported defect (fail-silent).ApprovalRouteService::assertSubjectAccessible()— refused EVERY valid subject on the REST instantiate path (fail-closed).MandateDirectory::resolve()— every LOCAL mandate resolved to null and passed as an external reference, silently skipping the effective/window/delegate checks (fail-open).AuditLogService's['uuid' => ...]is NOT this class: it goes to OR'sAuditTrailMapper, where uuid is a real column.Fakes repinned (a fake that agrees with the caller cannot fail): all five stateful/mocked store fakes now return zero rows for top-level id/uuid filters, like live OR, and resolve
find()by uuid.Defect 5 — the registry endpoint shipped dead (route ordering)
decisionTypes#index(GET/api/v1/decision-types) was declared below theapi#indexwildcard (GET/api/v1/{resource}); NC matches in declaration order, so the wildcard answered 404 "Unknown resource" and the picker silently fell back to its 13 shipped types. The literal route now precedes the wildcard, with the trap documented at both ends.Contract test:
RouteDeclarationOrderContractTestloads the realappinfo/routes.phptable and asserts every literal route precedes every wildcard that matches its URL and verb — verified RED on the pre-fix table, naming exactly this one route, which also confirms no other literal route is shadowed. A second test pins the measured pair by name.Verification
lint/phpcs/psalm/phpstanall exit 0; PHPMD clean on every touched file (the label repair lives in its own class so the engine stays under the complexity ceiling).StateMachineValidator.php, warn-mode until 2026-10-03).🤖 Generated with Claude Code