fix(dmn): carry rule priority, and pass the uuid as the uuid to deleteObject - #1564
Merged
Conversation
dossiq#1561 made the engine accept PRIORITY. Driving it against a live instance showed that was only half the story: the table evaluated, returned 200, and gave the WRONG answer, picking the priority-1 rule over the priority-10 one. Two things dropped the rank before it reached the evaluator. The DecisionTable schema had no `priority` property on a rule, because dossiq's old engine never implemented the policy that uses it. Added, defaulting to 0. DecisionTableService::validateRules() rebuilds every rule from a fixed list of four keys, so `priority` was stripped on write even once the schema allowed it. It is now carried, and only when the author supplied it: writing a default onto every rule of every table would put a meaningless field on tables that do not use it. Verified end to end: a PRIORITY table with rules ranked 1 and 10 now returns the rule ranked 10. Neither the unit tests nor reading the code found this. The e2e did, which is the argument for having written it.
…five places OpenRegister's signature is `deleteObject(uuid, register, schema, ...)`. Five dossiq services called it as `deleteObject($register, $schema, $id)`, which transposes all three arguments: the register id lands in the uuid slot and the object's uuid lands in the schema slot. Every one of those deletes has been failing. On a live instance the decision-table delete answered HTTP 500 with "Did expect one result but found none" against openregister_registers, because it was looking up a register whose id was really a schema's. A sixth, TenantSaasService, passed `id:` as a named argument. There is no `$id` parameter, so that raised "Unknown named parameter" and was swallowed by the surrounding catch. InspectionChecklistService already carries a docblock describing this exact defect, found the same way against a live instance: "no checklist has ever been deleted through the UI". That instance was fixed; the class never was. Swept now, and every remaining call site verified to use named arguments. Why no test caught it: AdvisoryBodyServiceTest's fake declared `deleteObject(string $register, string $schema, string $id)`. The fake had been written to agree with the CALLER rather than with OpenRegister, so it accepted the transposed call and the test passed on it. The fake now carries the real signature, and a new test asserts the uuid arrives in the uuid slot. It fails when the transposed call is put back, which is the only reason to trust it. Verified on a live instance: DELETE now answers 200 where it answered 500.
rubenvdlinde
requested review from
SudoThijn,
WilcoLouwerse,
bbrands02,
remko48 and
rjzondervan
as code owners
August 31, 2026 05:39
The dossiq-consumes-shared-dmn change asserts "PRIORITY MUST return the matching rule with the highest priority". When it merged, that was not satisfiable: the schema carried no `priority` property and validateRules() stripped the field, so a PRIORITY table returned 200 with the wrong rule. The requirement is right and is now met. The task list should say that it took a second change, rather than reading as though it was true on the day.
Contributor
Quality Report — ConductionNL/dossiq @
|
| 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-31 05:54 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Quality Report — ConductionNL/dossiq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| check-vue3-compile | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ❌ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 106/106 | |||
| npm | ✅ | ✅ 540/540 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | 🚨 NO VERDICT — enabled but never ran | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-31 06:18 UTC
Download the full PDF report from the workflow artifacts.
Two CI findings, both mine. Prettier: the DMN e2e test I added was not formatted. Ran the repo's own `format:fix`; the whole tree is clean. gate-16 spec-coverage: `RoutingRuleService::deleteRule()` and `DecisionTableService::validateRules()` changed without an `@spec`. Both do implement a specified requirement, so the tags say something true rather than silencing the gate: deleteRule carries the KCC task its class docblock already names, validateRules the dmn-decision-tables spec its siblings all carry.
Contributor
Quality Report — ConductionNL/dossiq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| check-vue3-compile | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 106/106 | |||
| npm | ✅ | ✅ 540/540 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | 🚨 NO VERDICT — enabled but never ran | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-31 07:06 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.
Follow-up to #1561, driven by writing the Playwright e2e for it. Both fixes here were found by running the thing, not by reading it.
1. PRIORITY was accepted but could not rank
#1561 made the engine accept
PRIORITY. Against a live instance that turned out to be half the story: the table evaluated, answered HTTP 200, and gave the wrong answer — the priority-1 rule beat the priority-10 one.Two things dropped the rank before it ever reached the evaluator:
priorityproperty on a rule, because dossiq's old engine never implemented the policy that uses it.DecisionTableService::validateRules()rebuilds every rule from a fixed list of four keys, soprioritywas stripped on write even after the schema allowed it.Both fixed.
priorityis carried only when the author supplied it, so tables that do not use PRIORITY do not gain a meaningless field.Verified end to end: a PRIORITY table ranked 1 and 10 now returns the rule ranked 10.
This is the argument for the e2e. A 200 with a plausible-looking decision is exactly what a unit test with a mocked evaluator cannot catch, and I had written those unit tests myself.
2. Five deletes that have never worked
OpenRegister's signature is
deleteObject(uuid, register, schema, ...). Five dossiq services called it asdeleteObject($register, $schema, $id)— all three arguments transposed. On a live instance the decision-table delete answered 500 withDid expect one result but found noneagainstopenregister_registers, because it was looking up a register whose id was really a schema's.A sixth,
TenantSaasService, passedid:as a named argument. There is no$idparameter, so it raisedUnknown named parameterand was swallowed by the surrounding catch.InspectionChecklistServicealready carries a docblock describing this exact defect, found the same way: "no checklist has ever been deleted through the UI". That instance was fixed; the class never was. Swept now, with every remaining call site checked.Why no test caught it
AdvisoryBodyServiceTest's fake declared:The fake had been written to agree with the caller rather than with OpenRegister, so it accepted the transposed call and the test passed on it. A fake that encodes the bug as the contract cannot fail.
The fake now carries the real signature, and a new test asserts the uuid arrives in the uuid slot. I put the transposed call back and confirmed the test goes red, because a test I have not seen fail is not evidence.
Verification
phpstan,phpcs,phpmdcleanchanged-surfaces.spec.ts: 5/5, including the new PRIORITY test, against a live instanceDELETE /api/decisions/{id}: 200, was 500