Verify pull #4 before it merges — report, do not fix #5
commented
I reviewed pull #4 by reading the diff against main, checking out wren's branch in a worktree, running the test suite, mutating three load-bearing lines (vision, defuse checkpoint, economy cap) to confirm the tests actually catch regressions, and exercising the live preview's /ws socket directly (join, buy phase, live round, position/hp injection attempts).
Verdict: request changes, posted as a review on pull #4. Findings:
- The vision, wire, and server-authority guarantees all hold —
arena.ts's snapshot filtering is unchanged, the renderer draws only from the snapshot, and the socket rejected every illegal command I tried (unaffordable buy, mid-round buy, forged position/hp). - The reported "Sage killed Sage" / duplicate "Kit" bug is real and diagnosed:
src/match.ts:74's bot-name formula lets its random term override the spacing meant to keep names distinct, so ~89% of matches (measured over 2000 simulated matches) get at least one duplicate bot name. It's neither a team kill nor a genuine self-kill — both are structurally impossible elsewhere inmatch.ts— it's two different players sharing a name, and the newkillfeed.tsmatches kills by name text, so it surfaces the collision directly in the feature this pull ships. Fix belongs inmatch.ts's name draw, not inkillfeed.ts. - Non-blocking notes: the PR description claims
arena.tsis untouched (it gained the killfeed hook) and cites a stale test count (117 vs the actual 143); worth a pass before merge but doesn't affect behavior.
Full evidence, file/line references, and the mutation-testing results are in the review on pull #4. No code was pushed to wren's branch, per the reviewer role — the fix is described precisely enough for wren to land it in one file.
Sign in to join this conversation.
Review pull #4 on this repository against what it claims and against the two issues it touches (#1 the game, #3 the first-person view and instructions). Wren wrote it; you are the second pair of eyes. Do not push to that branch — describe what is wrong precisely enough that wren fixes it in one attempt.
The preview is https://val--4--forge-apps.benjivers.com and the websocket is
/ws.Blocking if false
eventsmust carry names and weapons, never coordinates, and must not reveal an enemy's position by implication. Readarena.tsand check the wire.Known defect to diagnose
The kill feed printed "Sage killed Sage", and I have seen two bots on one team with the same name ("Kit" twice). Work out which it is: duplicate names in the bot roster, a team kill being reported without saying so, or a genuine self-kill. Say which, and where the fix belongs.
Worth reporting, not blocking
The legibility work from the last round (team roster, kill feed, chevrons on enemies, corpses lying flat): does it actually answer who is on my team and who is dead at a glance, in the first-person view and on the minimap? Also the gaps wren has already named — no client-side prediction, full-circle vision rather than a view cone, no overtime at 12–12, bots committing to one site — judge whether any of them should block a merge or wait for a later pull.
How to report
One review on pull #4. Verdict first — approve, or request changes — then a numbered list, each item with its evidence: file and line, what you did, what happened, what should have happened. Keep 'this is broken' separate from 'I would have done it differently'; only the first blocks. If it holds up, say so and approve it.