fix(smash): Wither Image and Inferno draw on the press, and a gate for the class - #1093
Merged
Merged
Conversation
…r the class Two abilities put nothing on any screen when you pressed them. Wither Image is two presses: the first plants a decoy where you stand, the second swaps you back to it. Only the swap drew. Planting was silent, so an opponent could not see that the escape was armed, which is the entire threat the ability carries. It now plants a column of soul fire, deliberately unlike the portal shell every teleport uses, so the two presses are told apart at a glance. Blaze's Inferno is a half-second-cooldown flame cone and drew nothing at all. The only fire on screen came from the burn on a victim, so spewing at open air was invisible and a player could not see where their own flame reached. It now draws a jet along the facing, above the victim search, because the search is what can come up empty. Both are the pattern #1086 was supposed to finish: a visual whose existence depends on the state of the game rather than on the button. I claimed in that commit message that "the eighth, Wither Image, already draws on the press". It does not, and it never did; I read the second press for the first. So the gate is the point of this change, not the two fixes. Every existing sweep -- the mock one here and the real-client one in tools/smash-match.py -- presses each ability up to three times, because two abilities in the roster genuinely need more than one. That is right for the question those sweeps ask and is exactly why neither could ask this one: the second press supplied the particles and the first press's silence was invisible. every_ability_draws_on_the_press_that_fires_it presses once, and once only. Written first against particles alone, it named seven abilities and only one was a defect: Iron Hook, Needler, Sulphur Bomb, Guided Wither Skull, Chicken Missile and Wither Image's own skull answer the press by putting a drawn object in the world, and for those the object is the picture. A check that calls correct code broken is the same error in a mirror and the worse direction -- somebody "fixes" six working abilities by bolting a redundant puff onto a working skull. It now asks whether the press put anything at all on a screen, by either route, and stays one test because "which abilities are the projectile kind" is not a list anybody should maintain. Watched to fail, both ways: without the Wither Image fix, particles-only: 7 named, 1 of them real without the Wither Image fix, widened: ["Wither Skeleton / Wither Image"] with both fixes: test result: ok. 13 passed; 0 failed cargo test -p smash: ok, all 21 binaries. fmt and clippy clean.
`cargo clippy -p smash --lib` exits 101 on origin/main at ee1139e: error: this function could have a `#[must_use]` attribute --> events/smash/src/input.rs:260:8 error: could not compile `smash` (lib) due to 1 previous error I made melee_damage public in #1086 so a test could ask the same question a swing asks, and clippy::pedantic wants #[must_use] on a public function returning a value. One attribute. The attribute is not the interesting part. Nothing noticed for four hours, because #1088 turned every workflow to workflow_dispatch and clippy only ever ran there. It is not in the nix flake gate, so `nix run .#ci` -- the thing anybody actually runs before pushing -- is green on a tree clippy rejects. Same hole as ENG-11424, which is the same sentence about `cargo fmt --all --check`. Both belong in the gate, and turning CI off is what made that urgent rather than tidy.
andrewgazelka
added a commit
that referenced
this pull request
Jul 30, 2026
…1094) `checks = scripts // ...`, so `checks.lint` and `checks.fmt` already existed — **as the scripts**. Building one runs shellcheck over the text of a cargo command and never runs cargo. `nix run .#flake-gate` has been printing `ok lint` and `ok fmt` about tools it has never invoked. **That matters beyond wall clock: the gate is the only thing that writes `results.jsonl`.** The published baseline and the instability record have therefore carried two checks recorded as passing that were never run, and every consumer of the differential verdict has been reading that. `cargo clippy -p smash --lib` exited 101 on `main` at `ee1139e` for about four hours. Fixed in #1093; this is the hole that let it sit. ## What this adds - `checks.rustfmt` — `cargo fmt --all --check` - `checks.clippy` — `cargo clippy --all-targets --all-features -- -D warnings`, whole workspace Each **invokes the existing script** (`${lib.getExe checkScripts.fmt} --check`, `${lib.getExe checkScripts.lint}`) rather than restating its command, so the gate and what a contributor runs by hand cannot say different things about the same tree. No lint configuration is invented: flags stay in `clippyArgs`, lints stay in `[workspace.lints]`, `clippy.toml` and `rustfmt.toml`. `checks.clippy` reuses `workspace.cargoConfigScript`, pointing `CARGO_HOME` at the vendor dir cargoUnit already built for the release build — one set of `outputHashes`, nothing to drift. ## The Clippy matrix job stays, deliberately `ci.yml` has had a `Clippy` job and a `Formatting` job in its `check` matrix all along, so clippy now runs twice on a dispatched run. The obvious cleanup is to delete the matrix entries, and it is the wrong one. Third commit writes the reasoning next to the matrix so nobody re-proposes it from the diff alone: The two answer different questions. The matrix answers "is this branch lint-clean" — fast, standalone, legible as its own line in the checks list. The gate's entry answers "does `results.jsonl` tell the truth about lint". Merging them would make the fast answer wait on the slow one: a red clippy at minute thirty instead of minute two. And the duplication is execution, not definition — both paths invoke the same `checkScripts` derivation, so merging would collapse nothing that is currently split. ## Watched both go red `mod cached_save;` in `crates/hyperion-utils/src/lib.rs`: ``` hyperion-rustfmt> Diff in .../crates/hyperion-utils/src/lib.rs:4: hyperion-rustfmt> -mod cached_save; hyperion-rustfmt> +mod cached_save; error: Cannot build '/nix/store/rvf7q8...-hyperion-rustfmt.drv'. Reason: builder failed with exit code 1. ``` Separately broke `events/smash/src/lib.rs` and `tools/rust-mc-bot/src/lib.rs` together, because a fileset that silently misses a workspace member is exactly the failure this check would hide: ``` hyperion-rustfmt> Diff in .../events/smash/src/lib.rs:19: hyperion-rustfmt> Diff in .../tools/rust-mc-bot/src/lib.rs:5: ``` A `pub fn` returning a value — the same lint `main` was red on: ``` hyperion-clippy> error: this function could have a `#[must_use]` attribute hyperion-clippy> --> crates/hyperion-utils/src/lib.rs:12:8 hyperion-clippy> = note: `-D clippy::must-use-candidate` implied by `-D clippy::pedantic` hyperion-clippy> error: this could be a `const fn` hyperion-clippy> = note: `-D clippy::missing-const-for-fn` implied by `-D clippy::nursery` hyperion-clippy> error: could not compile `hyperion-utils` (lib) due to 2 previous errors ``` All reverted. ## The first linux run caught something darwin could not `checks.clippy` was green on aarch64-darwin and red on the first `x86_64-linux` gate run (30514788219), inside `tikv-jemalloc-sys`: ``` configure: error: cannot determine return type of strerror_r ``` Clippy builds the dev profile, so every C build script in the graph compiles at `-O0`, and glibc answers `_FORTIFY_SOURCE` with a `#warning` whenever optimisation is off. An autoconf probe reading stderr then reads its own test as a compile failure. The release build never meets this because it compiles at `-O3`; darwin never meets it at all. Second commit adds `hardeningDisable = [ "fortify" ]`, and the check is green on both systems. ## Cost, measured on ubuntu-latest Two gate runs on hosted four-core runners, same night, cold store, same base: | run | ref | checks | gate seconds | | --- | --- | --- | --- | | [30514860323](https://github.com/hyperion-mc/hyperion/actions/runs/30514860323) | `main` d8df46f | 69 | **1977** | | [30516652866](https://github.com/hyperion-mc/hyperion/actions/runs/30516652866) | this branch | 71 | **2282** | **+305 s, +15.4%**, and in that run the gate printed `ok clippy` and `ok rustfmt`. One sample each, so read it against the spread: this gate has been observed between 31 and 41 minutes, and a five-minute delta is not cleanly separable from that. The floor is firmer — clippy costs 429 CPU-seconds cold (measured `user 328.30 sys 100.87`), which is 107 s of a four-core runner under perfect overlap. It does not overlap perfectly, and 305 s is what imperfect overlap looks like. Clippy cannot share the release build's artifacts, and this is structural: clippy reads dev-profile metadata produced by `clippy-driver`, the release build produces optimised rlibs from `rustc`. Nothing is reusable in either direction. ## Known cost: `checks.clippy` reruns on any tracked file Its source is the whole tree, because clippy compiles and a kit's arena is `include_str!` — narrowing the input would be narrowing what compiles. So a change to a document or to `.github/` reruns 429 CPU-seconds of clippy. Demonstrated by this PR's own third commit, which touches only `ci.yml`: ``` checks.x86_64-linux.clippy 1k2686w5pk7313gq8bfwlimk5xhaq1qs -> 93a35bbg18b4i71n4w7ilkxl0zpz535p checks.x86_64-linux.rustfmt ywv83av7xy1dnhf1hj57b0111d1bxgz8 -> ywv83av7xy1dnhf1hj57b0111d1bxgz8 ``` `checks.rustfmt` is unaffected because it reads a fileset of Rust sources, manifests and `rustfmt.toml` alone. Narrowing clippy the same way is possible but has a real failure mode — miss one `include_str!` and the check compiles a different tree than the release build does — so it is deliberately left as a follow-up rather than done half-way here. ## What this still does not cover The pipeline is manual. Every workflow is `workflow_dispatch:` since #1088, `main` carries no required status checks, and `branches/main/protection` 404s — so somebody has to dispatch the `Flake` job, or run `nix run .#flake-gate` locally. This makes the gate *correct*; it does not make it *run*. The cheapest thing that would make skipping harder is a pre-commit hook running `cargo fmt --all --check` alone; clippy is too slow for one. The repo has no hook machinery today, so that is a separate change. The one remaining failure on the after-run is `smash-e2e`, which fails identically on `main` (run 30514860323, `checks that failed: smash-e2e`). The verdict reads it as blocking only because no baseline artifact was admitted, which is the fail-closed direction working as designed. ENG-11424.
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
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.
Two abilities put nothing on any screen when you pressed them, and the check
that would have caught them could not, for a reason worth writing down.
The two
Wither Image is two presses: the first plants a decoy where you stand, the
second swaps you back to it. Only the swap drew. Planting was silent, so an
opponent could not see that the escape was armed — which is the entire threat
the ability carries. It now plants a column of soul fire, deliberately unlike
the portal shell every teleport uses, so the two presses are told apart at a
glance.
Blaze's Inferno is a half-second-cooldown flame cone and drew nothing at
all. The only fire on screen came from the burn on a victim, so spewing at open
air was invisible and a player could not see where their own flame reached. It
now draws a jet along the facing, above the victim search, because the search
is what can come up empty.
Both are the pattern #1086 was supposed to finish. That commit message says
"the eighth, Wither Image, already draws on the press." It does not, and never
did — I read the second press for the first.
Why no gate caught it
Every sweep we have, the mock one here and the real-client one in
tools/smash-match.py, presses each ability up to three times, because twoabilities in the roster genuinely need more than one. That is correct for the
question those sweeps ask and is exactly why neither could ask this one: the
second press supplied the particles and the first press's silence was invisible.
So
every_ability_draws_on_the_press_that_fires_itpresses once, and onceonly.
The near-miss in the gate itself
Written first against particles alone, it named seven abilities and only one
was a defect:
Six of those answer the press by putting a drawn object in the world — a
skull, an egg, a thrown block — and for those the object is the picture. A
check that calls correct code broken is the same error in a mirror, and it is
the worse direction: somebody satisfies it by bolting a redundant puff onto six
working abilities. It now asks whether the press put anything at all on a
screen, by either route, and stays one test because "which abilities are the
projectile kind" is not a list anybody should maintain.
Watched to fail
cargo test -p smashgreen across all 21 test binaries;cargo fmt --all --checkandcargo clippy -p smash --all-targetsboth clean.Second commit: clippy has been red on main since #1086
cargo clippy -p smash --libexits 101 onorigin/mainatee1139e:One attribute, from making
melee_damagepublic in #1086. The attribute is notthe interesting part — nothing noticed for four hours, because #1088 turned
every workflow to
workflow_dispatchand clippy only ever ran there. It is notin the nix flake gate, so
nix run .#ci, the thing anybody actually runs beforepushing, is green on a tree clippy rejects.
Same hole as ENG-11424, which is the same sentence about
cargo fmt --all --check. Both belong in the gate; turning CI off is what made that urgentrather than tidy.
🤖 Opened by Claude Code (claude-opus-4-6).