benjivers/val Public Public: anyone can read this; the crew can change it.

Verify pull #4 before it merges — report, do not fix #5

Closed opened this has it
opened this

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

  1. Vision is still honest. The server must never send a player you cannot see. Connect, sit still in spawn, and count enemies in the snapshot over a full round — it should be zero while nothing has a line on you, and non-zero only when one genuinely does. This was a live wallhack two pulls ago; confirm the renderer did not reintroduce it by drawing from anything other than the snapshot.
  2. The feed leaks nothing. events must carry names and weapons, never coordinates, and must not reveal an enemy's position by implication. Read arena.ts and check the wire.
  3. The server still owns everything. Over the socket, try: buying a gun you cannot afford, buying during a live round, casting an ability with no charges, planting while not carrying the spike, and sending a position or an hp field. Report exactly what the server accepted.
  4. A person arriving at any moment can play within seconds — mid-round, mid-buy, and just after a match ends.
  5. The tests are real. 143 pass; pick three that cover the load-bearing rules (vision, the defuse checkpoint, the economy) and confirm each fails when you break the line it covers. Put them back.

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.

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 in match.ts — it's two different players sharing a name, and the new killfeed.ts matches kills by name text, so it surfaces the collision directly in the feature this pull ships. Fix belongs in match.ts's name draw, not in killfeed.ts.
  • Non-blocking notes: the PR description claims arena.ts is 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.

↑↓ navigate ↵ open esc close

Keyboard shortcuts