merge: lane/proto — conformance stops downloading real files

docs/adr/0018-nullable-optional-fields-absent-vs-explicit-null.md

@@ -0,0 +1,107 @@
+# ADR 0018 — Nullable optional fields: absent vs. explicit null
+
+**Status:** accepted · **Date:** 2026-09-13 · **Lane:** PROTO
+**Prompted by:** a DAEMON report against `download.update`: the generated C++ parser gives
+`VeloxDispatcher` no way to tell "the caller left this field alone" from "the caller wants
+it cleared," so `download.update` and (the moment a nullable `SettingKey` exists)
+`settings.set` can set a nullable field but never clear it back to `null`.
+
+## Context
+
+`download.update`'s `patch` object documents the convention plainly: "Only the present
+fields change. An explicit null clears a nullable field." That is a deliberate, already-
+committed wire contract — not something up for redesign here. The gap is one layer down:
+`contracts/codegen/gen_cpp.py`'s `emit_field_parse` collapses "key absent" and "key present
+with value `null`" to the same `std::nullopt`, on purpose, and the comment says so:
+
+> Absent and null mean the same thing: the field is not set. A client that omits a
+> nullable field and one that sends null are treated identically on purpose.
+
+That collapse is *correct* for the common case — most nullable-optional fields are on
+create-style params (`DownloadSpec.saveDir`, `.categoryId`, …) where there is no existing
+value to distinguish "never set" from "explicitly cleared" in the first place; either way
+the daemon just uses a default. It is wrong specifically for **patch-style** params, where
+a field can already hold a value and the caller needs to say which of two different things
+they mean: "leave it" or "clear it."
+
+The schema IR (`schema_ir.py`) already tracks `required` and `nullable` as two independent
+booleans per `Field`, so the information needed to make this distinction exists all the way
+through parsing — `emit_field_parse` just doesn't act on it. Only `download.update`'s
+`patch` object is affected today (`filename`, `saveDir`, `categoryId`, `queueId`,
+`description`, `segments`, `bufferBytes`, `checksum` — all eight of its fields are
+nullable-and-optional with exactly this "leave vs. clear" meaning). No `SettingKey` is
+nullable yet, so `settings.set` has no live instance of the bug, but the same shape
+(`values` patches an existing bag) means the first nullable settings key will hit the exact
+same gap.
+
+## Decision
+
+**A JSON-null-aware optional, opt in per field via a new `x-clearable: true` annotation —
+not a blanket rule and not a companion "clear list" field.**
+
+- New per-field schema annotation, `x-clearable: true`, valid only on a field whose type
+  already includes `null` (schema error otherwise — clearable implies nullable). Marks
+  "this field distinguishes absent from explicit null"; every other nullable-optional field
+  keeps today's collapse.
+- The generated C++ type for a `x-clearable` field becomes `std::optional<std::optional<T>>`:
+  outer `nullopt` = absent (leave unchanged), outer engaged with an inner `nullopt` =
+  explicit `null` (clear it), outer engaged with an inner value = set it. One field, three
+  states, no parallel bitset to keep in sync and no second field to forget to check.
+- `emit_field_parse` for such a field stops folding `is_null()` into "absent": absent skips
+  the assignment (outer stays `nullopt`); present-and-null assigns an engaged-but-empty
+  inner optional; present-and-valued parses normally into the inner optional. Every other
+  field's codegen (the `required`/`nullable`-but-not-`clearable` majority) is unchanged.
+- TypeScript needs no generator change: `field?: T | null` already round-trips this exactly
+  the way JSON does — an omitted key serializes as absent, `null` serializes as `null`, and
+  `"field" in obj` / `obj.field === null` already distinguish the three states natively.
+  This gap is a C++-generator-only problem.
+- Applies now to `download.update`'s eight `patch` fields. `Settings` gets no annotation
+  today (nothing nullable to mark); the day a nullable `SettingKey` is added, it gets
+  `x-clearable: true` in the same PR, not left to rediscover this ADR.
+
+## Versioning
+
+Per ADR 0015: this retypes a generated C++ field (`optional<T>` -> `optional<optional<T>>`)
+with the wire byte-for-byte unchanged — a client sending the same JSON parses correctly
+either way. **Minor bump, with a migration note** for anyone reading `patch.filename` et al.
+directly (unwrap twice: check the outer, then the inner). Not major; `session.hello`'s
+major-only check must not refuse a wire-compatible peer over a binding-only change.
+
+## Consequences
+
+- `on_download_update` (DAEMON, not this lane) can finally implement "explicit null
+  clears": read the outer optional for presence, the inner for clear-vs-value, exactly the
+  three states the schema already promised.
+- The collapse comment in `emit_field_parse` stays as the default behavior and gets a
+  pointer to this ADR for the opt-in exception, instead of being read as an oversight.
+- Implementation (schema annotation support in `schema_ir.py`, the `gen_cpp.py` emission
+  change above, regenerating `core/generated/`, the `x-clearable: true` annotations on
+  `download.update`'s eight fields, the VERSION bump and migration note) is **not** done in
+  this change — recorded here so DAEMON isn't blocked on relitigating the design, tracked as
+  its own PROTO PR per the normal contracts process (schema + regenerated code + fixtures +
+  VERSION bump together, CLAUDE.md §2).
+
+## Alternatives rejected
+
+**An explicit clear list** (e.g. `patch.clearFields: ["categoryId", …]`, plain non-nullable
+`optional<T>` fields otherwise). Rejected: the wire contract "an explicit null clears a
+nullable field" is already written into `download.update`'s schema description and is what
+DAEMON built against — this would be a real, disruptive wire redesign to route around a
+generator gap, not a fix for it. It also doesn't compose: every patch-shaped object gains a
+second array to keep in sync with the first, by hand, forever.
+
+**A parallel "which fields were present" bitset** (struct of `optional<T>` fields plus a
+sibling presence-flags struct or bitset). Rejected: two things to check per field instead
+of one, and nothing stops a caller from reading the optional and forgetting the presence
+bit — exactly the class of bug this ADR exists to close.
+
+**Apply the tri-state to every `nullable && !required` field automatically**, using the IR
+flags already present, no annotation needed. Rejected: `emit_field_parse` only backs
+`parse<T>()`, used for *params* types the daemon receives — but the conformance C++ runner
+also instantiates `parse<T>()` for **result** types (round-tripping golden fixtures), and
+plenty of those are nullable-optional with no patch semantics at all (`TaskSummary.effectiveUrl`,
+"null until the first probe succeeds" — a plain nullable value, not a leave-or-clear
+choice). Blanket application would retype those too, forcing every read site across the
+daemon that already does `if (summary.effectiveUrl)` into an unwanted double-unwrap for a
+distinction that field doesn't have. Opt-in keeps the blast radius at exactly the fields
+that need it.