From 768f4f8e53221e516abbaafb937aaa8e9b2610eb Mon Sep 17 00:00:00 2001 From: sami Date: Thu, 10 Sep 2026 15:24:53 +0400 Subject: [PATCH] =?UTF-8?q?proto:=20ADR=200014=20=E2=80=94=20versioning=20?= =?UTF-8?q?a=20generated-binding=20break=20with=20an=20unchanged=20wire?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Records the rule 1.4.0 applied, so the next generated-binding retype cites it instead of relitigating README rule 4's "retype → major + ADR" from scratch. The rule: VERSION tracks the wire protocol, not any binding's API or ABI. A change that leaves the wire byte-identical but breaks a generated binding's source API (a C++ virtual's return type, a struct name) is a minor bump plus a migration note — major would make session.hello refuse a client whose wire behaviour is unchanged, which is worse than the problem. An ADR is still required when the change encodes a design decision; "only one lane consumes it" is not a reason to skip that, since GUI already links velox::proto and the next such change starts with more than one consumer. Rule 4 in contracts/README.md now points here so its "major + ADR" line isn't read in isolation. proto-answers-daemon-m1.md's P1 writeup references it as the durable home for the reasoning that was otherwise only in a commit message. Also names the nlohmann brace-init hazard the 1.4.0 work hit: json{nullptr} is the array [null], not JSON null, so HandlerError::data is `= nullptr`. The kind of thing a regeneration reintroduces; caught here only by an end-to-end assertion on dispatch() output, which is called out to keep. Docs only — no schema, VERSION, or generated-code change. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_012fgjnqFCS5h5L7gZTZo3rV --- contracts/README.md | 4 + contracts/proto-answers-daemon-m1.md | 16 ++-- ...enerated-binding-changes-and-versioning.md | 78 +++++++++++++++++++ 3 files changed, 92 insertions(+), 6 deletions(-) create mode 100644 docs/adr/0014-generated-binding-changes-and-versioning.md diff --git a/contracts/README.md b/contracts/README.md index cec2e0f..144c744 100644 --- a/contracts/README.md +++ b/contracts/README.md @@ -64,6 +64,10 @@ subset, `fixtures/` documents the fixture shape and the placeholder rules. renaming, retyping, or changing a default → **major** bump and a written migration note in `docs/adr/`. `session.hello` rejects a major mismatch with error `-32001` and a message the GUI renders as "Velox needs updating". + "Retype → major" is about the **wire** — a field a client parses off the socket. A + change that leaves the wire byte-identical but breaks a *generated binding's* source + API (a C++ virtual's return type, a struct name) is a **minor** bump plus a migration + note — see `docs/adr/0014-generated-binding-changes-and-versioning.md`. 5. **Changes arrive as a PR to `contracts/` alone**, containing: schema edit + fixtures + regenerated code + `VERSION` bump. Lanes rebase onto it. This is the only synchronization point in the whole project — keep it cheap and frequent rather than big and rare. diff --git a/contracts/proto-answers-daemon-m1.md b/contracts/proto-answers-daemon-m1.md index 9e0b665..a4efdd2 100644 --- a/contracts/proto-answers-daemon-m1.md +++ b/contracts/proto-answers-daemon-m1.md @@ -56,12 +56,16 @@ reason.) `Result` → `HandlerResult` automatically; `conformance_main.cpp` only ever inspects `dispatch()`'s JSON output and needed no change. -**Version:** minor, 1.3.0 → 1.4.0. The wire is byte-identical — no schema, fixture, or -OpenRPC change — but every implementer of `Dispatcher` must swap `Result` → `HandlerResult` -on their `on_*` overrides or they won't compile, and a version bump is how lanes are told -to regenerate and adapt. Not major and not an ADR: one lane consumes this binding, it's -the lane that asked, and there's no contested design here — the shape is the one you -proposed. `kProtocolVersion` moves to `"1.4.0"` with it. +**Version:** minor, 1.3.0 → 1.4.0. The wire is byte-identical — no schema, fixture, +or OpenRPC change — but every implementer of `Dispatcher` must swap `Result` → +`HandlerResult` on their `on_*` overrides or they won't compile, and a version bump is +how lanes are told to regenerate and adapt. Minor, not major: a major would make +`session.hello` refuse a client whose wire behaviour is unchanged. The rule — a +generated-binding API break with an unchanged wire is minor + migration note, because +`VERSION` is the protocol version, not the C++ ABI — is written up as +`docs/adr/0014-generated-binding-changes-and-versioning.md` (this instance is +mechanical; the ADR records the rule for the next one, which GUI will also consume +since it already links `velox::proto`). `kProtocolVersion` moves to `"1.4.0"` with it. ## P2 — clarifications diff --git a/docs/adr/0014-generated-binding-changes-and-versioning.md b/docs/adr/0014-generated-binding-changes-and-versioning.md new file mode 100644 index 0000000..3351282 --- /dev/null +++ b/docs/adr/0014-generated-binding-changes-and-versioning.md @@ -0,0 +1,78 @@ +# ADR 0014 — Versioning a generated-binding break with an unchanged wire + +**Status:** accepted · **Date:** 2026-09-10 · **Lane:** PROTO +**Prompted by:** the 1.4.0 bump (DAEMON's P1 — `HandlerResult` / `HandlerError` on the +generated C++ `Dispatcher`), recorded here as a rule rather than left as a one-off. + +## Context + +`contracts/README.md` rule 4 says: optional field or new method → **minor**; rename, +remove, retype, or default change → **major** + an ADR. Read literally, the 1.4.0 change +retyped every `Dispatcher::on_*` method (`Result` → `HandlerResult`) and so should +have been major with an ADR. + +It was shipped as a minor bump with a migration note and no ADR. That call was right, but +the reasoning only lived in a commit message and a `proto-answers-*` doc. The next +generated-binding change — and there will be one; the C++ and TypeScript emitters are +young — would get argued from rule 4's literal text again, and rule 4 would read as though +PROTO quietly exempts itself whenever it's inconvenient. + +## Decision + +**`VERSION` tracks the wire protocol, not any binding's API or ABI.** Two clients at the +same `VERSION` speak the same JSON-RPC: same methods, same params and result *shapes*, same +event payloads, same error codes on the wire. That is the whole promise `session.hello` +enforces (major-only), and it is the only thing `VERSION` is allowed to mean. + +From that, the versioning rule for a change that leaves the wire byte-identical but alters +the **generated binding's source-level API** (a return type, a struct name, a signature — +anything that makes a regenerated `core/generated/` or `extension/src/shared/protocol/` +stop compiling against last version's hand-written code): + +- **Minor bump.** Not major. A major bump makes `session.hello` reject a client that is + wire-compatible — actively worse than the problem, since the wire didn't change. +- **A migration note** in the same PR: what to regenerate, and the mechanical edit each + consumer makes (e.g. "swap `Result` → `HandlerResult` on every `on_*` override"). The + `proto-answers-*` doc for the requesting lane is the normal home; `contracts/README.md`'s + version banner carries the one-line summary. +- **An ADR only when the change encodes a design decision** — a new type's shape, a new + contract, a semantic split. A purely mechanical shape change (this one: the error-channel + type DAEMON proposed, applied as sketched) does not need one; it needs the migration + note. "One lane consumes it today" is **not** a reason to skip the ADR when the change is + a design decision — GUI already links `velox::proto`, and the next such change will have + more than one consumer from day one. + +**Rule 4's "retype → major" is about the wire.** Retyping a field on `TaskError`, +`TaskSummary`, a method's params or result — anything a client parses off the socket — is +major, because a peer at the old version mis-parses it. Retyping a generated C++ virtual's +return type is not that: no byte on the wire moved, and the compiler catches every call +site at build time. The two are different failure modes and get different rules. + +## A regeneration hazard worth naming + +`nlohmann::json` brace-initialization from `nullptr` produces the **array `[null]`**, not +JSON `null`. `HandlerError`'s `data` member is written `nlohmann::json data = nullptr;` +(copy-init) for exactly this reason; a member written `data{nullptr}` compiles, passes a +casual read, and makes every defaulted `HandlerError{}` emit `"data":[null]` in the error +response. It was caught here only by an end-to-end assertion on `dispatch()`'s output. + +Anyone hand-editing `contracts/codegen/gen_cpp.py` around json defaults or literals: prefer +`= nullptr` / `= {}` / `json::object()` / `json::array()` over brace-init with a single +element, and keep the `dispatch()`-output assertions that would catch a regression. + +## Consequences + +- The 1.4.0 bump stands as a minor with a migration note; this ADR is its written backing. +- A future generated-binding retype cites this ADR instead of relitigating rule 4. +- `contracts/README.md` rule 4 gets a pointer here so the "major + ADR" line isn't read in + isolation. + +## Alternatives rejected + +**Make it major, per rule 4's literal text.** Rejected: `session.hello` compares majors and +would refuse a client whose wire behaviour is unchanged. The rule's purpose is to stop +incompatible peers connecting; applying it here would block compatible ones. + +**Leave the reasoning in commit messages and `proto-answers-*` docs, as done for 1.4.0.** +Rejected: those are per-instance and get lost. The rule needs one durable home, which is +what `docs/adr/` is for (`ADR 0001`).