[iiamlewis]

Resolves #5768

Description:

Reworked after #5852. #5852 (now on main) keys the winner vote on the winner and the per-player stats, so a decided vote already means a majority agreed on those exact stats, and any disagreement goes to the server replay. This PR used to add its own second stats vote and a short window for late votes; that is all dropped in favour of #5852. What's left is what main is missing: the archived record says whether its stats are trustworthy, and a game decided by a single voter is checked by the replay instead of being trusted.

statsAgreed on the archived record

GameEndInfo gains an optional statsAgreed boolean (packages/shared/src/WireSchemas.ts, passed through createPartialGameRecord). The API only awards XP for a game whose record has statsAgreed: true (when progression is fully on), so this flag is what lets multiplayer games earn XP.

• True when:
  • the vote decided with one candidate and at least 2 distinct IPs sent that exact (winner, stats) key: archived directly, no replay;
  • the server replay supplied the winner and stats (disputed vote, no majority, or a lone voter);
  • the replay failed but the vote had decided on a key backed by at least 2 IPs (that vote's result is archived).
• False when nobody voted, when the replay failed with no decided vote, when the replay failed and the decided key had only 1 backer, or when a lone voter's game skipped the replay because the queue was full (below).
• Absent on singleplayer and LocalServer records (the API strips it for singleplayer anyway).

A lone deciding voter is replayed

A strict majority of one voting IP still decides, for example a 1v1 where the loser leaves without voting, or two clients on one IP. #5852 notes those stats are unchecked. Now settleWinner archives a vote directly only when its key has at least 2 backers (WinnerVote.backers(): unique IPs that sent the decided key, including IPs that have since left). With 1 backer it runs the existing replay; if the replay fails, the lone vote's result is archived with statsAgreed: false.

Keeping the replay queue bounded

One-voter games are replayed so their stats can be confirmed. They are common, though, and each worker runs one replay at a time. To keep the queue bounded, when a worker already has 3 replays waiting or running (LONE_VOTER_REPLAY_CAP), a one-voter game is recorded straight away with the vote's result as unconfirmed (statsAgreed: false, so no XP) and counted in a new skipped replay outcome. Disputed and no-majority games are unaffected: they always queue a replay.

Logging

• New start line "winner vote decided by one IP, replaying game", alongside "winner vote disputed, replaying game"; both carry backers.
• "winner replay result" gains reason: "disputed" | "lone voter".
• A failed replay logs "winner replay failed, archiving the vote" with reason, backers and statsAgreed.
• "archiving game" gains statsAgreed.
• A one-voter game that skips the replay logs "winner replay queue full, archiving the lone vote unconfirmed" (warn) with pending and cap.
• Lone-voter replays count in the same replay outcomes; openfront.winner_replay.outcomes gains skipped.

Rollout

The API only scores games with statsAgreed: true once PROGRESSION_ENABLED="1". This must be deployed to the game servers before production switches to "1"; until then production stays on "unverified".

Screenshots

No UI change.

Testing

• tests/server/WinnerReplaySettle.test.ts: every existing case now also checks statsAgreed. New cases: a lone voter whose replay succeeds (true), a lone voter whose replay fails (falls back, false), two clients on one IP (replayed as a lone voter), a departed IP that sent the decided key still counting as a backer (true, no replay), a no-majority game whose replay fails (winnerless, false). Queue cap: a lone voter at the cap is archived at once (false, no replay, skipped counted), one below the cap is still replayed, and a disputed vote at the cap is still replayed.
• Consensus.test.ts and VoteTally.test.ts: backers().
• tests/StatsAgreedRecord.test.ts: the flag on the record and its schema.
• npm run typecheck, eslint and prettier clean; tests/server, tests/core, the schema and wire tests pass (1660 passed).

Please complete the following:

• I have added screenshots for all UI updates
• I process any text displayed to the user through translateText() and I've added it to the en.json file
• I have added relevant tests to the test directory

Please put your Discord username so you can be contacted if a bug or regression is found:

Iamlewis

🤖 Generated with Claude Code

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 108ab222-8523-474b-bd74-6f60a278f886

---

📥 Commits

Reviewing files that changed from the base of the PR and between 926e2c3 and d1d7861.

---

📒 Files selected for processing (2)

• src/server/GameServer.ts
• tests/server/WinnerStatsAgreement.test.ts

---

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

---

---

Walkthrough

The server tallies stats separately from winner votes. After a winner decision, stats votes can remain open for up to five seconds. The server archives the selected stats and records whether they reached a strict majority.

Changes

Winner Stats Agreement

Layer / File(s)	Summary
Shared vote tally support src/server/VoteTally.ts, tests/server/VoteTally.test.ts	VoteRound uses a shared strict-majority check and exposes candidate standings, backers, and voters. Tests cover majority boundaries and active-IP filtering.
Per-winner stats consensus src/server/Consensus.ts, tests/server/Consensus.test.ts	WinnerVote tracks stats versions per winner and settles them separately from the winner decision. Tests cover majority agreement, fallback selection, active-IP settlement, and late votes.
Server settlement and record archival src/server/GameServer.ts, tests/server/WinnerStatsAgreement.test.ts, tests/server/WinnerReplaySettle.test.ts	GameServer allows a five-second stats-vote window and archives once after settlement or a lifecycle event. Tests cover late votes, departures, game end, and replay outcomes.
Archived agreement field packages/shared/src/SharedUtil.ts, packages/shared/src/WireSchemas.ts, tests/StatsAgreedRecord.test.ts, tests/client/LocalServer.test.ts	Game records accept an optional statsAgreed boolean. Tests cover parsing, serialization, omission, and single-player records.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Feature

Suggested reviewers: evanpelle, celant

Merge Risk: ⚪ Minimal · up to d1d78

No actionable merge-blocking issue was established; the change is ready for normal merge checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 48d28

Majority-backed stats replace unchecked first-voter stats, with bounded settlement and explicit disagreement handling. No introduced security bypass was established. Downstream enforcement of the agreement marker, particularly for client-uploaded records, remains unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — A malicious eligible voter can submit altered stats for other players in the same game's roster. The PR changes which snapshot wins persistence and permits bounded late influence, but does not establish cross-game identity selection or additional service privileges. Downstream account-scoring consequences remain unverified.

Trust Boundaries and Controls

• observed — Late stats submissions inherit schema validation, rate limiting, spectator exclusion, connected-roster membership, desync and kick checks, and one submission per client. WinnerVote receives the connection's IP, accepts only the decided winner key, and rejects stats submissions after settlement.
• observed — The shared record schema accepts statsAgreed in the same record shape used for singleplayer uploads. The normal singleplayer writer omits it and uploads with session authorization, while multiplayer archival uses a server API key. These local paths do not prove that the remote API strips client-supplied agreement markers or limits scoring to server-originated records.

Resilience and Maintainability Implications

• observed — Shared-IP backing of multiple stats candidates cannot produce agreement when more than one candidate reaches a majority. Forced fallback explicitly remains unagreed, preserving the distinction between a retained snapshot and an agreed snapshot.

Hardening Proposals

• proposed — At the archive API boundary, derive trusted agreement status from authenticated server-originated uploads, strip or ignore it on client uploads, and require explicit true plus trusted provenance before scoring. This is a downstream validation proposal, not an observed bypass.

---

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check		#5768 requirements are covered for voter-submitted stats: Consensus.ts counts winner-voter stats by unique IP, uses strict majority, supports late votes, and records the selected version with agreem…	In the replay-success path, set agreed from the winner-voter stats majority result, or set it to false when replay does not establish that result. Add or update a replay-success test that verifies the required statsAgreed value.
Docstring Coverage		Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 13 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Out of Scope Changes check		The changed consensus logic, shared majority helper, archive wait, replay settlement, schema field, logging, and tests directly support #5768. The available PR evidence shows no unrelated change.
Title check		The title clearly identifies the two main changes: recording whether archived stats were agreed and replaying lone-voter wins.
Description check		The description directly explains the statsAgreed record field, lone-voter replay behavior, replay queue limits, logging, rollout requirements, and related tests.

---

Full details: Linked Issues check

Explanation

#5768 requirements are covered for voter-submitted stats: Consensus.ts counts winner-voter stats by unique IP, uses strict majority, supports late votes, and records the selected version with agreement status. GameServer.ts preserves winner selection and guards archive races. Tests cover these paths. One requirement remains unmet: the replay-success path sets { stats: replayed.allPlayersStats, agreed: true }. Replay output is not a majority result from the winner's voters. Therefore statsAgreed: true is not limited to exactly one voter-submitted stats version with a strict majority.

---

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

---

_{Votes gather; stats take shape
A late reply can shift the count
The timer closes out the round
Agreed stats enter the record
The winner stays the winner
Clear flags mark what voters chose}

_{Comment @coderabbitai help to get the list of available commands.}

[Celant]

Coordinated review

Part of a batch for @Celant, reviewed alongside the backend's XP ingest. CI review was stale by 5 days and has been re-run.

The client cannot assert statsAgreed itself, which I checked specifically because the backend treats that flag as the thing that decides whether a game earns XP at all. It's written only by the game server in archiveGame (GameServer.ts:2064), the archive POST is API-key authenticated, and the client's singleplayer path never passes the argument — consistent with the backend deliberately stripping it on /archive_singleplayer_game.

Vote edge cases all hold up. Tie: no version reaches a strict majority, the window opens, and the forced settle takes the most-backed version (earliest on a tie) with agreed: false. Zero voters: archiveGame(null, null) → statsAgreed: false, per-player stats omitted, record still archived from end(). Winner not voting is irrelevant by design. Disconnects are handled — checkWinnerAfterElectorateShrink re-evaluates open stats, settleStatsAmong counts only still-connected players' IPs, and everyVoterHasVoted() force-settles when the last unvoted player leaves. Double-archive is guarded, the timer is cleared in settleWinner, and end() force-settles before GameManager drops the game.

Test coverage across Consensus.test.ts, WinnerStatsAgreement.test.ts, WinnerReplaySettle.test.ts and StatsAgreedRecord.test.ts is unusually thorough, including the shared-IP double-majority branch, key-order canonicalisation and the archive JSON round trip.

Three low findings below. I have one further observation about what statsAgreed certifies that I'm raising with @Celant directly first rather than here.

One note worth recording: nothing in this repo reads the backend's new requireStatsAgreed, so a game whose voters disagree currently scores nothing with no player-facing explanation. Probably a follow-up rather than a change here.

3 finding(s): 3 low

---

Findings whose anchor falls outside the diff:

[low] handleWinner isn't gated on the game having started (pre-existing, but this PR rebuilds the path)

src/server/GameServer.ts

Unlike handleReport, handleWinner doesn't check this.stage === "started". A winner frame accepted during the lobby can decide the vote and run straight through settleStatsOrArchive → settleWinner → archiveGame, which dereferences this.gameStartInfo (:248, non-null asserted) before start() has assigned it. SocketIngress only applies decode, rate-limit and spectator checks; it doesn't wrap onMessage.

Failure scenario: an early vote from a lobby throws inside the socket message path, and because archived has already been set the game then never archives its real record either.

Behaviour is identical on origin/main (the decided flag suppressed the later archive there too), so this is not a regression — but the PR rebuilds this path and the guard is one line.

Suggested fix: add this.stage !== "started" to the handleWinner early return, matching handleReport.

[iiamlewis]

Agreed on scope, and reworked on top of #5852 (d57d2e2, 6ee04a2):

• Dropped: this PR's second stats vote, the 5s window and the electorate changes. main's stats-keyed vote and replay are taken as they are.
• Kept: statsAgreed on the archived record. It is true when the vote decided with at least 2 distinct IPs behind the exact (winner, stats) key, or when the replay supplied the result.
• Lone deciding voter (the follow-up Key the winner vote on per-player stats, not just the winner #5852 names): a vote whose decided key has a single backer now goes to the existing replay instead of being archived directly. If the replay fails, it falls back to that vote with statsAgreed: false.
• Rollout is unchanged: this has to reach the game servers before production flips to PROGRESSION_ENABLED="1".

The description is rewritten to match. The diff against main is now 13 files.

🤖 Addressed by Claude Code

[Celant]

Re-reviewed after the rework. The reduction is exactly right, and it closes the thing I was most worried about.

The shrink from +1139/-101 to +343/-21 is real, not cosmetic: StatsAgreement, ArchivedStats, SettledStats, STATS_VOTE_WINDOW_MS, settleStatsAmong and settleStatsOrArchive are all at zero occurrences in Consensus.ts and GameServer.ts. Adopting #5852's stats-keyed vote and reducing this to recording the flag was the right call.

The flag itself is correct. I traced all three archiveGame call sites and all three settleWinner callers:

• candidates === 0 → archiveGame(null, false)
• candidates === 1 && voted !== null && backers >= 2 → archiveGame(voted, true)
• replay succeeded → archiveGame(replayed, true)
• replay failed → archiveGame(voted, votedAgreed) — false for a lone voter or no majority

Singleplayer cannot set it: createPartialGameRecord takes it as the 13th positional param and LocalServer.archiveGameRecord passes 7 args, so it stays undefined. That matches the API side, which deliberately does delete record.info.statsAgreed on archive_singleplayer_game and gates on record.info.statsAgreed !== true in ProgressionIngest.

And it closes the degenerate-electorate case. A 1-of-1 decided vote now takes the replay branch, so such a record only gets true if the server's own simulation produced it; a failed replay archives the client's self-reported stats with false. The residual case is two distinct IPs submitting byte-identical (winner, allPlayersStats), which is main's IP-weighted design and unchanged here. backers() counting departed IPs is right for the claim being made — that IP did independently send the same digest.

Test coverage on the new branches is thorough: lone-voter overturn, lone-voter replay failure, NAT-shared IP, departed backer counted, no-majority replay failure, singleplayer absent, JSON round trip, non-boolean rejected.

---

[high] The lone-voter branch is a behaviour and capacity change, not a recording change

Where: src/server/GameServer.ts:1949-1952.

Requiring backers >= 2 for the fast path means a 1-of-1 decided vote is now replayed, and archiveGame(replayed, true) makes the replay authoritative over the vote. That changes who wins, for a case this codebase documents as routine — checkWinnerAfterElectorateShrink's own comment says "In a 1v1 the loser often disconnects within a second of being eliminated — before their own client simulates the win tick and votes — leaving the winner's vote wedged at 1 of 2". That path calls tallyAmong(activeIPs) with activeIPs.size === 1, so it now always triggers a replay.

It also catches any multiplayer game where one client is the last connected at the win tick (common at the end of a long FFA), single-human public/private lobbies, and — per this PR's own test "counts IPs, not clients: two clients on one IP are a lone voter" — a genuine 1v1 between two players behind one NAT.

A replay is a whole game's worth of simulation in a forked subprocess at PRIORITY_LOW with a 30-minute timeout, serialised one at a time per worker, with an unbounded queue and only a pending gauge for backpressure.

Failure scenario: replays arrive faster than they drain. pending grows without bound, records are archived minutes-to-hours after the game ends (each pending game pinning a GameServer closure and a turn-log snapshot), and anything still queued at a worker restart or deploy is lost outright. Where a replay times out or the subprocess is OOM-killed, the winner of an ordinary 1v1 gets statsAgreed: false and therefore zero XP for a game nobody disputed.

Suggested fix: decide this explicitly and say so in the PR description, because it is well outside "record a flag". Either archive the lone voter's result immediately with statsAgreed: false and queue the replay only as an upgrade path, or gate the lone-voter replay on something cheap (turn count, map size, whether the record is XP-bearing at all) and add concurrency plus a queue-depth cap with a metric — so the common ending doesn't sit behind a serialised 30-minute worst case.

[medium] handleWinner still has no stage guard

Where: src/server/GameServer.ts:2166.

The early return covers desync.isDesynced, isKicked, winnerVote.winner() !== null and client.reportedVote !== null — but not this.stage !== "started", which handleReport does check at :2106. gameStartInfo/wireGameStartInfo are !-declared and only assigned in start(), so both are undefined before then, and handleClientMessage's "winner" case dispatches for any connected non-spectator.

I raised this in the previous round as a low, on the grounds that it was pre-existing and bounded. The rework changes that — the new lone-voter path routes through replayWinner, which makes the consequences materially worse than before. I've sent @Celant the detail separately rather than spelling it out here.

Suggested fix: add this.stage !== "started" || this.ended to the early return, matching handleReport. Separately, replayWinnerInChild chaining on a queue that can reject permanently is worth hardening independently of this PR, and settleWinner's .catch would be better falling back to archiveGame(voted, votedAgreed, turns, endTime) than only logging.

[low] reason: "disputed" and the "already archived" log line are now inaccurate

Where: src/server/GameServer.ts:1954-1955, and end() at :1535.

reason is "lone voter" only when candidates === 1 && voted !== null, so the candidates === 1 && voted === null case — one candidate, no majority, not a dispute — is logged and tagged "disputed". Separately, end() logs "game already archived" whenever winnerVote.winner() !== null, which under the new flow is frequently false; this PR's own test asserts await game.end(); expect(archive).not.toHaveBeenCalled() for a pending lone-voter replay.

These strings are what an operator reads when auditing why a game earned no XP, so they are worth getting right.

Suggested fix: add a third reason ("no majority"), and reword end()'s line to something like "winner already settled, record pending".

[low] A brittle field-order assertion in the new test

Where: tests/StatsAgreedRecord.test.ts:90-92.

expect(keys[keys.length - 1]).toBe("statsAgreed") asserts key ordering on GameEndInfoSchema.shape, but the file's own header says records travel only as JSON over HTTP, where order is irrelevant — GameEndInfoSchema appears only in the HTTP record schemas, not the zbin path. It will fail for the next field anyone appends.

The companion assertion in the same test — that "statsAgreed" in GameStartInfoSchema.shape is false — does protect the zbin-encoded start info and is worth keeping.

Suggested fix: drop the ordering assertion, keep the GameStartInfoSchema one.

---

Also checked, no findings: VoteTally.backers() and WinnerVote.backers() are pure reads with no effect on result/resultAmong, so the winner decision in those two files is unchanged; statsAgreed is appended last positionally in createPartialGameRecord, so the other 12 callers are unaffected; the replay correctly uses wireGameStartInfo rather than gameStartInfo, since that is what clients simulated. Comment style complies — the long GameEndInfoSchema block earns its length by encoding a cross-repo wire contract including the rolling-deploy case.

[iiamlewis]

Thanks. All four addressed in 8210cbc:

• Votes before start: handleWinner now returns unless this.stage === "started" and the game hasn't ended, matching handleReport. The test calls the handler directly from the lobby (a lobby frame doesn't decode before start in the harness) and checks the vote isn't recorded and a later unanimous vote archives without a replay. It fails without the guard.
• Replay that throws: the .catch now falls back to archiveGame(voted, votedAgreed, turns, endTime) and counts it as failed, unless a record was already handed over. Tested with a rejecting replay.
• Log accuracy: reason gains "no majority" (one candidate, no decision), with its own start line, and end() now logs "winner already settled, record sent or pending replay".
• Brittle test: the key-order assertion is gone; the GameStartInfoSchema check stays.

replayWinnerInChild's queue hardening is left for a separate change.

🤖 Addressed by Claude Code

[iiamlewis]

On the [high] (lone-voter replays and queue capacity), decided explicitly and in the description now (b5aa21b):

• One-IP votes are still replayed, since that's the only way such a record can be confirmed.
• The replay is capped: when this worker already has LONE_VOTER_REPLAY_CAP (3) replays waiting or running, a lone-voter game is archived immediately with its vote and statsAgreed: false instead of queuing. It's logged ("winner replay queue full, archiving the lone vote unconfirmed") and counted as skipped in openfront.winner_replay.outcomes, so the rate is visible.
• "disputed" and "no majority" queue exactly as on main.
• Tests: lone voter at the cap archives without a replay; one below the cap still replays; a disputed vote at the cap still replays.

The cap keeps the common ending out of a long serial queue at the cost of no XP for that game when a worker is busy. If the skipped rate turns out high, the next step is replay concurrency, which I'd rather do separately with the queue hardening.

🤖 Addressed by Claude Code

[github-actions]

🤖 Claude Code Review

Verdict: No issues found. Checked for bugs and CLAUDE.md compliance. (0 high / 0 medium / 0 low)

Reviewed settleWinner (the lone-voter replay, the LONE_VOTER_REPLAY_CAP skip path, the archived guard in the .catch fallback), WinnerVote.backers() / VoteRound.backers(), the statsAgreed plumbing through createPartialGameRecord and GameEndInfoSchema, the new stage/ended guard in handleWinner, and GameServerDeps.replayPending. GameServer merges the partial deps with the defaults, so existing callers still compile.

🤖 Generated with Claude Code

[Celant]

Approving: CI Claude review clean at 63348821, all required checks green, findings from every earlier round closed and verified in code.