# Code Quality Review — quality-1

## Summary

This is clean, idiomatic Cyrus/JMAP code that reads as RJBS would write it. The
refactor genuinely consolidates state-string generation into one place, the new
comments are all of the "explain why" variety the project wants (and are
correctly signed), and the two new Cassandane tests follow the helper-based,
declaration-style norm well. The headline concern — the `(off_t)` pointer cast —
turns out to be **consistent with an existing local idiom** (`jmap_push.c`,
`index_file.c`) and matches the `off_t`-typed `modseq_offset` field, so it is a
pre-existing portability wart the diff merely propagates, not a new smell. My
findings are mostly low-severity polish; nothing here blocks merge on quality
grounds.

Verdict: **approve**.

## What I Explored

- The full diff (`/private/tmp/claude-501/ocr-diff.txt`).
- `jmap_usercounters_get` / `usercounters_get` (`imap/jmap_core.c:240-320`) — the
  sibling method, to judge the hand-rolled parser against `jmap_get_parse`.
- `jmap_get_parse` (`imap/jmap_api.c`) — what the library parser validates that
  the hand-rolled one does not (ids element-type check, `MAX_OBJECTS_IN_GET`
  limit, creation-id `#` resolution, dedup).
- `jmap_get_fini` (`imap/jmap_api.c:1726`) and `hash_insert` (`lib/hash.c:102`) —
  to confirm `get.props` key lifetime and freeing are correct.
- `jmap_wantprop` macro (`imap/jmap_util.h:20`) — NULL-props-means-all semantics.
- `struct mboxname_counters` (`imap/mboxname.h`) and `jmap_data_type_t`
  (`imap/jmap_api.h:110`, `modseq_offset` is `off_t`) and the gperf table
  (`imap/jmap_data_types.gperf`).
- Existing `(off_t)`/`uintptr_t` offset idioms across `imap/` (`jmap_push.c:39,155`,
  `index_file.c:439,495`) to place the cast in context.
- `cassandane/tiny-tests/JMAPCore/user_counters` and the MboxEvent suite layout
  to judge test style and the `:min_version` guard convention.

## Findings

### Finding 1 — `(off_t)` pointer cast is non-portable, but consistent with existing idiom
- **Severity:** Low
- **Location:** `imap/jmap_core.c:345`, `imap/mboxevent.c:1688`
- **Issue:** `*(modseq_t *)((off_t) &srock->req->counters + dtype->modseq_offset)`
  casts a pointer through `off_t` — a *signed* integer type — for pointer
  arithmetic. The canonical C idiom is `(char *)&counters + offset` (byte
  arithmetic, no integer round-trip) or `(uintptr_t)` if an integer type is truly
  wanted. `off_t` is not guaranteed wide enough to hold a pointer; on a build
  without 64-bit `off_t` it could truncate.
- **Why it matters:** Round-tripping a pointer through a signed integer that
  isn't `intptr_t`/`uintptr_t` is implementation-defined and a classic
  portability trap. That said: Cyrus forces `_FILE_OFFSET_BITS=64`, the
  `modseq_offset` *field* is itself declared `off_t`, and this exact pattern
  already exists at `imap/jmap_push.c:39,155` and `imap/index_file.c:439,495`. So
  the diff is locally consistent and the practical risk is near zero — which is
  why this is Low, not Medium.
- **Suggestion:** Leave it for consistency with the surrounding code, OR (better,
  if you want to improve the whole family) switch all sites — including the
  `jmap_push.c` originals — to `(char *)&counters + offset`. Don't fix only the
  two new sites and leave the idiom split; that would *reduce* consistency.

### Finding 2 — `add_userstate_cb` and `add_jmap_state_cb` are near-identical
- **Severity:** Low
- **Location:** `imap/jmap_core.c:330` (`add_userstate_cb`), `imap/mboxevent.c:1680`
  (`add_jmap_state_cb`)
- **Issue:** Both callbacks do exactly: skip `modseq_offset < 0`, read the modseq
  at the offset, call `jmap_state_string_cstate`, set the result into a json
  object keyed by `dtype->name`. The only differences are the rock shape and that
  the core one also consults `jmap_wantprop`.
- **Why it matters:** Two copies of the offset-read-plus-format logic means two
  places to fix if the read idiom changes (see Finding 1). It's genuinely minor
  because the two live in different libraries (base lib vs http daemon) and a
  shared helper would need a small shared signature.
- **Suggestion:** Acceptable as-is given the library split. If you want to
  de-duplicate, a tiny exported helper
  `modseq_t jmap_counter_modseq(const struct mboxname_counters *, const jmap_data_type_t *)`
  in the base library would centralize the offset arithmetic and let both
  callbacks shrink to a `jmap_wantprop` guard plus a `json_object_set_new`. Not
  required.

### Finding 3 — hand-rolled parser is justified but skips two checks `jmap_get_parse` does
- **Severity:** Low
- **Location:** `imap/jmap_core.c:342-407` (`jmap_userstate_get`)
- **Issue:** The comment correctly explains *why* `jmap_get_parse` can't be used
  (its valid-prop set is a gperf hash for a different schema). Good. But the
  hand-rolled `ids` handling diverges from the library in two small ways:
  (a) a non-string element in `ids` is silently routed to `notFound` (because
  `json_string_value` returns NULL and the `id && ...` guard fails), whereas
  `jmap_get_parse` flags it as an invalid argument; (b) there's no
  `MAX_OBJECTS_IN_GET` limit and no dedup. For a singleton-only method these are
  cosmetic — the only valid id is `"singleton"`, dedup is moot, and a giant `ids`
  array just produces a giant `notFound`.
- **Why it matters:** Consistency of error shape across sibling methods. A client
  that sends `ids: [42]` to `UserState/get` gets `notFound: [42]` here but would
  get `invalidArguments` from a standard get. Low impact, but it's a behavioral
  inconsistency a careful client author could trip on.
- **Suggestion:** Optionally mirror `jmap_get_parse`'s element check: in the
  `ids` loop, if `json_string_value(jval)` is NULL, push an indexed invalid arg
  rather than appending to `notFound`. If you'd rather keep the method tiny, a
  one-line addition to the existing `-- claude` comment noting "non-string ids
  fall through to notFound, which is fine for a singleton" would document the
  deliberate divergence.

### Finding 4 — `(void *)1` sentinel is correct but unexplained
- **Severity:** Info / nit
- **Location:** `imap/jmap_core.c:368` (`hash_insert(name, (void *)1, get.props)`)
- **Issue:** `(void *)1` as a present-marker matches `jmap_wantprop`'s
  `!= NULL` test and is the same trick `jmap_get_parse` uses for its dedup table,
  so it's idiomatic here — no change needed. Flagging only because, read in
  isolation, the magic `1` reads as arbitrary. The surrounding code's use of the
  same pattern makes a comment unnecessary.
- **Suggestion:** None required; consistent with the codebase.

## What's Working Well

- **Comment discipline is exactly on-spec.** Every added comment explains a
  non-obvious *why* — the hidden-visibility duplication
  (`jmap_api.c`, `jmap_data_types.c`, `jmap_util.c`), why `jmap_get_parse` is
  unusable here, why the event is re-gated to mailbox-carrying events — and each
  is signed `-- claude, 2026-06-03`. No restate-the-code comments. The deleted
  `// add mandatory prefix` comment was preserved verbatim when the helper moved.
- **The refactor removes a real duplication.** `_state_string` plus its inline
  prefix decision collapsed into `jmap_state_string_prefixed` /
  `jmap_state_string_cstate`, with `jmap_state_string` delegating. The
  prefix-decision (`MBTYPE_EMAIL` && compact ids) now lives in one place, and
  `mboxevent.c` reaches the identical logic through the base-library copy — which
  is the whole point of the change.
- **Naming is consistent and descriptive.** `userstate_rock` / `jmap_states_rock`,
  `add_userstate_cb` / `add_jmap_state_cb`, `srock`/`jrock` match the existing
  `*_rock` + callback conventions (cf. `jmap_push.c`, `jmap_contact.c`'s `crock`).
- **Both tests follow the named-helper / compact-declaration norm.** The MboxEvent
  test's `_deliver_and_states($subject, $uid)` hides the getnotify/deliver/decode
  wiring so `test_jmap_states` reads as a list of assertions. The JMAPCore test's
  `%method_for` + `@calls`-build + `%by_tag` structure is a clean, data-driven
  declaration of the correspondence being asserted, with comments stating the
  invariant rather than the mechanics. The "assert correspondence, don't hardcode
  modseqs" strategy is the right call and is well-explained.
- **`jmap_data_types.c` is appropriately minimal** and its header comment fully
  justifies why a near-empty TU exists. Nothing dead, nothing stray.

## Clarifying Questions

- Finding 1: do you want the `(char *)` cleanup applied across the whole
  `modseq_offset` family (`jmap_push.c` included) in a follow-up, or is the
  established `(off_t)` idiom intentional and to be kept?
- Finding 3: is the "non-string id → notFound" behavior of `UserState/get`
  intended, or should it match `jmap_get_parse`'s `invalidArguments` shape?

## Questions for Other Reviewers

- **security:** does the `dtype->modseq_offset < 0` guard fully bound the offset
  read, given offsets are `offsetof(struct mboxname_counters, …)` and the base is
  always a real `struct mboxname_counters`? (My read: type-correct and aligned,
  no overrun — but the offset math is your domain.)
- **testing:** does flipping `conversations => yes` globally in
  `MboxEvent.pm::new` perturb any existing MboxEvent test's expected events? And
  is the absence of a `:min_version` guard on the new MboxEvent test fine (it
  matches the suite's prevailing style — 3 of 4 existing tests carry none)?
- **principal:** the dropped event keys (`Contact`/`ContactGroup`/`Quota`/`Racl`)
  and the Mailbox/Calendar item-vs-folder modseq change — is that wire change
  coordinated with downstream consumers?