test(smash): the selector check waits for the packet it asserts on - #1083
Merged
Conversation
`smash-selector-e2e` fails intermittently with
FAILED S1 picked the Skeleton and the action bar never said so: []
and the line it says never arrived is in the same log 0.2 ms later. The
server is not at fault. `lobby::choose` sends two packets, a chat line and
then a separate `Channel::ActionBar` line, and the check waited on
`client.kit`, which `on_chat` sets from the *first* of the two, and then
read the action bar with no further wait. The harness reads one packet per
pump, so a burst split across two reads leaves the second packet still on
the wire when the assertion runs. Draining harder would not have helped;
the bytes had genuinely not arrived.
Every other action bar assertion in the suite either settles first or waits
on the action bar itself. This was the only one keying its wait on a
different packet that merely precedes the one it asserts on. The rest of
the clients were swept and there is no second instance.
While here: the function printed PROVED for the same claim it had just
marked FAILED, so one run reported both. Removed.
Proved in both directions on a Linux builder rather than by one green run,
because the unfixed tree also passes on a good run and a single pass here
proves nothing:
fixed PASS ... the server answered ['You are the Skeleton.']
negative with the `Channel::ActionBar` send deleted from
`lobby::choose`, rc=1, failing after the full 10 s wait
rather than passing vacuously
Diagnosed and fixed by a subagent; the negative control was its own idea.
Benchmark Results for generalComparing to 34178ea |
andrewgazelka
added a commit
that referenced
this pull request
Jul 30, 2026
`smash-selector-e2e` failed in CI with exactly one assertion:
FAILED S1 picked the Skeleton and the action bar never said so: []
The server had sent that line. Run 30500640846 recorded the failure at
`00:21:41.7509285` and logged the line it was waiting for at
`00:21:41.7511018`:
```
00:21:41.7508758 15.94s [S1 ] <- chat: Kit set to Skeleton.
00:21:41.7509285 15.94s FAILED S1 picked the Skeleton and the action bar never said so: []
00:21:41.7511018 15.94s [S1 ] <- action bar: You are the Skeleton.
```
The check waited on `client.kit`, which is set from the chat packet
`lobby::choose` sends one ahead of the action bar, and `drain` takes a
single
`recv` per pump, so a burst split across two reads satisfies the wait
with the
asserted-on packet still in flight. Production code was not at fault.
That fix landed on its own in #1083. This is the rule that stops it
recurring.
## What this adds
`nix/verify-wire-assertions.py`, wired in as a flake check so the
subtractive
gate in `nix/ci/flake-gate.nix` picks it up without anybody adding it
anywhere.
The rule: an assertion about a field the packet handler fills must be
preceded,
in the same function, by a wait whose predicate reads that same field.
`must_become` and `must_not_become` make the predicate the assertion, so
waiting
on one thing and asserting on another is not expressible through them.
The rule is deliberately narrow. It only looks at functions that wait at
all,
because the bug is not "forgot to wait", it is "waited, and for the
wrong
packet". The observed field set is derived from what the packet handlers
fill
rather than listed, so a new handler brings its field under the rule the
day it
lands. Two limits are documented in the script: it does not follow a
field
through a helper call, and it says nothing about functions that never
wait.
## What it flags today, and what I did with it
Three sites, which is what the rule reports on this tree rather than a
set I
picked. I deliberately did not hand-convert the other 64 assertions:
that diff
is mechanical, and a conversion with a subtly wrong predicate turns a
real
assertion into one that cannot fail while the gate stays green.
| site | verdict |
| --- | --- |
| `a_click_picks_the_mob` | the shipped bug, now `must_become` |
| the mid-match refusal | real: slept `settle(1.0)` where a second
happened to be generous. Now `must_not_become` for the kit and
`must_become` for the refusal |
| the lobby-count assertion | false positive. Takes the escape hatch,
with a reason |
The escape hatch is `# not-a-wire-assertion: <why>` and the reason is
part of
the syntax: a marker with nothing after the colon is itself an error,
because a
bare marker is a mute button.
## Break-it results
The lint was broken three ways and watched, through the real flake
check. Three
cases and not one, because a rule that rejects everything passes the
reject-the-bad-thing test perfectly: the first version of this script
flagged 67
of 67 assertion sites.
- rejects the shape that shipped: `smash-selector.py:1191: in
a_click_picks_the_mob, this fails on action_bar, which the packet
handler fills, and no wait before it reads it.`
- accepts a correct `must_become`: `14 client scripts checked, 0
problems`
- rejects a marker with no reason: `smash-selector.py:1349:
not-a-wire-assertion with no reason after the colon. The reason is the
point: say which non-wire thing this asserts.`
Because two live assertions changed, the gate they live in was re-run
rather
than trusted: `nix build .#checks.x86_64-linux.smash-selector-e2e`,
rc=0, with
both converted claims passing.
## Enforcement, stated plainly
The check runs in CI automatically via the `Flake` job and nobody has to
remember it. It does not *block* a merge: ruleset 566717 on `main`
carries no
`required_status_checks` rule, so no check in this repository is a
required
context and a red gate is enforced by somebody noticing. ENG-10827.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1083 +/- ##
=======================================
Coverage 54.65% 54.65%
=======================================
Files 361 361
Lines 33257 33257
Branches 1259 1259
=======================================
Hits 18178 18178
Misses 14793 14793
Partials 286 286 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
smash-selector-e2efails intermittently withand the line it says never arrived is in the same log 0.2 ms later.
The server is not at fault.
lobby::choosesends two packets, a chat lineand then a separate
Channel::ActionBarline. The check waited onclient.kit, whichon_chatsets from the first of the two, then readthe action bar with no further wait. The harness reads one packet per pump,
so a burst split across two reads leaves the second packet still on the wire
when the assertion runs. Draining harder would not help; the bytes had
genuinely not arrived.
Every other action bar assertion in the suite either settles first or waits
on the action bar itself. This was the only one keying its wait on a
different packet that merely precedes the one it asserts on.
Proved in both directions on a Linux builder, because the unfixed tree
also passes on a good run and one green run here proves nothing:
fixed PASS ... the server answered ['You are the Skeleton.']
negative with the
Channel::ActionBarsend deleted fromlobby::choose,rc=1, failing after the full 10 s wait rather than passing
vacuously
That the negative control was necessary rather than merely thorough is now
measured:
smash-hud-e2eandsmash-selector-e2eeach failed one CI runand passed the next on effectively the same tree. Two of five failures in
that suite are coin flips.
Split out of #1082 deliberately. That PR carries this fix plus a 322 line
static analyzer that stops the whole class being written again, and the
analyzer deserves an unhurried review. This half is costing a 35 to 90
minute CI run every time it fires and reads as a server bug to whoever sees
it next, so it should not wait on the discretionary half.
Diagnosed, fixed and controlled by a subagent; the negative control was its
own idea. Split and landed by me.
(sent by an AI agent via Claude Code)