fix(proto): a zero-byte packet has to be Mojang's word, not ours - #1038
Merged
Conversation
`play/clientbound/minecraft:boss_event` has been extracted as carrying no bytes for this whole protocol version. It writes a UUID, an enum ordinal and a variable body. Nothing caught it, because a wrong answer and a correct one are the same shape to every gate we have: `complete: true`, absent from `coverage.incomplete`, no reason recorded anywhere. `Analyzer.method_body` resolved `Foo::write` by scanning the class body text for the first `write(` followed by a block. A class body carries its nested and anonymous classes inline, and `ClientboundBossEventPacket` declares an anonymous `Operation` whose `write` is empty *above* its own. Zero fields became `unit` became `complete`. Three changes, in increasing order of how much they matter. `methods()` only sees declarations at brace depth zero, so a member of a nested class is never mistaken for the outer class's own. This also let two other layouts through: `level_chunk_with_light` now resolves down to `Heightmap$Types#STREAM_CODEC`, and `update_recipes` past `SelectableRecipe.noRecipeCodec` to the dispatch underneath. An empty layout can no longer be inferred. `read_encode_body` returning zero fields is now a refusal; the only empty layout is one Mojang spells `StreamCodec.unit(..)`. No-op on today's data, which is the point: the silent path is gone rather than currently unoccupied. `coverage.empty` records every packet claimed to carry no bytes, and each one is warranted independently -- `StreamCodec.unit(..)` has to appear in the packet's own source or in the expression it was registered with. A packet that comes out empty without that fails the extraction by name. The baseline now holds the *cause* of each refusal rather than only its id, because the count did not move when this bug was introduced and would not move if it came back. And the four `custom_payload` entries are carved out of the gap list into `coverage.opaque`: a channel id followed by bytes the server has no layout for either is not work left, and counting it as work left makes 63 mean "59, plus four that are finished". Gaps: 62 -> 59. Generated packet structs: 180 -> 179, the one that vanished being the boss bar packet's empty struct, which was never a packet.
Benchmark Results for generalComparing to 035a8ce |
andrewgazelka
added a commit
that referenced
this pull request
Jul 28, 2026
A Java enum's constant order is a wire table. `FriendlyByteBuf.writeEnum` sends `ordinal()`; `ByteBufCodecs.idMapper` sends a field each constant declares. Either way the declaration order in the jar is as much a definition with a source of truth as a registry is, and hand-transcribing one is the same defect — with the extra hazard that an ordinal carries no name on the wire, so a transcription one constant out is a wrong value rather than a failed lookup. Eleven of them were typed out in this crate. `EquipmentSlot` twice, once per module, with different accessor names on the two copies. ## Why exactly these eleven, which is the interesting part A hand-written ordinal enum here is almost exactly the set whose enclosing packet the extractor refused. Where a packet's layout is recovered in full, `build.rs::lower_enum` already turns the enums it mentions into closed `#[repr]` Rust enums from `protocol.json` — twenty of them exist in `OUT_DIR` right now, and nobody typed any of them. `protocol.json` carries 42 extracted enum definitions with full constant lists; the other 22 never reach Rust because they sit inside a refused packet or data component. `AttributeModifier$Operation` is the clean proof: it *is* extracted, with its three constants, and it is *also* hand-written in `item/payload.rs`, purely because `dataComponent/minecraft:attribute_modifiers` is a refusal. So these eleven are the ones no recovered layout reaches — carried by `set_player_team`, `set_objective`, `set_equipment`, `commands`, `level_chunk_with_light`, or by the boss bar packet that until #1038 was extracted as carrying no bytes at all. `JAVA_ENUMS` in `nix/extract-protocol.py` reads them out of the decompiled class directly rather than waiting for those layouts to be recovered. | generated | Java class | numbered by | n | |---|---|---|---| | `BossBarColor` | `world.BossEvent$BossBarColor` | ordinal | 7 | | `BossBarOverlay` | `world.BossEvent$BossBarOverlay` | ordinal | 5 | | `ChatTypeParameter` | `network.chat.ChatTypeDecoration$Parameter` | ordinal | 3 | | `Direction` | `core.Direction` | ordinal | 6 | | `DisplaySlot` | `world.scores.DisplaySlot` | `id` | 19 | | `EquipmentSlot` | `world.entity.EquipmentSlot` | ordinal | 8 | | `HeightmapKind` | `world.level.levelgen.Heightmap$Types` | `id` | 6 | | `ObjectiveRenderType` | `world.scores.criteria.ObjectiveCriteria$RenderType` | ordinal | 2 | | `TeamCollisionRule` | `world.scores.Team$CollisionRule` | `id` | 4 | | `TeamColor` | `world.scores.TeamColor` | `id` | 16 | | `TeamVisibility` | `world.scores.Team$Visibility` | `id` | 4 | 80 constants. Same conventions as #1021: `#[repr(uN)]` with the discriminant being the number, `id()` an `as` cast, `name()` off a static table, `from_name` a binary search over a table the generator sorted, closed with no `Unknown`, and a `const` assertion per file proving the discriminant folds. ## The two numberings are declared, not guessed `EquipmentSlot` sends `ordinal()` from `ClientboundSetEquipmentPacket` (`output.writeByte(slotType.ordinal())`) and `s.id` from its own `STREAM_CODEC` (`idMapper(BY_ID, s -> s.id)`), and the two disagree: `OFFHAND` is 1 by ordinal and 5 by id. Which numbering a class uses is the second element of its `JAVA_ENUMS` entry, resolved against the source and named in the generated module's own docs. Nothing infers it, because a value read with the wrong table is a wrong value rather than a failed lookup, and the two tables are the same shape. The extractor refuses rather than approximates: a class that has moved, an id field that has gone, a constant the id table does not mention, or a set that is not dense `0..n-1` all fail the extraction with the class named. ## Checked against the server, not against the parse These constants are read out of decompiled Java by a regex. No test written from that same reading could catch a misreading of it, so the numbers are taken from the other side: `nix/java/VanillaEncoder.java` now emits `java_enum.<Type>.<CONSTANT>` for all 80, and for the six classes that publish a `StreamCodec` it gets the number by **running Mojang's encoder and reading the `VarInt` back out** (asserting the encoding is exactly one `VarInt` and no more). For the five with no codec it reads `DisplaySlot.id()` or `ordinal()` off the class. `EquipmentSlot` deliberately uses the ordinal and not its `STREAM_CODEC`, with the reason at the call site. `tests/java_enum.rs` then walks `ALL` for every type and compares both directions: every variant against the server's number for the name it claims, and the count of `java_enum.<Type>.` fixtures against the number of variants. The second half is what catches a *dropped* constant, which the first half cannot see. ## Guards broken, and what they said Each mutation was checked to have applied (`assert t.count(old) == 1`, then grep for the replacement in the file) and to parse, before being run. **G — a discriminant one out** (`TeamColor::DarkBlue = 1` → `= 2`). Caught, but **not by the test I was aiming at**: ``` error[E0081]: discriminant value `2` assigned more than once --> crates/hyperion-minecraft-proto/src/generated/java_enum/team_color.rs:22:1 ``` Reporting it as it happened: this mutation never reached the test, so it proves the compiler and not the fixture check. Hence H. **H — two entries swapped in the generated name table**, which compiles cleanly and is what a sorting bug in the generator would actually look like. Both tests fire: ``` panicked at tests/java_enum.rs:34: assertion `left == right` failed: TeamColor::Black disagrees with the server about its number left: 0 right: 1 panicked at tests/java_enum.rs:88: assertion `left == right` failed left: None right: Some(Black) ``` **I — the extractor drops a constant**, patched into `scan_java_enums` so the whole pipeline runs on a realistic misparse (`BossBarColor` loses `WHITE`). Regenerated, rebuilt, and the count half fires: ``` panicked at tests/java_enum.rs:42: assertion `left == right` failed: BossBarColor has 6 variants but the server declares 7: ["java_enum.BossBarColor.PINK", ..., "java_enum.BossBarColor.WHITE"] left: 6 right: 7 ``` All three restored; `nix run .#sync-minecraft-proto` reproduces the committed tables and `minecraft-proto-generated` passes. ## Call sites that changed, and one naming decision `DisplaySlot::to_id()` → `id()` (three tests and one line in `events/smash/src/adapter.rs`). `HeightmapKind::from_id` returned `Result`, the generated one returns `Option`, so `world/chunk.rs` does the `ok_or` itself with the reason for not clamping kept at the site. The two `EquipmentSlot` copies collapse into one, so `from_raw`/`from_ordinal` and `to_raw`/`ordinal` become `from_id`/`id`, plus one private `slot_byte` in `packets/play/entity.rs` because that packet packs the number into a byte with a continuation bit and the narrowing should happen once. `EquipmentSlot::MainHand` becomes `Mainhand`, and `OffHand` becomes `Offhand`. Mojang's constants are `MAINHAND` and `OFFHAND`, one word each; `MainHand` needs a dictionary the generator does not have, and hand-maintaining an exception list is the thing this PR removes. Two call sites in `crates/hyperion/src/simulation/inventory.rs`. ## Decisions on the hand-written enums this PR does *not* generate Listed because a hand-written type nobody has decided about is the thing to avoid, not the existence of hand-written types. - **`ChatVisiblity`, `ParticleStatus`** — generatable, blocked. Neither class is in the decompiled tree: `nix/minecraft-data.nix` selects classes referenced *one hop* from `net/minecraft/network`, and both are reached only via `net.minecraft.server.level.ClientInformation`, which is itself one hop out. Filed as ENG-10940. The extractor sees the gap today — both degrade to a bare `varint` noted `enum ordinal`, with the class name dropped. - **`StringArgumentKind`** — `com.mojang.brigadier.arguments.StringArgumentType$StringType`. Brigadier is a separate artifact and the decompile covers `net/minecraft` only. Staying hand-written; the reason is now at the type. - **`NamedColor`, `Decoration`** — `ChatFormatting`, but travelling as JSON/NBT *names* rather than numbers, and `Decoration` is a reordered subset. Their truth lives in DFU `MapCodec`s, which this extractor cannot read at all. Staying hand-written; see the note on the text module in the enumeration. - **`EntityDataSerializer`** — not a Java enum. `EntityDataSerializers` is a static registry of fields, numbered by `registerSerializer` call order, so it needs a different reader. Not attempted. - **`ComponentType`** — ENG-10872, three enums over one registry, blocked on a hand-transcribed `shape()` whose variant order is load-bearing. - **`HumanoidArm`** — the pure duplicate. `build.rs` already emits it into `OUT_DIR/types.rs`; the copy in `packets/configuration.rs` exists because that module was written in parallel with the generator and landed first. Deliberately left for its own change, because `packets/mod.rs` documents **eleven packets defined twice for exactly the same reason** and `HumanoidArm` is one thread of that knot rather than a separate cleanup. Filed as ENG-10941 with the eleven named. ## Checks `cargo test --workspace`, `cargo clippy --workspace --all-targets --all-features -- -D warnings` (CI's exact command) and `cargo fmt --all --check` all green. `nix build .#checks.aarch64-darwin.{minecraft-proto-coverage,minecraft-proto-json,minecraft-proto-generated,minecraft-encoder-fixtures,tool-paths}` all green. `minecraft-literals` is **red on `main`** and not from this branch: `events/smash/tests/visuals.rs` gained `minecraft:entity.player.hurt_on_fire` in #1035, one over the baseline of 322. Untouched here, reported rather than fixed. Co-authored-by: agent <agent@ix.dev>
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1038 +/- ##
==========================================
+ Coverage 52.61% 54.08% +1.46%
==========================================
Files 337 347 +10
Lines 30819 32497 +1678
Branches 1194 1234 +40
==========================================
+ Hits 16216 17576 +1360
- Misses 14339 14643 +304
- Partials 264 278 +14 see 29 files with indirect coverage changes 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
play/clientbound/minecraft:boss_eventhas been extracted as carrying no bytes for this whole protocol version. It writes a UUID, an enum ordinal and a variable body.Nothing caught it, and nothing here could have. A refusal is visible; a wrong answer is not. The packet was
complete: true, absent fromcoverage.incomplete, with no reason recorded anywhere — the same shape as a packet the generator got right. Every hand-written boss-bar type inpackets/play/player.rs, including the two ordinal enums at line 1023, exists because of this one silently wrong extraction that our own coverage measurement reported as covered.Root cause
Analyzer.method_bodyresolvedFoo::writeby regex-scanning the class body text for the firstwrite(followed by a block. A decompiled class body carries its nested and anonymous classes inline, so the scan cannot tell whose method it found — and the first match is not even reliably the outer one.ClientboundBossEventPacketdeclares an anonymousOperationat line 27 whosewriteis empty, textually above its ownwriteat line 78. Zero fields becameunitbecamecomplete: true.What changed
methods()only sees declarations at brace depth zero. A member of a nested or anonymous class is never mistaken for the outer class's own. This also let two other layouts through:level_chunk_with_lightnow resolves down toHeightmap$Types#STREAM_CODEC(which is, separately, the source of truth for the hand-writtenHeightmapKind), andupdate_recipespastSelectableRecipe.noRecipeCodecto the dispatch underneath.An empty layout can no longer be inferred.
read_encode_bodyreturning zero fields is now a refusal. The only empty layout is one Mojang spellsStreamCodec.unit(..). This is a no-op on today's data, which is exactly the point: the silent path is gone rather than merely unoccupied.coverage.emptyrecords every packet claimed to carry no bytes, and each is warranted independently of the code path that produced it —StreamCodec.unit(..)has to appear in the packet's own source or in the expression it was registered with, which is where the bundle delimiter's lives. A packet that comes out empty without that fails the extraction by name.The baseline holds the cause of each refusal, not only its id. This is the part that matters. The refusal count did not move when this bug was introduced and would not move if it came back, so a ratchet over ids alone measures the generator's honesty rather than its correctness.
The four
custom_payloadrefusals are carved out intocoverage.opaque. A channel id followed by bytes whose meaning belongs to whoever registered the channel has no layout in the server either. Counting them as gaps made the remaining number mean "work left, plus four that are finished". The carve-out is checked, not asserted: an id that stops being refused, or starts being refused for a different reason, fails the extraction rather than hiding a real gap behind a finished one.Numbers, with composition
Gaps 62 → 59: boss_event added (+1), the four
custom_payloadentries carved out (−4). The 59 are 19unmodelled statement, 14 runtime-dispatched union, 7 conditional presence, 5DataComponentPatch, 5 optionalItemStack, 4 loop over a sequence, 4 packed bitfield, 1 other. 16 layouts are declared empty byStreamCodec.unit, all warranted. Generated packet structs 180 → 179; the one that vanished is the boss bar packet's empty struct, which was never a packet.Guards broken, and what they said
Six mutations. Each was checked to have applied (
assert t.count(old) == 1, then grep for the replacement in the file) and to parse (ast.parse) before being run, because a mutation that does not apply and one that does not compile are both indistinguishable from a green run.A — remove the depth filter from
methods(). The interesting one, because it shows why the cause is held:The count is 63 both before and after. The id set is identical. The old ratchet would have passed.
B — revert both the depth filter and the inferred-empty refusal, i.e. the code exactly as it is on
main. The extractor refuses to finish:205 / 51aremain's own numbers. That is what this looked like from the outside for the whole version.C — a packet starts claiming an empty layout (
coverage.emptygains an entry). Exit 1:D — a packet stops being empty. Exit 1,
NO LONGER EMPTY play/serverbound/minecraft:player_loaded, baseline is looser than the code.E — a carve-out's phrase stops matching its refusal. Exit 1, four times:
F — a carve-out names a packet that is not refused. Exit 1:
All six restored afterwards; the extractor reproduces the committed
protocol.jsonbyte for byte andnix build .#checks.aarch64-darwin.minecraft-proto-jsonpasses.The blind spot this does not close
A layout that recovers some fields and silently drops others is still invisible. Only the zero-field case is now impossible, and only the zero-field case is warranted against the source. Closing the general case means round-tripping every
complete: truepacket through Mojang's ownSTREAM_CODECinnix/java/VanillaEncoder.javaand comparing byte for byte — which needs a valid instance of each of 179 packet classes constructed in Java, and that is not affordable in this change. Filed as ENG-10930 with this reasoning. Nothing here should be read as the class being closed; it is one instance fixed and one sub-case made impossible.Checks
cargo test --workspacegreen.cargo clippy --workspace --all-targets --all-features -- -D warnings(CI's exact command) green.nix build .#checks.aarch64-darwin.{minecraft-proto-coverage,minecraft-proto-json,tool-paths,minecraft-literals}all green. No generated path moved, sotool-pathshad nothing new to catch, but it was run.