bc16 r1 review fixes (F1-F11) - #11
Merged
Merged
Conversation
Review finding F2 (ACCEPTANCE CHECKLIST item 15): bc16 draws LLM-authored
text into `#bc16-doctrines-body` — a 280-rune `notes`, a 48-rune `motto` and
a 120-rune submitted sheet — and `tools/ci/renderer_fixture.html`'s `YEARS`
array had no `bc16` entry, so the one gate in this repo that renders model
text at its cap never set `data-year="bc16"`, never emitted a `#bc16-*` id
and never measured anything bc16 draws. design.md:2832 asked for the row.
What changed:
* `tools/ci/renderer_fixture.html` — `bc16` appended to `YEARS`; a bc16 row
reproducing renderArchons / renderHorde / renderEcon / renderUnits /
renderDoctrines element for element and class for class, with both seats at
the full 280-rune `notes` and 48-rune `motto` and with plainWords16()'s
WIDEST possible eleven clauses (502 chars); every stat MEASURED as the
widest value the field reached on any round of a `bulwark` mirror over all
six `small` maps at the 3000-round cap; `#bc16-doctrines-toggle` beside its
seven siblings; and a `bc16` entry in the FILLED map so the
"hides its own content" rule runs over bc16's five readouts.
The existing `Render the full-cap doctrine-text fixture` step
(`ci.yml:4253`, already `--strict-text-bounds`) therefore now lays bc16 out
at 360/720/1280 px and asserts the strings are still full length.
* `client/replay_broadcast.html` — two defects the new row found, both inside
bc16's own additions block:
- `#bc16-horde` was one nowrap flex line ~570 px wide inside a
`max-width: 92%; overflow: hidden` box, so at 360 px (331 px) and at
720 px the tail — `DENS`, the mini-timeline and the tiebreak countdown —
was laid out past the frame and clipped to nothing, against the design
note's explicit "keeps its wave composition, its multiplier and its
countdown at EVERY width including 360 px". It now wraps.
- `#bc16-econ`/`#bc16-units` rows likewise laid ~640 px of fields out
inside a 344 px box at 360 px, so `rubble +x / -y` was drawn off-frame.
The rows now wrap. Fields still never break mid-number.
- bc16's `notes` is now wrapped in `<i>` like all seven sibling years',
because `#<year>-doctrines .dline i` is the selector the fixture's
"the notes on seat N were shortened" check uses; a bare text node there
would have made that check vacuous for bc16 exactly as bc25's own class
names once did.
* `tests/test_viewer.nim` — the `YEARS` literal updated, and static checks
that the bc16 row exists, fills bc16's own readouts, carries both seats at
the cap with the envelope badge / motto / submitted sheet, is in the FILLED
map, and that the page wraps bc16 notes in `<i>`.
Verified locally against headless chromium 141 (the fixture served exactly as
ci.yml serves it, page CSS extracted the same way): all 24 year/width cells
report `ok`, and a direct probe of the bc16 cells shows 2 notes at 280 runes,
both mottos at 48 runes, the doctrine panel at 46 % of the frame height, and
`WAVE`/`OUTBREAK`/`ROUND` all inside the frame at 360 px.
`nim r tests/test_viewer.nim` → `test_viewer: ok (845 checks)`.
computed style proves it
Review finding F1: `client/replay_broadcast.html:3792-3799` shipped
html[data-year="bc16"] #endcard.show ~ #bc16-archons, … { visibility: hidden; }
and it was dead twice over. The class the page toggles is `.on`
(`#endcard.on { display: flex; }` at :1878, `classList.add('on')` at :6823,
`remove('on')` at :6771) — nothing anywhere sets `.show`. And `~` selects only
FOLLOWING siblings, while `#bc16-archons`/`-horde`/`-econ`/`-units`/
`-doctrines` (:3991-3999) are emitted BEFORE `#endcard` (:4057) inside
`#chrome`, so no sibling combinator could have matched whatever class it
named. `#endcard`'s background is a radial gradient at 0.82-0.95 alpha, so the
five boxes sat visible behind a score screen that is not opaque. This is the
only `visibility: hidden` declaration in the page; nothing else hid them.
The rule is now parent-scoped and keyed on the class that exists:
`html[data-year="bc16"] #chrome:has(#endcard.on) #bc16-…`. `:has()` is
supported by the headless chromium the smoke runs (verified: 141.0.7390.37,
`CSS.supports('selector(:has(*))')` → true). The change is inside bc16's own
additions block; no inherited chrome above the banner is touched and
`client/chrome_common.js` is not touched.
AND THE TESTS THAT COULD NOT SEE IT:
* `tests/test_viewer.nim:1425-1428` asserted the DEAD STRING WAS PRESENT. It
now pins the parent-scoped shape, asserts the two dead shapes are ABSENT,
and asserts every one of the five boxes is named in the rule.
* `tests/test_viewer.nim:490-491`'s `"#endcard.show {" notin page` passed only
because the bc16 rule's text was `#endcard.show ~ …`, not `#endcard.show {`.
It now reads the page's whole <style> block and refuses the substring in ANY
selector shape.
* THE REAL GATE IS NO LONGER A GREP. `tools/ci/renderer_fixture.html` gains
the page's own `#endcard` as a child of `#chrome`, and a
`SUPPRESSED_BY_ENDCARD` check that raises the card exactly as renderEndcard
does and reads `getComputedStyle(...).visibility` on all five boxes — then
takes it down and asserts they come back, because every seek dismisses the
card and the scrubber has to be able to pull the match back. That runs in
the existing `Render the full-cap doctrine-text fixture` step at 360, 720
and 1280 px, and `tests/test_viewer.nim` asserts the fixture carries it.
Evidence: with the fix, all 24 year/width cells of the fixture report `ok`
against a locally served copy driven by headless chromium 141 (page CSS
extracted exactly as ci.yml extracts it). With the dead selector restored,
the three bc16 cells report
`the endcard is up and #bc16-archons, #bc16-horde, #bc16-econ, #bc16-units,
#bc16-doctrines is still visible — the HUD bleeds through a score screen that
is not opaque`. `nim r tests/test_viewer.nim` → `ok (855 checks)`.
against the build that shipped Review finding F4: both inline tables were stale. They claimed healthy "3 of 6 … 20 dens killed; median 2147 rounds" and a broken median of 1029, while `ci.yml` run 34322655506 (`main` @ `fbc7d345`, `test` job 102372608026) measured `HEALTHY games=6 notDestroyed=3 dens=14 median=2015 rounds=@[3000, 695, 3000, 707, 3000, 1030]` and `median=873` for the control. Both tables — in `tests/test_bc16_survival.nim`'s header AND in `docs/RULES-BC16.md` §Divergences item 16 — are regenerated from the shipped build, and BOTH PLACES NOW NAME THE CI RUN THE NUMBERS CAME FROM, so a future staleness is detectable rather than invisible. The stale per-map rows were `zigzag` and `frogger` on the healthy side (605/1294 rounds against 695/1030) and `zigzag`/`frogger` on the broken side; the broken table gains the damage column it lacked, because that is the column F3 needs. Confirmed on the regenerated numbers, and stated in both places: every committed floor still holds — `MinNotDestroyed` 2 ≤ 3, `MinUnitsBuilt` 25 ≤ 58, `MinDamageDealt` 1500 ≤ 3608, `MinGuardsBuilt` 2 ≤ 5, `MinDensKilled` 4 ≤ 14, `MinMedianRounds` 1000 ≤ 2015, parts income 1 ≤ 11724 tenths, `swamp`'s pair clause 1 ≤ 1200 tenths. NO FLOOR WAS MOVED and no clause was dropped. Evidence: `nim r -d:release tests/test_bc16_survival.nim` on this tree prints `HEALTHY games=6 notDestroyed=3 dens=14 median=2015 rounds=@[3000, 695, 3000, 707, 3000, 1030]` and `BROKEN-CONTROL games=6 notDestroyed=0 dens=0 median=873 failures=16` — digit for digit what run 34322655506 printed — and finishes `test_bc16_survival: ok (11 checks)`.
anti-degeneracy floors the control also clears Review finding F3: `MinUnitsBuilt` 25, `MinDamageDealt` 1500 and `MinMedianRounds` 1000 sit below what the `-d:bc16BrokenChassis` control already achieves, so they do not discriminate — while the test header and `docs/RULES-BC16.md` §Divergences item 16 read as though all the substance clauses did. THE FLOORS ARE NOT FITTED TO THE CONTROL AND NOTHING IS LOWERED. Raising `MinDamageDealt` above the broken column's 3333 would put it above the HEALTHY weak seat's 3608 — fitting a floor to noise, and it would redden healthy runs. So every floor value stays exactly where it is, no clause is dropped, and the labelling is corrected instead. Both places now carry the same table: discriminating (a HARD ZERO on the control): the 2-of-6 ratio (0 of 6), guards built per seat per game (0), dens killed across the six maps (0), and `swamp`'s parts-collected-across-the-pair clause (0). anti-degeneracy only, and the named control CLEARS them: units built (25 against the control's 55), damage dealt (1500 against 3333), parts income (1 tenth against 10 621). They stay asserted because what they catch is a chassis that stops acting at all — the do-nothing sheet that wins because the opponent starved, the 2026-09-03 finding — not this control. the median floor sits INSIDE the control's own noise band, said explicitly: the previous session recorded 1029 (ABOVE the 1000 floor) and the shipped build measures 873 (BELOW it), so it is counted with the anti-degeneracy floors even though it does fire today. And CI's own run is quoted as agreeing: run 34322655506's control failed on sixteen clauses, and the eight it printed are all `guards built 0 < 2` plus `swamp: parts collected across the pair (tenths) 0 < 1` — not one printed failure is a units-built, damage-dealt, income or median failure. No code changed; the gate still runs and the control still comes back red: `nim r -d:release tests/test_bc16_survival.nim` → `BROKEN-CONTROL games=6 notDestroyed=0 dens=0 median=873 failures=16` / `test_bc16_survival: ok`.
…er ran Review finding F5: Tier A′ (the four scenario bots) was not built — `tools/oracle/bc16/` holds only `bc16greenhorn` and `bc16idle`, `ci.yml` builds only those two emitters, and the gap is honestly disclosed in `docs/PARITY.md:1844-1885`. But `tools/ci/parity_tiers_bc16.py` headed its summary column `tier A/A'` and printed "The phase-30 exit condition is Tiers A, A' and B passing with an EMPTY ledger" into the CI STEP SUMMARY, beneath a table of eighteen bit-exact pairs. A reader of the summary alone would conclude Tier A′ ran and passed. Tier A′ is NOT built here — that is out of scope for this round and stays disclosed where it already is. What changed is only the claim: * the column now reads `tier A / A″`, the two tiers the script actually compares; * the exit-condition line names Tiers A, A″, B and C, and says which of them this script runs (A, A″, C) versus which is the job's own byte-diff step (B); * a new line under the table states outright that **Tier A′ was NOT BUILT and did NOT run**, that nothing in the table is evidence for it, and points at `docs/PARITY.md` §bc16 "Tier A′ — NOT IMPLEMENTED" together with the exact consequence: `more_archon_health` and `more_parts_net_worth` have no Java-side evidence. No comparator logic is touched; only the strings it prints. `python3 tools/ci/parity_tiers_bc16.py --selftest` → `10 cases, all four known comparator bugs plus the origin tripwire covered`, exit 0. The identically-worded lines in the bc22/bc23/bc25 comparators are left alone: those years DID build their scenario bots, and touching them is outside this finding.
design.md:2574 Review finding F6, and the coordinator's ruling: NO CODE CHANGE. The substituted floors are sound and already reasoned in `ci.yml:3013-3023` — neither oracle bot survives to round 3000, measured 298-683 rounds for `bc16idle` and 485-1424 for `bc16greenhorn`, so the note's literal per-game 2900-round assertion would fail all eighteen pairs — and the trace runs to the engine's own `isRunning() == false`, so the end round, the winner and the domination factor are themselves compared. What was missing is the LEDGER LINE. `docs/PARITY.md` §bc16 now carries an entry naming the divergence from design.md:2574 explicitly: the per-game round floor is 250 rather than 2900 (with the measured bot lifetimes as the reason), and the zombie floor is 150 SUMMED `zombies_peak` over the eighteen pairs (measured 735) rather than 150 spawned per game, because peak-on-board is the number the trace carries. It also records the three floors the job adds that the note did not ask for (peak robots >= 10, an infection on >= 9 of 18 pairs, `saw_zombie_turn=true` on every pair), so the design-note-vs-shipped ledger is accurate in both directions.
Review finding F7: `src/battlecode/years/bc16/rules.nim:395-400` folded the
six player-type censuses base-100/base-1000000 into two `mixHash` calls and
`:418-421` folded the four zombie censuses base-100 into one. Any single count
of 100 or more carried into the next field, so two distinct censuses could
fold to the same chain value. Not hypothetical on this year: the parity job
measures `peak_robots` of 104-162 and the survival gate builds 177-212 units a
seat on `checkers`/`prisons`. The chain is a TRIPWIRE — a collision can only
hide a divergence, never manufacture one — which is exactly why a blind spot
in it had to go.
BOUNDED TO bc16's OWN PACKING, as ruled. `src/battlecode/match.nim:498-506`
(`archons` div/mod 100, `parts_worth` div/mod 100000) is DELIBERATELY NOT
TOUCHED: it is year-neutral and widening it would move every sibling year's
committed chain values. That limitation is recorded in `docs/PARITY.md` §bc16
with the measured peaks as the evidence that the margin is finite, and with
the condition a future year must check before it ships.
* `rules.nim` — nineteen per-team values (was fifteen) and thirteen globals
(was ten), every census its own call. The header comment's counts are
corrected to match.
* `tests/fixtures/replay-bc16.json` — re-recorded with
`tools/gen_bc16_fixture_replay.nim`. NO ASSERTION WAS WEAKENED to make the
new recording pass; one was ADDED (`tests/test_bc16_replay.nim`) that
re-derives the committed fixture to its LAST round, because
`tools/wasm_replay_smoke.cjs` steps 200 frames and its `mismatch_round: -1`
therefore covers only the first 200 rounds of a 2871-round recording.
* `tools/gen_bc16_fixture_replay.nim` — the third map is `closequarters`
instead of `river`. This is the tripwire the tool's own header describes
("a rule change therefore turns those tests red: re-record with this
program"): on the shipped chassis `river` no longer reaches the `rout`
threshold (five robots lost by one side in one round,
`years/bc16/rules.nim:324`), so the re-recorded fixture carried TWELVE beat
kinds and `tests/test_bc16_beats.nim` names thirteen. Measured over all
twenty-two bc16 maps in the third slot, `closequarters` is the one
substitution that restores the thirteenth kind while keeping `frogger`,
`checkers` and the three-game shape. The header's per-map attribution is
rewritten to say which map now supplies what.
* `ci.yml` — the comment beside the wasm smoke corrected: 2871 rounds, and
the 200-frame window named explicitly so nobody reads its
`mismatch_round: -1` as whole-recording evidence again.
PRE-EXISTING DRIFT FOUND WHILE DOING THIS, and fixed by the same
re-recording: the fixture committed on `main` @ fbc7d34 did NOT re-derive at
that sha — `mismatch_round=302`, verified in a clean worktree at HEAD with
this commit's changes backed out. The 200-frame wasm window was why no gate
saw it, and the new whole-recording assertion is why it cannot come back.
Verified locally: `test_bc16_beats: ok (91 checks)` with all thirteen kinds,
`test_bc16_replay: ok (79 checks)`, `test_viewer: ok (855)`,
`test_manifest: ok (1332)`, `test_determinism: ok (110)`, and the seven
sibling years' committed fixtures still re-derive with `mismatch_round=-1`.
`more_archons` Review finding F8: `src/battlecode/years/bc16/rules.nim:431-434` mapped `dfNone` — the "no winner at all" state — to `$dfPwned`, i.e. it wrote `more_archons` into the shipped `end_reason`. The state is unreachable in the shipped configuration (`checkEndOfMatch` always sets a winner via one of the four rungs, the abandoned path returns before this proc runs, and the only way in is `maxRounds <= 0`, which `config_schema.maxRounds.minimum = 50` forbids) — but an impossible state that reports a plausible answer is the wrong failure mode. A future rule change that makes it reachable has to surface as a failure, not as a wrong `end_reason` in a replay a spectator reads. It now raises a `Defect` naming the state, the round, `maxRounds` and `hasWinner`. `raise`, not `doAssert`, so it holds under `-d:danger` as well as `-d:release`. `endReasonFor` is exported so the behaviour is testable, and `tests/test_bc16_endladder.nim` gains the test: a fresh world's domination factor IS `dfNone`, `endReasonFor` refuses to name a reason for it, and a decided game still round-trips its own rung through the same proc (`more_parts_net_worth`, and never `more_archons` by accident). No other year exports a symbol of that name. Verified: `test_bc16_endladder: ok (52 checks)` in debug, `-d:release` and `-d:danger`; `test_bc16_replay`, `test_bc16_baselines`, `test_bc16_scoring` and `test_bc16_maps` all still green.
…ss the page actually sets Review finding F9, both halves. (a) THE LABEL. Coordinator ruling 4 made "THE HORDE" this year's flavour carrier and design.md:319-320 says the zombie team "is drawn and labelled THE HORDE" spectator-side. The words appeared only in a CSS comment (`client/replay_broadcast.html:3702`) and in the agent-facing observation (`decide.nim:639`, `:1163`); `renderHorde` drew glyph counts, `WAVE`, `OUTBREAK`, `DENS`, `ROUND` and no faction name. `renderHorde` now draws `<span class="horde">THE HORDE</span>` as the FIRST field of the strip, in every state, with a rule of its own, and it survives the 360 px media query (only `.tl`, the mini-timeline, is still dropped there). (b) THE STRIP TAKEOVER. `#bc16-horde.struck` (`:3724`) was a rule nothing ever activated — `grep -n struck` returned it and bc22's and nothing else. design.md:1772-1776 asks for a wave round and a `turned` event to take the strip over for two seconds with what happened in plain words. `hordeStrike()` now does exactly that, mirroring `renderArchons`'s add/remove shape for the archon pill's `flash` (`:5774-5776`): it reads the beats that landed between the frame the playhead came FROM and the frame it is ON, takes the LAST `wave` or `turned` beat in the step, puts THE BEAT'S OWN LABEL on the strip (so the strip, the killfeed and the scrubber button say the same sentence and there is no second string to keep in step — `WAVE — 34 zombies from 4 dens at outbreak level 5`, `CLAN ASH'S ARCHON TURNS — a BIGZOMBIE at 34,19`), adds `struck`, and removes it after 2000 ms re-rendering the readouts. `.struck` also sets `white-space: normal`, because the readouts' `nowrap` is there to stop a field breaking mid-number and applied to a SENTENCE inside an `overflow: hidden` box it clipped the sentence instead — measured at 360 px. TESTS, so neither becomes dead again (`tests/test_viewer.nim`), in the same triad shape the endcard's `.on` uses: the `.struck` rule exists, the page adds that exact class, the page takes it off again, the label span is drawn with a rule of its own and is not hidden by the 360 px query, and the takeover is keyed on `wave`/`turned` for 2000 ms in the beat's own words. AND THE LOOP IS CLOSED: the test reads `tests/fixtures/replay-bc16.json` through `beatsFor` and asserts the committed artefact really emits a LABELLED `wave` beat and a labelled `turned` beat — so the kinds the takeover keys on are kinds the fixture provably carries, not a hopeful pair of strings. `tools/ci/renderer_fixture.html`'s bc16 row carries the label too, so its layout is measured at 360/720/1280 px. Evidence, driven in headless chromium 141 against the page's own <style> block and its own bc16 game block: label drawn first at rest and while struck; `struck` present after stepping across the `wave` beat and after the `turned` beat; the border colour computes to the rule's `rgb(106, 90, 58)`; the class absent and `ROUND 2999` back after 2.3 s; the strip inside the 360 px frame and clipping nothing while struck. `test_viewer: ok (865 checks)`, `test_bc16_beats: ok (91 checks)`, and all 24 renderer-fixture cells `ok`.
…ason shape at the enum level Review finding F11 and the other half of coordinator ruling 7. The tolerance was already there at the STRING level — `stopped in ["deadline", "complete"]` read off the written document (`tests/test_bc23_replay.nim:157-160`, from the pre-run commit 16c6e49 that split this block in two) — which is why the flake that reddened push run 34285737260 while PR run 34285740451 passed on the same tree is already closed. What was missing is the shape ruling 7 named: the enum-level assertion on the value `playMatch` actually returned. That value was being thrown away by `discard reason`, because `recordAndDerive` returned only the GAME's end reason, not the EPISODE's. `recordAndDerive` now returns the `EpisodeReason` too, and the timed block asserts `episodeReason in [epDeadline, epComplete]` — bc22's exact shape (`tests/test_bc22_replay.nim:270`) — alongside the existing document-level check, plus a new `checkEq` that the two agree. Nothing is weakened: three assertions where there were two, and the loop call site takes `_` for the new field. `nim r tests/test_bc23_replay.nim` → `ok (98 checks)` in debug and in `-d:release` (was 95).
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 the
bc16year module. One commit per finding.#endcard.on; a computed-style check in the renderer fixture replaces the text greptools/ci/renderer_fixture.htmlgains its bc16 row; two real clipping defects it found are fixeddocs/PARITY.mdmixHash; the fixture is re-recorded;match.nim's year-neutral packing is recorded, not changedendReasonForraises ondfNoneinstead of writingmore_archons#bc16-horde.struckis activated onwave/turnedruns/2026-09-09-battlecode-2016/reviews/r1-fixes.mdNo test was disabled, skipped, weakened or deleted. Assertions added: 9 in
test_viewer, 5 intest_bc16_replay, 4 intest_bc16_endladder, 3 intest_bc23_replay, plus the renderer fixture's bc16 row and its endcardcomputed-style check.