Add .m.rule.is_room_mention push rule to DEFAULT_OVERRIDE_RULES

[t3chguy]

To handle the client not knowing it due to the server not proactively telling us down incremental syncs

[richvdh]

this still needs to happen before existing push rules?

[t3chguy]

this still needs to happen before existing push rules?

The spec doesn't state that the defaults are ordered as far as I can tell, we can't assume what order the server actually holds the rules in. The spec says rules can specify an ordering priority within a kind and that .m.rule.master beats all priority, nothing else I can spot

[richvdh]

we can't assume what order the server actually holds the rules in

You absolutely can.

I'm afraid that, whatever the words of the spec might say, the ordering of the push rules is very very important. If that's not obvious from the spec, then clarification PRs welcome, but it doesn't change anything here.

Note that MSC3952 explicitly says where the new push rule fits in:

The .m.rule.is_room_mention override push rule would appear directly before the .m.rule.roomnotif push rule:

[t3chguy]

In which case the current code is already not spec compliant as it appends .m.rule.reaction though it should instead occur before .m.rule.room.server_acl and .m.rule.suppress_edits and we're not making the situation any worse yet still fixing bugs.

[richvdh]

well, .m.rule.reaction is safe to reorder with respect to .m.rule.room.server_acl at least, because they cannot both match on the same event. .m.rule.is_room_mention is quite a bit earlier so I don't think we can et away with the same bodge.

We just need a list of the rule_ids for all the override rules, so that pushprocessor.ts can put them in the right order.

[t3chguy]

well, .m.rule.reaction is safe to reorder with respect to .m.rule.room.server_acl at least, because they cannot both match on the same event. .m.rule.is_room_mention is quite a bit earlier so I don't think we can et away with the same bodge.

The client ignores .m.rule.is_room_mention if the event has m.mentions so this should be fine

We just need a list of the rule_ids for all the override rules, so that pushprocessor.ts can put them in the right order.

Wouldn't this break if the spec adds a new rule inbetwixt other existing rules especially when MSC-namespaced rules are also in play, as they currently are in the hardcoded list

[richvdh]

well, .m.rule.reaction is safe to reorder with respect to .m.rule.room.server_acl at least, because they cannot both match on the same event. .m.rule.is_room_mention is quite a bit earlier so I don't think we can et away with the same bodge.

The client ignores .m.rule.is_room_mention if the event has m.mentions so this should be fine

Ok, but what about the other rules that come after .m.rule.is_room_mention? They might be fine but please could you add a comment explaining why it's safe for each of them to happen before .m.rule.is_room_mention ?

We just need a list of the rule_ids for all the override rules, so that pushprocessor.ts can put them in the right order.

Wouldn't this break if the spec adds a new rule inbetwixt other existing rules especially when MSC-namespaced rules are also in play, as they currently are in the hardcoded list

I don't follow, I'm afraid. I'm thinking of an algorithm that iterates through the current rules, comparing them against the expected rules, and inserting the new ones where needed.

EXPECTED_OVERRIDE_RULE_IDS = [".m.rule.master", ".m.rule.suppress_notices", /* ... */ ".m.rule.suppress_edits"];

let next_expected_rule_id_index = 0;
let new_rules = [];
for (rule of current_rules) {
    if (!rule.default) {
        // got to the end of the default list
        break;
    }
    const rule_index = EXPECTED_OVERRIDE_RULE_IDS.indexOf(rule.rule_id);
    if (rule_index === -1) {
        // an unrecognised rule; copy it over
        new_rules.push(rule);
        continue;
    }
    while (rule_index > next_expected_rule_index) {
        // insert new rules
        new_rules.push(EXPECTED_OVERRIDE_RULE_IDS[next_expected_rule_index]);
        next_expected_rule_index += 1;
    }    
    // copy over the existing rule
    new_rules.push(rule);
    next_expected_rule_index += 1;
}

// TODO: now copy over any remaining EXPECTED_OVERRIDE_RULE_IDS, and finally any
// non-default rules that were in `current_rules`.

Now, if the spec adds a new rule which we don't know about but the server does, we'll just copy it over. Right? What's the failure mode you're thinking of?

[yes, the need to do all of this complexity is horrible. Push rules in general are horrible, and I wish we'd get rid of them rather than continually trying to patch the system with "just one more push rule". But here we are.]

[t3chguy]

Now, if the spec adds a new rule which we don't know about but the server does, we'll just copy it over. Right? What's the failure mode you're thinking of?

If the server adds Y between X & Z (X, Y, Z) but we already have a local MSC rule M in order X, M, Z. Now we don't know the total order, is it XMYZ or XYMZ, one may cause buggy behaviour

[richvdh]

I mean, that's kinda on you for adopting MSC rules before they are specced. If the MSC doesn't take a view on whether it goes before or after Y, then everything implementing that MSC is basically broken because we have no idea what different implementations will do.

Presumably that shouldn't be a problem in practice, because by the time Y is specced, M has either itself been specced, or has failed to do so (in which case it seems like high time to rip it out of the codebase).

Anyway, can't we cross that bridge if/when we come to it? It's all a bit hypothetical and doesn't seem of much relevance to how we can implement this particular change correctly?

[richvdh]

Generally I feel like this is a bit under-tested, and it would lend itself well to unit tests which directly call rewriteDefaultRules. The special-casing of is_user_mention, and the fact that we update conditions and actions seems important, and I'm worried that we can rip it out without the tests caring.

[richvdh]

Sorry, what special-casing?

The special-casing that you just convinced me we don't need

[richvdh]

LGTM 🎉