r1 review fixes for the bc20 year module (F1-F15) - #2
Merged
Conversation
The "the doctrine panel may not take half the frame height" verdict in tools/ci/renderer_fixture.html read `#doctrines` unconditionally. Under `data-year="bc20"` that element is `display: none !important` (client/replay_broadcast.html:2653), so its rect is 0x0 and the rule passed vacuously on all three bc20 rows. It now measures `probe`, the same year-aware id the CSS-loaded check already resolves. Evidence: with the threshold temporarily forced to 0, the fixture reports three `the doctrine panel takes N%` errors before this change (the bc26 rows only) and six after -- the bc20 rows now measure 17-20% of their frame. At the shipped 0.5 threshold the fixture is green.
`dropUnit`/`dropHeldUnit` emit the event for ANY unit dropped onto a flooded tile -- a friendly landscaper or a neutral cow as readily as an enemy -- but match.nim built `victim_alias` as `aliasOfTeam(1 - e.b)`, i.e. always the other clan. The kill-feed line and the `drop` beat therefore named the wrong victim on a friendly or cow drop. The event now carries the dropped unit's team ordinal in its string slot and match.nim maps it: clan alias for 0/1, `neutral` for a cow. No counter, no hash-chain input and no results key changes. tests/test_bc20_drone.nim gains a friendly-drop and a cow-drop case and asserts the victim team on the existing enemy-drop case (33 checks, was 28).
decide.nim's branch for a reply that sends a `chassis` key printed "the clan runs the awu chassis" on every year. On bc20 the seat runs `bowl-of-chowder` (or the baseline its PLAYER_SCRIPTED names), so the line misinformed the operator on exactly the episode it exists to explain. It now prints `chassisNameFor(config.year, seats[slot], sheet)` -- the same resolution server.nim records on the seat. The record-and-never-honour behaviour is unchanged and still asserted by tests/test_bc20_sheet.nim.
`runFulfillmentCenter` implements only the roster/pool test; the module doc comment also promised "and always when `NEED_DRONES` is on the chain", which no code path provides -- nothing broadcasts `SigNeedDrones` and `readBlocks` drops it into `else: discard`. Documented rather than implemented: the branch would guard a signal that never arrives, and inventing a broadcaster is a play change, not a comment fix. The header now says what the code does, `SigNeedDrones` is marked RESERVED with the reason its code point is kept (renumbering the table would change the meaning of every recorded message), and docs/RULES-BC20.md gains §Divergences item 15. No behaviour change.
The design note's per-seat observation sample carries `rules_digest` and `sheet_schema`; `briefFor` emits neither. Both contents do reach every seat -- they are in `Bc20Preamble`, the system message, which the replay records once as `prompt_preamble` rather than twice inside `seats[].prompt`. Documented rather than changed: duplicating the ~7 KB digest and the whole knob table into each seat's recorded observation would grow every replay for no new information. docs/PROTOCOL.md's bc20 observation section now names both keys, says where they live, and tells a consumer which field to read.
`floodTableJson` reports `roundWaterReaches(7)`, which is the sentinel `WaterTableMaxRound + 1 = 1501` -- the committed water table covers rounds 0..1500 and the water never rises above elevation 7 inside the cap. The design note's payload shows 1546, the uncapped curve's real value, which is also what §Divergences item 4 of docs/RULES-BC20.md states. Documented rather than changed: recording the true 1546 would mean generating and committing water levels for rounds the sim can never play, and the committed table is byte-diffed against the JDK generator as a blocking CI step. docs/PROTOCOL.md now names both numbers, says which one the payload carries and why, and `floodTableJson`'s doc comment points at it. Levels 1-6 are unchanged and still pinned by tests/test_bc20_flood.nim.
`nextBuilding` inserts a Refinery second and sites every building at Chebyshev 2 (4 for the Refinery). The design note's order has no Refinery and puts the net guns on the HQ ring. The net-gun move was in the build report and in miner.nim's header, which pointed at docs/RULES-BC20.md -- where no such entry existed; the Refinery was explained only in a comment at the branch. Both are now §Divergences item 16 with the rule that forces each: a walled HQ is eight elevation steps up against MAX_DIRT_DIFFERENCE 3, so miners need a second drop-off the moment the wall closes; and dirt dropped on a building buries it, so a net gun on the ring is buried by its own wall. miner.nim's header now lists the Refinery it actually builds. No behaviour change.
The loop claimed "record -> re-derive covered every end reason" while three
of the seven entries were `seenReasons.add("...")` literals, `quality` was a
world-level ladder vector, and `abandoned` was added OUTSIDE the `if reason ==
epDeadline` branch. The branch never fires: a 1499-round CentralSoup game
re-derives in 268 ms in release, so a one-second guard cannot trip, and the
coverage loop passed on a path that had never run.
Now:
* two lists, `reDerived` and `ladderVector`, and the loop names which one each
reason must be in -- a reason nobody produced can no longer satisfy it;
* `broadcasts`, `highest_id` and `coin_flip` are real vectors through the same
`checkEndOfMatch` a played game calls, on a bare world (the committed maps
arrive with their own HQs and cows, which is why the last rungs need one);
* `abandoned` is end-to-end and deterministic: the recorder's own guard is
made to fire by holding the clock in `playGame`'s round callback -- the real
code path, not a mock -- and the resulting stop round is written as
`plan.abandon_after[0]`, re-derived from the written bytes, and the
deriver's hash chain at the stop round is compared against the recorder's
own `roundChains` entry. The abandoned game carries no GameHeader, so that
comparison is the only thing that proves the two agree.
59 checks, up from 45. Nothing was removed or loosened.
Both knobs were gated on the same proxy -- "in one more of the four games a friendly unit stood Chebyshev <= 1 from the enemy HQ" -- where the design note's table asks for a round delta (`opening`: the first enemy-half unit arrives >= 200 rounds earlier) and a deadline (`rush_trigger`: adjacent to the enemy HQ by round 350). Neither substitution was declared, unlike `net_gun_ring`'s. Both statistics turn out to be measurable, so they are measured rather than declared. `runSet` now records, per game, the round a friendly unit first stood closer to the enemy HQ than to its own (the round cap standing in for "never crossed") and whether one stood adjacent to the enemy HQ by round 350. * `opening` keeps the arrival counter and adds the note's statistic: turtle -> rush crosses into the enemy half 233 rounds earlier per game (1811 -> 878 summed over the four games), gated at 100 a game -- the same half-the-measured-delta rule the other nine thresholds follow. * `rush_trigger` keeps the arrival counter and adds the note's clause verbatim: 0 of 4 games reach the enemy HQ by round 350 at `0`, 1 of 4 at `220`. 16 checks, up from 14; no gate was removed or loosened. The header table records both new measurements and, with them, corrects its "Measured at GameVersion GV04" line to GV05 (also r1-F11).
The design note's rule 6.1 reads like a legality check -- "a drone may enter a flooded tile; nothing else may" -- and the port does something else: `canMove` never tests flooding and `move` destroys a non-flying mover. That is the engine, and it was asserted only by a comment. docs/RULES-BC20.md §Divergences gains item 17 with the upstream citation at the pinned commit 7618f6b: `RobotControllerImpl.assertCanMove` (:344-365) tests type, adjacency, bounds, occupancy, MAX_DIRT_DIFFERENCE and readiness and never mentions flooding; `move` (:382-391) tests it afterwards and calls `disintegrate()`, which throws RobotDeathException (:937-939); `GameWorld.updateRobot` (:190-191) then destroys the robot. `world.move`'s comment carries the same citation. tests/test_bc20_flood.nim now pins the path the oracle cannot reach: the move is legal, the miner dies, neither tile holds it, and a drone flies onto the same tile and lives. 32 checks, up from 26.
Four pointers named nothing: * `years/bc20/constants.nim:5` credited `tests/test_bc20_constants.nim` with regenerating and byte-diffing it; no such file exists. The gate is the `test` job's `gen_year_constants.py --year bc20 --check`. Fixed in the GENERATOR's header template and the file regenerated against the pinned sources, so `--check` still passes byte for byte (verified locally). * `tests/test_bc20_maps.nim:2` credited `tools/ci/check_bc20_maps.sh`; the byte-diff is `tools/convert_maps_bc20.py --check` in the same job. * `tools/ci/check_gameversion.sh`'s own comment still quoted GV04. * the script was wired into no workflow at all. It is now a step in the `test` job on every non-main ref, against a depth-1 fetch of origin/main. Same number + same rule headline exits 0, which is every PR that does not touch the rules -- including this one (verified locally: base GV05, head GV05, "no rule change claimed"). The fourth stale pointer, test_bc20_knobs.nim's "Measured at GameVersion GV04", was corrected to GV05 in the r1-F9 commit, which rewrote that table.
Nothing in src/ read `.baseline` after the year-aware baseline resolution landed: the only write was `app.policy[slot].baseline = blAwu` at server init, and every consumer goes through `baselineForSeat(year, seat)`, which resolves the raw `PLAYER_SCRIPTED` string per year. A field holding a bc26 baseline on a bc20 seat is exactly the confusion the year-aware change removed. Field, its one write and its one mention in a test constructor deleted; the `scripted` field's doc comment now says where the resolution happens and why it is not done here. No behaviour change; test_sheet, test_seats and test_bc20_sheet unchanged in count.
§Tests item 17 asks the wasm module to answer `bc_load_replay`/`bc_frame` on a committed bc20 fixture replay; `tests/fixtures/` held only `java_random_vectors.json`, and the smoke ran against the replay docker-smoke produced in the same run. It earns its keep: 5 881 bytes, 119 rounds, byte-identical on a re-run (the world RNG comes from the map's own randomSeed), and it is a REAL recording written by the same `ReplayDoc.toJson` the server writes. * `tools/gen_bc20_fixture_replay.nim` records it -- bowl-of-chowder vs examplefuncsplayer on maptestsmall, seed 3, 120 rounds -- and refuses to write a recording that does not re-derive. * `tests/fixtures/replay-bc20.json` is that recording. * `tests/test_bc20_replay.nim` proves the committed bytes still parse, carry a `GameVersion` in `ReplayCompatibleGameVersions`, and re-derive round for round under the current sim. A rule change turns this red, with the re-record command in the assertion's own message. 67 checks, up from 59. * `ci.yml` runs `wasm_replay_smoke.cjs` against it as a third target, so the wasm32 re-derivation is driven by committed bytes and not only by bytes the same run produced.
The design note lists six values for `first_build.unit` and spells the drone `drone`. `Bc20UnitNames` is the ten `RobotKind` ordinals, so the emitted vocabulary is the engine's own type names — `delivery_drone`, and also `miner` and `refinery`, both of which the chassis genuinely builds. Documented rather than narrowed: the beat names the type that was built, and renaming or suppressing kinds would make the feed line disagree with the sim while hiding two builds a spectator can see happen. docs/REPLAY.md now lists the eight reachable values, says why the drone is `delivery_drone`, and points at §Divergences item 16 for the miner and the refinery. Every value draws as the `build` beat, which has CSS.
Found while verifying the fixture commit above: the "Smoke the emitted wasm
module under node" step completes in 0.1 s with NO output, and has done so on
every run including main's green 33841592052. It exits 0 having tested
nothing, so committing a fixture for it bought nothing.
Cause: the emitted glue opens with
`var Module = typeof Module != "undefined" ? Module : {}`. Under `require()`
that `var` is hoisted into the module scope and shadows `global.Module`, so
`typeof Module` is "undefined" at that line and the glue builds its own empty
Module. `onRuntimeInitialized` -- the whole body of this smoke -- belongs to
our object and is never called; node then runs out of work and exits 0.
Fix: run the same bytes through `vm.runInThisContext`, where the declaration
sees the global object that already carries `Module` and adopts it, with
`__dirname`/`__filename`/`require` supplied because the glue uses them under
ENVIRONMENT_IS_NODE. Plus a 60 s watchdog that exits 1 with a message rather
than exiting 0 in silence.
Verified against the bundle CI built for this very run (downloaded from the
`static-replay-viewer` artifact of run 33846271859) on all three targets:
replay.json loaded=true GV05 first_packet=74797 frames=200 mismatch=-1
replay-bc20.json loaded=true GV05 first_packet=71039 frames=200 mismatch=-1
fixtures/replay-bc20.json loaded=true GV05 first_packet=69456 mismatch=-1
and as a negative control `results.json` now exits 1 with "not a
cogame-battlecode-replay document" where before it also exited 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.
Round-1 review fixes for
runs/2026-09-04-battlecode-2020-soup, one commit per finding.Code: F1 (the renderer fixture measured the hidden bc26 doctrine panel on its bc20 rows), F2 (
drone_water_dropnamed the wrong victim for a friendly or cow drop), F5 (the ignored-chassislog line said "awu" on bc20), F8 (the end-reason coverage loop passed on string literals and on anabandonedbranch that never fires), F9 (openingandrush_triggernow gate on the design note's own statistic), F11 (three stale pointers, andcheck_gameversion.shwired into CI for the first time), F12 (deadSeatPolicy.baselineremoved), F13 (the committed bc20 fixture replay the note names, plus a native re-derivation test and a third wasm-smoke target).Documentation, where the code is right and the note is not: F3, F4, F6, F7, F10, F14.
No test was weakened, skipped or deleted. bc20 shard check counts: drone 28->33, flood 26->32, replay 45->67, knobs 14->16; every other shard unchanged. All 32 shards pass locally in debug and release.