fix(notifications): make the accepted filter grammar the executable one - #2794
Merged
Conversation
added 2 commits
August 22, 2026 13:04
24 scheduled-notification filter entries across three apps were accepted at save time and matched nothing at scan time, silently, for months. Every deadline rappel in decidesk, shillinq's contract and AP reminders, and openconnector's job-overdue rule ran daily against nothing. The cause was two enumerations of one grammar that disagreed. NotificationAnnotationValidator opened with "Scalar shortcut: always accepted" for any array lacking an "operator" key; ScheduledFilterEvaluator then compared that array to a scalar field by identity. Accepted, then false forever. There is now one ScheduledFilterParser. The validator reports its errors and the evaluator walks its AST, so "a shape the validator accepts but the evaluator cannot run" is a contradiction rather than a bug that recurs. The grammar also grows what three teams independently reached for, which is why 22 of the 23 dead rules come back with no edit in any app: in / notIn membership; an array field matches on intersection before / after against "now", an absolute date, or a SIGNED duration [a, b] a bare list is shorthand for "in" all / any combinators, nested to a bounded depth before "P7D" reads equally naturally as "a week from now" and "a week ago", so the past direction is spelled "-P7D" and there is no unsigned past form. An ambiguous date operator in a reminder engine is how a thousand wrong emails get sent. Verified by replaying the fleet census against the new parser: 60 of 61 scheduled filters are executable, and the single rejection is openconnector's job.job-overdue, which spells the key "op" and now gets an error naming "operator". All 267 notification tests pass, including the 80 pre-existing ones unchanged. Closes #2787
The repo's phpcs standard requires named arguments on internal calls; the accept()/reject() helpers introduced while reducing method complexity were called positionally. Caught by CI, not locally, because the local sweep ran before the last refactor.
Contributor
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ❌ | ||||
| phpmd | ❌ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| composer | ✅ | ✅ 175/175 | |||
| npm | ✅ | ✅ 528/528 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ❌ | ||||
| Newman | ✅ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-22 11:11 UTC
Download the full PDF report from the workflow artifacts.
CI runs a second phpmd pass (phpmd-unusedparams.xml) that the main pass does not cover; resolveInstant took an $operator it never read. Caught there, not locally, because I ran only the main pass.
Contributor
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ❌ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| composer | ✅ | ✅ 175/175 | |||
| npm | ✅ | ✅ 528/528 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-22 11:27 UTC
Download the full PDF report from the workflow artifacts.
rubenvdlinde
added a commit
to ConductionNL/integriq
that referenced
this pull request
Aug 22, 2026
All three notification filters in this register used an invented dialect and had never fired. job.job-overdue (scheduled) named its operator under "op" and used "lt", neither of which the engine implements. Rewritten in the canonical grammar: "isEnabled" as a scalar equality and "nextRun" with the "before" operator that ConductionNL/openregister#2794 adds. Verified against the real parser and evaluator: it now notifies for an enabled job whose nextRun has passed, and stays silent for a disabled job or a future run. job_log.job-error (created) passed a field=>spec map where the created path reads a single {field, operator, value} clause. Rewritten as a clause. call_log.call-failed (created) cannot be repaired yet. It wants "statusCode >= 400", and createdFilterMatches() implements only equals|in|notIn over string comparison — there is no numeric operator on the created path. Rewriting it as an in-list of every 4xx/5xx code would be wrong the first time an unusual status appeared, so it is disabled with a _note explaining why, what it is blocked on, and a warning not to simply re-enable it: with no filter it would match every call_log row. Refs ConductionNL/openregister#2787 Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
Contributor
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| composer | ✅ | ✅ 175/175 | |||
| npm | ✅ | ✅ 528/528 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-22 11:42 UTC
Download the full PDF report from the workflow artifacts.
rubenvdlinde
pushed a commit
to ConductionNL/decidiq
that referenced
this pull request
Aug 22, 2026
15 updated-trigger rules declared a `filter`. The dispatcher does not read `filter` on an updated trigger — it reads `condition`, and its own comment records the consequence: "condition-less `updated` rules match on type alone (back-compat)". `filter` is consulted only for `created` triggers. So every one of these fired on EVERY update to the object, not on the state change they name. Anyone subscribed has been notified for each edit. This is the opposite failure from the silent ones fixed elsewhere in this sweep: not too quiet, far too loud. Each is rewritten as a condition. Where the lifecycle admits exactly one predecessor for the target state, `from` is included as well, so the rule fires on the transition itself rather than on any update while the state holds — 10 of the 15 qualify. mvIngepland and geheimhoudingOpgeheven have two possible predecessors and the grammar takes a single `from`, so they use equals alone and say so in a _note. consultationBesluitAfwijkend and the two Transcript rules have no lifecycle on that field. Three further rules in the same register were dead for an adjacent reason — an operator fieldChangeConditionMatches does not implement (it has only `changed` and `equals`, and an unknown operator falls through to false): - decisionSuperseded, decisionRepealed: isNotEmpty -> changed, which is a faithful reading of "a link was set". - outcomeEmitted: `in` has no equivalent. Left declared with a _note rather than split into three near-identical rules, because the scheduled path gained `in` in ConductionNL/openregister#2794 and the updated path should follow; splitting now would only have to be undone. Refs #849
rubenvdlinde
added a commit
to ConductionNL/decidiq
that referenced
this pull request
Aug 22, 2026
…s (19 rules) (#848) * fix(notifications): reactionPendingModeration used the wrong filter shape The created-trigger path reads a single clause, {field, operator, value} (AnnotationNotificationDispatcher::createdFilterMatches). This rule passed a field=>value map — the shape the SCHEDULED path takes — so the dispatcher looked up $filter['field'], found nothing, and returned false for every reaction ever created. Moderators have never been told a reaction is waiting. Rewritten as a clause. Verified against the dispatcher's own semantics: a pending reaction notifies, an approved one does not, and a reaction with no moderationStatus does not. Found by a fleet sweep of created-trigger filters after the same class of defect turned up in 24 scheduled filters (ConductionNL/openregister#2787). * fix(notifications): updated triggers carried a filter the engine ignores 15 updated-trigger rules declared a `filter`. The dispatcher does not read `filter` on an updated trigger — it reads `condition`, and its own comment records the consequence: "condition-less `updated` rules match on type alone (back-compat)". `filter` is consulted only for `created` triggers. So every one of these fired on EVERY update to the object, not on the state change they name. Anyone subscribed has been notified for each edit. This is the opposite failure from the silent ones fixed elsewhere in this sweep: not too quiet, far too loud. Each is rewritten as a condition. Where the lifecycle admits exactly one predecessor for the target state, `from` is included as well, so the rule fires on the transition itself rather than on any update while the state holds — 10 of the 15 qualify. mvIngepland and geheimhoudingOpgeheven have two possible predecessors and the grammar takes a single `from`, so they use equals alone and say so in a _note. consultationBesluitAfwijkend and the two Transcript rules have no lifecycle on that field. Three further rules in the same register were dead for an adjacent reason — an operator fieldChangeConditionMatches does not implement (it has only `changed` and `equals`, and an unknown operator falls through to false): - decisionSuperseded, decisionRepealed: isNotEmpty -> changed, which is a faithful reading of "a link was set". - outcomeEmitted: `in` has no equivalent. Left declared with a _note rather than split into three near-identical rules, because the scheduled path gained `in` in ConductionNL/openregister#2794 and the updated path should follow; splitting now would only have to be undone. Refs #849 --------- Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
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.
Closes #2787
What was broken
24 scheduled-notification filter entries across three apps were accepted at save time and matched nothing at scan time — silently, for months. Every deadline rappel in decidesk, shillinq's contract and AP reminders, and openconnector's job-overdue rule ran daily against nothing while reading as configured and enabled.
The cause was one grammar written down twice, disagreeing.
NotificationAnnotationValidator::validateScheduledFilterEntry()opened with:Any array without an
operatorkey was accepted.ScheduledFilterEvaluatorthen compared that array to a scalar field with===. Accepted, then false forever.Three teams had independently invented three dialects the four-operator grammar could not express — decidesk a bare list for set membership (18 rules), shillinq an
allcombinator withnotIn/before(4 rules), openconnector the keyopwithlt(1 rule, 2 entries).The fix
One
ScheduledFilterParser. The validator reports its errors; the evaluator walks its AST. "A shape the validator accepts but the evaluator cannot execute" is now a contradiction rather than a bug that recurs. No second enumeration of operators exists inlib/.The grammar grows what three teams reached for, which is why 22 of the 23 dead rules come back with no edit in any app:
in/notIn(values)before/after(value)"now", an absolute ISO date, or a signed duration["a","b"]inall/anyMAX_DEPTHbefore "P7D"reads equally naturally as "a week from now" and "a week ago", so the past direction is spelled-P7Dand there is no unsigned past form. An ambiguous date operator in a reminder engine is how a thousand wrong emails get sent.An empty
allmatches; an emptyanydoes not — "any of nothing" is false, and the alternative silently widens a rule to the whole table.Verification
Fleet census replayed against the new parser — 61 scheduled filters across the fleet:
The single rejection is exactly the one intended, and it now produces a message naming
operatorrather than a silent accept.Behaviour, old vs new, on the real annotations:
{"lifecycle": ["open","in-uitvoering"]}— matchesopenandin-uitvoering, notafgedaan, not a missing field.all+notIn+before— matches open-and-overdue, not paid, not future-dated.Tests: 267 notification tests pass, 540 assertions — including the 80 pre-existing tests unchanged, which is the backward-compatibility proof for the 49 filter entries that already worked (28 scalar, 21 canonical operator objects).
Gates: PHPCS 0 errors, PHPMD exit 0, PHPStan OK, Psalm no errors.
One correction to the issue as I first filed it
I originally wrote that a stricter validator "would have blocked" these. It would not have.
SchemaMapper::validateNotificationsAnnotation()(lib/Db/SchemaMapper.php:1364-1381) calls the validator and discards its errors by design, so a malformed optional annotation never aborts a register import. Tightening the validator is still right — it is what makes the accept-set equal the executable-set — but the Hydra gate is the only mechanism that can actually block this, and the spec argues it that way. Gate work is task 5.3, inConductionNL/.github, and is not in this PR.Follow-ups (named, not done here)
job.job-overdueneedsop->operatorandlt->before; pluscall_log.call-failedandjob_log.job-error, which pass map-shapedopfilters to the created-trigger path and were also never firing.hydra-gates, pinned against a fixture generated fromScheduledFilterGrammarso gate and engine cannot drift.created-vs-scheduledconvergence: converge the clause shape and vocabulary, explicitly not the comparison semantics (created is string-coercing, scheduled is strict===; merging either direction silently un-matches live rules).The design for all of this is the
notification-scheduled-filter-grammarOpenSpec change, which ships in #2791.