feat(smash): two players start the countdown, four fill the lobby - #1019
Merged
Conversation
Mineplex ran Super Smash Mobs at 4 to start and 8 for a full lobby. The people playing this server are the two or three developing it, so the countdown never ran and the match phases were unreachable by hand. LobbyConfig already existed as a singleton for exactly this, with a doc comment saying so, and the state machine is untouched. Only the defaults move: min_players 4 to 2, full_players 8 to 4. All three countdown lengths stay distinct at these numbers. Two players takes countdown_at_min, three satisfies the three-quarters rule, four is full. Dropping full_players below 4 would collapse two bands into one, so that is written next to the constant. Six lobby tests broke that were not about these numbers, because they spelled 3, 4, 6 and 8 as literals and so stated LobbyConfig::default a second time. They now derive their counts from the config, which is why the diff is larger than two lines.
Benchmark Results for generalComparing to 68c5429 |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1019 +/- ##
=======================================
Coverage 47.58% 47.58%
=======================================
Files 243 243
Lines 27608 27608
Branches 1118 1118
=======================================
Hits 13137 13137
Misses 14235 14235
Partials 236 236
🚀 New features to boost your workflow:
|
andrewgazelka
added a commit
that referenced
this pull request
Jul 28, 2026
…t has two #1018 put one boss bar on the wire, hand rolled in a game event's adapter. This is the API it should have been, and the two bars the operator asked for on top of it. A bar is an entity, its audience is `(ShownTo, player)` edges, and what each viewer was last told is the data on `(Sent, viewer)`. That last choice is what removes the two cases that are usually bugs: a mid-match joiner has no pair, so the next tick sends them `Add`, and every way a bar stops being shown ends with that pair going away, which one `OnRemove` observer turns into a `Remove`. There is no join or leave handler in this change. One field moves and it is that field's narrow operation; two or more and it is a single fresh `Add`, which is both atomic and smaller than the updates it replaces. The wire gate found that one: a lobby bar becoming a countdown was briefly a countdown in the lobby's blue. `/serverload`, Admin, opt in, puts the host's own CPU and RSS on two bars. CPU is `getrusage`, not `ps`, so the first sample reports nothing rather than a lifetime average. Both are drawn against the point the machine stops being able to give more and print that ceiling beside the reading, so the fill is the quotient of the two numbers in the bar's own label and `hud-check.py` divides them to check it. `smash-hud-e2e` goes from 25 checks to 48, including one that reads every boss bar packet of the whole run and asserts none of them said anything the client already knew. It also fixes a check #1019 left red on main.
andrewgazelka
added a commit
that referenced
this pull request
Jul 28, 2026
…t has two #1018 put one boss bar on the wire, hand rolled in a game event's adapter. This is the API it should have been, and the two bars the operator asked for on top of it. A bar is an entity, its audience is `(ShownTo, player)` edges, and what each viewer was last told is the data on `(Sent, viewer)`. That last choice is what removes the two cases that are usually bugs: a mid-match joiner has no pair, so the next tick sends them `Add`, and every way a bar stops being shown ends with that pair going away, which one `OnRemove` observer turns into a `Remove`. There is no join or leave handler in this change. One field moves and it is that field's narrow operation; two or more and it is a single fresh `Add`, which is both atomic and smaller than the updates it replaces. The wire gate found that one: a lobby bar becoming a countdown was briefly a countdown in the lobby's blue. `/serverload`, Admin, opt in, puts the host's own CPU and RSS on two bars. CPU is `getrusage`, not `ps`, so the first sample reports nothing rather than a lifetime average. Both are drawn against the point the machine stops being able to give more and print that ceiling beside the reading, so the fill is the quotient of the two numbers in the bar's own label and `hud-check.py` divides them to check it. `smash-hud-e2e` goes from 25 checks to 48, including one that reads every boss bar packet of the whole run and asserts none of them said anything the client already knew. It also fixes a check #1019 left red on main.
andrewgazelka
added a commit
that referenced
this pull request
Jul 28, 2026
…t has two (#1026) `BossEvent` reached the wire for the first time in #1018, as one bar, hand rolled in a game event's adapter: one UUID per player, `Add` for every change, and a diff in the game half to stop it resending. That was correct and it was measured, and it did not survive the next ask, which was two more bars carrying something that is not a game concept at all. ## A bar is an entity and its audience is a set of edges ```rust let bar = world .entity() .add(BossBar::id()) .set(Title(Text::text("CPU 240% of 1600%").color(NamedColor::Aqua))) .set(Progress(0.15)) .add((ShownTo, operator)); ``` and later `bar.set(Progress(0.31))`, which sends one `UpdateProgress` to each viewer and nothing else. `smash`'s `Server::set_boss_bar` is now four `set`s on one of these, and the packet it used to build is gone. The load-bearing choice is not the components; it is where the per-viewer sent state lives. `Sent` is the data on the `(Sent, viewer)` pair, holding exactly what that client was last told, and it is what makes the two cases that are usually bugs stop being code: * **Joining.** A player who was not there has no `(Sent, bar)` pair, so the next tick sends them `Add`. There is no join handler in this change. * **Leaving.** A bar is destroyed, a player disconnects, or an audience shrinks. All three end with the pair going away -- flecs does the first two itself under `(OnDeleteTarget, Remove)` -- and one `OnRemove` observer on `(Sent, Wildcard)` sends the `Remove`. Teardown cannot leak, because the fact that says "this client has this bar" and the thing that gets cleaned up are the same fact. There is no explicit join or leave path anywhere in `boss_bar.rs`. That was the test of whether the shape was right. ## The change the wire gate found A change of state usually moves several fields at once, each operation is its own packet, and between the first and the last the client is holding a bar that is half one state and half the next. There is no ordering that fixes it: put the title last and a countdown ticking over draws the next second's fill under this second's number; put it first and a lobby bar becoming a countdown is a countdown in the lobby's blue. `smash-hud-e2e` caught the second of those as a real failure -- one of #1018's own checks, `and it is yellow, not the lobby's blue`, against a bar that was briefly exactly that. So one field moving is that field's narrow operation, and two or more is a single fresh `Add`. `Add` is *also* the smaller packet: every operation repeats the sixteen byte id, so two updates already cost more than the one that carries all four fields, and `BossHealthOverlay.add` is a `put` into a map keyed on the id, so it replaces the bar in place. Packet count is unchanged from #1018's 1,135 -- both send one packet per change -- and each packet is now at most the size of the `Add` it replaced. ## The two bars that carry the host `/serverload`, Admin, opt in. Fifteen people in a lobby staring at server telemetry is a strange default and three stacked bars is most of the strip. **CPU is `getrusage(RUSAGE_SELF)`, not `ps`.** `ru_utime + ru_stime` is a cumulative counter, so a rate over a window is the only reading it can give and the first sample reports that it has nothing yet. `ps` answers the lifetime average instead, which is a different question wearing the same units: a server busy for a minute an hour ago reads busy under `ps` forever. **Memory is RSS**, which is what the kernel counts when it decides who to kill. `/proc/self/statm` field one on Linux, `proc_pidinfo`/`PROC_PIDTASKINFO` on darwin. The Linux path cannot run on the box the gates run on, so the parser is a pure function over a string with a fixture whose seven fields all hold different numbers -- an off-by-one cannot pass it. ### The bar that has no natural full Neither quantity is a fraction. A process can use more than one core's worth of CPU, and it can allocate until the machine refuses. So both are drawn against the point where the machine stops being able to give more -- every core it has, every byte of physical memory -- and both print that ceiling beside the reading: ``` CPU 240% of 1600% MEM 3.20 GiB of 128.0 GiB ``` which turns the choice into something checkable rather than a matter of taste: > **the fill is the quotient of the two numbers in the bar's own label.** `hud-check.py` reads both numbers off the wire and divides. A ceiling nobody printed cannot satisfy that, and neither can one invented to make the bar look busy. It is also what keeps the label unnormalised: the honest figure is the numerator, and the fill is that same fact taken against the denominator standing next to it. 200% is two cores, which is what `top` says and what the question meant. Cores come from `available_parallelism`, not the raw processor count, because in a container the honest ceiling is what the container is allowed. ## The gate `nix run .#smash-hud-e2e`, 48 checks, up from 25. `decode_boss_event` read only `Add` and returned `None` for everything else, because `Add` was all the server sent. Left alone against this server it would not have failed -- it would have reported that no boss bar ever arrived, which is a silent pass. It now decodes all six operations into a model of the client's own `BossHealthOverlay`, so #1018's twenty-five checks ask the same questions of the same states, and the operations are kept separately as the only place the diffing is visible at all. One check that was already there was also red before this branch touched anything. #1019 moved `LobbyConfig::min_players` from four to two, and the hub bar's `1/4` was written into `hud-check.py` as a literal, so the gate has been failing on `main` against a server saying `1/2`. #1019 rewrote six `tests/lobby.rs` tests for exactly this reason and the one in `tools/` was missed, because nothing in CI runs the wire gates. It now reads the denominator off the bar and additionally checks that the fill under the label is `1/needed`, which is a stronger claim than the literal it replaces and cannot go stale again. Filed as ENG-10878. The new checks, beyond the two bars' own numbers: * An update sends only the update, over **every** boss bar packet of the whole run rather than at one moment. Three rules -- no packet says anything the client already knew, a whole bar is only resent where two or more fields moved, nothing arrives for a bar the client was never given -- checked against 849 packets in the run below. * A mid-match joiner gets the bar for the match it walked into, as an `Add` carrying the whole thing. * A second viewer joining a *running* bar's audience gets it under the same id the first one holds. The load bars are the only ones in the game with an audience of more than one person, so they are the only place this is observable. * Leaving that audience sends a `Remove` to that viewer and nothing to anybody else. * A viewer disconnecting with a bar on their screen leaves the server drawing everybody else's. ## Broken, and watched to fail Every new guard was inverted and run. Selected output. ### The parser reads `/proc/self/statm`'s first field instead of its second ``` statm_reads_the_resident_field_and_not_a_neighbour left: Some(287072256) right: Some(27807744) a_statm_line_that_is_not_one_reads_as_nothing left: Some(287072256) right: None ``` The fixture's seven fields all hold different numbers, so the off-by-one is a wrong answer rather than a coincidence, and the second test caught it too. ### The CPU label is normalised, which is the whole point of the ask ``` the_cpu_label_is_unnormalised_and_the_fill_is_not left: "CPU 15% of 100%" right: "CPU 240% of 1600%" ``` and on the wire, with the same edit: ``` FAIL the CPU label is unnormalised, so its ceiling is every core and not one: CPU 0% of 100% ``` ### The first CPU sample answers with the lifetime average, the way `ps` does ``` the_first_sample_reports_nothing_rather_than_an_average left: Some(90.0) right: None ``` ### `Sent` is cleaned up with `Delete` rather than `Remove` ``` a_viewer_leaving_removes_only_their_own_record one viewer leaving destroyed the bar ``` Which is the whole teardown design failing in the direction that would be hardest to notice in play: one person quits and the bar goes off everybody's screen. ### Every change resends the whole bar ``` FAIL and a whole bar was only ever resent where two or more fields moved at once: 827 of 872 [('H1', '6879705f', ['title']), ('H1', '6879705f', ['title']), ...] FAIL a new CPU reading that moves only the label costs one packet carrying only the label: ['add', 'add', 'add', 'add', 'add'] ``` ### A client is recorded as having a bar it was never sent ``` FAIL and nothing arrived for a bar the client had never been given: 39 of 981 [('H1', '6879705f', 'update_name'), ...] FAIL the hub puts a boss bar up saying what it is waiting for: [] ``` The second line is what this failure looks like from the outside, and it is why the first one exists: a bar that never arrives is indistinguishable from a server that never drew one, until something is watching for updates to a bar nobody has. ### The id is per viewer rather than per bar, and the teardown observer is gone ``` FAIL a viewer joining a running bar's audience is sent it under the same id the others hold: ['...0584', '...5816a', '...5816d', '...58784'] FAIL with a Remove, while the viewer who is still watching keeps theirs: [] / ['...057e', '...5787e', '...57e6a', '...57e6d'] ``` Four ids where there should be two, because every viewer minted its own, and no `Remove` at all where a viewer had just left the audience. ## Verified Local only, per ENG-10817. * `nix run .#test` -- 704 passed, 1 skipped * `nix run .#smash-hud-e2e` -- 48 checks, 0 failed * `nix run .#fmt` -- clean * `nix run .#lint` -- **one failure, and it is not this branch's**: ``` error: unused import: `flecs_ecs::prelude` --> events/smash/tests/abilities.rs:595:9 ``` Reproduced against a detached worktree at `origin/main` with its own `CARGO_TARGET_DIR`, running the same command `flake.nix:182` runs, with no part of this branch present: ``` $ CARGO_TARGET_DIR=/tmp/mainlint-target cargo clippy --all-targets --all-features -- -D warnings error: unused import: `flecs_ecs::prelude` --> events/smash/tests/abilities.rs:595:9 rc=101 ``` Filed as ENG-10890 rather than fixed here, because it belongs to #1023 and not to this change. Worth knowing while chasing it: plain `cargo clippy -p smash --tests` over the same tree exits 0, because `unused_imports` is only a failure under the `-- -D warnings` the flake adds -- which is how it got past #1023's own check, and how it briefly looked like mine. ## Deliberately not built * **No synthetic load past 100% on the wire.** The gate proves the fill stays a fraction, that the ceiling is every core, and that the fill is the quotient of the label's own two numbers; it does not observe a reading above one core's worth, because forcing the server process over a core from a test client would mean a burn command in production code. The unnormalised case is a unit test with the numbers injected: 2.4 cores of 16 reads `CPU 240% of 1600%`, and 2.4 cores of 1 reads `CPU 240% of 100%` with a full bar. * **The `/proc/self/statm` fixture is written to `proc(5)`, not captured off a running host.** The gates run on darwin. What the fixture does prove is the field index, which is the failure that would ship, because every field holds a different number. * **The sidebar is still hand rolled in smash's adapter.** It has the same argument as the boss bar and it should follow, but porting it is a second change and this one already moves a gate. * **`ShownToEveryone` has no caller.** It is in the API because "everyone" is a rule rather than a list -- a player who connects a minute from now is in it -- and it is covered by the drive system's own path, but nothing in the tree shows a bar to everybody yet. ## Also filed **ENG-10878**: `smash-hud-e2e` was red on `main` before this branch, as above. Fixed here. The follow-up worth having is a job that runs the eight gates in `e2eOffsets`, because a change that breaks only one of them is invisible today until somebody runs it by hand. **ENG-10871**: `/perms set <player> Admin` is gated at `Normal`, so any player who can connect can make themselves Admin. The derive gates a whole clap enum at once, so `Get` and `Set` cannot differ. This gate depends on the hole -- it promotes its own clients, because there is no other way for a test client to reach an Admin command -- so closing it needs a way to seed a group at startup landed alongside. `hud-check.py` names the ticket at the line that depends on it. --- Written by Claude (Opus 5) with human direction.
andrewgazelka
added a commit
that referenced
this pull request
Jul 28, 2026
#1027) ## Effect `nix run .#smash-e2e` passes again, all 15 kits. It has been red since #1019. ## What broke #1019 moved `LobbyConfig::default()` from 4/8 to 2/4, which the operator asked for. `tools/smash-match.py` had those numbers written into it a second time -- as `max(1, self.args.clients - 1)`, as "`min_players` is four" in its docstring, and as "one short of the lobby's minimum" at the call site -- so the sweep's three clients went from below the threshold to above it. The countdown started, the match committed, and every later kit change was refused with `You cannot change kit once the game has started.` Reproduced on e4bbcc2 before changing anything: ``` 0.60s PROVED lobby started a match: server announced the countdown with 3 players connected 0.68s PROVED four in play: 3 clients hold a Login packet: P1=67110240, P2=1377, P3=1378 30.79s [P3 ] <- chat: §cYou cannot change kit once the game has started. 45.78s [P3 ] <- chat: §cYou cannot change kit once the game has started. 60.83s [P1 ] <- chat: §cYou cannot change kit once the game has started. ``` ## It was not the minimum that fired, and that decides the fix The countdown started 0.6 seconds in, not at `min_players`. `countdown_for` checked the three-quarters band before the minimum, so at 2/4 three players satisfy `3 * 4 >= 4 * 3` and take the 30-second band. So `min_players - 1` was never a sound expression of "a roster that cannot start a countdown". The real condition depends on `full_players` too. At 4/8 the subtraction worked by luck. ## Why not read the threshold off the wire #1018 puts `Waiting for players {n}/{min_players}` on a boss bar, and a harness that reads its own limit off that packet cannot disagree with the server. I rejected it as the fix, because it cannot be one. The sweep needs two clients to prove `hurts_target` and to call `wound_attacker` for `heals_caster`; at `min_players: 2` the number it would read off the wire is one, and a one-client sweep goes green while proving less. Observation cannot manufacture headroom, only control can. The bar also advertises only `min_players`, which per the section above is not the number that decides this. ## So the gate declares the lobby, and the harness checks it got it - `LobbyConfig::from_env()` reads `SMASH_MIN_PLAYERS` and `SMASH_FULL_PLAYERS`, in the `SMASH_`-prefixed namespace `hyperion_event_runner::run` already reads the address and certificates from. Not added to the shared `Args`, which bedwars would then carry and never read. - `nix/e2e.nix`'s `mkCheck` gained `serverEnv`. `smashGateLobby` in `flake.nix` declares 4/8 and a sweep roster of 3 in one place and feeds both the app and the check. - `smash-match.py` takes `--sweep-clients` and subtracts nothing. The docstring no longer says four. 4/8 and not just a higher minimum, because `full_players` is what gates the three-quarters band. These are the pre-#1019 numbers, under which three players start nothing even by the old band order, so the gate does not depend on the `countdown_for` change below. ## The guard is the repro `hub_only` fails the run if the lobby leaves the hub while the sweep needs it, checked before the sweep and again before each kit. It reads the phase off the server's own broadcasts rather than recomputing a threshold, so it stays right whatever the numbers become and is not a second copy of them. Broken deliberately, by pointing `smashGateLobby.env` at production's 2/4: ``` 0.72s SWEEP ABANDONED the lobby left the hub before the sweep began. 3 clients is enough for this server to start a countdown, so kits can no longer be changed and nothing further would be proved. Either --sweep-clients is too many for this lobby, or the server needs a higher SMASH_MIN_PLAYERS/SMASH_FULL_PLAYERS. 0.72s RESULT: 10 of 12 steps did not happen rc=1 ``` 571 seconds and a hang become 0.72 seconds and a cause. Restored to 4/8 afterwards. Breaking it is also what found a real bug in it. The first version stopped the run but only recorded its reason in `sweep_failures`, which `sweep` prints at the end of a sweep it no longer reached -- so the run failed with the reason nowhere in the transcript, and then went on to run a mostly-passing match underneath the failure. It now logs directly and returns straight to the report. ## countdown_for checks the minimum first Fixed here rather than filed, because it is a reordering in a file this PR already touches and its failure mode is silent. `min_players` was a partial lie: at `min_players: 4, full_players: 4`, three players started a countdown under a stated minimum of four. Provably inert for both configurations anyone runs. `the_live_configurations_are_unaffected_by_the_minimum_check` keeps the old three-branch order in the test and asserts the new one agrees with it at every count from 0 to `full_players * 3`, for 2/4 and 4/8. `a_lobby_never_starts_below_its_own_minimum` is the case that would have caught it, over every config from 1/1 to 12/12. ## Checks `cargo nextest run -p smash`: 257 passed. `cargo clippy -p smash --all-targets`: clean. `cargo fmt --check`: clean. `nix run .#smash-e2e`, all 15 kits and all 12 claims: ``` 254.43s === what the match proved === 254.43s proved the server published its ability registry 254.43s 51 abilities across 15 kits (Blaze, Chicken, Cow, Creeper, Enderman, Guardian, Iron Golem, Skeleton, Sky Squid, Slime, Snowman, Spider, Wither Skeleton, Wolf, Zombie), every one of them declaring what a client would see 254.43s proved every declared ability did what it declared 254.43s 51 abilities fired by a real client, each one checked against the effects its own registry entry names 254.43s proved every declared ability was heard 254.43s 51 abilities fired by a real client, each one answered by a ClientboundSoundPacket carrying the vanilla sound event its own registry entry names 254.43s proved cooldowns refused a second use 254.43s 47 abilities answered a second right-click inside their cooldown with 'That ability is recharging.' on the action bar 254.43s proved four in play 254.43s proved lobby started a match 254.43s server announced the countdown with 4 players connected 254.43s proved kits equipped 254.43s proved arena is a committed map 254.43s proved knockback from an ability 254.43s proved a hit was heard where it landed 254.43s proved life lost and respawned 254.43s proved match ended, back in the hub 254.43s RESULT: a whole match ran at protocol 776 rc=0 ``` The sweep is 73 seconds for all 51 abilities, not the 571 seconds the broken run took: that figure was time spent on refused kit changes, not real work. The check's 480s timeout has room. ## Not done, and one thing found I did not run the `smash-e2e` **check** derivation, only the app. CI covers it and the timing above says it fits. ENG-10889: `tools/smash-selector.py` has this same defect twice, `FULL_PLAYERS = 8` with a comment citing `LobbyConfig::default` and "Three, which is one short of `min_players`" above its own `connect(3)`. `smash-selector-e2e` is likely broken by #1019 in exactly the way this gate was. Not run and not fixed here. `hotbar-check.py:22` and `smash-map-check.py:1141` have stale prose about the same numbers but still behave.
andrewgazelka
added a commit
that referenced
this pull request
Jul 28, 2026
## Effect `smash-selector-e2e` gets the same treatment `smash-e2e` got in #1027. It had the identical defect, twice, and #1019 broke it the same way. ## The two copies `FULL_PLAYERS = 8`, under a comment naming `LobbyConfig::default` as its source, which is what made it a copy. #1019 moved that default to 4 and the number here stayed at 8, still claiming to be it. It is now `--full-clients`, declared by the gate off `SMASH_FULL_PLAYERS` so the two cannot disagree. And "Three, which is one short of `min_players`" above its own `connect(3)`. Three is now one *over* the minimum, so a countdown runs underneath the hub checks instead of after them. Three stays, because the checks need three named roles -- a holder, a watcher, and somebody to take a freed mob -- but it is no longer described as arithmetic on a threshold, because it never was. ## The guard, not the number `hub_only` fails the run if the lobby leaves `Waiting` while the hub checks still need it, called once after the clients join and again before the first claim. It reads the phase off the server's own broadcasts, so it is right whatever the thresholds become. `smash-selector-e2e` now also gets `serverEnv = smashGateLobby.env`, so it runs against the same declared 4/8 lobby as `smash-e2e`. ## One shared helper, and what it deliberately does not do The message both harnesses print is now `hub_lost` in `smash-match.py`, which `smash-selector.py`, `hotbar-check.py`, `hud-check.py` and `identity-check.py` all already load. One failure, one wording. It shares the consequence, not the number, and it cannot share the number. There is no single safe roster: the sweep wants 3, the selector wants 3 for unrelated reasons, and filling a lobby wants 8. The thing that was duplicated is `LobbyConfig::default`, and the right number of copies of that in Python is zero rather than one. A helper returning "the largest roster that will not start" would be the fourth copy wearing a hat, so `hub_lost` says in its own docstring that it will not answer that. ## Stale prose, three files `hotbar-check.py` claimed the lobby needs four players; it needs two, and that gate uses one client so only the reason was wrong. `smash-map-check.py` and `hud-check.py` both called 8 "`full_players`" rather than "at or above it". A comment asserting a false property is worse than no comment.
andrewgazelka
added a commit
that referenced
this pull request
Jul 28, 2026
## Effect `smash-selector-e2e` gets the same treatment `smash-e2e` got in #1027. It had the identical defect, twice, and #1019 broke it the same way. ## The two copies `FULL_PLAYERS = 8`, under a comment naming `LobbyConfig::default` as its source, which is what made it a copy. #1019 moved that default to 4 and the number here stayed at 8, still claiming to be it. It is now `--full-clients`, declared by the gate off `SMASH_FULL_PLAYERS` so the two cannot disagree. And "Three, which is one short of `min_players`" above its own `connect(3)`. Three is now one *over* the minimum, so a countdown runs underneath the hub checks instead of after them. Three stays, because the checks need three named roles -- a holder, a watcher, and somebody to take a freed mob -- but it is no longer described as arithmetic on a threshold, because it never was. ## The guard, not the number `hub_only` fails the run if the lobby leaves `Waiting` while the hub checks still need it, called once after the clients join and again before the first claim. It reads the phase off the server's own broadcasts, so it is right whatever the thresholds become. `smash-selector-e2e` now also gets `serverEnv = smashGateLobby.env`, so it runs against the same declared 4/8 lobby as `smash-e2e`. ## One shared helper, and what it deliberately does not do The message both harnesses print is now `hub_lost` in `smash-match.py`, which `smash-selector.py`, `hotbar-check.py`, `hud-check.py` and `identity-check.py` all already load. One failure, one wording. It shares the consequence, not the number, and it cannot share the number. There is no single safe roster: the sweep wants 3, the selector wants 3 for unrelated reasons, and filling a lobby wants 8. The thing that was duplicated is `LobbyConfig::default`, and the right number of copies of that in Python is zero rather than one. A helper returning "the largest roster that will not start" would be the fourth copy wearing a hat, so `hub_lost` says in its own docstring that it will not answer that. ## Stale prose, three files `hotbar-check.py` claimed the lobby needs four players; it needs two, and that gate uses one client so only the reason was wrong. `smash-map-check.py` and `hud-check.py` both called 8 "`full_players`" rather than "at or above it". A comment asserting a false property is worse than no comment.
andrewgazelka
added a commit
that referenced
this pull request
Jul 28, 2026
## Effect `smash-selector-e2e` gets the same treatment `smash-e2e` got in #1027. It had the identical defect, twice, and #1019 broke it the same way. ## The two copies `FULL_PLAYERS = 8`, under a comment naming `LobbyConfig::default` as its source, which is what made it a copy. #1019 moved that default to 4 and the number here stayed at 8, still claiming to be it. It is now `--full-clients`, declared by the gate off `SMASH_FULL_PLAYERS` so the two cannot disagree. And "Three, which is one short of `min_players`" above its own `connect(3)`. Three is now one *over* the minimum, so a countdown runs underneath the hub checks instead of after them. Three stays, because the checks need three named roles -- a holder, a watcher, and somebody to take a freed mob -- but it is no longer described as arithmetic on a threshold, because it never was. ## The guard, not the number `hub_only` fails the run if the lobby leaves `Waiting` while the hub checks still need it, called once after the clients join and again before the first claim. It reads the phase off the server's own broadcasts, so it is right whatever the thresholds become. `smash-selector-e2e` now also gets `serverEnv = smashGateLobby.env`, so it runs against the same declared 4/8 lobby as `smash-e2e`. ## One shared helper, and what it deliberately does not do The message both harnesses print is now `hub_lost` in `smash-match.py`, which `smash-selector.py`, `hotbar-check.py`, `hud-check.py` and `identity-check.py` all already load. One failure, one wording. It shares the consequence, not the number, and it cannot share the number. There is no single safe roster: the sweep wants 3, the selector wants 3 for unrelated reasons, and filling a lobby wants 8. The thing that was duplicated is `LobbyConfig::default`, and the right number of copies of that in Python is zero rather than one. A helper returning "the largest roster that will not start" would be the fourth copy wearing a hat, so `hub_lost` says in its own docstring that it will not answer that. ## Stale prose, three files `hotbar-check.py` claimed the lobby needs four players; it needs two, and that gate uses one client so only the reason was wrong. `smash-map-check.py` and `hud-check.py` both called 8 "`full_players`" rather than "at or above it". A comment asserting a false property is worse than no comment.
andrewgazelka
added a commit
that referenced
this pull request
Jul 28, 2026
## Effect `smash-selector-e2e` gets the same treatment `smash-e2e` got in #1027. It had the identical defect, twice, and #1019 broke it the same way. ## The two copies `FULL_PLAYERS = 8`, under a comment naming `LobbyConfig::default` as its source, which is what made it a copy. #1019 moved that default to 4 and the number here stayed at 8, still claiming to be it. It is now `--full-clients`, declared by the gate off `SMASH_FULL_PLAYERS` so the two cannot disagree. And "Three, which is one short of `min_players`" above its own `connect(3)`. Three is now one *over* the minimum, so a countdown runs underneath the hub checks instead of after them. Three stays, because the checks need three named roles -- a holder, a watcher, and somebody to take a freed mob -- but it is no longer described as arithmetic on a threshold, because it never was. ## The guard, not the number `hub_only` fails the run if the lobby leaves `Waiting` while the hub checks still need it, called once after the clients join and again before the first claim. It reads the phase off the server's own broadcasts, so it is right whatever the thresholds become. `smash-selector-e2e` now also gets `serverEnv = smashGateLobby.env`, so it runs against the same declared 4/8 lobby as `smash-e2e`. ## One shared helper, and what it deliberately does not do The message both harnesses print is now `hub_lost` in `smash-match.py`, which `smash-selector.py`, `hotbar-check.py`, `hud-check.py` and `identity-check.py` all already load. One failure, one wording. It shares the consequence, not the number, and it cannot share the number. There is no single safe roster: the sweep wants 3, the selector wants 3 for unrelated reasons, and filling a lobby wants 8. The thing that was duplicated is `LobbyConfig::default`, and the right number of copies of that in Python is zero rather than one. A helper returning "the largest roster that will not start" would be the fourth copy wearing a hat, so `hub_lost` says in its own docstring that it will not answer that. ## Stale prose, three files `hotbar-check.py` claimed the lobby needs four players; it needs two, and that gate uses one client so only the reason was wrong. `smash-map-check.py` and `hud-check.py` both called 8 "`full_players`" rather than "at or above it". A comment asserting a false property is worse than no comment.
andrewgazelka
added a commit
that referenced
this pull request
Jul 28, 2026
## Effect `nix run .#smash-selector-e2e` gets the treatment `smash-e2e` got in #1027. It had the identical defect, twice, and #1019 broke it the same way. Filed as ENG-10889 while fixing #1027; fixing rather than leaving it filed. ## The two copies **`FULL_PLAYERS = 8`**, under a comment naming `LobbyConfig::default` as its source, which is exactly what made it a copy. #1019 moved that default to 4 and the number here stayed at 8, still claiming to be it. It is now `--full-clients`, which the gate derives from its own `SMASH_FULL_PLAYERS` so the two cannot disagree. **"Three, which is one short of `min_players`"** above its own `connect(3)`. Three is now one *over* the minimum, so a countdown ran underneath the hub checks instead of after them -- the comment's own stated failure mode, arrived at by the comment going stale. Three stays. The checks need three named roles (a holder, a watcher, and somebody to take a freed mob), which is a fact about this gate and not arithmetic on a threshold. What changed is that it is no longer described as arithmetic on a threshold, because it never was. ## The guard, not the number `hub_only` fails the run if the lobby leaves `Waiting` while the hub checks still need it: once after the clients join, and again before the first claim, since the reads in between cost real seconds. It reads the phase off the server's own broadcasts, so it stays right whatever the thresholds become. `smash-selector-e2e` also now gets `serverEnv = smashGateLobby.env`, so it runs against the same declared 4/8 lobby as `smash-e2e` rather than inheriting the product's. ## One shared helper, and the thing it refuses to do The message both harnesses print is now `hub_lost` in `smash-match.py`, which `smash-selector.py`, `hotbar-check.py`, `hud-check.py` and `identity-check.py` all already load. One failure, one wording. **It shares the consequence, not the number, and it could not share the number.** There is no single safe roster to share: the ability sweep wants 3, the selector wants 3 for entirely unrelated reasons, and filling a lobby wants 8. The thing that was duplicated is `LobbyConfig::default`, and the right number of copies of that in Python is zero rather than one. A helper returning "the largest roster that will not start" would be the fourth copy wearing a hat, so `hub_lost` says in its own docstring that it will not answer that. ## No mechanical gate, and what a reviewer now has to catch I considered a lint over `tools/*.py` -- forbid an integer literal on a line mentioning `min_players`/`full_players`, or a module constant named `*_PLAYERS`. I did not add one. It would have caught `FULL_PLAYERS = 8` and would not have caught `connect(3)` under a false comment, which is the other half of this same bug. A regex over the spelling that happened to occur last time is the failure mode that lets the fourth instance through while reading as protection. The real protection is the runtime guard, which does not care how the number was arrived at. It is opt-in, so three things still need a human: 1. **A new hub-dependent harness that never calls `hub_only`.** Nothing forces adoption. 2. **A roster flag without the matching `SMASH_*` export.** `--full-clients` and `--sweep-clients` only mean anything because their gate also sets the threshold; the flag and the env have to move together. Only `smash-e2e` and `smash-selector-e2e` are wired this way. 3. **`hud-check.py` and `smash-map-check.py` still pass 8 as "enough to fill" against a server they do not configure.** They pass today only because 8 is above a `full_players` of 4. If it ever goes above 8 they will quietly stop testing the full band instead of failing. I did not close this: those gates belong to other agents, and closing it means giving them `serverEnv` too. Named here rather than left implicit. ## Stale prose, three files `hotbar-check.py` claimed the lobby needs four players. It needs two, and that gate uses one client, so only the reason was wrong. `smash-map-check.py` called 8 "`full_players`" rather than "at or above it". `hud-check.py` had the same in its docstring; its `--clients` help was already corrected on main by another agent and I took theirs over mine in the rebase. ## Checks `nix run .#smash-selector-e2e`, all 15 claims: ``` PASS the hub has a podium per mob, with that mob standing on it PASS every mob wears its kit's name, always visible 15 of 15 PASS a click plays the mob's own declared sound 15 of 15 kits answered PASS and only the player who clicked hears it S2 heard none of 15 selections PASS all fifteen mobs answer in different voices 15 distinct sounds over 15 kits PASS the other player sees the new skin on the wire PASS and it did not cost the wearer its world PASS a right-click on a podium picks that mob PASS a taken mob is red without anybody reading anything PASS a mob somebody else has is refused, by name PASS the refusal is about that mob and not about clicking PASS a holder who disconnects frees their mob at once PASS a full lobby holds one mob each S2=Iron Golem; S3=Skeleton; ... S9=Wolf PASS a selection made in the hub survives the countdown 8 clients started the match on the mob they clicked for PASS a click after the match commits is refused RESULT: the kit selector works, on the wire rc=0 ``` Guard broken deliberately, by pointing `smashGateLobby.env` at production's 2/4: ``` 0.72s FAILED the lobby left the hub before the hub checks began. 3 clients is enough for this server to start a countdown, so the hub is gone and nothing after this would be proved. Either the gate's roster is too many for this lobby, or the server needs a higher SMASH_MIN_PLAYERS/SMASH_FULL_PLAYERS. RESULT: 1 checks failed rc=1 ``` That run is also the confirmation ENG-10889 was missing: at 2/4 three clients start a countdown 0.72 seconds in, so `smash-selector-e2e` really was broken by #1019 and not merely suspicious. Restored to 4/8 afterwards. ## `smash-e2e` may be red on main, but I could not confirm it here, and this PR does not touch it Reported honestly rather than overstated: `nix run .#smash-e2e` failed on a Sky Squid `shields_caster` probe in every run of mine that reached it (5a486fd twice, 54de2ba once), filed as ENG-10906. But I cannot confirm a real bug from this machine, and the ticket now says so. The box has sat at load average ~20 all night (22 users, seven agents). The shield probe hits the caster immediately after the press expecting the immunity tag already applied, which makes it the tightest-timing probe in the sweep and the first to flake under load. When I stacked a `cargo` compile on top, the sweep took 377s instead of 121s and *five* unrelated timing probes failed (both shields, Storm Squid's launches, `buffs_melee` on two kits) -- a load signature, not five bugs. The load-immune instrument disagrees with the wire: `events/smash/tests/abilities.rs::every_declared_effect_actually_happens` passes on 54de2ba, and it is not a blind test -- I made `arm_shield` a no-op and watched it fail in 1.2s with verbatim the same sentence. So the shield works in-process, and the leading explanation for the wire failure is the probe racing the shield's application, not a broken shield. #1032 already retired the old "hit 50ms after the press" probe for exactly this reason. The wire verdict needs CI's `Flake` job or an idle host; ENG-10906 is rescoped to that rather than closed or asserted. None of this is in scope for this PR, which touches only the harness wiring and prose. Its own gate, `smash-selector-e2e`, is green. Python: `python3 -m py_compile` on all five changed harnesses, and `ruff check` clean on `smash-selector.py` before and after.
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.
Effect
The countdown starts at 2 players instead of 4, and 4 players is a full lobby instead of 8. Requested by the operator so the match phases are reachable by the two or three people actually on this server.
Why the defaults and not the state machine
LobbyConfigis already a singleton, and its own doc comment says it exists "so a server can run a two-player duels variant without touching the state machine". This uses that seam as designed.step,countdown_forand every phase transition are unchanged.The numbers still separate
countdown_forhas three bands. At 2/4 they stay distinct:countdown_at_min, 60scountdown_at_three_quarters, 30scountdown_at_full, 10sDropping
full_playersbelow 4 would collapse two bands into one. That reason is written next to the constant, so the next reader can tell the fence from a fossil.Why the diff is bigger than two lines
Six tests in
events/smash/tests/lobby.rsbroke that were not about these numbers. They spelled the counts as the literals 3, 4, 6 and 8, which statedLobbyConfig::defaulta second time. They now derive from the config through two named helpers, so the next threshold change moves the tests with it instead of breaking them.Checks
tests/hud.rs,tests/sound.rs,tests/selector.rs,tests/verification.rsandtests/harness/mod.rsall setLobbyConfigexplicitly and were unaffected, which is the design working.