# Discourse — jmap-states (round 1)

Six reviewers ran independently (principal×2, quality×2, security×1, testing×1).
The two principals reached **opposite** conclusions on the event-mask change, so
the Tech Lead arbitrated both contested findings against source. The arbitration
is the substance of this round.

---

## Contested point A — the `EVENT_JMAP_STATES` event-mask gate

Three reviewers touched the new mask
`(MESSAGE_EVENTS|FLAGS_EVENTS|MAILBOX_EVENTS|SUBS_EVENTS)` and disagreed:

- **principal-1 (CHALLENGE, High):** the mask is *too narrow* — it drops
  `EVENT_CALENDAR`/`EVENT_CALENDAR_ALARM`, so calendar events that used to carry
  `vnd.fastmail.jmapStates` silently stop.
- **security-1 (AGREE-ish):** the mask "restricts the expected param to events
  that carry a mailbox … strictly a reduction in surface" — treated as benign.
- **principal-2 (CHALLENGE, Medium):** the mask is *too wide* — `SUBS_EVENTS`
  (subscribe/unsubscribe) are in the mask but never call
  `mboxevent_extract_mailbox`, so the param is *expected but never filled* →
  `assert(filled_params())` fires in debug builds.

**Tech Lead arbitration (code-verified):**

1. **principal-1's calendar narrowing is REFUTED.** Calendar/DAV *mutations* are
   emitted as `EVENT_MESSAGE_EXPUNGE` / `EVENT_MAILBOX_DELETE|RENAME` events
   (`imap/jmap_calendar.c:5449/5460/5525/5534/5890/5933`, `imap/http_dav.c:5184/5196`,
   `imap/itip_support.c:1349/1360`) — all of which are **in** the new mask and
   still carry the param (now with corrected `Calendar`/`CalendarEvent` state).
   The genuinely calendar-typed event, `EVENT_CALENDAR_ALARM`
   (`imap/caldav_alarm.c:334`), **never calls `mboxevent_extract_mailbox`** (grep
   confirms zero such calls in that file), so it never filled jmapStates — on this
   branch *or on master*. There is no previously-emitted calendar data being
   dropped. principal-1 hedged appropriately and asked testing to confirm; the
   answer is "no regression."

2. **principal-2's over-inclusion finding is CONFIRMED — and is the real defect.**
   - The fill at `imap/mboxevent.c:1817` is gated by the *same*
     `mboxevent_expected_param` predicate as the mask, so the "unexpected param"
     direction is structurally impossible (principal-1 and security-1 are right
     about that, and the Tech Lead's original worry is unfounded).
   - But subscribe/unsubscribe (`EVENT_MAILBOX_SUBSCRIBE`/`UNSUBSCRIBE`,
     `imap/mboxlist.c:5470-5477`) are notified via `mboxevent_set_access` +
     `mboxevent_notify` and **never** call `mboxevent_extract_mailbox` → the param
     is expected (SUBS_EVENTS ∈ mask) but never filled.
   - `filled_params` is a stub returning `1` under `NDEBUG` (`mboxevent.c:2040`)
     but a **real check under debug** (`mboxevent.c:2051`): for any expected-but-
     unfilled param not on its small exemption list (`EVENT_FLAG_NAMES`,
     `EVENT_MESSAGE_CONTENT`, `EVENT_MODSEQ`), it appends to `missing` and returns
     0. `EVENT_JMAP_STATES` hits the `default` case → `assert(filled_params())` at
     `mboxevent.c:781` **fires**. The branch's own comment cites this exact assert
     as the reason for the gate — so the fix is real but **incomplete**: it
     excluded mailbox-less `Login` but left mailbox-*named*-but-counter-less
     `SUBS_EVENTS` (and likely some remote `MAILBOX_DELETE` paths) inside the mask.
   - This matters more now because the branch enables the param **globally** for
     the entire Cassandane MboxEvent suite (`MboxEvent.pm::new`), so a future
     subscribe-emitting MboxEvent test, or any assert-enabled build that subscribes
     with the param on, aborts.

   **Resolution:** promoted to **Blocker**. Suggested fix: drop `SUBS_EVENTS` from
   the mask and audit the `MAILBOX_EVENTS` members that don't always fill (remote
   `MAILBOX_DELETE`), **or** add `EVENT_JMAP_STATES` to `filled_params`' conditional
   exemptions exactly as `EVENT_MODSEQ` is handled. testing-1's F2 (no test pins
   the gate) is the corroborating coverage gap.

---

## Contested point B — does UserState/event really equal `Foo/get`?

- **principal-1 (AGREE):** correspondence is "correct by construction"; the tests
  prove it.
- **testing-1 (AGREE):** correspondence-based assertions are "the correct way to
  pin UserState == the per-type API state."
- **principal-2 (CHALLENGE, High):** true only for the **counter-derived** types
  (Email, Mailbox, Calendar, CalendarEvent, AddressBook, ContactCard,
  EmailSubmission). For **Note, SieveScript, VacationResponse** the live `Foo/get`
  reads a *different* modseq.

**Tech Lead arbitration (code-verified — principal-2 is CORRECT):**

- `Note/get` → `mbox->i.highestmodseq` of the notes mailbox (`jmap_notes.c:444`),
  **not** `req->counters.notesmodseq`.
- `SieveScript/get` → `mailbox->i.highestmodseq` of the sieve mailbox
  (`jmap_sieve.c:290`), **not** `sievemodseq`.
- `VacationResponse/get` → `sdata ? sdata->modseq : 0` (the vacation *resource*
  modseq, `jmap_vacation.c:291`), **not** `sievemodseq`.
- UserState/the event report `notesmodseq` / `sievemodseq` / `sievemodseq` for
  these three. So UserState gives `SieveScript == VacationResponse` (both
  `sievemodseq`), while their `Foo/get`s return *different* values. UserState
  cannot equal both.

The `JMAPCore/user_state` test asserts equality for all ten types and passes only
because on a freshly-created account every one of these divergent counters is `0`.
This is **false assurance**: the test "proves" a contract that is false for 3/10
types. Confirmed: principal-1's "by construction" claim holds for 7 types, not 10;
principal-2's counter-examples are real.

**Resolution:** promoted to **Blocker** (a new API ships a stated contract — "the
very same state string that type's own `Foo/get` returns" — that is untrue for
three types, with a test that hides the gap). Author must pick one: (a) reconcile
those `Foo/get`s to the per-user counter (the true unification); or (b) scope the
docstring/comment honestly and weaken the test to non-null + `J?\d+` shape for the
three divergent types, adding a *mutating* case that would otherwise expose the
gap.

---

## Consensus / corroborated findings (multiple reviewers, AGREE)

- **`(off_t)` signed pointer-cast for the offset read** — flagged by **5 of 6**
  reviewers (principal-1 F2, principal-2 F3, quality-1 F1, quality-2 F2,
  security-1 F1). Unanimous on facts: works on all supported LP64 builds, is a
  regression from the prior `(size_t)`, UB-adjacent, trivially fixed with
  `(const char *)` base arithmetic. **Very high confidence; Should Fix** (cheap,
  and the pattern now appears twice).
- **Duplicated offset-read callback** (`add_userstate_cb` vs `add_jmap_state_cb`)
  — principal-1 F3, quality-1 F2, quality-2 F1. The branch's own thesis is "one
  source of truth," so a shared `jmap_data_type_state_string()` helper completes
  it. **Suggestion.**
- **Two compiled copies of the gperf table** — principal-1 F4, principal-2 F4,
  quality-2 F5. Accepted, well-commented tradeoff forced by hidden-visibility.
  Optional: export the base copy's lookup/foreach and drop the daemon copy, or add
  a "this iterator must stay stateless" note. **Suggestion.**
- **Authorization of `UserState/get`** — security-1 and principal-1 F5 and
  principal-2 Q3 all CONCUR: identical gate to `UserCounters/get`
  (`JMAP_USERCOUNTERS_EXTENSION` + central `accountId` authz in `jmap_api()`), no
  new exposure. **Non-issue / Working Well.**
- **Hand-rolled parser memory/lifetime** — quality-2 and security-1 both verified
  `get.props`/`get.ids`/`get.state` are freed by `jmap_get_fini`, borrowed
  `properties` keys outlive the table, non-string `ids` land in `not_found`
  without deref. quality-1's "skips ids element-type check" is real but **benign**
  (handled by the `not_found` path). **Non-issue**, modulo the test gap below.

## Coverage gaps (testing-1, partially corroborated by both principals)

- J-prefix path untested (compact emailids off by default) — **Should Fix**.
- No test that a mailbox-less/non-filling event omits the param — **Should Fix**
  (directly pins both the Login fix and, with a subscribe case, Blocker #2 above).
- `UserState/get` parser negative paths (properties / bad ids / invalidArguments /
  not_found) untested — **Should Fix**.
- `:min_version_3_9` on a 3.13+ method → spurious failures on 3.9–3.12 servers —
  **Should Fix**.

## Minor (single-reviewer, low impact)

- Stray trailing blank line at `jmap_util.c:1549` (quality-2) — Suggestion.
- `properties` invalid-arg path diverges slightly from the `jmap_blob.c` idiom
  (quality-2) — Suggestion.
- Optionally assert exactly one `MessageNew` in the advance test (testing-1 F6) —
  Suggestion.