Implement vote for winner #2013
Merged
Conversation
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
WalkthroughIntroduces vote-based winner resolution in GameServer, tracking votes by unique IPs and setting the winner when votes reach at least half of active IPs. Adds Client.reportedWinner to persist a client’s reported winner (also preserved on reconnect). Extends GamePhase enum with FINISHED. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant GS as GameServer
participant W as winnerVotes (map)
participant A as Archive
rect rgba(230,240,255,0.4)
Note over C,GS: Client reports a winner
C->>GS: WS message { type: "winner", winner }
GS->>GS: handleWinner(client, msg)
end
alt Client invalid or game already has winner
GS-->>C: Ignore
else First-time report from this client
GS->>GS: client.reportedWinner = msg.winner
GS->>W: votes[winnerKey].add(client.ip)
GS->>GS: activeIPs = set(connected client IPs)
GS->>GS: if votes[winnerKey] < ceil(activeIPs/2) then wait
opt Threshold reached (>= 1/2 active IPs)
GS->>GS: this.winner = msg.winner
GS->>A: archive game
GS-->>C: Finalized
end
end
sequenceDiagram
autonumber
participant C1 as Old Client Instance
participant C2 as Reconnected Client
participant GS as GameServer
C1--xGS: Disconnect
C2->>GS: Reconnect (same identity)
GS->>C2: Copy C1.reportedWinner to C2.reportedWinner
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/GameServer.ts (1)
311-319: Avoid WebSocket set leakwebsockets.add(...) never removes on close. Delete closed sockets to prevent growth over long-lived processes.
client.ws.on("close", () => { this.log.info("client disconnected", { clientID: client.clientID, persistentID: client.persistentID, }); + this.websockets.delete(client.ws); this.activeClients = this.activeClients.filter( (c) => c.clientID !== client.clientID, ); });
🧹 Nitpick comments (4)
src/server/Master.ts (1)
153-155: Return valid JSON with correct content-type when list is emptySending an empty string produces text/html and breaks JSON clients. Serve JSON consistently.
-app.get("/api/public_lobbies", async (req, res) => { - res.send(publicLobbiesJsonStr); -}); +app.get("/api/public_lobbies", async (req, res) => { + res + .type("application/json") + .send(publicLobbiesJsonStr || '{"lobbies":[]}'); +});src/server/GameServer.ts (1)
206-310: Tighten error logging and fix typoSmall hygiene: use warn/error for exceptions and fix the log message typo.
- } catch (error) { - this.log.info( - `error handline websocket request in game server: ${error}`, - { - clientID: client.clientID, - }, - ); + } catch (error) { + this.log.warn( + `error handling websocket request in game server: ${String(error)}`, + { clientID: client.clientID }, + ); }src/server/Worker.ts (2)
162-195: Optional: restrict private updates to localhost/adminThere’s a TODO to restrict who can update private games. If this is meant only for internal tools, check req.ip is loopback or require admin token.
284-450: WebSocket join flow: use policy-violation code, close on not-found, and treat XFF carefully
- Use 1008 for Unauthorized/Forbidden (policy violation).
- Close the socket when the game isn’t found to avoid idle connections.
- Treat x-forwarded-for cautiously; if you can’t authenticate the proxy, prefer req.socket.remoteAddress or add an allowlist for trusted proxies.
- const forwarded = req.headers["x-forwarded-for"]; - const ip = Array.isArray(forwarded) - ? forwarded[0] - : // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing - forwarded || req.socket.remoteAddress || "unknown"; + const forwarded = req.headers["x-forwarded-for"]; + // Prefer socket address; only trust XFF if your infra guarantees it. + const ip = + (Array.isArray(forwarded) ? forwarded[0] : undefined) || + req.socket.remoteAddress || + "unknown"; @@ - ws.close(1002, "Unauthorized"); + ws.close(1008, "Unauthorized"); @@ - ws.close(1002, "Unauthorized"); + ws.close(1008, "Unauthorized"); @@ - ws.close(1002, "Forbidden"); + ws.close(1008, "Forbidden"); @@ const wasFound = gm.addClient( client, clientMsg.gameID, clientMsg.lastTurn, ); if (!wasFound) { log.info(`game ${clientMsg.gameID} not found on worker ${workerId}`); - // Handle game not found case + ws.close(1008, "Game not found"); }If you must keep XFF, consider checking req.socket.remoteAddress against a known LB CIDR list before trusting it, or have the LB set a separate, signed header.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/server/Client.ts(1 hunks)src/server/GameServer.ts(4 hunks)src/server/Gatekeeper.ts(0 hunks)src/server/Master.ts(1 hunks)src/server/Worker.ts(1 hunks)src/server/gatekeeper(0 hunks)
💤 Files with no reviewable changes (2)
- src/server/gatekeeper
- src/server/Gatekeeper.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/server/Client.ts (2)
src/core/game/Game.ts (1)
Tick(15-15)src/core/Schemas.ts (1)
Winner(387-387)
src/server/Master.ts (1)
src/core/Schemas.ts (1)
ID(184-187)
src/server/GameServer.ts (2)
src/core/Schemas.ts (3)
ClientSendWinnerMessage(100-100)ClientMessageSchema(481-488)ServerErrorMessage(99-99)src/server/Client.ts (1)
Client(6-24)
src/server/Worker.ts (8)
src/core/Schemas.ts (5)
ID(184-187)GameRecordSchema(521-523)GameRecord(524-524)ClientMessageSchema(481-488)ServerErrorMessage(99-99)src/server/GameManager.ts (1)
game(18-20)src/server/Archive.ts (2)
readGameRecord(124-157)archive(25-55)src/server/jwt.ts (2)
verifyClientToken(19-47)getUserMe(49-75)src/core/configuration/PreprodConfig.ts (1)
allowedFlares(14-22)src/core/configuration/DefaultConfig.ts (1)
allowedFlares(79-81)src/core/Util.ts (1)
assertNever(218-220)src/server/Client.ts (1)
Client(6-24)
🪛 GitHub Check: CodeQL
src/server/Master.ts
[failure] 171-179: Server-side request forgery
The URL of this request depends on a user-provided value.
The URL of this request depends on a user-provided value.
🔇 Additional comments (9)
src/server/Master.ts (1)
144-150: LGTM: simple env probe is fineReturns a stable JSON shape and 500 when unset. No concerns.
src/server/Client.ts (2)
4-4: LGTM: import of Winner typeConsistent with Schemas usage elsewhere.
11-11: LGTM: reportedWinner state on clientThis enables safe single-vote semantics across reconnects and is copied on rejoin.
src/server/GameServer.ts (2)
67-71: LGTM: minimal structure for vote trackingMap keyed by a winner key with per-IP sets makes sense. See below for keying/majority tweaks.
191-193: LGTM: carry winner state across reconnectThis prevents duplicate votes after reconnects.
src/server/Worker.ts (4)
197-202: LGTM: lightweight existence probeGood for clients to avoid 404 spam.
204-211: LGTM: explicit 404 on not foundNice and clear.
213-247: LGTM: version guard for archived games409 on git-commit mismatch is reasonable; includes details for clients.
249-262: LGTM: schema-validated singleplayer archiveGood validation and narrow JSON body.
Comment on lines
795
to
837
| private handleWinner(client: Client, clientMsg: ClientSendWinnerMessage) { | ||
| if ( | ||
| this.outOfSyncClients.has(client.clientID) || | ||
| this.kickedClients.has(client.clientID) || | ||
| this.winner !== null || | ||
| client.reportedWinner !== null | ||
| ) { | ||
| return; | ||
| } | ||
| client.reportedWinner = clientMsg.winner; | ||
|
|
||
| // Add client vote | ||
| const winnerKey = JSON.stringify(clientMsg.winner); | ||
| if (!this.winnerVotes.has(winnerKey)) { | ||
| this.winnerVotes.set(winnerKey, { ips: new Set(), winner: clientMsg }); | ||
| } | ||
| const potentialWinner = this.winnerVotes.get(winnerKey)!; | ||
| potentialWinner.ips.add(client.ip); | ||
|
|
||
| const activeUniqueIPs = new Set(this.activeClients.map((c) => c.ip)); | ||
|
|
||
| // Require at least two unique IPs to agree | ||
| if (activeUniqueIPs.size < 2) { | ||
| return; | ||
| } | ||
|
|
||
| // Check if winner has majority | ||
| if (potentialWinner.ips.size * 2 < activeUniqueIPs.size) { | ||
| return; | ||
| } | ||
|
|
||
| // Vote succeeded | ||
| this.winner = potentialWinner.winner; | ||
| this.log.info( | ||
| `Winner determined by ${potentialWinner.ips.size}/${activeUniqueIPs.size} active IPs`, | ||
| { | ||
| gameID: this.id, | ||
| winnerKey: winnerKey, | ||
| }, | ||
| ); | ||
| this.archiveGame(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Winner vote: require strict majority, canonicalize key, and end the game after archiving
- Use strict majority (>50%), not tie (currently accepts 50% on even counts).
- Canonicalize the winner key to avoid JSON key-order mismatches.
- After archiving, close the game to release resources immediately.
- const winnerKey = JSON.stringify(clientMsg.winner);
+ const winnerKey = stableStringify(clientMsg.winner);
@@
- if (potentialWinner.ips.size * 2 < activeUniqueIPs.size) {
+ if (potentialWinner.ips.size * 2 <= activeUniqueIPs.size) {
return;
}
@@
- this.archiveGame();
+ this.archiveGame();
+ // Cleanly finish the game session after winner consensus.
+ this.end();Add a stable stringify helper (top-level in this file):
function stableStringify(obj: unknown): string {
return JSON.stringify(
obj,
(_k, v) => {
if (v && typeof v === "object" && !Array.isArray(v)) {
return Object.fromEntries(
Object.entries(v as Record<string, unknown>).sort(([a], [b]) =>
a.localeCompare(b),
),
);
}
return v;
},
);
}Note: Using IPs is easy to game behind uncontrolled proxies. Consider:
- Counting unique persistentIDs (and optionally requiring IP diversity as a secondary check).
- Or trusting x-forwarded-for only when the connection is from known load balancer IPs.
🤖 Prompt for AI Agents
In src/server/GameServer.ts around lines 795–836, change winner vote logic to
require a strict majority, canonicalize the winner key with a stable stringify
helper, and close the game after archiving: add the provided
stableStringify(obj) helper at top-level of this file, replace
JSON.stringify(clientMsg.winner) with stableStringify(clientMsg.winner) when
computing winnerKey, change the majority check to reject ties (if
(potentialWinner.ips.size * 2 <= activeUniqueIPs.size) return;), and after
this.archiveGame() call invoke the game's close method (e.g., this.close()) to
release resources immediately.
Comment on lines
157
to
190
| app.post("/api/kick_player/:gameID/:clientID", async (req, res) => { | ||
| if (req.headers[config.adminHeader()] !== config.adminToken()) { | ||
| res.status(401).send("Unauthorized"); | ||
| return; | ||
| } | ||
|
|
||
| const { gameID, clientID } = req.params; | ||
| const { gameID, clientID } = req.params; | ||
|
|
||
| if (!ID.safeParse(gameID).success || !ID.safeParse(clientID).success) { | ||
| res.sendStatus(400); | ||
| return; | ||
| } | ||
| if (!ID.safeParse(gameID).success || !ID.safeParse(clientID).success) { | ||
| res.sendStatus(400); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const response = await fetch( | ||
| `http://localhost:${config.workerPort(gameID)}/api/kick_player/${gameID}/${clientID}`, | ||
| { | ||
| method: "POST", | ||
| headers: { | ||
| [config.adminHeader()]: config.adminToken(), | ||
| }, | ||
| try { | ||
| const response = await fetch( | ||
| `http://localhost:${config.workerPort(gameID)}/api/kick_player/${gameID}/${clientID}`, | ||
| { | ||
| method: "POST", | ||
| headers: { | ||
| [config.adminHeader()]: config.adminToken(), | ||
| }, | ||
| ); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`Failed to kick player: ${response.statusText}`); | ||
| } | ||
| }, | ||
| ); | ||
|
|
||
| res.status(200).send("Player kicked successfully"); | ||
| } catch (error) { | ||
| log.error(`Error kicking player from game ${gameID}:`, error); | ||
| res.status(500).send("Failed to kick player"); | ||
| if (!response.ok) { | ||
| throw new Error(`Failed to kick player: ${response.statusText}`); | ||
| } | ||
| }), | ||
| ); | ||
|
|
||
| res.status(200).send("Player kicked successfully"); | ||
| } catch (error) { | ||
| log.error(`Error kicking player from game ${gameID}:`, error); | ||
| res.status(500).send("Failed to kick player"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Harden internal proxy call (address SSRF warning), add timeout, validate responses
Even though IDs are validated and host is localhost, CodeQL flags this as SSRF. Bind the port to the computed worker index (not raw ID), and add a request timeout so this endpoint can’t hang.
app.post("/api/kick_player/:gameID/:clientID", async (req, res) => {
if (req.headers[config.adminHeader()] !== config.adminToken()) {
res.status(401).send("Unauthorized");
return;
}
const { gameID, clientID } = req.params;
if (!ID.safeParse(gameID).success || !ID.safeParse(clientID).success) {
res.sendStatus(400);
return;
}
try {
- const response = await fetch(
- `http://localhost:${config.workerPort(gameID)}/api/kick_player/${gameID}/${clientID}`,
- {
- method: "POST",
- headers: {
- [config.adminHeader()]: config.adminToken(),
- },
- },
- );
+ const controller = new AbortController();
+ const t = setTimeout(() => controller.abort(), 5000);
+ const workerIdx = config.workerIndex(gameID);
+ const port = config.workerPortByIndex(workerIdx);
+ const response = await fetch(
+ `http://localhost:${port}/api/kick_player/${gameID}/${clientID}`,
+ {
+ method: "POST",
+ headers: { [config.adminHeader()]: config.adminToken() },
+ signal: controller.signal,
+ },
+ ).finally(() => clearTimeout(t));
if (!response.ok) {
throw new Error(`Failed to kick player: ${response.statusText}`);
}
res.status(200).send("Player kicked successfully");
} catch (error) {
log.error(`Error kicking player from game ${gameID}:`, error);
res.status(500).send("Failed to kick player");
}
});Optional: if CodeQL still flags it, annotate the reason and document the invariants (localhost only, validated 8-char IDs, bounded worker port set).
🏁 Script executed:
#!/bin/bash
# Search TypeScript files for config worker port/index functions
rg -n "workerPort" -C3 --glob "*.ts" || true
rg -n "workerIndex" -C3 --glob "*.ts" || true
rg -n "workerPortByIndex" -C3 --glob "*.ts" || trueLength of output: 8709
Add timeout and document SSRF safety in kick_player endpoint
- In src/server/Master.ts (around the internal fetch at line 171), wrap the fetch in an AbortController with a short timeout (e.g. 5 seconds) and clear the timer after completion.
- Above that call, add a comment noting that the URL is safe (host=localhost, IDs validated via ID.safeParse, and config.workerPort(gameID) derives from a bounded workerIndex).
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 171-179: Server-side request forgery
The URL of this request depends on a user-provided value.
The URL of this request depends on a user-provided value.
🤖 Prompt for AI Agents
In src/server/Master.ts around lines 157 to 190, the internal fetch to the
worker lacks a timeout and missing documentation about why the URL is safe; wrap
the fetch call in an AbortController with a short timeout (e.g. 5000ms), pass
controller.signal to fetch, start a timer to call controller.abort() after
timeout and clear that timer in the finally/after-response path, and handle the
abort error as a normal fetch failure; also add a preceding comment that
documents SSRF safety: the target is localhost, gameID and clientID are
validated with ID.safeParse, and config.workerPort(gameID) is derived from a
bounded workerIndex so untrusted input cannot produce arbitrary hosts/ports.
Comment on lines
93
to
141
| app.post("/api/create_game/:id", async (req, res) => { | ||
| const id = req.params.id; | ||
| const creatorClientID = (() => { | ||
| if (typeof req.query.creatorClientID !== "string") return undefined; | ||
|
|
||
| const trimmed = req.query.creatorClientID.trim(); | ||
| return ID.safeParse(trimmed).success ? trimmed : undefined; | ||
| })(); | ||
| const trimmed = req.query.creatorClientID.trim(); | ||
| return ID.safeParse(trimmed).success ? trimmed : undefined; | ||
| })(); | ||
|
|
||
| if (!id) { | ||
| log.warn(`cannot create game, id not found`); | ||
| return res.status(400).json({ error: "Game ID is required" }); | ||
| } | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| const clientIP = req.ip || req.socket.remoteAddress || "unknown"; | ||
| const result = CreateGameInputSchema.safeParse(req.body); | ||
| if (!result.success) { | ||
| const error = z.prettifyError(result.error); | ||
| return res.status(400).json({ error }); | ||
| } | ||
| if (!id) { | ||
| log.warn(`cannot create game, id not found`); | ||
| return res.status(400).json({ error: "Game ID is required" }); | ||
| } | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| const clientIP = req.ip || req.socket.remoteAddress || "unknown"; | ||
| const result = CreateGameInputSchema.safeParse(req.body); | ||
| if (!result.success) { | ||
| const error = z.prettifyError(result.error); | ||
| return res.status(400).json({ error }); | ||
| } | ||
|
|
||
| const gc = result.data; | ||
| if ( | ||
| gc?.gameType === GameType.Public && | ||
| req.headers[config.adminHeader()] !== config.adminToken() | ||
| ) { | ||
| log.warn( | ||
| `cannot create public game ${id}, ip ${ipAnonymize(clientIP)} incorrect admin token`, | ||
| ); | ||
| return res.status(401).send("Unauthorized"); | ||
| } | ||
| const gc = result.data; | ||
| if ( | ||
| gc?.gameType === GameType.Public && | ||
| req.headers[config.adminHeader()] !== config.adminToken() | ||
| ) { | ||
| log.warn( | ||
| `cannot create public game ${id}, ip ${ipAnonymize(clientIP)} incorrect admin token`, | ||
| ); | ||
| return res.status(401).send("Unauthorized"); | ||
| } | ||
|
|
||
| // Double-check this worker should host this game | ||
| const expectedWorkerId = config.workerIndex(id); | ||
| if (expectedWorkerId !== workerId) { | ||
| log.warn( | ||
| `This game ${id} should be on worker ${expectedWorkerId}, but this is worker ${workerId}`, | ||
| ); | ||
| return res.status(400).json({ error: "Worker, game id mismatch" }); | ||
| } | ||
| // Double-check this worker should host this game | ||
| const expectedWorkerId = config.workerIndex(id); | ||
| if (expectedWorkerId !== workerId) { | ||
| log.warn( | ||
| `This game ${id} should be on worker ${expectedWorkerId}, but this is worker ${workerId}`, | ||
| ); | ||
| return res.status(400).json({ error: "Worker, game id mismatch" }); | ||
| } | ||
|
|
||
| // Pass creatorClientID to createGame | ||
| const game = gm.createGame(id, gc, creatorClientID); | ||
| // Pass creatorClientID to createGame | ||
| const game = gm.createGame(id, gc, creatorClientID); | ||
|
|
||
| log.info( | ||
| `Worker ${workerId}: IP ${ipAnonymize(clientIP)} creating ${game.isPublic() ? "Public" : "Private"}${gc?.gameMode ? ` ${gc.gameMode}` : ""} game with id ${id}${creatorClientID ? `, creator: ${creatorClientID}` : ""}`, | ||
| ); | ||
| res.json(game.gameInfo()); | ||
| }), | ||
| ); | ||
| log.info( | ||
| `Worker ${workerId}: IP ${ipAnonymize(clientIP)} creating ${game.isPublic() ? "Public" : "Private"}${gc?.gameMode ? ` ${gc.gameMode}` : ""} game with id ${id}${creatorClientID ? `, creator: ${creatorClientID}` : ""}`, | ||
| ); | ||
| res.json(game.gameInfo()); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate route param id and keep logs concise
Reject invalid IDs early; logs already anonymize IPs.
app.post("/api/create_game/:id", async (req, res) => {
- const id = req.params.id;
+ const id = req.params.id;
+ if (!ID.safeParse(id).success) {
+ return res.status(400).json({ error: "Invalid Game ID" });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| app.post("/api/create_game/:id", async (req, res) => { | |
| const id = req.params.id; | |
| const creatorClientID = (() => { | |
| if (typeof req.query.creatorClientID !== "string") return undefined; | |
| const trimmed = req.query.creatorClientID.trim(); | |
| return ID.safeParse(trimmed).success ? trimmed : undefined; | |
| })(); | |
| const trimmed = req.query.creatorClientID.trim(); | |
| return ID.safeParse(trimmed).success ? trimmed : undefined; | |
| })(); | |
| if (!id) { | |
| log.warn(`cannot create game, id not found`); | |
| return res.status(400).json({ error: "Game ID is required" }); | |
| } | |
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | |
| const clientIP = req.ip || req.socket.remoteAddress || "unknown"; | |
| const result = CreateGameInputSchema.safeParse(req.body); | |
| if (!result.success) { | |
| const error = z.prettifyError(result.error); | |
| return res.status(400).json({ error }); | |
| } | |
| if (!id) { | |
| log.warn(`cannot create game, id not found`); | |
| return res.status(400).json({ error: "Game ID is required" }); | |
| } | |
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | |
| const clientIP = req.ip || req.socket.remoteAddress || "unknown"; | |
| const result = CreateGameInputSchema.safeParse(req.body); | |
| if (!result.success) { | |
| const error = z.prettifyError(result.error); | |
| return res.status(400).json({ error }); | |
| } | |
| const gc = result.data; | |
| if ( | |
| gc?.gameType === GameType.Public && | |
| req.headers[config.adminHeader()] !== config.adminToken() | |
| ) { | |
| log.warn( | |
| `cannot create public game ${id}, ip ${ipAnonymize(clientIP)} incorrect admin token`, | |
| ); | |
| return res.status(401).send("Unauthorized"); | |
| } | |
| const gc = result.data; | |
| if ( | |
| gc?.gameType === GameType.Public && | |
| req.headers[config.adminHeader()] !== config.adminToken() | |
| ) { | |
| log.warn( | |
| `cannot create public game ${id}, ip ${ipAnonymize(clientIP)} incorrect admin token`, | |
| ); | |
| return res.status(401).send("Unauthorized"); | |
| } | |
| // Double-check this worker should host this game | |
| const expectedWorkerId = config.workerIndex(id); | |
| if (expectedWorkerId !== workerId) { | |
| log.warn( | |
| `This game ${id} should be on worker ${expectedWorkerId}, but this is worker ${workerId}`, | |
| ); | |
| return res.status(400).json({ error: "Worker, game id mismatch" }); | |
| } | |
| // Double-check this worker should host this game | |
| const expectedWorkerId = config.workerIndex(id); | |
| if (expectedWorkerId !== workerId) { | |
| log.warn( | |
| `This game ${id} should be on worker ${expectedWorkerId}, but this is worker ${workerId}`, | |
| ); | |
| return res.status(400).json({ error: "Worker, game id mismatch" }); | |
| } | |
| // Pass creatorClientID to createGame | |
| const game = gm.createGame(id, gc, creatorClientID); | |
| // Pass creatorClientID to createGame | |
| const game = gm.createGame(id, gc, creatorClientID); | |
| log.info( | |
| `Worker ${workerId}: IP ${ipAnonymize(clientIP)} creating ${game.isPublic() ? "Public" : "Private"}${gc?.gameMode ? ` ${gc.gameMode}` : ""} game with id ${id}${creatorClientID ? `, creator: ${creatorClientID}` : ""}`, | |
| ); | |
| res.json(game.gameInfo()); | |
| }), | |
| ); | |
| log.info( | |
| `Worker ${workerId}: IP ${ipAnonymize(clientIP)} creating ${game.isPublic() ? "Public" : "Private"}${gc?.gameMode ? ` ${gc.gameMode}` : ""} game with id ${id}${creatorClientID ? `, creator: ${creatorClientID}` : ""}`, | |
| ); | |
| res.json(game.gameInfo()); | |
| }); | |
| app.post("/api/create_game/:id", async (req, res) => { | |
| const id = req.params.id; | |
| if (!ID.safeParse(id).success) { | |
| return res.status(400).json({ error: "Invalid Game ID" }); | |
| } | |
| const creatorClientID = (() => { | |
| if (typeof req.query.creatorClientID !== "string") return undefined; | |
| const trimmed = req.query.creatorClientID.trim(); | |
| return ID.safeParse(trimmed).success ? trimmed : undefined; | |
| })(); | |
| if (!id) { | |
| log.warn(`cannot create game, id not found`); | |
| return res.status(400).json({ error: "Game ID is required" }); | |
| } | |
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | |
| const clientIP = req.ip || req.socket.remoteAddress || "unknown"; | |
| const result = CreateGameInputSchema.safeParse(req.body); | |
| if (!result.success) { | |
| const error = z.prettifyError(result.error); | |
| return res.status(400).json({ error }); | |
| } | |
| const gc = result.data; | |
| if ( | |
| gc?.gameType === GameType.Public && | |
| req.headers[config.adminHeader()] !== config.adminToken() | |
| ) { | |
| log.warn( | |
| `cannot create public game ${id}, ip ${ipAnonymize(clientIP)} incorrect admin token`, | |
| ); | |
| return res.status(401).send("Unauthorized"); | |
| } | |
| // Double-check this worker should host this game | |
| const expectedWorkerId = config.workerIndex(id); | |
| if (expectedWorkerId !== workerId) { | |
| log.warn( | |
| `This game ${id} should be on worker ${expectedWorkerId}, but this is worker ${workerId}`, | |
| ); | |
| return res.status(400).json({ error: "Worker, game id mismatch" }); | |
| } | |
| // Pass creatorClientID to createGame | |
| const game = gm.createGame(id, gc, creatorClientID); | |
| log.info( | |
| `Worker ${workerId}: IP ${ipAnonymize(clientIP)} creating ${game.isPublic() ? "Public" : "Private"}${gc?.gameMode ? ` ${gc.gameMode}` : ""} game with id ${id}${creatorClientID ? `, creator: ${creatorClientID}` : ""}`, | |
| ); | |
| res.json(game.gameInfo()); | |
| }); |
🤖 Prompt for AI Agents
In src/server/Worker.ts around lines 93 to 141, the route param id is not
validated and logs are verbose; trim and validate the id early using the same
ID.safeParse pattern (e.g., const idTrimmed = String(req.params.id).trim(); if
(!ID.safeParse(idTrimmed).success) return res.status(400).json({ error: "Invalid
game ID" });), then use idTrimmed everywhere (including workerIndex check and
createGame); simplify logs to a single concise message that uses
ipAnonymize(clientIP) once and avoids repeating details (e.g., "Worker X:
creating Public/Private game id Y" and append creator id only if present).
Ensure any error responses occur after validation so subsequent logic always
operates on a validated id.
Comment on lines
143
to
160
| // Add other endpoints from your original server | ||
| app.post( | ||
| "/api/start_game/:id", | ||
| gatekeeper.httpHandler(LimiterType.Post, async (req, res) => { | ||
| log.info(`starting private lobby with id ${req.params.id}`); | ||
| const game = gm.game(req.params.id); | ||
| if (!game) { | ||
| return; | ||
| } | ||
| if (game.isPublic()) { | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| const clientIP = req.ip || req.socket.remoteAddress || "unknown"; | ||
| log.info( | ||
| `cannot start public game ${game.id}, game is public, ip: ${ipAnonymize(clientIP)}`, | ||
| ); | ||
| return; | ||
| } | ||
| game.start(); | ||
| res.status(200).json({ success: true }); | ||
| }), | ||
| ); | ||
|
|
||
| app.put( | ||
| "/api/game/:id", | ||
| gatekeeper.httpHandler(LimiterType.Put, async (req, res) => { | ||
| const result = GameInputSchema.safeParse(req.body); | ||
| if (!result.success) { | ||
| const error = z.prettifyError(result.error); | ||
| return res.status(400).json({ error }); | ||
| } | ||
| const config = result.data; | ||
| // TODO: only update public game if from local host | ||
| const lobbyID = req.params.id; | ||
| if (config.gameType === GameType.Public) { | ||
| log.info(`cannot update game ${lobbyID} to public`); | ||
| return res.status(400).json({ error: "Cannot update public game" }); | ||
| } | ||
| const game = gm.game(lobbyID); | ||
| if (!game) { | ||
| return res.status(400).json({ error: "Game not found" }); | ||
| } | ||
| if (game.isPublic()) { | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| const clientIP = req.ip || req.socket.remoteAddress || "unknown"; | ||
| log.warn( | ||
| `cannot update public game ${game.id}, ip: ${ipAnonymize(clientIP)}`, | ||
| ); | ||
| return res.status(400).json({ error: "Cannot update public game" }); | ||
| } | ||
| if (game.hasStarted()) { | ||
| log.warn(`cannot update game ${game.id} after it has started`); | ||
| return res | ||
| .status(400) | ||
| .json({ error: "Cannot update game after it has started" }); | ||
| } | ||
| game.updateGameConfig(config); | ||
| res.status(200).json({ success: true }); | ||
| }), | ||
| ); | ||
| app.post("/api/start_game/:id", async (req, res) => { | ||
| log.info(`starting private lobby with id ${req.params.id}`); | ||
| const game = gm.game(req.params.id); | ||
| if (!game) { | ||
| return; | ||
| } | ||
| if (game.isPublic()) { | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| const clientIP = req.ip || req.socket.remoteAddress || "unknown"; | ||
| log.info( | ||
| `cannot start public game ${game.id}, game is public, ip: ${ipAnonymize(clientIP)}`, | ||
| ); | ||
| return; | ||
| } | ||
| game.start(); | ||
| res.status(200).json({ success: true }); | ||
| }); |
There was a problem hiding this comment.
Don’t return without a response; send proper 4xx codes
Two early returns produce hung HTTP connections.
app.post("/api/start_game/:id", async (req, res) => {
log.info(`starting private lobby with id ${req.params.id}`);
const game = gm.game(req.params.id);
if (!game) {
- return;
+ return res.status(404).json({ error: "Game not found" });
}
if (game.isPublic()) {
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
const clientIP = req.ip || req.socket.remoteAddress || "unknown";
log.info(
`cannot start public game ${game.id}, game is public, ip: ${ipAnonymize(clientIP)}`,
);
- return;
+ return res.status(400).json({ error: "Cannot start a public game" });
}
game.start();
res.status(200).json({ success: true });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Add other endpoints from your original server | |
| app.post( | |
| "/api/start_game/:id", | |
| gatekeeper.httpHandler(LimiterType.Post, async (req, res) => { | |
| log.info(`starting private lobby with id ${req.params.id}`); | |
| const game = gm.game(req.params.id); | |
| if (!game) { | |
| return; | |
| } | |
| if (game.isPublic()) { | |
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | |
| const clientIP = req.ip || req.socket.remoteAddress || "unknown"; | |
| log.info( | |
| `cannot start public game ${game.id}, game is public, ip: ${ipAnonymize(clientIP)}`, | |
| ); | |
| return; | |
| } | |
| game.start(); | |
| res.status(200).json({ success: true }); | |
| }), | |
| ); | |
| app.put( | |
| "/api/game/:id", | |
| gatekeeper.httpHandler(LimiterType.Put, async (req, res) => { | |
| const result = GameInputSchema.safeParse(req.body); | |
| if (!result.success) { | |
| const error = z.prettifyError(result.error); | |
| return res.status(400).json({ error }); | |
| } | |
| const config = result.data; | |
| // TODO: only update public game if from local host | |
| const lobbyID = req.params.id; | |
| if (config.gameType === GameType.Public) { | |
| log.info(`cannot update game ${lobbyID} to public`); | |
| return res.status(400).json({ error: "Cannot update public game" }); | |
| } | |
| const game = gm.game(lobbyID); | |
| if (!game) { | |
| return res.status(400).json({ error: "Game not found" }); | |
| } | |
| if (game.isPublic()) { | |
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | |
| const clientIP = req.ip || req.socket.remoteAddress || "unknown"; | |
| log.warn( | |
| `cannot update public game ${game.id}, ip: ${ipAnonymize(clientIP)}`, | |
| ); | |
| return res.status(400).json({ error: "Cannot update public game" }); | |
| } | |
| if (game.hasStarted()) { | |
| log.warn(`cannot update game ${game.id} after it has started`); | |
| return res | |
| .status(400) | |
| .json({ error: "Cannot update game after it has started" }); | |
| } | |
| game.updateGameConfig(config); | |
| res.status(200).json({ success: true }); | |
| }), | |
| ); | |
| app.post("/api/start_game/:id", async (req, res) => { | |
| log.info(`starting private lobby with id ${req.params.id}`); | |
| const game = gm.game(req.params.id); | |
| if (!game) { | |
| return; | |
| } | |
| if (game.isPublic()) { | |
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | |
| const clientIP = req.ip || req.socket.remoteAddress || "unknown"; | |
| log.info( | |
| `cannot start public game ${game.id}, game is public, ip: ${ipAnonymize(clientIP)}`, | |
| ); | |
| return; | |
| } | |
| game.start(); | |
| res.status(200).json({ success: true }); | |
| }); | |
| // Add other endpoints from your original server | |
| app.post("/api/start_game/:id", async (req, res) => { | |
| log.info(`starting private lobby with id ${req.params.id}`); | |
| const game = gm.game(req.params.id); | |
| if (!game) { | |
| return res.status(404).json({ error: "Game not found" }); | |
| } | |
| if (game.isPublic()) { | |
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | |
| const clientIP = req.ip || req.socket.remoteAddress || "unknown"; | |
| log.info( | |
| `cannot start public game ${game.id}, game is public, ip: ${ipAnonymize(clientIP)}`, | |
| ); | |
| return res.status(400).json({ error: "Cannot start a public game" }); | |
| } | |
| game.start(); | |
| res.status(200).json({ success: true }); | |
| }); |
🤖 Prompt for AI Agents
In src/server/Worker.ts around lines 143 to 160, the handler currently returns
early without sending an HTTP response when the game is not found or when the
game is public, causing hung connections; change the first early return to
res.status(404).json({ error: "game not found" }) and change the public-game
branch to respond with a 403 (or 409) e.g. res.status(403).json({ error: "cannot
start public game" }) while keeping the existing log statements and exiting the
handler after sending the response.
Comment on lines
264
to
280
| app.post("/api/kick_player/:gameID/:clientID", async (req, res) => { | ||
| if (req.headers[config.adminHeader()] !== config.adminToken()) { | ||
| res.status(401).send("Unauthorized"); | ||
| return; | ||
| } | ||
|
|
||
| const game = gm.game(gameID); | ||
| if (!game) { | ||
| res.status(404).send("Game not found"); | ||
| return; | ||
| } | ||
| const { gameID, clientID } = req.params; | ||
|
|
||
| game.kickClient(clientID); | ||
| res.status(200).send("Player kicked successfully"); | ||
| }), | ||
| ); | ||
| const game = gm.game(gameID); | ||
| if (!game) { | ||
| res.status(404).send("Game not found"); | ||
| return; | ||
| } | ||
|
|
||
| game.kickClient(clientID); | ||
| res.status(200).send("Player kicked successfully"); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate IDs on kick endpoint too
Mirror the master-side checks to avoid accepting malformed IDs directly on the worker.
app.post("/api/kick_player/:gameID/:clientID", async (req, res) => {
if (req.headers[config.adminHeader()] !== config.adminToken()) {
res.status(401).send("Unauthorized");
return;
}
const { gameID, clientID } = req.params;
+ if (!ID.safeParse(gameID).success || !ID.safeParse(clientID).success) {
+ return res.status(400).send("Invalid IDs");
+ }
const game = gm.game(gameID);
if (!game) {
res.status(404).send("Game not found");
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| app.post("/api/kick_player/:gameID/:clientID", async (req, res) => { | |
| if (req.headers[config.adminHeader()] !== config.adminToken()) { | |
| res.status(401).send("Unauthorized"); | |
| return; | |
| } | |
| const game = gm.game(gameID); | |
| if (!game) { | |
| res.status(404).send("Game not found"); | |
| return; | |
| } | |
| const { gameID, clientID } = req.params; | |
| game.kickClient(clientID); | |
| res.status(200).send("Player kicked successfully"); | |
| }), | |
| ); | |
| const game = gm.game(gameID); | |
| if (!game) { | |
| res.status(404).send("Game not found"); | |
| return; | |
| } | |
| game.kickClient(clientID); | |
| res.status(200).send("Player kicked successfully"); | |
| }); | |
| app.post("/api/kick_player/:gameID/:clientID", async (req, res) => { | |
| if (req.headers[config.adminHeader()] !== config.adminToken()) { | |
| res.status(401).send("Unauthorized"); | |
| return; | |
| } | |
| const { gameID, clientID } = req.params; | |
| if (!ID.safeParse(gameID).success || !ID.safeParse(clientID).success) { | |
| return res.status(400).send("Invalid IDs"); | |
| } | |
| const game = gm.game(gameID); | |
| if (!game) { | |
| res.status(404).send("Game not found"); | |
| return; | |
| } | |
| game.kickClient(clientID); | |
| res.status(200).send("Player kicked successfully"); | |
| }); |
🤖 Prompt for AI Agents
In src/server/Worker.ts around lines 264 to 280, the kick endpoint accepts raw
gameID and clientID without validating their format; add the same ID validation
used on the master side (reuse the existing
validateId/validateGameId/validateClientId helper if available) to check both
gameID and clientID before proceeding, return 400 Bad Request for malformed IDs,
then continue to lookup the game and verify the client belongs to that game
(return 404 if game or client not found) before calling game.kickClient and
sending the 200 response.
evanpelle
force-pushed
the
vote-for-winner
branch
from
c941a11 to
0cfe581
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/server/GameServer.ts (1)
795-838: Winner vote: strict majority, stable key, better logs, and end after archiveApply:
- const winnerKey = JSON.stringify(clientMsg.winner); + const winnerKey = stableStringify(clientMsg.winner); @@ - const ratio = `${potentialWinner.ips.size}/${activeUniqueIPs.size}`; + const ratio = `${potentialWinner.ips.size}/${activeUniqueIPs.size}`; this.log.info( - `recieved winner vote ${clientMsg.winner}, ${ratio} votes for this winner`, + `received winner vote ${JSON.stringify(clientMsg.winner)}, ${ratio} votes for this winner`, { clientID: client.clientID, }, ); @@ - if (potentialWinner.ips.size * 2 < activeUniqueIPs.size) { + // Require strict majority; ties do not pass. + if (potentialWinner.ips.size * 2 <= activeUniqueIPs.size) { return; } @@ - this.archiveGame(); + this.archiveGame(); + // Close sockets and free resources after archiving. + this.end();Add helper (top-level in this file):
function stableStringify(obj: unknown): string { return JSON.stringify( obj, (_k, v) => { if (v && typeof v === "object" && !Array.isArray(v)) { return Object.fromEntries( Object.entries(v as Record<string, unknown>).sort(([a], [b]) => a.localeCompare(b), ), ); } return v; }, ); }Optional: count unique persistentIDs for quorum (harder to game), and optionally also require IP diversity as a secondary check.
🧹 Nitpick comments (3)
src/server/GameServer.ts (3)
67-71: Winner votes map: clear it when the game endsAvoid retained memory across games; clear votes after archiving/ending.
Outside the selected range, add this at the end of end():
// after archiving/closing sockets this.winnerVotes.clear();
296-300: Unknown message warningReasonable default. Consider sampling if this gets noisy under abuse.
303-309: Typo and severity for error logUse error level and fix spelling.
- this.log.info( - `error handline websocket request in game server: ${error}`, + this.log.error( + `error handling websocket request in game server: ${String(error)}`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/server/Client.ts(1 hunks)src/server/GameServer.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/Client.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/server/GameServer.ts (2)
src/core/Schemas.ts (3)
ClientSendWinnerMessage(100-100)ClientMessageSchema(481-488)ServerErrorMessage(99-99)src/server/Client.ts (1)
Client(6-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (5)
src/server/GameServer.ts (5)
192-192: Good: preserve reportedWinner on reconnectKeeps vote idempotent across reconnects.
241-274: Kick intent: scope to lobby creator and prevent self-kickLooks correct and readable.
283-286: Ping handling is fineUpdates both server and client timestamps.
288-290: Hash tracking is fineStores per-turn hash as expected.
291-294: Winner routing OKDelegates to handleWinner; idempotency enforced there.
Comment on lines
206
to
224
| client.ws.on("message", async (message: string) => { | ||
| try { | ||
| const parsed = ClientMessageSchema.safeParse(JSON.parse(message)); | ||
| if (!parsed.success) { | ||
| const error = z.prettifyError(parsed.error); | ||
| this.log.error("Failed to parse client message", error, { | ||
| clientID: client.clientID, | ||
| }); | ||
| client.ws.send( | ||
| JSON.stringify({ | ||
| type: "error", | ||
| error, | ||
| message, | ||
| } satisfies ServerErrorMessage), | ||
| ); | ||
| client.ws.close(1002, "ClientMessageSchema"); | ||
| return; | ||
| } | ||
| const clientMsg = parsed.data; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Handle ws RawData correctly and avoid non-standard z.prettifyError
ws passes RawData, not string. Also, Zod has no prettifyError; use flatten().
Apply:
- client.ws.on("message", async (message: string) => {
+ client.ws.on("message", async (data: WebSocket.RawData) => {
+ const text = typeof data === "string" ? data : data.toString("utf8");
try {
- const parsed = ClientMessageSchema.safeParse(JSON.parse(message));
+ const parsed = ClientMessageSchema.safeParse(JSON.parse(text));
if (!parsed.success) {
- const error = z.prettifyError(parsed.error);
+ const error = parsed.error.flatten();
this.log.error("Failed to parse client message", error, {
clientID: client.clientID,
});
client.ws.send(
JSON.stringify({
type: "error",
error,
- message,
+ message: text,
} satisfies ServerErrorMessage),
);
client.ws.close(1002, "ClientMessageSchema");
return;
}To check for other uses of prettifyError:
🏁 Script executed:
#!/bin/bash
rg -nP 'prettifyError\s*\(' --type=ts -C2Length of output: 4509
Replace z.prettifyError with error.flatten() and convert WebSocket.RawData to string
- In GameServer.ts, change the ws “message” handler to accept
data: WebSocket.RawData, convert it to UTF-8 text, and then parse and flatten errors. For example:client.ws.on("message", async (data: WebSocket.RawData) => {
-
const text = typeof data === "string" ? data : data.toString("utf8"); try {
-
const parsed = ClientMessageSchema.safeParse(JSON.parse(message));
-
const parsed = ClientMessageSchema.safeParse(JSON.parse(text)); if (!parsed.success) {
-
const error = z.prettifyError(parsed.error);
-
const error = parsed.error.flatten(); this.log.error("Failed to parse client message", error, { clientID: client.clientID, }); client.ws.send( JSON.stringify({ type: "error", error,
-
message,
-
message: text, } satisfies ServerErrorMessage), ); client.ws.close(1002, "ClientMessageSchema"); return; }
- Apply the same replacement for every
z.prettifyError(...)across the codebase:and replace each withrg -nP 'prettifyError\s*\(' --type=ts -C2parsedError.flatten()(or similar) and, where applicable, send the converted text instead of raw data.
🤖 Prompt for AI Agents
In src/server/GameServer.ts around lines 206-224, update the WebSocket message
handler to accept data: WebSocket.RawData, convert it to a UTF-8 string before
JSON.parse, and use parsed.error.flatten() instead of
z.prettifyError(parsed.error); send the converted string in the error payload
and close as before. Also search the codebase for other uses of
z.prettifyError(...) and replace each with the corresponding ZodError.flatten()
call (and ensure any messages built from raw WebSocket data are first converted
to UTF-8 strings before sending/parsing).
evanpelle
force-pushed
the
vote-for-winner
branch
from
0cfe581 to
6f5e709
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/server/GameServer.ts (5)
291-294: Delegation looks good; keep message handler fix in mindRouting to
handleWinneris clean. Note: the ws “message” handler still typesmessage: stringand usesz.prettifyErrorelsewhere; see related comment with a concrete fix.
205-224: Handle WebSocket RawData and replace non-standardz.prettifyError
wsemitsRawData, notstring. Zod hasflatten(), notprettifyError.Apply:
- client.ws.on("message", async (message: string) => { + client.ws.on("message", async (data: WebSocket.RawData) => { + const message = typeof data === "string" ? data : data.toString("utf8"); try { const parsed = ClientMessageSchema.safeParse(JSON.parse(message)); if (!parsed.success) { - const error = z.prettifyError(parsed.error); + const error = parsed.error.flatten();
807-813: Canonicalize key and enforce one-vote-per-IP
- Use a stable stringify to avoid object key-order issues.
- Move an IP’s vote if they switch winners; don’t count the same IP in multiple buckets.
Apply:
- const winnerKey = JSON.stringify(clientMsg.winner); + const winnerKey = stableStringify(clientMsg.winner); @@ - const potentialWinner = this.winnerVotes.get(winnerKey)!; - potentialWinner.ips.add(client.ip); + const potentialWinner = this.winnerVotes.get(winnerKey)!; + const prevKey = this.ipVotes.get(client.ip); + if (prevKey && prevKey !== winnerKey) { + const prev = this.winnerVotes.get(prevKey); + prev?.ips.delete(client.ip); + } + this.ipVotes.set(client.ip, winnerKey); + potentialWinner.ips.add(client.ip);Add the helper (top-level in this file):
function stableStringify(obj: unknown): string { return JSON.stringify( obj, (_k, v) => { if (v && typeof v === "object" && !Array.isArray(v)) { return Object.fromEntries( Object.entries(v as Record<string, unknown>).sort(([a], [b]) => a.localeCompare(b), ), ); } return v; }, ); }
824-827: Require strict majority (>50%), not tieCurrent check accepts ties on even counts. Use strict majority.
Apply:
- if (potentialWinner.ips.size * 2 < activeUniqueIPs.size) { + if (potentialWinner.ips.size * 2 <= activeUniqueIPs.size) { return; }
828-837: End the game after archiving to release resourcesAfter
archiveGame(), close sockets and stop the turn loop.Apply:
this.archiveGame(); + this.end();
🧹 Nitpick comments (1)
src/server/GameServer.ts (1)
816-823: Fix log spelling and structure
- “recieved” → “received”.
- Avoid interpolating objects in template strings; use structured fields.
Apply:
- const ratio = `${potentialWinner.ips.size}/${activeUniqueIPs.size}`; - this.log.info( - `recieved winner vote ${clientMsg.winner}, ${ratio} votes for this winner`, - { - clientID: client.clientID, - }, - ); + const ratio = `${potentialWinner.ips.size}/${activeUniqueIPs.size}`; + this.log.info("received winner vote", { + clientID: client.clientID, + winnerKey, + ratio, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/server/Client.ts(1 hunks)src/server/GameServer.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/Client.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#860
File: src/server/GameServer.ts:50-53
Timestamp: 2025-05-24T20:45:07.040Z
Learning: In the GameServer voting system, only human players can vote - bots will not vote. This means the WinnerKey type only needs to handle player votes, not bot votes.
🧬 Code graph analysis (1)
src/server/GameServer.ts (2)
src/core/Schemas.ts (1)
ClientSendWinnerMessage(100-100)src/server/Client.ts (1)
Client(6-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (3)
src/server/GameServer.ts (3)
192-193: Good: preserve vote on reconnectCarrying over
reportedWinnerprevents duplicate votes after reconnect.
795-803: Guards look correctEarly returns for out-of-sync, kicked, already-has-winner, and already-reported are appropriate.
814-816: Drop bot filtering:activeClientsholds only human connections
NoisBot/botflag exists onClient, and bots aren’t part ofactiveClients(they have no IP), so the current ratio already counts only human voters.Likely an incorrect or invalid review comment.
Comment on lines
+67
to
+71
| private winnerVotes: Map< | ||
| string, | ||
| { winner: ClientSendWinnerMessage; ips: Set<string> } | ||
| > = new Map(); | ||
|
|
There was a problem hiding this comment.
One-vote-per-IP is not enforced across competing winners
An IP can appear in multiple winner buckets if different clients from the same IP vote for different winners. This can produce conflicting “majorities” depending on arrival order.
Apply this to track and move an IP’s vote between winners:
private winnerVotes: Map<
string,
{ winner: ClientSendWinnerMessage; ips: Set<string> }
> = new Map();
+ // Ensure one vote per IP across all winners.
+ private ipVotes: Map<string, string> = new Map();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private winnerVotes: Map< | |
| string, | |
| { winner: ClientSendWinnerMessage; ips: Set<string> } | |
| > = new Map(); | |
| private winnerVotes: Map< | |
| string, | |
| { winner: ClientSendWinnerMessage; ips: Set<string> } | |
| > = new Map(); | |
| // Ensure one vote per IP across all winners. | |
| private ipVotes: Map<string, string> = new Map(); |
🤖 Prompt for AI Agents
In src/server/GameServer.ts around lines 67-71, the current winnerVotes Map
allows the same IP to be present in multiple winner buckets, so implement a
global IP-to-winner lookup and update logic: maintain a Map<string, string>
ipToWinner that maps each IP to the winner ID; when processing a new vote, check
ipToWinner for that IP — if it points to a different winner, remove the IP from
the previous winner's ips Set and update that winner's data, then add the IP to
the new winner's ips Set and update ipToWinner to the new winner; if the vote is
duplicate for the same winner, ignore; ensure entries for winners are
created/cleaned up when their ips Sets become non-empty/empty.
This was referenced Sep 9, 2025
4 tasks
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.
Description:
Require majority of ips to report a winner.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan