Merge-safe writes refuse assignee ids the API really sends

SPEC.md

@@ -1700,12 +1700,28 @@ under-counted once already:
   do NOT show it: all three now decode a nested person's id as the
   `FlexibleInt64` it is, so `recordings.summarize()` gives `7` in every port.
 
-The merge-safe composites refuse such a body before writing, so on the write
-path it is the refusing direction — no id is invented and no partial update is
-sent. On a plain read it is a string where the type says number. Both are
-decoder coverage, field by field against the reference, rather than normalizer
-reach, and are tracked in
-[PR #913](https://github.com/basecamp/basecamp-sdk/pull/913).
+The gap shows on a merge-safe write as well as on a plain read. Both are decoder
+coverage, field by field against the reference, rather than normalizer reach,
+and they are closed to different extents:
+
+- **The write path is closed, with one residual in TypeScript.** The
+  merge-safe composites' id-list guard reads a person id itself, by the same
+  `ParseInt` scan the normalizer uses, at exactly the three fields it writes
+  back — `assignees`, `completion_subscribers` and a schedule entry's
+  `participants` ([PR #913](https://github.com/basecamp/basecamp-sdk/pull/913)).
+  It also answers the two shapes no walk reaches: an absent `id` is `0`, and a
+  `null` element is the zero person, as in the reference. An explicit
+  `"id": null` still fails the read. Python and Ruby match the reference on
+  every measured row. TypeScript does not on the JavaScript number boundary,
+  in both directions: an id past 2^53 is REFUSED where the reference reads it,
+  rather than rounded into a different person's id; and a JSON number spelled
+  `1024.0` or `1e3` is ACCEPTED where the reference refuses it, because
+  `JSON.parse` has made it an integer before any code can look.
+- **The plain read is still open.** A generated service read in Ruby, Python
+  and TypeScript still returns an untagged person's string id as the string,
+  where the reference's decoder gives the number. (Ruby's generated
+  `Types::Person` does read the id flexibly; the service hashes do not.)
+  Closing it needs a decoder at those reads, not a wider walk.
 
 **The oracle, not the documentation.** Every port that reasoned from
 `ParseInt`'s docs rather than probing it got something wrong. The corpus and

python/src/basecamp/_person_id.py

@@ -2,12 +2,13 @@
 
 BC3 serializes person ids as JSON strings in some responses and as JSON numbers
 in others, and it spells the system actors -- ``LocalPerson``, whose id is
-``"basecamp"`` or ``"campfire"`` -- in the same string field. Two places in this
-SDK have to turn that string into a number: the pre-decode normalizer that every
-response body walks (``generated/services/_base.py`` and its async twin) and the
-flexible id reader in the recording-summary decode
-(``services/_campfire_index.py``). The rule lives HERE so those two cannot drift
-apart; they had two copies of it, and the copies disagreed.
+``"basecamp"`` or ``"campfire"`` -- in the same string field. Three places in
+this SDK have to turn that string into a number: the pre-decode normalizer that
+every response body walks (``generated/services/_base.py`` and its async twin),
+the flexible id reader in the recording-summary decode
+(``services/_campfire_index.py``), and the merge-safe composites' id-list guard
+(``services/_merge_safe.py``). The rule lives HERE so they cannot drift apart;
+the first two had two copies of it, and the copies disagreed.
 
 So does the WALK that finds those objects (:func:`normalize_person_ids`), for the
 same reason: the sync and async base services had a copy each, and which people a
@@ -137,10 +138,11 @@ def coerce_person_id(obj: MutableMapping[str, Any]) -> None:
     * a range error leaves the string UNTOUCHED (``:63``) so the reader refuses
       it. The readers that do refuse it are the ones with a Go struct behind
       them: ``services/_campfire_index._decoded_flexible_int64`` raises
-      "overflows int64", and ``services/_merge_safe`` refuses a non-int id
-      outright. Coercing it here instead -- which ``int()`` was doing -- hands
-      back a Python arbitrary-precision int for a value that does not fit the
-      wire's ``int64``, and every reader downstream then believes it.
+      "overflows int64", and ``services/_merge_safe`` refuses a RANGE
+      refusal as not a person id. Coercing it here instead -- which ``int()``
+      was doing -- hands back a Python arbitrary-precision int for a value that
+      does not fit the wire's ``int64``, and every reader downstream then
+      believes it.
 
     An object whose ``id`` is already a number, or absent, is left alone -- which
     is also what makes this IDEMPOTENT, and :func:`normalize_person_ids` depends
@@ -192,10 +194,11 @@ def normalize_person_ids(obj: Any, *, embedded_people: bool = False) -> None:
     That missing decoder is also why this pass does NOT close every string person
     id off the wire, and does not try to -- see "WHERE PASS 2 RUNS" below. Off
     Go's two surfaces a string id in ``creator``, ``assignees`` or a schedule's
-    ``participants`` stays a string, which ``services/_merge_safe``'s
-    ``writable_id_list`` refuses: ``schedules.edit_entry`` refuses a body the
-    reference accepts. That is decoder coverage rather than normalizer reach, and
-    it is tracked in PR #913.
+    ``participants`` stays a string. That is decoder coverage rather than
+    normalizer reach, and it is closed at the reader rather than here:
+    ``services/_merge_safe``'s ``writable_id_list`` reads a string person id
+    through :func:`parse_int64` itself (PR #913), where it used to require an
+    ``int`` and made ``schedules.edit_entry`` refuse a body the reference accepts.
 
     ONE walk here where Go runs two, and the two are equivalent because
     :func:`coerce_person_id` is idempotent and both passes apply it unchanged.
@@ -224,12 +227,14 @@ def normalize_person_ids(obj: Any, *, embedded_people: bool = False) -> None:
     for a body the reference refuses outright. Accepting direction, identity
     field, which is the class this work exists to remove.
 
-    The cost of narrowing is real and is stated rather than hidden: the
-    ``schedules.edit_entry`` write path described above refuses string
-    ``participants`` ids again, because schedules is not one of Go's two
-    surfaces and Python has no decoder standing in for ``FlexibleInt64``
-    there. That is decoder coverage, not normalizer reach, and PR #913
-    (card 42) owns it.
+    The cost of narrowing is real and is stated rather than hidden: schedules is
+    not one of Go's two surfaces and Python has no decoder standing in for
+    ``FlexibleInt64`` there, so string ``participants`` ids are no longer
+    converted on the way in. That is decoder coverage, not normalizer reach.
+    The ``schedules.edit_entry`` WRITE path is closed at the reader instead --
+    ``services/_merge_safe`` reads the id by :func:`parse_int64` (PR #913) --
+    while a plain generated read still hands the string back (SPEC.md section 10,
+    "Person Ids Off the Wire").
     """
     if isinstance(obj, list):
         for item in obj:

python/src/basecamp/services/_merge_safe.py

@@ -35,9 +35,15 @@
 
 from typing import Any
 
+from basecamp._person_id import Refusal, parse_int64
 from basecamp._security import truncate as _truncate
 from basecamp.errors import ApiError
 
+# The wire's person id is an `int64`. Named here rather than imported, because
+# `basecamp._person_id` keeps its own copies private to its scan.
+_INT64_MIN = -(2**63)
+_INT64_MAX = 2**63 - 1
+
 _RESEND_HINT = (
     "The merge-safe update/edit resend this field verbatim, so a coerced or empty value "
     "would overwrite the current one. Use {escape} to write the record deliberately."
@@ -192,20 +198,99 @@ def writable_boolean(body: dict[str, Any], key: str, *, record: str, escape: str
     return required_writable_boolean(body, key, record=record, escape=escape)
 
 
+def _person_id_from_wire(value: object) -> int | None:
+    """A PERSON's id as the reference reads one, or ``None`` when it could not.
+
+    A person id is the one field in the generated model the reference decodes
+    *flexibly* (``types.FlexibleInt64``, reached through ``Person.Id``), so it
+    does not follow the rule the other id fields do:
+
+    * an ``int`` in int64 range is itself — and one OUTSIDE that range is a
+      decode failure there, so it is a refusal here;
+    * ``bool`` is not an id, even though it is an ``int`` in Python;
+    * a STRING is what ``ParseInt`` makes of it, sign included;
+    * a string that refuses on SYNTAX is ``0`` and *not an error*, because that
+      is the sentinel BC3 serves for system-generated entities (``"basecamp"``,
+      ``"campfire"``) and the reference reads it as the system actor;
+    * a string that refuses on RANGE fails the read;
+    * anything else — a float, a list, a mapping, ``None`` — is a decode
+      failure.
+
+    The grammar is :func:`basecamp._person_id.parse_int64`, the same scan the
+    pre-decode normalizer walks with, so the walk and this reader cannot drift:
+    a person the walk reached and a person it did not must land on the same id,
+    and until this called the shared scan they did not.
+
+    ``None`` is a refusal and an ABSENT key is not, which is the one place the
+    two part company: ``encoding/json`` calls the flexible decoder's own
+    ``UnmarshalJSON`` for a JSON null, its number path leaves the buffer empty,
+    and ``ParseInt("")`` fails — so ``{"id": null}`` fails the read while a
+    missing ``"id"`` is the zero value with no error at all. The caller keeps
+    them apart; an absent key never reaches this function.
+
+    The message belongs to the field rather than to this reader, so a refusal is
+    reported as ``None`` and the caller raises.
+    """
+    if isinstance(value, bool):
+        return None
+    if isinstance(value, int):
+        # A direct range check, not `parse_int64(str(value))`. Routing it
+        # through the scan would avoid naming the bounds twice, but `str` of an
+        # `int` past 4300 digits raises a raw ValueError under Python's
+        # default int-string limit — and, where a host has lifted that limit,
+        # is quadratic. Python's arbitrary-precision `int` is the reason a check
+        # is needed at all: the reference's decoder refuses a JSON number past
+        # int64, and without this it would sail through into the PUT.
+        return value if _INT64_MIN <= value <= _INT64_MAX else None
+    if isinstance(value, str):
+        parsed = parse_int64(value)
+        if parsed is Refusal.SYNTAX:
+            return 0
+        if parsed is Refusal.RANGE:
+            return None
+        return parsed
+    return None
+
+
 def writable_id_list(body: dict[str, Any], key: str, *, record: str, escape: str) -> list[int]:
     """Read a list of person records and project it to their integer IDs.
 
     The analogue of :func:`writable_string` for the ID-list fields. The list
     comprehension it replaces (``[p["id"] for p in body.get(key) or []]``) has
     three ways to go wrong on malformed data: a non-list iterates as something
     else (a string yields characters, a dict yields its keys), a non-mapping
-    element raises ``TypeError``, and a non-integer ``id`` rides through
+    element raises ``TypeError``, and a wrong-typed ``id`` rides through
     verbatim into the full-replace PUT — the same corruption as a wrong-typed
     string, one level down.
 
-    ``bool`` is excluded explicitly: it subclasses ``int`` in Python, so
-    ``isinstance(True, int)`` is true and ``True`` would otherwise pass as a
-    person ID.
+    What it must NOT do is refuse an id the reference accepts, which is the
+    other half of the same defect and the one this guard had (#913). The
+    reference reads each element through the generated ``Person``, whose ``Id``
+    is the flexible decoder, and ``fieldsFromTodo`` appends what that produced
+    with no filter of any kind — 0 included. So a string id, an absent id and a
+    null ELEMENT are all values the reference writes back, and refusing them
+    fails a call the reference completes.
+
+    It must not lean on the pre-decode normalizer having reached the person
+    first. Which people that walk finds is the walk's rule — the reference's own
+    positional pass covers ``creator`` and ``participants`` and nothing else —
+    and a person it did not find arrives here exactly as BC3 sent it. How often
+    that is a string the fixtures cannot say: none of the person objects in
+    ``spec/fixtures`` carries a string id. What they do show is that the marker
+    the ``personable_type`` pass keys on is often missing — 3 of 7 ``assignees``
+    people omit it.
+
+    Per-element shapes and where each comes from:
+
+    * a ``None`` element is ``0``. A JSON ``null`` in the array decodes to the
+      reference's zero ``Person``, whose ``Id`` is 0 and is appended like any
+      other.
+    * a non-mapping element is a malformed response. The reference cannot
+      unmarshal a number, string or boolean into a ``Person`` and fails the
+      whole decode.
+    * an ABSENT ``"id"`` is ``0`` (the zero value, no error), while a ``None``
+      one is malformed — see :func:`_person_id_from_wire` for why those two
+      differ here and nowhere else.
     """
     value = body.get(key)
     if value is None:
@@ -217,20 +302,21 @@ def writable_id_list(body: dict[str, Any], key: str, *, record: str, escape: str
         )
     ids: list[int] = []
     for index, element in enumerate(value):
+        if element is None:
+            ids.append(0)
+            continue
         if not isinstance(element, dict):
             raise malformed(
                 f"{record} field {key!r}[{index}] is not an object: {describe(element)}",
                 _RESEND_HINT.format(escape=escape),
             )
-        person_id = element.get("id")
+        if "id" not in element:
+            ids.append(0)
+            continue
+        person_id = _person_id_from_wire(element["id"])
         if person_id is None:
             raise malformed(
-                f"{record} field {key!r}[{index}] has no 'id'",
-                _RESEND_HINT.format(escape=escape),
-            )
-        if isinstance(person_id, bool) or not isinstance(person_id, int):
-            raise malformed(
-                f"{record} field {key!r}[{index}].id is not an integer: {describe(person_id)}",
+                f"{record} field {key!r}[{index}].id is not a person id: {describe(element['id'])}",
                 _RESEND_HINT.format(escape=escape),
             )
         ids.append(person_id)