# Testing Review — `jmap-states` (reviewer testing-1)

## Summary

The two new tests are well-constructed and idiomatic: both assert
*correspondence* (UserState vs `Foo/get`; equal-counter types share a state)
rather than hardcoding modseqs, and both positively assert the deliberate wire
changes (modern types present, superseded/stateless types absent). That is
exactly the right shape for this changeset and catches the regressions that
matter most.

However, coverage has three real holes, all on code paths this branch
specifically introduced or moved:

1. The **`J` prefix path is never exercised** by either test under the default
   config (compact emailids is off), so the moved `jmap_state_string_cstate`
   prefix logic — a stated goal of the refactor — has zero test coverage.
2. The **`EVENT_JMAP_STATES` gate** (the whole point of the mboxevent.c
   re-gate) is **not tested**: no test confirms a mailbox-less event (Login)
   *omits* the param. The change exists to stop a `filled_params()` assert, and
   nothing guards against regressing it.
3. The **hand-rolled `UserState/get` parser** has no negative-path coverage:
   no `properties`, no `ids:["singleton"]`, no `not_found`, no
   `invalidArguments`. These are brand-new branches.

Plus a likely-wrong `:min_version_3_9` guard on a method that only exists at
3.13+.

The global harness flip (`conversations => yes`) appears **safe** for the
existing MboxEvent tests, with one caveat about `tls_login_event` noted below.

## What I Explored

- Both new tests in full: `cassandane/tiny-tests/JMAPCore/user_state`,
  `cassandane/tiny-tests/MboxEvent/jmap_states`.
- Harnesses: `Cassandane/Cyrus/MboxEvent.pm` (the `new` change),
  `Cassandane/Cyrus/JMAPCore.pm` (where `user_state` is loaded;
  `conversations => yes` and `jmap_nonstandard_extensions => yes` are already
  set globally there).
- Sibling/peer MboxEvent tests (`http_trace_id`, `lmtp_trace_id`,
  `tls_login_event`) to judge whether the global config flip perturbs them.
- `Cassandane/Tiny/Loader.pm` (auto-includes every file under
  `tiny-tests/<Moniker>/`, so both new files are picked up with no registration).
- The changed C: `jmap_core.c` `jmap_userstate_get` parser, `mboxevent.c`
  gate + `add_jmap_state_cb`, `jmap_util.c` `_cstate`/`_prefixed` helpers.
- `USER_COMPACT_EMAILIDS` (`imap/user.h:15`) — NULL-safe, and gated on
  `cstate->compact_emailids`, which is **off by default**
  (`conversations.c:3602`, set only via the explicit enable path). Current tree
  version is 3.13.6 (`tools/git-version.sh`); newest version markers in the
  suite are 3.12/3.13.

## Findings

### F1 — J-prefix path has no coverage (the refactor's headline behavior is untested)
- **Severity:** Medium
- **Location:** both tests; root cause is default config in
  `MboxEvent.pm` / `JMAPCore.pm` (no compact-emailids enable).
- **Issue:** The whole reason `jmap_state_string_cstate` was extracted and
  shared is so the event and `UserState` carry the `J` prefix for Email/Mailbox
  under compact ids. But `compact_emailids` defaults off, so in both tests the
  prefix branch is never taken. In `user_state` the prefix assertions are
  literally inside `if ($userstate->{Email} =~ m/^J/)`, which is false, so the
  whole block is skipped. In `jmap_states` the regex `qr/^J?\d+$/` matches the
  no-J case and never demands the J. A regression that dropped the prefix (or
  applied it to Calendar) would pass both tests green.
- **Why it matters:** This is the single behavioral change most likely to bite
  a real client, and it is the part of the diff with the least machinery shared
  with already-tested code. It is currently asserting nothing.
- **Suggestion:** Add a case that enables compact emailids (find the existing
  knob — search Cassandane for how other JMAP tests turn on compact/reverse
  unique ids, e.g. an `enable_compactids`/JMAP-set path or an admin command),
  then assert positively that `Email`/`Mailbox` start with `J` and `Calendar`
  does *not*. Cheapest form: one extra `:needs`/setup variant in `user_state`,
  or a sibling tiny-test `user_state_compact`. This is the highest-value
  missing test.

### F2 — No test that a mailbox-less event OMITS `vnd.fastmail.jmapStates`
- **Severity:** Medium
- **Location:** `mboxevent.c` gate at the `EVENT_JMAP_STATES` case;
  no Cassandane coverage.
- **Issue:** The gate was added specifically so a mailbox-less event (Login)
  doesn't get the param expected-but-unfilled and trip `filled_params()` in
  debug builds. The `MboxEvent/jmap_states` test only inspects `MessageNew`,
  which always carries a mailbox — it exercises the *fill* side, never the
  *omit* side. So the gate itself is untested; a regression that widened or
  removed the mask would only fail in a debug-assert build, not in CI's
  behavioral assertions.
- **Why it matters:** This is the exact defect the change was written to
  prevent. Without a test pinning it, the gate can silently regress.
- **Suggestion:** In `tls_login_event` (already present, already runs with the
  param enabled globally now), or a new tiny-test, assert that the `Login`
  event's decoded JSON has **no** `vnd.fastmail.jmapStates` key
  (`assert_null($message->{'vnd.fastmail.jmapStates'})`). That directly locks
  the gate. Note `tls_login_event` is `:TLS` — if TLS isn't always available in
  CI, prefer a plain (non-TLS) login event test for the omit assertion so it
  always runs.

### F3 — `UserState/get` parser negative paths untested
- **Severity:** Medium
- **Location:** `jmap_core.c` `jmap_userstate_get` (the hand-rolled
  `json_object_foreach` parser).
- **Issue:** This parser is new, hand-rolled, and explicitly *not* reusing
  `jmap_get_parse`. None of its branches are covered:
  - `ids:["singleton"]` → returns the singleton (the happy `ids` path);
  - `ids:["bogus"]` → must land in `not_found`, not error;
  - `ids: 7` (non-array) → `invalidArguments` on `ids`;
  - `properties:["Email"]` → only that key present in the singleton;
  - `properties:["Contact"]` or `properties:["nope"]` →
    `invalidArguments` with the right `arguments` shape (the per-index
    `properties/N` push/pop path);
  - an unknown top-level arg (e.g. `foo`) → `invalidArguments`.
  The existing `user_state` test only calls `UserState/get` with `{}`.
- **Why it matters:** These are the branches most likely to have a subtle bug
  (index labeling, `not_found` vs error, prop-filter correctness, key lifetime
  in `get.props`). Right now a wrong `invalidArguments` shape, or `properties`
  silently ignored, would pass.
- **Suggestion:** Extend `user_state` (or add `user_state_args`) with a small
  table of `(args -> expected)` cases driven by one helper: happy singleton via
  `ids`, `not_found` for a bad id, `invalidArguments` for bad `ids`, bad
  `properties` element, and unknown arg; plus a `properties` filter case
  asserting only-requested keys appear. This matches the project's
  named-helper/table test idiom (per CLAUDE.md).

### F4 — `:min_version_3_9` is too low for a method introduced at 3.13
- **Severity:** Medium
- **Location:** `cassandane/tiny-tests/JMAPCore/user_state:10`.
- **Issue:** `UserState/get` is brand-new on this branch (tree is 3.13.6).
  `:min_version_3_9` tells Cassandane to *run* this test against any server
  >= 3.9. Against a real 3.9–3.12 server the method won't exist, so
  `UserState/get` returns an error/`unknownMethod`, `$by_tag{state}{list}[0]`
  is undef, and `assert_str_equals('singleton', undef)` fails — a false
  failure, not a skip. (`MboxEvent/jmap_states` has *no* version guard at all;
  same exposure for the param/types, though that param is older.)
- **Why it matters:** Version guards exist precisely so tests skip on servers
  lacking the feature. A guard set below the feature's introduction defeats
  that and produces spurious failures in cross-version runs.
- **Suggestion:** Bump to the series this actually ships in (`:min_version_3_13`
  or `_3_14`, matching how the team tags other new-feature tests). Confirm the
  intended introduction series with the author. Consider whether
  `MboxEvent/jmap_states` also needs a guard for the *dropped-types / J-prefix*
  behavior even though the param predates it.

### F5 — `_state_string` arg `int prefixed_state` vs JSON boolean coupling (test-observability nit)
- **Severity:** Low
- **Location:** `jmap_util.c` `jmap_state_string_cstate`; tests.
- **Issue:** The only thing distinguishing `J`-prefixed from not is the
  `mbtype == MBTYPE_EMAIL && compact` predicate. Because no test ever sets
  compact on, there is no test that `EmailSubmission` / `Note` / `SieveScript`
  (also email-family in some groupings?) do *not* get the prefix — i.e. that
  the predicate is `mbtype == MBTYPE_EMAIL` exactly and not "any mail-ish
  type". The `jmap_states` test asserts `Email == Mailbox` but Mailbox is
  `MBTYPE_EMAIL` too, so that doesn't isolate the predicate.
- **Why it matters:** Low, but with F1's compact-on case added, it's almost
  free to also assert `EmailSubmission` carries no `J` even when Email does,
  pinning the predicate to MBTYPE_EMAIL.
- **Suggestion:** Fold into the F1 compact-on case: assert exactly Email and
  Mailbox carry `J`, everything else does not.

### F6 — Modseq-advance assertion: low flakiness risk, but document the ordering assumption
- **Severity:** Low (informational)
- **Location:** `MboxEvent/jmap_states` second-delivery block.
- **Issue:** The test delivers a second message and asserts
  `strip(states2.Email) > strip(states.Email)`. This is a strict
  greater-than on the *Email/mail* counter, which a message delivery always
  bumps, so it's sound. The `getnotify()`/`getnotify->@*` pairing and the
  `grep MessageNew` (taking the first match via list-assign `my ($new) =`)
  correctly isolate the delivery event. No real ordering hazard here because
  the helper drains notifications immediately before each delivery.
- **Why it matters:** Minor — but if delivery ever emits more than one
  MessageNew (e.g. Sieve fan-out later added to this suite), `my ($new) =
  grep ...` silently takes the first. Fine today.
- **Suggestion:** No change required. Optionally assert exactly one MessageNew
  (as `lmtp_trace_id` does with `assert_num_equals(1, ...)`) to fail loudly if
  that assumption breaks.

### F7 — Global harness flip is safe, with one caveat
- **Severity:** Low (assessment)
- **Location:** `MboxEvent.pm::new`.
- **Issue/assessment:** Setting `conversations => yes` and adding the param
  globally affects the whole MboxEvent suite, not just the new test. I read the
  other three tests: `http_trace_id`, `lmtp_trace_id`, and `tls_login_event`.
  - `http_trace_id` / `lmtp_trace_id` only assert `vnd.fastmail.traceId` on
    every event; adding another param key is additive and doesn't break them.
  - Enabling conversations is generally a superset of behavior and shouldn't
    change trace-id propagation.
  - **Caveat:** `tls_login_event` asserts `Login => 1` and *comments out* a
    stricter `assert_deep_equals({ Login => 1 })`. With the param now enabled,
    Login events flow through the new `EVENT_JMAP_STATES` gate. If the gate were
    ever wrong for Login, this test would *not* catch it (it only counts Login
    events, doesn't inspect params). So the flip is safe, but it also means the
    suite-wide enablement buys us nothing for the gate unless we add F2's
    explicit omit-assertion.
- **Suggestion:** Acceptable as-is. Pair with F2 so the now-enabled param is
  actually asserted-absent on Login. Confirm the suite still passes for the
  three pre-existing tests under conversations=yes (a quick `dar test
  MboxEvent` run).

## What's Working Well

- **Correspondence-based assertions** instead of hardcoded modseqs: `user_state`
  fetches each `Foo/get` state and requires UserState to match. This is robust
  to counter churn and is the correct way to pin "UserState == the per-type API
  state."
- **Positive *and* negative type assertions:** both tests assert modern types
  present *and* superseded/stateless types absent (Contact, ContactGroup,
  Quota, Racl) — directly locking the deliberate wire change. Good.
- **Shared-counter equalities** (`Email==Mailbox`, `Calendar==CalendarEvent`,
  `AddressBook==ContactCard`, `SieveScript==VacationResponse`) pin the
  offset-table wiring without caring about values. Elegant.
- **`_deliver_and_states` helper** matches the project's named-helper idiom:
  call sites read as declarations of the case.
- **The `singleton` id check** and the empty-`ids` workaround comment
  (Email/get refusing null ids) show care about the real API contract.

## Clarifying Questions

1. What release series does `UserState/get` ship in? (Drives F4's guard.) Is
   3.13/3.14 correct?
2. Is there an existing Cassandane idiom to enable compact emailids in a test
   (an admin/JMAP `enable_compactids` path)? If so I'd recommend wiring F1/F5
   through it rather than inventing setup.

## Questions for Other Reviewers

- **@security-1 / @principal:** the offset read
  `*(modseq_t *)((off_t)&counters + dtype->modseq_offset)` in both
  `add_jmap_state_cb` and `add_userstate_cb` — if you conclude the `< 0` guard
  is the only thing standing between us and an OOB read, that strengthens the
  case for a test that drives a type with `modseq_offset == -1` through the
  iterator (Quota/Racl). The `jmap_states` test asserts those are *absent*,
  which is the observable proxy for the guard firing — is that sufficient, or
  do you want a more direct unit-level assertion?
- **@principal:** is the `MboxEvent/jmap_states` lack of any `:min_version`
  guard acceptable given the param predates this branch but the
  dropped-types/J-prefix semantics do not?