Skip to content

fix: emit message options in ascending field number order - #33

Closed
svc-finitelabs[bot] wants to merge 1 commit into
mainfrom
agent/DRV-136-deterministic-option-order
Closed

svc-finitelabs[bot] wants to merge 1 commit into
mainfrom
agent/DRV-136-deterministic-option-order

Conversation

@svc-finitelabs

Copy link
Copy Markdown
Contributor

Fixes DRV-136.

tools/gen_lua_proto_schema emitted the keys inside each options table in whatever order protoc happened to serialize them, so the same .proto and the same generator produced two different Lua files depending on the toolchain.

Mechanism

Not quite what the ticket recorded, so worth restating. The ticket described two mutually exclusive code paths selected by whether the Python runtime resolved the extensions. Measured, that is not what happens here: all 157 ESPHome messages take the unknown-fields path under every protoc tested, and the Extensions path never runs. The generator never registers the descriptor set into a pool, so these extensions cannot resolve.

The instability is entirely inside the one live path. protoc's own serialization order for extension options changed:

protoc order of options in the descriptor
3.21.12, 22.5 .proto declaration order
23.4, 24.4, 25.1, 29.3, 35.1 ascending by field number

parse_unknown_fields walks those bytes in wire order and the generator emitted them as-is. For ListEntitiesBinarySensorResponse, whose source declares id, base_class, source, ifdef, protoc 3.21.12 hands over exactly that while 29.3 hands over id, source, ifdef, base_class.

That also narrows the boundary: the ticket bracketed it between 22.5 and 24.4 with 23.x untested. It is between 22.5 and 23.4.

Change

Both option paths sort by field number. The Extensions path is dead for this input but is sorted too, so the ordering does not depend on which path runs.

The order this settles on is the one modern protoc already produced, which is what the committed consumer schemas were generated from, so no consumer has to regenerate. control4-esphome's check-proto passes against this generator under both 3.21.12 and 29.3. Before the change it failed under 3.21.12 with a 110 line diff.

Guard

make check-option-order is a new step in the check job, alongside the existing vector checks.

It builds its descriptor by hand instead of using a fixture .proto, and that is the point. Under a modern protoc an unsorted generator already emits ascending, so a fixture based check would pass on the defect everywhere except a runner that happens to install an old protoc. Feeding the options in descending order is the one input that separates a sorted generator from an unsorted one under every protoc. It also asserts the options really did arrive descending, so it cannot pass vacuously if a future protobuf release starts sorting unknown fields on reserialize.

Confirmed sensitive by reverting the sort: exit 1 with the defect, exit 0 with the fix.

Verification

Regenerated under protoc 3.21.12, 22.5, 23.4, 24.4, 25.1, 29.3 and 35.1, against the ESPHome 2026.8.2 API protos and every fixture in this repo (nested, maps, test_messages_proto3, empty):

  • After: one output per input across all seven.
  • Before: the ESPHome schema had two forms; the four local fixtures were already stable, and their output is byte identical before and after, so nothing else moved.

On the ticket's open question of whether anything else is unstable, the two ESPHome forms were identical as line multisets and every differing line fell inside an options block. Structurally, options was the only site reading raw serialized bytes or an unordered collection; every other emission loop walks a repeated descriptor field, which protoc orders by declaration.

format-check, check-types, check-schema, typecheck, test, check-wire-vectors, check-float-vectors and check-option-order all pass locally. make lint fails on this machine before and after the change: luacheck 1.2.0 cannot load under Homebrew's Lua 5.5. CI pins 5.4.

Not addressed

parse_unknown_fields raises on wire types 1 and 5, so a fixed32, fixed64, float or double message option would fail to parse. main wraps generation in a bare except that prints and exits 0, so that surfaces as a silently truncated schema rather than an error. Pre-existing and not reachable from the protos in use, so left alone rather than widened into this change.

protoc does not serialize extension options in a stable order: 3.21 and 22.5
emit them in .proto declaration order, 23.4 and later ascending by field
number. The generator read those bytes back unsorted, so the same .proto and
the same generator produced two different Lua files depending on which protoc
built the descriptor. A consumer's drift check then reports a false positive
against an unpinned toolchain, which is how this surfaced.

Both option paths now sort by field number. That settles on the order modern
protoc already produced, so the schemas committed in consumer repos are
unchanged: control4-esphome's check-proto passes against this generator under
both 3.21.12 and 29.3, where before it failed under 3.21.12.

check-option-order builds its descriptor by hand rather than from a fixture.
Under a modern protoc an unsorted generator already emits ascending, so a
fixture would pass on the defect everywhere except an old-protoc runner;
feeding the options descending is what makes a revert fail under every protoc.
Confirmed sensitive by reverting the sort.

Regenerating across protoc 3.21.12, 22.5, 23.4, 24.4, 25.1, 29.3 and 35.1 now
gives one output for the ESPHome API protos and for every fixture here. Before
the change the ESPHome schema had two forms, splitting between 22.5 and 23.4,
which narrows the 22.5-to-24.4 bracket the ticket recorded. Options ordering
was the only difference: the two forms were identical as line multisets and
every differing line fell inside an options block.

Fixes DRV-136
@svc-finitelabs

svc-finitelabs Bot commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor Author

Consumer-side impact, measured end to end

Run against control4-esphome's real make check-proto at its committed src/esphome/proto_schema.lua, 2x2 over generator ref and protoc. The DRV-128 session ran this first and passed it over (control4-esphome#131 comment); I reproduced the one cell I had never measured, plus its control.

generator protoc 29.3 protoc 3.21.12
v0.6.9 (68b7adf) matches MISMATCH, exit 2, 110 diff lines
this PR (4be071c) matches matches

The bottom row and the top-right control were measured when this PR was opened. New here is the top-left cell. The top-right is the positive control and still fails at exactly the 110 diff lines recorded in the ticket, so the harness is sensitive and this PR is what flips it.

Two consequences for the merge.

Output-neutral for the pinned consumer

control4-esphome pins protoc 29.3, and at that version the fixed generator converges on the byte order already committed there. So no regeneration is needed in that repo when this lands, and there is no consumer-side breakage to announce. This PR removes the dependency on the protoc version rather than changing the output anyone is currently holding.

A release carrying this does not change the runtime

v0.6.9...4be071c is two commits, not one: this PR, plus 9ae6305 (#32), already on main, which adds test/lpack_stub.lua and test/lpack_test.lua. That wider span is the one that matters for rollout, since nobody re-vendors a PR, they re-vendor a ref. Across the whole span the src/ and vendor/ trees are the same objects:

$ git rev-parse v0.6.9:src 4be071c:src
1e092198749fb43ed0c7becb96e29a7d0e65f9ce
1e092198749fb43ed0c7becb96e29a7d0e65f9ce
$ git rev-parse v0.6.9:vendor 4be071c:vendor
d0a00501e2853a3862c88fa2bb77e947c2f1cf35
d0a00501e2853a3862c88fa2bb77e947c2f1cf35

build/ is untracked and amalgamated from src/protobuf/init.lua, so equal trees settle it directly: an unchanged src/protobuf/init.lua cannot begin requiring a file that did not exist at the tag, and nothing under src/ or vendor/ names either new file. Reachability is the wrong leg to lean on here, for the record: LUA_PATH_LOCAL puts ./?.lua first, so a dotted require would in fact reach test/lpack_stub.lua. What closes that path is the identical tree, not the repo root being free of Lua modules. So build/protobuf.lua and build/protobuf-portable.lua come out byte-identical except the VERSION line injected by make build.

Worth knowing for rollout: re-vendoring the amalgamated runtime is a no-op across everything main has moved since v0.6.9, newly added .lua files included. This fix ships through tools/gen_lua_proto_schema only, so a consumer who re-vendors build/protobuf.lua to pick it up sees no change at all, which is easy to read as the fix not having landed.

Nothing pushed, head is still 4be071c. Awaiting your review.

@derek-miller

Copy link
Copy Markdown
Contributor

Closing, per Derek. Option key order in a Lua table has no runtime meaning, and since control4-esphome moved to the provenance check nothing regenerates and diffs across toolchains, so this is won't-fix (DRV-136). The silent exit-0 you flagged under "Not addressed" is being fixed separately; no further work on this PR.

@derek-miller
derek-miller deleted the agent/DRV-136-deterministic-option-order branch September 24, 2026 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant