fix(tools): scripted clients read protocol.json, not each other's source - #1028
Merged
Conversation
## What broke, and how it got through #1021 turned `crates/hyperion-minecraft-proto/src/generated/registry.rs` into a directory. Three Python clients under `tools/` were reading that file as text and pulling registry names out of it with a regex. They kept "building", passed every Rust gate, and failed twenty minutes into CI inside four end-to-end checks: ``` FileNotFoundError: [Errno 2] No such file or directory: '/nix/store/bavm52as33izbs3kl5zihvszxgskz7lw-source/crates/hyperion-minecraft-proto/src/generated/registry.rs' enforced checks that failed: completions-e2e smash-e2e smash-hotbar-e2e smash-hud-e2e ``` A path in a Python string is invisible to cargo and to every Rust grep. I searched for `generated::registry` before landing #1021 and found the Rust callers; I did not search for `generated/registry.rs`, and nothing made me. `smash-selector-e2e` is the same bug one merge later: `tools/smash-selector.py` read `src/entity_type.rs`, which #1024 deleted, with a regex matching `Self::new("...", n)` — a spelling that stopped existing when entity types became a plain enum. CI had not reached it yet. Running the whole gate locally found it: ``` enforced checks that failed: completions-e2e smash-e2e smash-hotbar-e2e smash-hud-e2e smash-selector-e2e ``` ## The fix is that they stop reading Rust Pointing three regexes at three new paths would have restored green and left the same trap armed. All three now call one reader: ```python def registry_entries(registry: str) -> list[str]: """A vanilla registry's entry names, in network-id order.""" ``` in `tools/client-26.2.py`, reading `crates/hyperion-minecraft-proto/protocol.json` — the same file `build.rs` reads and the same file every generated table comes out of. `protocol.json` joins the e2e client fileset in `nix/e2e.nix`. Two bugs went with the scrapers. `completions-check.py` carried a comment explaining that it had to index from `entries: &[` rather than from the declaration, because the registry's own name sits in the same table and looks exactly like an entry — getting it wrong shifted every id by one and turned `brigadier:string` into `brigadier:long`. `smash-match.py` compensated for the same thing with a bare `[1:]`. Neither is needed: `entries` in the JSON is the entries, and the index is the id. ## The gate, because the class outlives the instance `nix flake check .#tool-paths` fails when any tracked `.py`, `.sh` or `.nix` file names a repository path that does not exist. Only strings anchored at a real top-level directory count, and anything containing a format placeholder or a glob is skipped, so prose is not mistaken for a claim. It earned itself immediately. Its first run, before I had looked at `smash-selector.py`, found the bug #1024 had just introduced: ``` 4 repository paths named in scripts, 1 of them missing MISSING tools/smash-selector.py:89 crates/hyperion-minecraft-proto/src/entity_type.rs ``` Broken deliberately as well, by putting the dead path back into `completions-check.py`: ``` 4 repository paths named in scripts, 1 of them missing MISSING tools/completions-check.py:44 crates/hyperion-minecraft-proto/src/generated/registry.rs A script names a path this tree does not have. Nothing else catches this: a path in a string is invisible to cargo and to every Rust grep, so it fails inside whichever gate runs that script. Either fix the path, or -- better -- stop reaching into another component's source and read its committed data instead. ``` Restored, and it reports `3 repository paths named in scripts, 0 of them missing`. This is the cheap half of the fix and it is worth saying which half. It cannot tell that a regex has stopped matching a file that still exists — only that the file is gone. What stops the other half is the three clients no longer parsing Rust at all. ## Gates | gate | result | | --- | --- | | `nix build .#checks.aarch64-darwin.completions-e2e` | rc=0, `RESULT: every completion claim held` | | `nix build .#checks.aarch64-darwin.tool-paths` | rc=0, 3/3 | | `nix eval .#checks.aarch64-darwin.flake-gate.drvPath` | rc=0 | | `cargo fmt --all --check` | rc=0 | | `flake8` on all five changed scripts | rc=0 for everything this change touched | `completions-e2e` is the one that was failing and it is the one I ran to completion. The other four e2e checks share the same reader through `smash-match.py`'s `load_item_names` and `smash-selector.py`'s `entity_names`, both of which are exercised by the unit-level check above, but I did not wait on all five locally -- each compiles the whole server. CI runs them. Two lint findings in `tools/smash-selector.py` (E301, E302) are pre-existing; I checked by stashing and re-running. Not fixed here. --- AI-authored (Claude) under human direction.
Benchmark Results for generalComparing to 8318617 |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1028 +/- ##
==========================================
+ Coverage 51.14% 51.45% +0.31%
==========================================
Files 335 337 +2
Lines 30052 30554 +502
Branches 1137 1167 +30
==========================================
+ Hits 15369 15723 +354
- Misses 14438 14578 +140
- Partials 245 253 +8 see 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
andrewgazelka
added a commit
that referenced
this pull request
Jul 28, 2026
A fully drawn bow in bedwars fired at **3.6 blocks a tick**. Vanilla's
maximum is 3.0, and the comment next to the multiply said 3.0, but
`get_charge` returned *seconds* clamped to 1.2 and the caller multiplied
that by 3.0. The arrow flew, it hit things, and it was a fifth too fast
for as long as the module has existed, because nothing ever looked at
the number.
This fixes that and two other bugs in the same file, and adds the gate
that would have caught all of it.
## Where the bow actually was
The brief I started from was that the bow "does nothing" because
`BowModule` is never imported into smash. That turned out to be wrong,
and the measurement is worth recording because it changed the whole
shape of the work.
`nix run .#smash-e2e`, unmodified `e4bbcc2`, real clients:
```
0.89s PROVED kits equipped: P1 0:minecraft:bow, 1:minecraft:iron_axe, 2:minecraft:arrow
1.83s [P1] -> right-click hotbar slot 0 (Barrage)
4.27s [P1] -> release hotbar slot 0 (Barrage, fully charged)
4.27s [P2] <- sound minecraft:entity.arrow.shoot at (6.0, 65.0, -11.0)
4.33s [P1] <- knockback on P2: (-1.758, 1.788, 2.906) |v| = 3.839 blocks/tick
4.34s [P2] <- health 16.64/20 ... 13.28/20 ... 9.92/20
```
The bow in Super Smash Mobs is the **icon for a charged ability**, not a
weapon. `events/smash/src/input.rs` routes `ItemInteract` and
`ReleaseUseItem` to `ability::use_slot` / `ability::release_slot`, and
smash runs its own projectile integrator with its own `Position` type.
`ItemKind::Bow` is never on that path.
**Importing `BowModule` into smash would have broken the game.**
`EventQueue::drain()` is destructive and single-consumer; `BowModule`'s
release handler is `PreUpdate` and smash's is the default `OnUpdate`, so
the bow would have eaten `ReleaseUseItem` first and every charged
ability in the game would have silently stopped firing. So the module
stays in bedwars, which is the only game that wants a bow you nock and
spend arrows from — and which is `packages.default` and what `nix run
.#dev` builds, so all three bugs were live in the default deployment.
## The three fixes
**The charge curve.** `get_charge` is now `BowItem.getPowerForTime` and
nothing else: ticks over 20, quadratic, saturating at one second. It
returns a fraction, so the caller's `* 3.0` lands on vanilla's actual
maximum. `FULL_DRAW_TICKS` and `MAX_ARROW_SPEED` have names now.
**A self-hit that wasn't.** `if damage == 0.0 && owner == event.client`
refused a hit only when the arrow was both stationary *and* the victim's
own, so your own moving arrow damaged you, and a stuck arrow still
counted against everyone else. Two independent refusals wearing one
`&&`; now two `if`s.
**`ArrowsInEntity` grew without bound.** Incremented in `bow.rs` and
decremented nowhere in the tree — a player wore every arrow they had
ever taken, through every respawn. Now cleared on respawn from a
`HandlerRegistry` handler rather than a second drain of
`EventQueue<ClientStatusEvent>`, since that queue is single-consumer and
`attack.rs` already owns it.
**Plus a rename.** `PlayerInventory::get_cursor` is `held`,
`get_cursor_index` is `held_slot`. In Minecraft the cursor is the stack
on the mouse inside an open container; this has only ever returned
`self.get(self.hand_slot)`. The old name already sent one reader down
the wrong path this week.
## The gate
`nix run .#bedwars-bow-e2e` drives a real client and reads the launch
velocity out of `ClientboundAddEntity` — since 26.2 that packet carries
the velocity, so the charge curve is a number on the wire rather than
something inferred from a trajectory.
```
PASS /bow puts a minecraft:bow on a visible key (key 0)
PASS /bow puts minecraft:arrow in the inventory (64 of them)
PASS a full draw spawns exactly one minecraft:arrow the client is told about (got 1)
PASS a full draw launches at vanilla's 3.0 blocks a tick, not the 3.6 the old seconds-as-charge produced (got 3.000)
PASS an arrow that hits a block stops: it was re-sent at (12.50, 4.00, 40.00) with |v|=0.000
PASS firing spends exactly one arrow (64 -> 63)
PASS a short draw still fires (got 1 arrows)
PASS a shorter draw launches slower than a full one (0.697 < 3.000)
PASS two releases 49 ms apart fire once, not twice (got 1 arrows)
RESULT: ok (0 checks failed)
```
## Every guard broken, and what it said
**The charge curve** — reverted `get_charge` to the original
seconds-clamp:
```
<- AddEntity id=1018 type=6 motion=(0.000, 0.000, 3.600) |v|=3.600
FAIL a full draw launches at vanilla's 3.0 blocks a tick, not the 3.6 the old
seconds-as-charge produced (got 3.600)
RESULT: failure (1 checks failed) rc=1
```
**The cooldown, the block stop and the arrow spend**, broken together so
their messages could not be confused:
```
FAIL an arrow that hits a block stops: it was re-sent at (15.50, 4.00, 8.00) with |v|=3.000
FAIL firing spends exactly one arrow (64 -> 64)
FAIL two releases 49 ms apart fire once, not twice (got 2 arrows)
RESULT: failure (3 checks failed) rc=1
```
The double-fire is visible in the transcript as two distinct entity ids
where there should be one:
```
8.01s <- AddEntity id=1020 type=6 motion=(0.000, 0.000, 3.000)
8.07s <- AddEntity id=1021 type=6 motion=(0.000, 0.000, 3.000)
```
**The gate also caught its own bug on its first run.** The entity-type
id was scraped out of the generated Rust with a regex that also matched
the registry's own `name` field, so every id came out one too high and
it looked for an arrow of type 7 while the server sent type 6. It now
uses `base.registry_entries`, which #1028 added for exactly this reason
and whose docstring describes the same off-by-one.
## What this does not cover, and why
The **self-hit** and **`ArrowsInEntity`** fixes are not covered by the
gate. A self-hit needs an arrow that comes back to its shooter;
`ArrowsInEntity` is entity metadata only another client can see. Both
want a second connection this gate does not open. They were found by
reading and are argued in the commit message, not proven on the wire.
An earlier version also checked a **partial** draw against vanilla's
curve. I deleted it: it *passed* during the fire drill with the bug
present. At a quarter draw the quadratic and the linear bug are 0.2
blocks a tick apart while the script's own scheduling moves the answer
by more than that. The saturated full draw is the discriminating
assertion, and it is exact — `min(f, 1)` means any hold past a second is
1.0, so 3.0 is expected with no timing dependence at all.
## One note on the differential scenario
`crates/hyperion/tests/differential/scenarios/arrow-level-shot.json`
fires at `"power": 3.0` and passes, before and after this change. It
does **not** constrain the launch: `differential.rs:202` reads the
starting velocity out of the recorded vanilla trace, and `power` is an
input to the Java recorder only. So the scenario validates the flight
integrator, not the bow. Worth knowing, because a reader seeing
`"power": 3.0` next to a launch that produced 3.6 would reasonably
assume one bounded the other.
## Checks
- `nix run .#bedwars-bow-e2e` — green, 9 assertions, `rc=0`
- `nix run .#lint` — `rc=0`
- `cargo test --workspace` — 112 test targets, all ok, `rc=0`, including
`differential::scenarios_match_vanilla`
- `nix run .#e2e` (the bedwars gate this shares a runner with) — green,
`rc=0`
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.
fix(tools): scripted clients read protocol.json, not each other's source
What broke, and how it got through
#1021 turned
crates/hyperion-minecraft-proto/src/generated/registry.rsinto adirectory. Three Python clients under
tools/were reading that file as textand pulling registry names out of it with a regex. They kept "building", passed
every Rust gate, and failed twenty minutes into CI inside four end-to-end
checks:
A path in a Python string is invisible to cargo and to every Rust grep. I
searched for
generated::registrybefore landing #1021 and found the Rustcallers; I did not search for
generated/registry.rs, and nothing made me.smash-selector-e2eis the same bug one merge later:tools/smash-selector.pyread
src/entity_type.rs, which #1024 deleted, with a regex matchingSelf::new("...", n)— a spelling that stopped existing when entity typesbecame a plain enum. CI had not reached it yet. Running the whole gate locally
found it:
The fix is that they stop reading Rust
Pointing three regexes at three new paths would have restored green and left
the same trap armed. All three now call one reader:
in
tools/client-26.2.py, readingcrates/hyperion-minecraft-proto/protocol.json— the same file
build.rsreads and the same file every generated table comesout of.
protocol.jsonjoins the e2e client fileset innix/e2e.nix.Two bugs went with the scrapers.
completions-check.pycarried a commentexplaining that it had to index from
entries: &[rather than from thedeclaration, because the registry's own name sits in the same table and looks
exactly like an entry — getting it wrong shifted every id by one and turned
brigadier:stringintobrigadier:long.smash-match.pycompensated for thesame thing with a bare
[1:]. Neither is needed:entriesin the JSON is theentries, and the index is the id.
The gate, because the class outlives the instance
nix flake check .#tool-pathsfails when any tracked.py,.shor.nixfile names a repository path that does not exist. Only strings anchored at a
real top-level directory count, and anything containing a format placeholder or
a glob is skipped, so prose is not mistaken for a claim.
It earned itself immediately. Its first run, before I had looked at
smash-selector.py, found the bug #1024 had just introduced:Broken deliberately as well, by putting the dead path back into
completions-check.py:Restored, and it reports
3 repository paths named in scripts, 0 of them missing.This is the cheap half of the fix and it is worth saying which half. It cannot
tell that a regex has stopped matching a file that still exists — only that the
file is gone. What stops the other half is the three clients no longer parsing
Rust at all.
Gates
nix build .#checks.aarch64-darwin.completions-e2eRESULT: every completion claim heldnix build .#checks.aarch64-darwin.tool-pathsnix eval .#checks.aarch64-darwin.flake-gate.drvPathcargo fmt --all --checkflake8on all five changed scriptscompletions-e2eis the one that was failing and it is the one I ran tocompletion. The other four e2e checks share the same reader through
smash-match.py'sload_item_namesandsmash-selector.py'sentity_names,both of which are exercised by the unit-level check above, but I did not wait
on all five locally -- each compiles the whole server. CI runs them.
Two lint findings in
tools/smash-selector.py(E301, E302) are pre-existing;I checked by stashing and re-running. Not fixed here.
AI-authored (Claude) under human direction.