# Code Review: jmap-states (vs master)

**Date**: 2026-06-27
**Reviewers**: @principal-1, @principal-2, @quality-1, @quality-2, @security-1, @testing-1
**Mode**: Full (with discourse + Tech Lead arbitration)
**Scope**: 4 commits, 10 files, +379 / −59 — consolidate JMAP state-string production
onto the canonical `jmap_data_types` gperf table, rebuild `vnd.fastmail.jmapStates`
from it, and add a new `UserState/get` JMAP method.

---

## Verdict

**REQUEST CHANGES** — The refactor's core architecture is right and well-executed,
but two correctness issues were confirmed against source: (1) the new `UserState/get`
contract ("the very same state string that type's own `Foo/get` returns") is **false
for Note, SieveScript, and VacationResponse**, and its test passes only on a fresh
account; (2) the re-gated `EVENT_JMAP_STATES` mask still includes `SUBS_EVENTS`, which
never fill the param, so `assert(filled_params())` **fires in debug builds** — the very
failure class the change was written to prevent.

Note: the most attention-grabbing single finding raised in review — principal-1's
"calendar events stop carrying jmapStates" (High) — was **refuted** during arbitration
(see Consensus & Dissent). The two blockers below are the findings that survived
verification.

---

## Blockers

### 🚫 1. `UserState/get` does not match `Foo/get` for Note, SieveScript, VacationResponse
**Flagged by**: @principal-2 (independently verified by Tech Lead against source)
**Type**: Correctness / API-contract violation + misleading test
**Location**: `imap/jmap_core.c` `add_userstate_cb`; `cassandane/tiny-tests/JMAPCore/user_state`; contract stated in the `jmap_core.c` docstring. Counter-evidence: `imap/jmap_notes.c:444`, `imap/jmap_sieve.c:290`, `imap/jmap_vacation.c:291`.

**Issue**: `UserState/get` (and the event) compute every type's state from
`req->counters.<type>modseq`. But three live `Foo/get` methods derive `state` from a
**different** modseq:
- `Note/get` → `mbox->i.highestmodseq` of the notes mailbox (not `notesmodseq`).
- `SieveScript/get` → `mailbox->i.highestmodseq` of the sieve mailbox (not `sievemodseq`).
- `VacationResponse/get` → `sdata ? sdata->modseq : 0` — the vacation *resource* modseq (not `sievemodseq`).

UserState reports `SieveScript` and `VacationResponse` as the **same** value (both
`sievemodseq`), while their `Foo/get`s return **different** values — so UserState
cannot equal both. The `user_state` test asserts equality for all ten types and passes
only because on a freshly-created account each of these divergent counters is `0`.

**Why this blocks**: A brand-new public-ish API ships a stated contract that is untrue
for 3 of 10 types, guarded by a test that gives false assurance. A client trusting
`UserState.Note` (or `.VacationResponse`) as a poll token for the matching `Foo/get`
will desync — the subtle, hard-to-reproduce JMAP state bug this branch is otherwise
trying to eliminate.

**Suggested fix** (author's choice):
1. **Preferred** — reconcile those three `Foo/get`s to the per-user counter
   (`jmap_modseq(req, mbtype, …)`), making the unification real. (Caveat: that makes
   SieveScript and VacationResponse share a state, which may be a product question.)
2. Otherwise — scope the docstring/comment honestly (UserState reports the *per-user
   type counter*, which differs from the resource-level `Foo/get` state for these
   types), and weaken the test to assert non-null + `J?\d+` shape for the three
   divergent types. Add a **mutating** case (create a sieve script / set a vacation
   response / add a note, then re-assert) so the gap is surfaced, not hidden.

### 🚫 2. `EVENT_JMAP_STATES` mask includes `SUBS_EVENTS`, which never fill the param → debug-build assert
**Flagged by**: @principal-2 (independently verified by Tech Lead against source); coverage gap corroborated by @testing-1 (F2)
**Type**: Correctness — debug-build abort / incomplete fix of the cited assert
**Location**: `imap/mboxevent.c:582-588` (gate) vs `:1817` (fill); non-filling path `imap/mboxlist.c:5470-5477`; real `filled_params` at `imap/mboxevent.c:2051`; assert at `imap/mboxevent.c:781`.

**Issue**: The fill and the expectation share the same `mboxevent_expected_param`
predicate, so an "unexpected param" can never occur — good. The hazard is the opposite:
`SUBS_EVENTS` (`EVENT_MAILBOX_SUBSCRIBE`/`UNSUBSCRIBE`) are notified via
`mboxevent_set_access` + `mboxevent_notify` and **never call
`mboxevent_extract_mailbox`**, so the param is *expected but never filled*. Under
`NDEBUG`, `filled_params` is a stub returning `1` (no effect). In **debug builds** the
real `filled_params` (line 2051) appends `EVENT_JMAP_STATES` to its `missing` list via
the `default` case (it is not on the `EVENT_MODSEQ`-style exemption list) and returns
`0`, so `assert(filled_params())` **aborts**. The branch globally enables this param for
the entire Cassandane MboxEvent suite (`MboxEvent.pm::new`), making the abort reachable
for any subscribe-emitting test or any assert-enabled deployment that subscribes with
the param on. The branch's own comment cites this exact assert as the reason for the
gate — the gate excluded mailbox-less `Login` but missed mailbox-*named*-but-counter-less
`SUBS_EVENTS` (and likely remote `MAILBOX_DELETE` paths).

**Why this blocks**: It re-introduces, in a different event class, the precise
debug-abort the change set out to fix.

**Suggested fix** (either):
- Drop `SUBS_EVENTS` from the `EVENT_JMAP_STATES` mask and audit the `MAILBOX_EVENTS`
  members that do not always fill (remote `MAILBOX_DELETE`); **or**
- Add `EVENT_JMAP_STATES` to `filled_params`' conditional-exemption switch, exactly as
  `EVENT_MODSEQ` is handled (the least-brittle option — it survives future mask edits).

Pair the fix with a test (see Should Fix #2) asserting a `MailboxSubscribe` event omits
the param and triggers no assert.

---

## Should Fix

### 1. Pointer arithmetic laundered through signed `off_t`
**Flagged by**: @security-1, @principal-1, @principal-2, @quality-1, @quality-2 (5 of 6 reviewers)
**Location**: `imap/jmap_core.c:308-309` (`add_userstate_cb`) and `imap/mboxevent.c` (`add_jmap_state_cb`)

`*(modseq_t *)((off_t) &counters + dtype->modseq_offset)` casts a pointer to `off_t`
(a **signed** type) and back. The idiomatic, fully-defined form is `char *` byte
arithmetic: `*(const modseq_t *)((const char *)&counters + dtype->modseq_offset)`. It
works on all supported LP64 builds (`configure.ac` doesn't force `AC_SYS_LARGEFILE`, but
every supported target has 8-byte `off_t`), and the `modseq_offset < 0` guard is
unbypassable — so this is **not a live or exploitable bug**, but it is a regression from
the prior `(size_t)`, UB-adjacent, and now appears in two places. Cheap to fix while the
code is open, especially in a CVE-sensitive codebase.

### 2. Add test coverage for the paths this branch introduces
**Flagged by**: @testing-1 (F1, F2, F3), corroborated by @principal-1 and @principal-2
**Location**: `cassandane/tiny-tests/JMAPCore/user_state`, `cassandane/tiny-tests/MboxEvent/jmap_states`

Three new behaviors are currently unasserted:
- **`J` prefix** — compact emailids is off by default, so neither test ever exercises
  the prefix branch (the `user_state` prefix asserts sit inside `if (… =~ /^J/)`, which
  is false; `jmap_states` uses `J?\d+` which matches the no-J case). Add a compact-ids
  case asserting Email/Mailbox carry `J` and Calendar/EmailSubmission do **not** (this
  also isolates the predicate to `MBTYPE_EMAIL`, @testing-1 F5).
- **Gate omit** — no test confirms a mailbox-less/non-filling event omits the param.
  A plain (non-`:TLS`) Login test asserting `assert_null($msg->{'vnd.fastmail.jmapStates'})`,
  plus a `MailboxSubscribe` case, would pin both the Login fix and Blocker #2.
- **`UserState/get` parser negative paths** — `ids:["singleton"]`, `ids:["bogus"]`→not_found,
  non-array `ids`→invalidArguments, `properties` filter, invalid/`Contact` property→
  invalidArguments, unknown top-level arg. The existing test only calls `{}`.

### 3. `:min_version_3_9` is too low for a method introduced at 3.13
**Flagged by**: @testing-1 (F4)
**Location**: `cassandane/tiny-tests/JMAPCore/user_state:10`

Against a real 3.9–3.12 server `UserState/get` won't exist, so the test produces a
spurious failure rather than skipping. Bump to the series this ships in (`:min_version_3_13`
or `_3_14`) and confirm the intended introduction series. Consider whether
`MboxEvent/jmap_states` needs a guard for its dropped-types / `J`-prefix semantics.

---

## Suggestions

### Maintainability
- **Extract a shared state-string accessor.** `add_userstate_cb` and `add_jmap_state_cb`
  are near-identical (skip `< 0`, read at offset, format via `jmap_state_string_cstate`,
  store under `dtype->name`). A base-library `jmap_data_type_state_string(dtype, cstate,
  counters)` completes the branch's own "one source of truth" thesis and fixes the
  `off_t` cast in one place. — @principal-1, @quality-1, @quality-2
- **Protect the two-table-copies invariant.** The hidden-visibility-forced duplication
  (base lib `jmap_data_types.c` + http daemon `jmap_api.c`) is well-commented and
  correct, but establishes an unguarded "this iterator must stay stateless / is compiled
  into two link units" invariant. Optionally export the base copy's `lookup`/`foreach`
  and drop the daemon copy, or add a one-line note in the `.gperf` trailer. — @principal-1, @principal-2, @quality-2

### Style / Nits
- Stray trailing blank line at `imap/jmap_util.c:1549`. — @quality-2
- The `properties` invalid-arg path diverges slightly from the `jmap_blob.c` idiom;
  align if you prefer consistency. — @quality-2
- The `(void *)1` sentinel in the props hash is idiomatic but unexplained; a half-line
  note wouldn't hurt. — @quality-1

### Testing (lower priority)
- Optionally `assert_num_equals(1, …)` on the MessageNew count in the advance test, to
  fail loudly if delivery ever fans out to multiple events. — @testing-1 (F6)

---

## Consensus & Dissent

### Topic: the `EVENT_JMAP_STATES` mask (the central disagreement)
**Reviewers**: @principal-1, @principal-2, @security-1

| Reviewer | Position |
|----------|----------|
| @principal-1 | Mask is **too narrow** — drops calendar/alarm events (High) |
| @security-1 | Mask "narrows to events that carry a mailbox" — benign reduction |
| @principal-2 | Mask is **too wide** — `SUBS_EVENTS` never fill → debug assert (Medium) |

**Tech Lead arbitration (code-verified, see `discourse.md`)**:
- **principal-1 REFUTED.** Calendar mutations are emitted as `EVENT_MESSAGE_EXPUNGE` /
  `EVENT_MAILBOX_*` (in `jmap_calendar.c`, `http_dav.c`, `itip_support.c`) — all **in**
  the mask, still emitting (with corrected Calendar state). `EVENT_CALENDAR_ALARM`
  (`caldav_alarm.c:334`) **never** calls `mboxevent_extract_mailbox`, so it never filled
  the param on this branch or master. No previously-emitted data is dropped.
- **security-1 incomplete** on the same point: "carries a mailbox" ≠ "calls
  `extract_mailbox`"; `SUBS_EVENTS` carry a mailbox *name* but no counters.
- **principal-2 CONFIRMED** and **promoted to Blocker #2.**

### Topic: UserState ↔ `Foo/get` correspondence
**Reviewers**: @principal-1 ("correct by construction"), @testing-1 ("the correct way to pin it"), @principal-2 ("untrue for 3 types")
**Resolution**: principal-2 is correct (verified). True for 7 counter-derived types,
false for Note/SieveScript/VacationResponse → **Blocker #1**.

---

## What's Working Well

- **Single source of truth.** Driving the event, the API, and push notifications from
  one gperf table is the right architecture and fixes documented drift in the old
  `mboxevent` table (superseded Contact/ContactGroup, missing AddressBook, no `J`
  prefix). — @principal-1, @principal-2
- **Mechanism/policy split.** `jmap_state_string_prefixed` (mechanical) vs
  `jmap_state_string_cstate` (the `J`-prefix policy) puts the prefix decision in exactly
  one place; `jmap_state_string` delegates cleanly. — @principal-1, @principal-2
- **Memory safety verified sound.** Every dereferenced `modseq_offset` lands on a
  `modseq_t` field of a real `struct mboxname_counters`; the `< 0` guard is
  unbypassable and applied twice for `UserState/get` (defense in depth). — @security-1
- **Authorization is sound and adds no exposure.** `UserState/get` reuses the exact
  `JMAP_USERCOUNTERS_EXTENSION` + central `accountId` gate as `UserCounters/get`; every
  value is already exposed via `UserCounters/get` / `Foo/get`. — @security-1, @principal-1, @principal-2
- **Parser is careful.** Hand-rolled parser's ownership is correct against
  `jmap_get_fini`; borrowed `properties` keys outlive the table; non-string `ids` land
  in `not_found` without deref. The comment explaining why `jmap_get_parse` can't be
  reused is exactly the "why, not what" the project asks for. — @security-1, @quality-2
- **Correspondence-style tests** (fetch each `Foo/get` and require UserState to match;
  shared-counter equalities) are the right shape — they just need the fresh-account
  blind spot closed (Blocker #1). — @testing-1

---

## Clarifying Questions (please address)

### From @principal-2 / Tech Lead
- Is `UserState`/the event meant to report the **per-user type counter** or the exact
  `Foo/get` state? They differ for Note/SieveScript/VacationResponse today. Pick one and
  make code + test + docstring agree.

### From @principal-1 / @principal-2 / @security-1
- Is the `vnd.fastmail.jmapStates` wire change (Mailbox/Calendar now item-modseq;
  `Contact`/`ContactGroup`/`Quota`/`Racl` removed; `J` prefix added) coordinated with
  the actual downstream consumer(s)? "Matches the API" is the right end state, but it is
  still a vendor wire change — confirm no internal consumer depends on the old shape.

### From @testing-1
- What release series does `UserState/get` ship in (drives the `:min_version` guard)?
- Is there an existing Cassandane idiom to enable compact emailids in a test?

---

## Individual Reviews

| Reviewer | Verdict (proposed) | Headline finding | File |
|----------|--------------------|------------------|------|
| @principal-1 | NEEDS DISCUSSION | Calendar narrowing (High) — *refuted in arbitration* | `reviews/principal-1.md` |
| @principal-2 | REQUEST CHANGES | Correspondence gap (High) + mask assert (Medium) — *both confirmed* | `reviews/principal-2.md` |
| @quality-1 | APPROVE | `off_t` cast, DRY, parser-limits (Low) | `reviews/quality-1.md` |
| @quality-2 | APPROVE (minor) | DRY, `off_t`, props error path (Low) | `reviews/quality-2.md` |
| @security-1 | APPROVE | `off_t` cast (Low); authz/memory/parser sound | `reviews/security-1.md` |
| @testing-1 | REQUEST CHANGES | Coverage gaps + wrong `:min_version` (Medium) | `reviews/testing-1.md` |

**Session**: `.ocr/sessions/2026-06-27-jmap-states/`
**Discourse / arbitration**: `rounds/round-1/discourse.md`