OM-M1.4: read a preset, its grid and its scenes

CLAUDE.md

@@ -9,12 +9,13 @@ Read `docs/STEERING.md` before non-trivial work (new operations, transport or fr
 
 - Dev setup: `uv venv && uv pip install -e ".[dev]"` (or plain venv + pip, see contributing.md). Run tests with `.venv/bin/python -m pytest`. The suite passes offline - no hardware, no `hid` import, no `DYLD_LIBRARY_PATH`.
 - Two namespaces, one package (ADR-0006): `pyquadcortex` is the model of the unit, `pyquadcortex.protocol` is the message-level API. The model's code lives in `pyquadcortex/device/` - not `model/`, because in this codebase the identifier `model` means an amp or pedal block (`protocol/models.py`, `catalog.Model`, `ModelCatalog`, `set_block(model=...)`). The model imports the protocol layer; nothing under `pyquadcortex/protocol/` may import from `pyquadcortex/device/`.
-- Every conversion between a screen value and a wire value lives in `pyquadcortex/device/translate.py` and nowhere else in the package outside `pyquadcortex/protocol/` - the whole package, not just `device/`, because a rule scoped to a directory is satisfied by moving the code one directory up. It covers rows 1-4, slots 1-8, scene and footswitch letters, preset addresses and display units. Outside the boundary that means no `+1`/`-1` on a coordinate AND none of the other spellings of the same conversion (`ord`/`chr`, a letter table in any container, `divmod` on a position, a one-based `enumerate`, `ROWS.index(...)`), and no module reaching past the boundary for a protocol-layer name that carries a coordinate or a raw scale - the converters and also the readers that hand back wire indexes, such as `protocol.stomp_assignments`. `tests/test_translation.py` reads the source and proves both, and pins where each check stops seeing rather than implying it sees everything. A model API takes `FootswitchLetter`, never a bare footswitch integer, because a footswitch index and a block's column are different numbers that usually agree. A new conversion goes in that module with its own test, however small it is, and its protocol-layer name joins that file's allowlist in the same commit.
+- Every conversion between a screen value and a wire value lives in the `pyquadcortex/device/translate/` PACKAGE and nowhere else in the package outside `pyquadcortex/protocol/` - the whole package, not just `device/`, because a rule scoped to a directory is satisfied by moving the code one directory up. It covers rows 1-4, slots 1-8, scene and footswitch letters, preset addresses and display units. Outside the boundary that means no `+1`/`-1` on a coordinate AND none of the other spellings of the same conversion (`ord`/`chr`, a letter table in any container, `divmod` on a position, a one-based `enumerate`, `ROWS.index(...)`), and no module reaching past the boundary for a protocol-layer name that carries a coordinate or a raw scale - the converters and also the readers that hand back wire indexes, such as `protocol.stomp_assignments`. `tests/test_translation.py` reads the source and proves both, and pins where each check stops seeing rather than implying it sees everything. The boundary is a package, so its exemption covers a DIRECTORY: `BOUNDARY_MODULES` in that file names the modules inside it, and adding `translate/anything.py` has to come through that list with a reason - otherwise the arithmetic scan skips the new file for the same reason it skips the real converters. The same file now carries a second list, `PROTOCOL_NON_CONVERSIONS`, for protocol-layer names the boundary uses that are NOT conversions (`field_present`, the port enums). It exists because the boundary reads whole presets now, so "everything it reaches for is a conversion" stopped being true; a name has to be on one list or the other, and which one is a judgement a reviewer makes from the reason written beside it. A model API takes `FootswitchLetter`, never a bare footswitch integer, because a footswitch index and a block's column are different numbers that usually agree. A new conversion goes in that module with its own test, however small it is, and its protocol-layer name joins that file's allowlist in the same commit.
 - The model represents what the unit shows, in the unit's own words, and never guesses. A control we understand but cannot yet drive is modelled and REFUSES the operation (ADR-0007); a control we do not understand is omitted, with the reason recorded in `docs/domain-model.md`'s appendix. Nothing ships with a "this might be stale or wrong" caveat.
 - A model property that reads a device field checks the field is PRESENT (`protocol.field_present`) before reporting it. Most of this schema sits in synthetic `oneof`s, so protobuf returns `""` or `0` for a field the unit never sent, and reporting that as the answer is the guess the rule above forbids. `device/state.py` does this structurally - an absent field is simply not in the entry's copy - so a property reads through it rather than checking by hand. The exception is a field the schema gives no presence at all, where absent and default are the same bytes: those are declared in the entry's `FieldPlan.no_presence` WITH the recorded evidence for what the default means, and `tests/test_state.py` holds the declaration against the schema. Never assume; declare.
+- The cache holds a COPY of any submessage it keeps, never the container the RX thread decoded - that one is shared with every other listener and read afterwards from other threads. `entries._held` does it; `tests/test_state.py` proves it by mutating the source afterwards.
 - Never cache an incomplete reply AS IF IT WERE COMPLETE. Per field is the rule: the cache keeps what the unit actually sent and re-reads for what it did not, so a retry recovers the missing half without re-asking for the half already answered. What must never happen is a field the unit never sent being handed back as a value.
 - Model state lives in `device/state.py`, and what is tracked is a `StateEntry` in `device/entries.py` - not an attribute a property fills in itself (ADR-0011). Pushes MERGE (an absent field means "not mentioned", never "reset to default"); a read REPLACES, because it is the unit's whole answer. A message that sets any field the entry does not keep - a schema field or a field number the bindings have never heard of - marks that entry for one re-read; there is no "harmless field" category, and adding one is a guess with a table around it. A message type no entry tracks is ignored outright, which is what makes the metronome's tempo stream free.
-- A new entry decides for itself what `action` means on the types that feed it. The shared `SCAFFOLDING` skip covers it today because the two tracked types give it no meaning, and that is NOT true of `Grid`, where `action: DELETE` is what removes a block and an `UPDATE` with the same payload does nothing. Never widen `SCAFFOLDING` to make a new entry quiet.
+- A new entry decides for itself what `action` means on the types that feed it. The shared `SCAFFOLDING` skip covers the plain entries because `action` gives them no meaning, and that is NOT true of `Grid`, where `action: DELETE` is what removes a block and an `UPDATE` with the same payload does nothing. Never widen `SCAFFOLDING` to make a new entry quiet. `Grid` made its decision and it is `FieldPlan(invalidates=True)`: `action` does not matter, because every `Grid` push means the grid moved, and the entry re-reads rather than merging (ADR-0012). `invalidates` is also the answer where the per-field check is BLIND - `SceneLabel` gives `index` and `label` no presence, so renaming a scene to a blank label sets nothing at all in `ListFields()`. A push carrying every field an entry keeps clears the mark, because that is what a read returns; judge it on what the message carried, not on what its plan could carry.
 - Anything the model caches is valid only while its connection is. A closed `Device` refuses reads rather than answering from cache, because a model that reports the unit's state through an object with no unit behind it is the failure the whole layer exists to avoid. `Device.close()` closes the state layer first, so a `Device` built by `from_client` stops listening on a connection it never owned.
 - `import hid` appears exactly once, lazily, inside `session.open_device()`. Never import `hid` at module scope. A new module that needs it imports it inside the function that opens the device; `tests/test_import_cleanliness.py` walks the whole package and proves it.
 - Never gitignore or delete `pyquadcortex/protocol/proto/*_pb2.py` - the generated bindings are committed on purpose (ADR-0001, written before the proto directory was moved). Regenerate only via `scripts/compile_protos.sh`, and bump the `protobuf` pin in `pyproject.toml` in the same commit as regenerated bindings. The `grpcio-tools` floor in the dev extra is part of that same commit: `grpcio-tools` carries its own protoc, so the installed version decides the gencode, and an older one emits older gencode that still imports and quietly walks the pin backwards (ADR-0008). Both directions are now guarded - the script refuses to write a downgrade, and `tests/test_packaging.py` proves the committed gencode equals the pin floor - so trust the failure and fix the cause rather than working around either. Never read the floor off `grpcio-tools` metadata; 1.82.1 declares `protobuf>=7.35.1` and emits 7.35.0. Run the compiler and read the stamp. CI's `build` job runs `scripts/check_artifacts.py`, which proves the bindings are inside the wheel and the sdist.

docs/ADR.md

@@ -166,3 +166,39 @@ Records are append-only once `Decided` and built upon: a shipped decision is nev
   - A write the unit CONTRADICTS marks the entry too, which section 10 does not ask for. Section 10 asks for a log line, on the reasoning that a disagreement is a bug in our code rather than a stale cache. That is true of the field the unit named, whose value the echo has just put right. It is not true of the other fields in the same write: those went into the cache on our say-so, the echo did not carry them, and the write they belonged to is one the unit has just demonstrated it disagreed with. Leaving them there makes the one path that means "we have a bug" the one that cleans up after itself least, so it marks - which costs one read on a path that should never run.
   - `action` and `request_id` are skipped on every entry, and only one of them is really the transport's. `request_id` always is. `action` is not: on `Grid` it is load-bearing state, because an `UPDATE` carrying `hash: 0` is transmitted and ignored while the same payload with `action: DELETE` removes the block. It is skipped today because the two tracked message types give it no meaning, and a `Grid` entry (issue #12) therefore cannot inherit the skip - two pushes with identical payloads and opposite meanings would apply identically and mark nothing. That entry gives `action` its own decision rather than widening the shared set.
   - A field the wire gives no presence and the entry does not KEEP is undetectable rather than merely unkept: proto3 writes such a field only when it differs from its default, so a message leaving one at its default carries no bytes for it and no implementation of this check could see it. `tests/test_state.py` asserts no feeding type has one, which turns a limit of the wire into a question about our own code, checkable and checked.
+
+## ADR-0012: A grid push is noted and re-read, not merged, and the model publishes what it noticed
+
+- **Status:** Decided (2026-08-15)
+- **Decision:** A `Grid`, `SceneLabel` or `SceneColor` push makes the preset entry's copy untrusted **whatever the message carries**, and the next read fetches the whole live preset with `RecallPreset{READ}`. The model does not merge those deltas. Alongside it, the model publishes two events on `device.events` - `Changed` when a push moved a value it holds, `Invalidated` when it stopped trusting part of its copy - delivered on a thread the model owns, so a subscriber may read from the unit in response.
+- **Context:** One edit on the touchscreen produces about forty `Grid` pushes, each a sparse keyed delta into a deeply nested preset payload. ADR-0011 requires the model to notice anything a message names that it does not represent, and for a delta that means walking the payload recursively rather than reading `ListFields()` at the top level. Two of these types also defeat the per-field check outright: `Grid` carries its meaning in `action`, which the wire gives no presence, so an `UPDATE` and a `DELETE` with identical payloads are indistinguishable; `SceneLabel` gives `index` and `label` no presence either, so renaming a scene to a blank label sets nothing observable at all. Without merging, a caller who needs a fresh value promptly has no way to learn that it went stale, because re-reading only happens when somebody reads a property.
+- **Options:**
+  - **(a) Note it and re-read the whole preset, and publish what was noticed - chosen.** Forty pushes cost one read, because the note is a flag rather than a queue, and `RecallPreset{READ}` has no side effects on the grid, the active scene or the audio. A caller who cannot wait subscribes.
+  - **(b) Merge each delta by key.** Chain by row, model by column, parameter by index, plus the recursive version of ADR-0011's check to stay honest. Reads stay instant while somebody edits on the unit. Rejected for M1 because the recursive check is where all of the risk sits, and it would have landed beside the objects three other stories are blocked on. What it would take is recorded in `domain-model.md` section 9 so this stays a decision rather than an omission.
+  - **(c) Treat `action` as scaffolding on `Grid` like everywhere else.** Two pushes with identical payloads and opposite meanings would apply identically and mark nothing. This is the failure ADR-0011's consequences already name.
+  - **(d) Deliver events on the RX thread.** No new thread, and the same contract as `Transport.add_listener`. Rejected because ADR-0009 forbids reading from that thread, which makes the feature unusable for the thing it exists for: a subscriber's obvious reaction is to go and read the value.
+- **Open Questions:** Whether merging is worth doing once parameters land (#13), since a caller stepping through parameter values during an on-unit edit pays a whole-preset read per step. Measurable rather than arguable: the forced re-reads are logged with the entry that caused them. Also whether `Invalidated` firing only on the trusted-to-untrusted transition is the right shape for a subscriber that never reads - today the stream goes quiet until somebody does, which is documented but not hedged.
+- **Rationale:** The two halves answer the same question from opposite ends. Not merging means the model never has to understand a delta to stay correct, which is what makes the conservative rule affordable; publishing means the cost of not merging - a round trip on first access after an edit - is one a caller can choose to pay eagerly instead. The thread is not a convenience: it is the difference between an event a caller can act on and one that raises when they try.
+- **Consequences:**
+  - The first property read after an on-unit edit is a round trip. A merging cache would have answered instantly.
+  - A push that carries every field an entry keeps clears the mark, because that is exactly what a read returns. This is what makes the connect burst leave the cache genuinely warm: measured, it marks two entries and answers both in full a millisecond later. The check is on what the MESSAGE carried, not on what its plan could carry - a plan-level version was written first and every branch of it was unreachable.
+  - The model owns a thread whose contract is the opposite of the RX thread's: reading from the device is allowed there and expected. It is started on the first subscriber and stopped by `Device.close`, and it catches `BaseException` around a subscriber for the reason `Transport._notify_listeners` records.
+  - "A recall resets three things together" is declared once, on the entry that knows which slot is loaded, and fires only when that slot really changes. Declaring it as a plan on each dependent entry instead made the model's own read of the loaded slot look like a recall, and publish two events saying the unit had changed.
+
+## ADR-0013: The translation boundary is a package, and what it may reach for is two lists
+
+- **Status:** Decided (2026-08-15)
+- **Decision:** `pyquadcortex/device/translate.py` becomes a package, split by responsibility, with every public name re-exported so no caller changes. The structural test that proves no other module converts now exempts a DIRECTORY, so it names the package's modules and keys that list on the path within the package. The derived check that kept the protocol-conversion allowlist current is **weakened**: a protocol-layer name the boundary uses must appear on one of two lists - `PROTOCOL_CONVERSIONS`, which no other module may use, or `PROTOCOL_NON_CONVERSIONS`, which carries a written reason per entry.
+- **Context:** Reading a whole preset in screen coordinates is boundary work: the protocol helpers it needs (`blocks`, `splits`, `bypass_state`) hand back raw wire coordinates, so only the boundary may call them. Adding that to the existing module took it past 800 lines. The derived check said "whatever the boundary reaches for IS a conversion, because converting is all it does", which needed no list to maintain. That premise held while the boundary converted single values and stopped holding the moment it read messages: reading needs `field_present`, which the root `CLAUDE.md` REQUIRES every model property to call, and the `Input`/`Output` port enums, which are public model API. Under the old rule both would have been banned.
+- **Options:**
+  - **(a) Two lists, one derived check over their union - chosen.** The direction that actually rots is preserved: a new conversion the boundary starts calling cannot arrive silently, because it must be accounted for before the suite goes green.
+  - **(b) Keep the single derived rule and ban `field_present`.** Directly contradicts a root `CLAUDE.md` rule. Rejected.
+  - **(c) Put the grid readers outside the boundary and exempt them.** That is the guard-narrower-than-it-reads mistake this project keeps catching, and it would have exempted exactly the code most worth watching.
+  - **(d) Keep one file.** Rejected on size, and it would not have helped: the two-list problem is caused by what the boundary now DOES, not by where it lives.
+- **Open Questions:** Whether `PROTOCOL_NON_CONVERSIONS` should be scoped to the boundary rather than package-wide. Its only effect today is absence from the ban list, so every model module may use those names - including `Output.NEXT_ROW_3`, whose row number is precisely the inference `translate.routes_to_a_row` refuses to make on the record. Nothing in the model does this, and no test would catch it.
+- **Rationale:** A rule that contradicts another rule is not a stronger rule, it is a broken one, and the honest fix was to say what the second category is rather than to bend the first. The residual risk is named where a reviewer will meet it: a real conversion parked on the wrong list is the one failure no test can catch, so the criterion - what does the name HAND OVER? - is written beside both lists.
+- **Consequences:**
+  - Adding a module to `translate/` requires adding it to `BOUNDARY_MODULES` with a reason, in the same commit. The list is keyed on the path within the package, because a filename-keyed version let `translate/legacy/grid.py` pass as "grid" while the arithmetic scan skipped it.
+  - A reviewer now has judgement to apply where previously there was none: when a name appears, which list it belongs on.
+  - `scripts/check_artifacts.py` names the package's `__init__` AND a real converter module, because a packaging rule that took the directory but dropped its contents would ship a boundary that re-exports names it no longer has.
+