fix: look for new mail when a push names changes it can't read #34

The native push router and the push task decided separately whether a
push may carry mail, and disagreed. For an account entry that is not
an object, or a map that names no type, the router started the mail
task as possible mail, but the task's guard read the same payload as
"no mail" and returned, so new mail it announced was never looked for.
A push without a kind was judged by its map in the router but taken
for mail by the task.

Both now apply the same rules: anything but a StateChange, a map that
is missing, unreadable or names no type, an unreadable account entry,
or a mail type means the mail task runs. Only a readable StateChange
naming contact, calendar or other non-mail types skips it.

Checked with one table of payloads run against the router's decision
and the task's guard alike.

android/app/src/main/java/com/anonymous/bulwarkmobile/sync/PushRouting.kt

@@ -13,9 +13,11 @@ import org.json.JSONObject
  * The relay forwards the JMAP StateChange's `changed` map verbatim (FCM data
  * key `changed`: JMAP account id → type → state; only its first account is
  * repeated as `accountId`), so every account key is looked at. The mail task
- * runs only for mail: when some account's map has a mail type, for an
- * EmailPush, or when `changed` is missing or unreadable (older relays), so a
- * contact change never ends in a mail notification.
+ * runs only for mail: when some account's map has a mail type, for anything
+ * but a StateChange (an EmailPush), when `changed` is missing, unreadable or
+ * names no type (older relays), or when an account's entry is unreadable, so
+ * a contact change never ends in a mail notification. The push task's guard
+ * (`carriesNoMail` in src/lib/push-background-task.ts) applies the same rules.
  */
 object PushRouting {
     /** An Android account and the authorities it syncs, fed by one JMAP account. */
@@ -27,7 +29,7 @@ object PushRouting {
         val startMailTask: Boolean,
     )
 
-    private const val EMAIL_PUSH_KIND = "jmap-email-push"
+    private const val STATE_CHANGE_KIND = "jmap-state-change"
 
     /**
      * Types that may mean new mail: `EmailDelivery`, and `Email`/`Mailbox`,
@@ -42,7 +44,8 @@ object PushRouting {
 
     fun decide(data: Map<String, String>, routes: Map<String, List<Route>>): Decision {
         val changed = data["changed"]?.let(::parseChanged)
-        var mail = data["kind"] == EMAIL_PUSH_KIND || changed == null
+        // Only a StateChange can say it carries no mail.
+        var mail = data["kind"] != STATE_CHANGE_KIND || changed == null
         val syncs = LinkedHashSet<Pair<String, String>>()
         for ((jmapAccountId, types) in changed.orEmpty()) {
             if (types == null) {

android/app/src/test/java/com/anonymous/bulwarkmobile/sync/PushRoutingTest.kt

@@ -108,6 +108,56 @@ class PushRoutingTest {
         assertEquals(setOf("alice@example.org" to contacts), decision.syncs)
     }
 
+    @Test
+    fun `reads a push the same way as the push task`() {
+        // kind (null: none), changed (null: none) → whether the mail task starts. The
+        // same table as MAIL_CASES in src/lib/__tests__/push-background-task.test.ts,
+        // whose carriesNoMail must agree.
+        val state = "jmap-state-change"
+        val emailPush = "jmap-email-push"
+        val cases: List<Triple<String?, String?, Boolean>> = listOf(
+            // No readable map: as before device sync.
+            Triple(state, null, true),
+            Triple(state, "", true),
+            Triple(state, "not json", true),
+            Triple(state, "[1]", true),
+            Triple(state, "{}", true),
+            // A map that names no type is read like a missing one.
+            Triple(state, """{"c":{}}""", true),
+            Triple(state, """{"c":{},"team":{}}""", true),
+            // An account entry that can't be read may be mail.
+            Triple(state, """{"c":null}""", true),
+            Triple(state, """{"c":"garbled","team":{"ContactCard":"s"}}""", true),
+            Triple(state, """{"c":[],"team":{"ContactCard":"s"}}""", true),
+            Triple(state, """{"c":5}""", true),
+            // Mail types anywhere in the map.
+            Triple(state, """{"c":{"EmailDelivery":"s"}}""", true),
+            Triple(state, """{"c":{"Email":"s"}}""", true),
+            Triple(state, """{"c":{"Mailbox":"s"}}""", true),
+            Triple(state, """{"c":{"ContactCard":"s1"},"team":{"EmailDelivery":"s2"}}""", true),
+            // Only a StateChange can say it carries no mail.
+            Triple(null, """{"c":{"ContactCard":"s"}}""", true),
+            Triple(emailPush, """{"c":{"EmailDelivery":"s"}}""", true),
+            Triple(emailPush, null, true),
+            // Contact and calendar changes, and types that say nothing about mail.
+            Triple(state, """{"c":{"ContactCard":"s1","AddressBook":"s2"}}""", false),
+            Triple(state, """{"c":{"CalendarEvent":"e1"},"team":{"Calendar":"c1"}}""", false),
+            Triple(state, """{"c":{"ContactCard":"s"},"team":{}}""", false),
+            Triple(state, """{"elsewhere":{"ContactCard":"s"}}""", false),
+            Triple(state, """{"c":{"Thread":"t1","ContactCard":"s1"}}""", false),
+        )
+        for ((kind, changed, mail) in cases) {
+            val data = buildMap<String, String> {
+                if (kind != null) put("kind", kind)
+                put("accountLabel", "alice")
+                put("accountId", "c")
+                put("emailIds", "[]")
+                if (changed != null) put("changed", changed)
+            }
+            assertEquals("kind=$kind changed=$changed", mail, PushRouting.decide(data, routes).startMailTask)
+        }
+    }
+
     @Test
     fun `accounts without routes are ignored`() {
         val decision = PushRouting.decide(stateChange("""{"elsewhere":{"ContactCard":"s"}}"""), routes)

src/lib/__tests__/push-background-task.test.ts

@@ -238,6 +238,63 @@ describe('pushes for device sync (#34)', () => {
     expect(jmapClient.getStoredCredentials).toHaveBeenCalledWith(LOCAL);
   });
 
+  // kind (undefined: none), changed (undefined: none) → whether the push may
+  // carry mail. The same table as "reads a push the same way as the push task"
+  // in PushRoutingTest.kt, whose startMailTask must agree.
+  const STATE = 'jmap-state-change';
+  const EMAIL_PUSH = 'jmap-email-push';
+  const MAIL_CASES: Array<[string | undefined, string | undefined, boolean]> = [
+    // No readable map: as before device sync.
+    [STATE, undefined, true],
+    [STATE, '', true],
+    [STATE, 'not json', true],
+    [STATE, '[1]', true],
+    [STATE, '{}', true],
+    // A map that names no type is read like a missing one.
+    [STATE, '{"c":{}}', true],
+    [STATE, '{"c":{},"team":{}}', true],
+    // An account entry that can't be read may be mail.
+    [STATE, '{"c":null}', true],
+    [STATE, '{"c":"garbled","team":{"ContactCard":"s"}}', true],
+    [STATE, '{"c":[],"team":{"ContactCard":"s"}}', true],
+    [STATE, '{"c":5}', true],
+    // Mail types anywhere in the map.
+    [STATE, '{"c":{"EmailDelivery":"s"}}', true],
+    [STATE, '{"c":{"Email":"s"}}', true],
+    [STATE, '{"c":{"Mailbox":"s"}}', true],
+    [STATE, '{"c":{"ContactCard":"s1"},"team":{"EmailDelivery":"s2"}}', true],
+    // Only a StateChange can say it carries no mail.
+    [undefined, '{"c":{"ContactCard":"s"}}', true],
+    [EMAIL_PUSH, '{"c":{"EmailDelivery":"s"}}', true],
+    [EMAIL_PUSH, undefined, true],
+    // Contact and calendar changes, and types that say nothing about mail.
+    [STATE, '{"c":{"ContactCard":"s1","AddressBook":"s2"}}', false],
+    [STATE, '{"c":{"CalendarEvent":"e1"},"team":{"Calendar":"c1"}}', false],
+    [STATE, '{"c":{"ContactCard":"s"},"team":{}}', false],
+    [STATE, '{"elsewhere":{"ContactCard":"s"}}', false],
+    [STATE, '{"c":{"Thread":"t1","ContactCard":"s1"}}', false],
+  ];
+
+  it('reads a push the same way as the native router', () => {
+    for (const [kind, changed, mail] of MAIL_CASES) {
+      const data: Record<string, string> = { accountLabel: 'alice', accountId: 'c', emailIds: '[]' };
+      if (kind !== undefined) data.kind = kind;
+      if (changed !== undefined) data.changed = changed;
+      expect(carriesNoMail(parseRelayPushData(data)), `kind=${kind} changed=${changed}`).toBe(!mail);
+    }
+    // Native code hands `changed` over as a string; a map that is not an
+    // object is unreadable in any form.
+    expect(carriesNoMail(parseRelayPushData({ kind: STATE, changed: [{ ContactCard: 's' }] }))).toBe(false);
+  });
+
+  it("looks for mail when it can't read an account's entry, as the native router does", async () => {
+    for (const changed of ['{"jmap-primary":null}', '{"jmap-primary":{}}']) {
+      vi.clearAllMocks();
+      await pushBackgroundTask({ kind: STATE, accountLabel: 'alice', accountId: 'jmap-primary', emailIds: '[]', changed });
+      expect(jmapClient.getStoredCredentials, changed).toHaveBeenCalledWith(LOCAL);
+    }
+  });
+
   it('takes an Email or Mailbox change for possible mail, as a subscription from an older build sends it', () => {
     expect(carriesNoMail(parseRelayPushData(stateChange({ a: { Email: 's1', Mailbox: 's2' } })))).toBe(false);
     expect(carriesNoMail(parseRelayPushData(stateChange({ a: { Email: 's1' } })))).toBe(false);

src/lib/__tests__/push-notifications.test.ts

@@ -409,7 +409,7 @@ describe('push types for device sync (#34)', () => {
   }
 
   // A server that does not know a type refuses the whole write.
-  const refusedTypes = (types: string[] | undefined) => {
+  const refuseUnknownTypes = (types: string[] | undefined) => {
     const unknown = (types ?? []).filter((type) => type !== 'EmailDelivery');
     if (unknown.length > 0) throw new JMAPMethodError('invalidProperties', `Unknown types: ${unknown.join(', ')}`);
   };
@@ -458,7 +458,7 @@ describe('push types for device sync (#34)', () => {
       capabilities: { 'urn:ietf:params:jmap:core': {} },
     };
     createMock.mockImplementation(async (params: { types: string[] }) => {
-      refusedTypes(params.types);
+      refuseUnknownTypes(params.types);
       return CREATED;
     });
     const result = await setupPushNotifications({ relayBaseUrl: RELAY });
@@ -483,10 +483,10 @@ describe('push types for device sync (#34)', () => {
   it('keeps mail push on a server that refuses the device sync types', async () => {
     await syncing({ [CONTACTS]: true });
     createMock.mockImplementation(async (params: { types: string[] }) => {
-      refusedTypes(params.types);
+      refuseUnknownTypes(params.types);
       return CREATED;
     });
-    updateMock.mockImplementation(async (_id: string, patch: { types?: string[] }) => refusedTypes(patch.types));
+    updateMock.mockImplementation(async (_id: string, patch: { types?: string[] }) => refuseUnknownTypes(patch.types));
 
     // A new subscription: mail-only rather than none.
     const created = await setupPushNotifications({ relayBaseUrl: RELAY });

src/lib/push-background-task.ts

@@ -154,7 +154,7 @@ export function parseRelayPushData(data: unknown): RelayPushData {
     } catch {
       // ignore
     }
-  } else if (d.changed && typeof d.changed === 'object') {
+  } else if (d.changed && typeof d.changed === 'object' && !Array.isArray(d.changed)) {
     out.changed = d.changed as Record<string, Record<string, string>>;
   }
   // Older relays only sent `changed`; the account id is its first key.
@@ -389,14 +389,17 @@ const MAIL_TYPES = ['EmailDelivery', 'Email', 'Mailbox'];
  * calendar changes, which the native push router already turned into device
  * syncs (#34, docs/device-sync.md "Triggers"). Without a mail type there is
  * nothing to notify, and the legacy path (the newest unread mail) must not
- * run for it. An empty or missing `changed` map keeps the old behaviour.
+ * run for it. A missing map, or one that names no type, keeps the old
+ * behaviour, and an account entry it can't read may be mail. The same rules
+ * as PushRouting.decide, so the two never disagree about a push.
  */
 export function carriesNoMail(payload: RelayPushData): boolean {
   if (payload.kind !== 'jmap-state-change' || !payload.changed) return false;
-  const accounts = Object.values(payload.changed);
-  if (accounts.length === 0) return false;
-  return !accounts.some((types) => !!types && typeof types === 'object'
-    && MAIL_TYPES.some((type) => type in types));
+  const entries: unknown[] = Object.values(payload.changed);
+  if (entries.some((types) => !types || typeof types !== 'object' || Array.isArray(types))) return false;
+  const types = entries.flatMap((entry) => Object.keys(entry as Record<string, unknown>));
+  if (types.length === 0) return false;
+  return !types.some((type) => MAIL_TYPES.includes(type));
 }
 
 // Fired by BulwarkPushTaskService when a data FCM message arrives. Runs in a