Skip to content

fix(tools): decode schema options by declared type and fail loudly - #35

Merged
svc-finitelabs[bot] merged 4 commits into
mainfrom
agent/DRV-144-gen-schema-exit-status
Sep 25, 2026
Merged

svc-finitelabs[bot] merged 4 commits into
mainfrom
agent/DRV-144-gen-schema-exit-status

Conversation

@svc-finitelabs

@svc-finitelabs svc-finitelabs Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Fixes DRV-144.

The defect

On main, main() in tools/gen_lua_proto_schema catches every exception, prints An error occurred, and returns. A failed generation therefore exits 0 and leaves the output file untouched. parse_unknown_fields raises on wire types 1 and 5, so any double, float, fixed32, sfixed32, fixed64 or sfixed64 message option takes that path. In control4-esphome, gen-proto then runs fmt-lua and proto-provenance stamp on the unchanged file. The result is the old schema body stamped with the new version, and it passes check-proto.

Reproduced on main at 906d4b8: a fixed-width and a group option both print An error occurred, exit 0, and leave a sentinel output file as it was.

main also emits the raw varint for every varint option and wraps string options in quotes with no escaping. So a negative int32 or enum option comes out as 2^64 minus its magnitude, a bool comes out as 1/0, and a string containing " or a newline gives Lua that does not load, still at exit 0. Option names are emitted as bare keys. With package acme; the output has acme.opt_id = 7, inside options = {, and an option named end gives end = true,. Neither loads.

Changes

  • Exit status: main() no longer catches exceptions, so a failure exits 1 with a traceback. Every failure path runs before the output file is opened, so the output stays untouched.
  • Decoding by declared type: the extension map now records each option's declared type. The wire type alone cannot tell double from sfixed64, bool from uint32, or sint32 from int32.
    • Fixed-width: wire types 1 and 5 decode through struct, using the format for the declared type.
    • double/float: infinities emit math.huge and -math.huge, and NaN emits (0/0). Negative zero emits -(1/math.huge), because Lua 5.1 compiles a -0.0 literal to an earlier 0 constant in the same chunk and loses the sign.
    • Varint: bool emits true/false. int32, int64 and enum are read as two's-complement, since a negative value of any of them is a sign-extended 10-byte varint. sint32/sint64 are zigzag-decoded. uint32/uint64 are unchanged.
    • Strings and bytes: these emit a valid Lua literal. \, ", CR and LF are escaped, and other control bytes become zero-padded \ddd, as do bytes that are not valid UTF-8 (main used the lossy 'replace' decode). Valid UTF-8 passes through. The output file is written as UTF-8 explicitly instead of in the locale default.
  • Option keys: a name that is not a plain identifier, or that is a Lua keyword, is emitted in bracket syntax, e.g. ["acme.opt_id"] = 7 and ["end"] = true. Plain names are unchanged.
  • Fails loudly, naming the option or field:
    • a message-typed or group-typed option
    • a packed repeated option. protoc 22 and older serialize a packed option unpacked, so there it takes the repeated path below. Only protoc 23 and newer reach this error.
    • a TYPE_GROUP field in a message, which main emits as ProtoSchema.WIRE.UNKNOWN and which errors when the schema loads
    • a 64-bit option (int64, uint64, sint64, fixed64, sfixed64) whose magnitude exceeds 2^53. Exactly ±2^53 is still emitted.
  • Not changed: a non-packed repeated option still emits each value as a duplicate key, and the last one wins. Enum value names, and message and enum names, still go out as bare keys.

CLAUDE.md no longer says a failed generation exits 0.

Downstream effect

control4-esphome's next gen-proto will change its bool options from 1/0 to true/false. Regenerating against ESPHome 2026.8.2 with this head gives exactly 69 changed lines, all of them these options:

  • no_delay: 61 lines
  • speed_optimized: 6 lines
  • log: 1 line
  • inline_encode: 1 line

api_options.proto has no package, so no option key changes. On control4-esphome main, the driver reads options.id (client.lua 1989, 2065, 2498) and options.ifdef (152, 746). Neither changes: id is a uint32, and every ifdef is plain ASCII.

Tests

test/gen_schema_controls.sh is new and runs from the existing check-schema target, which gains one line, and the target's comment now says so. There is no new target or CI step. Every fixture is written inline to a temp directory. It has 15 controls and ends with an N/M controls passed footer, like test/proto_provenance_controls.sh.

  • Round-trips: each loads the generated Lua and compares values.
    • all six fixed-width types, plus a uint32 varint
    • varint, string and bytes: bools, negative int32/int64, negative sint32/sint64, uint64, a positive and a negative enum, a string with every escape class plus \001 followed by a digit and a UTF-8 é, bytes \377\000\200z (a lone 0x80 is the lower bound of the non-UTF-8 range), and both ±2^53 boundaries. It also asserts that the generated file contains no raw control byte, because Lua accepts raw control bytes inside a literal and a value comparison alone would pass.
    • ±infinity and negative zero for both double and float. Negative zero is checked with 1/x == -math.huge, and the file must not contain a -0.0 literal, because CI runs check-schema under Lua 5.4, which keeps the sign.
    • NaN, checked with ~= itself. protoc 3.21, the version CI installs from apt, cannot express a NaN option: it rejects nan, -nan and -inf. ±1e999 works on 3.21 and 35.1. So this control renders NaN through the generator's option_literal directly, for both double and float.
    • a packaged option name, and an option named end
  • Failures: each must exit non-zero, leave the output byte-identical, and name the option or field on stderr.
    • a group option and a message option
    • a group field
    • one option each of int64, uint64, sint64, fixed64 and sfixed64 at 2^53+1, negative for the signed types
    • a packed repeated option. CI's protoc cannot produce one, so this calls option_literal directly and checks the error names it as packed.

Output for the existing fixtures nested, maps, test_messages_proto3 and empty is byte-identical to main.

Revert-checks

At 02f6890, each mutation was applied alone, checked to apply exactly once, and restored, with the file's checksum confirmed after each one. Controls ran under Lua 5.5, LuaJIT 2.1 and Lua 5.1, all with protoc 3.21.12. Every mutation went red on all three, and each is killed by the control for its own arm:

mutation control that goes red
catch-and-print restored around main() every failure control
sfixed64 decoded as <Q fixed-width, varint/string/bytes, sfixed64 beyond 2^53
float decoded as <i fixed-width, infinity/negative zero, NaN
NaN branch disabled NaN
infinity branch disabled infinity/negative zero
negative-zero branch disabled infinity/negative zero
bool emitted as str(val) varint/string/bytes, keyword name
two's-complement conversion removed varint/string/bytes
enum left out of the two's-complement conversion varint/string/bytes
zigzag decode removed varint/string/bytes, sint64 beyond 2^53
escape table emptied varint/string/bytes
CR emitted raw varint/string/bytes
control-byte \ddd branch disabled varint/string/bytes (raw-byte scan)
non-UTF-8 \ddd branch disabled varint/string/bytes
0xDC80 <= changed to 0xDC80 < varint/string/bytes
surrogateescape reverted to 'replace' varint/string/bytes
option keys always bare packaged name, keyword name
keyword check removed keyword name
identifier check removed packaged name
message-option check removed (falls to the packed message) message option
packed error reverted to "is TYPE_INT32" packed repeated option
group-option naming disabled (falls back to Unsupported wire type: 3) group option
group-field guard disabled group field
2^53 guard disabled all five 64-bit controls
abs(val) > changed to val > sint64 and sfixed64 beyond 2^53
> changed to >= varint/string/bytes (the ±2^53 boundary)

Local runs

These all pass: make test, format-check, check-provenance, check-types, check-schema, typecheck, check-wire-vectors and check-float-vectors. The controls also pass under LuaJIT, Lua 5.1 and Lua 5.5, with both protoc 35.1 and 3.21.12. Local make lint fails identically on main, because Homebrew's luacheck breaks under Lua 5.5. This PR changes no Lua files, so CI is the lint signal.

No release: this rides the next lua-protobuf release, which the template re-vendors on the tag after v0.9.30.

…ptions

main() caught every exception, printed "An error occurred" and returned, so
a failed generation exited 0 with the output file untouched. A consumer that
formats and stamps the output next (control4-esphome's gen-proto) then
recorded provenance for a schema that was never generated. The handler is
gone, so any failure exits non-zero with a traceback before the output is
opened.

parse_unknown_fields rejected wire types 1 and 5, so any double, float,
fixed32, sfixed32, fixed64 or sfixed64 message option took that path. They
now decode, using the extension's declared type for sign and float
interpretation, and emit like the varint path. A non-finite float has no Lua
literal (inf would load as nil), so it fails instead of emitting one.

check-schema runs test/gen_schema_controls.sh: each fixed-width type round-
trips into the generated Lua, and an unsupported wire type (a group option)
and a non-finite double both exit non-zero with the output unchanged.

Fixes DRV-144
…ture

The apt protobuf-compiler on CI (3.21.x) rejects the bare `inf`
identifier as a double option value, so protoc failed before the
generator ran and the control saw the wrong error. `1e999` parses to
infinity on both 3.21.12 and 35.1, and the control still fails when the
non-finite guard is removed.
@derek-miller

Copy link
Copy Markdown
Contributor

@svc-finitelabs please widen this PR. Derek's decision is to decode every message option by its declared type instead of failing on the types we can represent. Keep the loud-failure path for the ones we can't.

Support instead of failing

  • Non-finite double/float: emit math.huge, -math.huge and (0/0). Drop the ValueError. The nonfinite_option.proto control becomes a round-trip check: inf == math.huge, and NaN is ~= itself.
  • Varint by declared type:
    • BOOL emits true/false.
    • INT32/INT64 are two's-complement signed, so a negative value is not emitted as a huge positive.
    • SINT32/SINT64 are zigzag-decoded.
    • UINT* and ENUM stay as they are.
  • Strings and bytes: emit a valid Lua string literal. Escape \, ", CR, LF and other control bytes, and use \ddd escapes for non-UTF-8 bytes instead of the lossy 'replace' decode.

Fail loudly, naming the option or field

  • Message-typed and group-typed options.
  • A TYPE_GROUP field in a message. Today parse_field emits ProtoSchema.WIRE.UNKNOWN, so the schema crashes on load instead of failing generation.
  • A 64-bit option (int64, uint64, sint64, fixed64, sfixed64) whose magnitude exceeds 2^53. Controllers run Lua 5.1 or LuaJIT, where the literal would silently round.

Tests and constraints

  • Extend the existing controls in test/gen_schema_controls.sh. Revert-check each new arm, as you did for the first two.
  • Output for the existing fixtures must stay byte-identical.
  • Say in the body that control4-esphome's no_delay and speed_optimized options change from 1/0 to true/false on its next regeneration. Its driver reads only options.id.
  • No new Makefile targets or CI steps. Comments one line or none. No attribution lines. Update this PR and stop. No release.

Options the Lua schema can represent are now rendered instead of
rejected or mis-rendered:

- double/float: infinities become math.huge / -math.huge, NaN (0/0)
- bool: true/false instead of 1/0
- int32/int64: two's-complement signed, so negatives stay negative
- sint32/sint64: zigzag-decoded
- string/bytes: a valid Lua literal. \ " CR LF and control bytes are
  escaped, and bytes that are not valid UTF-8 become zero-padded \ddd
  instead of the lossy U+FFFD replacement. Output is written as UTF-8.

Generation fails loudly, naming the option or field, for message- and
group-typed options, TYPE_GROUP message fields (previously emitted as
ProtoSchema.WIRE.UNKNOWN, which errors on load), and 64-bit options
whose magnitude exceeds 2^53, which Lua 5.1 and LuaJIT would round.

Output for the existing fixtures is byte-identical. protoc 3.21 cannot
express a NaN option, so the NaN control renders it through the
generator's option_literal directly.
@svc-finitelabs svc-finitelabs Bot changed the title fix(tools): fail gen_lua_proto_schema loudly and decode fixed-width options fix(tools): decode schema options by declared type and fail loudly Sep 24, 2026
@svc-finitelabs

Copy link
Copy Markdown
Contributor Author

Widened at 1c38644, and the body is rewritten. Three places differ from the ask:

  • The driver reads options.ifdef as well as options.id (client.lua 611, 630). Neither value changes: id is a uint32, and every ifdef is plain ASCII.
  • Four options flip, not two. log and inline_encode are also declared bool. Regenerating against ESPHome 2026.8.2 changes 69 lines: no_delay 61, speed_optimized 6, log 1, inline_encode 1. The body lists all four.
  • NaN does not go through nonfinite_option.proto. CI's apt protoc 3.21 rejects nan, -nan and -inf in an option. That fixture now round-trips ±infinity (±1e999) for double and float. The NaN control renders through the generator's option_literal and checks ~= itself.

One side effect: a packed repeated option now fails loudly through the same guard as message options. Non-packed repeated options still emit duplicate keys, as before.

Existing fixture output is byte-identical. Each of the 16 mutations turns its own control red. Stopped here, no release.

@derek-miller derek-miller left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is right and the gates pass at 1c38644 (check-schema with all 12 controls, make test, check-types, the controls under LuaJIT 2.1 and Lua 5.1 to 5.4). Output for nested, maps, test_messages_proto3 and empty is byte-identical to main. A few things before this merges.

Must fix

  • A negative enum option still comes out as 2^64-1 and exits 0. Enums go on the wire as int32, so a negative value is a sign-extended 10-byte varint, but option_literal (line 101) only applies two's complement to TYPE_INT32 and TYPE_INT64, and TYPE_ENUM is not in INT64_TYPES, so the 2^53 guard misses it too. enum Sign { NEG = -1; ZERO = 0; } with option (opt_sign) = NEG; gives opt_sign = 18446744073709551615, and loads as 1.844674407371e+19. Add TYPE_ENUM to the two's-complement branch, put a negative enum value in typed_options.proto, and drop "enum is unchanged" from the body.

Should fix

  • CLAUDE.md lines 443-445 still say the generator "wraps generation in a bare except that prints the error and still exits 0". That is now the opposite of the code. Delete it, or replace it with one line: a failed generation exits non-zero and leaves OUTPUT untouched.
  • Option keys are emitted bare (line 386), so a packaged or keyword option name gives invalid Lua and exits 0. With package acme; the output has acme.opt_id = 7, inside options = {; an option named end gives end = true,. Both fail to load. This predates the PR, but it is the same exit-0 failure DRV-144 is about, on the line this PR rewrites. Emit ["acme.opt_id"] = 7 (as lua_table_key already does for qualified names) when the name is not a plain non-keyword identifier, with one round-trip control.

Nits

  • A packed repeated option fails with "Option opt_packed is TYPE_INT32, which has no Lua literal", which is false for int32. Say it is a packed repeated field. The parse_unknown_fields docstring still says "Only decodes varints and strings for demonstration".
  • No fixture has a lone 0x80 byte, so 0xDC80 < code survives mutation, and under that mutant a "\200" option raises after the output file is opened and leaves it truncated. Add \200 to opt_bytes.
  • A -0.0 double loads as +0.0 on LuaJIT and 5.1. Optional: emit -(1/math.huge).
  • Match the repo: load the generator in the NaN control the way tools/check_wire_vectors does (type(sys) plus exec) instead of importlib; give the controls script the sibling's passed counter and N/M controls passed footer, and make out local; annotate INT64_TYPES and LUA_STRING_ESCAPES like the other module tables; extend the check-schema comment (Makefile line 168) to say it also runs the option and failure controls.
  • Fixtures: group_option.proto and the inline message_option are the same kind of check stored two ways. Moving the checked-in ones into write_proto heredocs like the rest would be simpler.

Body

It describes changes against this PR's own earlier heads: "The earlier ValueError is removed" (added in d4cfe88, never on main), "still runs from the existing check-schema target" (the script is new), byte-identity "to the previous head fa67c32", and the first-round mutation paragraphs. Rewrite it against main 906d4b8, and update the client.lua line numbers to esphome main (ifdef is read at 152 and 746, id at 1989, 2065 and 2498). The 69-line downstream diff checks out, and the driver reads no option whose value changes.

…n keys

A negative enum option is a sign-extended varint on the wire, so it came out as
2^64-1. Option names from a package, or that are Lua keywords, were emitted as
bare keys and gave a schema that fails to load while the generator exits 0.
They now go through bracket syntax.

Negative zero is emitted as -(1/math.huge), because Lua 5.1 compiles a -0.0
literal to an earlier 0 constant in the same chunk. A packed repeated option
now says so in its error.

The controls' fixtures are all written inline, and the script reports N/M
controls passed like the provenance controls. CLAUDE.md no longer says a failed
generation exits 0.
@svc-finitelabs

Copy link
Copy Markdown
Contributor Author

Addressed at 02f6890. CI is 8/8 green, and the Check log shows 15/15 generator controls under apt protoc 3.21.12.

Must fix

  • Negative enum: TYPE_ENUM is now in the two's-complement branch. typed has Sign { NEG = -1; ZERO = 0; } with opt_neg_enum = NEG, which round-trips as -1. Dropping TYPE_ENUM from the branch turns that control red. The body no longer says enum is unchanged.

Should fix

  • CLAUDE.md: replaced with "A failed generation exits non-zero and leaves OUTPUT untouched."
  • Option keys: a name that is not a plain identifier, or is a Lua keyword, is now emitted as ["acme.opt_id"] = 7 / ["end"] = true. There are two round-trip controls rather than one, because an extension in a package always gets the package prefix, so one proto cannot hold a bare keyword name. Removing the keyword check turns only the keyword control red, and removing the identifier check turns only the packaged one red. Enum value names, and message and enum names, still go out bare. That is noted under "Not changed" in the body and left out of scope.

Nits

  • Packed message: now reads "Option opt_packed is a packed repeated TYPE_INT32 field". An end-to-end control is not possible on CI: protoc 22.5 and older, 3.21 included, serialize a packed option unpacked, so it takes the duplicate-key path and exits 0. Only 23.4 and newer reach the error. So this control calls option_literal directly, as the NaN one does. The parse_unknown_fields docstring is rewritten.
  • \200: added to opt_bytes. The 0xDC80 < mutant now goes red.
  • -0.0: fixed, but LuaJIT is not affected. It keeps the sign in every case I tried (2.1 on arm64). Lua 5.1 loses it only when an earlier 0 constant is in the same chunk: a standalone return {a = -0.0} keeps it, and local x = 0; return {a = -0.0} does not. The generated file always has VARINT = 0 first, so 5.1 always loses it there. It now emits -(1/math.huge). CI runs check-schema only under 5.4, which keeps the sign, so the control also rejects a -0.0 literal in the text. That is what makes the mutant go red in CI.
  • Repo conventions: the NaN control loads the generator with type(sys) plus exec, the script has the passed counter and the N/M controls passed footer, out is local, INT64_TYPES and LUA_STRING_ESCAPES are annotated, and the Makefile comment names the controls.
  • Fixtures: all four checked-in protos are gone. Every fixture is now a write_proto heredoc.

Body: rewritten against main 906d4b8, with the client.lua lines set to esphome main a8a5db8 (ifdef 152 and 746, id 1989, 2065 and 2498, all confirmed). The ESPHome 2026.8.2 output is byte-identical to 1c38644's, so the 69-line downstream diff is unchanged. api_options.proto has no package, so no downstream key changes. The revert-check table was re-run at 02f6890 with the fixtures in their new place: 26 mutations, each red under 5.5, LuaJIT and 5.1.

@derek-miller derek-miller left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review's points are addressed at 02f6890: enum options sign-extend, non-identifier option keys are bracketed, CLAUDE.md is true again, and the nits and body are fixed. All 8 checks pass.

@svc-finitelabs
svc-finitelabs Bot merged commit 3ad17bb into main Sep 25, 2026
8 checks passed
@svc-finitelabs
svc-finitelabs Bot deleted the agent/DRV-144-gen-schema-exit-status branch September 25, 2026 18:19
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