ci: enforce the flake checks, by name, instead of exempting all of them - #1007
Merged
Conversation
Benchmark Results for generalComparing to 8c04c38 |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1007 +/- ##
=======================================
Coverage 47.17% 47.17%
=======================================
Files 238 238
Lines 27097 27097
Branches 1087 1087
=======================================
Hits 12784 12784
Misses 14077 14077
Partials 236 236 🚀 New features to boost your workflow:
|
`nix flake check` ran under `continue-on-error: true`, so no nix gate this repository has landed had ever been built by CI. The last green-looking run on main, 30340104855, reported zero of 41 checks as passing and the job was still advisory. ENG-10817. ## Why nothing built The daemon decides whether a content-addressed derivation can be realised, and cargoUnit content-addresses every crate unit. The flake's own `nixConfig.extra-experimental-features` and a workflow-level `NIX_CONFIG` both reach the client only. Run 30341528713 measured the three side by side on a one-line CA derivation: baseline fails, client-only fails, `extra-conf` on the installer action passes. Hence the job's own error, `store path '...bedwars-0.1.0.drv' does not exist`, which points nowhere near the cause. The gate now checks this itself and says what to set. ## Real versus environmental, measured per check With the daemon fixed, run 30341722090 built all 41 checks individually on ubuntu-latest. 38 passed. The 3 that failed were the sandboxed end to end gates, all with the same line: the server builds a reqwest client at startup and reqwest will not construct one without a trust store. A darwin build finds the keychain and a NixOS host often leaks /etc/ssl into the sandbox, so those gates only ever looked green on the machines they were written on. That is a real defect and nix/e2e.nix names the CA bundle now. `differential-traces` failed once, on run 30341066882, then passed four times: run 30341722090 plus three independent repeats in 30342250503. The only difference was ca-derivations, so it was the store and not the check. Every package builds too, 19 of 19 in each of three runs. The comment this replaces said `packages` "cannot succeed yet" and that cargoUnit could not vendor this lock. `nix flake check` never built packages in the first place, it prints "(build skipped)" for each one, and they build. ## What replaces the exemption nix/ci/flake-gate.nix, subtractive: every attribute of `checks` is built unless it is named in `excluded`, so a check added tomorrow is enforced the day it lands. `nixos-modules` moved into the per-system set so nothing sits outside the gate's view, and the gate reads its own names with `builtins.attrNames`, which is how three checks that landed in sibling PRs while this branch was open became enforced with no edit. The exclusion list is empty. An entry is the evidence that set it, never a justification: the gate prints that text, builds the check anyway, and fails if it passes. A misspelled exclusion throws at evaluation. Both guards were watched failing, in CI and not only locally. Run 30342733873 had `differential-traces` excluded while it in fact passed, and reported `STALE differential-traces` with the entry to delete. A deliberate argv reordering in the game server module, PR 1008, turned the job red with `hyperion NixOS module argv lost: /bin/bedwars --ip :: --port 35565` and was reverted. That first full run also caught a real regression nothing else would have: `smash-e2e` failed 47 of 47 cooldowns because the action bar had become an NBT compound the match client could not read. It was red on main for two commits. #1006 fixed the reader; ENG-10838 has the write-up. runs-on stays hosted against the house rule. The fleet cannot serve this org: ix `crates/ci/dispatcher/src/config.rs` holds `GithubApp.org` as one `String` and registers every runner at `orgs/indexable-inc`. ENG-10824. Authored by an AI agent (Claude Opus 4.5 via Claude Code).
andrewgazelka
force-pushed
the
eng-10817-self-hosted-nix-gates
branch
from
July 28, 2026 09:23
430e846 to
3a30234
Compare
Benchmark Results for generalComparing to 4779844 |
andrewgazelka
added a commit
that referenced
this pull request
Jul 28, 2026
…1012) ## The bug Twelve of the fifteen Super Smash Mobs kits left hotbar slot 0 empty and numbered their abilities from 1. A vanilla client selects slot 0 on its own when it reaches the world and no packet in the join sequence changes that, so twelve kits handed a player a bar whose first ability could not be fired until they scrolled. An operator saw it in a real client: an empty first square, then a sword, an axe and a block. Nothing failed. Every ability was present, correctly bound and reachable, and no gate had an opinion about which key a hand starts on. ## The layout, and why it is a shift Enumerated from source, all fifteen kits already declared their abilities in ascending slot order with the ultimate last, and the three that were right (Enderman, Skeleton, Slime) were exactly the three whose first ability was numbered 0: ``` enderman 0 1 2 8 ok skeleton 0 1 2 8 ok slime 0 1 8 ok blaze 1 2 8 no slot 0 guardian 1 2 3 8 iron_golem 1 2 3 8 ...twelve in all ``` Slot 8 was already consistent across the whole roster, so the far-right convention for the Smash Crystal was understood and simply not applied at the left end. Every kit's starting abilities were contiguous, just starting from the wrong number. So the whole set was one key to the right of where it belonged and the fix is a shift. The alternative reading, that slot 0 was reserved for a melee weapon not yet implemented, does not hold: `KitStats` documents that a kit hits for the same amount with any item including a fist, nothing in `kit.rs`, `server.rs` or `adapter.rs` has any notion of a weapon item, and `Op::SetHotbar` clears the inventory and refills it purely from abilities, so nothing could ever have occupied slot 0. ## Why the values were not the whole problem There is nothing a per-ability `slot: u8` can be checked against on its own. `slot: 1` is wrong only in relation to the rest of its kit, and the doc comment on `AbilitySpec` showed a `slot: 1` (Enderman's Blink, genuinely its second ability) that new kit files then copied as their *first*. That is the shape that let twelve kits drift. So the field is gone. `KitBuilder::ability` hands out slots in declaration order from 0 and `KitBuilder::ultimate` always takes `ULTIMATE_SLOT`. The order a kit file declares its abilities in *is* the layout. Fifty-one hand-written numbers deleted, and the empty first key is now unreachable rather than merely absent today. Declaring more starting abilities than fit to the left of the ultimate is an assert at module init rather than an ability nobody can press. ## The gates `events/smash/tests/hotbar.rs` sweeps `kit::registry` and `ability::manifest`, following the pattern `tests/sound.rs` set, so a kit added tomorrow is covered the moment its module runs. Four invariants, and each earns its place: * **slot 0 is filled** on every kit. The operator's bug, stated as itself, so a failure log reads "Blaze leaves slot 0 empty" rather than a puzzle about sets. * **starting abilities run from 0 with no holes.** Contiguity and not merely "starts at 0", because a gap is a dead key in the middle of the bar and two or three abilities have no business being spread across nine keys. * **every ultimate sits in the last slot.** It is the one binding a player carries across every mob, which is worth nothing if a kit can move it. * **no slot is off the end of the bar.** Worth having independently: this is the one that catches a typo, and `set_hotbar` returns an error the adapter discards, so an ability in slot 12 is silently absent rather than loud. Plus two that read one layer out: playing every kit on a real player and reading `kit::hotbar`, which is the function `push_stale_hotbars` calls, because a perfect declaration can still be dropped on instantiation; and granting a Smash Crystal, which reaches the bar through a different path from `kit::apply`. `nix run .#smash-hotbar-e2e` proves it over the protocol. One client stands in the hub (the lobby needs four to start a countdown), changes kit fifteen times, and holds the `ClientboundContainerSetSlot` packets it receives against the server's own `/abilities` output: slot 0 filled, keys exactly the ones the registry names, items matching, no holes, and slot 8 empty until a crystal is picked up. Seventy-six checks. Framing comes from `tools/client-26.2.py` by way of `smash-match.py`, the same route every other scripted client takes. Ports default off `smash-identity-e2e`'s, so it does not collide with the pair already sharing +4000 (ENG-10834). ### Watching them fail Reintroducing the bug as `next_slot: 1`: ``` test starting_abilities_fill_the_left_of_the_bar_with_no_holes ... FAILED a kit's starting abilities must occupy slot 0 upwards with no gaps: ["Blaze lays out {1, 2}, wanted {0, 1}", ... all fifteen] test every_kit_puts_something_on_the_key_a_hand_rests_on ... FAILED these kits leave hotbar slot 0 empty, so a player who spawns and right-clicks fires nothing: ["Blaze", "Chicken", "Cow", ... all fifteen] test playing_any_kit_hands_out_a_bar_that_starts_at_slot_zero ... FAILED ``` The same change against the e2e gate, which is the one that reads the wire: ``` FAIL Blaze fills slot 0, the key a client has selected when it spawns (bar: 1:minecraft:iron_sword, 2:minecraft:iron_axe) FAIL Blaze runs from slot 0 upwards with no holes: [1, 2] ... 30 failures, rc=1 ``` Making `next_slot` step by 2 fires the contiguity check only, and raising `ULTIMATE_SLOT` past `HOTBAR_SLOTS` fires the range check only, so each invariant was watched failing on its own cause. ## What depended on the old numbering Three things, and two of them would have failed quietly: * `tools/smash-match.py --ability-slot` defaulted to 3 for Iron Golem's Seismic Slam, now slot 2. A stale value produced *no message at all*: an empty slot has no registry entry to disagree with, so the check pressed a key holding nothing, saw no motion, and reported a broken knockback model. It is now a recorded failure the report reads, since the old complaint was appended to a list already folded into a proof by the time the match phase runs. * `tests/modularity.rs` asked slot 3 for a ground requirement and got `Ok(())`, which is what `activate` returns for a slot holding nothing. Its cooldown test passed on slot 1 while measuring a different ability than it named. * `tests/verification.rs` drives four test-local kits by slot. Skeleton, one of the three already correct, is deliberately left on slot 1 there: slot 0 is Barrage, which charges rather than firing, so a bare `use_slot` on it never reaches the cooldown that test is about. ## Verdict on the API shape The values were wrong, but so was the shape. A per-ability slot number with no notion of the kit's layout is unfalsifiable in isolation, and the doc example made the wrong value the default thing to copy. Deriving the layout from declaration order is what makes the class impossible rather than merely gated, and it costs a kit author nothing: no kit in the roster wanted a non-contiguous bar. `Slot` stays a plain component and not a relation. A slot is a scalar property of an ability, not a reference to another entity: there is no slot entity to point at, and no sharing for a relation to earn its keep on, unlike `(PlaysOnCast, sound)` where the sound really is interned and shared across abilities. Making it a relation would mint nine singletons to encode a `u8` and turn `granted_in_slot`, which runs on every right-click, from a component read into a target lookup. The layout is likewise not stored on the kit: walking `(Grants, ability)` already derives it, and a second copy is a thing that can disagree. ## Verified locally Rebased onto `b5ce627`, which closed ENG-10817 and made every attribute of `checks` enforced in CI subtractively. So `checks.smash-hotbar-e2e` and `checks.smash-hotbar-e2e-app` are enforced the day they land, with no edit to `nix/ci/flake-gate.nix`, which is what `nix eval` confirms: ``` $ nix eval --json .#checks.aarch64-darwin --apply \ 'c: builtins.filter (n: builtins.match ".*hotbar.*" n != null) (builtins.attrNames c)' ["smash-hotbar-e2e","smash-hotbar-e2e-app"] ``` The `+6000` port offset is for `nix run` only; the sandboxed check picks a free port, so it cannot collide with the pair already sharing `+4000` (ENG-10834). Run locally on aarch64-darwin, after the rebase: * `nix run .#test`: 636 passed, 1 skipped (the count moved with #1010 and #1011 deleting crates on main; it was 656 before the rebase) * `nix run .#lint`: clean * `nix run .#fmt`: clean * `nix run .#smash-hotbar-e2e`: RESULT ok, 0 checks failed, 76 passes * `nix run .#smash-e2e`: RESULT a whole match ran at protocol 776, rc=0. This is the gate whose `--ability-slot` moves, and it proved knockback from an ability on the new slot: P4 took (-2.227, 0.646, -2.227) blocks/tick, which solves to the same strength horizontally and vertically and so matches the model rather than a double impulse. Not done, and worth knowing: * The mutation gate (`nix run .#mutants`) was not run. * The other four e2e gates were not re-run, since nothing here touches their paths. `smash-e2e` was, because this moves its `--ability-slot`. * Neither e2e gate was run on linux. CI will be the first to do that, and per #1007 the sandboxed smash gates do now work there. * `cargo test -p smash` segfaulted once under parallel test threads, in `game_flow`, and passed on the next run and every run with `--test-threads=1`. Pre-existing flake in flecs registration, not caused by this change, and unfiled: it is outside this change's scope. Authored by Claude Code (Opus) at the operator's direction.
andrewgazelka
added a commit
that referenced
this pull request
Jul 28, 2026
## The gap, measured `crates/hyperion-minecraft-proto/protocol.json` carried **65 entries marked `complete: false`**. Each one is a packet or a data component whose byte layout the extractor could not recover in full, which means somebody hand-writes it or the feature does not exist. Nothing anywhere reported the number, so nobody knew there were 65. One of them was `minecraft:interact`: no entity interaction of any kind worked in this game until #1006 found it by hand. Grouped by the reason that stopped each one: | n | cause | | --- | --- | | 15 | runtime-dispatched union (`StreamCodec.dispatch`, registry dispatch) | | 13 | a helper `x.write(buf)` whose own body was itself unresolved | | 6 | optional `ItemStack` (`if (itemStack.isEmpty())`) | | 6 | conditional presence, an `if` that is really an `Option` or a small union | | 5 | `DataComponentPatch` add and remove split | | 4 | booleans packed into one bitfield byte | | 4 | loop over a fixed or length-known sequence | | 4 | `CustomPacketPayload`, an identifier plus trailing opaque bytes | | 3 | `Vec3#LP_STREAM_CODEC`, the quantised velocity vector | | 3 | one-off unmodelled statements | | 2 | two singletons (`potion_contents`, `update_recipes`) | Two things about that table are worth more than the counts. **The 13-entry bucket was not a cause at all, it was a reporting hole.** `_read_nested_write` recursed into the helper, and when the helper did not resolve it returned `None`, so the reason printed named the outer call site and the real cause was discarded. Thirteen of the 54 partial packets said nothing about why they were partial. **The #1006 diagnosis was wrong in an instructive way.** That PR reported the generator "had everything it needed and refused anyway" over one statement in the `Vec3` LP codec. It did not. `net.minecraft.network.LpVec3.write` is one byte for a near-zero vector, otherwise six bytes carrying three 15-bit quantised components and a two-bit shared scale, plus a trailing `VarInt` when the scale overflows two bits. No field list describes that and no amount of statement modelling recovers it. The refusal was correct. What was missing was a way to say so about **one field**, because this crate has implemented that exact codec all along, in `packets/play/entity.rs`. ## What this changes **A field can be delegated to a hand-written codec.** `protocol.json` gains `{"kind": "custom", "codec": <name>}`. `CUSTOM_CODECS` in `nix/extract-protocol.py` names the Java codecs it applies to; `CUSTOM_CODECS` in `build.rs` names the Rust type and the `#[proto(with = ...)]` module that owns those bytes. Nothing about the layout is asserted, so unlike the existing `CODEC_FIELDS` table this one cannot silently disagree with the source about a byte order. It claims only that the codec exists and that Rust owns it. Held together in both directions, and both watched failing: ``` $ nix build .#packages.aarch64-darwin.minecraft-protocol # after renaming the Java member > modelled Java names that no longer exist: > net.minecraft.world.phys.Vec3.LP_STREAM_CODEC_RENAMED $ cargo build -p hyperion-minecraft-proto # after renaming the Rust row thread 'main' panicked at crates/hyperion-minecraft-proto/build.rs:969:25: protocol.json names the custom codec `lp_vec3`, which build.rs has no Rust type for; add it to CUSTOM_CODECS ``` **Refusals name the field.** `unresolved_reasons` threads a field path, and a nested helper write now reports the reason from inside the helper rather than the call site. `minecraft:explode` went from `dispatched codec: the layout depends on a runtime type` to `blockParticles[].value.particle: dispatched codec ...`, which is a different kind of sentence: it names work. `minecraft:merchant_offers` now points at `offers[].itemCostA.components[]`. **27 protocol enums stopped being bare varints.** `ByteBufCodecs.idMapper` on an enum's own codec was emitting `{kind: varint}`, which is why 29 enums including `GameType`, `Difficulty`, `DyeColor`, `Rarity`, `InteractionHand` and `EquipmentSlotGroup` had to be retyped by hand downstream. The extractor now follows the key extractor (`GameType::getId`, `hand -> hand.id`, `Enum::ordinal`) back to the constructor argument the ids come from and reads them. Read, not assumed: `Rabbit.Variant.EVIL` is 99, not its ordinal 6. The two that still refuse do so honestly. `EquipmentSlot` takes its ids from one of two constructors and this reader cannot tell which constant used which; `TropicalFish.Pattern` computes its id rather than storing it. Both fall back to a varint rather than guess. **The gap is now visible and bounded.** `nix flake check .#minecraft-proto-coverage` prints the remaining gap grouped by cause on every run, pass or fail, and holds the list of ids against `nix/proto-coverage-baseline.json`. A ratchet, not a target of zero, and it fails in both directions: a new gap is a regression, and a closed one means the baseline has stopped bounding anything. `nix run .#sync-minecraft-proto` writes the baseline, so tightening it is the same command as regenerating what it describes. Because #1007 landed, this is enforced in CI the day it merges with no edit to `nix/ci/flake-gate.nix`. ### Watching the ratchet fail, on the real regression Deleting the `CUSTOM_CODECS` entry, which is exactly the state that lost `minecraft:interact`: ``` 62 layouts not recovered in full, by cause: 18 unmodelled statement 13 runtime-dispatched union 7 conditional presence ... NEW GAP play/clientbound/minecraft:add_entity movement: unmodelled statement: double z NEW GAP play/clientbound/minecraft:set_entity_motion movement: unmodelled statement: double z NEW GAP play/serverbound/minecraft:interact location: unmodelled statement: double z 3 layout(s) stopped being recoverable. Each one is a packet that now has to be hand-written or does not exist, so fix the extractor rather than widening the baseline. ``` And the other direction, with two ids added to the baseline: ``` CLOSED play/serverbound/minecraft:interact 1 layout(s) are now recovered, so the baseline is looser than the code. Tighten it with: nix run .#sync-minecraft-proto ``` ## Hand-written code retired `AddEntity`, `SetEntityMotion` and `Interact` are generated and only re-exported from `packets/play/entity.rs`. The verification is not that they compile: `add_entity_matches_the_server` and `set_entity_motion_matches_the_server` compare the generated codecs against bytes Mojang's own encoder printed, and pass unchanged. `interact_is_generated_and_round_trips` is new, and pins the byte count so a `location` that silently became three `f64`s would fail rather than go quiet. `GameType` is generated. The `byId` helpers and a `Default` stay hand-written in `play_login.rs`, deliberately: which variant is the default and how an out-of-range id resolves are facts about the game, not about the wire, and the generator only reads layouts. `crates/hyperion/src/simulation/gamemode.rs` is untouched and its `wire_ids_match_game_type` test now checks the generated type. `tests/play_combat.rs` had `operation: 1`. It is `Operation::AddMultipliedBase` now, which is the whole point of the enum work showing up as a compile error in a test rather than as a wrong byte on the wire. ## Judgements, including what I did not do **Refusal is not granular in the second sense the brief asked about, and should not be.** A packet with one unreadable field is still refused whole rather than emitted with that field missing. A codec short by one field looks correct and desynchronises the stream; there is no honest partial packet to emit, which is what `build.rs`'s own header has always said. What the escape hatch changes is the cost of fixing one, from writing a packet to writing a table row, and the field-path reporting is what makes the row obvious to write. Emitting the packet with a required hand-written hook was the other option considered; it would turn 62 silent gaps into 62 build failures on a tree nobody touched, which is a different bug. **`#[repr(i32)]` on generated enums was considered and rejected.** The derive encodes an enum through a `match` to a varint, not an `as` cast, so a repr buys no correctness and costs size: the default lets rustc pick one byte where one byte will do, and the wire form is variable-length anyway, so there is no fixed wire size for the repr to match. If the derive ever switches to a cast, the repr becomes load-bearing and should be added then. **Decoding is already the single boundary.** The derive's generated `decode` is a `match` returning `Error::InvalidEnum` for anything unknown, with no `Unknown(u32)` catch-all, so every generated protocol enum is closed inside the server and checked exactly once at the edge. That was already true; this PR only widens how many types get it. **Not in this PR, and sequenced deliberately.** The registry-id work (sound events, entity types, blocks, items as generated enums) and the migration of the 659 raw `minecraft:...` literals in hand-written code are stages 2 and 3. Stage 1 is the foundation: the `custom` escape hatch, per-field reporting, and the ratchet. Landing it first is what makes the rest cheap, and keeps this diff reviewable. **Buckets I looked at and left.** `ItemStack.OPTIONAL_STREAM_CODEC` (6 entries) and `DataComponentPatch` (5) are the next-cheapest wins and both have Rust implementations already, but `Slot<'a>` implements `Encode` and not `Decode`, so taking them needs `build.rs` to emit encode-only packets first. That is real work rather than a table row, and it belongs in its own change. ## Gates Every gate is a flake check or a cargo invocation, run locally, exit codes read from files rather than from a pipeline. | gate | result | | --- | --- | | `checks.aarch64-darwin.minecraft-proto-coverage` | rc=0 (new) | | `checks.aarch64-darwin.minecraft-proto-generated` | rc=0 | | `checks.aarch64-darwin.minecraft-proto-json` | rc=0 | | `checks.aarch64-darwin.minecraft-registry-data` | rc=0 | | `checks.aarch64-darwin.minecraft-tag-data` | rc=0 | | `checks.aarch64-darwin.minecraft-tags-load` | rc=0 | | `checks.aarch64-darwin.minecraft-block-states` | rc=0 | | `checks.aarch64-darwin.minecraft-entity-types` | rc=0 | | `checks.aarch64-darwin.minecraft-encoder-fixtures` | rc=0 | | `nix eval .#checks.aarch64-darwin.flake-gate.drvPath` | rc=0 | | `cargo test --workspace` | rc=0, 117 suites | | `cargo clippy -p hyperion-minecraft-proto --all-targets` | rc=0 | Coverage moved from 202 mechanical packets to 205, and `build.rs` from 177 generated packet classes to 180. 65 incomplete entries down to 62. ### Two red gates on main that are not mine Both reproduce on a pristine `origin/main` checkout in the same worktree, which is how I know: ``` $ git checkout --detach origin/main && cargo fmt --all --check Diff in .../crates/hyperion/src/net/intermediate.rs:185 $ cargo clippy -p hyperion-hot-reload-demo-module error: this could be a `const fn` --> crates/hyperion-hot-reload-demo-module/src/lib.rs:68:1 ``` Filed as ENG-10848. Not fixed here: `intermediate.rs` is a sibling agent's area this week and the charter keeps me inside `crates/hyperion-minecraft-proto/` and the generator. --- AI-authored (Claude) under human direction.
This was referenced Jul 28, 2026
andrewgazelka
added a commit
that referenced
this pull request
Jul 28, 2026
## Two implementations of one concept, down to one
`generated::registry::EntityType` arrived in the previous PR as one of 94
registry enums. `entity_type::EntityType` was already here, generated by its
own script into its own shape. Both described `minecraft:entity_type`, both
were generated from the same `protocol.json`, and `tests/entity_type.rs`
existed solely to check they agreed -- a test whose necessity was the defect.
This deletes the older one. What goes with it:
| deleted | lines |
| --- | --- |
| `crates/hyperion-minecraft-proto/src/entity_type.rs` | 544 |
| `nix/generate-entity-types.py` | 171 |
| `crates/hyperion-minecraft-proto/tests/entity_type.rs` | 51 |
| `entityTypeCodegen`, `generatedEntityTypes`, `syncEntityTypesScript`, `entityTypesUpToDate` in `nix/minecraft-data.nix` | 47 |
| `minecraft-entity-types` check, `sync-minecraft-entity-types` app, `minecraft-entity-types-rust` package in `flake.nix` | 3 |
One generator fewer, one nix check fewer, one sync command fewer, and the
staleness of what remains is covered by `minecraft-proto-generated`, which
already diffs the whole `src/generated` tree.
## What a caller writes now
`EntityType::PIG` is `EntityType::Pig`. 121 call sites, all but two of them in
`crates/hyperion/src/simulation/entity_kind.rs`, which is the table pairing a
flecs `EntityKind` tag with the vanilla type it spawns as.
The rename was not done with a SCREAMING-to-Pascal regex. It reads the variant
list out of the generated file and maps each old constant to the variant the
generator actually emitted, so a name whose conversion is not the obvious one
fails loudly rather than silently producing an identifier that happens to
compile as something else. Every one of the 121 mapped; the script reported no
unmapped names.
The rest of the surface:
| was | is |
| --- | --- |
| `entity_type(name)` | `EntityType::from_name(name)` |
| `ENTITY_TYPES` | `EntityType::ALL` |
| `ENTITY_TYPE_COUNT` | `EntityType::COUNT` |
| `hyperion_minecraft_proto::entity_type::EntityType` | `...::generated::registry::EntityType` |
`id()` and `name()` are spelled the same and mean the same, so no call site of
either changed.
## What this costs, in bytes
The retired struct was a `&'static str` and an `i32`, which with padding is 16
bytes, and every copy of an `EntityKind`'s entity type moved a pointer. The
enum is one byte: 158 entries fit in a `u8` discriminant, and that discriminant
is the network id. `Option<EntityType>` -- which is what
`EntityKind::entity_type` returns for the four kinds this version has no type
for -- is also one byte, because the closed enum leaves 98 unused values for
the niche.
`from_name` went from a linear scan over 158 entries to a binary search, though
that is the least interesting part: the only caller is `EntityKind::named`,
which runs when a name arrives from outside.
## The pins the deleted test was carrying
`tests/entity_type.rs` did three things. Two of them -- the table matching the
registry, and every name resolving to its own entry -- are now structurally
impossible to get wrong, because there is one table rather than two, and
`tests/registry_enum.rs` already puts all 6854 names in the crate through
`from_name`.
The third was worth keeping and has moved rather than gone:
```rust
#[test]
fn the_entity_type_ids_are_the_26_2_ones() {
assert_eq!(registry::EntityType::Pig.id(), RegistryId(100));
assert_eq!(registry::EntityType::Player.id(), RegistryId(156));
}
```
Every other test in that file would pass against a table that was wrong in the
same way twice. `minecraft:entity_type` was renumbered between 1.20.1 and 26.2,
and the symptom of holding the old numbering is not an error, it is the entity
*next to* the one that was asked for appearing in the world. So is
`a_type_this_version_dropped_does_not_resolve`, which pins that
`minecraft:boat` is gone (split per wood in 1.21.2) and that `pig` without the
namespace is not a name.
## Gates
| gate | result |
| --- | --- |
| `cargo build --workspace` | rc=0 |
| `cargo clippy --workspace --all-targets` | rc=0 |
| `cargo fmt --all --check` | rc=0 |
| `cargo test --workspace` | rc=0 |
| `cargo test -p hyperion-minecraft-proto --test registry_enum` | rc=0, 13 tests |
| `nix eval .#checks.aarch64-darwin.flake-gate.drvPath` | rc=0 |
The flake-gate eval is the one that matters for the deletions: #1007 made CI
enforce the flake checks by name, so removing three attributes from `flake.nix`
would break the gate's own evaluation if any of them were still referenced.
They are not.
## What I did not do
**No literal migration.** `EntityKind::named` still takes a `&str`, and the
callers that hand it one still hand it one. That is stage 3 and the gate that
goes with it.
**`EntityKind` itself is untouched.** It is 125 flecs tags whose relationship
to `minecraft:entity_type` is a 125-arm match, and four of those arms have no
vanilla type at all (`Gui`, and three others named in the last arm). Collapsing
`EntityKind` into `EntityType` would be a real design change to the simulation
layer, not a rename, and it belongs to whoever owns that layer.
---
AI-authored (Claude) under human direction.
andrewgazelka
added a commit
that referenced
this pull request
Jul 28, 2026
refactor(proto): one EntityType, and it is the generated enum
## Two implementations of one concept, down to one
`generated::registry::EntityType` arrived in the previous PR as one of
94
registry enums. `entity_type::EntityType` was already here, generated by
its
own script into its own shape. Both described `minecraft:entity_type`,
both
were generated from the same `protocol.json`, and `tests/entity_type.rs`
existed solely to check they agreed -- a test whose necessity was the
defect.
This deletes the older one. What goes with it:
| deleted | lines |
| --- | --- |
| `crates/hyperion-minecraft-proto/src/entity_type.rs` | 544 |
| `nix/generate-entity-types.py` | 171 |
| `crates/hyperion-minecraft-proto/tests/entity_type.rs` | 51 |
| `entityTypeCodegen`, `generatedEntityTypes`, `syncEntityTypesScript`,
`entityTypesUpToDate` in `nix/minecraft-data.nix` | 47 |
| `minecraft-entity-types` check, `sync-minecraft-entity-types` app,
`minecraft-entity-types-rust` package in `flake.nix` | 3 |
One generator fewer, one nix check fewer, one sync command fewer, and
the
staleness of what remains is covered by `minecraft-proto-generated`,
which
already diffs the whole `src/generated` tree.
## What a caller writes now
`EntityType::PIG` is `EntityType::Pig`. 121 call sites, all but two of
them in
`crates/hyperion/src/simulation/entity_kind.rs`, which is the table
pairing a
flecs `EntityKind` tag with the vanilla type it spawns as.
The rename was not done with a SCREAMING-to-Pascal regex. It reads the
variant
list out of the generated file and maps each old constant to the variant
the
generator actually emitted, so a name whose conversion is not the
obvious one
fails loudly rather than silently producing an identifier that happens
to
compile as something else. Every one of the 121 mapped; the script
reported no
unmapped names.
The rest of the surface:
| was | is |
| --- | --- |
| `entity_type(name)` | `EntityType::from_name(name)` |
| `ENTITY_TYPES` | `EntityType::ALL` |
| `ENTITY_TYPE_COUNT` | `EntityType::COUNT` |
| `hyperion_minecraft_proto::entity_type::EntityType` |
`...::generated::registry::EntityType` |
`id()` and `name()` are spelled the same and mean the same, so no call
site of
either changed.
## What this costs, in bytes
The retired struct was a `&'static str` and an `i32`, which with padding
is 16
bytes, and every copy of an `EntityKind`'s entity type moved a pointer.
The
enum is one byte: 158 entries fit in a `u8` discriminant, and that
discriminant
is the network id. `Option<EntityType>` -- which is what
`EntityKind::entity_type` returns for the four kinds this version has no
type
for -- is also one byte, because the closed enum leaves 98 unused values
for
the niche.
`from_name` went from a linear scan over 158 entries to a binary search,
though
that is the least interesting part: the only caller is
`EntityKind::named`,
which runs when a name arrives from outside.
## The pins the deleted test was carrying
`tests/entity_type.rs` did three things. Two of them -- the table
matching the
registry, and every name resolving to its own entry -- are now
structurally
impossible to get wrong, because there is one table rather than two, and
`tests/registry_enum.rs` already puts all 6854 names in the crate
through
`from_name`.
The third was worth keeping and has moved rather than gone:
```rust
#[test]
fn the_entity_type_ids_are_the_26_2_ones() {
assert_eq!(registry::EntityType::Pig.id(), RegistryId(100));
assert_eq!(registry::EntityType::Player.id(), RegistryId(156));
}
```
Every other test in that file would pass against a table that was wrong
in the
same way twice. `minecraft:entity_type` was renumbered between 1.20.1
and 26.2,
and the symptom of holding the old numbering is not an error, it is the
entity
*next to* the one that was asked for appearing in the world. So is
`a_type_this_version_dropped_does_not_resolve`, which pins that
`minecraft:boat` is gone (split per wood in 1.21.2) and that `pig`
without the
namespace is not a name.
## Gates
| gate | result |
| --- | --- |
| `cargo build --workspace` | rc=0 |
| `cargo clippy --workspace --all-targets` | rc=0 |
| `cargo fmt --all --check` | rc=0 |
| `cargo test --workspace` | rc=0 |
| `cargo test -p hyperion-minecraft-proto --test registry_enum` | rc=0,
13 tests |
| `nix eval .#checks.aarch64-darwin.flake-gate.drvPath` | rc=0 |
The flake-gate eval is the one that matters for the deletions: #1007
made CI
enforce the flake checks by name, so removing three attributes from
`flake.nix`
would break the gate's own evaluation if any of them were still
referenced.
They are not.
## What I did not do
**No literal migration.** `EntityKind::named` still takes a `&str`, and
the
callers that hand it one still hand it one. That is stage 3 and the gate
that
goes with it.
**`EntityKind` itself is untouched.** It is 125 flecs tags whose
relationship
to `minecraft:entity_type` is a 125-arm match, and four of those arms
have no
vanilla type at all (`Gui`, and three others named in the last arm).
Collapsing
`EntityKind` into `EntityType` would be a real design change to the
simulation
layer, not a rename, and it belongs to whoever owns that layer.
---
AI-authored (Claude) under human direction.
andrewgazelka
added a commit
that referenced
this pull request
Jul 28, 2026
## The gate first, the migration second A `"minecraft:..."` string in Rust is an unproven claim. Nothing checks that the name exists, that this version still spells it that way, or that the registry it indexes is the one the caller meant, so a typo or a Mojang rename becomes a `None` on whichever tick that value was next needed. Every static registry is a closed enum since #1021, so the name can be a variant and the compiler answers all three questions at the line that asked. Fixing occurrences by hand does not stop the class. `nix flake check .#minecraft-literals` does, and it is the point of this change; the migration below is what it enabled, not what it is for. ## Counted from the source, not from a regex over lines The brief warned about this and it was warranted. A regex cannot tell `//! minecraft:pig` in a doc comment from `"minecraft:pig"` in code, and this repo's doc comments name registry entries constantly -- `entity_kind.rs` alone has dozens. `nix/check-minecraft-literals.py` walks each file the way Rust lexes it: line comments, nested block comments, char literals and lifetimes are stepped over, and raw strings (`r#"..."#`) keep their contents. What that finds across every file in `git ls-files '*.rs'`: | | files | literals | | --- | --- | --- | | generated, recognised by the `@generated` header | 102 | 10009 | | hand-written | 56 | 765 | 765, against the 659 an earlier pass measured. I did not reconstruct how that number was reached, so I cannot say which 106 it missed, only that a Rust-aware scan finds more. Generated files are exempted by their own header rather than by a path list, so a new generated file is covered the day it appears and a file that stops being generated stops being exempt. ## Two kinds of exemption, and they are different on purpose `ALLOWED` is a path with a reason: a file where these literals are correct and are expected to stay. Two entries, 430 names between them. - `crates/hyperion/src/net/protocol/registries.rs` -- the element names of the 29 **dynamic** registries. `protocol.json` does not carry those, so they have no enum. The file is a name table of the same kind as generated output and its own header already says it should be deleted once the proto crate generates dynamic registry contents. - `crates/hyperion-minecraft-proto/tests/registry_enum.rs` -- the test proving the enums cover the registries has to name entries as strings to check that the string form resolves, and names deliberate non-entries to check that they do not. The baseline is a path with a *list*: names that have not been migrated yet. It is debt with a name on it, it ratchets in both directions, and the intended end state is that it is empty and the baseline file is deleted. 386 at the start of this change, 323 after the migration below, and 327 after rebasing onto a main that had grown four more while this branch was open. I chose a ratchet over a hard failure for the remainder because most of them are in `events/smash/**` and `crates/hyperion/src/simulation/**`, where three other agents have open branches tonight. Migrating those files now would conflict with work in flight for no gain the gate does not already give: the gate refuses the *next* one either way. ## What was migrated, and why this one `minecraft:command_argument_type`, 63 names across two files, chosen because it was the largest single cluster and because it fixes something the brief names directly: **encoding could fail**. ```rust -fn argument_type_id(name: &str) -> Result<i32> { - let id = registry::COMMAND_ARGUMENT_TYPE - .id_of(name) - .ok_or_else(|| Error::InvalidIdentifier(name.to_owned()))?; - i32::try_from(id).map_err(|_| Error::InvalidIdentifier(name.to_owned())) -} ``` That was a linear scan over 57 entries, returning a `Result`, run for every argument of every command in the tree, so that this server could discover at run time whether `brigadier:double` still existed. `ArgumentType::to_id` is now `const fn` and infallible, and `ArgumentType::Empty` and `ArgumentType::Registry` hold a `CommandArgumentType` rather than a bare `RegistryId` -- so the id cannot be one the registry does not have, rather than being checked against it. `decode` changed in the same direction and is the interesting half: it used to resolve the incoming id to a *name* and then match on string literals, which is 57 string comparisons in the worst case, and silently fell through to `Self::Empty` for a name it did not recognise. It now resolves through `CommandArgumentType::from_id` once -- the single decode boundary -- and matches on variants. `crates/hyperion/src/simulation/command.rs` lost its own duplicate `argument_type_id`, and `empty_argument` went from `-> anyhow::Result<...>` to a `const fn`, which deleted a `?` from 43 call sites. Runtime validation sites on non-decode paths, by the command in the previous PR's body: 26 before, 22 after. The four removed are the two `argument_type_id` helpers and the two `id_of` calls inside them. The remaining 22 are enumerated in the baseline and in that list; several are legitimate -- the anvil world loader really is resolving a name it read out of a file -- and the worst is `events/smash/src/adapter.rs:800`, `ItemKind::from_str(bare).unwrap_or(ItemKind::Stick)`, which hands back a stick for a name nobody typed correctly. That file belongs to another agent tonight. ## The gate, broken in every direction and watched **A new raw name appears.** Added a second biome lookup to `translate.rs`: ``` 387 raw registry names in Rust string literals, 386 allowed by the baseline NEW crates/hyperion/src/simulation/blocks/translate.rs minecraft:sunflower_plains 1 raw registry name(s) that were not there before. Every static registry is a closed enum in hyperion_minecraft_proto::generated::registry, so name the variant instead: a typo or a Mojang rename is then a compile error here rather than a None at run time. If the name belongs to a dynamic registry and genuinely has no enum, add the file to ALLOWED in nix/check-minecraft-literals.py with the reason. ``` **Names disappear and the baseline stops bounding anything.** This is not a hypothetical; it is what the command-argument migration produced before the baseline was tightened: ``` 323 raw registry names in Rust string literals, 386 allowed by the baseline GONE crates/hyperion-minecraft-proto/src/packets/play/player.rs brigadier:double GONE crates/hyperion-minecraft-proto/src/packets/play/player.rs brigadier:float ... 85 lines ... 63 raw registry name(s) are gone, so the baseline is looser than the code and has stopped bounding anything. Tighten it with: nix run .#sync-minecraft-literals ``` **The claim about comments, tested rather than asserted.** Added a doc comment naming `minecraft:block_pos` and `"minecraft:vec3"`, and a block comment containing `"minecraft:item_stack"`, to `command.rs`. The count stayed at 323 and no `NEW` line appeared, which is the whole reason this is a lexer and not a `rg` invocation. A line-based check would have reported three regressions there. ## The gate caught something before it even merged While this branch was open, #1023 landed on `main`. Rebasing onto it, with no prompting and no edit to the checker: ``` 327 raw registry names in Rust string literals, 323 allowed by the baseline NEW events/smash/src/module/kits/blaze.rs minecraft:entity.player.hurt_on_fire NEW events/smash/src/module/kits/spider.rs minecraft:entity.player.hurt_sweet_berry_bush NEW events/smash/tests/abilities.rs minecraft:entity.arrow.shoot NEW events/smash/tests/contract.rs minecraft:entity.player.hurt_on_fire ``` Four sound names written as strings in a PR authored after the enums existed, which is the class this is meant to stop and a fair demonstration that it does not stop itself. They are in the baseline rather than migrated: those files have work in flight against them tonight and the gate refuses the *next* one either way. Had this check been on `main` an hour earlier, #1023 would have been asked to write `SoundEvent::EntityPlayerHurtOnFire` and the four would not exist. ## Gates | gate | result | | --- | --- | | `nix build .#checks.aarch64-darwin.minecraft-literals` | rc=0, 327/327 | | `nix eval .#checks.aarch64-darwin.flake-gate.drvPath` | rc=0 | | `cargo clippy --workspace --all-targets` | rc=0 | | `cargo fmt --all --check` | rc=0 | | `cargo test --workspace` | rc=0 | | `flake8 nix/check-minecraft-literals.py` | rc=0 | Because #1007 made CI enforce the flake checks by name, `minecraft-literals` runs in CI the day this merges with no edit to `nix/ci/flake-gate.nix`. ## What I did not do **327 names are still raw.** Named above, bounded by the baseline, and left deliberately rather than overlooked. **The dynamic registries still have no enum.** 430 of the 765 hand-written literals are the one file that lists them, and generating them properly means teaching `nix/generate-registry-data.py` to emit enums the way `generate-rust.py` now does. That would delete the largest `ALLOWED` entry and is the obvious next step; it is not this PR. Filed as ENG-10887. **The worst remaining fallback is filed rather than fixed.** ENG-10884 covers `ItemKind::from_str(bare).unwrap_or(ItemKind::Stick)`, which is in another agent's file this week. **`nix flake check` was not run whole.** The three checks this change touches were built individually and `flake-gate` was evaluated, but the full check set compiles a server and I did not wait on it. ## One red gate here is not mine `cargo clippy --all-targets --all-features -- -D warnings` fails on `events/smash/tests/abilities.rs:595`, an unused `use flecs_ecs::prelude::*;`. It arrived with #1023, whose own Clippy check failed and which was merged anyway, so it is red on `main` independently of this branch. Removing that one line locally makes the whole workspace pass, which is how I know nothing here contributes to it. Left alone: that file has another agent's work in flight against it tonight. Worth recording how I nearly missed it. `cargo clippy --workspace --all-targets` reported clean three times on this tree, because the test target was cached and because it does not pass `-D warnings`. CI's invocation is `cargo clippy --all-targets --all-features -- -D warnings` and the difference turns a warning into a failure. Running anything less than CI's exact command is not a check. --- AI-authored (Claude) under human direction.
andrewgazelka
added a commit
that referenced
this pull request
Jul 28, 2026
## The gate first, the migration second A `"minecraft:..."` string in Rust is an unproven claim. Nothing checks that the name exists, that this version still spells it that way, or that the registry it indexes is the one the caller meant, so a typo or a Mojang rename becomes a `None` on whichever tick that value was next needed. Every static registry is a closed enum since #1021, so the name can be a variant and the compiler answers all three questions at the line that asked. Fixing occurrences by hand does not stop the class. `nix flake check .#minecraft-literals` does, and it is the point of this change; the migration below is what it enabled, not what it is for. ## Counted from the source, not from a regex over lines The brief warned about this and it was warranted. A regex cannot tell `//! minecraft:pig` in a doc comment from `"minecraft:pig"` in code, and this repo's doc comments name registry entries constantly -- `entity_kind.rs` alone has dozens. `nix/check-minecraft-literals.py` walks each file the way Rust lexes it: line comments, nested block comments, char literals and lifetimes are stepped over, and raw strings (`r#"..."#`) keep their contents. What that finds across every file in `git ls-files '*.rs'`: | | files | literals | | --- | --- | --- | | generated, recognised by the `@generated` header | 102 | 10009 | | hand-written | 56 | 765 | 765, against the 659 an earlier pass measured. I did not reconstruct how that number was reached, so I cannot say which 106 it missed, only that a Rust-aware scan finds more. Generated files are exempted by their own header rather than by a path list, so a new generated file is covered the day it appears and a file that stops being generated stops being exempt. ## Two kinds of exemption, and they are different on purpose `ALLOWED` is a path with a reason: a file where these literals are correct and are expected to stay. Two entries, 430 names between them. - `crates/hyperion/src/net/protocol/registries.rs` -- the element names of the 29 **dynamic** registries. `protocol.json` does not carry those, so they have no enum. The file is a name table of the same kind as generated output and its own header already says it should be deleted once the proto crate generates dynamic registry contents. - `crates/hyperion-minecraft-proto/tests/registry_enum.rs` -- the test proving the enums cover the registries has to name entries as strings to check that the string form resolves, and names deliberate non-entries to check that they do not. The baseline is a path with a *list*: names that have not been migrated yet. It is debt with a name on it, it ratchets in both directions, and the intended end state is that it is empty and the baseline file is deleted. 386 at the start of this change, 323 after the migration below, and 327 after rebasing onto a main that had grown four more while this branch was open. I chose a ratchet over a hard failure for the remainder because most of them are in `events/smash/**` and `crates/hyperion/src/simulation/**`, where three other agents have open branches tonight. Migrating those files now would conflict with work in flight for no gain the gate does not already give: the gate refuses the *next* one either way. ## What was migrated, and why this one `minecraft:command_argument_type`, 63 names across two files, chosen because it was the largest single cluster and because it fixes something the brief names directly: **encoding could fail**. ```rust -fn argument_type_id(name: &str) -> Result<i32> { - let id = registry::COMMAND_ARGUMENT_TYPE - .id_of(name) - .ok_or_else(|| Error::InvalidIdentifier(name.to_owned()))?; - i32::try_from(id).map_err(|_| Error::InvalidIdentifier(name.to_owned())) -} ``` That was a linear scan over 57 entries, returning a `Result`, run for every argument of every command in the tree, so that this server could discover at run time whether `brigadier:double` still existed. `ArgumentType::to_id` is now `const fn` and infallible, and `ArgumentType::Empty` and `ArgumentType::Registry` hold a `CommandArgumentType` rather than a bare `RegistryId` -- so the id cannot be one the registry does not have, rather than being checked against it. `decode` changed in the same direction and is the interesting half: it used to resolve the incoming id to a *name* and then match on string literals, which is 57 string comparisons in the worst case, and silently fell through to `Self::Empty` for a name it did not recognise. It now resolves through `CommandArgumentType::from_id` once -- the single decode boundary -- and matches on variants. `crates/hyperion/src/simulation/command.rs` lost its own duplicate `argument_type_id`, and `empty_argument` went from `-> anyhow::Result<...>` to a `const fn`, which deleted a `?` from 43 call sites. Runtime validation sites on non-decode paths, by the command in the previous PR's body: 26 before, 22 after. The four removed are the two `argument_type_id` helpers and the two `id_of` calls inside them. The remaining 22 are enumerated in the baseline and in that list; several are legitimate -- the anvil world loader really is resolving a name it read out of a file -- and the worst is `events/smash/src/adapter.rs:800`, `ItemKind::from_str(bare).unwrap_or(ItemKind::Stick)`, which hands back a stick for a name nobody typed correctly. That file belongs to another agent tonight. ## The gate, broken in every direction and watched **A new raw name appears.** Added a second biome lookup to `translate.rs`: ``` 387 raw registry names in Rust string literals, 386 allowed by the baseline NEW crates/hyperion/src/simulation/blocks/translate.rs minecraft:sunflower_plains 1 raw registry name(s) that were not there before. Every static registry is a closed enum in hyperion_minecraft_proto::generated::registry, so name the variant instead: a typo or a Mojang rename is then a compile error here rather than a None at run time. If the name belongs to a dynamic registry and genuinely has no enum, add the file to ALLOWED in nix/check-minecraft-literals.py with the reason. ``` **Names disappear and the baseline stops bounding anything.** This is not a hypothetical; it is what the command-argument migration produced before the baseline was tightened: ``` 323 raw registry names in Rust string literals, 386 allowed by the baseline GONE crates/hyperion-minecraft-proto/src/packets/play/player.rs brigadier:double GONE crates/hyperion-minecraft-proto/src/packets/play/player.rs brigadier:float ... 85 lines ... 63 raw registry name(s) are gone, so the baseline is looser than the code and has stopped bounding anything. Tighten it with: nix run .#sync-minecraft-literals ``` **The claim about comments, tested rather than asserted.** Added a doc comment naming `minecraft:block_pos` and `"minecraft:vec3"`, and a block comment containing `"minecraft:item_stack"`, to `command.rs`. The count stayed at 323 and no `NEW` line appeared, which is the whole reason this is a lexer and not a `rg` invocation. A line-based check would have reported three regressions there. ## The gate caught something before it even merged While this branch was open, #1023 landed on `main`. Rebasing onto it, with no prompting and no edit to the checker: ``` 327 raw registry names in Rust string literals, 323 allowed by the baseline NEW events/smash/src/module/kits/blaze.rs minecraft:entity.player.hurt_on_fire NEW events/smash/src/module/kits/spider.rs minecraft:entity.player.hurt_sweet_berry_bush NEW events/smash/tests/abilities.rs minecraft:entity.arrow.shoot NEW events/smash/tests/contract.rs minecraft:entity.player.hurt_on_fire ``` Four sound names written as strings in a PR authored after the enums existed, which is the class this is meant to stop and a fair demonstration that it does not stop itself. They are in the baseline rather than migrated: those files have work in flight against them tonight and the gate refuses the *next* one either way. Had this check been on `main` an hour earlier, #1023 would have been asked to write `SoundEvent::EntityPlayerHurtOnFire` and the four would not exist. ## Gates | gate | result | | --- | --- | | `nix build .#checks.aarch64-darwin.minecraft-literals` | rc=0, 327/327 | | `nix eval .#checks.aarch64-darwin.flake-gate.drvPath` | rc=0 | | `cargo clippy --workspace --all-targets` | rc=0 | | `cargo fmt --all --check` | rc=0 | | `cargo test --workspace` | rc=0 | | `flake8 nix/check-minecraft-literals.py` | rc=0 | Because #1007 made CI enforce the flake checks by name, `minecraft-literals` runs in CI the day this merges with no edit to `nix/ci/flake-gate.nix`. ## What I did not do **327 names are still raw.** Named above, bounded by the baseline, and left deliberately rather than overlooked. **The dynamic registries still have no enum.** 430 of the 765 hand-written literals are the one file that lists them, and generating them properly means teaching `nix/generate-registry-data.py` to emit enums the way `generate-rust.py` now does. That would delete the largest `ALLOWED` entry and is the obvious next step; it is not this PR. Filed as ENG-10887. **The worst remaining fallback is filed rather than fixed.** ENG-10884 covers `ItemKind::from_str(bare).unwrap_or(ItemKind::Stick)`, which is in another agent's file this week. **`nix flake check` was not run whole.** The three checks this change touches were built individually and `flake-gate` was evaluated, but the full check set compiles a server and I did not wait on it. ## Also in here: a stale reference in my own generator `nix/generate-rust.py` explained the `particle_type` carve-out by saying the particle enum is owned separately "the same way `nix/generate-entity-types.py` owns entity types". #1024 -- mine, two commits earlier -- deleted that script. So the comment justified a real decision by appealing to a precedent that no longer existed, which leaves the next reader either hunting a missing file or concluding the carve-out was arbitrary. It also named `nix/generate-particles.py` and `src/particle.rs` as though they were present. They are not on `main` yet. The reason for the carve-out is a fact about the data and is true today; the ownership is an arrangement that lands separately. The comment now says the first without asserting the second, and the reciprocal note in the generated `registry/particle_type.rs` changed with it, so the two cannot drift. Worth naming the general shape, because a gate in this very PR nearly caught it and did not. `tool-paths` only matches paths at the start of a string literal; this one was inside backticks in the middle of a sentence. A comment can name a file that does not exist and nothing anywhere objects. I have not widened the pattern -- matching every path-shaped substring in every comment would flag prose constantly -- so this stays a thing a reader catches, and one did. ## A note on what a green local clippy is worth `cargo clippy --workspace --all-targets` reported this tree clean three times while CI was failing, for two reasons that will recur. It does not pass `-D warnings`, so an unused import is a warning locally and an error in CI. And cargo caches per target, so a test target nobody touched is not re-checked and a warning that arrived with somebody else's merge stays invisible. `touch`ing the file made it appear at once. CI's invocation is `cargo clippy --all-targets --all-features -- -D warnings` (flake.nix:185). Anything less is not the same check, and this PR is verified with that one. Filed as ENG-10894 along with the red it exposed, which #1029 has since fixed. --- AI-authored (Claude) under human direction.
andrewgazelka
added a commit
that referenced
this pull request
Jul 28, 2026
…1025) feat(proto): a raw `minecraft:` name in Rust is now a build failure ## The gate first, the migration second A `"minecraft:..."` string in Rust is an unproven claim. Nothing checks that the name exists, that this version still spells it that way, or that the registry it indexes is the one the caller meant, so a typo or a Mojang rename becomes a `None` on whichever tick that value was next needed. Every static registry is a closed enum since #1021, so the name can be a variant and the compiler answers all three questions at the line that asked. Fixing occurrences by hand does not stop the class. `nix flake check .#minecraft-literals` does, and it is the point of this change; the migration below is what it enabled, not what it is for. ## Counted from the source, not from a regex over lines The brief warned about this and it was warranted. A regex cannot tell `//! minecraft:pig` in a doc comment from `"minecraft:pig"` in code, and this repo's doc comments name registry entries constantly -- `entity_kind.rs` alone has dozens. `nix/check-minecraft-literals.py` walks each file the way Rust lexes it: line comments, nested block comments, char literals and lifetimes are stepped over, and raw strings (`r#"..."#`) keep their contents. What that finds across every file in `git ls-files '*.rs'`: | | files | literals | | --- | --- | --- | | generated, recognised by the `@generated` header | 102 | 10009 | | hand-written | 56 | 765 | 765, against the 659 an earlier pass measured. I did not reconstruct how that number was reached, so I cannot say which 106 it missed, only that a Rust-aware scan finds more. Generated files are exempted by their own header rather than by a path list, so a new generated file is covered the day it appears and a file that stops being generated stops being exempt. ## Two kinds of exemption, and they are different on purpose `ALLOWED` is a path with a reason: a file where these literals are correct and are expected to stay. Two entries, 430 names between them. - `crates/hyperion/src/net/protocol/registries.rs` -- the element names of the 29 **dynamic** registries. `protocol.json` does not carry those, so they have no enum. The file is a name table of the same kind as generated output and its own header already says it should be deleted once the proto crate generates dynamic registry contents. - `crates/hyperion-minecraft-proto/tests/registry_enum.rs` -- the test proving the enums cover the registries has to name entries as strings to check that the string form resolves, and names deliberate non-entries to check that they do not. The baseline is a path with a *list*: names that have not been migrated yet. It is debt with a name on it, it ratchets in both directions, and the intended end state is that it is empty and the baseline file is deleted. 386 at the start of this change, 323 after the migration below, and 327 after rebasing onto a main that had grown four more while this branch was open. I chose a ratchet over a hard failure for the remainder because most of them are in `events/smash/**` and `crates/hyperion/src/simulation/**`, where three other agents have open branches tonight. Migrating those files now would conflict with work in flight for no gain the gate does not already give: the gate refuses the *next* one either way. ## What was migrated, and why this one `minecraft:command_argument_type`, 63 names across two files, chosen because it was the largest single cluster and because it fixes something the brief names directly: **encoding could fail**. ```rust -fn argument_type_id(name: &str) -> Result<i32> { - let id = registry::COMMAND_ARGUMENT_TYPE - .id_of(name) - .ok_or_else(|| Error::InvalidIdentifier(name.to_owned()))?; - i32::try_from(id).map_err(|_| Error::InvalidIdentifier(name.to_owned())) -} ``` That was a linear scan over 57 entries, returning a `Result`, run for every argument of every command in the tree, so that this server could discover at run time whether `brigadier:double` still existed. `ArgumentType::to_id` is now `const fn` and infallible, and `ArgumentType::Empty` and `ArgumentType::Registry` hold a `CommandArgumentType` rather than a bare `RegistryId` -- so the id cannot be one the registry does not have, rather than being checked against it. `decode` changed in the same direction and is the interesting half: it used to resolve the incoming id to a *name* and then match on string literals, which is 57 string comparisons in the worst case, and silently fell through to `Self::Empty` for a name it did not recognise. It now resolves through `CommandArgumentType::from_id` once -- the single decode boundary -- and matches on variants. `crates/hyperion/src/simulation/command.rs` lost its own duplicate `argument_type_id`, and `empty_argument` went from `-> anyhow::Result<...>` to a `const fn`, which deleted a `?` from 43 call sites. Runtime validation sites on non-decode paths, by the command in the previous PR's body: 26 before, 22 after. The four removed are the two `argument_type_id` helpers and the two `id_of` calls inside them. The remaining 22 are enumerated in the baseline and in that list; several are legitimate -- the anvil world loader really is resolving a name it read out of a file -- and the worst is `events/smash/src/adapter.rs:800`, `ItemKind::from_str(bare).unwrap_or(ItemKind::Stick)`, which hands back a stick for a name nobody typed correctly. That file belongs to another agent tonight. ## The gate, broken in every direction and watched **A new raw name appears.** Added a second biome lookup to `translate.rs`: ``` 387 raw registry names in Rust string literals, 386 allowed by the baseline NEW crates/hyperion/src/simulation/blocks/translate.rs minecraft:sunflower_plains 1 raw registry name(s) that were not there before. Every static registry is a closed enum in hyperion_minecraft_proto::generated::registry, so name the variant instead: a typo or a Mojang rename is then a compile error here rather than a None at run time. If the name belongs to a dynamic registry and genuinely has no enum, add the file to ALLOWED in nix/check-minecraft-literals.py with the reason. ``` **Names disappear and the baseline stops bounding anything.** This is not a hypothetical; it is what the command-argument migration produced before the baseline was tightened: ``` 323 raw registry names in Rust string literals, 386 allowed by the baseline GONE crates/hyperion-minecraft-proto/src/packets/play/player.rs brigadier:double GONE crates/hyperion-minecraft-proto/src/packets/play/player.rs brigadier:float ... 85 lines ... 63 raw registry name(s) are gone, so the baseline is looser than the code and has stopped bounding anything. Tighten it with: nix run .#sync-minecraft-literals ``` **The claim about comments, tested rather than asserted.** Added a doc comment naming `minecraft:block_pos` and `"minecraft:vec3"`, and a block comment containing `"minecraft:item_stack"`, to `command.rs`. The count stayed at 323 and no `NEW` line appeared, which is the whole reason this is a lexer and not a `rg` invocation. A line-based check would have reported three regressions there. ## The gate caught something before it even merged While this branch was open, #1023 landed on `main`. Rebasing onto it, with no prompting and no edit to the checker: ``` 327 raw registry names in Rust string literals, 323 allowed by the baseline NEW events/smash/src/module/kits/blaze.rs minecraft:entity.player.hurt_on_fire NEW events/smash/src/module/kits/spider.rs minecraft:entity.player.hurt_sweet_berry_bush NEW events/smash/tests/abilities.rs minecraft:entity.arrow.shoot NEW events/smash/tests/contract.rs minecraft:entity.player.hurt_on_fire ``` Four sound names written as strings in a PR authored after the enums existed, which is the class this is meant to stop and a fair demonstration that it does not stop itself. They are in the baseline rather than migrated: those files have work in flight against them tonight and the gate refuses the *next* one either way. Had this check been on `main` an hour earlier, #1023 would have been asked to write `SoundEvent::EntityPlayerHurtOnFire` and the four would not exist. ## Gates | gate | result | | --- | --- | | `nix build .#checks.aarch64-darwin.minecraft-literals` | rc=0, 327/327 | | `nix eval .#checks.aarch64-darwin.flake-gate.drvPath` | rc=0 | | `cargo clippy --workspace --all-targets` | rc=0 | | `cargo fmt --all --check` | rc=0 | | `cargo test --workspace` | rc=0 | | `flake8 nix/check-minecraft-literals.py` | rc=0 | Because #1007 made CI enforce the flake checks by name, `minecraft-literals` runs in CI the day this merges with no edit to `nix/ci/flake-gate.nix`. ## What I did not do **327 names are still raw.** Named above, bounded by the baseline, and left deliberately rather than overlooked. **The dynamic registries still have no enum.** 430 of the 765 hand-written literals are the one file that lists them, and generating them properly means teaching `nix/generate-registry-data.py` to emit enums the way `generate-rust.py` now does. That would delete the largest `ALLOWED` entry and is the obvious next step; it is not this PR. Filed as ENG-10887. **The worst remaining fallback is filed rather than fixed.** ENG-10884 covers `ItemKind::from_str(bare).unwrap_or(ItemKind::Stick)`, which is in another agent's file this week. **`nix flake check` was not run whole.** The three checks this change touches were built individually and `flake-gate` was evaluated, but the full check set compiles a server and I did not wait on it. ## Also in here: a stale reference in my own generator `nix/generate-rust.py` explained the `particle_type` carve-out by saying the particle enum is owned separately "the same way `nix/generate-entity-types.py` owns entity types". #1024 -- mine, two commits earlier -- deleted that script. So the comment justified a real decision by appealing to a precedent that no longer existed, which leaves the next reader either hunting a missing file or concluding the carve-out was arbitrary. It also named `nix/generate-particles.py` and `src/particle.rs` as though they were present. They are not on `main` yet. The reason for the carve-out is a fact about the data and is true today; the ownership is an arrangement that lands separately. The comment now says the first without asserting the second, and the reciprocal note in the generated `registry/particle_type.rs` changed with it, so the two cannot drift. Worth naming the general shape, because a gate in this very PR nearly caught it and did not. `tool-paths` only matches paths at the start of a string literal; this one was inside backticks in the middle of a sentence. A comment can name a file that does not exist and nothing anywhere objects. I have not widened the pattern -- matching every path-shaped substring in every comment would flag prose constantly -- so this stays a thing a reader catches, and one did. ## A note on what a green local clippy is worth `cargo clippy --workspace --all-targets` reported this tree clean three times while CI was failing, for two reasons that will recur. It does not pass `-D warnings`, so an unused import is a warning locally and an error in CI. And cargo caches per target, so a test target nobody touched is not re-checked and a warning that arrived with somebody else's merge stays invisible. `touch`ing the file made it appear at once. CI's invocation is `cargo clippy --all-targets --all-features -- -D warnings` (flake.nix:185). Anything less is not the same check, and this PR is verified with that one. Filed as ENG-10894 along with the red it exposed, which #1029 has since fixed. --- AI-authored (Claude) under human direction.
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.
Closes ENG-10817.
What was wrong
nix flake checkran undercontinue-on-error: true. No nix gate thisrepository has landed had ever been built by CI. The last green-looking run on
main, 30340104855,
reported 0 of 41 checks passing and the job was still advisory.
Why nothing built, measured
The daemon decides whether a content-addressed derivation can be realised, and
cargoUnit content-addresses every crate unit.
Run 30341528713
built a one-line CA derivation under three configurations:
CA-BUILD-FAILNIX_CONFIG: experimental-features = ... ca-derivationsCA-BUILD-FAILextra-conf: extra-experimental-features = ca-derivationsCA-BUILD-OKThe flake's own
nixConfigand a workflowNIX_CONFIGreach the client only,which is why the failure read as
store path '...bedwars-0.1.0.drv' does not exist. The gate re-checks this itself and prints what to set.Real versus environmental
Per check rather than as one all-or-nothing job,
run 30341722090,
41 checks:
minecraft-*up-to-date check,nixos-modules,genmap-url-pinnedanddifferential-traces.e2e,smash-e2e,completions-e2e, all on the same line.Real, not the runner. The server builds a reqwest client at startup and reqwest
will not construct one without a trust store; a darwin build finds the keychain
and a NixOS host often leaks
/etc/sslinto the sandbox, so these gates onlyever looked green on the machines they were written on.
SSL_CERT_FILEinnix/e2e.nixmakes the bundle an input.differential-tracesis the one that looked ambiguous: it failed on run30341066882 and then passed four times (30341722090 plus three independent
repeats in
30342250503).
The only difference was ca-derivations. Environmental, and enforced.
Every package builds, 19 of 19 in each of three runs. The prose this replaces
said
packages"cannot succeed yet" and that cargoUnit could not vendor thislock.
nix flake checknever built packages at all, it prints(build skipped)for each, and they build.What the first real run caught
Run 30342733873
is the first time
smash-e2ehas ever run to completion in CI. It failed, on47 of 47 abilities:
The action bar had become an NBT compound when the component API landed in
#1004, and
tools/smash-match.pyread onlyTag::String, so it logged<- action bar: <nbt tag type 0x0A, not a string>and never saw the refusal.It was red on main for two commits and nothing reported it. #1006 fixed the
reader; ENG-10838 has the write-up and the remaining gap.
What is enforced now
nix/ci/flake-gate.nix, subtractive: every attribute ofchecksis builtunless named in
excluded, so a check added tomorrow is enforced the day itlands.
nixos-modulesmoved into the per-system set so nothing sits outsidethe gate's view, and the gate reads its own names with
builtins.attrNames,which is how three checks that landed in sibling PRs while this branch was open
(
smash-text-no-legacy-formatting,smash-selector-e2e,smash-selector-e2e-app) became enforced with no edit.45 enforced. Exclusion list empty.
Watching the guards fire
Three guards, none of them trusted on reading alone.
An exclusion that starts passing turns the gate red. Run 30342733873 still
had
differential-tracesexcluded while it in fact passed:Breaking something only a newly enforced check protects turns the job red.
PR #1008 reordered
--portahead of--ipin the game server module. No cargojob can see that: Formatting, Clippy, Documentation, Tests, deny, unused-deps
and Code Coverage all read rust.
Job 90222622214
went red with the message intended:
Reverted immediately.
A misspelled exclusion throws at evaluation.
And the ca-derivations preflight, run against a store without it:
Not done, and tracked
runs-onstaysubuntu-latestagainst the house rule. The fleet cannot servethis org: ix
crates/ci/dispatcher/src/config.rsholdsGithubApp.orgas oneStringandgithub.rsregisters every JIT runner atorgs/{org}withorg = indexable-inc, so ahyperion-mcjob is never offered one. Pointingruns-onat anix-ci-run-*label today would hang until timeout, which isstrictly worse than a hosted runner that works. ENG-10824. The gate is the
same
nix build .#checks.x86_64-linux.<name>either way, so the migration isone line when it lands.
No CI job here is a required status check, and the comment on the
checkjobclaimed the opposite. Corrected in this PR; ENG-10827 proposes adding the
rule, not done here because four agents had PRs in flight and turning eight
jobs into required contexts mid-flight would have blocked all of them.
Authored by an AI agent (Claude Opus 4.5 via Claude Code).