From 4be071cfaa67012e835af91c72378d2600b03337 Mon Sep 17 00:00:00 2001 From: "svc-finitelabs[bot]" <269744575+svc-finitelabs[bot]@users.noreply.github.com> Date: Sat, 19 Sep 2026 11:58:40 -0500 Subject: [PATCH] fix: emit message options in ascending field number order 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 --- .github/workflows/build.yml | 3 ++ CLAUDE.md | 14 +++++ Makefile | 9 ++++ tools/check_option_order | 104 ++++++++++++++++++++++++++++++++++++ tools/gen_lua_proto_schema | 9 ++-- 5 files changed, 136 insertions(+), 3 deletions(-) create mode 100755 tools/check_option_order diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index a37cf91..5ca4c56 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -65,6 +65,9 @@ jobs: - name: Check float vectors run: make check-float-vectors + - name: Check option order + run: make check-option-order + test: needs: check runs-on: ubuntu-latest diff --git a/CLAUDE.md b/CLAUDE.md index 28a47c9..5248cb3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -452,6 +452,20 @@ deliver them, but `parse_descriptor_set` skips any file whose package is `check-schema` fails. `test/test_messages_proto3.proto` is vendored with those fields trimmed for that reason. Tracked as FL-17. +**Message options are sorted by field number, and the sort is load-bearing.** 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 number. The generator reads +those bytes back, so without the sort the same `.proto` produces two different Lua +files depending on the toolchain, and a consumer's drift check reports a false +positive against an unpinned protoc. `make check-option-order` guards it. + +That check 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 CI runner; feeding the options in descending +order is what makes the check fail on a revert under every protoc. CI installs +Ubuntu's `protobuf-compiler`, which is 3.21.12 on noble, so `check-types` and +`check-schema` run on the declaration-order side of that split. + ## Building The build process uses `amalg` to create single-file distributions: diff --git a/Makefile b/Makefile index 84a7251..9f7531c 100644 --- a/Makefile +++ b/Makefile @@ -249,6 +249,15 @@ check-float-vectors: @echo "Checked-in float vectors match the generator." @LUA_BINARY=$(LUA_BINARY) .venv/bin/python3 tools/check_float_vectors +# Check the generator sorts message options. Needs the venv, so not in `check`. +.PHONY: check-option-order +check-option-order: + @if [ ! -f .venv/bin/python3 ]; then \ + echo "Python virtual environment not found. Run 'make setup-schema-generator' first."; \ + exit 1; \ + fi + @.venv/bin/python3 tools/check_option_order + # Format Lua code with stylua .PHONY: format format: diff --git a/tools/check_option_order b/tools/check_option_order new file mode 100755 index 0000000..8503b45 --- /dev/null +++ b/tools/check_option_order @@ -0,0 +1,104 @@ +#!/usr/bin/env python3 +""" +Verify message options are emitted in ascending field number order. + +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 reads those bytes back, so without a sort its output depends on which +protoc built the descriptor. + +A fixture `.proto` cannot check this on its own. Under a modern protoc an unsorted +generator already emits ascending, so the check would pass on the defect and only +fail where CI happens to install an old protoc. The descriptor here is therefore +built by hand with the options in descending order, which is the one input that +tells a sorted generator from an unsorted one under every protoc. + +1. Setup: the options really do arrive descending, so the comparison is not vacuous. +2. Order: the generator emits them ascending. +""" +import os +import re +import sys + +from google.protobuf.descriptor_pb2 import DescriptorProto +from google.protobuf.internal import decoder, encoder + +_GEN = os.path.join(os.path.dirname(os.path.abspath(__file__)), "gen_lua_proto_schema") +_module = type(sys)("gen_lua_proto_schema") +exec(compile(open(_GEN).read(), _GEN, "exec"), _module.__dict__) + +# Numbers and shapes mirror ESPHome's api_options.proto, the schema that surfaced this. +EXTENSIONS = {1036: "id", 1038: "ifdef", 1041: "base_class"} + + +def varint(number: int, value: int) -> bytes: + out = bytearray() + write = encoder._VarintEncoder() + write(out.extend, number << 3 | 0, True) + write(out.extend, value, True) + return bytes(out) + + +def length_delimited(number: int, value: str) -> bytes: + out = bytearray() + write = encoder._VarintEncoder() + write(out.extend, number << 3 | 2, True) + encoded = value.encode() + write(out.extend, len(encoded), True) + out.extend(encoded) + return bytes(out) + + +def wire_order(raw: bytes) -> list[int]: + """Field numbers in the order they appear in `raw`.""" + pos = 0 + numbers = [] + while pos < len(raw): + tag, pos = decoder._DecodeVarint(raw, pos) + numbers.append(tag >> 3) + if tag & 0x7 == 0: + _, pos = decoder._DecodeVarint(raw, pos) + else: + length, pos = decoder._DecodeVarint(raw, pos) + pos += length + return numbers + + +def main() -> int: + failures = [] + + message = DescriptorProto() + message.name = "OptionOrder" + message.options.MergeFromString( + length_delimited(1041, "BaseProtoMessage") + varint(1038, 1) + varint(1036, 3) + ) + + arrived = wire_order(message.options.SerializeToString()) + expected_input = sorted(EXTENSIONS, reverse=True) + if arrived != expected_input: + failures.append( + "options did not arrive descending: %s, wanted %s. The generator would be " + "handed sorted input and the order assertion below would pass on any " + "generator." % (arrived, expected_input) + ) + + lua = _module.parse_message(message, message_option_extensions=EXTENSIONS) + body = lua.split("options = {", 1)[1].split("}", 1)[0] + emitted = re.findall(r"^\s*(\w+) = ", body, re.MULTILINE) + wanted = [EXTENSIONS[number] for number in sorted(EXTENSIONS)] + if emitted != wanted: + failures.append("emitted %s, wanted %s" % (emitted, wanted)) + + print("Option order: %d options fed in descending order, emitted as %s" + % (len(arrived), emitted)) + for detail in failures: + print(" FAIL: " + detail) + if failures: + print("Option order: FAILED") + return 1 + print("Option order: the generator sorts by field number") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tools/gen_lua_proto_schema b/tools/gen_lua_proto_schema index 456c93d..8fe4827 100755 --- a/tools/gen_lua_proto_schema +++ b/tools/gen_lua_proto_schema @@ -295,8 +295,11 @@ def parse_message( # Some messages extend MessageOptions in a way that does not get parsed # into the Extensions field but is available in the raw data. if len(message.options.Extensions) == 0 and message_option_extensions: - for num, typ, val in parse_unknown_fields( - message.options.SerializeToString() + # protoc does not serialize these in a stable order: 3.21 and 22.5 emit + # them in .proto declaration order, 23.4 and later ascending by number. + for num, typ, val in sorted( + parse_unknown_fields(message.options.SerializeToString()), + key=lambda unknown: unknown[0], ): if num not in message_option_extensions: continue @@ -308,7 +311,7 @@ def parse_message( val = f"\"{val.decode('utf-8', 'replace')}\"" lua += (" " * (indent + 2)) + f"{name} = {val},\n" else: - for field in message.options.Extensions: + for field in sorted(message.options.Extensions, key=lambda ext: ext.number): if not new_line_added: lua += "\n" new_line_added = True